Return the AcceptBlock CValidationState directly in ProcessNewBlock

TheBlueMatt commented at 7:25 pm on June 24, 2019: member

This is a first step towards a series of cleanups leading to a (more-complete) #16175. It’s just two rather-nice (IMO) cleanups.

In practice this means that CheckBlock+ContextualCheckBlock are called with a passed-in CValidationState before we move onto connecting the best chain. This makes conceptual sense as these calls represent the DoS checks on a block (ie PoW and malleability) which the caller almost certainly wants to know about right away and shouldn’t have to wait on a callback for (and other validationinterface clients shouldn’t care about someone submitting bogus malleated blocks to PNB).

This also makes it much, much easier to move the best chain activation logic to a background thread as it implies that if PNB returns with a IsValid() CValidationState we don’t need to care about trying to process (non-malleated) copies of the block from other peers.

DrahtBot added the label Mining on Jun 24, 2019

DrahtBot added the label P2P on Jun 24, 2019

DrahtBot added the label RPC/REST/ZMQ on Jun 24, 2019

DrahtBot added the label Tests on Jun 24, 2019

DrahtBot added the label Validation on Jun 24, 2019

DrahtBot commented at 0:13 am on June 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:

#17140 (test: Fix bug in blockfilter_index_tests. by jimpo)
#16411 (Signet support by kallewoof)
#15921 (validation: Tidy up ValidationState interface 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.

in src/net_processing.cpp:2894 in dd07ec726c outdated

2909@@ -2902,10 +2910,14 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
2910             // disk-space attacks), but this should be safe due to the
2911             // protections in the compact block handler -- see related comment
2912             // in compact block optimistic reconstruction handling.
2913-            ProcessNewBlock(chainparams, pblock, /*fForceProcessing=*/true, &fNewBlock);
2914-            if (fNewBlock) {
2915+            CValidationState dos_state;
2916+            ProcessNewBlock(chainparams, pblock, dos_state, /*fForceProcessing=*/true, &fNewBlock);

promag commented at 7:04 am on June 25, 2019:

Does it make sense to have a ProcessAndNotifyNewBlock to wrap this repeated code?

TheBlueMatt commented at 10:38 pm on June 25, 2019:

I’d prefer to just wait and clean it up in a later PR, given I want to move towards doing this in the background anyway.

ryanofsky commented at 9:14 pm on June 28, 2019:

re: #16279 (review)

In commit “Return the AcceptBlock CValidationState directly in ProcessNewBlock” (dd07ec726c5ade950435876d39741d52a33471ce)

I’d prefer to just wait and clean it up in a later PR, given I want to move towards doing this in the background anyway

Seems to me deduplicating this code now would only simplify the later PR.

TheBlueMatt commented at 2:22 am on July 2, 2019:

Right, but in the full branch I first clean up some cruft, and then merge the now-identical code instead of the currently-partially-identical code. Easier to just leave it as-is.

promag commented at 7:05 am on June 25, 2019: member

Concept ACK.

in src/validation.h:200 in dd07ec726c outdated

197@@ -198,22 +198,30 @@ static const uint64_t MIN_DISK_SPACE_FOR_BLOCK_FILES = 550 * 1024 * 1024;
198  * block is made active. Note that it does not, however, guarantee that the
199  * specific block passed to it has been checked for validity!
200  *
201- * If you want to *possibly* get feedback on whether pblock is valid, you must
202- * install a CValidationInterface (see validationinterface.h) - this will have
203- * its BlockChecked method called whenever *any* block completes validation.
204+ * Performs initial sanity checks using the provided CValidationState before
205+ * connecting any block(s). If you want to *possibly* get feedback on whether

ryanofsky commented at 8:25 pm on June 28, 2019:

In commit “Return the AcceptBlock CValidationState directly in ProcessNewBlock” (dd07ec726c5ade950435876d39741d52a33471ce)

Why is this saying “possibly get feedback”? If you check CValidationState and wait for BlockChecked, are there still cases where there wouldn’t be feedback? If so, it’d be good to give some hint of when or why this would happen. If not, I’d suggest changing “If you want to possibly get feedback” to “If you want to get feedback” and “you must install” to “you must also install”

in src/validation.h:209 in dd07ec726c outdated

209+ * that any invalidity returned via state will *not* also be provided via
210+ * BlockChecked).
211  *
212- * Note that we guarantee that either the proof-of-work is valid on pblock, or
213- * (and possibly also) BlockChecked will have been called.
214+ * If pblock connects, and has been malleated, state is guaranteed to be some

ryanofsky commented at 8:35 pm on June 28, 2019:

In commit “Return the AcceptBlock CValidationState directly in ProcessNewBlock” (dd07ec726c5ade950435876d39741d52a33471ce)

Could you clarify what malleated means here, or what case this is talking about? When is a block connected but also non valid?

TheBlueMatt commented at 2:27 am on July 2, 2019:

Maybe the confusion was the use of the old term “malleated” instead of the “mutated” term used in ValidationInvalidReason. I changed references to mutated.

in src/validation.h:213 in dd07ec726c outdated

214+ * If pblock connects, and has been malleated, state is guaranteed to be some
215+ * non-IsValid() state.
216  *
217- * May not be called in a
218- * validationinterface callback.
219+ * If fForceProcessing is set, barring pruning and a desire to re-download a

ryanofsky commented at 8:45 pm on June 28, 2019:

In commit “Return the AcceptBlock CValidationState directly in ProcessNewBlock” (dd07ec726c5ade950435876d39741d52a33471ce)

I don’t understand what this is saying. Is it just saying it would be wasteful to call with fForceProcessing set in this case, or something more? Would the recommendation be to call with fForceProcessing unset instead, or try to avoid calls entirely? Probably it would be clearer to say when fForceProcessing should be set rather than when it shouldn’t be set (if I’m not just completely misunderstanding point of this).

TheBlueMatt commented at 2:28 am on July 2, 2019:

This is saying something very specific - if fForceProcessing is set (but deliberately not saying anything about when you should set it, as that is a rather complicated topic), unless your goal is to re-download a block, you never need to call ProcessNewBlock again with the block. That is to say, “this copy of the block is good, no need to try to find an alternate source”, hence the somewhat verbose text. Feel free to suggest an alternative.

in src/net_processing.cpp:2809 in dd07ec726c outdated

2826             } else {
2827+                if (!dos_state.IsValid()) {
2828+                    BlockChecked(*pblock, dos_state, connman);
2829+                }
2830                 LOCK(cs_main);
2831                 mapBlockSource.erase(pblock->GetHash());

ryanofsky commented at 9:10 pm on June 28, 2019:

Why is mapBlockSource.erase call needed here? In not valid case, this should already happen in explicit BlockChecked call. In valid case, I assume it happens in BlockChecked validation callback?

This logic is messy and repetitive and seems like it could be simplified by moving more of it out of here into the BlockChecked() function, or breaking BlockChecked up into reusable parts.

TheBlueMatt commented at 2:30 am on July 2, 2019:

This PR removes the (now redundant) BlockChecked call in case a block is completely bogus (ie !dos_state.IsValid()), so, no.

jnewbery commented at 4:03 pm on October 9, 2019:

I think you’re right that we can’t get rid of this mapBlockSource.erase() call, but not for the reason you say. We still need to call it if the block is valid, but this isn’t the first time we’ve received it (ie fNewBlock is returned as true). I think this logic would be easier to follow if structured as follows, with comments:

 0        ProcessNewBlock(chainparams, pblock, dos_state, forceProcessing, &fNewBlock);
 1        if (!dos_state.IsValid()) {
 2            // The block failed anti-dos / mutation checks. Call BlockChecked() callback here.
 3            // This clears the block from mapBlockSource.
 4            BlockChecked(*pblock, dos_state, connman);
 5        } else if (!fNewBlock) {
 6            // Block was valid but we've seen it before. Clear it from mapBlockSource.
 7            LOCK(cs_main);
 8            mapBlockSource.erase(pblock->GetHash());
 9        } else {
10            // Block is valid and we haven't seen it before. set nLastBlockTime for this peer.
11            pfrom->nLastBlockTime = GetTime();
12        }

TheBlueMatt commented at 0:15 am on October 15, 2019:

Hmm, yea, thats a bit cleaner, however, after #16323, which this was refactored out of, the original goes back to being cleaner, so would prefer to just leave it (also cause its fewer LoC changed).

ryanofsky commented at 9:24 pm on June 28, 2019: member

Concept ACK. Changes look good and I should add a review ACK soon, but I need to dig in a little more to understand this better.

TheBlueMatt force-pushed on Jul 2, 2019

in src/validation.h:210 in 8d3ca746de outdated

210+ * BlockChecked). There is, of course, no guarantee that any given block which
211+ * is not a part of the eventual best chain will ever be checked.
212  *
213- * Note that we guarantee that either the proof-of-work is valid on pblock, or
214- * (and possibly also) BlockChecked will have been called.
215+ * If pblock connects, and has been mutation, state is guaranteed to be some

instagibbs commented at 2:54 pm on October 7, 2019:

s/has been mutation/has been mutated/

instagibbs commented at 2:55 pm on October 7, 2019:

what does “connects” mean here? If it’s not valid how does it connect?

TheBlueMatt commented at 1:50 am on October 8, 2019:

Good point, clarified it a bunch, though its pretty subtle. I hope its a bit better now.

in src/validation.h:215 in 8d3ca746de outdated

215+ * If pblock connects, and has been mutation, state is guaranteed to be some
216+ * non-IsValid() state.
217  *
218- * May not be called in a
219- * validationinterface callback.
220+ * If fForceProcessing is set (or fNewBlock returns true), barring pruning and

instagibbs commented at 2:56 pm on October 7, 2019:

move the parenthetical barring pruning and a desire... to after the last condition that needs to be satisfied state.IsValid() to have this part flow better.

TheBlueMatt commented at 1:48 am on October 8, 2019:

But the “barring pruning…” bit applies both to fForceProcessing and fNewBlock being set to true upon return? I think that would change the meaning.

jkczyz commented at 0:53 am on October 15, 2019:

I’m also having a tough time parsing this sentence. I’m reading it as:

If <X>, barring <Y>, if <Z>, there should never ...

The if <Z> part is throwing me off. Should there be an ‘and’ or an ‘or’ before it?

TheBlueMatt commented at 7:40 pm on October 15, 2019:

Hmm, maybe I misunderstood what @instagibbs was suggesting. Took the changes.

instagibbs commented at 7:49 pm on October 15, 2019:

I can’t remember what my suggestion meant but the new text reads much better

in src/validation.cpp:3431 in 8d3ca746de outdated

3427@@ -3428,14 +3428,13 @@ bool CChainState::AcceptBlock(const std::shared_ptr<const CBlock>& pblock, CVali
3428     return true;
3429 }
3430 
3431-bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<const CBlock> pblock, bool fForceProcessing, bool *fNewBlock)
3432+bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<const CBlock> pblock, CValidationState& state, bool fForceProcessing, bool *fNewBlock)

instagibbs commented at 3:23 pm on October 7, 2019:

let’s rename it to dos_state for clarity since it seems all callers(aside from dummy usage) calls it that.

TheBlueMatt commented at 1:50 am on October 8, 2019:

Done. Also fixed an issue where the FormatStateMessage in ActivateBestChain failure prints the state state, not the dummy_state state.

in src/validation.cpp:3777 in 8d3ca746de outdated

3456     NotifyHeaderTip();
3457 
3458-    CValidationState state; // Only used to report errors, not invalidity - ignore it
3459-    if (!::ChainstateActive().ActivateBestChain(state, chainparams, pblock))
3460+    CValidationState dummy_state; // Only used to report errors, not invalidity - ignore it
3461+    if (!::ChainstateActive().ActivateBestChain(dummy_state, chainparams, pblock))

instagibbs commented at 3:26 pm on October 7, 2019:

unrelated to PR but can state still be valid here?

in src/net_processing.cpp:1229 in 8d3ca746de outdated

1231@@ -1232,7 +1232,7 @@ void PeerLogicValidation::UpdatedBlockTip(const CBlockIndex *pindexNew, const CB
1232  * Handle invalid block rejection and consequent peer banning, maintain which
1233  * peers announce compact blocks.

instagibbs commented at 3:39 pm on October 7, 2019:

Suggested enhancement to comment:

“This is called after cursory DoS checks fail, and after full validation of the block in question.”

TheBlueMatt force-pushed on Oct 8, 2019

instagibbs commented at 3:30 pm on October 8, 2019: member

looks like it doesn’t compile anymore

TheBlueMatt force-pushed on Oct 8, 2019

TheBlueMatt commented at 8:10 pm on October 8, 2019: member

Right, just an issue triggered by the rebase-on-master testing. Rebased so should work now.

jnewbery commented at 8:39 pm on October 8, 2019: member

Concept ACK. Will review when the build is fixed.

TheBlueMatt commented at 11:03 pm on October 8, 2019: member

Built is fixed :)

in src/net_processing.cpp:2811 in 3f88fbdd66 outdated

2859                 LOCK(cs_main);
2860                 mapBlockSource.erase(pblock->GetHash());
2861             }
2862-            LOCK(cs_main); // hold cs_main for CBlockIndex::IsValid()
2863-            if (pindex->IsValid(BLOCK_VALID_TRANSACTIONS)) {
2864+            if (dos_state.IsValid()) {

jnewbery commented at 4:36 pm on October 9, 2019:

This seems like a behaviour change. If ConnectBlock() fails for this block, then pindex->nStatus will be updated with BLOCK_FAILED_VALID. Previously, that would prevent this branch from being executed, but now we’re only looking at the return CValidationState from CheckBlock() and AcceptBlock().

If this is an intentional behaviour change, can you pull it out into a separate commit and comment on why in the commit log?

TheBlueMatt commented at 0:28 am on October 15, 2019:

Don’t think it matters either way (though also don’t think it was intentional per se). It’s not so much an important change as it is an accidental tweak thats probably a bit better but shouldn’t matter. Once we get the block we have to mark it as received, but we cannot do that if the block is malleated. If the block is invalid, we don’t care too much - essentially anyone who sends us the block is gonna get the banhammer (if the block is not received via compact blocks).

jnewbery commented at 8:19 pm on October 15, 2019:

If it doesn’t matter either way, can you leave it unchanged? Trying to pick through what exactly MarkBlockAsReceived() is doing and whether changing whether it’s called or not is adding quite a lot of burden to review.

TheBlueMatt commented at 8:31 pm on October 15, 2019:

IMO, this is much cleaner, even if the behavior proper doesn’t matter. a) reaching into validation to figure out what happened after calling PNB is…a massive layer violation, this should be returned, not figured out by side-effects, b) it means one chunk of cs_main less (even if its only in the uncommon case), and c) it matters a lot for #16323.

jnewbery commented at 8:41 pm on October 15, 2019:

ok, if it does matter, can you separate into its own commit, with a commit log saying why it matters a lot :)

TheBlueMatt commented at 3:10 am on October 16, 2019:

Two different definitions of “matter”, I suppose. Anyway, I pulled it out on its own.

jonatack commented at 4:53 pm on October 16, 2019:

Tests that clarify and assert the points in this discussion would be reassuring.

ariard commented at 1:31 am on October 17, 2019:

I lean to agree that’s a minor change. I’m not even sure there is a difference between the 2 checks as BLOCK_VALID_TRANSACTION is set in ReceivedBlockTransactions itself called at the end of AcceptBlock after all checks modifying state to invalid. Even it’s lower the bar and increase the number of bogus blocks we download we’re going to ban peer through the BlockChecked callback. So if banhammer works bogus downloads are going to cease soon.

TheBlueMatt commented at 5:43 pm on October 17, 2019:

There really is no material difference in behavior here. Its much cleaner code, but the behavior is the same. Hence the two different definitions of “matter”.

in src/rpc/mining.cpp:136 in 3f88fbdd66 outdated

131@@ -132,7 +132,8 @@ static UniValue generateBlocks(const CScript& coinbase_script, int nGenerate, ui
132             continue;
133         }
134         std::shared_ptr<const CBlock> shared_pblock = std::make_shared<const CBlock>(*pblock);
135-        if (!ProcessNewBlock(Params(), shared_pblock, true, nullptr))
136+        CValidationState state;
137+        if (!ProcessNewBlock(Params(), shared_pblock, state, true, nullptr) || !state.IsValid())

jnewbery commented at 4:39 pm on October 9, 2019:

I don’t think || !state.IsValid() is necessary. ProcessNewBlock() cannot return true if state is invalid.

TheBlueMatt commented at 0:29 am on October 15, 2019:

Might very well be #16323 bleeding in, but hopefully that is required soon :).

in src/validation.h:213 in 81bc5bc52b outdated

213- * Note that we guarantee that either the proof-of-work is valid on pblock, or
214- * (and possibly also) BlockChecked will have been called.
215+ * If the block pblock is built on is in our header tree, and pblock is a
216+ * candidate for accepting to disk (either because it is a candidate for the
217+ * best chain soon, or due to fForceProcessing being set), but pblock has been
218+ * mutated, state is guaranteed to be some non-IsValid() state.

jkczyz commented at 10:38 pm on October 14, 2019:

It’s a little difficult to parse this sentence. It may be easier to understand if written a little more succinctly as:

0If the block that pblock is built on is in our header tree, and pblock is a
1candidate for accepting to disk (either because it is a candidate for the
2best chain or fForceProcessing is set) but has been mutated, then state is
3guaranteed to be non-IsValid().

Does that look equivalent?

TheBlueMatt commented at 7:32 pm on October 15, 2019:

Note that it doesn’t have to be a candidate for the best chain, as it can meet the “more or same work” criteria, hence the word “soon”. I took the drop of “due to”, but am not a huge fan of dropping the redundant subject after the parenthetical (as then you have to re-read skipping the parenthetical).

in src/validation.cpp:3770 in 81bc5bc52b outdated

3771+            ret = ::ChainstateActive().AcceptBlock(pblock, dos_state, chainparams, &pindex, fForceProcessing, nullptr, fNewBlock);
3772         }
3773         if (!ret) {
3774-            GetMainSignals().BlockChecked(*pblock, state);
3775-            return error("%s: AcceptBlock FAILED (%s)", __func__, FormatStateMessage(state));
3776+            return error("%s: AcceptBlock FAILED (%s)", __func__, FormatStateMessage(dos_state));

jkczyz commented at 10:57 pm on October 14, 2019:

Consider turning this into an if-else or if guard.

TheBlueMatt commented at 7:36 pm on October 15, 2019:

Hmm? Note that ret may get set to false in the if (ret) {} conditional above.

jkczyz commented at 8:28 pm on October 15, 2019:

Ah, missed that. Not sure if CheckBlock(...) && ::ChainstateActive().AcceptBlock(...) is any better, but feel free to leave it as is.

jkczyz commented at 9:43 pm on October 15, 2019:

Unrelated to this commit, the error here may be from either CheckBlock or AcceptBlock. Related to this commit, the returned CValidationState could also be from either rather than just AcceptBlock but the commit summary mentions only the latter.

I think this reflects a need for better naming / code organization rather than the need to make the commit message more verbose, however.

fjahr commented at 11:02 pm on October 15, 2019:

I missed it too :) Maybe consider not reusing the ret variable but giving the return of AcceptBlock a different name. check_ret and accept_ret for example.

amitiuttarwar commented at 11:36 pm on October 15, 2019:

but since this commit is touching this code, might be worth updating error message to “AcceptBlock or CheckBlock FAILED…” to be more accurate?

TheBlueMatt commented at 1:38 am on October 16, 2019:

Historically, CheckBlock was only called as a part of AcceptBlock, and I think we’d like to eventually go back there. See #9765 and the bitcoin-dev mail titled “Vulnerability relating to 64-byte transactions in Bitcoin Core 0.13 branch”. (sorry, lots and lots of context to this code…..)

in src/rpc/mining.cpp:745 in 81bc5bc52b outdated

748-    return BIP22ValidationResult(sc.state);
749+    if (dos_state.IsValid()) {
750+        return BIP22ValidationResult(sc.state);
751+    } else {
752+        return BIP22ValidationResult(dos_state);
753+    }

jkczyz commented at 1:09 am on October 15, 2019:

Consider the less verbose version:

0return BIP22ValidationResult(dos_state.IsValid() ? sc.state : dos_state);

Or use a guard clause to be consistent with the previous if:

0if (!dos_state.IsValid()) {
1    return BIP22ValidationResult(dos_state);
2}
3
4return BIP22ValidationResult(sc.state);

jkczyz commented at 4:28 am on October 15, 2019:

Actually, you can remove the duplicate check on dos_state.IsValid() like so:

0if (!dos_state.IsValid()) {
1    return BIP22ValidationResult(dos_state);
2}
3
4if (!sc.found) {
5    return "inconclusive";
6}
7
8return BIP22ValidationResult(sc.state);

jnewbery commented at 7:57 pm on October 15, 2019:

Much better. Can you do something similar here: #16279 (review) please :)

TheBlueMatt commented at 3:11 am on October 16, 2019:

Ugh, github’s review tools have gone to shit. I responded at #16279 (review)

in src/validation.h:220 in 3f88fbdd66 outdated

224+ * never be any reason to re-ProcessNewBlock any block with the same hash.
225+ *
226+ * May not be called in a validationinterface callback.
227  *
228  * @param[in]   pblock  The block we want to process.
229+ * @param[out] state This may be set to an Error state if any error occurred processing them

jkczyz commented at 4:23 am on October 15, 2019:

Nit: Alignment

Remove unnecessary cs_mains in denialofservice_tests

9fdf05d70cac4a62d1aeeb4299e2c3a9a866f8af resolved some lock
inversion warnings in denialofservice_tests, but left in a number
of cs_main locks that are unnecessary (introducing lock inversion
warnings in future changes).

81d2c312c3

TheBlueMatt force-pushed on Oct 15, 2019

in src/validation.cpp:3771 in 8f6a4a70bd outdated

3769             // Store to disk
3770-            ret = ::ChainstateActive().AcceptBlock(pblock, state, chainparams, &pindex, fForceProcessing, nullptr, fNewBlock);
3771+            ret = ::ChainstateActive().AcceptBlock(pblock, dos_state, chainparams, &pindex, fForceProcessing, nullptr, fNewBlock);
3772         }
3773         if (!ret) {
3774-            GetMainSignals().BlockChecked(*pblock, state);

jkczyz commented at 9:26 pm on October 15, 2019:

The commit message mentions this in an offhand sort of way. But it seems like this (i.e. removing the event notification) and the way callers handle the secondary return are the primary changes introduced in this commit. The commit log could better reflect this rather than describing the implementation mechanics (i.e. the ‘how’).

You could argue that removing the event notification should be a commit unto itself. Doing so would definitely make formulating the two commit summaries much easier.

TheBlueMatt commented at 1:44 am on October 16, 2019:

Clarified the commit message to make it more clear. Not gonna bother splitting as it really is the same idea - we return the dos_state instead of passing it back via a callback.

in src/rpc/mining.cpp:135 in 8f6a4a70bd outdated

131@@ -132,7 +132,8 @@ static UniValue generateBlocks(const CScript& coinbase_script, int nGenerate, ui
132             continue;
133         }
134         std::shared_ptr<const CBlock> shared_pblock = std::make_shared<const CBlock>(*pblock);
135-        if (!ProcessNewBlock(Params(), shared_pblock, true, nullptr))
136+        CValidationState state;

fjahr commented at 11:09 pm on October 15, 2019:

This one could also be named dos_state, I think?

fjahr commented at 0:06 am on October 16, 2019: member

tested ACK eabc9a192765df5b66fd4cdc65c4b645a39391cf

Reviewed code and ran tests + modified tests to try failure scenarios. While beyond the scope of this PR, something to note is that all the tests that were changed only tested the happy path. Something to improve in follow-ups, maybe after #16323.

Return the AcceptBlock CValidationState directly in ProcessNewBlock

In practice this means that CheckBlock+ContextualCheckBlock are
called with a passed-in CValidationState before we move onto
connecting the best chain. This makes conceptual sense as these
calls represent the DoS checks on a block (ie PoW and malleability)
which the caller almost certainly wants to know about right away
and shouldn't have to wait on a callback for.

Further, as other validationinterface clients shouldn't care about
someone submitting bogus malleated blocks to PNB, the BlockChecked
callback is no longer called in response to a malleated block (ie
it really does now mean "block has been checked as a part of
connecting it").

This also makes it much, much easier to move the best chain
activation logic to a background thread as it implies that if PNB
returns with a IsValid() CValidationState we don't need to care
about trying to process (non-malleated) copies of the block from
other peers.

c98e4d39f0

Use dos_state to check for malleation instead of CBlockIndex state c16e139246

Clarify comment in BlockChecked in net_processing a touch. 2ec121f09d

TheBlueMatt force-pushed on Oct 16, 2019

etscrivner commented at 3:04 am on October 16, 2019: contributor

tested ACK 2ec121f09d8f7117fc9a8f830a7242f9a3602b78.

Compiled and ran all of the tests. Ran a testnet node with this code for ~1 day without issue. Currently running one with the updated commits from today (will post if any issues).

jonatack commented at 4:34 pm on October 16, 2019: member

Concept ACK 2ec121f09d8f7117fc9a8f830a7242f9a3602b78. Changes seem ~ok, build/tests now green, bitcoind runs without apparent issues afaict. Will need to dig deeper into the code and context to add a code review ACK.

adamjonas commented at 6:06 pm on October 16, 2019: member

tested ACK 2ec121f. Compiled and ran unit and functional tests.

in src/net_processing.cpp:1231 in 2ec121f09d

1226@@ -1227,6 +1227,9 @@ void PeerLogicValidation::UpdatedBlockTip(const CBlockIndex *pindexNew, const CB
1227 /**
1228  * Handle invalid block rejection and consequent peer banning, maintain which
1229  * peers announce compact blocks.
1230+ * Called both in case of cursory DoS checks failing (implying the peer is likely
1231+ * sending us bogus data) and after full validation of the block fails (which may

ariard commented at 0:51 am on October 17, 2019:

Is second alternative of comment right ? “after full validation of the block fails” in fact it’s called in ConnectTip also when block is valid. Also comment should said what function is doing “Punish peer if sending bogus data unless it’s under BIP152 relaxation or eventually promote peer to announce blocks via compact blocks”

TheBlueMatt commented at 5:42 pm on October 17, 2019:

The existing comment already says that. “Handle invalid block rejection and consequent peer banning”.

in src/validation.h:217 in c98e4d39f0 outdated

219- * validationinterface callback.
220+ * If fForceProcessing is set (or fNewBlock returns true), and state.IsValid(),
221+ * barring pruning and a desire to re-download a pruned block, there should
222+ * never be any reason to re-ProcessNewBlock any block with the same hash.
223+ *
224+ * May not be called in a validationinterface callback.

ariard commented at 1:46 am on October 17, 2019:

Could you clarify the reasons? I think it’s due a lock conflict with cs_main but maybe you’re anticipating on the introduction of cs_peerstate

in src/validation.h:202 in c98e4d39f0 outdated

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

ariard commented at 2:03 am on October 17, 2019:

“The installed BlockChecked method will be called for any block completing validation. It may not be called for the provided pblock as this one not being part of the best chain. But if BlockChecked is called for it and its CValidationState is invalid, the invalidity reason will be different than the CValidationState got back from ProcessNewBlock`”. Just a verbose attempt to underscores clearly the fact you will get 2 different set of invalidity reasons.

in src/validation.h:223 in c98e4d39f0 outdated

225  *
226  * @param[in]   pblock  The block we want to process.
227+ * @param[out]  state This may be set to an Error state if any error occurred processing them
228  * @param[in]   fForceProcessing Process this block even if unrequested; used for non-network block sources and whitelisted peers.
229  * @param[out]  fNewBlock A boolean which is set to indicate if the block was first received via this call
230  * @returns     If the block was processed, independently of block validity

ariard commented at 2:12 am on October 17, 2019:

The whole comment could be clearer by defining what we mean by processed. Like “A processed block is one saved to disk and submit as candidate to be part of the best chain.” We can also add “passed the cursory mutation/Dos checks” if I’m correct will always return failed (non-processed) if block fail them. So both concepts are a bit mangled, as processed presume these preliminary checks as successful. For DoS storage risks, we don’t want to process blocks without basic safeguards.

in src/rpc/mining.cpp:744 in c98e4d39f0 outdated

740     UnregisterValidationInterface(&sc);
741     if (!new_block && accepted) {
742         return "duplicate";
743     }
744+    if (!dos_state.IsValid()) {
745+        return BIP22ValidationResult(dos_state);

ariard commented at 2:24 am on October 17, 2019:

nit: slight behavior change as now we return error on weak validity check, but would say it’s now more accurate as BlockChecked wasn’t giving state of the block we wait for.

TheBlueMatt commented at 5:44 pm on October 17, 2019:

Ehh, getblocktemplate is a mess already. TBH who really cares since no one actually uses it except to get a block template and submit a block. No one uses the return value.

ariard approved

ariard commented at 2:29 am on October 17, 2019: member

Code review ACK 2ec121f. Mostly suggestions to improve comments on a very mangled part of the codebase, had a look on following PRs, beyond expected performance improvements, it should be far easier to understand with a clean layer separation.

DrahtBot added the label Needs rebase on Oct 17, 2019

DrahtBot commented at 1:01 pm on October 17, 2019: member

laanwj removed the label P2P on Nov 1, 2019

laanwj removed the label RPC/REST/ZMQ on Nov 1, 2019

laanwj removed the label Mining on Nov 1, 2019

TheBlueMatt closed this on Nov 12, 2019

TheBlueMatt commented at 9:43 pm on November 12, 2019: member

Oops cause I didn’t comment, this kind of trivial refactor is probably detrimental in the death-by-1000-cuts model without #16323 and #16324, which seem to be too scary to get any review, so would recommend against running with it without reviving those (which are, of course, up for grabs if someone thinks they can make progress with it).

On Nov 12, 2019, at 15:19, Matt Corallo notifications@github.com wrote: Closed #16279.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

jnewbery commented at 10:16 pm on November 12, 2019: member

It’s very difficult for reviewers to understand the motivation and direction for this sequence of PRs, and consequently to help you move them forward toward merge. The OP for this PR states “It’s just two rather-nice (IMO) cleanups.”, but your final comment here is “would recommend against running with it without reviving those [other PRs]”. Those comments seem to me to be in direct contraction with each other. If the changes here are rather-nice cleanups, that suggests to me that they’re worth taking independently from #16323 and #16324.

TheBlueMatt commented at 11:18 pm on November 12, 2019: member

Right, but nice cleanups don’t always need to be merged, and do, by themselves, carry other weight. See the regular stream of “if this gets merged, X, Y, and Z will conflict” from Drahtbot (which is awesome). If they have a goal towards enabling other work, that’s great, but by themselves, unless they make it less likely some issue crops up in the future (which I kinda doubt for these changes), they’re often not worth merging.

On Nov 12, 2019, at 17:16, John Newbery notifications@github.com wrote:

It’s very difficult for reviewers to understand the motivation and direction for this sequence of PRs, and consequently to help you move them forward toward merge. The OP for this PR states “It’s just two rather-nice (IMO) cleanups.”, but your final comment here is “would recommend against running with it without reviving those [other PRs]”. Those comments seem to me to be in direct contraction with each other. If the changes here are rather-nice cleanups, that suggests to me that they’re worth taking independently from #16323 and #16324.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or unsubscribe.

ryanofsky commented at 4:57 pm on November 13, 2019: member

Marking up for grabs

http://www.erisian.com.au/bitcoin-core-dev/log-2019-11-12.html#l-455

<jnewbery> #16279 seems to have already had a lot of review. I liked everything except the last commit in that one, so I might grab those

ryanofsky added the label Up for grabs on Nov 13, 2019

TheBlueMatt commented at 6:26 pm on November 13, 2019: member

See above discussion. I don’t think that tag makes sense here.

On Nov 13, 2019, at 11:57, Russell Yanofsky notifications@github.com wrote:

Marking up for grabs

http://www.erisian.com.au/bitcoin-core-dev/log-2019-11-12.html#l-455

#16279 seems to have already had a lot of review. I liked everything except the last commit in that one, so I might grab those

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or unsubscribe.

ryanofsky removed the label Up for grabs on Nov 13, 2019

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

Rebased in #17479

MarcoFalke locked this on Dec 16, 2021

Return the AcceptBlock CValidationState directly in ProcessNewBlock #16279

Conflicts