Return BlockValidationState from ProcessNewBlock if CheckBlock/AcceptBlock fails

jnewbery commented at 5:17 pm on November 14, 2019: member

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 commented at 5:18 pm on November 14, 2019: member

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

fanquake added the label Validation on Nov 14, 2019

DrahtBot commented at 7:15 pm on November 14, 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:

#19791 ([net processing] Move Misbehaving() to PeerManager by jnewbery)
#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 force-pushed on Nov 14, 2019

jnewbery commented at 9:19 pm on November 14, 2019: member

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 commented at 10:28 pm on November 14, 2019: member

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 commented at 9:06 pm on November 15, 2019: member

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

in src/validation.h:154 in 7186766638 outdated

204- * its BlockChecked method called whenever *any* block completes validation.
205+ * Performs initial sanity checks using the provided BlockValidationState before
206+ * connecting any block(s). If you want to *possibly* get feedback on whether
207+ * pblock is valid beyond just cursory mutation/DoS checks, you must install
208+ * a CValidationInterface (see validationinterface.h) - this will have its
209+ * BlockChecked method called whenever *any* block completes validation (note

dongcarl commented at 9:07 pm on November 15, 2019:

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

jnewbery commented at 10:23 pm on November 15, 2019:

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 force-pushed on Nov 15, 2019

jnewbery commented at 10:19 pm on November 15, 2019: member

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

in src/rpc/mining.cpp:971 in a49a153f7c outdated

783@@ -784,6 +784,9 @@ static UniValue submitblock(const JSONRPCRequest& request)
784     if (!new_block && accepted) {
785         return "duplicate";
786     }
787+    if (!dos_state.IsValid()) {
788+        return BIP22ValidationResult(dos_state);

ariard commented at 7:50 pm on November 18, 2019:

submitblock_StateCacher is the last client of validationinterface, I think we can go further, tweak it a bit and remove definitely BlockChecked, see : https://github.com/ariard/bitcoin/commits/2019-11-processnewblock-review (but maybe to include this in a follow-up cleanup PR?)

jnewbery commented at 9:31 pm on November 18, 2019:

Yes, I was planning to do this as a follow-up PR.

I’ve had a quick look at your branch. I think it misses breaks the ‘promote to announcing compact blocks’ behaviour. We’d want to move that to a different validation interface event (probably UpdatedBlockTip())

ariard commented at 4:41 pm on November 19, 2019:

Ah yes, agree with you, better to do it in a follow-up PR to avoid breaking things inadvertently.

jnewbery commented at 9:21 pm on November 22, 2019:

Work in progress: https://github.com/jnewbery/bitcoin/tree/2019-11-remove-block-checked

in src/net_processing.cpp:2101 in a49a153f7c outdated

1897+void static BlockProcessed(CNode* pfrom, CConnman* connman, std::shared_ptr<CBlock> pblock, BlockValidationState& state, bool new_block)
1898 {
1899-    if (new_block) {
1900-        pfrom->nLastBlockTime = GetTime();
1901-    } else {
1902+    if (!state.IsValid()) {

ariard commented at 7:57 pm on November 18, 2019:

Isn’t this check break BlockChecked where we check state.IsValid for promoting peer to compact block announcement?

jnewbery commented at 9:25 pm on November 18, 2019:

No. BlockProcessed is only called when ProcessNewBlock() returns early because the block failed CheckBlock() or AcceptBlock(). The logic you’re talking about for promoting a peer to compact block announcement in BlockChecked() is only invoked when BlockChecked() is called after successfully connecting a valid block.

ariard commented at 4:38 pm on November 19, 2019:

Right, right, the call to BlockChecked is still in ConnectTip path on your branch. I still think that’s counter-intuitive to have both peer punishment and compact blocks promotion in one function, so would be down to split in a follow-up PR and like you said previously rely on UpdatedBlockTip for the latter.

in src/net_processing.cpp:2007 in a49a153f7c outdated

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

ariard commented at 8:03 pm on November 18, 2019:

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

jnewbery commented at 9:29 pm on November 18, 2019:

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.

ariard commented at 8:10 pm on November 18, 2019: member

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

in src/net_processing.cpp:1893 in a49a153f7c outdated

1887@@ -1888,16 +1888,23 @@ void static ProcessOrphanTx(CConnman* connman, std::set<uint256>& orphan_work_se
1888 }
1889 
1890 /**
1891- *  A block has been processed. If this is the first time we've seen the block,
1892+ *  A block has been processed. If the block was failed anti-dos / mutation checks, then
1893+ *  call BlockChecked() to maybe punish peer. If this is the first time we've seen the block,
1894  *  update the node's nLastBlockTime. Otherwise erase it from mapBlockSource.

jkczyz commented at 10:53 pm on November 21, 2019:

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

jkczyz commented at 10:57 pm on November 21, 2019: contributor

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

jnewbery force-pushed on Nov 22, 2019

jnewbery commented at 9:27 pm on November 22, 2019: member

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

DrahtBot added the label Needs rebase on Dec 16, 2019

jnewbery force-pushed on Dec 19, 2019

jnewbery commented at 7:23 pm on December 19, 2019: member

rebased

DrahtBot removed the label Needs rebase on Dec 19, 2019

DrahtBot added the label Needs rebase on Mar 1, 2020

jnewbery commented at 8:46 pm on March 13, 2020: member

rebased

jnewbery force-pushed on Mar 13, 2020

DrahtBot removed the label Needs rebase on Mar 13, 2020

DrahtBot added the label Needs rebase on Mar 16, 2020

jnewbery force-pushed on Mar 22, 2020

jnewbery commented at 2:18 am on March 22, 2020: member

Thanks :octopus: . I’ve rebased on master.

DrahtBot removed the label Needs rebase on Mar 22, 2020

DrahtBot added the label Needs rebase on Apr 8, 2020

jnewbery force-pushed on Apr 15, 2020

jnewbery commented at 8:52 pm on April 15, 2020: member

rebased

DrahtBot removed the label Needs rebase on Apr 15, 2020

DrahtBot added the label Needs rebase on Apr 16, 2020

jnewbery force-pushed on Apr 16, 2020

jnewbery commented at 7:33 pm on April 16, 2020: member

rebased

DrahtBot removed the label Needs rebase on Apr 16, 2020

DrahtBot added the label Needs rebase on May 12, 2020

jnewbery force-pushed on May 14, 2020

DrahtBot removed the label Needs rebase on May 14, 2020

jnewbery force-pushed on May 14, 2020

jnewbery commented at 8:34 pm on May 14, 2020: member

Oops. Bad rebase. Should be fixed now.

DrahtBot added the label Needs rebase on May 23, 2020

in src/validation.h:161 in db0eecc2bd outdated

156@@ -157,12 +157,13 @@ extern uint64_t nPruneTarget;
157  * May not be called in a
158  * validationinterface callback.
159  *
160- * @param[in]   pblock  The block we want to process.
161- * @param[in]   fForceProcessing Process this block even if unrequested; used for non-network block sources and whitelisted peers.
162- * @param[out]  fNewBlock A boolean which is set to indicate if the block was first received via this call
163- * @returns     If the block was processed, independently of block validity
164+ * @param[in]   pblock            The block we want to process.
165+ * @param[out]  state             Currently unused.

ariard commented at 1:31 am on May 26, 2020:

db0eecc

nit: Is there any guidelines on parameters sorting ? You can sort by param[in] then param[out]. It makes easier to reason on function. You can also precise in commit that new dos_state isn’t used.

in src/net_processing.cpp:1389 in a85e8b7393 outdated

1331@@ -1332,8 +1332,11 @@ void PeerLogicValidation::UpdatedBlockTip(const CBlockIndex *pindexNew, const CB
1332 /**
1333  * Handle invalid block rejection and consequent peer banning, maintain which
1334  * peers announce compact blocks.
1335+ * Called both in case of cursory DoS checks failing (implying the peer is likely
1336+ * sending us bogus data) and after full validation of the block fails (which may

ariard commented at 1:35 am on May 26, 2020:

a85e8b7

In ConnectTip, we call ConnectBlock then BlockChecked callback independently of success or failure of block validation ?

in src/validation.h:171 in 8fc192c3ec outdated

174+ *
175+ * May not be called in a validationinterface callback.
176  *
177  * @param[in]   pblock            The block we want to process.
178- * @param[out]  state             Currently unused.
179+ * @param[out]  state             Only used for failures in CheckBlock/AcceptBlock. For failure in block connection,

ariard commented at 1:39 am on May 26, 2020:

8fc192c

Is commit message right ? It mentions “Net processing now passes a BlockValidationState object into ProcessNewBlock()” but in fact it’s passing this object to BlockProcessed therefore making it used.

ariard commented at 1:42 am on May 26, 2020:

8fc192c

Still on commit message, “Net processing wants to know about those failed check immediately…” Can you explain why ? Overall, PR needs better motivation, I think you can point to changes in #18963, and the asynchronicity gain, instead of compact blocks ones.

in src/net_processing.cpp:2105 in 8fc192c3ec outdated

1992-    } else {
1993+    if (!state.IsValid()) {
1994+        // The block failed anti-dos / mutation checks. Call BlockChecked() callback here.
1995+        // This clears the block from mapBlockSource.
1996+        BlockChecked(*pblock, state, connman);
1997+    } else if (!new_block) {

ariard commented at 1:45 am on May 26, 2020:

8fc192c

How does this new check behaves with regards to write-failure, first-seen block ? In AcceptBlock, if blockPos returned by SaveBlockToDisk is null, an error is set and not an invalid state, so it won’t be erased by above BlockChecked call. I think it’s catch up in current code but not after changes.

in src/validation.h:157 in 8fc192c3ec outdated

155+ * pblock is valid beyond just cursory mutation/DoS checks, you must install
156+ * a CValidationInterface (see validationinterface.h) - this will have its
157+ * BlockChecked method called whenever *any* block completes validation (note
158+ * that any invalidity returned via state will *not* also be provided via
159+ * BlockChecked). There is, of course, no guarantee that any given block which
160+ * is not a part of the eventual best chain will ever be checked.

ariard commented at 1:49 am on May 26, 2020:

8fc192c

I think this whole comment can be made clearer.

“You have 2 types of feedbacks:

unconditional : submitted block validity with regards to mutation, anti-Dos is returned through BlockValidationState. It may be used for housekeeping as in BlockProcessed
conditional : if caller subscribe to CValidationInterface and if submitted block passed those first checks, ActivateBestChain, will be called to try to make progress towards the best known block and make it the tip of our block chain. Submitted block may be among such tip candidates. Consensus-validity feedback on best known block may be used for peer punishment and compact block promotion as in BlockChecked.”

ariard commented at 1:53 am on May 26, 2020: member

Re-Concept ACK 50048f4

I think well-commented this PR can be made easier for review. Note, switching from checking CheckBlock, AcceptBlock returned values to BlockValidationState for post-processing must be heeded, see comment on write-failure, first-seen block.

jonatack commented at 6:37 am on July 8, 2020: member

Concept ACK, will review after rebase.

jnewbery force-pushed on Jul 12, 2020

DrahtBot removed the label Needs rebase on Jul 12, 2020

jnewbery force-pushed on Jul 12, 2020

jnewbery commented at 4:47 pm on July 12, 2020: member

rebased

dongcarl commented at 4:54 pm on July 14, 2020: member

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

My understanding is that prior to 5e5d58c829001a774e2481a4a8c3598917b06e53, 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 5e5d58c829001a774e2481a4a8c3598917b06e53, 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:

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

DrahtBot added the label Needs rebase on Jul 15, 2020

jnewbery force-pushed on Jul 16, 2020

jnewbery commented at 2:03 pm on July 16, 2020: member

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

DrahtBot removed the label Needs rebase on Jul 16, 2020

in src/validation.h:892 in 2adc864a0e outdated

870+     * @param[in]   fForceProcessing  This block was requested from the peer or came from a non-network source.
871+     * @param[out]  fNewBlock         A boolean which is set to indicate if the block was first received via this call
872+     * @returns     bool              If the block was processed, independently of block validity
873      */
874-    bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<const CBlock> pblock, bool fForceProcessing, bool* fNewBlock) LOCKS_EXCLUDED(cs_main);
875+    bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<const CBlock> pblock, BlockValidationState& state, bool fForceProcessing, bool* fNewBlock) LOCKS_EXCLUDED(cs_main);

jonatack commented at 4:55 pm on July 24, 2020:

1dd78f41

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?

in src/net_processing.cpp:2099 in 2adc864a0e outdated

2017@@ -2011,6 +2018,25 @@ void static ProcessOrphanTx(CConnman& connman, CTxMemPool& mempool, std::set<uin
2018     }
2019 }
2020 
2021+/**
2022+ *  A block has been processed. Handle potential peer punishment and housekeeping.
2023+ */
2024+void static BlockProcessed(CNode& pfrom, CConnman& connman, CBlock& pblock, BlockValidationState& state, bool new_block)

jonatack commented at 5:02 pm on July 24, 2020:

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

in src/net_processing.cpp:2110 in 2adc864a0e outdated

2030+    } else if (!new_block) {
2031+        // Block was valid but we've seen it before. Clear it from mapBlockSource.
2032+        LOCK(cs_main);
2033+        ::mapBlockSource.erase(pblock.GetHash());
2034+    } else {
2035+        // Block is valid and we haven't seen it before. set nLastBlockTime for this peer.

jonatack commented at 5:07 pm on July 24, 2020:

372913c9df 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/

in src/validation.cpp:3845 in 2adc864a0e outdated

3852         LOCK(cs_main);
3853 
3854         // Ensure that CheckBlock() passes before calling AcceptBlock, as
3855         // belt-and-suspenders.
3856-        bool ret = CheckBlock(*pblock, state, chainparams.GetConsensus());
3857+        bool ret = CheckBlock(*pblock, dos_state, chainparams.GetConsensus());

jonatack commented at 5:13 pm on July 24, 2020:

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

in src/validation.cpp:3857 in 2adc864a0e outdated

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

jonatack commented at 5:16 pm on July 24, 2020:

5f552a0a34 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.

jonatack commented at 6:05 pm on July 24, 2020: member

Almost ACK all commits except am still reviewing removal of the call to BlockChecked() in PNB in 372913c9 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.

[net processing] Deduplicate post-block-processing code

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

a9141c43f7

[validation] Add BlockValidationState inout param to ProcessNewBlock

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>

03668d2dc2

[net processing] Make BlockChecked a standalone, static function

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>

89175a7e6d

[validation] Return the AcceptBlock BlockValidationState directly in 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>

935b7c9a5f

[validation] trivial: Rename state to dummy_state for clarity

This is a pure refactor commit.

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

9f8a81b837

[test/rpc] Additional checks for dos_state validity

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

17170a5dc3

jnewbery commented at 2:30 pm on August 24, 2020: member

rebased

jnewbery force-pushed on Aug 24, 2020

DrahtBot added the label Needs rebase on Sep 7, 2020

DrahtBot commented at 5:41 pm on September 7, 2020: member

🐙 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 commented at 9:21 am on September 16, 2020: member

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 closed this on Sep 16, 2020

DrahtBot locked this on Feb 15, 2022

Return BlockValidationState from ProcessNewBlock if CheckBlock/AcceptBlock fails #17479

Conflicts