Check that new headers are not a descendant of an invalid block #11487

pull TheBlueMatt wants to merge 5 commits into bitcoin:master from TheBlueMatt:2017-10-acceptblock-validity-check changing 3 files +220 −117

TheBlueMatt commented at 9:26 PM on October 11, 2017: member

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.

Rewrite p2p-acceptblock in preparation for slight behavior changes

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).

ee11354b78

Stop always storing blocks from whitelisted peers

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.

78814d2fb6

Accept unrequested blocks with work equal to our tip

This is a simple cleanup that makes our accept requirements the
same as our request requirements.

fbb7b47ca7

Check that new headers are not a descendant of an invalid block

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.

09cf35122a

[qa] test that invalid blocks on an invalid chain get a disconnect 7a8e117c65
fanquake added the label Validation on Oct 11, 2017
sipa commented at 12:36 AM on October 12, 2017: member

Concept ACK, utACK 09cf35122a219217f841e4e4f7847386eb0b0b8a, haven't reviewed the tests.

in src/validation.cpp:3140 in 7a8e117c65

3137 |      // Try to process all requested blocks that we don't have, but only
3138 |      // process an unrequested block if it's new and has enough work to
3139 |      // advance our tip, and isn't too many blocks ahead.
3140 |      bool fAlreadyHave = pindex->nStatus & BLOCK_HAVE_DATA;
3141 | -    bool fHasMoreWork = (chainActive.Tip() ? pindex->nChainWork > chainActive.Tip()->nChainWork : true);
3142 | +    bool fHasMoreOrSameWork = (chainActive.Tip() ? pindex->nChainWork >= chainActive.Tip()->nChainWork : true);

promag commented at 2:12 PM on October 12, 2017:

While here why not remove the ternary and add above assert(chainActive.Tip())?

achow101 commented at 7:20 PM on October 12, 2017: member

Isn't this already being done by AcceptBlockHeader? It checks that the previous block was not marked invalid. What does this do in addition to that?
laanwj added this to the milestone 0.15.1 on Oct 12, 2017
laanwj removed this from the milestone 0.15.1 on Oct 12, 2017
laanwj added this to the milestone 0.15.0.2 on Oct 12, 2017
laanwj added this to the "Review priority 0.15.0.2" column in a project

theuni commented at 10:51 PM on October 12, 2017: member

utACK 7a8e117c659c1245e95b86db8672ac60c0d192cb. @achow101 Say someone sends you 2 valid-looking connecting headers. Then an invalid block 1. Then header 3. At that point you should mark 2 as invalid rather than accepting 3.

in src/validation.cpp:3110 in 7a8e117c65

3104 | @@ -3081,9 +3105,9 @@ bool ProcessNewBlockHeaders(const std::vector<CBlockHeader>& headers, CValidatio
3105 |  {
3106 |      {
3107 |          LOCK(cs_main);
3108 | +        CBlockIndex *pindex = nullptr; // Use a temp pindex instead of ppindex to avoid a const_cast
3109 |          for (const CBlockHeader& header : headers) {
3110 | -            CBlockIndex *pindex = nullptr; // Use a temp pindex instead of ppindex to avoid a const_cast
3111 | -            if (!AcceptBlockHeader(header, state, chainparams, &pindex)) {
3112 | +            if (!AcceptBlockHeader(header, state, chainparams, &pindex, pindex)) {

knoxcard commented at 8:33 AM on October 14, 2017:

Drop the {, if the following line is just return false.

knoxcard commented at 8:33 AM on October 14, 2017:

if (!AcceptBlockHeader(header, state, chainparams, &pindex, pindex)) return false;

knoxcard approved
theuni commented at 4:41 PM on October 19, 2017: member

Needs rebase
laanwj added the label Needs backport on Oct 19, 2017
TheBlueMatt commented at 9:15 PM on October 19, 2017: member

See #11531 for a (likely better) version of this.
achow101 commented at 4:10 AM on October 25, 2017: member

Should this be closed in favor of #11531?
TheBlueMatt closed this on Oct 25, 2017
MarcoFalke removed the label Needs backport on Oct 25, 2017
MarcoFalke removed this from the milestone 0.15.0.2 on Oct 25, 2017
MarcoFalke removed this from the "Review priority 0.15.0.2" column in a project

laanwj referenced this in commit cffa5ee132 on Nov 1, 2017
codablock referenced this in commit b246b58720 on Sep 26, 2019
codablock referenced this in commit 7d21a78fa1 on Sep 29, 2019
barrystyle referenced this in commit 95b0dcef19 on Jan 22, 2020
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#11531 Check that new headers are not a descendant of an invalid block (more effeciently)