Ensure nStatus is set properly for all invalid blocks

jamesob commented at 7:41 pm on February 10, 2018: member

Prerequisite PR: #12431 (it wouldn’t be catastrophic to merge this before that, but preferable to get that in first.)

In trying to fix the fork warning code (validation.cpp:CheckForkWarningConditions*), it became apparent that we are marking some (but not all) invalid blocks as invalid (via nStatus) when they are received and subsequently dropped. The fact that we never mark some invalid blocks as such prevents us from e.g. detecting and warning on invalid chains with significant work.

This change has ProcessNewBlock call InvalidateBlock on the invalid block to do the expected bookkeeping in mapBlockIndex before dropping the block.

This change also consolidates the setting of CBlockIndex’s nStatus |= BLOCK_FAILED_VALID to a single function (InvalidBlockFound) since there’s peripheral bookkeeping (e.g. g_failed_blocks.insert()) that we want to do consistently but is duplicated in some places or not done in other cases when it apparently should be.

One such replacement with InvalidBlockFound ensures addition of invalid blocks to g_failed_blocks and so (in theory) reduces CPU burden when being spammed with descendants of an invalid block, since we no longer have to walk mapBlockIndex to determine its invalidity. Based on reading usage of g_failed_blocks I can’t tell if this savings is real, but in any case it seems worthwhile to keep that set consistent (i.e. 1-1 with blocks marked BLOCK_FAILED_VALID since last restart).

jamesob commented at 9:38 pm on February 10, 2018: member

Hm: this changeset is failing tests that initially transmit a block with unexpected witness data (which gets rejected) and then retransmit without witness data for expected acceptance, but we get hung up on an existing mapBlockIndex entry that’s been marked invalid.

Is it worth special-casing out "unexpected-witness" rejections from the InvalidateBlock call? Seems a little ugly.

Is the bookkeeping this PR wants to do even worth it? I guess we could just lump long invalid forks in with long headers-only forks, but that doesn’t seem great either…

sipa commented at 9:40 pm on February 10, 2018: member

You can only mark a block as invalid when fPossibleCorruption is not set in the validation status.

jamesob force-pushed on Feb 10, 2018

jamesob commented at 9:49 pm on February 10, 2018: member

Ah good point, thanks @sipa. That seems to have fixed it.

jamesob force-pushed on Feb 10, 2018

dcousens approved

dcousens commented at 4:56 am on February 13, 2018: contributor

utACK 7bd2d85906f12661efb9e0747d8bbd30530d149d

fanquake added the label Validation on Feb 14, 2018

jamesob force-pushed on Feb 14, 2018

jamesob commented at 10:29 pm on February 14, 2018: member

I’ve rebased this changeset, removing the code that’s now in #12431 as well as the semi-related log statement. I’ve also DRY’d up parts of validation which set BLOCK_FAILED_VALID on nStatus to use InvalidBlockFound instead.

This change should only be merged after #12431 since this will exercise the bug that PR fixes.

jamesob force-pushed on Feb 14, 2018

in test/functional/test_framework/util.py:378 in e2be3284d6 outdated

372@@ -373,10 +373,13 @@ def sync_blocks(rpc_connections, *, wait=1, timeout=60):
373             if all(t["hash"] == tips[0]["hash"] for t in tips):
374                 return
375             raise AssertionError("Block sync failed, mismatched block hashes:{}".format(
376-                                 "".join("\n  {!r}".format(tip) for tip in tips)))
377+                "".join("\n  Node {}: {!r}".format(
378+                    rpc_connections[i].index, tip)
379+                        for i, tip in enumerate(tips))))

ryanofsky commented at 8:59 pm on March 6, 2018:

In commit “[tests] Add utility for generating bad coinbase transactions”

I think:

0(r.index, tip for r, tip in zip(rpc_connections, tips))

would be clearer than:

0(rpc_connections[i].index, tip for i, tip in enumerate(tips))

but you should ignore this suggestion if you prefer enumerate.

jamesob commented at 9:49 pm on March 6, 2018:

Agree, will do.

in test/functional/test_framework/blocktools.py:118 in e2be3284d6 outdated

111@@ -110,6 +112,16 @@ def create_transaction(prevtx, n, sig, value, scriptPubKey=CScript()):
112     tx.calc_sha256()
113     return tx
114 
115+
116+def create_coinbase_with_bad_txin(*args, **kwargs):
117+    """Return a coinbase CTransaction with an invalid txin."""
118+    kwargs['txin'] = CTxIn(

ryanofsky commented at 9:02 pm on March 6, 2018:

In commit “[tests] Add utility for generating bad coinbase transactions”

Could drop kwargs assignment with

0return create_coinbase(*args, **kwargs. txin=CTxIn(...))

jamesob commented at 9:49 pm on March 6, 2018:

Ah yep, that’s better.

jamesob commented at 5:13 pm on March 7, 2018:

Ah, Python 3.4 doesn’t support this syntax (which is what we run on Travis). Will have to revert this.

in src/validation.cpp:3428 in 6af17e4089 outdated

3424+            // Mark the block as invalid if we recognize it in mapBlockIndex.
3425+            // This doesn't happen within CheckBlock so we have to include a call
3426+            // here. It *does* happen within AcceptBlock below.
3427+            BlockMap::iterator it = mapBlockIndex.find(pblock->GetHash());
3428+
3429+            // Duplicate call to CorruptionPossible() here just to be careful.

ryanofsky commented at 9:24 pm on March 6, 2018:

In commit “Do the proper InvalidateBlock bookkeeping for DoSy blocks”

Duplicating the CorruptionPossible check here seems unnecessarily confusing and fragile to me, though perhaps there’s an advantage I’m not seeing.

jamesob commented at 10:07 pm on March 6, 2018:

Agree in hindsight that this was overly paranoid. Will remove.

ryanofsky commented at 9:44 pm on March 6, 2018: member

Slight utACK 6af17e40890651e5e5f5968a1a131c6a48ce998c. Slight because I don’t know all the implications of adding the new InvalidBlockFound calls.

New test code looks great.

I’d find the main commit (“Do the proper InvalidateBlock bookkeeping for DoSy blocks”) easier to understand if it were split up, with the actual behavior change in ProcessNewBlock in one commit, and all the documentation/assertlock/DRY cleanup stuff in a different commit or set of commits.

jamesob force-pushed on Mar 6, 2018

jamesob commented at 11:39 pm on March 6, 2018: member

@ryanofsky thanks very much for the review and good feedback. I’ve incorporated your recommendations and split out the last commit into two: one for the actual behavioral change and one for the cleanup.

jamesob force-pushed on Mar 7, 2018

in src/validation.cpp:2804 in bdfe29e043 outdated

2740@@ -2727,7 +2741,6 @@ bool CChainState::InvalidateBlock(CValidationState& state, const CChainParams& c
2741         it++;
2742     }
2743 
2744-    InvalidChainFound(pindex);

jamesob commented at 6:38 pm on March 7, 2018:

This now happens in the InvalidBlockFound call above.

MarcoFalke commented at 3:22 pm on March 8, 2018: member

Needs rebase to fix travis (sorry)

jamesob force-pushed on Mar 8, 2018

jamesob commented at 3:39 pm on March 8, 2018: member

Rebased.

ryanofsky commented at 10:09 pm on March 12, 2018: member

Slight re-utACK a12ce0dde462911bb702070d31ed28ba32353a05. Again someone more knowledgeable than me should review safety of adding new InvalidBlockFound calls in ProcessNewBlock, AcceptBlock, and InvalidateBlock.

Changes since last review were rebase, python cleanups, adding CorruptionPossible log print, and some suggestions from the last review.

You may want to update PR description since it will become part of git history if this is merged.

laanwj referenced this in commit 947c25ead2 on Mar 15, 2018

in src/validation.cpp:1290 in a12ce0dde4 outdated

1292-        setBlockIndexCandidates.erase(pindex);
1293-        InvalidChainFound(pindex);
1294+    AssertLockHeld(cs_main);
1295+
1296+    if (state.CorruptionPossible()) {
1297+        LogPrintf("%s: can't be called when corruption is possible (invalid block=%s)",

sipa commented at 0:17 am on March 17, 2018:

Why log this unconditionally? I think it may be trivial to cause someone’s logfile to be filled with these.

jamesob commented at 5:15 pm on March 23, 2018:

Yep, that’s a good point. I’ll change this into a comment.

jamesob force-pushed on Mar 23, 2018

jamesob commented at 7:32 pm on March 23, 2018: member

Thanks @ryanofsky and @sipa for the re-review. I’ve incorporated @sipa’s change (remove unconditional log in InvalidBlockFound when corruption is possible), rebased, and updated the PR description to better reflect the change’s contents. I’ve added some text discussing the refactoring, reproduced here

This change also consolidates the setting of CBlockIndex’s nStatus |= BLOCK_FAILED_VALID to a single function (InvalidBlockFound) since there’s peripheral bookkeeping (e.g. g_failed_blocks.insert()) that we want to do consistently but is duplicated in some places or not done in other cases when it apparently should be.

One such replacement with InvalidBlockFound ensures addition of invalid blocks to g_failed_blocks and so (in theory) reduces CPU burden when being spammed with descendants of an invalid block, since we no longer have to walk mapBlockIndex to determine its invalidity. Based on reading usage of g_failed_blocks I can’t tell if this savings is real, but in any case it seems worthwhile to keep that set consistent (i.e. 1-1 with blocks marked BLOCK_FAILED_VALID since last restart).

jamesob force-pushed on Mar 23, 2018

jamesob commented at 3:55 pm on May 2, 2018: member

Any hope of a concept ACK/NACK on this from @sipa @TheBlueMatt @sdaftuar?

in src/validation.cpp:2762 in 16d75a01b9 outdated

2735@@ -2724,6 +2736,11 @@ bool PreciousBlock(CValidationState& state, const CChainParams& params, CBlockIn
2736     return g_chainstate.PreciousBlock(state, params, pindex);
2737 }
2738 
2739+/**

laanwj commented at 2:09 pm on May 14, 2018:

The usual convention for method documentation in this project is to add the comment in the class definition, not above the implementation. This makes it easier to read the API documentation in one place when looking at the class. That said, as both are in the .cpp file for this class I don’t think that’s much of an issue here.

jamesob commented at 9:05 pm on May 21, 2018:

Fixed, thanks.

Empact commented at 7:23 am on May 18, 2018: member

Needs rebase

[tests] Add utility for generating bad coinbase transactions

Also adds a small logging clarification for `sync_blocks`. Includes feedback
from @ryanofsky.

6a369f1b0a

[tests] Add a failing test that asserts invalidity of DoS-responding blocks

Ensure that we mark any block which merits a DoS response as invalid, as well
as its associated chain tip. This will become important for detecting and
warning about long invalid forks.

8434fab3cc

Do the proper InvalidateBlock bookkeeping for DoSy blocks

When we receive a block which causes a DoS response, ensure that we mark its
status as "invalid" in `mapBlockIndex`. This is necessary for detecting and
warning about forks which are invalid but long.

1b922fdce6

jamesob force-pushed on May 21, 2018

Refactoring/DRYing for InvalidBlockFound

Consolidate BLOCK_FAILED_VALID nStatus updates to use InvalidBlockFound. Clean
up InvalidBlockFound for a defensive early exit if called when corruption is
possible.

29bd4800ce

jamesob force-pushed on May 21, 2018

jamesob commented at 9:07 pm on May 21, 2018: member

Rebased and addressed @laanwj’s feedback.

DrahtBot commented at 8:08 pm on June 15, 2018: member

DrahtBot added the label Needs rebase on Jun 15, 2018

jamesob commented at 8:08 pm on June 18, 2018: member

Closing this; isn’t getting much traction and no point in rebasing if that’s the case.

jamesob closed this on Jun 18, 2018

Mengerian referenced this in commit d28f9f8d88 on Aug 6, 2019

laanwj removed the label Needs rebase on Oct 24, 2019

PastaPastaPasta referenced this in commit ace8fb8abc on Jun 10, 2020

PastaPastaPasta referenced this in commit 36523d13df on Jun 13, 2020

PastaPastaPasta referenced this in commit 62e36f39b3 on Jun 13, 2020

PastaPastaPasta referenced this in commit a22d90420c on Jun 13, 2020

PastaPastaPasta referenced this in commit c1f34f70cd on Jun 17, 2020

BrannonKing referenced this in commit e607b86812 on Sep 4, 2020

BrannonKing referenced this in commit 60af36c34b on Sep 4, 2020

jonspock referenced this in commit 3c88c4fa73 on Sep 30, 2020

jonspock referenced this in commit a3b64bc8f6 on Oct 5, 2020

jonspock referenced this in commit da59fa5680 on Oct 10, 2020

DrahtBot locked this on Dec 16, 2021

Ensure nStatus is set properly for all invalid blocks #12407