Rewrite the interface between validation and net

TheBlueMatt commented at 9:17 pm on November 8, 2017: member

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 force-pushed on Nov 8, 2017

fanquake added the label Refactoring on Nov 8, 2017

laanwj added this to the "Blockers" column in a project

TheBlueMatt force-pushed on Dec 15, 2017

TheBlueMatt commented at 10:10 pm on December 15, 2017: member

Rebased.

TheBlueMatt force-pushed on Dec 16, 2017

in src/test/txvalidation_tests.cpp:57 in 09dca76d67 outdated

52@@ -53,9 +53,8 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_reject_coinbase, TestChain100Setup)
53     BOOST_CHECK(state.IsInvalid());
54     BOOST_CHECK_EQUAL(state.GetRejectReason(), "coinbase");
55 
56-    int nDoS;
57-    BOOST_CHECK_EQUAL(state.IsInvalid(nDoS), true);
58-    BOOST_CHECK_EQUAL(nDoS, 100);
59+    BOOST_CHECK_EQUAL(state.IsInvalid(), true);
60+    BOOST_CHECK(state.GetReason() == ValidationInvalidReason::CONSENSUS);

promag commented at 2:15 pm on December 20, 2017:

Also BOOST_CHECK_EQUAL?

TheBlueMatt commented at 4:27 pm on December 24, 2017:

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

Empact commented at 0:25 am on May 1, 2018:

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

in src/validation.cpp:1912 in 09dca76d67 outdated

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

promag commented at 2:25 pm on December 20, 2017:

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:

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

The same happens in line 1951.

TheBlueMatt commented at 4:45 pm on December 24, 2017:

Done.

promag commented at 11:27 am on January 9, 2018:

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

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

TheBlueMatt commented at 10:04 pm on January 9, 2018:

Errrr….fixed, actually this time.

promag commented at 2:33 pm on December 20, 2017: member

Concept ACK.

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

TheBlueMatt force-pushed on Dec 24, 2017

in src/consensus/validation.h:33 in 2a117b2cc3 outdated

28+  * These are much more granular than the rejection codes, which may be more
29+  * useful for some other use-cases.
30+  */
31+enum class ValidationInvalidReason {
32+    // txn and blocks:
33+    VALID,           //!< not actually invalid

promag commented at 11:38 am on January 9, 2018:

ValidationInvalidReason::NONE instead?

TheBlueMatt commented at 10:04 pm on January 9, 2018:

Sure, sounds good.

TheBlueMatt force-pushed on Jan 9, 2018

in src/validation.cpp:1966 in 0272d2e808 outdated

1913@@ -1906,11 +1914,17 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
1914         {
1915             CAmount txfee = 0;
1916             if (!Consensus::CheckTxInputs(tx, state, view, pindex->nHeight, txfee)) {
1917+                if (state.GetReason() == ValidationInvalidReason::MISSING_INPUTS) {
1918+                    // CheckTxInputs may return MISSING_INPUTS but we can't return that, as
1919+                    // its nonsense for a block, so we reset the reason flag to CONSENSUS here.
1920+                    state.DoS(state.GetDoS(), ValidationInvalidReason::CONSENSUS, false,
1921+                              state.GetRejectCode(), state.GetRejectReason(), state.GetDebugMessage());

sdaftuar commented at 6:30 pm on February 27, 2018:

The arguments don’t match here:

0validation.cpp:1921:79: error: no viable conversion from 'std::string' (aka 'basic_string<char, char_traits<char>, allocator<char> >') to 'bool'
1                              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).

sipa commented at 9:04 pm on April 20, 2018:

This seems fixed now, @sdaftuar?

in src/net_processing.cpp:797 in 4781a3d3ce outdated

773+ */
774+static bool MayResultInDisconnect(const CValidationState& state, bool via_compact_block) {
775+    switch (state.GetReason()) {
776+    case ValidationInvalidReason::NONE:
777+        return false;
778+    // The node is mean:

sdaftuar commented at 6:34 pm on February 27, 2018:

I don’t quite understand this comment?

jnewbery commented at 8:01 pm on April 16, 2018:

I also don’t understand this comment!

in src/net_processing.cpp:801 in 4781a3d3ce outdated

777+        return false;
778+    // The node is mean:
779+    case ValidationInvalidReason::CONSENSUS:
780+    case ValidationInvalidReason::MUTATED:
781+    case ValidationInvalidReason::UNKNOWN_INVALID: // Really "duplicate of invalid "
782+        if (via_compact_block) return false;

sdaftuar commented at 6:36 pm on February 27, 2018:

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.

jnewbery commented at 8:03 pm on April 16, 2018:

Agree. an else return true would be clearer here.

jnewbery commented at 8:13 pm on April 16, 2018:

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.

jnewbery commented at 7:22 pm on April 17, 2018:

Not addressed.

Fall through case statements are confusing.

in src/validation.cpp:1877 in 0272d2e808 outdated

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

sdaftuar commented at 7:09 pm on February 27, 2018:

I think this should probably be CONSENSUS rather than SOFT_FORK.

TheBlueMatt commented at 3:29 pm on March 2, 2018:

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.

in src/validation.cpp:3167 in 0272d2e808 outdated

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

sdaftuar commented at 7:16 pm on February 27, 2018:

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.

in src/validation.cpp:3227 in 0272d2e808 outdated

3221@@ -3194,7 +3222,9 @@ static bool ContextualCheckBlock(const CBlock& block, CValidationState& state, c
3222     // the block hash, so we couldn't mark the block as permanently
3223     // failed).
3224     if (GetBlockWeight(block) > MAX_BLOCK_WEIGHT) {
3225-        return state.DoS(100, false, REJECT_INVALID, "bad-blk-weight", false, strprintf("%s : weight limit failed", __func__));
3226+        // We can call this a consensus failure as any data-providers who provided
3227+        // us with witness data can be expected to support SegWit validation.
3228+        return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-blk-weight", false, strprintf("%s : weight limit failed", __func__));

sdaftuar commented at 7:19 pm on February 27, 2018:

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.

TheBlueMatt commented at 3:33 pm on March 2, 2018:

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.

in src/validation.cpp:1964 in 0272d2e808 outdated

1961+            if (!CheckInputs(tx, state, view, fScriptChecks, flags, fCacheResults, fCacheResults, txdata[i], nScriptCheckThreads ? &vChecks : nullptr)) {
1962+                if (state.GetReason() == ValidationInvalidReason::NOT_STANDARD) {
1963+                    // CheckInputs may return NOT_STANDARD for extra flags we passed,
1964+                    // but we can't return that, as its nonsense, so we reset the
1965+                    // reason flag to SOFT_FORK here.
1966+                    state.DoS(state.GetDoS(), ValidationInvalidReason::SOFT_FORK, false,

sdaftuar commented at 7:39 pm on February 27, 2018:

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.

TheBlueMatt commented at 3:34 pm on March 2, 2018:

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.

sdaftuar commented at 2:23 pm on March 4, 2018:

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.

in src/validation.cpp:3192 in f2c33c3f19 outdated

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

sdaftuar commented at 7:51 pm on February 27, 2018:

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?

TheBlueMatt commented at 3:30 pm on March 2, 2018:

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

sdaftuar changes_requested

sdaftuar commented at 8:01 pm on February 27, 2018: member

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

TheBlueMatt commented at 3:35 pm on March 2, 2018: member

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 commented at 2:38 pm on March 4, 2018: member

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.

laanwj removed this from the "Blockers" column in a project

TheBlueMatt commented at 3:52 pm on April 5, 2018: member

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 commented at 5:01 pm on April 5, 2018: member

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 commented at 6:15 pm on April 5, 2018: member

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 commented at 6:47 pm on April 5, 2018: member

@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 commented at 8:15 pm on April 5, 2018: member

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 force-pushed on Apr 16, 2018

TheBlueMatt commented at 5:11 pm on April 16, 2018: member

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 commented at 5:12 pm on April 16, 2018: contributor

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 commented at 5:32 pm on April 16, 2018: member

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).

TheBlueMatt force-pushed on Apr 16, 2018

in src/validation.cpp:3099 in f22fdb51af outdated

3101-        if (!CheckTransaction(*tx, state, false))
3102-            return state.Invalid(false, state.GetRejectCode(), state.GetRejectReason(),
3103-                                 strprintf("Transaction check failed (tx hash %s) %s", tx->GetHash().ToString(), state.GetDebugMessage()));
3104+    for (const auto& tx : block.vtx) {
3105+        if (!CheckTransaction(*tx, state, false)) {
3106+            LogPrintf("Transaction check failed (tx hash %s) %s", tx->GetHash().ToString(), state.GetDebugMessage());

MarcoFalke commented at 6:01 pm on April 16, 2018:

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

TheBlueMatt force-pushed on Apr 16, 2018

in src/consensus/validation.h:25 in 01640d11ae outdated

21@@ -22,6 +22,39 @@ static const unsigned char REJECT_NONSTANDARD = 0x40;
22 static const unsigned char REJECT_INSUFFICIENTFEE = 0x42;
23 static const unsigned char REJECT_CHECKPOINT = 0x43;
24 
25+/** A "reason" why something was invalid, suitable for (possibly) getting angry

jnewbery commented at 7:21 pm on April 16, 2018:

I’m not a big fan of these colorful comments - mainly because without context, it’s difficult for new contributors to understand the spirit in which they were written.

Can we keep them a bit more precise, eg A reason why something was invalid. This is used to determine what action to take towards the provider of the object (eg disconnect, ban, etc).

TheBlueMatt commented at 3:08 pm on April 17, 2018:

Gotta ruin all my fun…fixed.

in src/validation.cpp:569 in 01640d11ae outdated

566     // Reject transactions with witness before segregated witness activates (override with -prematurewitness)
567     bool witnessEnabled = IsWitnessEnabled(chainActive.Tip(), chainparams.GetConsensus());
568     if (!gArgs.GetBoolArg("-prematurewitness", false) && tx.HasWitness() && !witnessEnabled) {
569-        return state.DoS(0, false, REJECT_NONSTANDARD, "no-witness-yet", true);
570+        // We call this WITNESS_MUTATED because we don't bother to check if the witness was maybe garbage
571+        // and its important clients realize that they shouldn't assume the witness indicated the

jnewbery commented at 7:27 pm on April 16, 2018:

I find this comment really difficult to parse. What are the witness’s important clients? Also, please remove bother to.

TheBlueMatt commented at 3:08 pm on April 17, 2018:

Removed the comment and replaced it with a more detailed comment on the WITNESS_MUTATED definition.

in src/validation.cpp:708 in 01640d11ae outdated

712@@ -710,21 +713,21 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
713         // MAX_BLOCK_SIGOPS; we still consider this an invalid rather than
714         // merely non-standard transaction.
715         if (nSigOpsCost > MAX_STANDARD_TX_SIGOPS_COST)
716-            return state.DoS(0, false, REJECT_NONSTANDARD, "bad-txns-too-many-sigops", false,
717+            return state.Invalid(ValidationInvalidReason::NOT_STANDARD, false, REJECT_NONSTANDARD, "bad-txns-too-many-sigops",

jnewbery commented at 7:32 pm on April 16, 2018:

This seems to contradict the comment above we still consider this an invalid rather than merely non-standard transaction. Perhaps update the comment?

TheBlueMatt commented at 3:08 pm on April 17, 2018:

Removed the comment, looks like it was just wrong.

in src/validation.cpp:1408 in 01640d11ae outdated

1422@@ -1422,7 +1423,7 @@ bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsVi
1423                     // as to the correct behavior - we may want to continue
1424                     // peering with non-upgraded nodes even after soft-fork
1425                     // super-majority signaling has occurred.
1426-                    return state.DoS(100,false, REJECT_INVALID, strprintf("mandatory-script-verify-flag-failed (%s)", ScriptErrorString(check.GetScriptError())));
1427+                    return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, strprintf("mandatory-script-verify-flag-failed (%s)", ScriptErrorString(check.GetScriptError())));

jnewbery commented at 7:37 pm on April 16, 2018:

Perhaps update comment above, which talks about DoS banning nodes.

TheBlueMatt commented at 3:08 pm on April 17, 2018:

Removed the comment. It isnt adding anything that the RECENT_CONSENSUS_CHANGE documentation didnt already cover.

in src/validation.cpp:1977 in 01640d11ae outdated

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

jnewbery commented at 7:49 pm on April 16, 2018:

This comment would be clearer if it explained why the reason needs to be changed, rather than just asserting that “its nonsense”. Perhaps something like:

0`MISSING_INPUTS` is a legitimate `InvalidReason` for a transaction, but not for a block. If one of the block's transactions is missing inputs, then the block is consensus invalid. Update the `InvalidReason`.

Same for the other ’nonsense’ comment below.

TheBlueMatt commented at 3:08 pm on April 17, 2018:

Fixed.

in src/validation.cpp:2047 in 01640d11ae outdated

2043+        return state.Invalid(ValidationInvalidReason::CONSENSUS,
2044                          error("ConnectBlock(): coinbase pays too much (actual=%d vs limit=%d)",
2045                                block.vtx[0]->GetValueOut(), blockReward),
2046                                REJECT_INVALID, "bad-cb-amount");
2047 
2048+    // TODO: We should be able to ban peers that send us completely bogus scripts,

jnewbery commented at 7:50 pm on April 16, 2018:

I don’t understand this TODO. We are returning CONSENSUS here, not SOFT_FORK.

TheBlueMatt commented at 3:08 pm on April 17, 2018:

Ah missed removing that when I changed things for @sdaftuar’s suggestion. Removed.

in src/consensus/validation.h:44 in 01640d11ae outdated

40+     * is uninteresting.
41+     */
42+    RECENT_CONSENSUS_CHANGE,
43+    UNKNOWN_INVALID,         //!< this object was cached as being invalid, but we don't know why
44+    // Only blocks:
45+    MUTATED,                 //!< the block's data didn't match the data committed to by the PoW

jnewbery commented at 7:52 pm on April 16, 2018:

It may be clearer to prefix the block-only reasons with BLOCK_ and the transaction-only reasons with TX_

Empact commented at 3:07 am on April 17, 2018:

Agreed

TheBlueMatt commented at 3:08 pm on April 17, 2018:

I’d rather just split it up into separate validation state objects in a later refactor, then its type-checked instead.

jnewbery commented at 7:27 pm on April 17, 2018:

Sounds good, but is there any downside to making it clearer until that future refactor?

in src/validation.cpp:3088 in 01640d11ae outdated

3101-        if (!CheckTransaction(*tx, state, false))
3102-            return state.Invalid(false, state.GetRejectCode(), state.GetRejectReason(),
3103-                                 strprintf("Transaction check failed (tx hash %s) %s", tx->GetHash().ToString(), state.GetDebugMessage()));
3104+    for (const auto& tx : block.vtx) {
3105+        if (!CheckTransaction(*tx, state, false)) {
3106+            LogPrintf("Transaction check failed (tx hash %s) %s\n", tx->GetHash().ToString(), state.GetDebugMessage());

jnewbery commented at 7:59 pm on April 16, 2018:

It’s not clear to me why you’ve removed the Invalid() call in the Add useful-for-dos “reason” field to CValidationState commit.

Empact commented at 3:51 am on April 17, 2018:

The Invalid call here just adds to the debug info - see that each arg is carried over from the existing state, and the debug strprintf prepends a message to the existing debug info. Logging is potentially an equivalently communicative alternative, though I would use FormatStateMessage here to include all the state info.

Empact commented at 5:52 am on May 16, 2018:

nit: I still think the full FormatStateMessage is the better info to record here.

in src/net_processing.cpp:800 in 01640d11ae outdated

795+    case ValidationInvalidReason::NONE:
796+        return false;
797+    // The node is mean:
798+    case ValidationInvalidReason::CONSENSUS:
799+    case ValidationInvalidReason::MUTATED:
800+    case ValidationInvalidReason::UNKNOWN_INVALID: // Really "duplicate of invalid "

jnewbery commented at 8:02 pm on April 16, 2018:

What does “duplicate of invalid " mean?

jnewbery commented at 8:40 pm on April 16, 2018:

Ah, I think I understand now. This comment should be clarified and moved to the enum definition. Perhaps even rename to DUPLICATE_INVALID?

TheBlueMatt commented at 3:08 pm on April 17, 2018:

The enum definition is pretty clear IMO “this object was cached as being invalid, but we don’t know why”. Any suggestions for a different comment?

in src/net_processing.cpp:806 in 01640d11ae outdated

801+        if (via_compact_block) return false;
802+    case ValidationInvalidReason::CHECKPOINT:
803+    case ValidationInvalidReason::INVALID_PREV:
804+    case ValidationInvalidReason::MISSING_PREV:
805+        return true;
806+    // Speed-of-light or out-of-sync:

jnewbery commented at 8:04 pm on April 16, 2018:

What does this mean?!

TheBlueMatt commented at 3:08 pm on April 17, 2018:

Clearer?

in src/net_processing.cpp:836 in 01640d11ae outdated

831+        {
832+            LOCK(cs_main);
833+            Misbehaving(nodeid, 100, message);
834+        }
835+        return true;
836+    // Speed-of-light or out-of-sync:

jnewbery commented at 8:15 pm on April 16, 2018:

Again, not sure what this means. In this function MISSING_PREV seems to count as Speed-of-light or out-of-sync, but not in MayResultInDisconnect()

in src/net_processing.cpp:821 in 01640d11ae outdated

816+    return false;
817+}
818+
819+//! Returns true if the peer was punished (probably disconnected)
820+static bool MaybePunishNode(NodeId nodeid, const CValidationState& state, bool via_compact_block, const std::string& message = "") {
821+    switch (state.GetReason()) {

jnewbery commented at 8:16 pm on April 16, 2018:

Again, I think this is clearer if you remove the switch statement.

in src/net_processing.cpp:1041 in 01640d11ae outdated

1032         if (it != mapBlockSource.end() && State(it->second.first) && state.GetRejectCode() > 0 && state.GetRejectCode() < REJECT_INTERNAL) {
1033             CBlockReject reject = {(unsigned char)state.GetRejectCode(), state.GetRejectReason().substr(0, MAX_REJECT_MESSAGE_LENGTH), hash};
1034             State(it->second.first)->rejects.push_back(reject);
1035-            if (nDoS > 0 && it->second.second)
1036-                Misbehaving(it->second.first, nDoS);
1037+            MaybePunishNode(it->second.first, state, !it->second.second);

jnewbery commented at 8:46 pm on April 16, 2018:

This !it->second.second is pretty obscure:

when adding an entry to mapBlockSource, we check whether it was received as a compact block, invert that to mean ‘should punish peer’
when reading from mapBlockSource, we take the .second, and invert it to mean via compact_block before calling MaybePunishNode().

Can we change mapBlockSource to just store whether the block was received via compact block to avoid this double inversion?

TheBlueMatt commented at 3:08 pm on April 17, 2018:

Happy to do it in a followup, this is already enough code and I don’t want to risk a bug if it sits for another 6 months and requires rebase.

jnewbery commented at 7:47 pm on April 17, 2018:

Yes - can be done in a follow up, but in the meantime:

MaybePunishNode(it->second.first, state, !it->second.second);

is almost completely inscrutable!

Empact commented at 10:25 pm on April 18, 2018:

In fairness, it was this way when he found it. :P Def worthy of cleanup.

Empact commented at 5:46 am on May 16, 2018:

FYI, a more self-documenting alternative looks something like:

 0if (state.IsInvalid()) {      
 1    // Don't send reject message with code 0 or an internal reject code.
 2    NodeId node_id;
 3    bool punish_if_block_invalid;
 4    std::tie(node_id, punish_if_block_invalid) = it->second;
 5    if (it != mapBlockSource.end() && State(node_id) && state.GetRejectCode() > 0 && state.GetRejectCode() < REJECT_INTERNAL) {
 6        CBlockReject reject = {(unsigned char)state.GetRejectCode(), state.GetRejectReason().substr(0, MAX_REJECT_MESSAGE_LENGTH), hash};
 7        State(node_id)->rejects.push_back(reject);
 8        MaybePunishNode(node_id, state, !punish_if_block_invalid);
 9    }
10}

Note you can early return for the == end() case here because it’s present in every case.

in src/net_processing.cpp:1470 in 01640d11ae outdated

1465@@ -1406,6 +1466,10 @@ bool static ProcessHeadersMessage(CNode *pfrom, CConnman *connman, const std::ve
1466                 // etc), and not just the duplicate-invalid case.
1467                 pfrom->fDisconnect = true;
1468             }
1469+            if (!MaybePunishNode(pfrom->GetId(), state, false, "invalid header received")) {
1470+                // MaybePunishNode returning true indicates Misbehaving logged the error for us

jnewbery commented at 8:53 pm on April 16, 2018:

This seems unnecessary. Why not just log inside MaybePunishNode() (it knows whether it called Misbehaving()) and remove the return value from MaybePunishNode(). Then the callers don’t need to worry about logging.

TheBlueMatt commented at 3:08 pm on April 17, 2018:

Done.

in src/net_processing.cpp:839 in 01640d11ae outdated

834+        }
835+        return true;
836+    // Speed-of-light or out-of-sync:
837+    case ValidationInvalidReason::MISSING_PREV:
838+        {
839+            // TODO: Handle this much more gracefully

jnewbery commented at 8:58 pm on April 16, 2018:

Can you comment why you think this is ungraceful?

TheBlueMatt commented at 3:08 pm on April 17, 2018:

Fixed.

in src/consensus/validation.h:81 in 01640d11ae outdated

101-            nDoSOut = nDoS;
102-            return true;
103-        }
104-        return false;
105-    }
106-    bool CorruptionPossible() const {

jnewbery commented at 9:18 pm on April 16, 2018:

Alternatively, you could just change this function to return whether reason is set to MUTATED or WITNESS_MUTATED

TheBlueMatt commented at 3:08 pm on April 17, 2018:

Then it’d be dead code and forcing the caller to be more clear about what exactly they’re checking for is much more stable.

jnewbery commented at 7:46 pm on April 17, 2018:

Understood. This is fine as is.

jnewbery commented at 9:19 pm on April 16, 2018: member

I’ve left a bunch of comments, mostly around making the commenting clearer.

in src/consensus/validation.h:30 in 332461018e outdated

26+  * (and retaliating) at the provider of the object in question (ie banning
27+  * peers).
28+  * These are much more granular than the rejection codes, which may be more
29+  * useful for some other use-cases.
30+  */
31+enum class ValidationInvalidReason {

Empact commented at 3:06 am on April 17, 2018:

Renaming this InvalidReason would save a lot of chars overall. The only other prefix in this module is REJECT_, so RejectReason is an option.

Empact commented at 4:40 am on April 17, 2018: member

Is it possible to also kill the REJECT_* codes here? They seem possibly duplicative with the new reason value.

TheBlueMatt force-pushed on Apr 17, 2018

in src/net_processing.cpp:800 in 8d483f3de6 outdated

795+    case ValidationInvalidReason::NONE:
796+        return false;
797+    // The node is is providing invalid data:
798+    case ValidationInvalidReason::CONSENSUS:
799+    case ValidationInvalidReason::MUTATED:
800+    case ValidationInvalidReason::UNKNOWN_INVALID: // Really "duplicate of invalid "

jnewbery commented at 7:21 pm on April 17, 2018:

I still think this comment belongs where the enum is defined. Something like:

`/!< this object was previously cached as being invalid, and we’ve now received a duplicate notification. We don’t store the reason that it was previously marked invalid.

TheBlueMatt commented at 7:24 pm on April 17, 2018:

“where the enum is defined” - I’m confused, at the enum definition it says “this object was cached as being invalid, but we don’t know why”, is that insufficient?

jnewbery commented at 7:36 pm on April 17, 2018:

Apparently it is insufficient, since you’ve felt the need to leave a comment here explaining what you really mean (and repeated it in the function below) :slightly_smiling_face:

Empact commented at 10:26 pm on April 18, 2018:

Agreed the comment should be carried over, or the value renamed.

TheBlueMatt commented at 5:54 pm on April 27, 2018:

Cleaned up.

TheBlueMatt force-pushed on Apr 17, 2018

in src/net_processing.cpp:2373 in 41ca0cf9b9 outdated

2369@@ -2304,8 +2370,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
2370                 // Never relay transactions that we would assign a non-zero DoS
2371                 // score for, as we expect peers to do the same with us in that
2372                 // case.
2373-                int nDoS = 0;
2374-                if (!state.IsInvalid(nDoS) || nDoS == 0) {
2375+                if (!state.IsInvalid() || !MayResultInDisconnect(state, false)) {

jnewbery commented at 7:38 pm on April 17, 2018:

At this point, it’s clearer to switch the ordering of the conditionals:

0                if (state.IsInvalid() && MayResultInDisconnect(state, false)) {
1                    LogPrintf("Not relaying invalid transaction %s from whitelisted peer=%d (%s)\n", tx.GetHash().ToString(), pfrom->GetId(), FormatStateMessage(state));
2                } else {
3                    LogPrintf("Force relaying tx %s from whitelisted peer=%d\n", tx.GetHash().ToString(), pfrom->GetId());
4                    RelayTransaction(tx, connman);
5                }

if (THING AND THING) is more natural than if (^THING OR ^THING)

TheBlueMatt commented at 5:54 pm on April 27, 2018:

Done.

jnewbery commented at 7:45 pm on April 17, 2018: member

Does it make sense to squash the commits Use state reason field to check for collisions in cmpctblocks and Remove references to CValidationState’s DoS and CorruptionPossible?

jnewbery commented at 7:49 pm on April 17, 2018: member

Much clearer to me now. Thanks for the updates!

A few more comments inline.

Empact commented at 10:29 pm on April 18, 2018: member

Renaming ValidationInvalidReason to InvalidReason would save 1,130 chars. Think of all those chars. ;) https://github.com/Empact/bitcoin/commit/beb63f3baadbe054f12ed8847fae4e38293cc46f

in src/validation.cpp:746 in 3de3878a45 outdated

742@@ -748,7 +743,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
743             const uint256 &hashAncestor = ancestorIt->GetTx().GetHash();
744             if (setConflicts.count(hashAncestor))
745             {
746-                return state.DoS(10, false,
747+                return state.DoS(10, ValidationInvalidReason::CONSENSUS, false,

sipa commented at 8:31 pm on April 20, 2018:

CONSENSUS seems strange here, as it’s not a script or amount violation.

CONFLICT or MISSING_INPUTS seems more appropriate.

TheBlueMatt commented at 3:05 pm on April 23, 2018:

MISSING_INPUTS and CONFLICT imply that its possible the transaction may be valid, but we’re missing something or otherwise unable to add it to the mempool right now. That is not the case here - there is no way this transaction could have been included in the mempool of any well-behaved node, nor could it ever make it into a block, so CONSENSUS is correct here.

sipa commented at 6:09 pm on April 23, 2018:

Ah, I see. The block “missing inputs” case is also reported as CONSENSUS; makes sense.

in src/validation.cpp:1976 in 3de3878a45 outdated

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

sipa commented at 8:32 pm on April 20, 2018:

Nit: it’s

TheBlueMatt commented at 5:54 pm on April 27, 2018:

Fixed

in src/consensus/validation.h:58 in 3de3878a45 outdated

53+     * Transaction might be missing a witness, have a witness prior to SegWit
54+     * activation, or witness may have been malleated (which includes
55+     * non-standard witnesses).
56+     */
57+    WITNESS_MUTATED,
58+    CONFLICT,                //!< tx already in mempool or conflicts with an existing one (which is in chain or in mempool and RBF failed)

sipa commented at 8:37 pm on April 20, 2018:

The distinction between CONFLICT and MEMPOOL_LIMIT seems very vague (at least when it’s about conflicts with the existing mempool), and perhaps hard to maintain if the mempool policy gets more complicated.

What would you think about using CONFLICT only for conflicts with confirmed transactions, and have a MEMPOOL_POLICY for all the rest?

TheBlueMatt commented at 3:17 pm on April 23, 2018:

Seems reasonable. Will do.

in src/validation.cpp:2019 in 3de3878a45 outdated

2016             bool fCacheResults = fJustCheck; /* Don't cache results if we're actually connecting blocks (still consult the cache, though) */
2017-            if (!CheckInputs(tx, state, view, fScriptChecks, flags, fCacheResults, fCacheResults, txdata[i], nScriptCheckThreads ? &vChecks : nullptr))
2018+            if (!CheckInputs(tx, state, view, fScriptChecks, flags, fCacheResults, fCacheResults, txdata[i], nScriptCheckThreads ? &vChecks : nullptr)) {
2019+                if (state.GetReason() == ValidationInvalidReason::NOT_STANDARD) {
2020+                    // CheckInputs may return NOT_STANDARD for extra flags we passed,
2021+                    // but we can't return that, as its not defined for a block, so

sipa commented at 8:38 pm on April 20, 2018:

Nit: it’s

TheBlueMatt commented at 5:54 pm on April 27, 2018:

Fixed

in src/validation.cpp:2006 in 3de3878a45 outdated

2015             std::vector<CScriptCheck> vChecks;
2016             bool fCacheResults = fJustCheck; /* Don't cache results if we're actually connecting blocks (still consult the cache, though) */
2017-            if (!CheckInputs(tx, state, view, fScriptChecks, flags, fCacheResults, fCacheResults, txdata[i], nScriptCheckThreads ? &vChecks : nullptr))
2018+            if (!CheckInputs(tx, state, view, fScriptChecks, flags, fCacheResults, fCacheResults, txdata[i], nScriptCheckThreads ? &vChecks : nullptr)) {
2019+                if (state.GetReason() == ValidationInvalidReason::NOT_STANDARD) {
2020+                    // CheckInputs may return NOT_STANDARD for extra flags we passed,

sipa commented at 8:40 pm on April 20, 2018:

How is this possible? CheckInputs for blocks should only run with consensus flags, in which case we shouldn’t ever return NOT_STANDARD, I think?

TheBlueMatt commented at 3:17 pm on April 23, 2018:

In an earlier version of the patch it may have returned NON_STANDARD for flags which were added in a soft fork, but at @sdaftuar’s suggestion that is no longer the case. Will fix the comment, but we’ll need this when we need RECENT_CONSENSUS_CHANGE at some point.

sdaftuar commented at 2:47 pm on May 15, 2018:

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.

MarcoFalke commented at 10:28 am on April 24, 2018: member

Needs rebase. (Presumably due to #13032)

TheBlueMatt force-pushed on Apr 27, 2018

TheBlueMatt commented at 5:54 pm on April 27, 2018: member

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.

TheBlueMatt force-pushed on Apr 27, 2018

in src/net_processing.cpp:2309 in efd9c1728d outdated

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

MarcoFalke commented at 8:19 pm on April 27, 2018:

Should this be orphan_state instead of state?

TheBlueMatt force-pushed on Apr 27, 2018

TheBlueMatt force-pushed on Apr 28, 2018

jnewbery commented at 2:20 pm on April 30, 2018: member

utACK f29c11a45edb138711ae7004b3b550ba416fc215

sipa commented at 0:04 am on May 1, 2018: member

utACK f29c11a45edb138711ae7004b3b550ba416fc215

MarcoFalke commented at 10:30 pm on May 8, 2018: member

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

sipa commented at 6:42 pm on May 13, 2018: member

Needs rebase due to #13185.

TheBlueMatt force-pushed on May 13, 2018

TheBlueMatt commented at 7:26 pm on May 13, 2018: member

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

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

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.

711f775986

Add functions to convert CValidationInterface's reason to DoS info

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.

eb597b6b92

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

Use state reason field to check for collisions in cmpctblocks d22c9dbb73

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

scripted-diff: Remove DoS calls to CValidationState

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

54d68ca936

Remove references to CValidationState's DoS and CorruptionPossible 8c35ac4d3d

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

in src/consensus/validation.h:64 in fa85da0e90 outdated

59+     * Tx already in mempool or conflicts with a tx in the chain
60+     * (if it conflicts with another tx in mempool, we use MEMPOOL_POLICY as it failed to reach the RBF threshold)
61+     * TODO: Currently this is only used if the transaction already exists in the mempool or on chain,
62+     * TODO: ATMP's fMissingInputs and a valid CValidationState being used to indicate missing inputs
63+     */
64+    TX_CONFLICT,

sdaftuar commented at 12:18 pm on May 14, 2018:

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?

in src/validation.cpp:663 in fa85da0e90 outdated

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

sdaftuar commented at 12:27 pm on May 14, 2018:

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?)

in src/validation.cpp:686 in fa85da0e90 outdated

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

sdaftuar commented at 12:30 pm on May 14, 2018:

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?

in src/validation.cpp:1962 in fa85da0e90 outdated

1970@@ -1971,11 +1971,17 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
1971         {
1972             CAmount txfee = 0;
1973             if (!Consensus::CheckTxInputs(tx, state, view, pindex->nHeight, txfee)) {
1974+                if (state.GetReason() == ValidationInvalidReason::TX_MISSING_INPUTS) {

sdaftuar commented at 12:43 pm on May 14, 2018:

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?

TheBlueMatt force-pushed on May 14, 2018

sipa commented at 5:16 pm on May 14, 2018: member

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

in src/consensus/validation.h:43 in 711f775986 outdated

38+     * so differentiating between always-invalid and invalid-by-pre-SegWit-soft-fork
39+     * is uninteresting.
40+     */
41+    RECENT_CONSENSUS_CHANGE,
42+    CACHED_INVALID,          //!< this object was cached as being invalid, but we don't know why
43+    // Only blocks:

sdaftuar commented at 2:57 pm on May 15, 2018:

nit: “(or headers)”?

in src/net_processing.cpp:797 in eb597b6b92 outdated

792+ */
793+static bool MayResultInDisconnect(const CValidationState& state, bool via_compact_block) {
794+    switch (state.GetReason()) {
795+    case ValidationInvalidReason::NONE:
796+        return false;
797+    // The node is is providing invalid data:

sdaftuar commented at 3:00 pm on May 15, 2018:

nit: “is is”

in src/net_processing.cpp:1479 in 16ec67eff7 outdated

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

sdaftuar commented at 5:31 pm on May 15, 2018:

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

in src/net_processing.cpp:2431 in 16ec67eff7 outdated

2434-                    Misbehaving(pfrom->GetId(), nDoS, strprintf("Peer %d sent us invalid header via cmpctblock\n", pfrom->GetId()));
2435-                } else {
2436-                    LogPrint(BCLog::NET, "Peer %d sent us invalid header via cmpctblock\n", pfrom->GetId());
2437-                }
2438+            if (state.IsInvalid()) {
2439+                MaybePunishNode(pfrom->GetId(), state, true, "invalid header via cmpctblock");

sdaftuar commented at 5:39 pm on May 15, 2018:

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?

sdaftuar changes_requested

sdaftuar commented at 5:45 pm on May 15, 2018: member

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

in src/net_processing.cpp:801 in e73cf5195d

796+        return false;
797+    // The node is is providing invalid data:
798+    case ValidationInvalidReason::CONSENSUS:
799+    case ValidationInvalidReason::BLOCK_MUTATED:
800+    case ValidationInvalidReason::CACHED_INVALID:
801+        if (via_compact_block) { return false; } else { return true; }

Empact commented at 5:22 am on May 16, 2018:

nit: return !via_compact_block

in src/net_processing.cpp:857 in e73cf5195d

852+    case ValidationInvalidReason::TX_MISSING_INPUTS:
853+    case ValidationInvalidReason::TX_WITNESS_MUTATED:
854+    case ValidationInvalidReason::TX_CONFLICT:
855+    case ValidationInvalidReason::TX_MEMPOOL_POLICY:
856+        break;
857+    }

Empact commented at 5:23 am on May 16, 2018:

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

MarcoFalke added the label Needs rebase on Jun 6, 2018

TheBlueMatt closed this on Jun 14, 2018

laanwj referenced this in commit d7d7d31506 on May 4, 2019

laanwj removed the label Needs rebase on Oct 24, 2019

MarcoFalke locked this on Dec 16, 2021

Rewrite the interface between validation and net_processing wrt DoS #11639