Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite DoS interface between validation and net_processing #15141

Merged
merged 20 commits into from May 4, 2019

Conversation

@sdaftuar
Copy link
Member

commented Jan 10, 2019

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.

@laanwj laanwj added the Validation label Jan 10, 2019

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

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

@practicalswift

This comment has been minimized.

Copy link
Member

commented Jan 10, 2019

Concept ACK

@laanwj laanwj added this to Blockers in High-priority for review Jan 10, 2019

@sipa

This comment has been minimized.

Copy link
Member

commented Jan 11, 2019

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

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

Concept ACK

@jnewbery
Copy link
Member

left a comment

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

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jan 14, 2019

Member

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

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

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Jan 15, 2019

Author Member

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

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

Anyway I'll update this comment to be clearer.

This comment has been minimized.

Copy link
@ajtowns

ajtowns Jan 15, 2019

Contributor

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

Show resolved Hide resolved src/net_processing.cpp Outdated
Show resolved Hide resolved src/net_processing.cpp Outdated
@ajtowns
Copy link
Contributor

left a comment

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

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

This comment has been minimized.

Copy link
@ajtowns

ajtowns Jan 15, 2019

Contributor

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

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

This comment has been minimized.

Copy link
@ajtowns

ajtowns Jan 15, 2019

Contributor

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

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

This comment has been minimized.

Copy link
@ajtowns

ajtowns Jan 15, 2019

Contributor

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

@ryanofsky
Copy link
Contributor

left a comment

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

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

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

  • a5415e8 Add useful-for-dos "reason" field to CValidationState (1/8)
  • 33213ad Add functions to convert CValidationInterface's reason to DoS info (2/8)
  • 6bdc449 Use new reason-based DoS/disconnect logic instead of state.nDoS (3/8)
  • 963699d Use state reason field to check for collisions in cmpctblocks (4/8)
  • 06e4247 Prep for scripted-diff by removing some \ns which annoy sed. (5/8)
  • 8131896 scripted-diff: Remove DoS calls to CValidationState (6/8)
  • 6ee2c45 Remove references to CValidationState's DoS and CorruptionPossible (7/8)
  • 94874dd Update some comments in validation.cpp as we arent doing DoS there (8/8)
@@ -160,24 +160,24 @@ bool CheckTransaction(const CTransaction& tx, CValidationState &state, bool fChe
{
// Basic checks that don't depend on any context
if (tx.vin.empty())
return state.DoS(10, false, REJECT_INVALID, "bad-txns-vin-empty");
return state.DoS(10, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-vin-empty");

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 15, 2019

Contributor

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

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

git log -p -n1 -U0 --word-diff-regex=. a5415e85caaf2f5a77d6bae9574bb6d21139ee34
@@ -716,27 +716,22 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
fSpendsCoinbase, nSigOpsCost, lp);
unsigned int nSize = entry.GetTxSize();

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 15, 2019

Contributor

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

Why remove this comment?

This comment has been minimized.

Copy link
@kallewoof

kallewoof Mar 5, 2019

Member

Good question.

This comment has been minimized.

Copy link
@Sjors

Sjors Mar 6, 2019

Member

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

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Mar 7, 2019

Author Member

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

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

This comment has been minimized.

Copy link
@ajtowns

ajtowns Apr 1, 2019

Contributor

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

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

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 15, 2019

Contributor

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

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

nDoS += level;

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

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 15, 2019

Contributor

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

Extra space on this line

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 15, 2019

Contributor

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

This also seems to double state.nDoS.

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 15, 2019

Contributor

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

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


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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 15, 2019

Contributor

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

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

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

New way seems better.

This comment has been minimized.

Copy link
@Sjors

Sjors Mar 6, 2019

Member

So if I understand this correctly:

  • state.Invalid() just calls state.DoS() with level=0andcorruptionIn=false` (default).
  • CheckTransaction() can currently fail in various ways, calling:
    • state.DoS with:
      • level 10 or 100: why isn't this higher level a problem?
      • corruptionIn not specified (so defaults to false)

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Mar 7, 2019

Author Member

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

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

This comment has been minimized.

Copy link
@ajtowns

ajtowns Apr 1, 2019

Contributor

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

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 15, 2019

Contributor

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

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

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

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

This comment has been minimized.

Copy link
@ajtowns

ajtowns Jan 18, 2019

Contributor

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

@ajtowns

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2019

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

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

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

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

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

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

which then allows dropping the instance variables:

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

Then the code is changed to use reasons directly:

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

And the now obsolete DoS/etc stuff is dropped:

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

That leaves a couple more things:

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

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

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

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

EDIT:

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

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

@sdaftuar

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2019

Thanks all for the review so far!

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

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2019

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

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

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

@naumenkogs

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2019

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

@jnewbery jnewbery removed this from Blockers in High-priority for review Jan 22, 2019

@sdaftuar sdaftuar force-pushed the sdaftuar:2019-01-rewrite-validation-interface branch from a642744 to 7e0090c Jan 24, 2019

@sdaftuar

This comment has been minimized.

Copy link
Member Author

commented Jan 24, 2019

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

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

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

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

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

@jnewbery jnewbery added this to Blockers in High-priority for review Jan 24, 2019

@ryanofsky
Copy link
Contributor

left a comment

Started review (will update list below with progress).

  • 59ff8e6 Remove redundant state.Invalid() call after CheckTransaction() (1/27)
  • e7edc15 drop obsolete comment (2/27)
  • f3fd64c [refactor] stateDummy -> orphan_state (3/27)
  • 4159f7c [refactor] Use maybepunish etc (4/27)
  • bfa94c7 Update comment to reference MaybePunishNode (5/27)
  • 70906c5 [refactor] drop IsInvalid(nDoSOut) (6/27)
  • 858bae6 set nDoS rather than bumping it (7/27)
  • 96cedc8 Clean up banning levels (8/27)
  • 6e27c50 === end of functionality changes (9/27)
  • ac3873e [refactor] Add useful-for-dos "reason" field to CValidationState (10/27)
  • bee1d4f TX_MISSING_INPUTS now has a DoS score of 0 (11/27)
  • 95d7de9 ==== start switch to reasons (12/27)
  • 6e3332a [refactor] Drop redundant nDoS, corruptionPossible, SetCorruptionPossible (13/27)
  • c558eba LookupBlockIndex -> CACHED_INVALID (14/27)
  • 8ed1801 CorruptionPossible -> TX_WITNESS_MUTATED (15/27)
  • 3048533 CorruptionPossible -> BLOCK_MUTATED (16/27)
  • 3466993 [refactor] Use Reasons directly instead of DoS codes (17/27)
  • 9dd6fc1 Fix handling of invalid headers (18/27)
  • 5e85f54 ==== drop nDoS info (19/27)
  • e5f43f3 Allow use of state.Invalid() for all reasons (20/27)
  • 5507fea [refactor] Prep for scripted-diff by removing some \ns which annoy sed. (21/27)
  • c664daf scripted-diff: Remove DoS calls to CValidationState (22/27)
  • 8e4590e [refactor] Drop unused state.DoS(), state.GetDoS(), state.CorruptionPossible() (23/27)
  • f494f78 ==== cleanup (24/27)
  • 9b7978e [refactor] Update some comments in validation.cpp as we arent doing DoS there (25/27)
  • 99d9689 [refactor] swap if/else order (26/27)
  • 7682566 nit: reason -> m_reason (27/27)
Show resolved Hide resolved src/validation.cpp Outdated
static bool MaybePunishNode(NodeId nodeid, const CValidationState& state, bool via_compact_block, const std::string& message = "") {
int nDoS = state.GetDoS();
if (nDoS > 0 && !via_compact_block) {
LOCK(cs_main);

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 24, 2019

Contributor

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

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

Show resolved Hide resolved src/validation.cpp

@sdaftuar sdaftuar force-pushed the sdaftuar:2019-01-rewrite-validation-interface branch from 7e0090c to 4e1ae68 Jan 29, 2019

@sdaftuar

This comment has been minimized.

Copy link
Member Author

commented Jan 29, 2019

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

@sdaftuar

This comment has been minimized.

Copy link
Member Author

commented Feb 8, 2019

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

@sipa

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

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

@sdaftuar sdaftuar force-pushed the sdaftuar:2019-01-rewrite-validation-interface branch from fd047f7 to 7682566 Feb 8, 2019

@sdaftuar

This comment has been minimized.

Copy link
Member Author

commented Feb 8, 2019

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

@DrahtBot DrahtBot removed the Needs rebase label Feb 8, 2019

@sipa

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

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

@laanwj

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

  • apart from f3883a3 whose rationale I disagree with (I don't think it's redundant, as commented), I think this is overall a move in the right direction
  • my last comment could be addressed (if at all) in a follow-up PR

utACK fdd7683

TheBlueMatt and others added some commits Apr 16, 2018

[refactor] Refactor misbehavior ban decisions to MaybePunishNode()
Isolate the decision of whether to ban a peer to one place in the
code, rather than having it sprinkled throughout net_processing.

Co-authored-by: Anthony Towns <aj@erisian.com.au>
                Suhas Daftuar <sdaftuar@gmail.com>
                John Newbery <john@johnnewbery.com>
[refactor] drop IsInvalid(nDoSOut)
Co-authored-by: Anthony Towns <aj@erisian.com.au>
Drop obsolete sigops comment
This comment was confusing and incorrect when first added ("invalid rather than
merely non-standard" has the opposite meaning of what is actually the case),
and was also not updated after segwit with the correct variable names.

Delete it since the code reads just fine on its own.

Co-authored by: Anthony Towns <aj@erisian.com.au>
                Suhas Daftuar <sdaftuar@gmail.com>
[refactor] rename stateDummy -> orphan_state
Co-authored-by: Anthony Towns <aj@erisian.com.au>
                Suhas Daftuar <sdaftuar@gmail.com>
Clean up banning levels
Compared with previous bans, the following changes are made:
 * Txn with empty vin/vout or null prevouts move from 10 DoS
   points to 100.
 * Loose transactions with a dependency loop now result in a ban
   instead of 10 DoS points.
 * Many pre-segwit soft-fork errors now result in a ban.
   Note: Transactions that violate soft-fork script flags since P2SH do not generally
   result in a ban. Also, banning behavior for invalid blocks is dependent on
   whether the node is validating with multiple script check threads, due to a long-
   standing bug. That inconsistency is still present after this commit.
 * Proof of work failure moves from 50 DoS points to a ban.
 * Blocks with timestamps under MTP now result in a ban, blocks
   too far in the future continue to *not* result in a ban.
 * Inclusion of non-final transactions in a block now results in a
   ban instead of 10 DoS points.

Co-authored-by: Anthony Towns <aj@erisian.com.au>
Ban all peers for all block script failures
This eliminates a discrepancy between block validation with multiple
script check threads, versus a single script check thread.
[refactor] 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.

Behavior change: peers will now be banned for providing blocks
with premature coinbase spends.

Co-authored-by: Anthony Towns <aj@erisian.com.au>
                Suhas Daftuar <sdaftuar@gmail.com>
[refactor] Drop redundant nDoS, corruptionPossible, SetCorruptionPoss…
…ible

Co-authored-by: Anthony Towns <aj@erisian.com.au>
LookupBlockIndex -> CACHED_INVALID
Co-authored-by: Anthony Towns <aj@erisian.com.au>
CorruptionPossible -> TX_WITNESS_MUTATED
Co-authored-by: Anthony Towns <aj@erisian.com.au>
CorruptionPossible -> BLOCK_MUTATED
Co-authored-by: Anthony Towns <aj@erisian.com.au>
Fix handling of invalid headers
We only disconnect outbound peers (excluding HB compact block peers and manual
connections) when receiving a CACHED_INVALID header.
Allow use of state.Invalid() for all reasons
Co-authored-by: Anthony Towns <aj@erisian.com.au>
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 src/consensus/tx_check.cpp
sed -i 's/state.GetRejectCode(), state.GetRejectReason(), [^,]\+, state.GetDebugMessage())/state.GetRejectCode(), state.GetRejectReason(), state.GetDebugMessage())/' src/validation.cpp
sed -i 's/\.DoS([^,]*, /.Invalid\(/' src/validation.cpp src/consensus/tx_verify.cpp src/consensus/tx_check.cpp
-END VERIFY SCRIPT-

Co-authored-by: Suhas Daftuar <sdaftuar@gmail.com>
[refactor] Drop unused state.DoS(), state.GetDoS(), state.CorruptionP…
…ossible()

Co-authored-by: Anthony Towns <aj@erisian.com.au>

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

@sdaftuar

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

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

Previous version of the code is at 15141.14.

@jnewbery

This comment has been minimized.

Copy link
Member

commented May 3, 2019

utACK 0ff1c2a

Only change is dropping the Drop obsolete sigops comment commit.

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

@ryanofsky
Copy link
Contributor

left a comment

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

@laanwj

This comment has been minimized.

Copy link
Member

commented May 4, 2019

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

@laanwj laanwj removed this from Blockers in High-priority for review May 4, 2019

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request May 4, 2019

Merge #15141: Rewrite DoS interface between validation and net_proces…
…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

domob1812 added a commit to domob1812/namecore that referenced this pull request May 6, 2019

Merge branch 'auxpow'
For bitcoin/bitcoin#15141, rewrite all the
state.Invalid calls made when validating name transactions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.