Get cs_main out of the critical path in ProcessMessages

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

Built on #16323, this takes one next step on the first future-work bullet point listed there - we now do not immediately wait on cs_main after returning from ProcessMessage and can (maybe) get to another peer before blocking ProcessMessage waiting on ActivateBestChain to finish in the background. Sadly this isn’t expected to result in a material performance improvement on IBD until the next ProcessNewBlock calls’ CheckBlock can begin running without waiting on cs_main, which is not yet the case.

Further, this now moves CBlock::fChecked out of cs_main, making CheckBlock parallel.

DrahtBot commented at 4:37 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:

#16878 (Fix non-deterministic coverage of test DoS_mapOrphans by davereikher)
#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)
#16748 ([WIP] Add support for addrv2 (BIP155) by dongcarl)
#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)
#15545 ([doc] explain why CheckBlock() is called before AcceptBlock by Sjors)
#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.

TheBlueMatt force-pushed on Jul 2, 2019

TheBlueMatt force-pushed on Jul 3, 2019

DrahtBot added the label Mining on Jul 3, 2019

DrahtBot added the label P2P on Jul 3, 2019

DrahtBot added the label RPC/REST/ZMQ on Jul 3, 2019

DrahtBot added the label Tests on Jul 3, 2019

DrahtBot added the label Validation on Jul 3, 2019

TheBlueMatt force-pushed on Jul 3, 2019

in src/net_processing.cpp:3333 in c2814df836 outdated

3334+            // expected by some of our functional tests.
3335+            peerstate->pending_event_wait = true;
3336+            NodeId node_id = pfrom->GetId();
3337+            CallFunctionInValidationInterfaceQueue([node_id] {
3338+                LOCK(cs_peerstate);
3339+                CPeerState* peerstate = PeerState(node_id);

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

This peerstate shadows the peerstate in the outer context. Consider renaming :-)

promag commented at 6:05 pm on August 2, 2019:

It’s not captured so does is it really shadows?

in src/validation.cpp:3578 in c2814df836 outdated

3488+            fForceProcessing = std::get<1>(tuple);
3489+            result_promise = std::move(std::get<2>(tuple));
3490+            m_block_validation_queue.pop_front();
3491+        }
3492+
3493+        CChainParams chainparams = Params();

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

Could be const ref since only used as such?

DrahtBot added the label Needs rebase on Jul 16, 2019

TheBlueMatt commented at 9:20 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:08 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, modulo moving CheckBlock out of cs_main.

DrahtBot removed the label Needs rebase on Jul 30, 2019

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

fanquake removed the label Mining on Jul 30, 2019

fanquake removed the label RPC/REST/ZMQ on Jul 30, 2019

fanquake removed the label Tests on Jul 30, 2019

DrahtBot added the label Needs rebase on Aug 14, 2019

TheBlueMatt force-pushed on Aug 14, 2019

DrahtBot removed the label Needs rebase on Aug 14, 2019

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

Move nDoS counters to CPeerState (and, thus, out of cs_main) 1439f719d2

Move rejects into cs_peerstate.

This removes the cs_main lock which is taken on every
ProcessMessages call.

cbec3a36b2

Move blocks-in-flight related tracking from CNodeState to CPeerState

This moves one more group of variables out of cs_main. Importantly,
these moves allow us to do block processing almost entirely without
cs_main once mapBlockIndex moves to its own (read) lock.

ae11ddc3b2

Move CBlock::fChecked (and, thus, CheckBlock) out of cs_main

This has the effect of allowing CheckBlock to run in parallel with
other block connection things in some cases during IBD.

99108e2b0c

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 commented at 4:56 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) :)

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

fanquake removed the label Needs rebase on Aug 20, 2020

DrahtBot locked this on Feb 15, 2022

Get cs_main out of the critical path in ProcessMessages #16324

Conflicts