This is a rebase of #11639 with some fixes for the last few comments which were not yet addressed.
The original PR text, with some strikethroughs of text that is no longer correct:
This cleans up an old main-carryover - it made sense that main could decide what DoS scores to assign things because the DoS scores were handled in a different part of main, but now validation is telling net_processing what DoS scores to assign to different things, which is utter nonsense. Instead, we replace CValidationState’s nDoS and CorruptionPossible with a general ValidationInvalidReason, which net_processing can handle as it sees fit. I keep the behavior changes here to a minimum, but in the future we can utilize these changes for other smarter behavior, such as disconnecting/preferring to rotate outbound peers based on them providing things which are invalid due to SOFT_FORK because we shouldn’t ban for such cases.
This is somewhat complementary with, though obviously conflicts heavily with #11523, which added enums in place of DoS scores, as well as a few other cleanups (which are still relevant).
Compared with previous bans, the following changes are made:
Txn with empty vin/vout or null prevouts move from 10 DoS points to 100. Loose transactions with a dependency loop now result in a ban instead of 10 DoS points.
BIP68-violation no longer results in a ban as it is SOFT_FORK.Non-SegWit SigOp violation no longer results in a ban as it considers P2SH sigops and is thus SOFT_FORK.Any script violation in a block no longer results in a ban as it may be the result of a SOFT_FORK. This should likely be fixed in the future by differentiating between them.Proof of work failure moves from 50 DoS points to a ban. Blocks with timestamps under MTP now result in a ban, blocks too far in the future continue to not result in a ban. Inclusion of non-final transactions in a block now results in a ban instead of 10 DoS points.
Note: The change to ban all peers for consensus violations is actually NOT the change I’d like to make – I’d prefer to only ban outbound peers in those situations. The current behavior is a bit of a mess, however, and so in the interests of advancing this PR I tried to keep the changes to a minimum. I plan to revisit the behavior in a followup PR.
EDIT: One reviewer suggested I add some additional context for this PR:
The goal of this work was to make net_processing aware of the actual reasons for validation failures, rather than just deal with opaque numbers instructing it to do something.
In the future, I’d like to make it so that we use more context to decide how to punish a peer. One example is to differentiate inbound and outbound peer misbehaviors. Another potential example is if we’d treat RECENT_CONSENSUS_CHANGE failures differently (ie after the next consensus change is implemented), and perhaps again we’d want to treat some peers differently than others.
