< instagibbs>
pure utility rpcs? Not that I wouldn't appreciate the functionality but I don't think the motivation for adding it versus other utility rpcs has been given
< promag>
0.18 list is quite big
< fanquake>
promag Some stuff on there that'll get dropped, as well as issues that dont actually require any more action
< fanquake>
sipa I know it's arbitrary, but how long would you expect test-exhaust (from minisketch) to run for?
< bitcoin-git>
bitcoin/master 851380c Gregory Sanders: remove deprecated mentions of signrawtransaction from fundraw help
< bitcoin-git>
bitcoin/master fc21bb4 MarcoFalke: Merge #15245: remove deprecated mentions of signrawtransaction from fundra...
< bitcoin-git>
[bitcoin] MarcoFalke merged pull request #15245: remove deprecated mentions of signrawtransaction from fundraw help (master...fundraw_signraw) https://github.com/bitcoin/bitcoin/pull/15245
< dongcarl>
I'm talking to distro maintainers about #12255, when checking if a directory is a valid bitcoin directory, would checking `[ -f foo/bar/peers.dat ]` be sufficient? Would it be better to check `[ -f foo/bar/.lock ]`?
< gleb>
It seems like we restricted getdata on sending to 100 entries, and on receiving to 50,000 entries :)
< gleb>
If we restrict getdata to 100 on receive, it seems we would punish btcd nodes for misbehaving :)
< gleb>
If this is done on purpose, perhaps we should leave a comment at least...
< sdaftuar>
you mean leave a comment as to why we picked 100?
< gleb>
Why there are 2 different numbers where it is expected to have the same number (because we don't want to punish btcd?)
< sdaftuar>
or punish old nodes
< sipa>
well, it's always been 50000
< sipa>
we can't just change what we accept
< sipa>
we can change what we send
< sdaftuar>
^ right
< sipa>
so indeed, the needed comment is why are we only sending 100 :)
< gmaxwell>
we should probably limit rx too, that just takes coordination time.
< sdaftuar>
limiting rx implies creating some rate limit (to be useful). not sure what we gain otherwise except coordination headache
< sdaftuar>
gleb: i picked 100 by looking at the already_asked_for size relative to the number of peers i thought you might reasonably have, and picking a round number that would give us some buffer
< gmaxwell>
sdaftuar: not a rate limit, but the number of invs in a single message.
< sdaftuar>
gmaxwell: what purpose would that serve?
< gmaxwell>
We should not have 1.8MB packets in the protocol, they insert huge latency/processing spikes.
< sdaftuar>
ah, i guess that's fair
< sdaftuar>
we could also make our code smarter and allow interruption of processing such messages
< gmaxwell>
not the most critical of issues, but I can't see any reason why we'd ever want getdatas that large.
< sipa>
sdaftuar: i think we do, for getdata
< sdaftuar>
sipa: yep
< sdaftuar>
but for tx announcements
< gmaxwell>
sdaftuar: still have to buffer them.
< gmaxwell>
an obvious thing to do would be to limit them with the introduction of the encrypted transport.
< sdaftuar>
sure
< gleb>
But huge getdatas are as bad as huge invs, right?
< sdaftuar>
i think so
< sdaftuar>
we queue them up for processing
< sdaftuar>
in theory getdatas should be limited by what we announce, which is rate limited
< sdaftuar>
a misbehaving peer could send us garbage
< sdaftuar>
and we wouldn't punish (instead, we helpfully send notfound's)
< gmaxwell>
I thought we stopped sending notfounds?
< sdaftuar>
no, we stopped sending rejects i think
< sdaftuar>
i have a PR coming that will use the NOTFOUNDs! so i hope we don't get rid of those
< gmaxwell>
what would you use them for?
< sdaftuar>
to request a transaction from someone else. my thought was that we can make it so that not providing a transaciton is no longer a DoS on our peer
< sdaftuar>
and this would be a way to eventually drop mapRelay
< gmaxwell>
that requires the peer to cooperate, a DOS attacker wouldn't...
< sdaftuar>
gmaxwell: agree, but a DoS attacker has no bearing on whether we keep mapRelay around
< gmaxwell>
So all that would do is speed things up in the cooperative case? which sounds okay to me, but I don't see how we could drop maprelay?
< sdaftuar>
well i think then we can just serve things from the mempool
< sdaftuar>
and give a notfound otherwise
< gmaxwell>
oh maprealy sorry, being dumb and thinking you were talking about the map we use to schedule rerequests.
< gmaxwell>
sounds good.
< sdaftuar>
this should be easy to implement after gleb's PR, so i was hoping we might slip it into 0.18
< sdaftuar>
(the smarter behavior after receiving a NOTFOUND, i mean)
< gmaxwell>
I like this idea.
< phantomcircuit>
gmaxwell, im not sure how much of a processing spike a 1.8MB inv would cause, since it gets pushed into it's own queue and each inv is processed individually
< phantomcircuit>
(s/inv/getdata/)
< gmaxwell>
not processing spike, as much as it head of line blocks the socket, uses up a big buffer just checksumming it, etc. It's not much of a problem but it serves no use.
< phantomcircuit>
think the only issue there is the memory it uses
< phantomcircuit>
blocking the socket for the peer that's being stupid seems fine to me
< gmaxwell>
right, I said about it isn't so much a problem as its just dumb.
< gmaxwell>
though it does mean that we need to be willing to buffer 1.8mb of memory, but we have to for other messages in any case.
< promag>
so the issue is in wait_for_rpc_connection
< promag>
when using --usecli the TestNode still establishes a RPC connection
< promag>
to see if the RPC interface is ready
< promag>
however that connection is persistent by default
< sdaftuar>
so the issue is that the python tests aren't properly closing the connection, when using --usecli?
< promag>
in other words, if the daemon has 2 persistent connections, and 1 sends stop, the server (at the moment) won't force disconnect the other, it will timeout
< promag>
yes
< sdaftuar>
what's the mechanism for that to work when we're not using --usecli?
< sdaftuar>
oh the daemon will initiate the close?
< promag>
when not using --usecli the rpc connection will receive the header Connection: close, because the server is shutting down
< bitcoin-git>
bitcoin/master 58180b5 James O'Beirne: tests: add utility to easily profile node performance with perf
< bitcoin-git>
bitcoin/master 13782b8 James O'Beirne: docs: add perf section to developer docs
< bitcoin-git>
bitcoin/master 5029e94 MarcoFalke: Merge #14519: tests: add utility to easily profile node performance with p...
< bitcoin-git>
[bitcoin] MarcoFalke merged pull request #14519: tests: add utility to easily profile node performance with perf (master...2018-10-func-test-profiling) https://github.com/bitcoin/bitcoin/pull/14519
< gleb>
It's confusing that we do not log "AcceptToMemoryPool" upon accepting orphan transactions (when receive a valid parent)
< jnewbery>
promag: I'm not sure without digging into it. Feel free to @ me on the PR
< gleb>
We do it for a parent, but not for an not-orphan-anymore child...
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #15349: travis: Only exit early if compilation took longer than 30 min (master...Mf1902-travis30) https://github.com/bitcoin/bitcoin/pull/15349
< gmaxwell>
gleb: weird. patch accepted?
< gleb>
gmaxwell: Sorry not sure what you're asking, are you suggesting me to fix it?
< gmaxwell>
gleb: yes, I agree it's weird that we don't log. it also should be a trivial change to add a log entry for it.