< 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
< bitcoin-git> [bitcoin] MarcoFalke pushed 4 commits to master: https://github.com/bitcoin/bitcoin/compare/d4159984c360...816132e6eb23
< bitcoin-git> bitcoin/master 1b3d700 Jon Atack: Allow zero-fee fundrawtxn and walletcreatefundedpsbt calls
< bitcoin-git> bitcoin/master 3f1e10b Jon Atack: Update feeRate (BTC/kvB) to fee_rate (sat/vB) in wallet_bumpfee
< bitcoin-git> bitcoin/master 9f08780 Jon Atack: Use the correct incremental fee constant in bumpfee help
< bitcoin-git> [bitcoin] MarcoFalke merged pull request #20426: wallet: allow zero-fee fundrawtransaction/walletcreatefundedpsbt and other fixes (master...fee_rate_followups) https://github.com/bitcoin/bitcoin/pull/20426
< fanquake> achow101: if you wanted to run a test build, I have a branch here that seems to achieve the same thing as 20436, but maybe slightly less "nasty": https://github.com/fanquake/bitcoin/commits/no_echo_fix_clang_qt_determinism_backported
< wumpus> O3 is the default?
< wumpus> the highest optimization level also has the highest risk of exposing compiler issues, wouldn't mind turning down the whole thing to O2
< fanquake> wumpus: I haven’t looked at the qt code, but yes apparently so.
< wumpus> fanquake: but making it macos scoped is a good idea at least I guess
< wumpus> it looks like mypy is not running on the python scripts in contrib/devtools in the CI
< wumpus> my glaring mistake in read_symbols in #20434 just passed (I've tried with mypy locally and did complain there)
< gribble> https://github.com/bitcoin/bitcoin/issues/20434 | contrib: Parse ELF directly for symbol and security checks by laanwj · Pull Request #20434 · bitcoin/bitcoin · GitHub
< 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> aha it needs to be run per package you can't just run it over the whole tree https://github.com/python/mypy/issues/4008
< wumpus> fair enough I'll just add contrib/devtools though there are some more things that need to be fixed before it passes (not caused by my PR)
< wumpus> and I don't even have type annotations in pixie itself yet
< fanquake> I'm happy to fix up anything else in there
< wumpus> two in circular-dependencies and one in security-check.py to do with macos: https://0bin.net/paste/TbGa7dzE#1zLWR4E0d6EA04YtkIEq5Ss3ZuGUljw7Hru9PxEkvMZ
< gribble> https://github.com/bitcoin/bitcoin/issues/1 | JSON-RPC support for mobile devices ("ultra-lightweight" clients) · Issue #1 · bitcoin/bitcoin · GitHub
< fanquake> cool
< 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> _base = None # type: Any
< wumpus> class ELFRecord(_base):
< wumpus> tada
< fanquake> hehe
< fanquake> Any is the get out of jail free card
< wumpus> fwiw it *does seem* __annotations__ is guaranteed to be ordered as in the definitions so you can use type annotations for defining ordered records https://stackoverflow.com/questions/64444638/is-the-order-of-entries-in-class-annotations-consistent-with-their-definitio
< 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
< gribble> https://github.com/bitcoin/bitcoin/issues/20403 | wallet: upgradewallet fixes, improvements, test coverage by jonatack · Pull Request #20403 · bitcoin/bitcoin · GitHub
< bitcoin-git> [bitcoin] luke-jr opened pull request #20448: RPC/Wallet: unloadwallet: Allow specifying wallet_name param matching RPC request, and document behaviour (master...unloadwallet_namematch) https://github.com/bitcoin/bitcoin/pull/20448
< bitcoin-git> [bitcoin] achow101 opened pull request #20449: build: Fix Windows installer build (master...fix-gitian-installers) https://github.com/bitcoin/bitcoin/pull/20449