Skip to content

Conversation

@dongcarl
Copy link
Contributor

@dongcarl dongcarl commented May 12, 2020

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:

  • Add as much documentation as possible to aid with review
  • Split up commits as much as possible to aid with review

@dongcarl
Copy link
Contributor Author

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

TheBlueMatt and others added 7 commits May 14, 2020 09:02
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).
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]>
@dongcarl
Copy link
Contributor Author

Rebased on newer #17479. git-bisecting the validationinterface_tests/unregister_all_during_call says:

194935b1a2968b594a42cb880e30701dd2e2bc7c is the first bad commit

@ryanofsky
Copy link
Contributor

Rebased on newer #17479. git-bisecting the validationinterface_tests/unregister_all_during_call says:

194935b1a2968b594a42cb880e30701dd2e2bc7c 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:

--- 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;

@dongcarl
Copy link
Contributor Author

Now p2p_unrequested_blocks.py is failing. git-bisect says:

1d9f66ea37ac6f22f26013cd57ed75fc04a9481f is the first bad commit

As noted in the Google Doc:

Up to this point, it seems that splitting this commit into:

  1. Changing the call semantics of ProcessNewBlock
  2. Changing the return type to a std::future

Might make it easier to review.

So I plan to split it up and track it down.

dongcarl and others added 4 commits May 19, 2020 14:45
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.
@dongcarl dongcarl force-pushed the 2020-05-async-pnb branch from a2e7fd6 to 893c635 Compare May 19, 2020 19:29
@dongcarl
Copy link
Contributor Author

Split up the first commit into:

  1. cb17890 which changes the call semantics of ProcessNewBlock and deals with the fallout of that
  2. 7532cf6 which changes the return from a bool to a future of a bool and deals with the fallout of that
  3. 2dcdb53 which contains changes that are non-obvious to me to be correct

I also botched the git-bisect before (didn't call make in a bash -c), new output:

$ git bisect run bash -c "make -j50 && python3 test/functional/p2p_unrequested_blocks.py"

...

7c77827558715180594f5881b6d02982504c4fad is the first bad commit
commit 7c77827558715180594f5881b6d02982504c4fad
Author: Matt Corallo <[email protected]>
Date:   Mon Jun 17 13:13:36 2019 -0400

    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.

 src/net_processing.cpp | 79 ++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 67 insertions(+), 12 deletions(-)
bisect 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.
@dongcarl dongcarl force-pushed the 2020-05-async-pnb branch from 893c635 to d6b1729 Compare May 21, 2020 22:05
@dongcarl
Copy link
Contributor Author

Got the unit tests and functional tests to pass!

Copy link

@ariard ariard left a 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.
Copy link

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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is reject here making reference to reject messages ? I think it doesn't make sense anymore post-#15437 and post-#17004. Also you may lay out the expected callback sequence (as we do for TransactionRemovedFromMempool in src/validationinterface.h).

}

/**
* A block has been processed. Handle potential peer punishment and housekeeping.
Copy link

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
Copy link

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)
Copy link

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
Copy link

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.

@dongcarl
Copy link
Contributor Author

Not planning to work on this anytime soon unfortunately.

@dongcarl dongcarl closed this Apr 20, 2021
@jnewbery
Copy link
Contributor

image

🙁

Let me know if you pick this up again!

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
@hebasto
Copy link
Member

hebasto commented May 17, 2023

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

@bitcoin bitcoin unlocked this conversation May 17, 2023
@maflcko maflcko removed the Tests label Aug 8, 2023
@bitcoin bitcoin locked and limited conversation to collaborators Aug 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants