This is a follow-up to PR
15141, which rewrote how DoS
protection was handled. Previously, validation would assign DoS scores if a
transaction or block was rejected, and pass the score back to net processing
inside a CValidationState object. Net processing could then disconnect or
ban the peer that provided the transaction or block. PR 15141 moved DoS
handling responsibility to net processing.
Net processing (and other components like the wallet/RPC) pass new inventory
(transactions, blocks and headers) to validation through different function
calls, such as:
AcceptToMemoryPool()
(ATMP) processes a transaction for acceptance to the memory pool.
ProcessNewBlock()
(PNB) processes a new block (and if it’s valid, adds it to our mapBlockIndex and
potentially adds it to the block chain).
Both ATMP and PNBH have a CValidationState& argument. The function returns
details about the validation of the transaction or headers in that state
object. PNB doesn’t currently take a state argument, but instead receives
state back from PNB through the CValidationInterface::BlockChecked()
callback. PR 16279 (covered
in a previous PR review club) would
add that state argument to PNB.
This PR tidies up some loose ends left by PR 15141. Most notably it adds
BlockValidationState and TxValidationState as derived classes of
ValidationState, since the reasons that a block and transaction can be
rejected are different.
The important changes in this PR are in src/consensus/validation.h. Most of
the other changes are updating the call sites to use the changed objects.
This PR is much easier to review if you look at the commits individually,
especially if you use ryanofsky’s review tip in the PR
description.
<@jnewbery> (1) can lead to follow-up PRs that can improve the code further, for (2) you might still find a bug that other reviewers have missed, and we should all be doing (3) all the time!
<@jnewbery> (as always, question 1 is Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK? (Don’t forget to put your PR review on GitHub.)
<amiti> I thought it was interesting to review. Since it touched all the sites that care about block / txn validation, it exposed me to new parts of the codebase
<@jnewbery> amiti: good! The PR title says this is a tidy-up, but I thought it might be interesting because it touches the net_processing-validation interface, which I think are the two most interesting components in Bitcoin Core
<amiti> I mostly just read the code one commit at a time & tried to understand the changes, and poke around the larger codebase to get context for them
<@jnewbery> What do we do to peers that send us txs/blocks that are invalid because of CONSENSUS? What do we do if they're invalid for RECENT_CONSENSUS_CHANGE?
<fjahr> I guess a new change would be flagged with this and then this would be used for a conditional to manage behaviour differently. So we don't ban peers who have not upgraded to segwit yet.
<@jnewbery> > What do we do to peers that send us txs/blocks that are invalid because of CONSENSUS? What do we do if they're invalid for RECENT_CONSENSUS_CHANGE?
<fjahr> ah, got it, so segwit is just the point after which this was introduced but there was nothing about segwit that made this necessary in particular.
<lightlike> i understand it only in parts: Couldn't it be introduced just as well together with the next consensus change instead of now? Has it been implemented now so that future consensus changes don't forget about the idea?
<@jnewbery> the risk is that some portion of the network upgrades, the softfork is activated, and then someone sends a transaction that is invalid under the new rules to nodes that haven't upgraded yet
<@jnewbery> so we want to treat txs/blocks that fail validation for RECENT_CONSENSUS_CHANGE differently than those that fail for long-standing CONSENSUS rules
<amiti> ok so thats the reason for having the distinction between recent & existing, but the reason that code is _already_ introduced is so its not forgotten about during implementation?
<@jnewbery> My final question was for all of you to either prepare a question or share an observation you made while reviewing the PR. Anyone have anything?
<@jnewbery> fjahr: I'm not sure it achieves much. peer punishment logic is so tied to net processing logic that separating them into separate translation units doesn't seem like an improvement
<amiti> jnewbery: I have a question about your git workflow- I noticed you pushed fixup commits in response to reviewer comments, and then squashed & force pushed afterwards. I was wondering if there was a specific reason you did that (vs squash and force push initially)... potentially to make it easier for reviewers to see the recent changes?
<@jnewbery> amiti: yeah, that's it. Just so those reviewers could very quickly see what changes I'd made in response to their comments. Lightlike suggested a much better way to do what he'd asked for, so I was able to throw away my commit and use his suggested method
<@jnewbery> as soon as those reviewers were happy I just squashed the changes into the relevant commits and force pushed. Verifying that a force-push doesn't change the final code is very quick
<@jnewbery> if I'd squashed and force-pushed initially and then lightlike had made his suggestion, I'd have wasted a bunch of time changing the initial commit and then changing it back and potentially having to resolve merge conflicts
<@jnewbery> of course if your node is old or isn't validating the same consensus rules as everyone else and an adversary knows that, then they can send you a tx that isn't valid according to the rest of the network and is valid for you. You'd try to relay that one
<sdaftuar> this is why we strive to ensure that features that transactions which will obey new consensus rules are policy-invalid (though obviously not consensus invalid) to old peers