Call ProcessNewBlock() asynchronously

TheBlueMatt commented at 11:51 am on June 9, 2019: member

This is essentially a rebase of #12934, though its actually greenfield to reduce diff a bit. Based on #16174 as without it the parallelism would be almost entirely useless. Note that this is draft as I haven’t (yet) bothered doing the backgrounding of the block validation, its currently just a return value change and the net_processing changes to handle it.

DrahtBot added the label Mining on Jun 9, 2019

DrahtBot added the label P2P on Jun 9, 2019

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

DrahtBot added the label Tests on Jun 9, 2019

DrahtBot added the label Validation on Jun 9, 2019

DrahtBot commented at 3:05 pm on June 9, 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:

#16202 (Refactor network message deserialization by jonasschnelli)
#16194 (refactor: share blockmetadata with BlockManager by jamesob)
#15921 (Tidy up ValidationState interface by jnewbery)
#15606 ([experimental] UTXO snapshots by jamesob)
#15545 ([doc] explain why CheckBlock() is called before AcceptBlock by Sjors)
#15505 ([p2p] Request NOTFOUND transactions immediately from other outbound peers, when possible by sdaftuar)
#15206 (Immediately disconnect on invalid net message checksum by jonasschnelli)
#14046 (net: Refactor message parsing (CNetMessage), adds flexibility by jonasschnelli)
#14032 (Add p2p layer encryption with ECDH/ChaCha20Poly1305 by jonasschnelli)
#9849 (Qt: Network Watch tool by luke-jr)

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 Jun 9, 2019

TheBlueMatt commented at 7:32 pm on June 9, 2019: member

Oops, it turns out its actually not possible to do #16174 first separately (and without it, this PR is entirely useless). By itself, #16174 ends up hitting the deadly cross-validationinterface-queue deadlock - it takes cs_peerstate first, then (eventually) calls PNB, which may call LimitValidationInterfaceQueue(). Meanwhile, in the validation queue, we may end up waiting on cs_peerstate to update some peer’s state. Of course, once we actually move block validation into its own thread, this is no longer (materially) a concern - we trivially guarantee there are no locks held going into LimitValidationInterfaceQueue().

TheBlueMatt force-pushed on Jun 9, 2019

TheBlueMatt marked this as ready for review on Jun 9, 2019

TheBlueMatt force-pushed on Jun 9, 2019

TheBlueMatt commented at 10:04 pm on June 9, 2019: member

Note that there is still a regression causing some functional tests to time out as we may end up waiting 100ms to discover that a block has been processed and we can process the next message from that peer (this shouldn’t be an issue in the case of downloading from multiple peers since we’ll always wake on a new tip block, and seems to not be an issue for one peer, even though its possible it would be due to a wake race). Not 100% sure what the best solution to that is.

jamesob commented at 2:38 am on June 11, 2019: member

Oops, it turns out its actually not possible to do #16174 first separately […] Meanwhile, in the validation queue, we may end up waiting on cs_peerstate to update some peer’s state.

For other reviewers: the particular validationinterface callback at fault here looks to be PeerLogicValidation::BlockChecked.

Big Concept ACK on the lock-splitting at the very least, though I do wish it could be done separately somehow. I’m a Concept-Concept ACK on this (in concept), but want to make sure I fully understand the implications.

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

9bc8ca61f5

Make ProcessNewBlock return a future instead of an immediate bool

This prepares for making ProcessNewBlock processing actually happen
in a separate thread from the caller, even though all callsites
currently block on the return value immediately.

Note that this makes some of the unit tests less restrictive.

79e6e446dd

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.

e89822964e

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.

8ddbb12678

Run ProcessNewBlock in a background thread (XXX: p2p_sendheaders broken)

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.

64e74ce8a9

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.

ba810b9e88

Move BlockChecked to a background thread 3499d988a9

Move nDoS counters to CPeerState (and, thus, out of cs_main) 445bb84bd3

Move rejects into cs_peerstate.

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

b6a05524f9

Actually parallelize validation somewhat 2d1e8f64cd

in src/net_processing.cpp:403 in 54b3431c40 outdated

398@@ -388,7 +399,20 @@ struct CNodeState {
399 // Keeps track of the time (in microseconds) when transactions were requested last time
400 limitedmap<uint256, int64_t> g_already_asked_for GUARDED_BY(cs_main)(MAX_INV_SZ);
401 
402+/** Note that this must be locked BEFORE cs_main! */
403+CCriticalSection cs_peerstate;