Don't process unrequested, low-work blocks #11458
Conversation
|
Concept ACK, though fForceProcessing/fRequested doesn't do the thing you think it does, see, eg #9352 (review). |
|
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 |
@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 Please see promag@4198798. |
|
@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 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. This commit fixes that behavior.
sdaftuar
force-pushed
the
2017-10-blocks-before-minwork
branch
from
f0940e4 to
08fd822
Compare
|
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 01b52ce |
|
utACK 01b52ce, didnt review the test parts. |
01b52ce Add comment explaining forced processing of compact blocks (Suhas Daftuar) 08fd822 qa: add test for minchainwork use in acceptblock (Suhas Daftuar) ce8cd7a Don't process unrequested, low-work blocks (Suhas Daftuar) Pull request description: 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. Tree-SHA512: 1a4fb0bbd78054b84683f995c8c3194dd44fa914dc351ae4379c7c1a6f83224f609f8b9c2d9dde28741426c6af008ffffea836d21aa31a5ebaa00f8e0f81229e
…id block (more effeciently) f3d4adf Make p2p-acceptablock not an extended test (Matt Corallo) 00dcda6 [qa] test that invalid blocks on an invalid chain get a disconnect (Matt Corallo) 015a525 Reject headers building on invalid chains by tracking invalidity (Matt Corallo) 932f118 Accept unrequested blocks with work equal to our tip (Matt Corallo) 3d9c70c Stop always storing blocks from whitelisted peers (Matt Corallo) 3b4ac43 Rewrite p2p-acceptblock in preparation for slight behavior changes (Matt Corallo) Pull request description: @sdaftuar pointed out that the version in #11487 was somewhat DoS-able as someone could feed you a valid chain that forked off the the last checkpoint block and force you to do lots of work just walking backwards across blocks for each new block they gave you. We came up with a few proposals but settled on the one implemented here as likely the simplest without obvious DoS issues. It uses our existing on-load mapBlockIndex walk to make sure everything that descends from an invalid block is marked as such, and then simply caches blocks which we attempted to connect but which were found to be invalid. To avoid DoS issues during IBD, this will need to depend on #11458. Includes tests from #11487. Tree-SHA512: 46aff8332908e122dae72ceb5fe8cd241902c2281a87f58a5fb486bf69d46458d84a096fdcb5f3e8e07fbcf7466232b10c429f4d67855425f11b38ac0bf612e1
|
Needs backport (#11531 depends on this somewhat indirectly) |
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. This commit fixes that behavior. Github-Pull: bitcoin#11458 Rebased-From: ce8cd7a
Github-Pull: bitcoin#11458 Rebased-From: 08fd822
Github-Pull: bitcoin#11458 Rebased-From: 01b52ce
01b52ce Add comment explaining forced processing of compact blocks (Suhas Daftuar) 08fd822 qa: add test for minchainwork use in acceptblock (Suhas Daftuar) ce8cd7a Don't process unrequested, low-work blocks (Suhas Daftuar) Pull request description: 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. Tree-SHA512: 1a4fb0bbd78054b84683f995c8c3194dd44fa914dc351ae4379c7c1a6f83224f609f8b9c2d9dde28741426c6af008ffffea836d21aa31a5ebaa00f8e0f81229e
…n invalid block (more effeciently) f3d4adf Make p2p-acceptablock not an extended test (Matt Corallo) 00dcda6 [qa] test that invalid blocks on an invalid chain get a disconnect (Matt Corallo) 015a525 Reject headers building on invalid chains by tracking invalidity (Matt Corallo) 932f118 Accept unrequested blocks with work equal to our tip (Matt Corallo) 3d9c70c Stop always storing blocks from whitelisted peers (Matt Corallo) 3b4ac43 Rewrite p2p-acceptblock in preparation for slight behavior changes (Matt Corallo) Pull request description: @sdaftuar pointed out that the version in bitcoin#11487 was somewhat DoS-able as someone could feed you a valid chain that forked off the the last checkpoint block and force you to do lots of work just walking backwards across blocks for each new block they gave you. We came up with a few proposals but settled on the one implemented here as likely the simplest without obvious DoS issues. It uses our existing on-load mapBlockIndex walk to make sure everything that descends from an invalid block is marked as such, and then simply caches blocks which we attempted to connect but which were found to be invalid. To avoid DoS issues during IBD, this will need to depend on bitcoin#11458. Includes tests from bitcoin#11487. Tree-SHA512: 46aff8332908e122dae72ceb5fe8cd241902c2281a87f58a5fb486bf69d46458d84a096fdcb5f3e8e07fbcf7466232b10c429f4d67855425f11b38ac0bf612e1
10 participants
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.