< bitcoin-git>
bitcoin/master 8b57013 John Newbery: [net processing] Remove nStartingHeight check from block relay
< bitcoin-git>
bitcoin/master 94d2cc3 John Newbery: [net processing] Remove unnecesary nNewHeight variable in UpdatedBlockTip()
< bitcoin-git>
bitcoin/master f636008 John Newbery: [net processing] Clarify UpdatedBlockTip()
< bitcoin-git>
[bitcoin] MarcoFalke merged pull request #20624: net processing: Remove nStartingHeight check from block relay (master...2020-12-remove-starting-height) https://github.com/bitcoin/bitcoin/pull/20624
< aj>
really? proposed on friday, merged on monday?
< wumpus>
that *is* really fast for a (non-trivial) P2P change
< wumpus>
especially over a december weekend where everyone was occupied with macos signing
< wumpus>
(pretty sure it's correct but we should be careful)
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #20649: refactor: Remove nMyStartingHeight from CNode/Connman (master...2012-netNoMyHeight) https://github.com/bitcoin/bitcoin/pull/20649
< bitcoin-git>
[bitcoin] hebasto opened pull request #20650: depends: Drop workaround for a fixed bug in Qt build system (master...201214-xcb) https://github.com/bitcoin/bitcoin/pull/20650
< wumpus>
it's certain that there will be a rc4, that said, if you're going to sign rc3 or not is up to you
< wumpus>
it's still worth testing rc3 for other things, besides the macos determinism change i do not expect many differences
< provoostenator>
The determinism thing relates only to the signed binary, or also to the unsiged?
< provoostenator>
(code signed)
< wumpus>
sorry, it's not a determinism problem this time, it's a "broken code signature" problem, and it applies only to the codesigned build
< wumpus>
the -unsigned macos build should be fine
< provoostenator>
Ok, I managed to get a different unsigned build with #20638, so I'll build rc3 as well to see what's going on
< gribble>
https://github.com/bitcoin/bitcoin/issues/20638 | build: Fix macOS code signing by pre-allocating space for the code signature during gitian build by achow101 · Pull Request #20638 · bitcoin/bitcoin · GitHub
< ariard>
it's end of year and it can be cool to reflect on the last 12 months of p2p PRs, what we can learn from :)
< ariard>
feel free to add more example PRs, the ones I picked up were mostly on the number of comments
< luke-jr>
I wonder if there should be a "Backported" label to make it easier to look for missing backports (MarcoFalke, thoughts?)
< wumpus>
please not another backport related label :-)
< luke-jr>
<.<
< wumpus>
I mean it's a little bit more work but you could look in the history of the PR whether it's been backported, or was ever to be backported, to the branch you're interested in
< wumpus>
ariard: good idea
< luke-jr>
is there a motive behind #20599 ?
< gribble>
https://github.com/bitcoin/bitcoin/issues/20599 | net processing: Tolerate sendheaders and sendcmpct messages before verack by jnewbery · Pull Request #20599 · bitcoin/bitcoin · GitHub
< luke-jr>
wumpus: my thought is to list all merged PRs minus backported ones
< luke-jr>
+ a filter for bugfixes, it'd be pretty clean/easy actually
< wumpus>
FWIW you could do this with a script, looking at commit metadata on the branches
< luke-jr>
maybe, but a github link would be easier/nicer :P
< wumpus>
it's more thorough than a tag that people might forget or not
< wumpus>
also it's *per branch*
< wumpus>
luke-jr: yes, to make it (on the very long term) possible move all protocol negotiation to that phase
< jonatack>
ariard: i wonder if long-term contributors think the review bottleneck has improved over the past year...i'm too close to be objective but not sure that it has
< bitcoin-git>
bitcoin/master b316dcb John Newbery: [net processing] Tolerate sendheaders and sendcmpct messages before verack...
< bitcoin-git>
bitcoin/master fff7d05 MarcoFalke: Merge #20599: net processing: Tolerate sendheaders and sendcmpct messages ...
< bitcoin-git>
[bitcoin] MarcoFalke merged pull request #20599: net processing: Tolerate sendheaders and sendcmpct messages before verack (master...2020-12-tolerate-early-send-messages) https://github.com/bitcoin/bitcoin/pull/20599
< luke-jr>
jonatack: IMO I'm also too close (and haven't even tried to look at it objectively), but I also think it hasn't improved
< achow101>
provoostenator: can you provide the bitcoin-7f9d623e0210-osx-unsigned.tar.gz for your build of #20638?
< gribble>
https://github.com/bitcoin/bitcoin/issues/20638 | build: Fix macOS code signing by pre-allocating space for the code signature during gitian build by achow101 · Pull Request #20638 · bitcoin/bitcoin · GitHub
< achow101>
also, did you build 7f9d623 or the merge with master?
< provoostenator>
achow101: I'll try to drop it somewhere. I built the commit
< provoostenator>
(the branch)
< ariard>
jonatack: how would you qualify such improvements? for really good reasons, it's a hard question, you have to consider we've all different priorities :)
< achow101>
provoostenator: can you rebuild with a clean cache? it seems like the qt nondeterminism issue we fixed for rc2. did you perhaps do a gitian build on master earlier?
< provoostenator>
Remind me how to nuke cache?
< provoostenator>
Last thing I built before this was rc2
< provoostenator>
I never built rc3
< provoostenator>
(although I just did, but will do that again later, with a fresh cache)
< jonatack>
rm -r cache
< jonatack>
?
< achow101>
if you did a master build before that, it would use the master cache
< jonatack>
make -C ../bitcoin/depends download SOURCES_PATH=`pwd`/cache/common
< achow101>
or if you want to nuke the entire cache, rm -r gitian-builder/cache
< provoostenator>
I janked the whole cache, building your branch again now
< provoostenator>
I prefer nuking cache over carefully salvaging it
< jonatack>
heh
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #20653: doc: Move add relay comment in net to correct place (master...2012-docNetAddrRelay) https://github.com/bitcoin/bitcoin/pull/20653
< wumpus>
luke-jr: i'm not sure how to interpret your comment on #20605, are there any cases in which you see the code is unsafe on windows?
< wumpus>
the question is, is the solution wit ha condition variable safe, the pipe trick itself is very much a UNIX specitic thing and makes no sense *as it is* on win32
< wumpus>
I think I prefer changing the PR to keep using the poll loop on windows, I dont' know enough about that OS to be sure of this
< wumpus>
or maybe just close the PR maybe it's not relevant at all I dunno
< provoostenator>
achow101: nuked cache, ran the build again, same result
< sipa>
wumpus: so i'm confused in my link whether Win32 refers to just 32-bit windows
< sipa>
or all windows
< provoostenator>
Do I also need to replace the SDK?
< sipa>
because it says it invokes the signal handler in a new thread
< wumpus>
sipa: I don't know either
< achow101>
provoostenator: shouldn't be necessary. I'll dig a bit further then
< achow101>
but it doesn't seem like the issue is with the changes in the pr since it's all in the middle of the binary
< provoostenator>
I'll push my rc3 as well
< luke-jr>
sipa: it sounds like that's not even applicable to the signal() call
< luke-jr>
sipa: as I read it, you can't use signal() for SIGINT because SIGINT is supposed to get its own thread (implying signal() doesn't do own thread)
< sipa>
luke-jr: yes
< wumpus>
we don't *use* signal() on windows though
< wumpus>
so it shouldn't affect safety in our case because it doesn't call RequestShutdown
< sipa>
wumpus: oh!
< sipa>
there is an equivalent to catch interrupt signals for console applications, though
< sipa>
we just don't?
< wumpus>
there's a call to SetConsoleCtrlHandler
< wumpus>
I have no idea what that does
< sipa>
ah
< provoostenator>
My rc3 checksums for macOS are pretty much the same as other contributors.
< wumpus>
it does a Sleep(INFINITE) after calling StartShutdown so it seems to be different from a UNIX signal
< achow101>
provoostenator: I've seen this non-determinism before. It has something to do with version numbers. I forget how (if) that was resolved though... I think it might be another instance of the clang 8 bug
< wumpus>
if it's another instance of the clang 8 bug, I'd really suggest to switch compilers this time
< wumpus>
this bug-already fixed in upstream relaases-has given us enough work
< wumpus>
if only we had that kind of parallelism :p
< wumpus>
how many cores/hardware threads does your CPU have? i think at least llvmpipe creates a thread for every one
< luke-jr>
64
< wumpus>
par is limited to 15
< wumpus>
how many threads does bitcoind have there?
< luke-jr>
?
< wumpus>
MAX_SCRIPTCHECK_THREADS = 15
< luke-jr>
I didn't change it
< wumpus>
as far as i know that's the only variable number of threads scaled to cores, all the other ones are only based on configuration settings
< luke-jr>
oh well :/
< luke-jr>
is there a reason we prefer Freedesktop notifications over QSystemTray?
< wumpus>
would be interesting to do a "info threads all" (or whatever the exact command is) in gdb
< luke-jr>
the latter seems more ideal - not only is it platform-neutral, but it also presumably ties the notification to the icon
< luke-jr>
Invalid thread ID: all
< wumpus>
at the time it didn't work anymore for some desktops, static qt was unable to get notifications to ubuntu unity because it required a special plugin which wasn't available there
< wumpus>
of course, unity is dead now, i have no clue how it's handled in gnome3 etc
< luke-jr>
and it didn't fallback?
< wumpus>
if notifications through qsystemtray work okay across linux desktop environments now that's be nice
< wumpus>
i'm sure it was some really annoying problem, e.g. it claimed to support it but just ignored them then
< luke-jr>
>_<
< wumpus>
we've had many of those problems, if you're going to implement it differently it'll require a lot of testing (of the gitian-built binaries) across distros to be sure
< luke-jr>
what if I just add a guinotify=qsystemtray option or soemthing?
< wumpus>
bah, that doesn't seem worth a configuration option
< wumpus>
it's very much out of scope of the project, this is something that needs to be gotten right automatically
< luke-jr>
it's automatic, but QDBus takes like 20 seconds to timeout :/
< wumpus>
if we can drop the custom dbus code that'd be great but i'm sure i wrote it for a good reason at the time
< luke-jr>
and it doesn't look like there's a way to change the timeout in our code
< wumpus>
but that's what, half a decade ago?
< luke-jr>
shrug
< wumpus>
many new qt versions since, and as said, unity is dead
< wumpus>
and ideally notifications is something that gets handlded correctly by qt, that's why we use an abstraction like qt
< luke-jr>
but requires lots of testing = likely to get stuck in review limbo forever :/
< wumpus>
otoh deleting code usually gets people enthousiatic
< wumpus>
just don't add any regular expressions :-)
< luke-jr>
☹
< luke-jr>
I was going to reimplement Notificator in Perl tho
< luke-jr>
wumpus: Unity doesn't seem to be dead :/
< wumpus>
luke-jr: i don't get it why is dbus timing out for you in the first place? i'd expect it'd either make a connection (if the socket is available, available through the environment or throuh some X hack) or not
< wumpus>
dbus is a local socket so it has no reason at all to time out
< luke-jr>
wumpus: my development user has no access to my notifications
< luke-jr>
the timeout is internal to dbus
< luke-jr>
I don't know how it works
< andytoshi>
is it considered a problem that `getblockcount` sometimes returns a blockheight before all the BlockConnected signals have been processed? i assume not, such is the nature of signals
< wumpus>
it seems to me like there is some problem in the stack there; flatpak and such can also selectively reject dbus acces, it should be possible to reject access without causing applicatons to spend time timing out
< andytoshi>
but it can mean that you `generate` 100 blocks then try to spend a coin and find that you can't, because `getblockcount` says you have 101 blocks but the wallet doesn't know yet
< luke-jr>
wumpus: regardless, tiemout appears to be the default behaviour
< wumpus>
andytoshi: there's generally no guarantee there, the RPC tests do some tricks there IIRC
< andytoshi>
ok, cool, i'll study the existing tests
< wumpus>
luke-jr: definitely an upstream problem though
< wumpus>
we don't do anything partcularly complex or special
< sipa>
andytoshi: tests synchronize by sending a ping and waiting for a pong
< andytoshi>
sipa: i only have one node here
< andytoshi>
this is really hard to trigger, you have to generate and then immediately try to send coins (to a fixed address, even calling getnewaddress() is enough delay that you no longer see the race)
< sipa>
andytoshi: the python code tests have a full p2p implementation
< andytoshi>
yeah, but the funcitonal tests have a generate method which internally calls generatetoaddress
< andytoshi>
on a fresh getnewaddress
< sipa>
ok
< bitcoin-git>
[gui] luke-jr opened pull request #152: GUI: Initialise DBus notifications in another thread (master...gui_notify_setup_bg) https://github.com/bitcoin-core/gui/pull/152
< andytoshi>
ah i'm wrong, generatetoaddress and getblockcount are consistent
< andytoshi>
but generatetoaddress followed by an immediate spend may not be (coin selection may fail, as the wallet has not yet received the BlockConnected signal)
< sipa>
try syncwithvalidationinterfacequeue probably works
< andytoshi>
(you can repro by adding `self.nodes[0].generate(101)` and `self.nodes[0].getnewaddress()` to the CLTV test, say, or anything with only one node, then looking at the logs, you will see intermittently that the `getnewaddress` is received before the final block is connected)
< andytoshi>
ok thanks
< andytoshi>
i didn't know about this RPC, i think it's exactly what i watn
< andytoshi>
sorry for tech support spamming
< jonatack>
probably not relevant but -generate is back, as client-side cli only, not rpc
< jonatack>
as a replacement for the former generate rpc
< andytoshi>
oh, that's good to know. though i'm fine using generatetoaddress
< andytoshi>
the discrepancy was just between `getblockcount` and the wallet view. i wonder if `getblockcount` should call SyncWithValidationInterfaceQueue()
< sipa>
i think a better long-term approach is giving the wallet its own getblockcount and related RPCs
< andytoshi>
ah, yeah, i think that'd be cleaner and more performant
< wumpus>
yes, that would make sense
< wumpus>
the node methods shouldn't depend on being synchronized with the wallet, but the wallet already needs to keep track of where it is in regard to synchronization with the node so exposing that in a RPC would make sense
< wumpus>
each wallet separately in case of multiple wallets
< wumpus>
these things mostly come up for the tests
< wumpus>
luke-jr: any idea what QThreadPoolThread is? you have 64 of those too apparently
< wumpus>
so 64 for mesa, 64 for qthreadpool, 15 for script validation makes 143 of 195, ~15 normal bitcoind threads, still missing about 40 but getting closer