ProcessNewBlock()
is net processing’s entry point into validation, called whenever net
processing receives a new block. It can be called in ProcessMessage() when
a BLOCK, CMPCTBLOCK or BLOCKTXN message is received and net processing
is ready to hand a block to the validation layer.
ProcessNewBlock() calls CheckBlock() to carry out non-contextual
validation tests on the block. If those checks pass, then
ProcessNewBlock() calls AcceptBlock(), which does more checks on the block
and then stores it to disk.
If AcceptBlock() passes and the block is valid, then ActivateBestChain()
is called to update the node’s view of the blockchain. If the new block has
more work than the node’s current tip, then ActivateBestChain will try to
connect it to the chain and update the UTXO set. This can fail, for
example if a transaction in the block spends non-existent coins.
If a peer is sending us invalid blocks, then depending on the invalidity
reason, we may wish to disconnect that peer.
If either CheckBlock() or AcceptBlock() fails, then the failure reason
will be saved in a CValidationState object and returned to
ProcessNewBlock().
Prior to this PR, ProcessNewBlock() would then call the BlockChecked method
in the CValidationInterface. net processing would receive the
BlockChecked callback and then potentially disconnect the peer.
This PR changes ProcessNewBlock so that if CheckBlock() or AcceptBlock()
fail, then it will return the CValidationState directly to the caller, which
can then decide whether to ban the peer.
This PR is a step towards PR
16175, which makes
ProcessNewBlock() asynchronous, and would allow parallelization between net
processing and validation.
Did anything about the way new blocks are checked and accepted surprise you?
Do you have any questions about the series of steps to add a block to the
block chain?
The author describes this PR as “a first step towards a series of cleanups
leading to a (more-complete) #16175. It’s just two rather-nice (IMO)
cleanups.” Do you agree? Is this just a cleanup PR or are there behaviour
changes?
Are there any other CValidationInterface clients that use the
BlockChecked callback?
The other place that the BlockChecked method is called is in
ConnectTip() in ActivateBestChain(). Why does that need to be a
callback? Why does net processing need to keep a mapBlockSource map from the
block to the peer that provided it?
<jnewbery> we've touched on the CValidationInterface in a few previous reviews, but I think this is the first time we've looked at changes in validation.cpp
<jnewbery> pinheadmz: I'm thinking more about on the net processing layer. Which function sees that we've received an invalid block and then handles whether to punish peers?
<jnewbery> so prior to this PR, we had net processing calling into validation (using ProcessNewBlock()), which then called directly into net processing (using the BlockChecked() validation interface method, which is a direct function call)
<jnewbery> zenogais: exactly. Any client that wants to register with the validation interface needs to call that RegisterValidationInterface() function
<jnewbery> really, I think we want net processing to just be passing blocks/transactions up to validation, and being informed asynchronously of any validation events
<jnewbery> if we want net processing and validation to run in parallel, we want net processing to be simply handing messages to validation and not have net processing and validation calling into each other
<jnewbery> ok, next question: Did anything about the way new blocks are checked and accepted surprise you? Do you have any questions about the series of steps to add a block to the block chain?
<jnewbery> The author describes this PR as “a first step towards a series of cleanups leading to a (more-complete) #16175. It’s just two rather-nice (IMO) cleanups.” Do you agree? Is this just a cleanup PR or are there behaviour changes?
<zenogais> Hard to draw the line between cleanup and behavior change. I think some of the stuff pointed out with BlockStatus by jnewbery leads me to believe there's a behavior change.
<jnewbery> before I put my ACK on a PR I want to understand exactly how behaviour might change, especially in net processing and validation where very tiny changes can have surprising and dangerous effects
<jnewbery> changing from `if (pindex->IsValid(BLOCK_VALID_TRANSACTIONS))` to `if (dos_state.IsValid())` seems reasonable, but it sent me off in lots of different directions trying to work out when BlockStatus is changed, what MarkBlockAsReceived() is doing exactly, etc
<jnewbery> The other place that the BlockChecked method is called is in ConnectTip() in ActivateBestChain(). Why does that need to be a callback? Why does net processing need to keep a mapBlockSource map from the block to the peer that provided it?
<jnewbery> and so at that point, if it is invalid (eg it contains a double-spend or invalid tx), we want to disconnect from the peer that sent it to us
<amiti> so, reasons you would go through ProcessNewBlock are 1. fNewBlock is true 2. you force it with fForceProcessing 3. pruning enabled & trying to re-download a block that was pruned
<jnewbery> I think the comment is trying to say - we don't need to call ProcessNewBlock again if we receive a block with the same hash, as long as the state returned was valid and (it was fForceProcessing or a new block)