Ensure nStatus is set properly for all invalid blocks #12407

jamesob · 2018-02-10T19:41:13Z

Prerequisite PR: Only call NotifyBlockTip when chainActive changes #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 · 2018-02-10T21:38:57Z

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 · 2018-02-10T21:40:59Z

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

jamesob · 2018-02-10T21:49:50Z

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

dcousens

utACK 7bd2d85906f12661efb9e0747d8bbd30530d149d

jamesob · 2018-02-14T22:29:44Z

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.

ryanofsky

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.

ryanofsky · 2018-03-06T20:59:36Z

test/functional/test_framework/util.py

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.

Agree, will do.

ryanofsky · 2018-03-06T21:02:17Z

test/functional/test_framework/blocktools.py

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

Could drop kwargs assignment with

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

Ah yep, that's better.

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

ryanofsky · 2018-03-06T21:24:21Z

src/validation.cpp

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.

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

jamesob

@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 · 2018-03-06T21:49:42Z

test/functional/test_framework/blocktools.py

Ah yep, that's better.

jamesob · 2018-03-06T21:49:58Z

test/functional/test_framework/util.py

Agree, will do.

jamesob · 2018-03-06T22:07:49Z

src/validation.cpp

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

jamesob · 2018-03-07T18:38:24Z

src/validation.cpp

This now happens in the InvalidBlockFound call above.

maflcko · 2018-03-08T15:22:27Z

Needs rebase to fix travis (sorry)

jamesob · 2018-03-08T15:39:03Z

Rebased.

ryanofsky · 2018-03-12T22:09:51Z

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.

f98b543 Only call NotifyBlockTip when the active chain changes (James O'Beirne) 152b7fb [tests] Add a (failing) test for waitforblockheight (James O'Beirne) Pull request description: This is a subset of the more controversial #12407, but this also adds a test demonstrating the bug. In InvalidateBlock, we're calling NotifyBlockTip with the now-invalid block's prev regardless of what chain the ancestor block is on. This could create numerous issues, but it at least screws up `waitforblockheight` (or anything else relying on `rpc/blockchain.cpp:latestblock`) when InvalidateBlock is called on a block not in chainActive, which can happen via RPC. Only call NotifyBlockTip when the block being marked invalid is on the active chain. Tree-SHA512: 9a54fe5e8c7eb489daf5df4483c0986129e871e2ca931a456ba869ecb5d5a8d4f7bd27ccc9e711e9292c9ed79ddef896c85d0e81fc76883503e327995b0e914f

sipa · 2018-03-17T00:17:12Z

src/validation.cpp

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

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

jamesob · 2018-03-23T19:32:02Z

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 · 2018-05-02T15:55:43Z

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

laanwj · 2018-05-14T14:09:39Z

src/validation.cpp

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.

Fixed, thanks.

Empact · 2018-05-18T07:23:13Z

Needs rebase

@ryanofsky

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

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

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.

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

jamesob · 2018-05-21T21:07:20Z

Rebased and addressed @laanwj's feedback.

DrahtBot · 2018-06-15T20:08:51Z

Needs rebase

jamesob · 2018-06-18T20:08:13Z

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

Summary: f98b543 Only call NotifyBlockTip when the active chain changes (James O'Beirne) 152b7fb [tests] Add a (failing) test for waitforblockheight (James O'Beirne) Pull request description: This is a subset of the more controversial bitcoin/bitcoin#12407, but this also adds a test demonstrating the bug. In InvalidateBlock, we're calling NotifyBlockTip with the now-invalid block's prev regardless of what chain the ancestor block is on. This could create numerous issues, but it at least screws up `waitforblockheight` (or anything else relying on `rpc/blockchain.cpp:latestblock`) when InvalidateBlock is called on a block not in chainActive, which can happen via RPC. Only call NotifyBlockTip when the block being marked invalid is on the active chain. Tree-SHA512: 9a54fe5e8c7eb489daf5df4483c0986129e871e2ca931a456ba869ecb5d5a8d4f7bd27ccc9e711e9292c9ed79ddef896c85d0e81fc76883503e327995b0e914f Backport of Core PR12431 https://github.com/bitcoin/bitcoin/pull/12431/files Completes T680 Test Plan: ``` make check test_runner.py ``` Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D3748

f98b543 Only call NotifyBlockTip when the active chain changes (James O'Beirne) 152b7fb [tests] Add a (failing) test for waitforblockheight (James O'Beirne) Pull request description: This is a subset of the more controversial bitcoin#12407, but this also adds a test demonstrating the bug. In InvalidateBlock, we're calling NotifyBlockTip with the now-invalid block's prev regardless of what chain the ancestor block is on. This could create numerous issues, but it at least screws up `waitforblockheight` (or anything else relying on `rpc/blockchain.cpp:latestblock`) when InvalidateBlock is called on a block not in chainActive, which can happen via RPC. Only call NotifyBlockTip when the block being marked invalid is on the active chain. Tree-SHA512: 9a54fe5e8c7eb489daf5df4483c0986129e871e2ca931a456ba869ecb5d5a8d4f7bd27ccc9e711e9292c9ed79ddef896c85d0e81fc76883503e327995b0e914f

From bitcoin/bitcoin#12407

Summary: f98b54352 Only call NotifyBlockTip when the active chain changes (James O'Beirne) 152b7fb25 [tests] Add a (failing) test for waitforblockheight (James O'Beirne) Pull request description: This is a subset of the more controversial bitcoin/bitcoin#12407, but this also adds a test demonstrating the bug. In InvalidateBlock, we're calling NotifyBlockTip with the now-invalid block's prev regardless of what chain the ancestor block is on. This could create numerous issues, but it at least screws up `waitforblockheight` (or anything else relying on `rpc/blockchain.cpp:latestblock`) when InvalidateBlock is called on a block not in chainActive, which can happen via RPC. Only call NotifyBlockTip when the block being marked invalid is on the active chain. Tree-SHA512: 9a54fe5e8c7eb489daf5df4483c0986129e871e2ca931a456ba869ecb5d5a8d4f7bd27ccc9e711e9292c9ed79ddef896c85d0e81fc76883503e327995b0e914f Backport of Core PR12431 https://github.com/bitcoin/bitcoin/pull/12431/files Completes T680 Test Plan: ``` make check test_runner.py ``` Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D3748

jamesob force-pushed the jamesob/2018-02-mark-headers-invalid branch from fe6c253 to 5ed8fe9 Compare February 10, 2018 21:48

jamesob force-pushed the jamesob/2018-02-mark-headers-invalid branch from 5ed8fe9 to 7bd2d85 Compare February 10, 2018 21:54

dcousens approved these changes Feb 13, 2018

View reviewed changes

jamesob mentioned this pull request Feb 14, 2018

Only call NotifyBlockTip when chainActive changes #12431

Merged

fanquake added the Validation label Feb 14, 2018

jamesob force-pushed the jamesob/2018-02-mark-headers-invalid branch from 7bd2d85 to 869f981 Compare February 14, 2018 22:23

jamesob force-pushed the jamesob/2018-02-mark-headers-invalid branch from 869f981 to 6af17e4 Compare February 14, 2018 22:37

ryanofsky reviewed Mar 6, 2018

View reviewed changes

jamesob force-pushed the jamesob/2018-02-mark-headers-invalid branch from 6af17e4 to 7d6e014 Compare March 6, 2018 23:38

jamesob commented Mar 6, 2018

View reviewed changes

jamesob force-pushed the jamesob/2018-02-mark-headers-invalid branch from 7d6e014 to bdfe29e Compare March 7, 2018 18:36

jamesob commented Mar 7, 2018

View reviewed changes

src/validation.cpp Outdated

Copy link

Contributor Author

jamesob Mar 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now happens in the InvalidBlockFound call above.

jamesob force-pushed the jamesob/2018-02-mark-headers-invalid branch from bdfe29e to a12ce0d Compare March 8, 2018 15:37

sipa reviewed Mar 17, 2018

View reviewed changes

jamesob force-pushed the jamesob/2018-02-mark-headers-invalid branch 2 times, most recently from c478e41 to 06a7c1e Compare March 23, 2018 17:17

jamesob force-pushed the jamesob/2018-02-mark-headers-invalid branch 3 times, most recently from b3eec70 to 16d75a0 Compare March 23, 2018 20:10

laanwj reviewed May 14, 2018

View reviewed changes

jamesob added 3 commits May 21, 2018 16:50

[tests] Add utility for generating bad coinbase transactions

6a369f1

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

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

8434fab

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

Do the proper InvalidateBlock bookkeeping for DoSy blocks

1b922fd

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.

jamesob force-pushed the jamesob/2018-02-mark-headers-invalid branch 3 times, most recently from 79f4e5f to ba1e6a0 Compare May 21, 2018 21:02

Refactoring/DRYing for InvalidBlockFound

29bd480

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

jamesob force-pushed the jamesob/2018-02-mark-headers-invalid branch from ba1e6a0 to 29bd480 Compare May 21, 2018 21:05

This was referenced Jun 5, 2018

rpc: Add submitheader #13399

Merged

Document validationinterace callback blocking deadlock potential. #13402

Merged

[net,mempool] Call AcceptToMemoryPool() asynchronously in p2p #13413

Closed

DrahtBot added the Needs rebase label Jun 15, 2018

jamesob closed this Jun 18, 2018

laanwj removed the Needs rebase label Oct 24, 2019

BrannonKing pushed a commit to lbryio/lbrycrd that referenced this pull request Sep 4, 2020

brought in a fix for keeping invalid blocks marked as such

e607b86

From bitcoin/bitcoin#12407

BrannonKing added a commit to lbryio/lbrycrd that referenced this pull request Sep 4, 2020

brought in a fix for keeping invalid blocks marked as such

60af36c

From bitcoin/bitcoin#12407

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Ensure nStatus is set properly for all invalid blocks #12407

Ensure nStatus is set properly for all invalid blocks #12407

Uh oh!

Conversation

jamesob commented Feb 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamesob commented Feb 10, 2018

Uh oh!

sipa commented Feb 10, 2018

Uh oh!

jamesob commented Feb 10, 2018

Uh oh!

dcousens left a comment

Choose a reason for hiding this comment

Uh oh!

jamesob commented Feb 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesob left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Mar 8, 2018

Uh oh!

jamesob commented Mar 8, 2018

Uh oh!

ryanofsky commented Mar 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesob commented Mar 23, 2018

Uh oh!

jamesob commented May 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Empact commented May 18, 2018

Uh oh!

jamesob commented May 21, 2018

Uh oh!

DrahtBot commented Jun 15, 2018

Uh oh!

jamesob commented Jun 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

jamesob commented Feb 10, 2018 •

edited

Loading

jamesob commented Feb 14, 2018 •

edited

Loading

ryanofsky left a comment •

edited

Loading