Call ProcessNewBlock() asynchronously

TheBlueMatt commented at 3:47 pm on July 2, 2019: member

This is a bit of a rework of #16175, which is a rework of #12934. Built on top of #16279.

This is a roughly-minimal patchset to get PNB processing blocks in the background in a clean way, however it doesn’t actually enable any material parallelism yet - it only sets up a structure for significantly untangling cs_main in coming work.

Next steps:

Notably, ProcessMessages will now call PNB, get a future for the block being fully processed, and then immediately block on cs_main waiting to process reject messages and other CNodeState data. This is rather easy to resolve using the new CPeerState (see comment about how we should slowly move things into CPeerState to reduce cs_main contention).
In parallel, PNB (and, thus, CheckBlock, and the pre-store AcceptBlock bits) should start losing cs_main, this is primarily doable by splitting mapBlockIndex out of cs_main (probably via something like mempool.cs, where a cs_blockindex acts as a read mutex while writers take cs_main. Given the prevalence of cs_main calls just to look up entries in mapBlockIndex, this should improve things pretty significantly.

DrahtBot commented at 4:38 pm on July 2, 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:

#16839 (Replace Connman and BanMan globals with Node local by ryanofsky)
#16834 (Fetch Headers over DNS by TheBlueMatt)
#16762 (Rust-based Backup over-REST block downloader by TheBlueMatt)
#16757 (doc: CChainState return values by MarcoFalke)
#16688 (log: Add validation interface logging by jkczyz)
#16442 (Serve BIP 157 compact filters by jimpo)
#16411 (Signet support by kallewoof)
#16333 (test: Set BIP34Height = 2 for regtest by MarcoFalke)
#16279 (Return the AcceptBlock CValidationState directly in ProcessNewBlock by TheBlueMatt)
#16202 (Refactor network message deserialization by jonasschnelli)
#15206 (Immediately disconnect on invalid net message checksum by jonasschnelli)
#14053 (Add address-based index (attempt 4?) by marcinja)
#8994 (Testchains: Introduce custom chain whose constructor… by jtimon)

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.

laanwj added the label Validation on Jul 2, 2019

TheBlueMatt force-pushed on Jul 2, 2019

TheBlueMatt force-pushed on Jul 3, 2019

in src/net_processing.cpp:3358 in 5517e78efa outdated

3347+
3348 bool PeerLogicValidation::ProcessMessages(CNode* pfrom, std::atomic<bool>& interruptMsgProc)
3349 {
3350     const CChainParams& chainparams = Params();
3351+    LOCK(cs_peerstate);
3352+    CPeerState* peerstate = PeerState(pfrom->GetId());

practicalswift commented at 10:19 am on July 10, 2019:

Can PeerState(pfrom->GetId()) return nullptr here?

in src/net_processing.cpp:3663 in 5517e78efa outdated

3652@@ -3531,6 +3653,13 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
3653         // If we get here, the outgoing message serialization version is set and can't change.
3654         const CNetMsgMaker msgMaker(pto->GetSendVersion());
3655 
3656+        LOCK(cs_peerstate);
3657+        CPeerState* peerstate = PeerState(pto->GetId());

practicalswift commented at 10:20 am on July 10, 2019:

Can PeerState(pfrom->GetId()) return nullptr here?

in src/validation.h:227 in 5517e78efa outdated

232  * @param[in]   pblock  The block we want to process.
233+ * @param[out] state This may be set to an Error state if any error occurred processing them
234  * @param[in]   fForceProcessing Process this block even if unrequested; used for non-network block sources and whitelisted peers.
235- * @param[out]  fNewBlock A boolean which is set to indicate if the block was first received via this call
236- * @return True if state.IsValid()
237+ * @return      A future which complets with a boolean which is set to indicate if the block was first received via this call

practicalswift commented at 10:20 am on July 10, 2019:

Should be “completes” :-)

fanquake added the label Needs Conceptual Review on Jul 10, 2019

laanwj commented at 10:39 am on July 10, 2019: member

This is a huge structural change. Please, let’s discuss concepts and concept ACK here first before jumping to small nits on the current implementation.

DrahtBot added the label Needs rebase on Jul 16, 2019

TheBlueMatt commented at 9:19 pm on July 18, 2019: member

Working on a big structural cleanup, will reopen something new once I have a new PR series.

TheBlueMatt closed this on Jul 18, 2019

TheBlueMatt reopened this on Jul 30, 2019

TheBlueMatt force-pushed on Jul 30, 2019

TheBlueMatt commented at 6:07 pm on July 30, 2019: member

Actually I was somewhat confused by some existing bugs triggering on travis. I think this is good as-is, original description applies.

DrahtBot removed the label Needs rebase on Jul 30, 2019

fanquake added this to the "Chasing Concept ACK" column in a project

in src/rpc/mining.cpp:137 in a7871c839e outdated

131@@ -132,7 +132,9 @@ 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+        ProcessNewBlock(Params(), shared_pblock, state, true).wait();
138+        if (ChainActive().Tip()->GetBlockHash() != pblock->GetHash())

promag commented at 1:17 am on August 6, 2019:

A wild block could be mined in between?

TheBlueMatt commented at 9:30 pm on August 6, 2019:

Right, but most likely something was mined before the new block got accepted, which is the right error for that. That said, I think there’s a bug here that ChainActive().Tip() needs cs_main, which isn’t held here.

promag commented at 1:20 am on August 6, 2019: member

Concept ACK, at least the idea to separate net and validation sounds reasonable to me.

DrahtBot added the label Needs rebase on Aug 15, 2019

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

688592f825

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

e416b66a60

Make ProcessNewBlock return a future instead of an immediate bool

This prepares for making best-chain-activation and disk writes
happen in a separate thread from the caller, even though all
callsites currently block on the return value immediately.

269a06007e

Add a new peer state tracking class to reduce cs_main contention.

CNodeState was added for validation-state-tracking, and thus,
logically, was protected by cs_main. However, as it has grown to
include non-validation state (taking state from CNode), and as
we've reduced cs_main usage for other unrelated things, CNodeState
is left with lots of cs_main locking in net_processing.

In order to ease transition to something new, this adds only a
dummy CPeerState which is held as a reference for the duration of
message processing.

Note that moving things is somewhat tricky pre validation-thread as
a consistent lockorder must be kept - we can't take a lock on the
new cs_peerstate in anything that's called directly from
validation.

49b17e5cc6

Move net_processing's ProcessNewBlock calls to resolve async.

Essentially, our goal is to not process anything for the given peer
until the block finishes processing (emulating the previous behavior)
without actually blocking the ProcessMessages loops. Obviously, in
most cases, we'll just go on to the next peer and immediately hit a
cs_main lock, blocking us anyway, but this we can slowly improve
that state over time by moving things from CNodeState to CPeerState.

56b98066d4

TheBlueMatt force-pushed on Aug 20, 2019

DrahtBot removed the label Needs rebase on Aug 20, 2019

Run the ActivateBestChain in ProcessNewBlock in a background thread

Spawn a background thread at startup which validates each block as
it comes in from ProcessNewBlock, taking advantage of the new
std::future return value to keep tests simple (and the new
net_processing handling of such values async already).

This makes introducing subtle validationinterface deadlocks much
harder as any locks held going into ProcessNewBlock do not interact
with (in the form of lockorder restrictions) locks taken in
validationinterface callbacks.

Note that after this commit, feature_block and feature_assumevalid
tests time out due to increased latency between block processing
when those blocks do not represent a new best block. This will be
resolved in the next commit.

6859b228bc

Add a callback to indicate a block has been processed

This resolves the performance regression introduced in the previous
commit by always waking the message processing thread after each
block future resolves.

Sadly, this is somewhat awkward - all other validationinterface
callbacks represent an actual change to the global validation state,
whereas this callback indicates only that a call which one
validation "client" made has completed. After going back and forth
for some time I didn't see a materially better way to resolve this
issue, and luckily its a rather simple change, but its far from
ideal. Note that because we absolutely do not want to ever block on
a ProcessNewBlock-returned-future, the callback approach is
critical.

e44b1e3785

Split AcceptBlock into three stages to write to disk in background

To keep the API the same (and for simplicity of clients, ie
net_processing), this splits AcceptBlock into the do-I-want-this
stage, the checking stage, and the writing stage.

ProcessNewBlock calls the do-I-want-this and checking (ie
malleability checking) stuff, and then dumps blocks that pass
into the background thread. In the background, we re-test the
do-I-want-this logic but skip the checking stuff, before writing
the block to disk and activating the best chain.

22400fbbde

Move BlockChecked to a background thread

As reject messages are required to go out in-order (ie before any
further messages are processed), this sadly requires that we
further delay re-enabling a peer after a block has been processed
by waiting for current validationinterface callbacks to drain.

This commit enables further reduction of cs_main in net_processing
by allowing us to lock cs_peerstate before cs_main in BlockChecked
(ie allows us to move things which are accessed in BlockChecked,
including DoS state and rejects into CPeerState and out of
CNodeState).

b4f3a33978

Move mapBlockSource to cs_peerstate from cs_main

This technically resolves a race where entries are added to
mapBlockSource before we know that they're non-malleated and then
removed only after PNB returns, though in practice this wasn't an
issue since all access to mapBlockSource already held cs_peerstate.

3a3b13e198

TheBlueMatt force-pushed on Aug 20, 2019

DrahtBot commented at 11:36 am on September 16, 2019: member

DrahtBot added the label Needs rebase on Sep 16, 2019

TheBlueMatt closed this on Nov 12, 2019

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

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

Marking up for grabs

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

<BlueMatt> #16323 and #16324 are up for grabs if anyone wants to work on them, but there seems to be ~zero interest in reviewing them, cause they have wonderfully scary titles (despite the code actually being pretty simple) :)

Also

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

<BlueMatt> jnewbery: note that “Add new peer state tracking class to reduce cs_main contention” only vaguely makes sense on its own - the vast majority of the contension is around cs_main for a reason…witout the rest of the patch to make ProcessNewBlock and the like not take cs_main it is a lot of code movement for not very much performance gain.

laanwj removed this from the "Chasing Concept ACK" column in a project

fanquake removed the label Needs rebase on May 13, 2020

fanquake removed the label Up for grabs on May 13, 2020

DrahtBot locked this on Feb 15, 2022

Call ProcessNewBlock() asynchronously #16323

Conflicts