[Refactor] CValidation State #11523

JeremyRubin · 2017-10-19T00:51:32Z

This PR is a refactor only (i.e., any functional changes should be reported in review) which does the following:

Replace state.DoS with more descriptive calls where straightforward. Rather than just call DoS or Invalid, different causes of invalidity have different functions. This makes it easier to quickly find all related causes of that class of error.
Convert all nDoS usage to an enum class with named levels (none 0, low 1, medium 10, elevated 20, high 50, and critical 100)
Use a custom enum class for reporting corruption to make it more clear where corruption occurs
return false directly from CValidtationState update call sites. state.DoS never returns true, so it makes it easier to see that the return value is not dependent on the call.
Don't pass error() as an argument to a function in DoS. error always returns false, and this is confusing for readers/reviewers.

If anyone is interested, there's an unsquashed version too, but I figured this is simple enough to review squashed.

The only code quality 'decrease' is that some reject codes move from validation.h to consensus/validation.h. This abstraction barrier violation is already present (the CValidationState class is expected to handle those reject codes appropriately) so I think that this change is a lesser evil.

Motivation

I'm currently working on reworking the separation between reporting errors and DoS. As a first step in this process, I've cleaned up the interface without making any functional changes or major architecture changes. The resulting code should be easier to read and review changes to. The future plan to split CValidationState up into ~3 different subclasses (one to handle DoS, one to handle consensus correctness, and one to handle system errors) and then migrate the DoS completely to net_processing.cpp. This is a superior architecture because it better respects the boundaries between events on the network and faults in validation. I decided to start with this PR because I think it is an low hanging fruit immediate improvement independent of further modularization efforts.

Thanks to @ryanofsky and @TheBlueMatt for feedback on an earlier version of this PR.

Replace state.DoS with state.Invalid where possible Replace state.DoS with more descriptive calls where straightforward Replace all remaining uses of state.DoS and state.Invalid with more descriptive calls Convert all nDoS usage to an enum class with named levels Remove dead CValidationState::Invalid fn Clean up one use of NonStandardTx to use default args Use a custom enum class for reporting corruption to make it more clear where corruption occurs CValidationState: Remove rejectreason default argument Drop useless ret parameter from CValidationState::DoS return false directly from CValidtationState update call sites

promag

Another possibility is to return the validation state, for instance:

CValidationState& CheckTransaction(...)

and add a CValidationState::operator bool().

Even the methods like BadTx could return the CValidationState& so this could be possible:

return state.Critical().Reject("reason", code).Debug("message");
// or
return state.BadTx("bad-txns-vin-empty").Medium();

Either way, this patch could use the new code style.

promag · 2017-10-19T01:15:58Z

src/consensus/validation.h

+/** Too high fee. Can not be triggered by P2P transactions */
+static const unsigned int REJECT_HIGHFEE = 0x100;
+
+enum class DoS_SEVERITY : int {


enum class DoSSeverity

promag · 2017-10-19T01:26:32Z

src/consensus/tx_verify.cpp

-    if (tx.vout.empty())
-        return state.DoS(10, false, REJECT_INVALID, "bad-txns-vout-empty");
+    if (tx.vin.empty()) {
+        state.BadTx("bad-txns-vin-empty", "", DoS_SEVERITY::MEDIUM);


You could overload BadTx (and others too) to avoid the empty string:

void BadTx(const std::string& reject_reason, const std::string& debug_message="", DoSSeverity level=DoSSeverity::CRITICAL, unsigned int reject_code=REJECT_INVALID); void BadTx(const std::string& reject_reason, DoSSeverity level=DoSSeverity::CRITICAL, unsigned int reject_code=REJECT_INVALID);

Nit: also note the style change.

JeremyRubin · 2017-10-19T02:00:02Z

@promag Thanks for the review!

I like your suggestion returning the class and calling modifiers on it -- not a fan of the use of default arguments -- but I think that it adds a bit more complexity to this patch that I'd like to avoid because I think that it makes it more difficult for future PRs working to separate this interface further.

The reason why I didn't do an operator bool is that we never return anything but false, so I think that it is more straightforward to return the literal false from the callsite, even if it is a little bit more verbose.

ajtowns

ACK 863748b Seems like a good improvement.

ajtowns · 2017-10-19T04:19:23Z

src/consensus/validation.h

+                 const std::string &_strDebugMessage="", CORRUPTION_POSSIBLE corrupted=CORRUPTION_POSSIBLE::False,
+                 DoS_SEVERITY level=DoS_SEVERITY::NONE) {
+        DoS(level, REJECT_NONSTANDARD, _strRejectReason, corrupted, _strDebugMessage);
+    }


Each of CorruptBlockHeader, BadBlock, CorruptBlock, CorruptTx and NonStandardTx have all their callers leave level as the default, so it could be dropped as a parameter and hardcoded.

ajtowns · 2017-10-19T04:20:13Z

src/consensus/validation.h

+    void DuplicateData(const std::string &_strRejectReason,
+                 const std::string &_strDebugMessage="") {
+        DoS(DoS_SEVERITY::NONE, REJECT_DUPLICATE, _strRejectReason, CORRUPTION_POSSIBLE::False, _strDebugMessage);
+    }


None of the calls to DuplicateData make use of strDebugMessage, so it could also be dropped as a param and hardcoded.

ajtowns · 2017-10-19T05:00:33Z

src/validation.cpp

+                state.BadTx("bad-txns-spends-conflicting-tx", strprintf("%s spends conflicting transaction %s",
+                            hash.ToString(),
+                            hashAncestor.ToString()), DoS_SEVERITY::MEDIUM);
+                return false;


Nit: old indentation of strprintf() args was easiers to follow...

ajtowns · 2017-10-19T05:10:46Z

src/validation.cpp

-            return state.DoS(100, false, REJECT_INVALID, "bad-txns-duplicate", true, "duplicate transaction");
+        if (mutated) {
+            state.CorruptTx("bad-txns-duplicate", "duplicate transaction");
+            return false;


Would have expected this to be CorruptBlock?

ajtowns · 2017-10-19T05:15:03Z

src/validation.cpp

-                                 strprintf("Transaction check failed (tx hash %s) %s", tx->GetHash().ToString(), state.GetDebugMessage()));
+        if (!CheckTransaction(*tx, state, false)) {
+            state.SetDebugMessage(strprintf("Transaction check failed (tx hash %s) %s", tx->GetHash().ToString(), state.GetDebugMessage()));
+            return false;


Could use a comment noting "state filled in by CheckTransaction" as per AcceptToMemoryPoolWorker

ajtowns · 2017-10-19T05:24:36Z

src/validation.cpp

-                                 REJECT_INVALID, "bad-txns-accumulated-fee-outofrange");
+                error("%s: accumulated fee in the block out of range.", __func__);
+                state.BadTx("bad-txns-accumulated-fee-outofrange");
+                return false;


Shouldn't this be BadBlock? I don't see how'd you reach it without hitting a BadTx first though...

ajtowns · 2017-10-19T05:32:22Z

src/validation.cpp

-                strprintf("%d > %d", nFees, nAbsurdFee));
+        if (nAbsurdFee && nFees > nAbsurdFee) {
+            state.RejectFee( REJECT_HIGHFEE, "absurdly-high-fee", strprintf("%d > %d", nFees, nAbsurdFee));
+            return false;


Nit: space after paren

Sjors · 2017-10-20T11:00:01Z

Thanks, state.BadTx instead of state.DoS and DoS_SEVERITY::MEDIUM instead of 10 made things more readable to me.

Empact · 2018-02-07T07:43:17Z

My 2 cents, just ideas and opinions:

Returning false everywhere is duplicative. Imo returning from DoS as before is better as those extra lines have cost.
The CorruptionPossible enum/bool seems like a smell to me - if an enum is playing the role of a bool, why not a bool? If a bool isn’t clear, why not another construct?
I like the severity level enum
I don’t like that the severity param is buried as the last argument, IME severity make sense as a leading param or even the method name. Compare with log level apis.
I doubt making methods for each case is the way to go. There are a few independent variables here and ones with fewer, more stable values tend to make for better / more stable codebases. Consider severity or corruption / not corruption as alternatives.

dcousens · 2018-02-07T20:50:48Z

concept ACK

JeremyRubin · 2018-02-08T01:47:36Z

@Empact, thanks for the review.

The motivation for the code changes I made is that the DoS code should eventually be completely relegated to a net_processing construct, whereas the validity code should be handled in the validation. So the goal is to make the code in validation.h as abstract as possible and descriptive of what went wrong, providing a reason, rather than ascribing a Denial of Service level.

The extra lines for 'return false' don't have cost other than LoC, and the impact there is minimal. What they do help with is stricter modularization boundaries (should failure-reason code be returning information on validity?). Also returning a value from a function that always returns false is kind of stupid and makes the code harder to read.

I'm indifferent on severity levels ordering.

Empact · 2018-02-08T21:32:16Z

Fair enough. For severity levels, if you're indifferent I'd default to maintaining consistency with the prior behavior, which is using them as the leading param.

JeremyRubin · 2018-02-09T01:38:06Z

Hmm... now that I've thought about it a bit again, I think there is value to having it be last... because with the current API we're focusing on describing what happened, not how we should treat it for DoS. Eventually, the severity param could be completely dropped, because in validity there is only a notion of valid or invalid, and not 'how invalid' something was.

fanquake · 2018-04-26T12:56:07Z

Given that this conflicts quite heavily with #11639 (which is currently getting lots of review) and needs rebasing, I'm going to close for now. It can be re-opened/reviewed once (presumably) #11639 has been merged.

JeremyRubin · 2018-04-27T01:51:48Z

👍

I chatted with Matt out of band a bit ago and agree with waiting on #11639.

…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

JeremyRubin force-pushed the dos-cleanup branch from 6cf5d00 to 863748b Compare October 19, 2017 01:14

promag reviewed Oct 19, 2017

View reviewed changes

fanquake added Refactoring Validation labels Oct 19, 2017

ajtowns approved these changes Oct 19, 2017

View reviewed changes

Sjors mentioned this pull request Oct 20, 2017

Check specific validation error in miner tests #11220

Merged

TheBlueMatt mentioned this pull request Nov 8, 2017

Rewrite the interface between validation and net_processing wrt DoS #11639

Closed

JeremyRubin mentioned this pull request Feb 7, 2018

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

Closed

Empact mentioned this pull request Feb 17, 2018

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

Closed

fanquake closed this Apr 26, 2018

sdaftuar mentioned this pull request Jan 10, 2019

Rewrite DoS interface between validation and net_processing #15141

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

[Refactor] CValidation State #11523

[Refactor] CValidation State #11523

Uh oh!

Conversation

JeremyRubin commented Oct 19, 2017

Motivation

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeremyRubin commented Oct 19, 2017

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors commented Oct 20, 2017

Uh oh!

Empact commented Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dcousens commented Feb 7, 2018

Uh oh!

JeremyRubin commented Feb 8, 2018

Uh oh!

Empact commented Feb 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JeremyRubin commented Feb 9, 2018

Uh oh!

fanquake commented Apr 26, 2018

Uh oh!

JeremyRubin commented Apr 27, 2018

Uh oh!

Uh oh!

Empact commented Feb 7, 2018 •

edited

Loading

Empact commented Feb 8, 2018 •

edited

Loading