Validation: Move CheckBlock() mutation guard to AcceptBlock()

jnewbery commented at 8:37 pm on November 25, 2019: member

We do not mark any blocks that fail CheckBlock() as BLOCK_FAILED_VALID since they could have been mutated and marking a valid-but-mutated block as invalid would prevent us from ever syncing to that chain. See https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-February/016697.html for full details.

The current guard against marking CheckBlock() failed blocks as invalid is by calling CheckBlock() prior to AcceptBlock() in ProcessNewBlock(). That is brittle since AcceptBlock() has an implicit assumption that any block submitted has been checked for mutation. A future change to ProcessNewBlock() could overlook that implicit assumption and introduce a consensus failure.

Move the mutation guard logic into AcceptBlock() and add comments to explain why we never mark CheckBlock() failed blocks as invalid.

fanquake added the label Validation on Nov 25, 2019

DrahtBot commented at 9:37 pm on November 25, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19271 (validation: Warm coins cache during prevalidation to connect blocks faster by andrewtoth)
#15545 ([doc] explain why CheckBlock() is called before AcceptBlock by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/validation.cpp:3805 in bdf7b9df31 outdated

3780@@ -3769,18 +3781,9 @@ bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<cons
3781         if (fNewBlock) *fNewBlock = false;
3782         BlockValidationState state;
3783 
3784-        // CheckBlock() does not support multi-threaded block validation because CBlock::fChecked can cause data race.

ariard commented at 6:26 pm on December 5, 2019:

If you keep CheckBlock as it is can we keep this to inform future refactors ?

jnewbery commented at 3:49 pm on March 13, 2020:

I think this should either:

be a comment in CheckBlock saying that cs_main should usually be held when calling CheckBlock
add an AssertLockHeld(cs_main) to CheckBlock (and also add that to FillBlock and update the callers in bench to hold the lock)
remove fChecked so that CheckBlock doesn’t need to hold cs_main

in src/validation.cpp:3768 in bdf7b9df31 outdated

3732+        // Never mark a block as invalid if CheckBlock() fails.  This is
3733+        // protective against consensus failure if there are any unknown forms
3734+        // of block mutation that cause CheckBlock() to fail; see e.g.
3735+        // CVE-2012-2459 and
3736+        // https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-February/016697.html.
3737+        // Because CheckBlock() is not very expensive, the anti-DoS benefits of

ariard commented at 6:39 pm on December 5, 2019:

You said here than not caching failure is okay because CheckBlock() isn’t very expensive but at same time we use fChecked to return early to avoid reprocessing. It seems a bit an inconsistent position. If it’s cheap enough we shouldn’t bother with fChecked and lock tacking shouldn’t cover CheckBlock? Or we could split CheckBlock between CheckBlockIntegrity and CheckBlockValidity and have a fDefinitelyInvalid to skip both if see block again?

jnewbery commented at 3:41 pm on March 13, 2020:

fChecked is a slightly different caching mechanism. It’s stored on the CBlock object and prevents having to do the merkle root and POW checking for the same object more than once. The CBlock object is new each time we redownload a block, so this caching doesn’t prevent us from re-checking an invalid block downloaded more than once. On the other hand, the BlockManager.m_block_index is what prevents us from checking an invalid block downloaded a second time.

We may actually be able to remove fChecked after this PR. Before this PR, we call CheckBlock three times on the same block:

from ProcessNewBlock()
from AcceptBlock()
from ConnectBlock()

(1) is removed by this PR and (3) is not on the critical path for compact block relay (since we relay the compact block as soon as we’ve done the merkle tree/pow checks the first time, and before we save to disk or connect the block).

in src/validation.cpp:3781 in c433cc4371 outdated

3780-        if (fNewBlock) *fNewBlock = false;
3781-        BlockValidationState state;
3782-
3783         LOCK(cs_main);
3784 
3785+        BlockValidationState state;

ariard commented at 6:42 pm on December 5, 2019:

nit: keep struct construction outside of cs_main, that’s still a concurrency

promag commented at 8:08 am on December 6, 2019:

that’s still a concurrency

What you mean?

jnewbery commented at 3:19 pm on March 13, 2020:

I think @ariard means that there’s no need to construct the BlockValidationState object within the cs_main scope. That’s true, but constructing this object is very cheap, so I don’t think it’s a problem (and placing the variable declaration next to where it’s used make this clearer).

I’m actually going to remove this commit from the PR, since I don’t think it’s necessary (and may make it more likely to introduce a bug, since the callers to ProcessNewBlock() in net_processing don’t check the return code of the function before using the fNewBlock out param.

ariard commented at 6:51 pm on December 5, 2019: member

Concept ACK.

Maybe hold for follow-up but did you look to remove CheckBlockHeader from CheckBlock given it’s called now in AcceptBlockHeader which is call in AcceptBlock ? I think that’s okay if we add a CheckBlockHeader in TestBlockValidity.

On the DoS-side we now have AcceptBlockHeader called before CheckBlock, I think that’s better given block header checks are cheaper than block ones.

promag commented at 8:09 am on December 6, 2019: member

Concept ACK.

[validation] Move CheckBlock() mutation guard to AcceptBlock()

We do not mark any blocks that fail CheckBlock() as BLOCK_FAILED_VALID
since they could have been mutated and marking a valid-but-mutated block
as invalid would prevent us from ever syncing to that chain. See
https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-February/016697.html
for full details.

The current guard against marking CheckBlock() failed blocks as invalid
is by calling CheckBlock() prior to AcceptBlock() in ProcessNewBlock().
That is brittle since AcceptBlock() has an implicit assumption that any
block submitted has been checked for mutation. A future change to
ProcessNewBlock() could overlook that implicit assumption and introduce
a consensus failure.

In this commit we move the mutation guard logic into AcceptBlock() and
add comments to explain why we never mark CheckBlock() failed blocks as
invalid.

a8dd24b647

[validation] Remove unused pindex outparam from AcceptBlock() eb3b20e157

jnewbery force-pushed on Mar 13, 2020

jnewbery commented at 5:04 pm on March 13, 2020: member

Rebased on latest master and removed the final commit. @ariard

Maybe hold for follow-up but did you look to remove CheckBlockHeader from CheckBlock

As you note, if we did that, we’d need to add CheckBlockHeader() to TestBlockValidity() and also CVerifyDB::VerifyDB(). I don’t think that counts as a simplification.

jnewbery commented at 5:54 am on July 9, 2020: member

Closing this for now. I think it’s the right change, but it’s not high priority and there doesn’t seem to be much interest.

jnewbery closed this on Jul 9, 2020

jonatack commented at 9:04 am on July 9, 2020: member

I agree this is a good change, as well as, in increasing order of preference, the items in your comment https://github.com/bitcoin/bitcoin/pull/17601/#discussion_r392311705.

ACK eb3b20e157f79da5afff18af30eb8cf224c980ff code review, built and tested after rebasing onto master.

jonatack commented at 9:08 am on July 9, 2020: member

GitHub doesn’t seem to parse its own link https://github.com/bitcoin/bitcoin/pull/17601/#discussion_r392311705 (https://github.com/bitcoin/bitcoin/pull/17601/#discussion_r392311705) correctly, so I meant the items in the following comment, in increasing order of preference:

I think this should either:

* be a comment in `CheckBlock` saying that `cs_main` should usually be held when calling `CheckBlock`

* add an `AssertLockHeld(cs_main)` to `CheckBlock` (and also add that to `FillBlock` and update the callers in bench to hold the lock)

* remove `fChecked` so that `CheckBlock` doesn't need to hold `cs_main`

jonatack commented at 9:25 am on July 9, 2020: member

I think it’s the right change, but it’s not high priority and there doesn’t seem to be much interest.

I empathise with this sentiment and have closed my own pull requests for the same reason, combined with the growing stack of open PRs needing attention and thinking I shouldn’t have too many PRs open. That said, unless a squeaky wheel calls for grease, review attention seems to be flood-or-drought. Dormant, then suddenly, unexpectedly present. So I guess what matters most is if the PR author is still interested in their own PR.

jnewbery commented at 9:35 am on July 9, 2020: member

This is consensus-critical and the changing the ordering of checks here could potentially introduce very subtle consensus failures, and so this PR requires very careful review. I have 9 other PRs open and more branches that I haven’t PRed and I’d prefer to focus review attention on those.

DrahtBot locked this on Feb 15, 2022

Validation: Move CheckBlock() mutation guard to AcceptBlock() #17601

Conflicts