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_call fails.
Goals:
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).
Co-authored-by: Carl Dong <contact@carldong.me>
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 <john@johnnewbery.com>
Co-authored-by: Carl Dong <contact@carldong.me>
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 <john@johnnewbery.com>
Co-authored-by: Carl Dong <contact@carldong.me>
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 <john@johnnewbery.com>
Co-authored-by: Carl Dong <contact@carldong.me>
This is a pure refactor commit.
Co-authored-by: John Newbery <john@johnnewbery.com>
Co-authored-by: Carl Dong <contact@carldong.me>
Co-authored-by: John Newbery <john@johnnewbery.com>
Co-authored-by: Carl Dong <contact@carldong.me>
Rebased on newer #17479.
git-bisecting thevalidationinterface_tests/unregister_all_during_callsays:0194935b1a2968b594a42cb880e30701dd2e2bc7c is the first bad commit
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:
0--- a/src/test/validationinterface_tests.cpp
1+++ b/src/test/validationinterface_tests.cpp
2@@ -57,15 +57,13 @@ public:
3 {
4 if (m_on_destroy) m_on_destroy();
5 }
6- void BlockChecked(const CBlock& block, const BlockValidationState& state) override
7+ void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock> &block) override
8 {
9 if (m_on_call) m_on_call();
10 }
11 static void Call()
12 {
13- std::shared_ptr<const CBlock> block = std::make_shared<CBlock>();
14- BlockValidationState state;
15- GetMainSignals().BlockChecked(block, state);
16+ GetMainSignals().NewPoWValidBlock(nullptr, std::make_shared<CBlock>());
17 }
18 std::function<void()> m_on_call;
19 std::function<void()> m_on_destroy;
Now p2p_unrequested_blocks.py is failing. git-bisect says:
01d9f66ea37ac6f22f26013cd57ed75fc04a9481f is the first bad commit
As noted in the Google Doc:
Up to this point, it seems that splitting this commit into:
- Changing the call semantics of
ProcessNewBlock- Changing the return type to a
std::futureMight make it easier to review.
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.
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.
Split up the first commit into:
ProcessNewBlock and deals with the fallout of thatI also botched the git-bisect before (didn’t call make in a bash -c), new output:
0$ git bisect run bash -c "make -j50 && python3 test/functional/p2p_unrequested_blocks.py"
1
2...
3
47c77827558715180594f5881b6d02982504c4fad is the first bad commit
5commit 7c77827558715180594f5881b6d02982504c4fad
6Author: Matt Corallo <git@bluematt.me>
7Date: Mon Jun 17 13:13:36 2019 -0400
8
9 Move net_processing's ProcessNewBlock calls to resolve async.
10
11 Essentially, our goal is to not process anything for the given peer
12 until the block finishes processing (emulating the previous behavior)
13 without actually blocking the ProcessMessages loops. Obviously, in
14 most cases, we'll just go on to the next peer and immediately hit a
15 cs_main lock, blocking us anyway, but this we can slowly improve
16 that state over time by moving things from CNodeState to CPeerState.
17
18 src/net_processing.cpp | 79 ++++++++++++++++++++++++++++++++++++++++++--------
19 1 file changed, 67 insertions(+), 12 deletions(-)
20bisect run success
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.
247@@ -237,6 +248,30 @@ namespace {
248 } // namespace
249
250 namespace {
251+/**
252+ * Maintain state about nodes, protected by our own lock. Historically we put all
253+ * peer tracking state in CNodeState, however this results in significant cs_main
254+ * contention. Thus, new state tracking should go here, and we should eventually
255+ * move most (non-validation-specific) state here.
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.
260+ //! peer.
261+ std::future<bool> pending_block_processing;
262+ //! The hash of the block which is pending download.
263+ uint256 pending_block_hash;
264+ //! Once we've finished processing a block from this peer, we must still wait for
265+ //! any related callbacks to fire (to ensure, specifically, that rejects go out
TransactionRemovedFromMempool in src/validationinterface.h).
2046@@ -1973,6 +2047,23 @@ void static ProcessOrphanTx(CConnman* connman, CTxMemPool& mempool, std::set<uin
2047 }
2048 }
2049
2050+/**
2051+ * A block has been processed. Handle potential peer punishment and housekeeping.
3176- mapBlockSource.emplace(resp.blockhash, std::make_pair(pfrom->GetId(), false));
3177 }
3178 } // Don't hold cs_main when we call into ProcessNewBlock
3179 if (fBlockRead) {
3180- bool fNewBlock = false;
3181+ // BIP 152 permits peers to relay compact blocks after validating
3728@@ -3729,23 +3729,10 @@ static FlatFilePos SaveBlockToDisk(const CBlock& block, int nHeight, const CChai
3729 return blockPos;
3730 }
3731
3732-/** Store block on disk. If dbp is non-nullptr, the file is known to already reside on disk */
3733-bool CChainState::AcceptBlock(const std::shared_ptr<const CBlock>& pblock, BlockValidationState& state, const CChainParams& chainparams, CBlockIndex** ppindex, bool fRequested, const FlatFilePos* dbp, bool* fNewBlock)
3734+bool CChainState::ShouldMaybeWrite(CBlockIndex* pindex, bool fRequested)
3922+ }
3923+ }
3924+
3925+ NotifyHeaderTip();
3926+
3927+ BlockValidationState state; // Only used to report errors, not invalidity - ignore it
🙁
Let me know if you pick this up again!
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).