-
Notifications
You must be signed in to change notification settings - Fork 38.7k
Call ProcessNewBlock() asynchronously #16323
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
2f64e4a to
5517e78
Compare
src/net_processing.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can PeerState(pfrom->GetId()) return nullptr here?
src/net_processing.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can PeerState(pfrom->GetId()) return nullptr here?
src/validation.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be "completes" :-)
|
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. |
|
Working on a big structural cleanup, will reopen something new once I have a new PR series. |
5517e78 to
a7871c8
Compare
|
Actually I was somewhat confused by some existing bugs triggering on travis. I think this is good as-is, original description applies. |
promag
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK, at least the idea to separate net and validation sounds reasonable to me.
src/rpc/mining.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A wild block could be mined in between?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
9fdf05d 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).
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.
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.
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.
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.
a7871c8 to
e11110b
Compare
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.
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.
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.
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).
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.
e11110b to
3a3b13e
Compare
| Needs rebase |
|
Marking up for grabs
Also
|
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: