Check that new headers are not a descendant of an invalid block #11487
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Removes checking whitelisted behavior (which will be removed, the difference in behavior here makes little sense) and no longer requires that blocks at the same work as our tip be dropped if not requested (in part because we *do* request those blocks).
There is no reason to wish to store blocks on disk always just because a peer is whitelisted. This appears to be a historical quirk to avoid breaking things when the accept limits were added.
This is a simple cleanup that makes our accept requirements the same as our request requirements.
This is symmetric with the check in FindNextBlocksToDownload. Because we do not mark all children of a failed block as invalid during connection (we cannot walk down the block tree), this may prevent accepting an invalid block/header.
|
Concept ACK, utACK 09cf351, haven't reviewed the tests. |
promag
reviewed
Oct 12, 2017
| return false; | ||
|
|
||
| // Try to process all requested blocks that we don't have, but only | ||
| // process an unrequested block if it's new and has enough work to | ||
| // advance our tip, and isn't too many blocks ahead. | ||
| bool fAlreadyHave = pindex->nStatus & BLOCK_HAVE_DATA; | ||
| bool fHasMoreWork = (chainActive.Tip() ? pindex->nChainWork > chainActive.Tip()->nChainWork : true); | ||
| bool fHasMoreOrSameWork = (chainActive.Tip() ? pindex->nChainWork >= chainActive.Tip()->nChainWork : true); |
There was a problem hiding this comment.
While here why not remove the ternary and add above assert(chainActive.Tip())?
|
Isn't this already being done by |
knoxcard
approved these changes
Oct 14, 2017
| for (const CBlockHeader& header : headers) { | ||
| CBlockIndex *pindex = nullptr; // Use a temp pindex instead of ppindex to avoid a const_cast | ||
| if (!AcceptBlockHeader(header, state, chainparams, &pindex)) { | ||
| if (!AcceptBlockHeader(header, state, chainparams, &pindex, pindex)) { |
There was a problem hiding this comment.
Drop the {, if the following line is just return false.
There was a problem hiding this comment.
if (!AcceptBlockHeader(header, state, chainparams, &pindex, pindex)) return false;
|
Needs rebase |
|
See #11531 for a (likely better) version of this. |
|
Should this be closed in favor of #11531? |
laanwj
added a commit
that referenced
this pull request
Nov 1, 2017
…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
codablock
pushed a commit
to codablock/dash
that referenced
this pull request
Sep 26, 2019
…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
codablock
pushed a commit
to codablock/dash
that referenced
this pull request
Sep 29, 2019
…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
barrystyle
pushed a commit
to PACGlobalOfficial/PAC
that referenced
this pull request
Jan 22, 2020
…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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is symmetric with the check in FindNextBlocksToDownload.
Because we do not mark all children of a failed block as invalid
during connection (we cannot walk down the block tree), this may
prevent accepting an invalid block/header.
Plus some other AcceptBlock cleanups.