c4b0c08f7c Update tx-size-small comment with relevant CVE disclosure (Gregory Sanders)
Pull request description:
Code first introduced under https://github.com/bitcoin/bitcoin/pull/11423 with essentially no description and no discussion.
ACKs for top commit:
MarcoFalke:
ACK c4b0c08f7c
fanquake:
ACK c4b0c08f7c
Tree-SHA512: 95d5c92998b8b1e944c477dbaee265b62612b6e815099ab31d9ff580b4dff777abaf7f326a284644709f918aa1510412d62310689b1250ef6e64de7b19ca9f71
CVE-2018-17144 and CVE-2012-2459 are only partially tested for regression.
- CVE-2018-17144 is not tested for the inflation bug.
- CVE-2012-2459 is only tested for the mutated block being rejected, not
for the original block being accepted afterwards.
This commit fixes that limitation.
Also added functional test for CVE-2010-5137.
0ff1c2a838 Separate reason for premature spends (coinbase/locktime) (Suhas Daftuar)
54470e767b Assert validation reasons are contextually correct (Suhas Daftuar)
2120c31521 [refactor] Update some comments in validation.cpp as we arent doing DoS there (Matt Corallo)
12dbdd7a41 [refactor] Drop unused state.DoS(), state.GetDoS(), state.CorruptionPossible() (Matt Corallo)
aa502b88d1 scripted-diff: Remove DoS calls to CValidationState (Matt Corallo)
7721ad64f4 [refactor] Prep for scripted-diff by removing some \ns which annoy sed. (Matt Corallo)
5e78c5734b Allow use of state.Invalid() for all reasons (Matt Corallo)
6b34bc6b6f Fix handling of invalid headers (Suhas Daftuar)
ef54b486d5 [refactor] Use Reasons directly instead of DoS codes (Matt Corallo)
9ab2a0412e CorruptionPossible -> BLOCK_MUTATED (Matt Corallo)
6e55b292b0 CorruptionPossible -> TX_WITNESS_MUTATED (Matt Corallo)
7df16e70e6 LookupBlockIndex -> CACHED_INVALID (Matt Corallo)
c8b0d22698 [refactor] Drop redundant nDoS, corruptionPossible, SetCorruptionPossible (Matt Corallo)
34477ccd39 [refactor] Add useful-for-dos "reason" field to CValidationState (Matt Corallo)
6a7f8777a0 Ban all peers for all block script failures (Suhas Daftuar)
7b999103e2 Clean up banning levels (Matt Corallo)
b8b4c80146 [refactor] drop IsInvalid(nDoSOut) (Matt Corallo)
8818729013 [refactor] Refactor misbehavior ban decisions to MaybePunishNode() (Matt Corallo)
00e11e61c0 [refactor] rename stateDummy -> orphan_state (Matt Corallo)
f34fa719cf Drop obsolete sigops comment (Matt Corallo)
Pull request description:
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.
ACKs for commit 0ff1c2:
jnewbery:
utACK 0ff1c2a838
ryanofsky:
utACK 0ff1c2a838. Only change is dropping the first commit (f3883a321bf4ab289edcd9754b12cae3a648b175), and dropping the temporary `assert(level == GetDoS())` that was in 35ee77f2832eaffce30042e00785c310c5540cdc (now c8b0d22698)
Tree-SHA512: e915a411100876398af5463d0a885920e44d473467bb6af991ef2e8f2681db6c1209bb60f848bd154be72d460f039b5653df20a6840352c5f7ea5486d9f777a3
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.
* Many pre-segwit soft-fork errors now result in a ban.
Note: Transactions that violate soft-fork script flags since P2SH do not generally
result in a ban. Also, banning behavior for invalid blocks is dependent on
whether the node is validating with multiple script check threads, due to a long-
standing bug. That inconsistency is still present after this commit.
* 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.
Co-authored-by: Anthony Towns <aj@erisian.com.au>
