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

    // 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

    }
-    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

                // 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

        // "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

    // 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

                              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

            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

            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 32747d0746d91a8f63e39cedfb232f8c36b33bc6 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

    // 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

+        return state.DoS(100, false, REJECT_INVALID, "bad-txns-vin-empty");
    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

        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

            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

    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

                !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

        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

    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

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.

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

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