Ensure nStatus is set properly for all invalid blocks #12407

pull jamesob wants to merge 4 commits into bitcoin:master from jamesob:jamesob/2018-02-mark-headers-invalid changing 4 files +137 −65
  1. 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).

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

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

  4. jamesob force-pushed on Feb 10, 2018
  5. jamesob commented at 9:49 PM on February 10, 2018: member

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

  6. jamesob force-pushed on Feb 10, 2018
  7. dcousens approved
  8. dcousens commented at 4:56 AM on February 13, 2018: contributor

    utACK 7bd2d85906f12661efb9e0747d8bbd30530d149d

  9. jamesob cross-referenced this on Feb 14, 2018 from issue Only call NotifyBlockTip when chainActive changes by jamesob
  10. fanquake added the label Validation on Feb 14, 2018
  11. jamesob force-pushed on Feb 14, 2018
  12. 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.

  13. jamesob force-pushed on Feb 14, 2018
  14. 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:

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

    would be clearer than:

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

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

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

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

  17. ryanofsky commented at 9:44 PM on March 6, 2018: contributor

    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.

  18. jamesob force-pushed on Mar 6, 2018
  19. 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.

  20. jamesob force-pushed on Mar 7, 2018
  21. 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.

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

    Needs rebase to fix travis (sorry)

  23. jamesob force-pushed on Mar 8, 2018
  24. jamesob commented at 3:39 PM on March 8, 2018: member

    Rebased.

  25. ryanofsky commented at 10:09 PM on March 12, 2018: contributor

    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.

  26. laanwj referenced this in commit 947c25ead2 on Mar 15, 2018
  27. 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 12: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.

  28. jamesob force-pushed on Mar 23, 2018
  29. jamesob force-pushed on Mar 23, 2018
  30. 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).

  31. jamesob force-pushed on Mar 23, 2018
  32. jamesob force-pushed on Mar 23, 2018
  33. jamesob force-pushed on Mar 23, 2018
  34. jamesob commented at 3:55 PM on May 2, 2018: member

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

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

  36. Empact commented at 7:23 AM on May 18, 2018: member

    Needs rebase

  37. [tests] Add utility for generating bad coinbase transactions
    Also adds a small logging clarification for `sync_blocks`. Includes feedback
from @ryanofsky.
    6a369f1b0a
  38. [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
  39. 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
  40. jamesob force-pushed on May 21, 2018
  41. jamesob force-pushed on May 21, 2018
  42. jamesob force-pushed on May 21, 2018
  43. 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
  44. jamesob force-pushed on May 21, 2018
  45. jamesob commented at 9:07 PM on May 21, 2018: member

    Rebased and addressed @laanwj's feedback.

  46. DrahtBot cross-referenced this on Jun 5, 2018 from issue rpc: Add submitheader by MarcoFalke
  47. DrahtBot cross-referenced this on Jun 7, 2018 from issue Document validationinterace callback blocking deadlock potential. by TheBlueMatt
  48. DrahtBot cross-referenced this on Jun 7, 2018 from issue [net,mempool] Call AcceptToMemoryPool() asynchronously in p2p by skeees
  49. DrahtBot commented at 8:08 PM on June 15, 2018: contributor

    <!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase

  50. DrahtBot added the label Needs rebase on Jun 15, 2018
  51. 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.

  52. jamesob closed this on Jun 18, 2018
  53. Mengerian referenced this in commit d28f9f8d88 on Aug 6, 2019
  54. laanwj removed the label Needs rebase on Oct 24, 2019
  55. PastaPastaPasta referenced this in commit ace8fb8abc on Jun 10, 2020
  56. PastaPastaPasta referenced this in commit 36523d13df on Jun 13, 2020
  57. PastaPastaPasta referenced this in commit 62e36f39b3 on Jun 13, 2020
  58. PastaPastaPasta referenced this in commit a22d90420c on Jun 13, 2020
  59. PastaPastaPasta referenced this in commit c1f34f70cd on Jun 17, 2020
  60. BrannonKing referenced this in commit e607b86812 on Sep 4, 2020
  61. BrannonKing referenced this in commit 60af36c34b on Sep 4, 2020
  62. jonspock referenced this in commit 3c88c4fa73 on Sep 30, 2020
  63. jonspock referenced this in commit a3b64bc8f6 on Oct 5, 2020
  64. jonspock referenced this in commit da59fa5680 on Oct 10, 2020
  65. bitcoin locked this on Dec 16, 2021
Contributors
jamesobsipadcousensryanofskyMarcoFalkelaanwjEmpactDrahtBot
Labels
Validation
Linked (view graph)
#12431 Only call NotifyBlockTip when chainActive changes
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:55 UTC