A peer could try to waste our resources by sending us unrequested blocks with low work (eg to fill up our disk). Since e265200 we no longer request blocks until we know we're on a chain with more than nMinimumChainWork (our anti-DoS threshold), but we would still process unrequested blocks that had more work than our tip (which generally has low-work during IBD), even though we may not yet have found a headers chain with sufficient work.
Fix this and add a test.
Concept ACK
Concept ACK, though fForceProcessing/fRequested doesn't do the thing you think it does, see, eg #9352#pullrequestreview-13343287.
This is only useful if tip chain work is less than minimum chain work, which I think only happens in IBD?
Edit: ready more carefully the PR description.
Maybe we should just ignore unrequested blocks if tip chain work < minimum chain work? This way this test could be moved before AcceptBlockHeader?
Concept ACK, though fForceProcessing/fRequested doesn't do the thing you think it does, see, eg #9352 (review). @TheBlueMatt Is there a mistake here? I know we use the fForceProcessing/fRequested in a couple ways but I believe this is correct -- for instance the unusual use of it in compact block processing shouldn't be an issue, because we don't try to reconstruct compact blocks when our chain tip is far behind. Did I miss a case?
Concept ACK
@sdaftuar Indeed. You were right, I was confused, but this does leave the logic being added here super brittle to future changes. For those following along at home, the reason compact blocks always passing fRequested as true doesn't break this is that it does a CanDirectFetch first, which only looks at the timestamp of our chainActive.Tip(). Thus, this logic would be broken if there were any way for an attacker to cheaply feed us a single best-chain block which had a timestamp close to today, though there should be no way to do so as other ways to get the block onto disk (and, thus, to become our chainActive.Tip()) without meeting the nMinimumChainWork test in FindNextBlocksToDownload. This anti-DoS-logic-stradling-validation-and-net_processing stuff irritates me endlessly, but I think this is fine for now (and should likely be backported in 0.15.0.2 if we do one as a part of #11531).
utACK (didn't review test changes, which will conflict with #11531) f0940e462cb435f10ef6f052dae609655202f245
Also, maybe add a comment about the CanDirectFetch stuff somewhere.
A peer could try to waste our resources by sending us unrequested blocks with
low work, eg to fill up our disk. Since
e2652002b6011f793185d473f87f1730c625593b we no longer request blocks until we
know we're on a chain with more than nMinimumChainWork (our anti-DoS
threshold), but we would still process unrequested blocks that had more work
than our tip. This commit fixes that behavior.
Needed a rebase due to a conflict in the test.
@TheBlueMatt I added a comment. @promag Thanks for the patch and for the review, but I (slightly) prefer the code that I've presented already. Skipping the call to AcceptBlockHeader doesn't do much in the adversarial case (which is the only case we're concerned about here), because an attacker would just separately deliver the header which we would process and store. And it seems clearer to me to just put this in the same block with the other checks we do when a block is not requested.
utACK 01b52ce.
Nit, I would squash the test with the commit adding the feature.
utACK 01b52cedd42f50a93b40981c91af7c12de6e45ce
utACK 01b52cedd42f50a93b40981c91af7c12de6e45ce, didnt review the test parts.
Needs backport (#11531 depends on this somewhat indirectly)