< wumpus>
MarcoFalke: nice, since #13051 it's possible to run individual functional tests in an out-of-tree build without having to manually provide BITCOIND=...
< bitcoin-git>
bitcoin/master c4fda76 João Barbosa: wallet: Interrupt rescan on shutdown request
< bitcoin-git>
bitcoin/master 2afdc29 Wladimir J. van der Laan: Merge #12507: Interrupt rescan on shutdown request...
< wumpus>
promag: I don't really like how everything is gaining a dependency on init.h for ShutdownRequested, but that's an architectural issue that can be solved separately
< antipovka_>
Someone asked me about this Satoshi Mine Bot and here it is! this bot simply automate your actions! It has a lot of settings and actually can help you being fast. But first you have to have your winning strategy! I am not going to explain about it, you are smart people, you are going to handle it! https://www.youtube.com/watch?v=ak-JKQvbDdc&t=7s
< bitcoin-git>
[bitcoin] laanwj closed pull request #13157: test: Handle timestamps without microseconds in combine_logs (master...2018_05_logcombine_timestamps) https://github.com/bitcoin/bitcoin/pull/13157
< bitcoin-git>
[bitcoin] practicalswift opened pull request #13159: Don't close old debug log file handler prematurely when trying to re-open (on SIGHUP) (master...handle-reopen-failed) https://github.com/bitcoin/bitcoin/pull/13159
< bitcoin-git>
[bitcoin] real-or-random opened pull request #13161: wallet: Reset BerkeleyDB handle after connection fails (master...bdb-reset) https://github.com/bitcoin/bitcoin/pull/13161
< MarcoFalke>
wumpus: (re out of tree tests) It works because test_runner.py is aware of the srcdir
< bitcoin-git>
[bitcoin] jnewbery opened pull request #13162: [logging] Don't incorrectly log that REJECT messages are unknown. (master...fix_reject_logging) https://github.com/bitcoin/bitcoin/pull/13162
<@wumpus>
but I eas not running test runner, that already worked before, but individual tests
< MarcoFalke>
test_runner is linked from the srcdir, so we can follow that link
< MarcoFalke>
hmm
< MarcoFalke>
individual tests are not available in the build dir?
<@wumpus>
MarcoFalke: indeed, so used the full path to the source dir, and it works, no matter whether the current directory is in the build directory or the source one
<@wumpus>
MarcoFalke: apparently I have a test/config.ini in the source dir, pointing to the BUILDDIR
<@wumpus>
MarcoFalke: not sure this is the intention, will wipe it and reconfiguer
< MarcoFalke>
Yeah, otherwise it would fail
< MarcoFalke>
the config.ini is required now
<@wumpus>
shouldn't it be in the build dir, though?
<@wumpus>
to be clear I think this is convenient, but it probably breaks the case where one source directory is used with multiple build dirs
<@wumpus>
or when configuring from a read-only source dir
<@wumpus>
ok, after reconfiguring the config.ini appears in the build dir, not the source dir, seems to have been an anomaly
< jamesob>
cfields: would it make sense to at some point move logging into a separate thread? (about to look at your changes, but was just wondering)
<@wumpus>
MarcoFalke: it looks like 'make clean' does not delete config.ini
< MarcoFalke>
oh
< cfields>
jamesob: yea, I think just about everyone has proposed that at some point, but nobody's done it :)
< MarcoFalke>
and make distclean?
<@wumpus>
MarcoFalke: what happened is probably: I configured the source dir, cleaned that, then configured in a separate build directory
< cfields>
jamesob: might be a good topic for the meeting today, I'm sure there are lots of thoughts on that
< jamesob>
cfields: welp, I'll add it to the list :)
< MarcoFalke>
Yeah, that is what I assumed
< jamesob>
cfields: cool, will bring it up
<@wumpus>
distclean does remove it
< MarcoFalke>
Usually you are asked to disclean before doing an out of tree build, at least I am
<@wumpus>
yes
<@wumpus>
I think that makes sense...
< sipa>
i'm going to be 10-15 min late for keeting
< sipa>
meeting
<@wumpus>
ok
< MarcoFalke>
Proposed short topic: Delete 0.8, 0.9 and 0.10 git branches
<@wumpus>
ack
<@wumpus>
oh
<@wumpus>
#startmeeting
< lightningbot>
Meeting started Thu May 3 19:00:10 2018 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
< jnewbery>
There's a whole series of sequential PRs on wallet load/create/unload. It'd be good to start moving through those, but I don't know how high priority they are for others
<@wumpus>
jnewbery: if it's blocking anyone, the first one on the list should be on there at least
< jtimon>
perhaps put the load one in high priority?
< jonasschnelli>
has also a unicorn, reload solved
<@wumpus>
yes
< LukeJr>
unicorns probably have a high street value
<@wumpus>
added
< jnewbery>
not any more. The market's been flooded
< BlueMatt>
topic: 0.15.1
< LukeJr>
shows what I know of unicorn markets
< BlueMatt>
err 16
<@wumpus>
LukeJr: yes, I"m trying to farm them and sell them, but I have more now than atoms in the knows universe so you could say the supply is more than the demand...
<@wumpus>
#topic Delete 0.8, 0.9 and 0.10 git branches
< MarcoFalke>
Background: Those last tagged versions on those branches are EOL for more than a year now. The tags can be kept for archival reasons, but the branches are no longer required. See https://bitcoincore.org/en/lifecycle/#schedule
<@wumpus>
yes
< BlueMatt>
ack
<@wumpus>
bye
< achow101>
ack
< jonasschnelli>
ack
< jtimon>
I didn't know we still had those
< LukeJr>
is the latest commit of each tagged?
< LukeJr>
If not, maybe a 0.n_final tag is appropriate
< jonasschnelli>
LukeJr: i hope so
<@wumpus>
LukeJr: agree, will check that first
< cfields>
er, are they not tagged from their respective branches?
< LukeJr>
jonasschnelli: it wouldn't be the first time we have backported fixes and never released with them
< MarcoFalke>
LukeJr: For the 0.8 one not
< MarcoFalke>
The others are last tag == tip of branch, last time I checked
<@wumpus>
will create a v0.8_final for that
<@wumpus>
+tag
< LukeJr>
maybe it can be a non-object tag, to avoid confusing users with a recent date
<@wumpus>
yes fine with me, just to avoid losing history
< MarcoFalke>
Oh, and the 0.9 one is missing one commit (upnp)
< sipa>
back
<@wumpus>
MarcoFalke: so same for 0.9 then
<@wumpus>
#topic Moving logging to a separate thread (jamesob)
< jamesob>
after working on #13099, I think it may be worthwhile to move logging into a separate thread
< BlueMatt>
ACKACKACKACKACKACKACKACKACKACK (this is a surprisingly high lag-creator for miners, at least for those with spinning-disk-backed-or-cloud-hosted machines
< jnewbery>
I started working on a branch for this last year, but didn't get very far
< jnewbery>
definite concept ACK
<@wumpus>
yes, concept ACK
< BlueMatt>
I did not propose it upstream as it means if we LogPrintf(); assert(false) we'll likely miss it
< jnewbery>
means the message processing thread can't be blocked on i/o hangs
< BlueMatt>
and I figured folks wouldnt want that
< skeees>
ACK - should point out that it could make crash debugging substantially more complicated
< BlueMatt>
jnewbery: it still will be a ton cause it ReadBlockFromDisk()s
<@wumpus>
BlueMatt: we could have special priority log calls that always make it to disk?
< jamesob>
skeees: right, I think that's the only caveat I can think of
< LukeJr>
BlueMatt: surely there's a way to run log flushing at assert
<@wumpus>
BlueMatt: CriticalLogPrintf?
< promag>
+1 BlueMatt point
< cfields>
are there any possible interactions where someone might be tailing the log and assume that it's synchronous?
< jonasschnelli>
use gdb
< BlueMatt>
wumpus: I mean the number of times its been helpful just to see which lines actually made it to debug.log when someone submitted a crash is super helpful
< LukeJr>
cfields: it couldn't be?
< BlueMatt>
and gdb isnt really an option when debugging remotely over github issues :/
< jimpo>
Does anyone know the relative performance of logging to console vs debug log file?
< BlueMatt>
jimpo: on non-serial-consoles, console logging should be fast, serial consoles can block
< LukeJr>
in theory, we could fork() a logging-only process, but that feels ugly
< BlueMatt>
(or vtt on a slow-refresh-monitor)
< sipa>
We should fork a separate process for logging, and then open a FIFO with it and pipe the log data there. If bitcoind crashes, that process hopefully survives :)
< sipa>
!hi5 LukeJr
< gribble>
Error: "hi5" is not a valid command.
<@wumpus>
it's mostly a win for low priority debugging messages, I guess
< BlueMatt>
or we could just make it optional
<@wumpus>
if logging is low volume it's never a bottleneck
< BlueMatt>
miners can enable it, everyone else probably doesnt need to
<@wumpus>
only miners that have debug=net enabled?
< skeees>
just throwing out some alternatives with a properly tuned buffer cache - logging shouldn't hit disk that often? if its still flushing frequently - could consider a more compact binary format for logs - though that means you'd need a special tool to read them
< jimpo>
I feel like disabling logging to file and piping stdout through tee or something should work fine
<@wumpus>
skeees: well the logging is unbuffered at this moment so can't do much worse than that...
< BlueMatt>
wumpus: I mean -debug should always mean non-async loggin, I think
< cfields>
skeees: it's not just hitting disk, it's serialization as well. Though I guess we couldn't defer serialization if references are passed in.
<@wumpus>
even line-buffered would be better for performance
< jamesob>
wumpus: I thought fwrite was buffered?
<@wumpus>
BlueMatt: right
< skeees>
another option is to have debug logging compiled out
< skeees>
instead of just disabled via flag as it is currently
<@wumpus>
jamesob: yes, but the buffer is disabled after opening
< BlueMatt>
I mean I dont think people who want performance dont want a debug.log
< BlueMatt>
they just dont want it to result in 8-ms pauses
<@wumpus>
skeees: that's not the issue; debug logging is already not executed if it's disabled
< * LukeJr>
wonders if we skip serialization when the debug log level is such that it will not be logged
<@wumpus>
skeees: even formatting is bypassed in that case
<@wumpus>
LukeJr: yes
< skeees>
LukeJr: yeah that's exactly the question i was trying to ask
< skeees>
ah ok
< skeees>
i dunno actually
<@wumpus>
tbh I don't think there's an actual issue here
< skeees>
because theres a lot of uint256.ToString() stuff
< cfields>
operations on incoming vars aren't skipped though, I believe. Like LogPrint("foo: %s", foo.get())
< BlueMatt>
the only issue here is for folks who care a shitload about small disk pauses
<@wumpus>
although the ring buffer would be nice because of gmaxwell's argument (log at higher debug message, but don't log the messages to disk unless a crash)
< BlueMatt>
hence why I use it in fibre
< BlueMatt>
when we get ReadBlockFromDisk to be async for peer handling, then it may make more sense to revisit this
<@wumpus>
so a crash would log the last low-priority debug messages even if debugging is disabled.. then agian, it's not possible to bypass formatting anymore in that case
< BlueMatt>
we'd have to swap assert() for dump_log_and_crash()
<@wumpus>
not sure that even requires logging to be in a sepaerate thread
< LukeJr>
wumpus: not sure if formatting is a big deal tbh
< LukeJr>
BlueMatt: or handle SIGABRT?
< BlueMatt>
no, but then I dont get to upstream my lockless ring buffer logger :p
<@wumpus>
LukeJr: well some messages are extremely high volume
< BlueMatt>
luke-jr: I dont think we want to do that much work in a signal handler
<@wumpus>
LukeJr: try enabling *all* debugging for fun
< BlueMatt>
matt@bitcoin-seednode:~$ ls -lha ~/.bitcoin/debug.log
< BlueMatt>
-rw------- 1 matt matt 8.6T May 3 19:24 /home/matt/.bitcoin/debug.log
<@wumpus>
heh
< sipa>
how long?
< LukeJr>
BlueMatt: if it's plain old C code, we can probably get away with it
<@wumpus>
hence my proposal at some point to split up net logging into low and high volume messages
< * jamesob>
buys BlueMatt a pair of socks with the logrotate manpage on them
< BlueMatt>
cause they're threading issues they almost certainly wont effect anyone except submitblock users
< BlueMatt>
ie miners, and only rare races
< BlueMatt>
but, still, I think given that and some of the other various fixes we've had, it may be worth backporting
<@wumpus>
13092 is an issue, not a PR, can't label it for backport
< BlueMatt>
further, at least personally, I'd kinda like to see #11423 make some movement, and making that kind of policy change in a non-minor-version strikes me as weird
< gribble>
https://github.com/bitcoin/bitcoin/issues/11423 | [Policy] Make OP_CODESEPARATOR and FindAndDelete in non-segwit scripts non-std by jl2012 · Pull Request #11423 · bitcoin/bitcoin · GitHub
< BlueMatt>
yes, it needs a pr, sdaftuar has a branch, I believe, but not open
< sdaftuar>
skeees has a pr, one sec
< skeees>
#13023
< sdaftuar>
#13023
< BlueMatt>
was the skeees pr updated for the new method?
< gribble>
https://github.com/bitcoin/bitcoin/issues/13023 | Always refresh most work chain when reacquiring cs_main in ActivateBestChain() by skeees · Pull Request #13023 · bitcoin/bitcoin · GitHub
< gribble>
https://github.com/bitcoin/bitcoin/issues/13023 | Always refresh most work chain when reacquiring cs_main in ActivateBestChain() by skeees · Pull Request #13023 · bitcoin/bitcoin · GitHub
< sdaftuar>
no we need to settle on the right fix
< skeees>
still needs a bit of work - i need to pull in some fixes @sdaftuar made
< sdaftuar>
i think the fix i proposed works, but maybe someone has a better idea
<@wumpus>
ok added tags
< BlueMatt>
ok, yes, I think that is doable, however, and I think between that and making progress on the standardness changes from jl2012 probably are worth a minor bump
< skeees>
there's also #12988 - which is a similar type of bug
< BlueMatt>
there are two main approaches, but both end up requiring similar refactors for the majority of their work
< BlueMatt>
in the past I've looked at doing ProcessMessages() in parallel
< BlueMatt>
in this pr skeees moves the validation processing of txn/blocks into a queue and does that in a separate thread
< sipa>
how does that interact with eg sending a ping after a block, and expecting it to be processed after the pong returns?
< BlueMatt>
in both cases, we end up building logic to "pause" processing of a peer until whatever message it just generated has been processed
<@wumpus>
that sounds sensible, though I'm really afraid of c++ threading
< BlueMatt>
sipa: ^
< gmaxwell>
Seems like it would add more locking overhead.
< BlueMatt>
(which would also likely be useful for eg ReadBlockFromDisk moving to be more async)
< sipa>
sound reasonable to me
< BlueMatt>
and a ton of logic to move CNodeState out of cs_main (likely by creating a CNodeState2 during transision)
< BlueMatt>
cfields: may have more thoughts
< sipa>
that sounds even better
< skeees>
the other side benefit of this approach is that ultimately it may simplify the concurrency model inside validation
< sipa>
though everything depends on the details...
< BlueMatt>
gmaxwell: indeed, thats a major difference between the skeees approach and the parallel ProcessMessages approach
< skeees>
having a single thread processing from an ordered queue would have also solved the concurrency issues discussed before
< BlueMatt>
ProcessMessages will still do the work on the processing thread, which should have a bit less locking overhead
< cfields>
BlueMatt: just reading now. at a glance, this approach sounds similar to what I had in mind as well
< BlueMatt>
but is definitely more complicated than the skeees approach
< BlueMatt>
which draws a much cleaner boundary between validation and net_processing
< gmaxwell>
The biggest gain I'm aware of from concurrency is that right now when connecting multiple blocks at a time (due to out of order fetching in IBD) we stop downloading new blocks, ... fixing that would be a big gain; so it might be worth prioritizing improvements that would let that happen.
< cfields>
BlueMatt: I don't see why they'd be mutually exclusive?
< BlueMatt>
gmaxwell: that and relaying compact block gettxn during block connection
< BlueMatt>
which is the one big cheapish win left for block-relay-latency
< gmaxwell>
(e.g. esp when fetching from slow peers, watching network traffic during IBD is comical... transfering at only a couple mbps for a while then nothing for seconds at a time while it connects)
< BlueMatt>
cfields: well doing both is maybe not so useful
< BlueMatt>
since they'd accomplish largely the same thing
< gmaxwell>
BlueMatt: Fair enough for gettxn, though right now my own node makes almost no gettxn requests... and orphaning rates appear to be exceptionally low.
< gmaxwell>
(so what I'm saying is that at the moment I don't think of latency improvements as that critical)
< BlueMatt>
gmaxwell: the skeees approach is certainly better for doing things like deserialization of blocks coming in from other peers while calling ProcessNewBlock/ActivateBestChain
< BlueMatt>
indeed, right now (with mempool ~empty) compact block performance is ~perfect
< BlueMatt>
its not always so, however
< gmaxwell>
hm. interesting point that making deserialization concurrent would potentially be a multi-core gain.
< BlueMatt>
also, fibre nodes see more getblocktxn than miners
< sipa>
by "mempool ~empty" you mean "mempool is a perfect match" ?
< BlueMatt>
which would be equally solved by miners delaying 1 second before including new txn in blocks....
< gmaxwell>
sipa: if each block mostly clears the mempool, miner-specific prioritizaition of transactions doesn't cause as much surprise txn.
< gmaxwell>
BlueMatt: and also making use of the CB facility for transmitting extra txn proactively.
< BlueMatt>
yea, that too
< BlueMatt>
anyway, so tl;dr: I was a bigger fan of the skeees approach and wanted a little bit of concept discussion on the idea of putting a queue in front of the entire validation stuff
< BlueMatt>
since thats a huge departure from current operation
< BlueMatt>
but I think the potential gain is nice
< BlueMatt>
plus all our blocks are already passed in as shared_ptrs anyway.....
< gmaxwell>
so right my point was that speading up IBD is probably a big win, .. improving latency and what not would be nice too but I think it's less important.
< LukeJr>
+1
< BlueMatt>
yes, right now that is definitely true
< BlueMatt>
certainly this has the potential to address both, however
< gmaxwell>
making use of more cores during sync would be nice too, e.g. if deserilization and hashing can happen on seperate threads async with connection.
< BlueMatt>
we can do CheckBlock on the net_processing thread
< BlueMatt>
(since, and sipa is gonna die a little bit inside, here, the result is cached in the CBlock structure)
< sdaftuar>
that is kinda gross :)
< BlueMatt>
its incredibly gross
< gmaxwell>
(e.g. multiple message handling threads, which do deserialization and maybe stateless checks)
<@wumpus>
oh no...
< bitcoin-git>
[bitcoin] practicalswift opened pull request #13163: Make it clear which functions that are intended to be translation unit local (master...internal-linkage) https://github.com/bitcoin/bitcoin/pull/13163
< gmaxwell>
though I did a dumb hack a while back that reordered some of the block validation steps to interleave checks that operate per transaction and got a few percent sync speed improvement... we should be mindful of memory locality and not introduce even more passes over blocks.
< BlueMatt>
gmaxwell: dunno about multiple message handling threads being doable in the immediate future, but doing CheckBlock on the message processing thread (which does merkle root verification, which is a good chunk of the work) may be worth a chunk
< skeees>
cool - so if no strong objections or concerns with the approach i'll continue this and come back when its more ready for review
< gmaxwell>
Funny, I would have thought handling seperate peers in seperate threads would almost just work now, vs stuff that handled messages from the same peer concurrently, which violates protocol assumptions about ordering without special work to add processing barriers.
< BlueMatt>
gmaxwell: CNodeState requires cs_main, so....not even close :(
< gmaxwell>
(almost just work, because the critical data structues have locks ... oh CNodeState)
< BlueMatt>
I've got some old branches that do that, if you want to look
< gmaxwell>
cs_main lite.
< BlueMatt>
but they're....not small changes
< BlueMatt>
and cfields hates them, though he's probably right
< BlueMatt>
anyway, so </topic> </meeting> ["DIE", "XML"]?
< gmaxwell>
in particular our most commons messages are transaction invs and get datas, and those should only need mempool stuff for much of their activity.
< gmaxwell>
k.
< BlueMatt>
gmaxwell: yes, I did all that in an old branch
< BlueMatt>
that in particular is much closer than it used to be
< BlueMatt>
cause now the HandleGetData stuff cant take cs_main at the top
< BlueMatt>
(it got a bunch of time in helgrind, too, and got all the issues fixed, so I'm at least kinda confident in it, but its essentially unreviewable, even by my standards)
<@wumpus>
#endmeeting
< lightningbot>
Meeting ended Thu May 3 19:57:40 2018 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
< gmaxwell>
I think it would be simpler now, a lot of changes in the last year would have made it easier (I say this without having recently looked at that branch)
< BlueMatt>
yes, I agree
< BlueMatt>
rather significantly
< BlueMatt>
and if we took the "split CNodeState" approach instead of "pull CNodeState above cs_main in global lock order and give it its own lock all in one go" approach that that branch takes, it'd likely be wayyy simpler
< bitcoin-git>
[bitcoin] jamesob closed pull request #13099: Use thread names in logs and deadlock diagnostics (master...2018-04-26-use-threadnames) https://github.com/bitcoin/bitcoin/pull/13099
< cfields>
jamesob: noo!
< cfields>
jamesob: I hope you don't think I'm being NIH about that PR, that wasn't my intention at all. Coding up an alternative myself helps me to understand the challenges better, it wasn't meant to replace your work.
<@wumpus>
"Note: GitHub Services are being deprecated. Please contact your integrator for more information on how to migrate or replace a service to webhooks or GitHub Apps. "
<@wumpus>
I wonder if that means the IRC notification bot will disappear
< luke-jr>
isn't it a webhook?
<@wumpus>
no
<@wumpus>
it's under "integrations and services" which displays that
< luke-jr>
IRC notifications of projects seems to have mostly died with CIA :<
< jamesob>
cfields: oh no not at all! sorry if the PR close came off as me being upset
< jamesob>
cfields: I just like your commits better :). Maybe I can cherry-pick them and then shuffle in my GetProcessName/SetProcessName stuff + some tests
< jamesob>
if that sounds good, would it make sense to repurpose that same PR or open a new one?
< cfields>
jamesob: ok, good
< cfields>
jamesob: if you're going to do away with the suffix handling, I'd say just do a new PR
< cfields>
jamesob: either way, my code is all yours if you like it
< jamesob>
cfields: yeah, should be easy to incorporate the suffix stuff inline as you suggest
< jamesob>
just felt embarrassed I didn't see that earlier :)