< bitcoin-git>
bitcoin/master 6b8d86d Andrew Chow: Require a public key to be retrieved when signing a P2PKH input...
< bitcoin-git>
bitcoin/master 16e3b17 Pieter Wuille: Merge #14689: Require a public key to be retrieved when signing a P2PKH input...
< bitcoin-git>
[bitcoin] sipa closed pull request #14689: Require a public key to be retrieved when signing a P2PKH input (master...fix-pkh-pubkeys) https://github.com/bitcoin/bitcoin/pull/14689
< provoostenator>
Review shout out to #12783 which prevents macOS from going into app nap during initial sync.
< wumpus>
currently it's prevented from going into app nap completely
< wumpus>
but yes it makes sense to only prevent so during sync
< fanquake>
wumpus the app-nap prevention is currently broken on master, and likely recent versions. Apple seems to have killed the plist flag we were using to disable it.
< wumpus>
ohhh wonderful
< fanquake>
All documentation about the flag seems to have also disappeared at some point
< bitcoin-git>
[bitcoin] fanquake opened pull request #14701: build: Add CLIENT_VERSION_BUILD to CFBundleGetInfoString (master...add-build-version-to-info-plist) https://github.com/bitcoin/bitcoin/pull/14701
< wumpus>
wonder how long it will take Apple to blackhole this functionality too
< fanquake>
If anything, Apple seems to have a thing for conserving/reducing resource & power usage
< wumpus>
that makes sense, it's just that it seems they offer the least amount of API stability
< fanquake>
We'll probably be able to go down the "more properly optimise the macOS resource usage path", although hopefully without introducing lots of macOS specific IBD weirdness/issues etc.
< wumpus>
it's too bad that standards are a thing of the past, or we'd be able to do it for all OSes at the same time...
< wumpus>
AHHH poll broken too on MacOS?
< fanquake>
supposedly, I was going to get a newer version of the darwin-xnu source and see if anything had changed
< * wumpus>
wonders how libevent handles this, but… I suspect they use kqueue directly where available
< wumpus>
this is what you get for trying to reinvent the wheel :/
< wumpus>
oh... it was easy to switch to poll,wasn't it
< wumpus>
just a few lines!
< wumpus>
they said...
< wumpus>
now we're finding bugs in operating systems
< fanquake>
definitely not a first for bitcoin core
< wumpus>
apparent conclusion: if you want to implement asynchronous I/O, implementing all the different methodologies of different operating systems is not *optional* but a requirement, not just for performance but for basic functionaltiy
< wumpus>
everything is broken somewhere
< wumpus>
anyhow good investigation jamesob!
< fanquake>
^
< wumpus>
fanquake: in any case with regard to resource, I/O, scheduling, power optimization, I think we need some kind of abstraction behind which we can put the various OS-specific methods
< wumpus>
just littering the code with #ifdef MacOSX is not a nice way forward
< fanquake>
I agree. The macOS stuff is also likely to be Objective-C
< wumpus>
and similar things exist for all operating systems, e.g. for linux there's #9245
< gribble>
https://github.com/bitcoin/bitcoin/issues/9245 | Drop IO priority to idle while reading blocks for peer requests and startup verification by luke-jr · Pull Request #9245 · bitcoin/bitcoin · GitHub
< wumpus>
fanquake: or swift!
< fanquake>
wumpus heh I've been investigating that. I'm writing a fair bit of Swift at the moment (iOS), and would happily swap out all Objective-C for it.
< wumpus>
fanquake: I think at the moment that would complicate things because objc is part of the standard toolchains and swift is not, but might be wrong
< bitcoin-git>
bitcoin/0.17 89306ab Russell Yanofsky: [wallet] Restore ability to list incoming transactions by label...
< bitcoin-git>
bitcoin/0.17 5150acc Wladimir J. van der Laan: Merge #14441: [wallet] Backport(0.17): Restore ability to list incoming transactions by label...
< meshcollider>
IMO #13100 should be tagged for 0.18
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #14705: travis: Avoid timeout on verify-commits check (master...Mf1811-travisVerifyCommits) https://github.com/bitcoin/bitcoin/pull/14705
< phantomcircuit>
wumpus, libevent uses kqueue is the answer to avoiding this issue
< phantomcircuit>
wumpus, i could also switch the USE_POLL macro to a whitelist of working systems (ie linux)
< wumpus>
phantomcircuit: yes I think whitelisting is the only option for now
< wumpus>
phantomcircuit: it's disappointing that this is needed but not your fault
< wumpus>
phantomcircuit: you could have no idea that this was such a bottomless pit of horror
< sipa>
wumpus: it's almost as if someone did realize that, and then decided to write libevent :)
< phantomcircuit>
at this point i think it would have been easier to move to libevent honestly
< phantomcircuit>
but oh well
< sipa>
phantomcircuit: the refactorings are useful, it's not wasted work
< sipa>
even if we eventually get libevent
< sipa>
phantomcircuit: also, in practice, i expect that most many-connection nodes run linux
< phantomcircuit>
sipa, agreed
< phantomcircuit>
sipa, well poll doesn't work on windows (actually it would how we're using it...)
< phantomcircuit>
so it's windows using select and linux using poll and then ...? freebsd and osx?
< sipa>
but select also doesn't have the same 1024-low-fds-only restriction on windows
< phantomcircuit>
sipa, exactly
< phantomcircuit>
so uh what's the right macro to check for linux systems?
< sipa>
__linux__ exists, apparently
< phantomcircuit>
wumpus, switched to whitelisting
< luke-jr>
wumpus: should we use QtNetwork? :P
< luke-jr>
jk
< luke-jr>
re #9245, note that I have Mac/Windows ports too ready for once it gets merged :/
< gribble>
https://github.com/bitcoin/bitcoin/issues/9245 | Drop IO priority to idle while reading blocks for peer requests and startup verification by luke-jr · Pull Request #9245 · bitcoin/bitcoin · GitHub
< sipa>
gmaxwell: feel like having a look at #14624 ?
< phantomcircuit>
i think i've fixed the style nits too exept in the select code where it's just a move
< phantomcircuit>
i guess the loops could use socket_id instead of hSocket, but it's using the moved hSocketMax so that might be less clear
< sipa>
makes sense
< sipa>
phantomcircuit: going to try running
< phantomcircuit>
missed braces in SocketEvents commit actually
< phantomcircuit>
sipa, ok fixed
< sipa>
net.cpp:1355:10: warning: types may not be defined in a for-range-declaration
< sipa>
for (struct pollfd pollfd : vpollfds) {
< sipa>
^~~~~~
< sipa>
phantomcircuit: i'm benchmarking the time to construct the input to poll()... looks like around 10 microseconds for 15 connections
< Lightsword>
is there a reason we require pkg-config on non-windows builds?
< sipa>
phantomcircuit: that does mean around 0.6 ms to start a poll every 50 ms with 1000 connections, which is a nontrivial cpu load
< gmaxwell>
to be fair, handling more connections really shouldn't be a goal for this change right now... goal should be not running into the 1024 FD limit, while handling the same number of connections.
< gmaxwell>
poll itself is not really the most optimal approach for large numbers of connections inherently.
< sipa>
phantomcircuit: you should update this line in init.cpp:
< sipa>
maybe we want another max connections hard limit, but it shouldn't be dependent on FD_SETSIZE anymore
< gmaxwell>
yea, 1000 or something? it should be some level that we've actually tested the code at.
< gmaxwell>
lest we end up with users who've already set that value to 10,000 for inexplicable reasons finding themselve easily OOMed by random internet jerks.
< sipa>
please to do DoS my bitcoin.sipa.be node :)
< sipa>
it's running the poll PR
< sipa>
*try to
< sipa>
but i don't get more than ~100 connections organically