utACK

Seems like a slightly kludgy approach, but better than not, and at least its simple. Have we tested much with it set to also pull from inbound just to make sure there are no issues with that?

WRT localhost, thats probably reasonable but otoh it might hide issues. I think of this restriction on inbound as temporary until we get more controls in place to manage resource usage, after that it can go away completely I think.

Yeah that's pretty much how I feel too (kludgy but simple). I did some controlled regtest-testing when I wrote this, but I haven't done any live network tests yet, which I agree would be worth doing so I'll set something up and report back...

utACK. It's kludgy, but fine as an interim solution.

It should even be perfectly fine to always enable fetching from any peer, except for the implicitly accepted behavior that not accepting incoming connections will reduce bandwidth usage. When headers-first is more widely deployed, we can get rid of that policy I think, and just add an actual bandwidth throttler.

I tried testing initial block download with this code to see what kind of load it might put on an inbound peer (in the steady state, it doesn't seem that this code triggers very often, as expected). It is apparently easier than I realized to run into a situation where a node could start up and acquire inbound peers quickly, before any outbound connections are established. In repeated testing last night, I restarted a long-running node (with -reindex and without the first blockfile, to trigger downloading the whole chain), and my node was selecting an inbound peer to be the peer it was using for initial headers sync, and then with this code that peer was exclusively serving blocks for a while.

Of course that load would have come down after the headers were almost synced and parallel download started, but even at that point you might expect that this code could cause that inbound peer to be serving as much as 1/9 of the blocks to the node (probably less than that due to the insecure_rand() guard but would depend on download speeds from the outbound peers I think).

And more generally, for a node starting up and doing a large download of the blockchain, even if it starts up and is downloading only from outbound peers for a while, if it receives an inv from an inbound peer for a new block, then it would retrieve headers from that inbound peer using the existing inv-processing logic, and this change would then cause that inbound peer to serve earlier blocks as well.

Perhaps this shouldn't be merged as is if we want to avoid using inbound peers for large-ish block download?

@sdaftuar Any suggestions for a better strategy?

I've pushed an alternate fix here; please review this alternative solution.

What about dropping the insecure_rand in favor of a call to IsInitialBlockDownload():

if (!pto->fDisconnect && !pto->fClient && (fFetch || !IsInitialBlockDownload()) && state.nBlocksInFlight < MAX_BLOCKS_IN_TRANSIT_PER_PEER) {

This would mean that we'd be willing to let inbound peers share the download burden on up to the last ~288 blocks or so (our tip is within 24 hours of our best known header and our best known header is within 24 hours of the current time, in practice this is probably usually closer to 144 blocks). If we do this, I think we should also go ahead and send the initial getheaders messages to inbound peers as well, which I've done in this commit. This now fixes the problem gavin reported here:
#5138 (comment)

Also, I don't think the insecure_rand was giving us much; I think the idea was that in the steady state case, that would cause us to prefer downloading blocks on a reorg chain from outbound peers rather than inbound peers, but that is sufficiently rare and the load sufficiently low that I don't think optimizing that case is very important (especially because we're already willing to download the first new block announced by an inbound peer immediately, so why not the next block or two as well?).

I think this ought to work reasonably well when a far-behind node starts up, because it should have a number of peers by the time it gets close to catching up, meaning that the burden on inbound peers shouldn't be very large.

If a node were to come up an only be a day or two behind, though, then it's possible that a couple inbound peers would be hit with a large fraction of these 288 blocks, so if that is too much potential load, we could lighten it further by reducing MAX_BLOCKS_IN_TRANSIT_PER_PEER for inbound peers -- perhaps just allow 1 or 2 blocks in flight at a time from inbound peers here? I'm not sure how much optimizing is reasonable for this case; the existing logic already would put all the load on inbound peers if we don't have any outbound peers.

One behavior worth noting is that with this change, our maximum number of blocks in flight goes up (from 128 to 16 * Number of Network Peers). It's probably not generally going to exceed 288, the most blocks that can be outstanding when inbound peers start being used with the outbound peers while fetching. But if you have a stalling peer that isn't returning a block, then with the existing block download timeout logic (which depends on the number of blocks (with validated headers) in flight at the time of a block request), that stalling peer could have about 24 hours to deliver a block before being disconnected.

Still, there are easy workarounds (eg restarting a sync with listen=0 if you hit a bad peer before finishing a big sync with the network), and this change doesn't make the problem of an unresponsive peer any worse in the steady state, so I'm not sure how much optimization is worthwhile for this case. Thoughts?

FYI I just opened #5976 to mitigate any issues with longer block download timeouts that the approach here might otherwise exacerbate.

Concept ACK on the new approach. In the steady state, this shouldn't matter much, as almost all blocks there are fetched through the direct-fetching logic, rather than the asynchronous headers-based fetching.

utACK.

utACK