Rewrite the interface between validation and net_processing wrt DoS #11639

TheBlueMatt · 2017-11-08T21:17:00Z

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.

TheBlueMatt · 2017-12-15T22:10:09Z

Rebased.

promag

Concept ACK.

Do you think it's reasonable to treat ValidationInvalidReason like CONSENSUS | MISSING_INPUTS, so you could accumulate reasons instead of overwriting?

promag · 2017-12-20T14:15:55Z

src/test/txvalidation_tests.cpp

-    BOOST_CHECK_EQUAL(state.IsInvalid(nDoS), true);
-    BOOST_CHECK_EQUAL(nDoS, 100);
+    BOOST_CHECK_EQUAL(state.IsInvalid(), true);
+    BOOST_CHECK(state.GetReason() == ValidationInvalidReason::CONSENSUS);


Also BOOST_CHECK_EQUAL?

Doesn't compile as GetReason() can't be printed.

Not worthy of revisiting here, but fyi I added a facility here to support BOOST_CHECK_EQUAL for enum: https://github.com/bitcoin/bitcoin/pull/13076/files#diff-4a0cb5ad5e93d187a1b065a99bbae143

promag · 2017-12-20T14:25:50Z

src/validation.cpp

@@ -1906,11 +1907,15 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
        {
            CAmount txfee = 0;
            if (!Consensus::CheckTxInputs(tx, state, view, pindex->nHeight, txfee)) {
+                // CheckTxInputs may return MISSING_INPUTS but we can't return that, as
+                // its nonsense for a block, so we reset the reason flag to CONSENSUS here.
+                state.Invalid(ValidationInvalidReason::CONSENSUS, false,


Make it CONSENSUS only if NOT_STANDARD? I know that atm only those 2 are set but, imo, it's more clear to do so inside that condition:

if (state.GetReason() == ValidationInvalidReason::NOT_STANDARD) { state.Invalid(ValidationInvalidReason::CONSENSUS, ...); }

The same happens in line 1951.

ATM CheckTxInputs set MISSING_INPUTS and CONSENSUS reasons and only makes sense to override when reason is MISSING_INPUTS:

if (state.GetReason() == ValidationInvalidReason::MISSING_INPUTS) { state.Invalid(ValidationInvalidReason::CONSENSUS, ...); }

Errrr....fixed, actually this time.

promag · 2018-01-09T11:27:02Z

src/validation.cpp

@@ -1906,11 +1907,15 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
        {
            CAmount txfee = 0;
            if (!Consensus::CheckTxInputs(tx, state, view, pindex->nHeight, txfee)) {
+                // CheckTxInputs may return MISSING_INPUTS but we can't return that, as
+                // its nonsense for a block, so we reset the reason flag to CONSENSUS here.
+                state.Invalid(ValidationInvalidReason::CONSENSUS, false,


ATM CheckTxInputs set MISSING_INPUTS and CONSENSUS reasons and only makes sense to override when reason is MISSING_INPUTS:

if (state.GetReason() == ValidationInvalidReason::MISSING_INPUTS) { state.Invalid(ValidationInvalidReason::CONSENSUS, ...); }

promag · 2018-01-09T11:38:26Z

src/consensus/validation.h

+  */
+enum class ValidationInvalidReason {
+    // txn and blocks:
+    VALID,           //!< not actually invalid


ValidationInvalidReason::NONE instead?

Sure, sounds good.

sdaftuar · 2018-02-27T18:30:07Z

src/validation.cpp

+                    // CheckTxInputs may return MISSING_INPUTS but we can't return that, as
+                    // its nonsense for a block, so we reset the reason flag to CONSENSUS here.
+                    state.DoS(state.GetDoS(), ValidationInvalidReason::CONSENSUS, false,
+                              state.GetRejectCode(), state.GetRejectReason(), state.GetDebugMessage());


The arguments don't match here:

validation.cpp:1921:79: error: no viable conversion from 'std::string' (aka 'basic_string<char, char_traits<char>, allocator<char> >') to 'bool' state.GetRejectCode(), state.GetRejectReason(), state.GetDebugMessage());

but more generally I think this would be a lot more readable if we could just add an explicit way to set the invalid reason on a CValidationState, rather than invoke DoS() in this way (here and elsewhere as well).

This seems fixed now, @sdaftuar?

sdaftuar

Concept ACK, but don't think we need SOFT_FORK to be used anywhere now.

sdaftuar · 2018-02-27T18:34:21Z

src/net_processing.cpp

+    switch (state.GetReason()) {
+    case ValidationInvalidReason::NONE:
+        return false;
+    // The node is mean:


I don't quite understand this comment?

I also don't understand this comment!

sdaftuar · 2018-02-27T18:36:09Z

src/net_processing.cpp

+    case ValidationInvalidReason::CONSENSUS:
+    case ValidationInvalidReason::MUTATED:
+    case ValidationInvalidReason::UNKNOWN_INVALID: // Really "duplicate of invalid "
+        if (via_compact_block) return false;


nit: I personally find this fall-through for the non-compact block case harder to follow and more fragile than having each case end with a return or break.

Agree. an else return true would be clearer here.

Actually, I think this would be clearer if it wasn't a switch statement at all. There are only a handful of cases where we return true. Explicitly test them and return false otherwise.

Not addressed.

Fall through case statements are confusing.

sdaftuar · 2018-02-27T19:09:23Z

src/validation.cpp

@@ -1866,7 +1874,7 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
        for (const auto& tx : block.vtx) {
            for (size_t o = 0; o < tx->vout.size(); o++) {
                if (view.HaveCoin(COutPoint(tx->GetHash(), o))) {
-                    return state.DoS(100, error("ConnectBlock(): tried to overwrite transaction"),
+                    return state.DoS(100, ValidationInvalidReason::SOFT_FORK, error("ConnectBlock(): tried to overwrite transaction"),


I think this should probably be CONSENSUS rather than SOFT_FORK.

We should figure out where we want to draw a line between CONSENSUS (ban the peer) and SOFT_FORK (ignore the peer). Obviously this case is a SOFT_FORK, but at this point its so old that it may not be worth keeping that distinction. I aired on the side of "let any node, no matter how old, not get banned" in general.

sdaftuar · 2018-02-27T19:16:57Z

src/validation.cpp

@@ -3136,7 +3164,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(10, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-nonfinal", false, "non-final transaction");


Note that this could be "SOFT_FORK" (BIP 113). But as I wrote elsewhere I think we should just use CONSENSUS everywhere in place of SOFT_FORK, for now.

sdaftuar · 2018-02-27T19:19:45Z

src/validation.cpp

-        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
+        // us with witness data can be expected to support SegWit validation.
+        return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-blk-weight", false, strprintf("%s : weight limit failed", __func__));


If we're going to use this line of reasoning, then I think we may as well also not use SOFT_FORK anywhere else either (at least for invalid blocks, perhaps loose transactions should be different) -- presumably no nodes that support segwit validation should be unaware of any of the prior soft forks.

Huh? Doing so would break the ability of a pre-segwit node to provide us a non-segwit-but-valid block. Further, this line of reasoning does not assume that we only download blocks from segwit-enabled peers (which may be the case in practice in our current net layer, but making that assumption strong in validation seems bad). The line of reasoning here is only applicable to nodes that provided the witness, not nodes that (for some reason) does not.

sdaftuar · 2018-02-27T19:39:27Z

src/validation.cpp

+                    // CheckInputs may return NOT_STANDARD for extra flags we passed,
+                    // but we can't return that, as its nonsense, so we reset the
+                    // reason flag to SOFT_FORK here.
+                    state.DoS(state.GetDoS(), ValidationInvalidReason::SOFT_FORK, false,


As all parallel-validation script failures are SOFT_FORK further down, I think we ought to make the non-parallel validation case be the same, rather than filter for NOT_STANDARD first.

Since our blocks are only coming from segwit peers, I actually think we should just change all these to CONSENSUS anyway, and leave a comment explaining that when we deploy a new soft fork, we need to be careful about not splitting the network.

I'm confused. The NOT_STANDARD return here is to keep behavior the same for transactions as well as blocks. NOT_STANDARD is not a valid response for block ValidationReasons, so we have to swap it here.

Re 1), I'm just saying that we should always rewrite the reason to be the same as we'd get if we did parallel validation (which is SOFT_FORK in this PR currently), versus only return SOFT_FORK if the reason was NOT_STANDARD. It doesn't really make sense for ConnectBlock() to behave differently with regard to error codes if using single vs parallel script checking.

sdaftuar · 2018-02-27T19:51:26Z

src/validation.cpp

@@ -3168,11 +3189,11 @@ static bool ContextualCheckBlock(const CBlock& block, CValidationState& state, c
            // already does not permit it, it is impossible to trigger in the
            // witness tree.
            if (block.vtx[0]->vin[0].scriptWitness.stack.size() != 1 || block.vtx[0]->vin[0].scriptWitness.stack[0].size() != 32) {
-                return state.DoS(100, false, REJECT_INVALID, "bad-witness-nonce-size", true, strprintf("%s : invalid witness nonce size", __func__));
+                return state.Invalid(ValidationInvalidReason::MUTATED, false, REJECT_INVALID, "bad-witness-nonce-size", strprintf("%s : invalid witness nonce size", __func__));


Not really obvious here why we use MUTATED rather than WITNESS_MUTATED, here and in the next few lines (other than needing to match the code in InvalidBlockFound() and AcceptBlock(), which only check against MUTATED). Perhaps add a comment explaining the usage somewhere?

WITNESS_MUTATED is only for transactions. In the future we could separate out the InvalidReason into two different ones for transactions/blocks.

TheBlueMatt · 2018-03-02T15:35:57Z

Commented on the points I disagreed with, I obviously strongly disagree about making assumptions about the net_processing layer in validation, as that's been one of my overarching goals over the past few years. Will address the other points when I get a chance to rebase.

sdaftuar · 2018-03-04T14:38:52Z

The more I think about this, the more I think we should drop the distinction between "SOFT_FORK" and "CONSENSUS" altogether. At least when it comes to block processing, the only concept that matters is CONSENSUS.

The reason we care to distinguish SOFT_FORK from CONSENSUS is to highlight recent rule changes that our peers may not know about, so that the net_processing layer can optionally do something different for SOFT_FORK violations than it would do for CONSENSUS violations. However I don't think this is a terribly meaningful distinction. The main goals of the net_processing/net layers, with respect to these validation issues, is:

keep us connected to the honest network
prevent attackers from DoS'ing us
don't needlessly partition old nodes that have fallen back to SPV security because they're unaware of the consensus rules

So what is our strategy for each of these points? For keeping us connected to the honest network, I think we mainly rely on having robust outbound peers -- any outbound peer that is not enforcing all the consensus rules should be disconnected. Combined with the mitigations we deployed last fall for outbound peers whose tips aren't keeping up, and for protection in the case that our tip hasn't advanced in a while, I believe this is sufficient. (See https://gist.github.com/sdaftuar/c2a3320c751efb078a7c1fd834036cb0 for more details around my thinking here.)

For not needlessly partitioning old nodes -- I think it's sufficient to not disconnect inbound peers that relay blocks that violate consensus rules, regardless of whether the rule is a recent soft fork or not.

And to prevent DoS -- I think we just ensure that any blocks have valid proof-of-work, combined with protections against processing blocks that are too far behind our tip (or on a too-little-work-chain).

In all of these cases, I don't think we need to distinguish "SOFT_FORK" from "CONSENSUS". And as its pretty arbitrary about when to decide to move something from the SOFT_FORK designation to CONSENSUS, I think we may as well not bother.

TheBlueMatt · 2018-04-05T15:52:14Z

Discussed this offline with @sdaftuar for a bit - I'm OK with redefining "SOFT_FORK" to mean "any future SOFT_FORK after segwit, so is currently unused", but I'd rather leave it there as we'd certainly need it for "any future SOFT_FORK". Happy to do so if others like. Still want more review here, though.

sdaftuar · 2018-04-05T17:01:41Z

No objection to leaving "SOFT_FORK" in for future use (even if I'm not sure we'll ever use it, I am open to being convinced). Nit: I would somewhat prefer to call it something other than "SOFT_FORK" -- perhaps "RECENT_CONSENSUS_CHANGE" or something else that indicates the relevant substance rather than the form?

sipa · 2018-04-05T18:15:33Z

Big concept ACK, I'm happy to see the DoS scores get out of the network processing logic.

As for SOFT_FORK vs CONSENSUS: I like calling it RECENT_CONSENSUS_CHANGE or something similar, and making it match whatever we're currently not banning for. The discussion about how to deal with softforks and banning and DoS protection in general is more complicated and can be continued later.

TheBlueMatt · 2018-04-05T18:47:03Z

@sipa well, current master consider RECENT_CONSENSUS_CHANGE/not-banning as any soft fork, ever, kinda, but also some other stuff, but not SegWit things. Its kinda a grab-bag as @sdaftuar points out. This PR cleaned it up somewhat but aired on the side of "all soft-forks", @sdaftuar wants it to air more on the side of "things after segwit", which I think makes sense. Leaving it the way it is on master kinda makes no sense (and would be hard to do).

sdaftuar · 2018-04-05T20:15:14Z

In the interests of advancing this refactor, I'm fine with doing whatever is least controversial (presumably, less change to existing behavior) in this PR, and saving the larger behavioral changes for a future PR.

TheBlueMatt · 2018-04-16T17:11:26Z

Rebased and changed SOFT_FORK to RECENT_CONSENSUS_CHANGE, redefining it to mean "change more recent than segwit" (ie it is currently unused, as @sdaftuar requested).

skeees · 2018-04-16T17:12:32Z

Concept ack to the motivation - while validation shouldn't be deciding how many points to assign peers, you could argue that it should be telling net processing "this is rejected because I don't have enough information to assess that it is valid" vs. "this is invalid because it was maliciously constructed" - it would be nice from a readability perspective if that were more immediately apparent (e.g. by categorizing the reason names or the like)

TheBlueMatt · 2018-04-16T17:32:33Z

I think the only cases where we can say "I'm missing something this is based on" is ValidationInvalidReason::MISSING_PREV and MISSING_INPUTS, which are pretty self-explanitory. Open to suggestions if you had something else in mind (or a comment to clarify).

maflcko · 2018-04-16T18:01:49Z

src/validation.cpp

-                                 strprintf("Transaction check failed (tx hash %s) %s", tx->GetHash().ToString(), state.GetDebugMessage()));
+    for (const auto& tx : block.vtx) {
+        if (!CheckTransaction(*tx, state, false)) {
+            LogPrintf("Transaction check failed (tx hash %s) %s", tx->GetHash().ToString(), state.GetDebugMessage());


from the linter: "All calls to LogPrintf() should be terminated with \n"

TheBlueMatt · 2018-04-27T17:54:53Z

Rebased and addressed all outstanding feedback (I believe). I didnt opt for shortening the name of "ValidationInvalidReason"...I dont think we should be shortening unless things are just as clear as the more verbose name.

maflcko · 2018-04-27T20:19:53Z

src/net_processing.cpp

                            setMisbehaving.insert(fromPeer);
                            LogPrint(BCLog::MEMPOOL, "   invalid orphan tx %s\n", orphanHash.ToString());
                        }
                        // Has inputs but not accepted to mempool
                        // Probably non-standard or insufficient fee
                        LogPrint(BCLog::MEMPOOL, "   removed orphan tx %s\n", orphanHash.ToString());
                        vEraseQueue.push_back(orphanHash);
-                        if (!orphanTx.HasWitness() && !stateDummy.CorruptionPossible()) {
+                        if (!orphanTx.HasWitness() && state.GetReason() != ValidationInvalidReason::TX_WITNESS_MUTATED) {


Should this be orphan_state instead of state?

jnewbery · 2018-04-30T14:20:42Z

utACK f29c11a45edb138711ae7004b3b550ba416fc215

sipa · 2018-05-01T00:04:07Z

utACK f29c11a45edb138711ae7004b3b550ba416fc215

maflcko · 2018-05-08T22:30:48Z

weak utACK f29c11a45edb138711ae7004b3b550ba416fc215 (Did a single pass over all commits)

sipa · 2018-05-13T18:42:28Z

Needs rebase due to #13185.

TheBlueMatt · 2018-05-13T19:26:00Z

(trivially) rebased. Would love another once-over from @sdaftuar.

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.

CValidationInterface's new GetReason() field provides more than enough information for net_processing to determine what to do with the peer, use that instead of trusting validation's idea of what should result in a DoS ban. 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. * Any pre-segwit soft-fork errors (ie all soft-fork errors) now result in a ban. * 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.

-BEGIN VERIFY SCRIPT- sed -i 's/\.DoS(\(.*\), REJECT_\(.*\), \(true\|false\)/.DoS(\1, REJECT_\2/' src/validation.cpp src/consensus/tx_verify.cpp sed -i 's/state.GetRejectCode(), state.GetRejectReason(), state.CorruptionPossible(), state.GetDebugMessage())/state.GetRejectCode(), state.GetRejectReason(), state.GetDebugMessage())/' src/validation.cpp sed -i 's/\.DoS([^,]*, /.Invalid\(/' src/validation.cpp src/consensus/tx_verify.cpp -END VERIFY SCRIPT-

sipa · 2018-05-14T17:16:36Z

re-utACK e73cf51 (verified that the only diff since my previous ACK after a rebase is dealing with #11423 and #13185).

sdaftuar

Looks pretty good; left some nits and spotted a couple behavior changes with headers processing that should be addressed.

sdaftuar · 2018-05-14T12:18:37Z

src/consensus/validation.h

+     * TODO: Currently this is only used if the transaction already exists in the mempool or on chain,
+     * TODO: ATMP's fMissingInputs and a valid CValidationState being used to indicate missing inputs
+     */
+    TX_CONFLICT,


nit: TX_CONFLICT for conflicting with an in-mempool transaction makes sense to me, but it's not clear to me why we'd also include 'duplicate-transaction-in-chain' and 'conflicts-with-an-in-chain-transaction' as a reason, which would be covered by TX_MISSING_INPUTS?

sdaftuar · 2018-05-14T12:27:11Z

src/validation.cpp

@@ -660,7 +660,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
                for (size_t out = 0; out < tx.vout.size(); out++) {
                    // Optimistically just do efficient check of cache for outputs
                    if (pcoinsTip->HaveCoinInCache(COutPoint(hash, out))) {
-                        return state.Invalid(false, REJECT_DUPLICATE, "txn-already-known");
+                        return state.Invalid(ValidationInvalidReason::TX_CONFLICT, false, REJECT_DUPLICATE, "txn-already-known");


nit: I commented elsewhere that I thought this may make more sense as MISSING_INPUTS, but either way, it would be nice to add a comment explaining why we're not using CONSENSUS here. (I think it's because relay delays can cause transaction to propagate after a block is found, and CONSENSUS errors are for things that should never be propagated in the first place -- is there another reason?)

sdaftuar · 2018-05-14T12:30:24Z

src/validation.cpp

@@ -683,7 +683,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
        // Must keep pool.cs for this unless we change CheckSequenceLocks to take a
        // CoinsViewCache instead of create its own
        if (!CheckSequenceLocks(tx, STANDARD_LOCKTIME_VERIFY_FLAGS, &lp))
-            return state.DoS(0, false, REJECT_NONSTANDARD, "non-BIP68-final");
+            return state.DoS(0, ValidationInvalidReason::TX_NOT_STANDARD, false, REJECT_NONSTANDARD, "non-BIP68-final");


nit: This seems fine, but I did note that for premature coinbase spends, we return MISSING_INPUTS, whereas for premature bip68 spends, we're returning NOT_STANDARD. Seems like those could be the same reason?

sdaftuar · 2018-05-14T12:43:18Z

src/validation.cpp

@@ -1971,11 +1971,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) {


nit: this seems brittle, as a future change to CheckTxInputs that returns a new reason code would break this logic. Can we just always overwrite the reason to be CONSENSUS, or perhaps overwrite it to be CONSENSUS if the reason code is not a valid block-reason?

sdaftuar · 2018-05-15T14:47:07Z

src/validation.cpp

-            if (!CheckInputs(tx, state, view, fScriptChecks, flags, fCacheResults, fCacheResults, txdata[i], nScriptCheckThreads ? &vChecks : nullptr))
+            if (!CheckInputs(tx, state, view, fScriptChecks, flags, fCacheResults, fCacheResults, txdata[i], nScriptCheckThreads ? &vChecks : nullptr)) {
+                if (state.GetReason() == ValidationInvalidReason::NOT_STANDARD) {
+                    // CheckInputs may return NOT_STANDARD for extra flags we passed,


nit: Similar to my nit above for handling CheckTxInputs() response, it seems brittle to look for one kind of error code and only overwrite to CONSENSUS in that case. If someone changes CheckInputs() in the future to return other TX invalid-reasons, then this logic won't be correct, and this would be an easy oversight.

I would suggest that we always overwrite the reason here to be CONSENSUS (leaving in the comment about potentially needing to be careful to distinguish between RECENT_CONSENSUS_CHANGE in the future) so that this logic -- which only triggers in the case where we're not doing parallel script checks -- matches the invalid reason given at line 2051 below.

sdaftuar · 2018-05-15T14:57:40Z

src/consensus/validation.h

+     */
+    RECENT_CONSENSUS_CHANGE,
+    CACHED_INVALID,          //!< this object was cached as being invalid, but we don't know why
+    // Only blocks:


nit: "(or headers)"?

sdaftuar · 2018-05-15T15:00:11Z

src/net_processing.cpp

+    switch (state.GetReason()) {
+    case ValidationInvalidReason::NONE:
+        return false;
+    // The node is is providing invalid data:


nit: "is is"

sdaftuar · 2018-05-15T17:31:23Z

src/net_processing.cpp

@@ -1485,6 +1476,7 @@ bool static ProcessHeadersMessage(CNode *pfrom, CConnman *connman, const std::ve
                // etc), and not just the duplicate-invalid case.
                pfrom->fDisconnect = true;
            }
+            MaybePunishNode(pfrom->GetId(), state, false, "invalid header received");


I believe this can result in a behavior change, where we would DoS-ban peers that send duplicate headers that are invalid.

sdaftuar · 2018-05-15T17:39:52Z

src/net_processing.cpp

-                    LogPrint(BCLog::NET, "Peer %d sent us invalid header via cmpctblock\n", pfrom->GetId());
-                }
+            if (state.IsInvalid()) {
+                MaybePunishNode(pfrom->GetId(), state, true, "invalid header via cmpctblock");


This also appears to be a behavior change; some headers failures currently result in a Misbehaving call, but as-is I believe compact block peers are never punished for errors here?

Empact · 2018-05-16T05:22:00Z

src/net_processing.cpp

+    case ValidationInvalidReason::CONSENSUS:
+    case ValidationInvalidReason::BLOCK_MUTATED:
+    case ValidationInvalidReason::CACHED_INVALID:
+        if (via_compact_block) { return false; } else { return true; }


nit: return !via_compact_block

Empact · 2018-05-16T05:23:34Z

src/net_processing.cpp

+    case ValidationInvalidReason::TX_CONFLICT:
+    case ValidationInvalidReason::TX_MEMPOOL_POLICY:
+        break;
+    }


Maybe log a warning or throw on the default case, to make unhandled cases more visible?

…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

TheBlueMatt force-pushed the 2017-10-dos-rewrite branch from 96d067d to 760b6c7 Compare November 8, 2017 21:25

fanquake added the Refactoring label Nov 8, 2017

TheBlueMatt force-pushed the 2017-10-dos-rewrite branch from 993493d to 93cc7b7 Compare December 15, 2017 22:06

TheBlueMatt force-pushed the 2017-10-dos-rewrite branch from 93cc7b7 to 09dca76 Compare December 16, 2017 01:30

promag reviewed Dec 20, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-10-dos-rewrite branch 3 times, most recently from 9e14f5d to 2a117b2 Compare December 24, 2017 18:30

promag reviewed Jan 9, 2018

View reviewed changes

TheBlueMatt force-pushed the 2017-10-dos-rewrite branch 2 times, most recently from 74315a9 to f2c33c3 Compare January 9, 2018 22:48

sdaftuar mentioned this pull request Feb 5, 2018

Rationalize the CValidationState interface by splitting ::DoS across ::Invalid and ::Corrupt #12357

Closed

Empact mentioned this pull request Feb 17, 2018

Drop the return and corruptionPossible arguments from CValidationState::DoS, and rename to ::Reject #12463

Closed

sdaftuar reviewed Feb 27, 2018

View reviewed changes

sdaftuar suggested changes Feb 27, 2018

View reviewed changes

jnewbery mentioned this pull request Apr 16, 2018

Remove unused default args to Invalid and DoS #12991

Closed

TheBlueMatt force-pushed the 2017-10-dos-rewrite branch from f2c33c3 to 1406301 Compare April 16, 2018 17:10

TheBlueMatt force-pushed the 2017-10-dos-rewrite branch from 1406301 to f22fdb5 Compare April 16, 2018 17:37

maflcko reviewed Apr 16, 2018

View reviewed changes

TheBlueMatt force-pushed the 2017-10-dos-rewrite branch from 45c3f67 to efd9c17 Compare April 27, 2018 18:47

maflcko reviewed Apr 27, 2018

View reviewed changes

TheBlueMatt force-pushed the 2017-10-dos-rewrite branch 2 times, most recently from 8f87a0a to f29c11a Compare April 28, 2018 01:26

TheBlueMatt force-pushed the 2017-10-dos-rewrite branch from f29c11a to 5da9182 Compare May 13, 2018 19:25

TheBlueMatt added 8 commits May 14, 2018 12:50

Add useful-for-dos "reason" field to CValidationState

711f775

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.

Use new reason-based DoS/disconnect logic instead of state.nDoS

16ec67e

Use state reason field to check for collisions in cmpctblocks

d22c9db

Prep for scripted-diff by removing some \ns which annoy sed.

905bca5

Remove references to CValidationState's DoS and CorruptionPossible

8c35ac4

Update some comments in validation.cpp as we arent doing DoS there

e73cf51

TheBlueMatt force-pushed the 2017-10-dos-rewrite branch from 5da9182 to e73cf51 Compare May 14, 2018 16:56

sdaftuar suggested changes May 15, 2018

View reviewed changes

Empact reviewed May 16, 2018

View reviewed changes

maflcko added the Needs rebase label Jun 6, 2018

TheBlueMatt closed this Jun 14, 2018

sdaftuar mentioned this pull request Jan 10, 2019

Rewrite DoS interface between validation and net_processing #15141

Merged

laanwj removed the Needs rebase label Oct 24, 2019

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Rewrite the interface between validation and net_processing wrt DoS #11639

Rewrite the interface between validation and net_processing wrt DoS #11639

Uh oh!

Conversation

TheBlueMatt commented Nov 8, 2017

Uh oh!

TheBlueMatt commented Dec 15, 2017

Uh oh!

promag 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!

sdaftuar 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!

TheBlueMatt commented Mar 2, 2018

Uh oh!

sdaftuar commented Mar 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Apr 5, 2018

Uh oh!

sdaftuar commented Apr 5, 2018

Uh oh!

sipa commented Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

sdaftuar commented Mar 4, 2018 •

edited

Loading

sipa commented Apr 5, 2018 •

edited

Loading