Return BlockValidationState from ProcessNewBlock if CheckBlock/AcceptBlock fails #17479

jnewbery · 2019-11-14T17:17:43Z

Net processing now passes a BlockValidationState object into ProcessNewBlock(). If CheckBlock() or AcceptBlock() fails, then PNB returns to net processing without calling the (asynchronous) BlockChecked Validation Interface method. net processing can use the invalid BlockValidationState returned to punish peers.

CheckBlock() and AcceptBlock() represent the DoS checks on a block (ie PoW and malleability). Net processing wants to know about those failed checks immediately and shouldn't have to wait on a callback. Other validation interface clients don't care about net processing submitting bogus malleated blocks to validation, so they don't need to be notified of BlockChecked.

Furthermore, if PNB returns a valid BlockValidationState, we never need to try to process (non-malleated) copies of the block from other peers. That makes it much easier to move the best chain activation logic to a background thread in future work.

jnewbery · 2019-11-14T17:18:17Z

This is the first two commits from #16279. Thanks for your work, @TheBlueMatt!

DrahtBot · 2019-11-14T19:15:18Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

[net processing] Move Misbehaving() to PeerManager #19791 ([net processing] Move Misbehaving() to PeerManager by jnewbery)
[doc] explain why CheckBlock() is called before AcceptBlock #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.

jnewbery · 2019-11-14T21:19:41Z

I've added a commit that deduplicates post-block-checking code, which makes the main commit in this PR smaller and (I hope) easier to understand.

jnewbery · 2019-11-14T22:28:29Z

I've taken the final (behaviour change) commit from #16279 and PR'ed it as #17485. That gives some justification for this PR. If we get the anti-dos / mutation checks back from validation immediately, we can use that state to mark the block as no longer in-flight, rather than by indirectly checking that the block has been written to disk.

Separating into two PRs makes this easier to review. This PR should result in no behaviour changes (modulo the BlockChecked() callback in net processing now being called immediately instead of on the background scheduler thread), and the follow-up PR has the (minor) behaviour change along with lots of commenting and justification.

dongcarl · 2019-11-15T21:06:51Z

I found the last commit in this PR (7186766) a bit hard to parse, so I split it up in my branch here: master...dongcarl:2019-11-processnewblock-early-return-redo-last. The main change in the last commit on this branch resides in f46102b on mine, and f46102b also retains the same commit message. Feel free to use/use only to review/not use.

dongcarl · 2019-11-15T21:07:35Z

src/validation.h

+ * a CValidationInterface (see validationinterface.h) - this will have its
+ * BlockChecked method called whenever *any* block completes validation (note


Perhaps I'm understanding wrong but this shouldn't apply anymore?

BlockChecked is called for any block that we try to connect to the chain. See the call in ConnectTip():

if the call to ConnectBlock() failed, then we'll call BlockChecked() with a state that is NOT IsValid().

if the call to ConnectBlock() succeeded, then the block has been connected and is part of the main chain, and we'll call BlockChecked() with a state that IS IsValid().

Note that we may never even call ConnectTip() if we don't try to connect the block to the most work chain.

jnewbery · 2019-11-15T22:19:05Z

Thanks for the branch @dongcarl ! I've reset to that and made a couple of minor comment changes.

ariard

Concept ACK, I think commit a49a153 can be made a bit better.

src/rpc/mining.cpp

src/net_processing.cpp

ariard · 2019-11-18T20:03:34Z

src/net_processing.cpp

+        // This clears the block from mapBlockSource.
+        BlockChecked(*pblock, state, connman);
+    } else if (!new_block) {
+        // Block was valid but we've seen it before. Clear it from mapBlockSource.
        LOCK(cs_main);
        ::mapBlockSource.erase(pblock->GetHash());


Can't this branch combined with BlockChecked by passing new_block as param? Even wonder if we still need fNewBlock in PNB, as BlockChecked erase block from mapBlockSource independently of validity/novelty

BlockChecked() will only be called from BlockProcessed() if the block failed CheckBlock()/AcceptBlock(). This else branch is catching the case where the block didn't fail, but we've seen it before.

If you think this can be cleaned up further, perhaps we can do it in a follow-up PR.

jkczyz

Concept ACK. My previous comments had been addressed in #16279. Additional commits look good. Built and ran unit and functional tests using commit 0bf87d2.

jkczyz · 2019-11-21T22:53:01Z

src/net_processing.cpp

+ *  A block has been processed. If the block was failed anti-dos / mutation checks, then
+ *  call BlockChecked() to maybe punish peer. If this is the first time we've seen the block,
 *  update the node's nLastBlockTime. Otherwise erase it from mapBlockSource.


The documentation strikes me as repetitive with the code. Perhaps it could be phrased differently such that if the code changes the comment doesn't need to be updated.

nit: s/was failed/failed

jnewbery · 2019-11-22T21:27:01Z

Thanks for the review @jkczyz . I've updated the function comment to be less redundant.

jnewbery · 2019-12-19T19:23:12Z

rebased

jonatack · 2020-07-08T06:37:47Z

Concept ACK, will review after rebase.

jnewbery · 2020-07-12T16:47:53Z

rebased

dongcarl · 2020-07-14T16:54:19Z

Took some time to prove to myself that there are no unintended behaviour changes. It wasn't obvious to me that 5e5d58c is trivially correct, so I'm most likely missing some context here.

My understanding is that prior to 5e5d58c, BlockChecked is only called when CheckBlock or AcceptBlock returns false in ProcessNewBlock, causing ProcessNewBlock to return early right after calling BlockChecked.

However, it seems that after 5e5d58c, it is possible for both CheckBlock and AcceptBlock to return true, but for dos_state.IsValid() to be false, causing BlockChecked to be called in BlockProcessed where it wouldn't have before. Perhaps this is fine and intended, but I just wanted to make sure.

Here's the diff I used for testing, and it seems to fail at validation_block_tests/processnewblock_signals_ordering:

diff --git a/src/validation.cpp b/src/validation.cpp
index 524ae94b9a..3f5737fd35 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -3856,9 +3856,11 @@ bool ChainstateManager::ProcessNewBlock(const CChainParams& chainparams, const s
             ret = ::ChainstateActive().AcceptBlock(pblock, dos_state, chainparams, &pindex, fForceProcessing, nullptr, fNewBlock);
         }
         if (!ret) {
+            assert(!dos_state.IsValid());  // We should always trigger BlockChecked in BlockProcessed in this codepath
             return error("%s: AcceptBlock FAILED (%s)", __func__, dos_state.ToString());
         }
     }
+    assert(dos_state.IsValid());  // We should NOT trigger BlockChecked in BlockProcessed in this codepath
 
     NotifyHeaderTip();

jnewbery · 2020-07-16T14:03:02Z

The first commit has now been merged in #19533. I've rebased the remaining commits on master.

jonatack

Almost ACK all commits except am still reviewing removal of the call to BlockChecked() in PNB in 372913c and other reasoning about this that @dongcarl states well in #17479 (comment). A few notes below. Code review, debug-built and ran tests at each commit; running a node on the branch ATM and observing behavior.

jonatack · 2020-07-24T16:55:13Z

src/validation.h

     */
-    bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<const CBlock> pblock, bool fForceProcessing, bool* fNewBlock) LOCKS_EXCLUDED(cs_main);
+    bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<const CBlock> pblock, BlockValidationState& state, bool fForceProcessing, bool* fNewBlock) LOCKS_EXCLUDED(cs_main);


1dd78f4

params order: can the [out] param state be inserted after the [in] params?

should @param[in] chainparams be added to the doxygen comment?

maybe I'm missing something obvious; any reason to not name state/dos_state consistently throughout, while adding it?

jonatack · 2020-07-24T17:02:42Z

src/net_processing.cpp

+/**
+ *  A block has been processed. Handle potential peer punishment and housekeeping.
+ */
+void static BlockProcessed(CNode& pfrom, CConnman& connman, CBlock& pblock, BlockValidationState& state, bool new_block)


In 6730b59 and 372913c, can doxygen documentation be added for BlockProcessed params as they are added?

jonatack · 2020-07-24T17:07:45Z

src/net_processing.cpp

+        LOCK(cs_main);
+        ::mapBlockSource.erase(pblock.GetHash());
+    } else {
+        // Block is valid and we haven't seen it before. set nLastBlockTime for this peer.


372913c nits:

slightly clearer, more differentiating wording could be s/we've/we have/ and
s/we haven't/we have not/

why is past tense used above ("block failed", "was valid") and here present tense ("is valid")

s/set/Set/

jonatack · 2020-07-24T17:13:31Z

src/validation.cpp


        // CheckBlock() does not support multi-threaded block validation because CBlock::fChecked can cause data race.
        // Therefore, the following critical section must include the CheckBlock() call as well.
        LOCK(cs_main);

        // Ensure that CheckBlock() passes before calling AcceptBlock, as
        // belt-and-suspenders.
-        bool ret = CheckBlock(*pblock, state, chainparams.GetConsensus());
+        bool ret = CheckBlock(*pblock, dos_state, chainparams.GetConsensus());


1dd78f4 Is there a reason to rename state to dos_state throughout this function? Doing so adds a fair amount of noise.

jonatack · 2020-07-24T17:16:41Z

src/validation.cpp

-    BlockValidationState state; // Only used to report errors, not invalidity - ignore it
-    if (!::ChainstateActive().ActivateBestChain(state, chainparams, pblock))
-        return error("%s: ActivateBestChain failed (%s)", __func__, state.ToString());
+    BlockValidationState dummy_state; // Only used to report errors, not invalidity - ignore it


5f552a0 This change could be done in the same commit 1dd78f4 that adds state/dos_state to the params. That said, is it strictly necessary? (a) The change adds noise and ISTM the comment and code position make it clear enough; (b) dummy is the kind of naming, like certain others (e.g. master/slave, whitelist/blacklist, etc.) that is perhaps a bit in decline nowadays and being deprecated, so perhaps no need to add it.

Co-authored-by: Carl Dong <contact@carldong.me>

This is a pure refactor commit. This commit enables the caller of ProcessNewBlock to access the final BlockValidationState passed around between CheckBlock(), AcceptBlock(), and BlockChecked() inside ProcessNewBlock(). This is useful because in a future commit, we will move the BlockChecked() call out of ProcessNewBlock(), and BlockChecked() still needs to be able to access the BlockValidationState. Co-authored-by: John Newbery <john@johnnewbery.com> Co-authored-by: Carl Dong <contact@carldong.me>

This is a pure refactor commit. Since BlockChecked() doesn't actually depend on all of PeerLogicValidation but just PeerLogicValidation's CConnman, we can make a standalone, static function that simply has an extra CConnman parameter and have the non-static version call the static one. This also means that, in a future commit, when we move the BlockChecked() call out of ProcessNewBlock(), the caller of ProcessNewBlock() can call BlockChecked() directly even if they only have a CConnman. Co-authored-by: John Newbery <john@johnnewbery.com> Co-authored-by: Carl Dong <contact@carldong.me>

…ProcessNewBlock Net processing now passes a BlockValidationState object into ProcessNewBlock(). If CheckBlock() or AcceptBlock() fails, then PNB returns to net processing without calling the (asynchronous) BlockChecked Validation Interface method. net processing can use the invalid BlockValidationState returned to punish peers. CheckBlock() and AcceptBlock() represent the DoS checks on a block (ie PoW and malleability). Net processing wants to know about those failed checks immediately and shouldn't have to wait on a callback. Other validation interface clients don't care about net processing submitting bogus malleated blocks to validation, so they don't need to be notified of BlockChecked. Furthermore, if PNB returns a valid BlockValidationState, we never need to try to process (non-malleated) copies of the block from other peers. That makes it much easier to move the best chain activation logic to a background thread in future work. Co-authored-by: John Newbery <john@johnnewbery.com> Co-authored-by: Carl Dong <contact@carldong.me>

This is a pure refactor commit. Co-authored-by: John Newbery <john@johnnewbery.com> Co-authored-by: Carl Dong <contact@carldong.me>

Co-authored-by: John Newbery <john@johnnewbery.com> Co-authored-by: Carl Dong <contact@carldong.me>

jnewbery · 2020-08-24T14:30:38Z

rebased

DrahtBot · 2020-09-07T17:41:21Z

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

jnewbery · 2020-09-16T09:21:37Z

I'm focused on other things right now, so I'm going to close this with the intention of opening it again at some point in the future.

jnewbery mentioned this pull request Nov 14, 2019

Return the AcceptBlock CValidationState directly in ProcessNewBlock #16279

Closed

fanquake added the Validation label Nov 14, 2019

jnewbery force-pushed the 2019-11-processnewblock-early-return branch from 111d7b4 to 7186766 Compare November 14, 2019 21:18

jnewbery mentioned this pull request Nov 14, 2019

WIP: net processing: Don't reach into CBlockIndex to check for block mutation #17485

Closed

dongcarl reviewed Nov 15, 2019

View reviewed changes

jnewbery force-pushed the 2019-11-processnewblock-early-return branch from 7186766 to 0bf87d2 Compare November 15, 2019 22:18

ariard reviewed Nov 18, 2019

View reviewed changes

jkczyz reviewed Nov 21, 2019

View reviewed changes

jnewbery force-pushed the 2019-11-processnewblock-early-return branch from 0bf87d2 to e48152f Compare November 22, 2019 21:26

DrahtBot added the Needs rebase label Dec 16, 2019

jnewbery force-pushed the 2019-11-processnewblock-early-return branch from e48152f to b2ee50d Compare December 19, 2019 19:23

DrahtBot removed the Needs rebase label Dec 19, 2019

DrahtBot mentioned this pull request Feb 28, 2020

validation: Templatize ValidationState instead of subclassing #17399

Merged

DrahtBot added the Needs rebase label Mar 1, 2020

jnewbery force-pushed the 2019-11-processnewblock-early-return branch from 50048f4 to cd4a987 Compare July 12, 2020 12:55

DrahtBot removed the Needs rebase label Jul 12, 2020

jnewbery force-pushed the 2019-11-processnewblock-early-return branch from cd4a987 to 6f1505d Compare July 12, 2020 16:44

This was referenced Jul 12, 2020

validation: Warm coins cache during prevalidation to connect blocks faster #19271

Closed

p2p: banscore updates to gui, tests, release notes #19512

Merged

DrahtBot added the Needs rebase label Jul 15, 2020

jnewbery mentioned this pull request Jul 16, 2020

[tests] Remove unnecessary cs_mains in denialofservice_tests #19533

Merged

jnewbery force-pushed the 2019-11-processnewblock-early-return branch from 6f1505d to 2adc864 Compare July 16, 2020 13:59

DrahtBot removed the Needs rebase label Jul 16, 2020

jonatack reviewed Jul 24, 2020

View reviewed changes

DrahtBot mentioned this pull request Aug 7, 2020

Run clang-tidy -*,performance-* #19675

Closed

DrahtBot mentioned this pull request Aug 20, 2020

Net processing: move ProcessMessage() to PeerLogicValidation #19704

Merged

jnewbery and others added 6 commits August 24, 2020 15:13

[net processing] Deduplicate post-block-processing code

a9141c4

Co-authored-by: Carl Dong <contact@carldong.me>

[validation] trivial: Rename state to dummy_state for clarity

9f8a81b

This is a pure refactor commit. Co-authored-by: John Newbery <john@johnnewbery.com> Co-authored-by: Carl Dong <contact@carldong.me>

[test/rpc] Additional checks for dos_state validity

17170a5

Co-authored-by: John Newbery <john@johnnewbery.com> Co-authored-by: Carl Dong <contact@carldong.me>

jnewbery force-pushed the 2019-11-processnewblock-early-return branch from 2adc864 to 17170a5 Compare August 24, 2020 14:30

DrahtBot mentioned this pull request Aug 24, 2020

[net processing] Move Misbehaving() to PeerManager #19791

Merged

DrahtBot added the Needs rebase label Sep 7, 2020

jnewbery closed this Sep 16, 2020

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

		* a CValidationInterface (see validationinterface.h) - this will have its
		* BlockChecked method called whenever any block completes validation (note

Return BlockValidationState from ProcessNewBlock if CheckBlock/AcceptBlock fails #17479

Return BlockValidationState from ProcessNewBlock if CheckBlock/AcceptBlock fails #17479

Uh oh!

Conversation

jnewbery commented Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Nov 14, 2019

Uh oh!

DrahtBot commented Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jnewbery commented Nov 14, 2019

Uh oh!

jnewbery commented Nov 14, 2019

Uh oh!

dongcarl commented Nov 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Nov 15, 2019

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Nov 22, 2019

Uh oh!

jnewbery commented Dec 19, 2019

Uh oh!

jonatack commented Jul 8, 2020

Uh oh!

jnewbery commented Jul 12, 2020

Uh oh!

dongcarl commented Jul 14, 2020

Uh oh!

jnewbery commented Jul 16, 2020

Uh oh!

jonatack 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!

jnewbery commented Aug 24, 2020

Uh oh!

DrahtBot commented Sep 7, 2020

Uh oh!

jnewbery commented Sep 16, 2020

Uh oh!

Uh oh!

jnewbery commented Nov 14, 2019 •

edited

Loading

DrahtBot commented Nov 14, 2019 •

edited

Loading

jonatack left a comment •

edited

Loading