< bitcoin-git>
[bitcoin] fanquake closed pull request #20251: Move major version to first version integer as specified by SemVer (master...semver2) https://github.com/bitcoin/bitcoin/pull/20251
< wumpus>
not a very important concern but I guess if we have these type specifications and don't check them they'll be out of whack complately in no time
< fanquake>
wumpus: yea I added most of the annotations, but we never hooked up a linter. Probably worth it now that you're writing pixie from scratch
< wumpus>
I've tried to change the linter to mypy all .py files it gives a weird error about a duplicate module 'test_runner' (which should be allowed they're in different directories), so I see why it's only enabled for specific paths
< wumpus>
gah adding type hints to pixie turns out to kind of annoying as all the structures are dynamically generated, so you get all kinds of errors like error: "Symbol" has no attribute "st_name"
< wumpus>
and adding manual hints for all of those would make the code three times longer or so, which feels like fighting python instead of using it
< fanquake>
Oh nooo
< wumpus>
I'm thinking of something quite evil: are the __annotations__ ordered? if so, I could do it the other way around and generate the Structs() from them by defining the various Elf integer types, this would even make the code more similar to elf.h
< wumpus>
nah don't know if I'm actually gonig to do it, I think the code is fine as-is, could just add a workaround to ElfRecord for mypy to ignore it
< wumpus>
inheriting from types.SimpleNamespace works too
< wumpus>
much less ugly
< wumpus>
and you get a sensible __repr__ for free, yippie
< wumpus>
pushed the mypy annotations; modern python is pretty neat, I may still do the annotation-based structure generation but we already went a long way from parsing the humongous elfread output to this, I think what would be nice is tests that it works for the range of architectures we support, although the guix/gitian run will help at least a bit there
< fanquake>
wumpus: sounds good. Looking forward to having a review
< fanquake>
This new code should be must nicer going forward
< wumpus>
fanquake: thanks!
< wumpus>
oh apparently there is typing.Annotated / PEP 593 to do exactly what I wanted to do (e.g. for annotate fields as struct types, it even has a hypothetical 'struct2' as exmple), buuut it's python 3.9+ 🤦
< wumpus>
I'll leave it for a future TODO
< wumpus>
let's see if I can create a lxc environment with alll the cross compilers and create some testing for the symbols/versions detection
< bitcoin-git>
[bitcoin] fanquake opened pull request #20440: build: fix determinism issue when building qt with Clang 8 (master...no_echo_fix_clang_qt_determinism) https://github.com/bitcoin/bitcoin/pull/20440
< fanquake>
That's enough Qt for one Saturday. Good chance we wont even use that patch heh
< wumpus>
nice! yeah I still think the most effortless and straightforward "fix" seems be to bite the bullet and use llvm 9, but, dunno what the risks of being out of whack with apple's compiler entail really
< fanquake>
Yea, that's probably what we'll end up doing for 22.0. Don't think we want to be swapping compilers last minute pre-release though.
< wumpus>
well it's rc1 :)
< fanquake>
the first of many perhaps
< wumpus>
that's what I mean, we don't actually have a release yet, rc1 can be considered non-existent so rc2 is rc1
< wumpus>
effectively
< achow101>
fanquake: I have a third solution. I have a patch to qt_intersect_spans that doesn't trigger the bug in llvm. I did 40 or so gitian builds overnight and they all get the same result. https://github.com/achow101/bitcoin/tree/0.21-qpainter-fix
< bitcoin-git>
[bitcoin] achow101 opened pull request #20447: depends: Patch qt_intersect_spans to avoid non-deterministic behavior in LLVM 8 (master...0.21-qpainter-fix) https://github.com/bitcoin/bitcoin/pull/20447
< luke-jr>
can't we patch or wrap LLVM instead? what's to stop us from accidentally hitting this in our own code in 22.0?
< luke-jr>
or do we expect LLVM to backport a fix by then?
< achow101>
luke-jr: I think the plan is to bump to LLVM 9 which fixes this problem
< luke-jr>
i c
< luke-jr>
I guess that makes sense - probbly need it for macOS ARM too
< jonatack>
wait, are we all supposed to submit a qt proposal? i missed the memo :D
< jonatack>
(great work though)
< wumpus>
gah ubuntu bionic gcc-8 with -Wl,-z,separate-code seems to make a printf hello world program about 4MB (from 8kB without), did we notice any size ballooning for bitcoin? (no such problem for gcc 9.3 on focal)
< wumpus>
looks like it aligns the separated sections to increments of 2MB (and actually stores the zeroes) instead of the page size, only for x86_64 though not other architectures
< wumpus>
bitcoind x86_64-linux-gnu went from 9.9M to 15M from 0.20.1 to 0.21.0rc1 ... I doubt this is actual code growth, will check another arch just to be sure
< wumpus>
for comparison: aarch64 went from 9.1M to 11M in the same span
< wumpus>
doesn't seem really worrying anyway but I just noticed
< wumpus>
it's more visible, relatively, for bitcoin-cli which went from 2.8M to 4.6M for x86_64 and 1.8M to 1.9M for aarch64
< wumpus>
at least it's zeroes so it compresses very well in the tar.gz :-)
< jonatack>
bitcoin-cli did see additions of -netinfo and -generate, but wow o_O
< wumpus>
I don't think it matters for memory usage in practice, paging operating systems are never going to read the empty, unreferenced pages; the virtual memory usage doesn't matter on 64 bit; it's just more disk space
< * jonatack>
thinks back to when software was delivered with constraints of 4/16/48 KB of RAM
< wumpus>
heh the times entire games fit into that
< jonatack>
exactly
< wumpus>
and to realize that on average, compilers became *better* at optimizing since then
< luke-jr>
abstractions tend to break those optimisations tho
< wumpus>
right
< luke-jr>
I'm confused about unloadwallet's intended behaviour. It looks broken.
< luke-jr>
it appears that if the RPC request specifies a wallet, AND a wallet is named in the param, it errors even if those two match?
< luke-jr>
I can see erroring if they differ, but even then the error message doesn't make sense for that scenario
< luke-jr>
and the docs don't help
< jonatack>
Hm, with unloadwallet I see the log printing "Releasing wallet"... but then no confirmation when the operation is completed, nor if it was successful.
< jonatack>
and the rpc result object only says "warning": "". i guess this can be improved to be more reassuring for users with funds.
< luke-jr>
hrm, probabyl warning should be null or absent
< jonatack>
i'd not show a warning field unless there was a warning to display, and otherwise print "result": "Wallet xyz was successfully unloaded."
< jonatack>
like with the upgradewallet fixes PR tagged for 0.21
< jonatack>
it's easy to add, if you're fixing things...happy to review if you do
< jonatack>
(i agree with you)
< jonatack>
even better, a "wallet_name": string field and a "result": "Wallet successfully unloaded.", see #20403