-
Notifications
You must be signed in to change notification settings - Fork 38.8k
[WIP] rebase: Call ProcessNewBlock() asynchronously #18963
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
|
Since this PR touches critical parts of the codebase, I think it deserves a thorough review. I'm going to go through the original PR (#16323) commit by commit and write down my notes here: https://docs.google.com/document/d/1tduRmqcvhdl3FRkmdnj0f3_fknXuIZ57R8zdCGHNt6k/edit?usp=sharing |
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).
Co-authored-by: Carl Dong <[email protected]>
This is a pure refactor commit. This commit enables the caller of ProcessNewBlock to access the final BlockValidationState passed around between CheckBlock(), AcceptBlock(), and BlockChecked() inside ProcessNewBlock(). This is useful because in a future commit, we will move the BlockChecked() call out of ProcessNewBlock(), and BlockChecked() still needs to be able to access the BlockValidationState. Co-authored-by: John Newbery <[email protected]> Co-authored-by: Carl Dong <[email protected]>
This is a pure refactor commit. Since BlockChecked() doesn't actually depend on all of PeerLogicValidation but just PeerLogicValidation's CConnman, we can make a standalone, static function that simply has an extra CConnman parameter and have the non-static version call the static one. This also means that, in a future commit, when we move the BlockChecked() call out of ProcessNewBlock(), the caller of ProcessNewBlock() can call BlockChecked() directly even if they only have a CConnman. Co-authored-by: John Newbery <[email protected]> Co-authored-by: Carl Dong <[email protected]>
…ProcessNewBlock Net processing now passes a BlockValidationState object into ProcessNewBlock(). If CheckBlock() or AcceptBlock() fails, then PNB returns to net processing without calling the (asynchronous) BlockChecked Validation Interface method. net processing can use the invalid BlockValidationState returned to punish peers. CheckBlock() and AcceptBlock() represent the DoS checks on a block (ie PoW and malleability). Net processing wants to know about those failed checks immediately and shouldn't have to wait on a callback. Other validation interface clients don't care about net processing submitting bogus malleated blocks to validation, so they don't need to be notified of BlockChecked. Furthermore, if PNB returns a valid BlockValidationState, we never need to try to process (non-malleated) copies of the block from other peers. That makes it much easier to move the best chain activation logic to a background thread in future work. Co-authored-by: John Newbery <[email protected]> Co-authored-by: Carl Dong <[email protected]>
This is a pure refactor commit. Co-authored-by: John Newbery <[email protected]> Co-authored-by: Carl Dong <[email protected]>
Co-authored-by: John Newbery <[email protected]> Co-authored-by: Carl Dong <[email protected]>
7efe6f9 to
178a758
Compare
|
Rebased on newer #17479. |
Test was relying on the fact that BlockChecked as synchronous and that commit made it asynchronous. You can switch it to a different synchronous method: --- a/src/test/validationinterface_tests.cpp
+++ b/src/test/validationinterface_tests.cpp
@@ -57,15 +57,13 @@ public:
{
if (m_on_destroy) m_on_destroy();
}
- void BlockChecked(const CBlock& block, const BlockValidationState& state) override
+ void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock> &block) override
{
if (m_on_call) m_on_call();
}
static void Call()
{
- std::shared_ptr<const CBlock> block = std::make_shared<CBlock>();
- BlockValidationState state;
- GetMainSignals().BlockChecked(block, state);
+ GetMainSignals().NewPoWValidBlock(nullptr, std::make_shared<CBlock>());
}
std::function<void()> m_on_call;
std::function<void()> m_on_destroy; |
|
Now As noted in the Google Doc:
So I plan to split it up and track it down. |
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.
… an immediate bool"
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.
a2e7fd6 to
893c635
Compare
|
Split up the first commit into:
I also botched the |
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.
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.
893c635 to
d6b1729
Compare
|
Got the unit tests and functional tests to pass! |
ariard
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.
Great to see progress on this, for now just high-level comments calling for better comments . I will review John PR again and then your linked notes to help to make them easier to read. I think that's the right way to come with a clear display of what is the current validation model, what this changes propose to do and why. There is so much context, that we can't expect all reviewers to do the PR context history digging by themselves.
| * Maintain state about nodes, protected by our own lock. Historically we put all | ||
| * peer tracking state in CNodeState, however this results in significant cs_main | ||
| * contention. Thus, new state tracking should go here, and we should eventually | ||
| * move most (non-validation-specific) state here. |
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.
I think you should define more difference between validation-state and non-validation-state. Right now peer tracking state is spread among multiple class like CNode with TxRelay or CNodeState and now CPeerState. We should have a clear idea of what should go where, according to which thread uses it. You should also explain how CPeerState aims to reduce cs_main contention with regards to the new threading model.
| //! The hash of the block which is pending download. | ||
| uint256 pending_block_hash; | ||
| //! Once we've finished processing a block from this peer, we must still wait for | ||
| //! any related callbacks to fire (to ensure, specifically, that rejects go out |
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 block has been processed. Handle potential peer punishment and housekeeping. |
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.
Please define more "housekeeping".
| } // Don't hold cs_main when we call into ProcessNewBlock | ||
| if (fBlockRead) { | ||
| bool fNewBlock = false; | ||
| // BIP 152 permits peers to relay compact blocks after validating |
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.
I'm not sure that BIP152 low-bandwidth authorizes invalid header propagation, maybe precise.
|
|
||
| /** Store block on disk. If dbp is non-nullptr, the file is known to already reside on disk */ | ||
| bool CChainState::AcceptBlock(const std::shared_ptr<const CBlock>& pblock, BlockValidationState& state, const CChainParams& chainparams, CBlockIndex** ppindex, bool fRequested, const FlatFilePos* dbp, bool* fNewBlock) | ||
| bool CChainState::ShouldMaybeWrite(CBlockIndex* pindex, bool fRequested) |
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.
Please can you comment on what conditional write is laying on.
|
|
||
| NotifyHeaderTip(); | ||
|
|
||
| BlockValidationState state; // Only used to report errors, not invalidity - ignore it |
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.
You should point where errors are distinguished from invalidity.
|
Not planning to work on this anytime soon unfortunately. |
|
From my research it follows that the "Move BlockChecked to a background thread" commit fixes a lock-order-inversion in the MessageHandler thread. See #19303 (comment). |

This is a (currently naive) rebase of #16323, which is a rework of #16175, which is a rework of #12934.
Built on top of #17479.
Currently
validationinterface_tests/unregister_all_during_callfails.Goals: