JeremyRubin commented at 0:51 am on October 19, 2017: contributor

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.

Refactor CValidationState API

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

863748bdd9

JeremyRubin force-pushed on Oct 19, 2017

in src/consensus/validation.h:36 in 863748bdd9

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

promag commented at 1:15 am on October 19, 2017:

enum class DoSSeverity

in src/consensus/tx_verify.cpp:163 in 863748bdd9

158@@ -159,47 +159,65 @@ int64_t GetTransactionSigOpCost(const CTransaction& tx, const CCoinsViewCache& i
159 bool CheckTransaction(const CTransaction& tx, CValidationState &state, bool fCheckDuplicateInputs)
160 {
161     // Basic checks that don't depend on any context
162-    if (tx.vin.empty())
163-        return state.DoS(10, false, REJECT_INVALID, "bad-txns-vin-empty");
164-    if (tx.vout.empty())
165-        return state.DoS(10, false, REJECT_INVALID, "bad-txns-vout-empty");
166+    if (tx.vin.empty()) {
167+        state.BadTx("bad-txns-vin-empty", "", DoS_SEVERITY::MEDIUM);

promag commented at 1:26 am on October 19, 2017:

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

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

Nit: also note the style change.

promag commented at 1:37 am on October 19, 2017: member

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

0CValidationState& CheckTransaction(...)

and add a CValidationState::operator bool().

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

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

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

fanquake added the label Refactoring on Oct 19, 2017

fanquake added the label Validation on Oct 19, 2017

JeremyRubin commented at 2:00 am on October 19, 2017: contributor

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

in src/consensus/validation.h:113 in 863748bdd9

119+    }
120+    void NonStandardTx(const std::string &_strRejectReason,
121+                 const std::string &_strDebugMessage="", CORRUPTION_POSSIBLE corrupted=CORRUPTION_POSSIBLE::False,
122+                 DoS_SEVERITY level=DoS_SEVERITY::NONE) {
123+        DoS(level, REJECT_NONSTANDARD, _strRejectReason, corrupted, _strDebugMessage);
124+    }

ajtowns commented at 4:19 am on October 19, 2017:

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.

in src/consensus/validation.h:117 in 863748bdd9

123+        DoS(level, REJECT_NONSTANDARD, _strRejectReason, corrupted, _strDebugMessage);
124+    }
125+    void DuplicateData(const std::string &_strRejectReason,
126+                 const std::string &_strDebugMessage="") {
127+        DoS(DoS_SEVERITY::NONE, REJECT_DUPLICATE, _strRejectReason, CORRUPTION_POSSIBLE::False, _strDebugMessage);
128+    }

ajtowns commented at 4:20 am on October 19, 2017:

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

in src/validation.cpp:682 in 863748bdd9

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

ajtowns commented at 5:00 am on October 19, 2017:

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

in src/validation.cpp:2857 in 863748bdd9

2856         // while still invalidating it.
2857-        if (mutated)
2858-            return state.DoS(100, false, REJECT_INVALID, "bad-txns-duplicate", true, "duplicate transaction");
2859+        if (mutated) {
2860+            state.CorruptTx("bad-txns-duplicate", "duplicate transaction");
2861+            return false;

ajtowns commented at 5:10 am on October 19, 2017:

Would have expected this to be CorruptBlock?

in src/validation.cpp:2888 in 863748bdd9

2892-        if (!CheckTransaction(*tx, state, false))
2893-            return state.Invalid(false, state.GetRejectCode(), state.GetRejectReason(),
2894-                                 strprintf("Transaction check failed (tx hash %s) %s", tx->GetHash().ToString(), state.GetDebugMessage()));
2895+        if (!CheckTransaction(*tx, state, false)) {
2896+            state.SetDebugMessage(strprintf("Transaction check failed (tx hash %s) %s", tx->GetHash().ToString(), state.GetDebugMessage()));
2897+            return false;

ajtowns commented at 5:15 am on October 19, 2017:

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

in src/validation.cpp:1799 in 863748bdd9

1793@@ -1766,8 +1794,9 @@ static bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockInd
1794             }
1795             nFees += txfee;
1796             if (!MoneyRange(nFees)) {
1797-                return state.DoS(100, error("%s: accumulated fee in the block out of range.", __func__),
1798-                                 REJECT_INVALID, "bad-txns-accumulated-fee-outofrange");
1799+                error("%s: accumulated fee in the block out of range.", __func__);
1800+                state.BadTx("bad-txns-accumulated-fee-outofrange");
1801+                return false;

ajtowns commented at 5:24 am on October 19, 2017:

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

in src/validation.cpp:655 in 863748bdd9

658-            return state.Invalid(false,
659-                REJECT_HIGHFEE, "absurdly-high-fee",
660-                strprintf("%d > %d", nFees, nAbsurdFee));
661+        if (nAbsurdFee && nFees > nAbsurdFee) {
662+            state.RejectFee( REJECT_HIGHFEE, "absurdly-high-fee", strprintf("%d > %d", nFees, nAbsurdFee));
663+            return false;

ajtowns commented at 5:32 am on October 19, 2017:

Nit: space after paren

ajtowns approved

ajtowns commented at 5:36 am on October 19, 2017: member

ACK 863748bdd99f37d655921ba220705cf0bec107fd Seems like a good improvement.

Sjors commented at 11:00 am on October 20, 2017: member

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

Empact commented at 7:43 am on February 7, 2018: member

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 commented at 8:50 pm on February 7, 2018: contributor

concept ACK

JeremyRubin commented at 1:47 am on February 8, 2018: contributor

@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 commented at 9:32 pm on February 8, 2018: member

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 commented at 1:38 am on February 9, 2018: contributor

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 commented at 12:56 pm on April 26, 2018: member

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.

fanquake closed this on Apr 26, 2018

JeremyRubin commented at 1:51 am on April 27, 2018: contributor

:+1:

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

laanwj referenced this in commit d7d7d31506 on May 4, 2019

MarcoFalke locked this on Sep 8, 2021

[Refactor] CValidation State #11523

Motivation