Rewrite DoS interface between validation and net_processing #15141

sdaftuar · 2019-01-10T16:57:13Z

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.

DrahtBot · 2019-01-10T18:31:14Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15681 ([mempool] Allow one extra single-ancestor transaction per package by TheBlueMatt)
#15505 ([p2p] Request NOTFOUND transactions immediately from other outbound peers, when possible by sdaftuar)
#15253 (Net: Consistently log outgoing INV messages by Empact)
#13868 (Remove unused fScriptChecks parameter from CheckInputs by Empact)
#13525 (Report reason inputs are nonstandard from AreInputsStandard by Empact)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

practicalswift · 2019-01-10T18:34:05Z

Concept ACK

sipa · 2019-01-11T18:17:13Z

Concept ACK; I'll review for changes in behavior for specific validation reasons later.

laanwj · 2019-01-14T14:23:14Z

Concept ACK

jnewbery

I've reviewed a5415e8. A few nits/questions inline.

src/consensus/validation.h

jnewbery · 2019-01-14T22:09:50Z

src/validation.cpp

@@ -3333,7 +3341,9 @@ static bool ContextualCheckBlock(const CBlock& block, CValidationState& state, c
    // the block hash, so we couldn't mark the block as permanently
    // failed).
    if (GetBlockWeight(block) > MAX_BLOCK_WEIGHT) {
-        return state.DoS(100, false, REJECT_INVALID, "bad-blk-weight", false, strprintf("%s : weight limit failed", __func__));
+        // We can call this a consensus failure as any data-providers who provided


This seems entirely obvious and not requiring a comment to me, which makes me think there's some subtlety I've missed. Is this just saying that if we receive a block with witness data, it should be valid-according-to-BIP141?

Pedantic nit: I'd also avoid talking about 'data-providers' in validation.cpp. After this PR, validation should be unconcerned with data-providers and only be validating blocks based on the block data.

I believe this comment is contrasting a CONSENSUS failure from a RECENT_CONSENSUS_CHANGE -- I think in @TheBlueMatt's original PR he had some validation failures marked as RECENT_CONSENSUS_CHANGE, but eventually we decided to switch them all out (and reserve RECENT_CONSENSUS_CHANGE as something we might do in the future).

I think I agree with you philosophically that validation ought not be very concerned with 'data providers', but I think the ValidationReasons interface is also driven by the needs of net_processing, so sometimes we may need to explain reasons that maybe don't make sense in a totally neutral validation library because our application requires it. RECENT_CONSENSUS_CHANGE is one such possible example (though we're not using it in this PR and I am not sure we ever will); the BLOCK_INVALID_HEADER enum I added is another (net_processing needs to be able to distinguish some headers failures from others, in order to maintain the current ban behavior).

Anyway I'll update this comment to be clearer.

I think this comment is justifying upgrading the (at the time recent) segwit test from a RECENT_CONSENSUS_CHANGE to just CONSENSUS_CHANGE, the reason being that either you've got an old client that didn't provide segwit data -- in which case this test won't trigger because the bad-blk-length test will already have failed -- or it is providing segwit data but doing it wrong, in which case there's no reason to use the more forgiving RECENT version. So I think just dropping the comment (now that segwit isn't recent) is fine, fwiw.

src/net_processing.cpp

ajtowns

Looks good to me; haven't fully looked through "Use new reason-based DoS/disconnect logic instead of state.nDoS" though.

ajtowns · 2019-01-15T06:07:14Z

src/consensus/validation.h

@@ -117,14 +107,7 @@ class CValidationState {
    bool IsError() const {
        return mode == MODE_ERROR;
    }
-    bool CorruptionPossible() const {
-        return corruptionPossible;
-    }


Maybe having inline bool CorruptionPossible() const { return reason == BLOCK_MUTATED; } would make for nicer code elsewhere?

ajtowns · 2019-01-15T12:57:07Z

src/validation.cpp

@@ -889,7 +889,6 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
                // Only the witness is missing, so the transaction itself may be fine.
                state.Invalid(ValidationInvalidReason::TX_WITNESS_MUTATED, false,
                          state.GetRejectCode(), state.GetRejectReason(), state.GetDebugMessage());
-                state.SetCorruptionPossible();


This change isn't a clean refactor -- !state.CorruptionPossible() would have returned false after this, but its replacement in this commit (ie, state.GetReason() != BLOCK_MUTATED) will return true. I think this is okay though, since CorruptionPossible() is only checked for block updates, and this just deals with mempool tx's, and the uses of state.CorruptionPossible() that this would have effected were already changed to state.GetReason() != TX_WITNESS_MUTATED in an earlier commit.

ajtowns · 2019-01-15T14:18:58Z

src/blockencodings.cpp

@@ -203,7 +203,7 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<
        // but that is expensive, and CheckBlock caches a block's
        // "checked-status" (in the CBlock?). CBlock should be able to
        // check its own merkle root and cache that check.
-        if (state.CorruptionPossible())
+        if (state.GetReason() == ValidationInvalidReason::BLOCK_MUTATED)


Seems like this change could be squashed into "Remove references to CValidationState's DoS and CorruptionPossible" ?

ryanofsky

Started reviewing this, but IMO, the way this PR is structured makes it difficult to verify that it doesn't unintentionally change behavior.

I think a nicer way to write this would be to have one commit adding empty ValidationInvalidReason, MayResultInDisconnect, and MaybePunishNode definitions, and adding a state.Invalid() overload taking an optional ValidationInvalidReason argument. Then have a sequence of small followup commits which each add a few enum values at a time, passing them through state.Invalid() and translating them into Misbehaving() calls, where each commit is self contained and is deals with related reasons so it is easy to spot and understand changes in behavior.

If this is a bad idea, or too much work, I'd be ok with trying to review this PR as it is, but I wanted to suggest something to be able to have more confidence in it, and to maybe make it easier to find other reviewers.

a5415e8 Add useful-for-dos "reason" field to CValidationState (1/8)
33213ad Add functions to convert CValidationInterface's reason to DoS info (2/8)
6bdc449 Use new reason-based DoS/disconnect logic instead of state.nDoS (3/8)
963699d Use state reason field to check for collisions in cmpctblocks (4/8)
06e4247 Prep for scripted-diff by removing some \ns which annoy sed. (5/8)
8131896 scripted-diff: Remove DoS calls to CValidationState (6/8)
6ee2c45 Remove references to CValidationState's DoS and CorruptionPossible (7/8)
94874dd Update some comments in validation.cpp as we arent doing DoS there (8/8)

ryanofsky · 2019-01-15T19:30:40Z

src/consensus/tx_verify.cpp

@@ -160,24 +160,24 @@ bool CheckTransaction(const CTransaction& tx, CValidationState &state, bool fChe
 {
    // Basic checks that don't depend on any context
    if (tx.vin.empty())
-        return state.DoS(10, false, REJECT_INVALID, "bad-txns-vin-empty");
+        return state.DoS(10, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-vin-empty");


In commit "Add useful-for-dos "reason" field to CValidationState" (a5415e8)

Note: word-diff is useful here to review new function arguments:

git log -p -n1 -U0 --word-diff-regex=. a5415e85caaf2f5a77d6bae9574bb6d21139ee34

ryanofsky · 2019-01-15T19:36:28Z

src/validation.cpp

@@ -716,27 +716,22 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
                              fSpendsCoinbase, nSigOpsCost, lp);
        unsigned int nSize = entry.GetTxSize();

-        // Check that the transaction doesn't have an excessive number of


In commit "Add useful-for-dos "reason" field to CValidationState" (a5415e8):

Why remove this comment?

Good question.

Note that MAX_BLOCK_SIGOPS has been renamed / replaced by MAX_STANDARD_TX_SIGOPS_COST as part of SegWit in 2b1f6f9. In addition to this comment, MAX_BLOCK_SIGOPS is also still mentioned in the function test framework. But that doesn't explain why the comment can be removed.

I think this comment is not very helpful. It was originally added in #4150, and in the review on that PR people complained that the phrasing in this comment is confusing ("invalid rather than merely non-standard" - huh?).

If reviewers prefer it, then I can just improve this comment rather than delete it -- it just seems to me like the code reads just fine on its own.

MAX_BLOCK_SIGOPS is replaced by MAX_BLOCK_SIGOPS_COST (which is just multiplied by the witness scale factor of 4) as part of segwit. MAX_STANDARD_TX_SIGOPS_COST is just a separate rule at the relay/mempool level saying "you have to use at least 5 tx's to hit the sigop limit".

I agree that the comment's just confusing given how we understand "invalid" (breaks consensus rules) vs "non-standard" (not interesting for mempool/relay, but not too strongly punished either).

ryanofsky · 2019-01-15T19:44:08Z

src/validation.cpp

+                if (state.GetReason() == ValidationInvalidReason::TX_MISSING_INPUTS) {
+                    // CheckTxInputs may return MISSING_INPUTS but we can't return that, as
+                    // it's not defined for a block, so we reset the reason flag to CONSENSUS here.
+                    state.DoS(state.GetDoS(), ValidationInvalidReason::CONSENSUS, false,


In commit "Add useful-for-dos "reason" field to CValidationState" (a5415e8)

This seems like it is doubling the state.nDoS level, in addition to updating the reason enum:

bitcoin/src/consensus/validation.h

Line 96 in a5415e8

nDoS += level;

Would suggest replacing this change something more straightforward like state.UpdateReason(ValidationInvalidReason::CONSENSUS)

ryanofsky · 2019-01-15T19:45:33Z

src/validation.cpp

+                    // but we can't return that, as it's not defined for a block, so
+                    // we reset the reason flag to CONSENSUS here.
+                    // (note that this may not be the case until we add additional
+                    // soft-fork flags to our script flags, in which case we  need to


In commit "Add useful-for-dos "reason" field to CValidationState" (a5415e8)

Extra space on this line

ryanofsky · 2019-01-15T19:47:21Z

src/validation.cpp

+                    // soft-fork flags to our script flags, in which case we  need to
+                    // be careful to differentiate RECENT_CONSENSUS_CHANGE and
+                    // CONSENSUS)
+                    state.DoS(state.GetDoS(), ValidationInvalidReason::CONSENSUS, false,


In commit "Add useful-for-dos "reason" field to CValidationState" (a5415e8)

This also seems to double state.nDoS.

ryanofsky · 2019-01-15T19:52:30Z

src/validation.cpp

@@ -1988,11 +1988,17 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
        {
            CAmount txfee = 0;
            if (!Consensus::CheckTxInputs(tx, state, view, pindex->nHeight, txfee)) {
+                if (state.GetReason() == ValidationInvalidReason::TX_MISSING_INPUTS) {
+                    // CheckTxInputs may return MISSING_INPUTS but we can't return that, as
+                    // it's not defined for a block, so we reset the reason flag to CONSENSUS here.


In commit "Add useful-for-dos "reason" field to CValidationState" (a5415e8)

Is there a check for the requirement that MISSING_INPUTS is not used for a block? I would expect to see an assert(reason != MISSING_INPUTS) or assert(ValidForBlock(reason)) or something like that somewhere.

ryanofsky · 2019-01-15T19:57:17Z

src/validation.cpp


    // Check transactions
-    for (const auto& tx : block.vtx)
-        if (!CheckTransaction(*tx, state, true))
-            return state.Invalid(false, state.GetRejectCode(), state.GetRejectReason(),


In commit "Add useful-for-dos "reason" field to CValidationState" (a5415e8)

Note: I guess this line used to set state.corruptionPossible = false but no longer does.

bitcoin/src/consensus/validation.h

Lines 54 to 58 in cebe910

bool Invalid(bool ret = false,

unsigned int _chRejectCode=0, const std::string &_strRejectReason="",

const std::string &_strDebugMessage="") {

return DoS(0, ret, _chRejectCode, _strRejectReason, false, _strDebugMessage);

}

New way seems better.

So if I understand this correctly:

state.Invalid() just calls state.DoS() with level=0andcorruptionIn=false` (default).

CheckTransaction() can currently fail in various ways, calling:

state.DoS with:

level 10 or 100: why isn't this higher level a problem?

corruptionIn not specified (so defaults to false)

level 10 or 100: why isn't this higher level a problem?

@Sjors I don't understand your question -- can you rephrase?

If I understand correctly: the higher level (ie, changing the 10's to 100's in 96cedc8 - the "clean up banning levels" commit) isn't a problem because these failures are all consensus ones, so any reasonable implementation shouldn't be making them. (Except for immature coinbase and missing inputs at the mempool level which are downgraded elsewhere)

ryanofsky · 2019-01-15T20:21:21Z

src/net_processing.cpp

        // Don't send reject message with code 0 or an internal reject code.
        if (it != mapBlockSource.end() && State(it->second.first) && state.GetRejectCode() > 0 && state.GetRejectCode() < REJECT_INTERNAL) {
            CBlockReject reject = {(unsigned char)state.GetRejectCode(), state.GetRejectReason().substr(0, MAX_REJECT_MESSAGE_LENGTH), hash};
            State(it->second.first)->rejects.push_back(reject);
-            if (nDoS > 0 && it->second.second)
-                Misbehaving(it->second.first, nDoS);
+            MaybePunishNode(/*nodeid=*/ it->second.first, state, /*via_compact_block=*/ !it->second.second);


In commit "Use state reason field to check for collisions in cmpctblocks" (963699d)

Since the mapBlockSource bool is now being passed as !via_compact_block, it seems like the field description should mention something about setting it based on whether the source was a compact or full block:

bitcoin/src/net_processing.cpp

Lines 104 to 105 in 6bdc449

* Set mapBlockSource[hash].second to false if the node should not be

* punished if the block is invalid.

ajtowns · 2019-01-18T13:19:59Z

src/test/txvalidation_tests.cpp

-    int nDoS;
-    BOOST_CHECK_EQUAL(state.IsInvalid(nDoS), true);
-    BOOST_CHECK_EQUAL(nDoS, 100);
+    BOOST_CHECK_EQUAL(state.IsInvalid(), true);


We checked state.IsInvalid() a couple of lines earlier, so this addition is redundant.

ajtowns · 2019-01-18T13:42:02Z

Started reviewing this, but IMO, the way this PR is structured makes it difficult to verify that it doesn't unintentionally change behavior.

FWIW, I've had a go at redoing the patchset to try to make the (potential) functionality changes more clear: https://github.com/ajtowns/bitcoin/commits/201901-dosreasons

This has (I think) all the behaviour changes first:

d9451de0d0 drop obsolete comment
acdb469525 [refactor] stateDummy -> orphan_state
5cd7a4d338 [refactor] Use maybepunish etc
0d1d471eac [refactor] drop IsInvalid(nDoSOut)
a0776a5d8a set nDoS rather than bumping it
e0cff4e133 Clean up banning levels

before introducing the new reason field, along with checks that the implied DoS value for each reason matches the actual DoS values presented/used:

89e8dea284 [refactor] Add useful-for-dos "reason" field to CValidationState

which then allows dropping the instance variables:

27089e55be [refactor] Drop redundant nDoS, corruptionPossible, SetCorruptionPossible

Then the code is changed to use reasons directly:

15d9023106 LookupBlockIndex -> CACHED_INVALID
519fb78934 CorruptionPossible -> TX_WITNESS_MUTATED
221d17f332 CorruptionPossible -> BLOCK_MUTATED
32747d0746 [refactor] Use Reasons directly instead of DoS codes

And the now obsolete DoS/etc stuff is dropped:

4d110a59c6 [refactor] Prep for scripted-diff by removing some \ns which annoy sed.
9a89a47257 scripted-diff: Remove DoS calls to CValidationState
327591b016 [refactor] Drop unused state.DoS(), state.GetDoS(), state.CorruptionPossible()

That leaves a couple more things:

94cf0deffb [refactor] Update some comments in validation.cpp as we arent doing DoS there
04c6b24a66 [refactor] swap if/else order
96f0ee075d remaining commits vs 94874ddfdf4ae9c4b10f3f91d5e3280e2c24c371

but finally ends up with the same code as this PR (minus the latest commit anyway).

Anyway I think this approach might be easier to review? It could also allow splitting the PR into two -- one making the changes to DoS behaviour but not changing the way DoS works; followed by a second PR that actually adds the Reasons and refactors but doesn't change behaviour.

(Proof of concept only: bunches of these commits should probably be combined, commit messages need improvement, and I think I lost a bunch of authorship info)

EDIT:

I've added an extra commit prior to the DoS->Invalid refactor, namely "5b15205883 Allow use of state.Invalid() for all reasons" that avoids assertions that Invalid() is only used for DoS-level-0 problems failing.

That just leaves one test failure in the intermediate commits; feature_block.py fails after the changing the DoS levels but before adding the "reason" code. I think this is due to lowering bad-txns-inputs-missingorspent from 100 to 0 with the tests still expecting a disconnect when that happens in a block. Adding the reasons fixes this because that includes code to update that problem from 0/TX_MISSING_INPUTS to 100/CONSENSUS when it affects a block rather than a loose transaction. (And similarly, the tests are adujsted to expect disconnects due to premature coinbase spends, but that functionality only occurs as part of the 0/TX to 100/CONS step)

sdaftuar · 2019-01-18T14:46:55Z

Thanks all for the review so far!

I'd started taking a stab at rewriting this; I'll continue with my approach to see how it ends up but @ajtowns thank you for your help -- @ryanofsky if you have any thoughts on @ajtowns's rework please let me know, happy to adapt his breakdown and include here if that approach looks good.

ryanofsky · 2019-01-18T15:12:31Z

@ryanofsky if you have any thoughts on @ajtowns's rework please let me know

Took a quick look, and I think ajtowns's refactor is great. It's a slightly different approach than I suggested in that the 32747d0 commit which starts using reason codes is done all at once instead of incrementally as reasons are added, so it requires a little bit of grepping to verify, but this is easy to do and I think it's a huge improvement.

I think it would be best to use ajtown's branch here, unless you've done a lot of work on your own already or see problems I'm missing.

naumenkogs · 2019-01-21T19:16:15Z

Concept ACK, I will take a closer look once the code is updated per comments above I guess.

sdaftuar · 2019-01-24T18:26:48Z

I have redone this along the lines of @ajtowns branch, and cleaned up each commit (I think!) so that each one should be logically correct, pass tests, etc.

I've saved the original version of this PR here: https://github.com/sdaftuar/bitcoin/commits/15141.original

The diff between the two is pretty small (just some formatting changes that were getting tedious to resolve, and I removed a couple lines that some reviewers had commented on as being unnecessary):

diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
index fb04c1c0abf..a7b31ff7c56 100644
--- a/src/consensus/tx_verify.cpp
+++ b/src/consensus/tx_verify.cpp
@@ -221,8 +221,7 @@ bool Consensus::CheckTxInputs(const CTransaction& tx, CValidationState& state, c
 
         // If prev is coinbase, check that it's matured
         if (coin.IsCoinBase() && nSpendHeight - coin.nHeight < COINBASE_MATURITY) {
-            return state.Invalid(ValidationInvalidReason::TX_MISSING_INPUTS, false,
-                REJECT_INVALID, "bad-txns-premature-spend-of-coinbase",
+            return state.Invalid(ValidationInvalidReason::TX_MISSING_INPUTS, false, REJECT_INVALID, "bad-txns-premature-spend-of-coinbase",
                 strprintf("tried to spend coinbase at depth %d", nSpendHeight - coin.nHeight));
         }
 
diff --git a/src/consensus/validation.h b/src/consensus/validation.h
index daf8b9b87cc..09a5630a4f3 100644
--- a/src/consensus/validation.h
+++ b/src/consensus/validation.h
@@ -81,8 +81,8 @@ private:
 public:
     CValidationState() : mode(MODE_VALID), reason(ValidationInvalidReason::NONE), chRejectCode(0) {}
     bool Invalid(ValidationInvalidReason reasonIn, bool ret = false,
-             unsigned int chRejectCodeIn=0, const std::string &strRejectReasonIn="",
-             const std::string &strDebugMessageIn="") {
+            unsigned int chRejectCodeIn=0, const std::string &strRejectReasonIn="",
+            const std::string &strDebugMessageIn="") {
         reason = reasonIn;
         chRejectCode = chRejectCodeIn;
         strRejectReason = strRejectReasonIn;
diff --git a/src/test/txvalidation_tests.cpp b/src/test/txvalidation_tests.cpp
index 00fd7fef12a..aa30129361f 100644
--- a/src/test/txvalidation_tests.cpp
+++ b/src/test/txvalidation_tests.cpp
@@ -52,8 +52,6 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_reject_coinbase, TestChain100Setup)
     // Check that the validation state reflects the unsuccessful attempt.
     BOOST_CHECK(state.IsInvalid());
     BOOST_CHECK_EQUAL(state.GetRejectReason(), "coinbase");
-
-    BOOST_CHECK_EQUAL(state.IsInvalid(), true);
     BOOST_CHECK(state.GetReason() == ValidationInvalidReason::CONSENSUS);
 }
 
diff --git a/src/validation.cpp b/src/validation.cpp
index e1f562ffbfd..20759bf96e7 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -888,7 +888,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
                 !CheckInputs(tx, stateDummy, view, true, scriptVerifyFlags & ~SCRIPT_VERIFY_CLEANSTACK, true, false, txdata)) {
                 // Only the witness is missing, so the transaction itself may be fine.
                 state.Invalid(ValidationInvalidReason::TX_WITNESS_MUTATED, false,
-                          state.GetRejectCode(), state.GetRejectReason(), state.GetDebugMessage());
+                        state.GetRejectCode(), state.GetRejectReason(), state.GetDebugMessage());
             }
             return false; // state filled in by CheckInputs
         }
@@ -1980,7 +1980,7 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
                     // CheckTxInputs may return MISSING_INPUTS but we can't return that, as
                     // it's not defined for a block, so we reset the reason flag to CONSENSUS here.
                     state.Invalid(ValidationInvalidReason::CONSENSUS, false,
-                              state.GetRejectCode(), state.GetRejectReason(), state.GetDebugMessage());
+                            state.GetRejectCode(), state.GetRejectReason(), state.GetDebugMessage());
                 }
                 return error("%s: Consensus::CheckTxInputs: %s, %s", __func__, tx.GetHash().ToString(), FormatStateMessage(state));
             }
@@ -3341,8 +3341,6 @@ static bool ContextualCheckBlock(const CBlock& block, CValidationState& state, c
     // the block hash, so we couldn't mark the block as permanently
     // failed).
     if (GetBlockWeight(block) > MAX_BLOCK_WEIGHT) {
-        // We can call this a consensus failure as any data-providers who provided
-        // us with witness data can be expected to support SegWit validation.
         return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-blk-weight", strprintf("%s : weight limit failed", __func__));
     }

Also if this version is not actually easier to review I'm happy to go back to the original or try another approach.

ryanofsky

Started review (will update list below with progress).

src/validation.cpp

ryanofsky · 2019-01-24T22:31:03Z

src/net_processing.cpp

+static bool MaybePunishNode(NodeId nodeid, const CValidationState& state, bool via_compact_block, const std::string& message = "") {
+    int nDoS = state.GetDoS();
+    if (nDoS > 0 && !via_compact_block) {
+         LOCK(cs_main);


In commit "[refactor] Use maybepunish etc" (8226bed)

Note: This acquires lock recursively in PeerLogicValidation::BlockChecked. Seems fine, but just wanted to note it wasn't happening before.

src/validation.cpp

sdaftuar · 2019-01-29T16:52:34Z

I addressed @ryanofsky's comments so far (which rewrote the git history, since one of the commit messages changed, so I also squashed in a comment change as well). Previous version of this PR is now here: https://github.com/sdaftuar/bitcoin/commits/15141.1.

sdaftuar · 2019-02-08T17:57:22Z

This needs a simple rebase, but can I get concept ACK/NACK from more reviewers on whether the reworked form of this PR (which broke things up into many more commits) is preferable compared to the original formulation?

sipa · 2019-02-08T18:43:36Z

I haven't reviewed the last few commits yet (only up to "[refactor] Use Reasons directly instead of DoS codes"), but so far the structure is very clear. Concept ACK on that.

sdaftuar · 2019-02-08T19:31:53Z

Thanks @sipa. Rebased. Prior version is here: 15141.2

sipa · 2019-02-08T20:19:20Z

One overall comment: it seems there is a subset of ValidationInvalidReasons that are valid for transactions, and another subset that is valid for blocks. Perhaps it's useful to have functions to test whether one belongs to those sets, and invoke those functions in assertions after validation returns in their respective contexts. That seems a bit more future-proof than just having comments of the form "CheckTxInputs may return MISSING_INPUTS but we can't return that". It would make me also a bit more comfortable with changes to checks from CorruptionPossible() to testing for a specific invalidity reason (assuming we know TX_WITNESS_MUTATED in the only tx-valid corruptionpossible one, and BLOCK_MUTATED the only block-valid corruptionpossible one).

src/consensus/validation.h

src/net_processing.cpp

ryanofsky

Mostly finished reviewing this, just a few more commits left: #15141 (review)

ryanofsky · 2019-02-11T20:55:41Z

src/consensus/tx_verify.cpp

@@ -160,9 +160,9 @@ bool CheckTransaction(const CTransaction& tx, CValidationState &state, bool fChe
 {
    // Basic checks that don't depend on any context
    if (tx.vin.empty())
-        return state.DoS(10, false, REJECT_INVALID, "bad-txns-vin-empty");
+        return state.DoS(100, false, REJECT_INVALID, "bad-txns-vin-empty");


In commit "Clean up banning levels" (96cedc8)

Note: this is "Txn with empty vin/vout or null prevouts move from 10 DoS points to 100"

ryanofsky · 2019-02-11T20:55:46Z

src/consensus/tx_verify.cpp

    if (tx.vout.empty())
-        return state.DoS(10, false, REJECT_INVALID, "bad-txns-vout-empty");
+        return state.DoS(100, false, REJECT_INVALID, "bad-txns-vout-empty");


In commit "Clean up banning levels" (96cedc8)

Note: this is "Txn with empty vin/vout or null prevouts move from 10 DoS points to 100"

ryanofsky · 2019-02-11T20:56:02Z

src/consensus/tx_verify.cpp

@@ -199,7 +199,7 @@ bool CheckTransaction(const CTransaction& tx, CValidationState &state, bool fChe
    {
        for (const auto& txin : tx.vin)
            if (txin.prevout.IsNull())
-                return state.DoS(10, false, REJECT_INVALID, "bad-txns-prevout-null");
+                return state.DoS(100, false, REJECT_INVALID, "bad-txns-prevout-null");


In commit "Clean up banning levels" (96cedc8)

Note: this is "Txn with empty vin/vout or null prevouts move from 10 DoS points to 100"

ryanofsky · 2019-02-11T20:56:24Z

src/consensus/tx_verify.cpp

-            return state.Invalid(false,
-                REJECT_INVALID, "bad-txns-premature-spend-of-coinbase",
+            return state.DoS(100, false,
+                REJECT_INVALID, "bad-txns-premature-spend-of-coinbase", false,


In commit "Clean up banning levels" (96cedc8)

Note: this is "Inclusion of a premature coinbase spend now results in a ban"

ryanofsky · 2019-02-11T20:56:51Z

src/validation.cpp

@@ -763,7 +763,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
            const uint256 &hashAncestor = ancestorIt->GetTx().GetHash();
            if (setConflicts.count(hashAncestor))
            {
-                return state.DoS(10, false,
+                return state.DoS(100, false,


In commit "Clean up banning levels" (96cedc8)

Note: this is "Loose transactions with a dependency loop now result in a ban instead of 10 DoS points"

ryanofsky · 2019-02-11T20:59:01Z

src/validation.cpp

@@ -3271,7 +3271,7 @@ static bool ContextualCheckBlock(const CBlock& block, CValidationState& state, c
    // Check that all transactions are finalized
    for (const auto& tx : block.vtx) {
        if (!IsFinalTx(*tx, nHeight, nLockTimeCutoff)) {
-            return state.DoS(10, false, REJECT_INVALID, "bad-txns-nonfinal", false, "non-final transaction");
+            return state.DoS(100, false, REJECT_INVALID, "bad-txns-nonfinal", false, "non-final transaction");


In commit "Clean up banning levels" (96cedc8)

Note: this is "Inclusion of non-final transactions in a block now results in a ban"

ryanofsky · 2019-02-11T21:17:58Z

src/validation.cpp

@@ -901,7 +901,8 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
            if (!tx.HasWitness() && CheckInputs(tx, stateDummy, view, true, scriptVerifyFlags & ~(SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_CLEANSTACK), true, false, txdata) &&
                !CheckInputs(tx, stateDummy, view, true, scriptVerifyFlags & ~SCRIPT_VERIFY_CLEANSTACK, true, false, txdata)) {
                // Only the witness is missing, so the transaction itself may be fine.
-                state.SetCorruptionPossible();
+                state.DoS(0, ValidationInvalidReason::TX_WITNESS_MUTATED, false,


In commit "[refactor] Add useful-for-dos "reason" field to CValidationState" (ac3873e)

Passing state.GetDoS() instead of 0 might make it clearer behavior isn't changing.

I agree that this is confusing. IMO, even better than passing in state.GetDoS() to state.DoS() would be:

state.Invalid(ValidationInvalidReason::TX_WITNESS_MUTATED, false, state.GetRejectCode(), state.GetRejectReason(), state.GetDebugMessage()); state.SetCorruptionPossible();

ryanofsky · 2019-02-11T21:33:17Z

src/consensus/validation.h

            return 100;
        case ValidationInvalidReason::BLOCK_MISSING_PREV:
            return 10;
        case ValidationInvalidReason::CACHED_INVALID:
        case ValidationInvalidReason::RECENT_CONSENSUS_CHANGE:
        case ValidationInvalidReason::BLOCK_BAD_TIME:
        case ValidationInvalidReason::TX_NOT_STANDARD:
+        case ValidationInvalidReason::TX_MISSING_INPUTS:


In commit "TX_MISSING_INPUTS now has a DoS score of 0" (bee1d4f)

Not sure, but it seems like it might be nice to have a comment here saying TX_MISSING_INPUTS reason will change to CONSENSUS if the transaction is included in a block, and lead to a higher dos score in that case.

ryanofsky · 2019-02-11T21:52:40Z

src/net_processing.cpp

@@ -1558,7 +1558,7 @@ bool static ProcessHeadersMessage(CNode *pfrom, CConnman *connman, const std::ve
    CBlockHeader first_invalid_header;
    if (!ProcessNewBlockHeaders(headers, state, chainparams, &pindexLast, &first_invalid_header)) {
        if (state.IsInvalid()) {
-            if (punish_duplicate_invalid && LookupBlockIndex(first_invalid_header.GetHash())) {
+            if (punish_duplicate_invalid && state.GetReason() == ValidationInvalidReason::CACHED_INVALID) {


In commit "LookupBlockIndex -> CACHED_INVALID" (c558eba)

Note: behavior should be unchanged here because ProcessNewBlockHeaders calls AcceptBlockHeader which sets CACHED_INVALID if a header is already known and has BLOCK_FAILED_MASK is set. If the first invalid header is invalid for a different reason the check shouldn't trigger either before or after this change.

src/net_processing.cpp

sdaftuar · 2019-05-03T13:40:53Z

@laanwj Thanks for the review. I updated the PR to drop that commit, so that we once again use a CValidationState to return the debug information if CheckTransaction() fails for a transaction in a block.

Previous version of the code is at 15141.14.

jnewbery · 2019-05-03T14:10:21Z

utACK 0ff1c2a

~~Only change is dropping the Drop obsolete sigops comment commit.~~

EDIT: Copy-paste error above. The only change was dropping the Remove redundant state.Invalid() call after CheckTransaction() commit.

ryanofsky

utACK 0ff1c2a. Only change is dropping the first commit (f3883a3), and dropping the temporary assert(level == GetDoS()) that was in 35ee77f (now c8b0d22)

laanwj · 2019-05-04T09:59:19Z

thanks for addressing my comment, and sorry for holding this up last-minute
utACK 0ff1c2a

…sing 0ff1c2a Separate reason for premature spends (coinbase/locktime) (Suhas Daftuar) 54470e7 Assert validation reasons are contextually correct (Suhas Daftuar) 2120c31 [refactor] Update some comments in validation.cpp as we arent doing DoS there (Matt Corallo) 12dbdd7 [refactor] Drop unused state.DoS(), state.GetDoS(), state.CorruptionPossible() (Matt Corallo) aa502b8 scripted-diff: Remove DoS calls to CValidationState (Matt Corallo) 7721ad6 [refactor] Prep for scripted-diff by removing some \ns which annoy sed. (Matt Corallo) 5e78c57 Allow use of state.Invalid() for all reasons (Matt Corallo) 6b34bc6 Fix handling of invalid headers (Suhas Daftuar) ef54b48 [refactor] Use Reasons directly instead of DoS codes (Matt Corallo) 9ab2a04 CorruptionPossible -> BLOCK_MUTATED (Matt Corallo) 6e55b29 CorruptionPossible -> TX_WITNESS_MUTATED (Matt Corallo) 7df16e7 LookupBlockIndex -> CACHED_INVALID (Matt Corallo) c8b0d22 [refactor] Drop redundant nDoS, corruptionPossible, SetCorruptionPossible (Matt Corallo) 34477cc [refactor] Add useful-for-dos "reason" field to CValidationState (Matt Corallo) 6a7f877 Ban all peers for all block script failures (Suhas Daftuar) 7b99910 Clean up banning levels (Matt Corallo) b8b4c80 [refactor] drop IsInvalid(nDoSOut) (Matt Corallo) 8818729 [refactor] Refactor misbehavior ban decisions to MaybePunishNode() (Matt Corallo) 00e11e6 [refactor] rename stateDummy -> orphan_state (Matt Corallo) f34fa71 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 0ff1c2a ryanofsky: utACK 0ff1c2a. Only change is dropping the first commit (f3883a3), and dropping the temporary `assert(level == GetDoS())` that was in 35ee77f (now c8b0d22) Tree-SHA512: e915a411100876398af5463d0a885920e44d473467bb6af991ef2e8f2681db6c1209bb60f848bd154be72d460f039b5653df20a6840352c5f7ea5486d9f777a3

For bitcoin/bitcoin#15141, rewrite all the state.Invalid calls made when validating name transactions.

sdaftuar · 2020-02-05T15:09:25Z

src/validation.cpp


        // Check for non-standard witness in P2WSH
        if (tx.HasWitness() && fRequireStandard && !IsWitnessStandard(tx, view))
-            return state.DoS(0, false, REJECT_NONSTANDARD, "bad-witness-nonstandard", true);
+            return state.Invalid(ValidationInvalidReason::TX_WITNESS_MUTATED, false, REJECT_NONSTANDARD, "bad-witness-nonstandard");


@TheBlueMatt I was tracking down our use of TX_WITNESS_MUTATED and the introduction of it here seems like a bug -- any idea why we did this? TX_NOT_STANDARD seems more correct, and actually I think now that we should rename TX_WITNESS_MUTATED to TX_WITNESS_STRIPPED to make it clear how we use it.

It was my understanding that this could trigger in case the witness was malleated by a third part (making it nonstandard), which was the original definition for WITNESS_MUTATED.

Summary: Co-authored-by: Anthony Towns <aj@erisian.com.au> Suhas Daftuar <sdaftuar@gmail.com> This is a partial backport of Core [[bitcoin/bitcoin#15141 | PR15141]] : bitcoin/bitcoin@00e11e6 Test Plan: ninja Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D6673

Summary: Isolate the decision of whether to ban a peer to one place in the code, rather than having it sprinkled throughout net_processing. Co-authored-by: Anthony Towns <aj@erisian.com.au> Suhas Daftuar <sdaftuar@gmail.com> John Newbery <john@johnnewbery.com> This is a partial backport of Core [[bitcoin/bitcoin#15141 | PR15141]] : bitcoin/bitcoin@8818729 Slightly modified because the original screws with locks and only fixes it toward the end. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D6683

Summary: Co-authored-by: Anthony Towns <aj@erisian.com.au> This is a partial backport of Core [[bitcoin/bitcoin#15141 | PR15141]] : bitcoin/bitcoin@b8b4c80 Depends on D6683 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D6684

Summary: 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> This is a partial backport of Core [[bitcoin/bitcoin#15141 | PR15141]] : bitcoin/bitcoin@7b99910 Depends on D6684 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D6685

Summary: This is a first step towards cleaning up our DoS interface - make validation return *why* something is invalid, and let net_processing figure out what that implies in terms of banning/disconnection/etc. Behavior change: peers will now be banned for providing blocks with premature coinbase spends. Co-authored-by: Anthony Towns <aj@erisian.com.au> Suhas Daftuar <sdaftuar@gmail.com> This is a partial backport of Core [[bitcoin/bitcoin#15141 | PR15141]] : bitcoin/bitcoin@34477cc Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D6698

…ible Summary: Co-authored-by: Anthony Towns <aj@erisian.com.au> This is a partial backport of Core [[bitcoin/bitcoin#15141 | PR15141]] : bitcoin/bitcoin@c8b0d22 Depends on D6698 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D6699

Summary: Co-authored-by: Anthony Towns <aj@erisian.com.au> This is a partial backport of Core [[bitcoin/bitcoin#15141 | PR15141]] : bitcoin/bitcoin@7df16e7 Depends on D6698 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D6700

Summary: Co-authored-by: Anthony Towns <aj@erisian.com.au> This is a partial backport of Core [[bitcoin/bitcoin#15141 | PR15141]] : bitcoin/bitcoin@9ab2a04 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D6707

Summary: This is a partial backport of Core [[bitcoin/bitcoin#15141 | PR15141]] : bitcoin/bitcoin@ef54b48 Depends on D6707 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D6708

Summary: We only disconnect outbound peers (excluding HB compact block peers and manual connections) when receiving a CACHED_INVALID header. This is a partial backport of Core [[bitcoin/bitcoin#15141 | PR15141]] : bitcoin/bitcoin@6b34bc6 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D6727

Summary: Co-authored-by: Anthony Towns <aj@erisian.com.au> This is a partial backport of Core [[bitcoin/bitcoin#15141 | PR15141]] : bitcoin/bitcoin@5e78c57 Depends on D6727 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D6728

Summary: Co-authored-by: Anthony Towns <aj@erisian.com.au> This is a partial backport of Core [[bitcoin/bitcoin#15141 | PR15141]] : bitcoin/bitcoin@6e55b29 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D6734

…ossible() Summary: Co-authored-by: Anthony Towns <aj@erisian.com.au> This is a partial backport of Core [[bitcoin/bitcoin#15141 | PR15141]] : bitcoin/bitcoin@12dbdd7 Depends on D6734 and D6736 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D6737

…oS there Summary: This is a backport of Core [[bitcoin/bitcoin#15141 | PR15141]] : bitcoin/bitcoin@2120c31 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D6738

Summary: This is a patial backport of Core [[bitcoin/bitcoin#15141 | PR15141]] : bitcoin/bitcoin@54470e7 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D6742

Summary: This is a partial backport of Core [[bitcoin/bitcoin#15141 | PR15141]] : bitcoin/bitcoin@0ff1c2a Depends on D6742 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D6743

Summary: 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf [validation] Remove fMissingInputs from AcceptToMemoryPool() (John Newbery) c428622a5bb1e37b2e6ab2c52791ac05d9271238 [validation] Remove unused first_invalid parameter from ProcessNewBlockHeaders() (John Newbery) 7204c6434b944f6ad51b3c895837729d3aa56eea [validation] Remove useless ret parameter from Invalid() (John Newbery) 1a37de4b3174d19a6d8691ae07e92b32fdfaef11 [validation] Remove error() calls from Invalid() calls (John Newbery) 067981e49246822421a7bcc720491427e1dba8a3 [validation] Tidy Up ValidationResult class (John Newbery) a27a2957ed9afbe5a96caa5f0f4cbec730d27460 [validation] Add CValidationState subclasses (John Newbery) Pull request description: Carries out some remaining tidy-ups remaining after PR 15141: - split ValidationState into TxValidationState and BlockValidationState (commit from ajtowns) - various minor code style tidy-ups to the ValidationState class - remove the useless `ret` parameter from `ValidationState::Invalid()` - remove the now unused `first_invalid` parameter from `ProcessNewBlockHeaders()` - remove the `fMissingInputs` parameter from `AcceptToMemoryPool()`, and deal with missing inputs the same way as other errors by using the `TxValidationState` object. Tip for reviewers (thanks ryanofsky!): The first commit ("[validation] Add CValidationState subclasses" ) is huge and can be easier to start reviewing if you revert the rote, mechanical changes: Substitute the commit hash of commit "[validation] Add CValidationState subclasses" for <CommitHash> in the commands below. ```sh git checkout <CommitHash> git grep -l ValidationState | xargs sed -i 's/BlockValidationState\|TxValidationState/CValidationState/g' git grep -l ValidationResult | xargs sed -i 's/BlockValidationResult\|TxValidationResult/ValidationInvalidReason/g' git grep -l MaybePunish | xargs sed -i 's/MaybePunishNode\(ForBlock\|ForTx\)/MaybePunishNode/g' git diff HEAD^ ``` After that it's possible to easily see the mechanical changes with: ```sh git log -p -n1 -U0 --word-diff-regex=. <CommitHash> ``` ACKs for top commit: laanwj: ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf amitiuttarwar: code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf. Also built & ran tests locally. fjahr: Code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf . Only nit style change and pure virtual destructor added since my last review. ryanofsky: Code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf. Just whitespace change and pure virtual destructor added since last review. Tree-SHA512: 511de1fb380a18bec1944ea82b513b6192df632ee08bb16344a2df3c40811a88f3872f04df24bc93a41643c96c48f376a04551840fd804a961490d6c702c3d36 Backport of Core [[bitcoin/bitcoin#15921 | PR15921]] and [[bitcoin/bitcoin#17624 | PR17624]] (small fix to 15921) Followup to [[bitcoin/bitcoin#15141 | PR15141]] Some differences reviewers will encounter: 1. RejectCodes (such as REJECT_INVALID) do not appear in the original PR due to an out-of-order backport, but we currently still support them, so it was retained. 2. Some files (notably tests) contain refactors not present in the original PR. This is due to a few out-of-order backports but otherwise harmless. Depends on D6923, D6929, D6930 Test Plan: `ninja check check-functional-extended` Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6860

laanwj added the Validation label Jan 10, 2019

jnewbery reviewed Jan 14, 2019

View reviewed changes

sdaftuar commented Jan 15, 2019

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

sdaftuar mentioned this pull request Jan 15, 2019

Parallelize CheckInputs() in AcceptToMemoryPool() #15169

Closed

ajtowns reviewed Jan 15, 2019

View reviewed changes

ryanofsky reviewed Jan 15, 2019

View reviewed changes

ajtowns reviewed Jan 18, 2019

View reviewed changes

sdaftuar force-pushed the 2019-01-rewrite-validation-interface branch from a642744 to 7e0090c Compare January 24, 2019 18:21

ryanofsky reviewed Jan 24, 2019

View reviewed changes

sdaftuar force-pushed the 2019-01-rewrite-validation-interface branch from 7e0090c to 4e1ae68 Compare January 29, 2019 16:50

DrahtBot added the Needs rebase label Feb 8, 2019

sdaftuar force-pushed the 2019-01-rewrite-validation-interface branch from fd047f7 to 7682566 Compare February 8, 2019 19:30

DrahtBot removed the Needs rebase label Feb 8, 2019

sipa reviewed Feb 8, 2019

View reviewed changes

src/consensus/validation.h Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Show resolved Hide resolved

bitcoin deleted a comment from ZeusNightBolt Feb 10, 2019

ryanofsky reviewed Feb 11, 2019

View reviewed changes

sdaftuar force-pushed the 2019-01-rewrite-validation-interface branch from fdd7683 to 0ff1c2a Compare May 3, 2019 13:37

ryanofsky approved these changes May 3, 2019

View reviewed changes

laanwj merged commit 0ff1c2a into bitcoin:master May 4, 2019

domob1812 added a commit to domob1812/namecoin-core that referenced this pull request May 6, 2019

Merge branch 'auxpow'

7c2a551

For bitcoin/bitcoin#15141, rewrite all the state.Invalid calls made when validating name transactions.

Empact mentioned this pull request Jun 5, 2019

Build: enable -Wdocumentation via isystem #14920

Closed

jnewbery mentioned this pull request Aug 22, 2019

Replace -banscore with -ignoremisbehaviour #16683

Closed

ajtowns mentioned this pull request Sep 5, 2019

Relax punishment for peers relaying invalid blocks and headers #10593

Closed

sdaftuar commented Feb 5, 2020

View reviewed changes

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

	bool Invalid(bool ret = false,
	unsigned int _chRejectCode=0, const std::string &_strRejectReason="",
	const std::string &_strDebugMessage="") {
	return DoS(0, ret, _chRejectCode, _strRejectReason, false, _strDebugMessage);
	}

	* Set mapBlockSource[hash].second to false if the node should not be
	* punished if the block is invalid.

Rewrite DoS interface between validation and net_processing #15141

Rewrite DoS interface between validation and net_processing #15141

Uh oh!

Conversation

sdaftuar commented Jan 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jan 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

practicalswift commented Jan 10, 2019

Uh oh!

sipa commented Jan 11, 2019

Uh oh!

laanwj commented Jan 14, 2019

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajtowns commented Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdaftuar commented Jan 18, 2019

Uh oh!

ryanofsky commented Jan 18, 2019

sdaftuar commented Jan 10, 2019 •

edited

Loading

DrahtBot commented Jan 10, 2019 •

edited

Loading

ajtowns commented Jan 18, 2019 •

edited

Loading

ryanofsky left a comment •

edited

Loading

sdaftuar commented Feb 8, 2019 •

edited

Loading

sipa commented Feb 8, 2019 •

edited

Loading

ryanofsky left a comment •

edited

Loading