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

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 0: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

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