validation: Tidy up ValidationState interface

jnewbery commented at 4:52 pm on April 29, 2019: member

Carries out some remaining tidy-ups remaining after PR 15141:

split ValidationState into TxValidationState and BlockValidationState (commit from ajtowns)
various minor code style tidy-ups to the ValidationState class
remove the useless ret parameter from ValidationState::Invalid()
remove the now unused first_invalid parameter from ProcessNewBlockHeaders()
remove the fMissingInputs parameter from AcceptToMemoryPool(), and deal with missing inputs the same way as other errors by using the TxValidationState object.

Tip for reviewers (thanks ryanofsky!): The first commit ("[validation] Add CValidationState subclasses" ) is huge and can be easier to start reviewing if you revert the rote, mechanical changes:

Substitute the commit hash of commit “[validation] Add CValidationState subclasses” for in the commands below.

0git checkout <CommitHash>
1git grep -l ValidationState | xargs sed -i 's/BlockValidationState\|TxValidationState/CValidationState/g'
2git grep -l ValidationResult | xargs sed -i 's/BlockValidationResult\|TxValidationResult/ValidationInvalidReason/g'
3git grep -l MaybePunish | xargs sed -i 's/MaybePunishNode\(ForBlock\|ForTx\)/MaybePunishNode/g'
4git diff HEAD^

After that it’s possible to easily see the mechanical changes with:

0git log -p -n1 -U0 --word-diff-regex=. <CommitHash>

jnewbery commented at 4:53 pm on April 29, 2019: member

~This is built on #15141. Only commit [validation] Add CValidationState subclasses onwards are for review in this PR.~

#15141 is merged. This is ready for review.

jnewbery force-pushed on Apr 29, 2019

DrahtBot added the label Mempool on Apr 29, 2019

DrahtBot added the label Mining on Apr 29, 2019

DrahtBot added the label P2P on Apr 29, 2019

DrahtBot added the label RPC/REST/ZMQ on Apr 29, 2019

DrahtBot added the label Tests on Apr 29, 2019

DrahtBot added the label Utils/log/libs on Apr 29, 2019

DrahtBot added the label Validation on Apr 29, 2019

DrahtBot added the label Wallet on Apr 29, 2019

DrahtBot added the label Needs rebase on Apr 29, 2019

promag commented at 10:33 pm on April 29, 2019: member

Concept ACK, looks like a good refactor and improvement to add ValidationState subclasses.

fanquake removed the label Mempool on Apr 30, 2019

fanquake removed the label Mining on Apr 30, 2019

fanquake removed the label P2P on Apr 30, 2019

fanquake removed the label Tests on Apr 30, 2019

fanquake removed the label Utils/log/libs on Apr 30, 2019

fanquake added the label Refactoring on Apr 30, 2019

JeremyRubin commented at 5:58 am on April 30, 2019: contributor

Concept ACK.

I’d like to see Invalid and ilk return void instead of always false, and clean up the call sites correspondingly. This adds a lines here and there, but I think it improves the readability of the code to see a false literal returned explicitly.

I also somewhat think that ideally there should be a third state class which handles Corruption cases for ‘system state’ or something. This covers the notion that the issue is not the fault of the block or the transaction, it is an issue with our local state or implementation. Not sure it’s worth the churn though, two is already a huge improvement.

laanwj commented at 7:33 pm on April 30, 2019: member

Concept ACK

ajtowns commented at 0:02 am on May 3, 2019: member

I also somewhat think that ideally there should be a third state class which handles Corruption cases for ‘system state’ or something. This covers the notion that the issue is not the fault of the block or the transaction,

The current split is more about “what was being tested” not “what was at fault” – that’s easy to deal with via types, because you know what’s being tested at compile time; but you don’t know what’s going to have been at fault at compile time, so I think dealing with that via the type system (ie as a third state class) wouldn’t work.

jnewbery force-pushed on May 4, 2019

DrahtBot removed the label Needs rebase on May 4, 2019

jnewbery force-pushed on May 4, 2019

DrahtBot commented at 7:58 pm on May 4, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17268 (Epoch Mempool by JeremyRubin)
#17037 (Testschains: Many regtests with different genesis and default datadir by jtimon)
#16981 (Improve runtime performance of –reindex by LarryRuane)
#16974 (Walk pindexBestHeader back to ChainActive().Tip() if it is invalid by TheBlueMatt)
#16658 (validation: Rename CheckInputs to CheckInputScripts by jnewbery)
#16333 (test: Set BIP34Height = 2 for regtest by MarcoFalke)
#13533 ([tests] Reduced number of validations in tx_validationcache_tests by lucash-dev)
#10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

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.

in src/validation.h:230 in 4b4d081155 outdated

228@@ -228,7 +229,7 @@ bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<cons
229  * @param[out] ppindex If set, the pointer will be set to point to the last new block index object for the given headers
230  * @param[out] first_invalid First header that fails validation, if one exists

practicalswift commented at 4:01 pm on May 7, 2019:

first_invalid has been dropped, right? :-)

jnewbery commented at 9:07 pm on May 8, 2019:

Good catch. Fixed.

DrahtBot added the label Needs rebase on May 8, 2019

jnewbery force-pushed on May 8, 2019

DrahtBot removed the label Needs rebase on May 8, 2019

jnewbery commented at 9:07 pm on May 8, 2019: member

rebased: 8d88c8608

jnewbery force-pushed on May 8, 2019

jnewbery commented at 9:08 pm on May 8, 2019: member

Fixed @practicalswift’s comment: b4c8f0079059772f1bb662820c5cce8dab66237d

in src/consensus/validation.h:16 in b4c8f00790 outdated

11@@ -12,24 +12,17 @@
12 #include <primitives/transaction.h>
13 #include <primitives/block.h>
14 
15-/** "reject" message codes */
16-static const unsigned char REJECT_MALFORMED = 0x01;
17+/** A reject code must be provided for P2P reject messages. Just always send REJECT_INVALID
18+ *  TODO: remove support for BIP 61 reject messages.

flack commented at 12:46 pm on May 9, 2019:

nit: * is not aligned quite right

0  * TODO: remove support for BIP 61 reject messages.

jnewbery commented at 2:46 pm on May 9, 2019:

thanks. Fixed!

jnewbery force-pushed on May 9, 2019

DrahtBot added the label Needs rebase on May 14, 2019

jnewbery commented at 3:45 pm on May 16, 2019: member

rebased

jnewbery force-pushed on May 16, 2019

DrahtBot removed the label Needs rebase on May 16, 2019

DrahtBot added the label Needs rebase on May 17, 2019

jnewbery commented at 3:09 pm on May 20, 2019: member

rebased

jnewbery force-pushed on May 20, 2019

DrahtBot removed the label Needs rebase on May 20, 2019

DrahtBot added the label Needs rebase on May 23, 2019

jnewbery commented at 9:07 pm on May 23, 2019: member

rebased

jnewbery force-pushed on May 23, 2019

DrahtBot removed the label Needs rebase on May 23, 2019

in src/consensus/validation.h:149 in 1faf0c4b01 outdated

166+    void FromTxValidationState(TxValidationState tx_state) {
167+        switch (tx_state.GetResult()) {
168+        case TxValidationResult::NONE:
169+        case TxValidationResult::TX_NOT_STANDARD:
170+        case TxValidationResult::TX_MEMPOOL_POLICY:
171+            m_result = BlockValidationResult::NONE;

ryanofsky commented at 6:27 pm on May 29, 2019:

In commit “[validation] Add CValidationState subclasses” (1faf0c4b0120995b4361d38ca0cc0114e633955d)

It seems error prone that calls to FromTxValidationState and Invalid might actually change the state from invalid to valid. I wonder if both of these methods should start off asserting that m_result == NONE to reduce the risk that a previously invalid transaction or block is later treated as valid.

jnewbery commented at 3:20 pm on May 30, 2019:

For FromTxValidationState() should we assert that tx_state.GetResult() does not return TX_NOT_STANDARD or TX_MEMPOOL_POLICY? This should only be called from block validation code, where transactions shouldn’t fail for standardness or policy reasons.

I don’t think Invalid() can change state from invalid to valid: m_mode is either kept as MODE_ERROR or changed to MODE_INVALID. Can you point out where I’m wrong?

ryanofsky commented at 3:35 pm on May 30, 2019:

I’m just saying that this seems error prone, not that there currently is an error. Adding asserts to check the previous state and ensure that calling these methods never changes it from invalid to valid seems like an easy change that could prevent a serious bug. But I could be wrong about this. Maybe it wouldn’t be an easy change or wouldn’t be helpful in preventing bugs.

jnewbery commented at 5:19 pm on May 30, 2019:

In fact, I think I should just remove this method entirely. FromTxValidationState() is only called from three places and in each case we want the BlockValidationState.m_result to be set to CONSENSUS.

in src/validation.cpp:2153 in 1faf0c4b01 outdated

2027-                    // but we can't return that, as it's not defined for a block, so
2028-                    // we reset the reason flag to CONSENSUS here.
2029-                    // In the event of a future soft-fork, we may need to
2030-                    // consider whether rewriting to CONSENSUS or
2031-                    // RECENT_CONSENSUS_CHANGE would be more appropriate.
2032-                    state.Invalid(ValidationInvalidReason::CONSENSUS, false,

ryanofsky commented at 6:34 pm on May 29, 2019:

In commit “[validation] Add CValidationState subclasses” (1faf0c4b0120995b4361d38ca0cc0114e633955d)

I might be missing something but it seems like the previous code is rewriting TX_NOT_STANDARD to CONSENSUS and the new code in FromTxValidationState would rewrite it to NONE.

ajtowns commented at 6:54 am on May 30, 2019:

That looks wrong to me too. Might be clearer to tell CheckInputs() which validation rules are consensus vs standardness though?

jnewbery commented at 5:21 pm on May 30, 2019:

Yes, this is a bug (and a very bad one!) I’m going to remove the FromTxValidationState() method entirely and just set the BlockValidationState.m_result through a call to Invalid() (see #15921 (review)).

in src/validation.cpp:3273 in 1faf0c4b01 outdated

3272     // check for version 2, 3 and 4 upgrades
3273     if((block.nVersion < 2 && nHeight >= consensusParams.BIP34Height) ||
3274        (block.nVersion < 3 && nHeight >= consensusParams.BIP66Height) ||
3275        (block.nVersion < 4 && nHeight >= consensusParams.BIP65Height))
3276-            return state.Invalid(ValidationInvalidReason::BLOCK_INVALID_HEADER, false, REJECT_OBSOLETE, strprintf("bad-version(0x%08x)", block.nVersion),
3277+            return state.Invalid(BlockValidationResult::CONSENSUS, false, REJECT_OBSOLETE, strprintf("bad-version(0x%08x)", block.nVersion),

ryanofsky commented at 6:42 pm on May 29, 2019:

In commit “[validation] Add CValidationState subclasses” (1faf0c4b0120995b4361d38ca0cc0114e633955d)

Why the change from BLOCK_INVALID_HEADER to CONSENSUS here?

jnewbery commented at 9:45 pm on May 30, 2019:

No idea. Fixed!

in src/validation.cpp:1442 in 1faf0c4b01 outdated

1435@@ -1437,18 +1436,18 @@ bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsVi
1436                         CScriptCheck check2(coin.out, tx, i,
1437                                 flags & ~STANDARD_NOT_MANDATORY_VERIFY_FLAGS, cacheSigStore, &txdata);
1438                         if (check2())
1439-                            return state.Invalid(ValidationInvalidReason::TX_NOT_STANDARD, false, REJECT_NONSTANDARD, strprintf("non-mandatory-script-verify-flag (%s)", ScriptErrorString(check.GetScriptError())));
1440+                            return state.Invalid(TxValidationResult::TX_NOT_STANDARD, false, REJECT_NONSTANDARD, strprintf("non-mandatory-script-verify-flag (%s)", ScriptErrorString(check.GetScriptError())));
1441                     }
1442                     // MANDATORY flag failures correspond to
1443-                    // ValidationInvalidReason::CONSENSUS. Because CONSENSUS
1444+                    // TxValidationInvalidReason::CONSENSUS. Because CONSENSUS

ryanofsky commented at 6:51 pm on May 29, 2019:

In commit “[validation] Add CValidationState subclasses” (1faf0c4b0120995b4361d38ca0cc0114e633955d)

This should say TxValidationResult instead of TxValidationInvalidReason

jnewbery commented at 3:24 pm on May 30, 2019:

fixed

in src/bench/block_assemble.cpp:41 in 1faf0c4b01 outdated

38@@ -39,7 +39,7 @@ static void AssembleBlock(benchmark::State& state)
39         LOCK(::cs_main); // Required for ::AcceptToMemoryPool.
40 
41         for (const auto& txr : txs) {
42-            CValidationState state;
43+            TxValidationState state;

ryanofsky commented at 7:01 pm on May 29, 2019:

In commit “[validation] Add CValidationState subclasses” (1faf0c4b0120995b4361d38ca0cc0114e633955d)

Note to other reviewers: This first commit is huge and can be easier to start reviewing if you revert the rote, mechanical changes:

0git checkout 1faf0c4b0120995b4361d38ca0cc0114e633955d
1git grep -l ValidationState | xargs sed -i 's/BlockValidationState\|TxValidationState/CValidationState/g'
2git grep -l ValidationResult | xargs sed -i 's/BlockValidationResult\|TxValidationResult/ValidationInvalidReason/g'
3git grep -l MaybePunish | xargs sed -i 's/MaybePunishNode\(ForBlock\|ForTx\)/MaybePunishNode/g'
4git diff HEAD^

After that it’s possible to easily see the mechanical changes with:

0git log -p -n1 -U0 --word-diff-regex=. 1faf0c4b0120995b4361d38ca0cc0114e633955d

jnewbery commented at 3:29 pm on May 30, 2019:

Thanks Russ. I’ve added that review tip to the PR description.

ryanofsky commented at 7:03 pm on May 29, 2019: member

Started review (will update list below with progress).

1faf0c4b0120995b4361d38ca0cc0114e633955d [validation] Add CValidationState subclasses (1/7)
a3076817588a642b4de107dc96a76ea8939070df [validation] Remove Reject Code from ValidationState (2/7)
00e997c21af1e107c4370a6cae15b19a293cae63 [validation] Tidy Up ValidationResult class (3/7)
eba4651d76f8f636953cacfcb0979c10ec1025ea [validation] Remove error() calls from Invalid() calls (4/7)
578a4a166f575174cf38ae55131f96750c807bdf [validation] Remove useless ret parameter from Invalid() (5/7)
f21373b0211bd1f175171a2e7592690ff220d008 [validation] Remove unused first_invalid parameter from ProcessNewBlockHeaders() (6/7)
4cef7e521aa1ff7752653bc638e3bfc9a449af26 [validation] Remove fMissingInputs from AcceptToMemoryPool() (7/7)

Note that travis currently seems to be failing.

in src/consensus/validation.h:17 in a307681758 outdated

11@@ -12,20 +12,13 @@
12 #include <primitives/transaction.h>
13 #include <primitives/block.h>
14 
15-/** "reject" message codes */
16-static const unsigned char REJECT_MALFORMED = 0x01;
17+/** A reject code must be provided for P2P reject messages. Just always send REJECT_INVALID
18+  * TODO: remove support for BIP 61 reject messages.
19+  */
20 static const unsigned char REJECT_INVALID = 0x10;

ryanofsky commented at 2:56 pm on May 30, 2019:

In commit “[validation] Remove Reject Code from ValidationState” (a3076817588a642b4de107dc96a76ea8939070df)

It might clearer to return an entirely new code instead of REJECT_INVALID. Calling transactions invalid when they just have low fees or are nonstandard seems to stretch the definition of invalid.

jnewbery commented at 10:08 pm on May 30, 2019:

I’m not sure if that’s possible. The BIP61 reject codes are defined here: https://github.com/bitcoin/bips/blob/master/bip-0061.mediawiki#reject_version_codes.

I hope we’ll remove REJECT messages soon (https://github.com/bitcoin/bitcoin/pull/15437)

ryanofsky commented at 11:41 am on June 7, 2019:

re: #15921 (review)

In commit “[validation] Remove Reject Code from ValidationState” (22be379c54eb03e77803b0bbd8270082452d8fff)

It seems like this commit changing p2p codes would make more sense as a separate PR (maybe based on this PR) not mixed in with all the refactoring changes in this PR. Someone could be indifferent to the refactoring changes but want to raise an issue with the p2p change, or vice versa, so it’d be nice if any discussions would happen in separate places.

Also, I think the text in the commit message should be a release note.

jnewbery commented at 8:15 pm on July 18, 2019:

I wanted to get all of these changes into the same PR. All the commits touch the same lines, so splitting the changes across multiple PRs means more rebasing for other PRs. However, what you say makes sense - I shouldn’t mix P2P changes with a refactor.

I’ve removed this commit from the branch, so this PR should be basically a refactor with very minor logging/error message changes.

The removal of Reject Code from the ValidationState object can wait until we remove REJECT messages entirely (https://github.com/bitcoin/bitcoin/pull/15437).

in src/net_processing.cpp:1264 in a307681758 outdated

1247@@ -1249,9 +1248,11 @@ void PeerLogicValidation::BlockChecked(const CBlock& block, const BlockValidatio
1248     std::map<uint256, std::pair<NodeId, bool>>::iterator it = mapBlockSource.find(hash);
1249 
1250     if (state.IsInvalid()) {
1251-        // Don't send reject message with code 0 or an internal reject code.
1252-        if (it != mapBlockSource.end() && State(it->second.first) && state.GetRejectCode() > 0 && state.GetRejectCode() < REJECT_INTERNAL) {
1253-            CBlockReject reject = {(unsigned char)state.GetRejectCode(), state.GetRejectReason().substr(0, MAX_REJECT_MESSAGE_LENGTH), hash};
1254+        // Send reject messages for invalid blocks

ryanofsky commented at 3:03 pm on May 30, 2019:

In commit “[validation] Remove Reject Code from ValidationState” (a3076817588a642b4de107dc96a76ea8939070df)

Can you add a comment about what why BLOCK_MISSING_PREV and BLOCK_CACHED_INVALID are special here? Missing prev case seems to make sense and I see that the cached_invalid case isn’t changing behavior, but I don’t understand it.

jnewbery commented at 8:16 pm on July 18, 2019:

No longer part of this PR.

in src/net_processing.cpp:2629 in a307681758 outdated

2603@@ -2603,8 +2604,8 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
2604             LogPrint(BCLog::MEMPOOLREJ, "%s from peer=%d was not accepted: %s\n", tx.GetHash().ToString(),
2605                 pfrom->GetId(),
2606                 FormatStateMessage(state));
2607-            if (enable_bip61 && state.GetRejectCode() > 0 && state.GetRejectCode() < REJECT_INTERNAL) { // Never send AcceptToMemoryPool's internal codes over P2P
2608-                connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::REJECT, strCommand, (unsigned char)state.GetRejectCode(),
2609+            if (enable_bip61 && state.IsInvalid()) {

ryanofsky commented at 3:10 pm on May 30, 2019:

In commit “[validation] Remove Reject Code from ValidationState” (a3076817588a642b4de107dc96a76ea8939070df):

Note: IIUC, there is no change in behavior here because previous state.GetRejectCode() < REJECT_INTERNAL condition was always true. REJECT_HIGHFEE is never triggered for p2p transactions.

jnewbery commented at 8:16 pm on July 18, 2019:

No longer part of this PR.

in src/validation.cpp:449 in a307681758 outdated

587@@ -588,28 +588,28 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
588 
589     // Coinbase is only valid in a block, not as a loose transaction
590     if (tx.IsCoinBase())
591-        return state.Invalid(TxValidationResult::CONSENSUS, false, REJECT_INVALID, "coinbase");
592+        return state.Invalid(TxValidationResult::CONSENSUS, false, "coinbase");
593 
594     // Rather not work on nonstandard transactions (unless -testnet/-regtest)
595-    std::string reason;
596+    std::string reason {};

ryanofsky commented at 3:12 pm on May 30, 2019:

In commit “[validation] Remove Reject Code from ValidationState” (a3076817588a642b4de107dc96a76ea8939070df)

Unintentional change? Doesn’t serve an obvious purpose.

jnewbery commented at 8:16 pm on July 18, 2019:

No longer part of this PR.

in src/validation.cpp:752 in a307681758 outdated

750-            return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, false, REJECT_INSUFFICIENTFEE, "min relay fee not met", strprintf("%d < %d", nModifiedFees, ::minRelayTxFee.GetFee(nSize)));
751+            return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, false, "min relay fee not met", strprintf("%d < %d", nModifiedFees, ::minRelayTxFee.GetFee(nSize)));
752         }
753 
754         if (nAbsurdFee && nFees > nAbsurdFee)
755             return state.Invalid(TxValidationResult::TX_NOT_STANDARD, false,

ryanofsky commented at 3:15 pm on May 30, 2019:

In commit “[validation] Remove Reject Code from ValidationState” (a3076817588a642b4de107dc96a76ea8939070df)

Code predates this commit, but TX_MEMPOOL_POLICY would seem to make more sense here than TX_NOT_STANDARD. It might be good to add a comment if using TX_NOT_STANDARD is important here, since it’s not the obvious choice.

jnewbery commented at 8:16 pm on July 18, 2019:

No longer part of this PR.

jnewbery force-pushed on May 30, 2019

jnewbery commented at 10:09 pm on May 30, 2019: member

Thanks for the review @ryanofsky and @ajtowns . I’ve addressed all the feedback on commit CValidationState subclasses.

DrahtBot added the label Needs rebase on Jun 5, 2019

jnewbery force-pushed on Jun 5, 2019

jnewbery commented at 11:12 am on June 5, 2019: member

rebased

DrahtBot removed the label Needs rebase on Jun 5, 2019

in src/consensus/validation.h:84 in b31987c450 outdated

 96+    BLOCK_CACHED_INVALID,    //!< this object was cached as being invalid, but we don't know why
 97+    BLOCK_INVALID_HEADER,    //!< invalid proof of work or time too old
 98+    BLOCK_MUTATED,           //!< the block's data didn't match the data committed to by the PoW
 99+    BLOCK_MISSING_PREV,      //!< We don't have the previous block the checked one is built on
100+    BLOCK_INVALID_PREV,      //!< A block this one builds on is invalid
101+    BLOCK_BAD_TIME,          //!< block timestamp was > 2 hours in the future (or our clock is bad)

ryanofsky commented at 1:36 pm on June 6, 2019:

In commit “[validation] Add CValidationState subclasses” (b31987c45045cc26ca7867d9d8aa645f5d36b5e6)

The previous name BLOCK_TIME_FUTURE seems more descriptive than BLOCK_BAD_TIME. It’s nice how it says more specifically how the time is bad.

jnewbery commented at 8:21 pm on July 18, 2019:

Reverted. This PR was based on an earlier version of 15141, which I guess had the name BLOCK_BAD_TIME.

in src/net_processing.cpp:1001 in b31987c450 outdated

990  * block message or not. If the compact block had a valid header, but contained invalid
991  * txs, the peer should not be punished. See BIP 152.
992  *
993  * @return Returns true if the peer was punished (probably disconnected)
994- *
995- * Changes here may need to be reflected in TxRelayMayResultInDisconnect().

ryanofsky commented at 10:10 am on June 7, 2019:

In commit “[validation] Add CValidationState subclasses” (b31987c45045cc26ca7867d9d8aa645f5d36b5e6)

Any reason this comment is being deleted? It’s little vague, but I think it’s just saying that this function bans peers, and that TxRelayMayResultInDisconnect tries to avoid being banned by peers, so a change here might require a change there.

jnewbery commented at 8:24 pm on July 18, 2019:

Because this function is to decide whether to punish a peer for a bad block. The function below is to decide whether to punish a peer for a bad tx. Changes to that function need to be reflected in TxRelayMayResultInDisconnect() since they’re both concerned with txs.

in src/validation.cpp:1889 in b31987c450 outdated

1891-                    state.Invalid(ValidationInvalidReason::CONSENSUS, false,
1892-                              state.GetRejectCode(), state.GetRejectReason(), state.GetDebugMessage());
1893-                }
1894+            TxValidationState tx_state;
1895+            if (!CheckInputs(tx, tx_state, view, fScriptChecks, flags, fCacheResults, fCacheResults, txdata[i], nScriptCheckThreads ? &vChecks : nullptr)) {
1896+                // Any transaction validation failure in ConectBlock is a block consensus failure

ryanofsky commented at 10:19 am on June 7, 2019:

In commit “[validation] Add CValidationState subclasses” (b31987c45045cc26ca7867d9d8aa645f5d36b5e6)

spelling ConnectBlock

jnewbery commented at 8:26 pm on July 18, 2019:

fixed

in src/validation.cpp:1847 in b31987c450 outdated

1847-                    state.Invalid(ValidationInvalidReason::CONSENSUS, false,
1848-                            state.GetRejectCode(), state.GetRejectReason(), state.GetDebugMessage());
1849-                }
1850+            TxValidationState tx_state;
1851+            if (!Consensus::CheckTxInputs(tx, tx_state, view, pindex->nHeight, txfee)) {
1852+                // Any transaction validation failure in ConectBlock is a block consensus failure

ryanofsky commented at 10:19 am on June 7, 2019:

In commit “[validation] Add CValidationState subclasses” (b31987c45045cc26ca7867d9d8aa645f5d36b5e6)

spelling ConnectBlock

jnewbery commented at 8:26 pm on July 18, 2019:

fixed

in src/validation.cpp:2101 in b31987c450 outdated

1837@@ -1839,20 +1838,16 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
1838         if (!tx.IsCoinBase())
1839         {
1840             CAmount txfee = 0;
1841-            if (!Consensus::CheckTxInputs(tx, state, view, pindex->nHeight, txfee)) {
1842-                if (!IsBlockReason(state.GetReason())) {

ryanofsky commented at 10:23 am on June 7, 2019:

In commit “[validation] Add CValidationState subclasses” (b31987c45045cc26ca7867d9d8aa645f5d36b5e6)

To be sure, is there no change in behavior here because IsBlockReason would always return false if CheckTxInputs set failed with any reason other than CONSENSUS? Or is there no change in behavior for some other reason? Or is there a actually a change in some cases?

Related question: Is it a problem going forward that TxValidationResult::RECENT_CONSENSUS_CHANGE will be translated to BlockValidationResult::CONSENSUS_CHANGE instead of BlockValidationResult::RECENT_CONSENSUS_CHANGE?

jnewbery commented at 9:53 pm on July 18, 2019:

Take a look at CheckTxInputs(). Prior to this commit, if it returned false, then state.m_reason could be set to:

TX_MISSING_INPUTS
TX_PREMATURE_SPEND
CONSENSUS

In all cases, that would be converted to CONSENSUS, so this is not a change in behaviour.

Is it a problem going forward that TxValidationResult::RECENT_CONSENSUS_CHANGE will be translated to BlockValidationResult::CONSENSUS_CHANGE instead of BlockValidationResult::RECENT_CONSENSUS_CHANGE?

I don’t think so. Anyone introducing RECENT_CONSENSUS_CHANGE should verify that it gets properly propagated up the call stack.

in src/validation.cpp:2146 in b31987c450 outdated

1879         if (!tx.IsCoinBase())
1880         {
1881             std::vector<CScriptCheck> vChecks;
1882             bool fCacheResults = fJustCheck; /* Don't cache results if we're actually connecting blocks (still consult the cache, though) */
1883-            if (!CheckInputs(tx, state, view, fScriptChecks, flags, fCacheResults, fCacheResults, txdata[i], nScriptCheckThreads ? &vChecks : nullptr)) {
1884-                if (state.GetReason() == ValidationInvalidReason::TX_NOT_STANDARD) {

ryanofsky commented at 10:31 am on June 7, 2019:

In commit “[validation] Add CValidationState subclasses” (b31987c45045cc26ca7867d9d8aa645f5d36b5e6)

Similar questions here as for the CheckTxInputs check above. What’s the explanation for why there’s no change in behavior here? Would CheckInputs never set reasons other than CONSENSUS and TX_NOT_STANDARD?

And is it a problem that the recent_consensus code will be translated to just consensus?

jnewbery commented at 9:55 pm on July 18, 2019:

Would CheckInputs never set reasons other than CONSENSUS and TX_NOT_STANDARD?

Exactly correct.

And is it a problem that the recent_consensus code will be translated to just consensus?

As above. Any PR that introduces recent_consensus should include tests to make sure it does what it’s supposed to.

ryanofsky commented at 10:47 am on June 7, 2019: member

Started review (will update list below with progress).

b31987c45045cc26ca7867d9d8aa645f5d36b5e6 [validation] Add CValidationState subclasses (1/7)
22be379c54eb03e77803b0bbd8270082452d8fff [validation] Remove Reject Code from ValidationState (2/7)
b38f22795106501afb63823cc28aa768590ac5c4 [validation] Tidy Up ValidationResult class (3/7)
1550f781fc0a86fd318883512a7104d018cf7c56 [validation] Remove error() calls from Invalid() calls (4/7)
c3d2a7c712481eedefe6da2cdfd5eb4f5859d16f [validation] Remove useless ret parameter from Invalid() (5/7)
3b578fdca854d230592023baf5ef7863ead317b6 [validation] Remove unused first_invalid parameter from ProcessNewBlockHeaders() (6/7)
b7e1600e4df926da35b1664c32911d909899dd44 [validation] Remove fMissingInputs from AcceptToMemoryPool() (7/7)

in src/validation.cpp:3306 in b31987c450 outdated

2981-            return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-cb-multiple", "more than one coinbase");
2982+            return state.Invalid(BlockValidationResult::CONSENSUS, false, REJECT_INVALID, "bad-cb-multiple", "more than one coinbase");
2983 
2984     // Check transactions
2985-    for (const auto& tx : block.vtx)
2986-        if (!CheckTransaction(*tx, state, true))

ryanofsky commented at 10:59 am on June 7, 2019:

In commit “[validation] Add CValidationState subclasses” (b31987c45045cc26ca7867d9d8aa645f5d36b5e6)

Similar questions here as for the ConnectBlock checks above. What’s the explanation for why there’s no change in behavior here? Would CheckTransaction never set a reason besides CONSENSUS? Is it a problem that the recent_consensus code will be translated to just consensus?

jnewbery commented at 9:59 pm on July 18, 2019:

Would CheckTransaction never set a reason besides CONSENSUS?

Correct - CheckTransaction only ever sets CONSENSUS.

Is it a problem that the recent_consensus

as above

in src/validation.cpp:1806 in 1550f781fc outdated

1798@@ -1799,8 +1799,8 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state,
1799         for (const auto& tx : block.vtx) {
1800             for (size_t o = 0; o < tx->vout.size(); o++) {
1801                 if (view.HaveCoin(COutPoint(tx->GetHash(), o))) {
1802-                    return state.Invalid(BlockValidationResult::CONSENSUS, error("ConnectBlock(): tried to overwrite transaction"),
1803-                                     "bad-txns-BIP30");
1804+                    LogPrintf("ConnectBlock(): tried to overwrite transaction\n");

ryanofsky commented at 10:47 am on June 8, 2019:

In commit “[validation] Remove error() calls from Invalid() calls” (1550f781fc0a86fd318883512a7104d018cf7c56)

Is it intentional to drop the ERROR: prefix here and elsewhere in this PR?

https://github.com/bitcoin/bitcoin/blob/5d2ccf0ce9ca1571c650a69319fb9c1e0b626ecb/src/util/system.h#L60-L65

If so, it would be good to mention the change in behavior in the commit message, and maybe in release notes.

jnewbery commented at 10:08 pm on July 18, 2019:

I can’t remember whether it was intentional or not. I’ve re-added the ERROR: prefix so there’s no change in logging behaviour.

in src/validation.h:224 in 3b578fdca8 outdated

228@@ -229,9 +229,8 @@ bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<cons
229  * @param[out] state This may be set to an Error state if any error occurred processing them
230  * @param[in]  chainparams The params for the chain we want to connect to
231  * @param[out] ppindex If set, the pointer will be set to point to the last new block index object for the given headers
232- * @param[out] first_invalid First header that fails validation, if one exists

ryanofsky commented at 11:23 am on June 8, 2019:

In commit “[validation] Remove unused first_invalid parameter from ProcessNewBlockHeaders()” (3b578fdca854d230592023baf5ef7863ead317b6)

Note: it looks like the last place a first_invalid output value was actually used was removed in 7df16e70e67c753c871797ce947ea09d7cb0e519 from #15141

in src/interfaces/chain.cpp:155 in b7e1600e4d outdated

151@@ -152,7 +152,7 @@ class LockImpl : public Chain::Lock, public UniqueLock<CCriticalSection>
152     bool submitToMemoryPool(const CTransactionRef& tx, CAmount absurd_fee, TxValidationState& state) override
153     {
154         LockAssertion lock(::cs_main);
155-        return AcceptToMemoryPool(::mempool, state, tx, nullptr /* missing inputs */, nullptr /* txn replaced */,
156+        return AcceptToMemoryPool(::mempool, state, tx, nullptr /* missing inputs */,

ryanofsky commented at 11:37 am on June 8, 2019:

In commit “[validation] Remove fMissingInputs from AcceptToMemoryPool()” (b7e1600e4df926da35b1664c32911d909899dd44)

Should remove the “missing inputs” argument instead of “txn replaced” here.

jnewbery commented at 10:10 pm on July 18, 2019:

fixed

in src/node/transaction.cpp:44 in b7e1600e4d outdated

29@@ -30,16 +30,15 @@ TransactionError BroadcastTransaction(const CTransactionRef tx, uint256& hashTx,
30     if (!fHaveMempool && !fHaveChain) {
31         // push to local node and sync with wallets
32         TxValidationState state;
33-        bool fMissingInputs;
34-        if (!AcceptToMemoryPool(mempool, state, std::move(tx), &fMissingInputs,
35+        if (!AcceptToMemoryPool(mempool, state, std::move(tx),
36                                 nullptr /* plTxnReplaced */, false /* bypass_limits */, highfee)) {
37             if (state.IsInvalid()) {
38+                if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) {

ryanofsky commented at 11:41 am on June 8, 2019:

In commit “[validation] Remove fMissingInputs from AcceptToMemoryPool()” (b7e1600e4df926da35b1664c32911d909899dd44)

This is not a change in behavior, but it seems strange that err_string is not set in this case. It would be good to add a comment if this is intentional, because otherwise it seems like a bug.

promag commented at 1:28 am on June 9, 2019:

Good catch.

jnewbery commented at 10:15 pm on July 18, 2019:

It was intentional to maintain existing behaviour, but you’re right that it looks odd and buggy to anyone reading the code now. Rather than write a comment explaining that I’m maintaining existing behaviour, I’m just going to set err_string in all cases.

in src/rpc/rawtransaction.cpp:903 in b7e1600e4d outdated

932     if (!test_accept_res) {
933         if (state.IsInvalid()) {
934+            if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) {
935+                result_0.pushKV("reject-reason", "missing-inputs");
936+            }
937             result_0.pushKV("reject-reason", strprintf("%s", state.GetRejectReason()));

ryanofsky commented at 11:49 am on June 8, 2019:

In commit “[validation] Remove fMissingInputs from AcceptToMemoryPool()” (b7e1600e4df926da35b1664c32911d909899dd44)

Seems like a bug here. It looks like this pushKV will overwrite the value set by the pushKV call added above, making it have no effect. If this is fixed, maybe the python tests below don’t have to change.

promag commented at 1:33 am on June 9, 2019:

Just one more case where univalue should have different modifiers.

Anyway looks like it’s should be in else { ... } block.

jnewbery commented at 11:55 pm on July 18, 2019:

Wow, that’s an embarrassing bug. I think I just misread the pushKVs as early returns. Thanks for catching!

Yes, the fix here is to put this code in an else block. Done.

ryanofsky approved

ryanofsky commented at 12:14 pm on June 8, 2019: member

Conditional utACK b7e1600e4df926da35b1664c32911d909899dd44 if a few comments are addressed:

#15921 (review) #15921 (review) #15921 (review) #15921 (review)

It would also be nice to get responses to:

#15921 (review) #15921 (review)

My other comments are minor and could be ignored.

This cleanup is really nice and makes the ValidationState interface a lot nicer and more understandable.

DrahtBot added the label Needs rebase on Jun 25, 2019

in src/consensus/validation.h:88 in b7e1600e4d outdated

136+    } m_mode;
137+    std::string m_reject_reason;
138+    std::string m_debug_message;
139+protected:
140+    void Invalid(const std::string &reject_reason="",
141+                 const std::string &debug_message="")

l2a5b1 commented at 9:50 pm on July 1, 2019:

Happy to see that Invalid’s parameter-declaration-clause has been cleaned-up.

Perhaps you can take it one step further and also remove the two empty string default arguments here and in TxValidationState::Invalid(..) and BlockValidationState::Invalid(..)? As far as I can tell Invalid is always called with a reason and debug message (the way it should be called).

jnewbery commented at 10:43 pm on July 18, 2019:

I’d like to remove the optional parameters, but there are many calls to Invalid() that don’t set a debug message.

in src/consensus/validation.h:77 in b7e1600e4d outdated

107-/** Capture information about block/transaction validation */
108-class CValidationState {
109+/** Base class for capturing information about block/transaction validation. This is subclassed
110+ *  by TxValidationState and BlockValidationState for validation information on transactions
111+ *  and blocks respectively. */
112+class ValidationState {

l2a5b1 commented at 9:50 pm on July 1, 2019:

Not sure if you consider ValidationState to be an abstract base class that should not be instantiated directly, but if you do perhaps you could add a pure virtual destructor to protect against this.

jnewbery commented at 10:59 pm on July 18, 2019:

Done. Thanks for the review!

jkczyz commented at 11:34 pm on October 30, 2019:

Sorry for the late comment. Would a template class be more suitable? It appears that the subclasses only differ by the type of a member variable. So making this a template would remove a bit of boilerplate. Then a typedef could be used to keep the subclass names if desired.

jkczyz commented at 1:17 am on November 7, 2019:

Made the change in #17399. Happy to close the PR if this isn’t desirable for some reason.

l2a5b1 commented at 11:06 pm on July 1, 2019: contributor

Nice work!

My feedback is not critical, but it could make sense to address it as we are tidying things up here.

l2a5b1 commented at 1:26 pm on July 2, 2019: contributor

Not critical, but nice to have since we are tidying things up here:

In this PR the return type of ValidationState::Invalid method has been changed to void.

Perhaps you can also change the return type of ValidationState::Error, TxValidationState ::Invalid, BlockValidationState::Invalid methods to void?

I don’t think it is the responsibility of these methods to return false and make assumptions on behalf of the code at the call site.

jnewbery force-pushed on Jul 18, 2019

jnewbery commented at 6:49 pm on July 18, 2019: member

Rebased on master: https://github.com/bitcoin/bitcoin/compare/b7e1600e4df926da35b1664c32911d909899dd44..d79797b61ef986f6457235febdf5797a68683ad1

I still have to address all of Russ’s feedback.

DrahtBot removed the label Needs rebase on Jul 18, 2019

jnewbery force-pushed on Jul 18, 2019

jnewbery commented at 8:15 pm on July 18, 2019: member

I’ve removed the [validation] Remove Reject Code from ValidationState commit from this PR (https://github.com/bitcoin/bitcoin/compare/d79797b61ef986f6457235febdf5797a68683ad1..47380ebccfb426d3cfbb44b901dafbbde839559b). See #15921 (review) for reason.

jnewbery force-pushed on Jul 18, 2019

jnewbery commented at 11:01 pm on July 18, 2019: member

I believe I’ve now addressed all comments. This should be ready for review. @l2a5b1

Perhaps you can also change the return type of ValidationState::Error, TxValidationState ::Invalid, BlockValidationState::Invalid methods to void?

Seems like a reasonable change, but I’d rather not expand this PR even more. It’s already making a lot of changes.

MarcoFalke removed the label Refactoring on Jul 18, 2019

jnewbery force-pushed on Jul 18, 2019

jnewbery commented at 11:55 pm on July 18, 2019: member

Fixed the bad rebase in validation_block_tests.cpp.

jnewbery force-pushed on Jul 18, 2019

DrahtBot added the label Needs rebase on Jul 19, 2019

jnewbery force-pushed on Jul 19, 2019

jnewbery commented at 7:57 pm on July 19, 2019: member

rebased

DrahtBot removed the label Needs rebase on Jul 19, 2019

DrahtBot added the label Needs rebase on Jul 27, 2019

jnewbery renamed this:
~~Tidy up ValidationState interface~~
validation: Tidy up ValidationState interface
on Aug 25, 2019

jnewbery force-pushed on Oct 1, 2019

jnewbery commented at 10:37 pm on October 1, 2019: member

Rebased on master. This is now based on #17004. Please review that first.

DrahtBot removed the label Needs rebase on Oct 2, 2019

ryanofsky commented at 2:28 pm on October 2, 2019: member

re: #15921#pullrequestreview-247365826

Conditional utACK b7e1600 if a few comments are addressed

It’s been a while, but this is next on my review list after 16341 (giant achow keyman pr), so I didn’t forget about this. It may help to add “Based on #15437 and #17004” to the top of the PR description to make this seem less big and show other reviewers where to look first.

in src/consensus/validation.h:16 in 907ef02667 outdated

14@@ -15,7 +15,7 @@
15 /** A "reason" why something was invalid, suitable for determining whether the
16   * provider of the object should be banned/ignored/disconnected/etc.

ryanofsky commented at 4:08 pm on October 8, 2019:

In commit “[validation] Add CValidationState subclasses” (907ef02667b6c224df1457b67b7d7950cd58494f)

A previous version of this commit changed “something” to “a transaction” and “object” to “tx”. Would be nice to keep these changes.

jnewbery commented at 8:29 pm on October 8, 2019:

fixed

in src/validation.cpp:1552 in 907ef02667 outdated

1545@@ -1547,7 +1546,7 @@ bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsVi
1546                 CScriptCheck check2(coin.out, tx, i,
1547                         flags & ~STANDARD_NOT_MANDATORY_VERIFY_FLAGS, cacheSigStore, &txdata);
1548                 if (check2())
1549-                    return state.Invalid(ValidationInvalidReason::TX_NOT_STANDARD, false, strprintf("non-mandatory-script-verify-flag (%s)", ScriptErrorString(check.GetScriptError())));
1550+                    return state.Invalid(TxValidationResult::TX_NOT_STANDARD, false, strprintf("non-mandatory-script-verify-flag (%s)", ScriptErrorString(check.GetScriptError())));
1551             }
1552             // MANDATORY flag failures correspond to
1553             // ValidationInvalidReason::CONSENSUS. Because CONSENSUS

ryanofsky commented at 4:16 pm on October 8, 2019:

In commit “[validation] Add CValidationState subclasses” (907ef02667b6c224df1457b67b7d7950cd58494f)

Could s/ValidationInvalidReason/TxValidationResult/ here like previous versions of this commit

jnewbery commented at 8:29 pm on October 8, 2019:

fixed

jnewbery force-pushed on Oct 8, 2019

jnewbery commented at 8:29 pm on October 8, 2019: member

Thanks for the review @ryanofsky . I’ve addressed both of your comments.

jnewbery force-pushed on Oct 9, 2019

jnewbery commented at 2:14 pm on October 9, 2019: member

rebased. Still depends on #17004, so please review that PR first.

in src/validation.cpp:2143 in b885aec88a outdated

2150-                    // RECENT_CONSENSUS_CHANGE would be more appropriate.
2151-                    state.Invalid(ValidationInvalidReason::CONSENSUS, false,
2152-                              state.GetRejectReason(), state.GetDebugMessage());
2153-                }
2154+            TxValidationState tx_state;
2155+            if (!CheckInputs(tx, tx_state, view, flags, fCacheResults, fCacheResults, txdata[i], nScriptCheckThreads ? &vChecks : nullptr)) {

ryanofsky commented at 6:21 pm on October 15, 2019:

In commit “[validation] Add CValidationState subclasses” (b885aec88a627363bf6771c941e855fa735145c6)

Is it intentional to drop the fScriptChecks && condition above?

jnewbery commented at 7:03 pm on October 15, 2019:

eeek. Good catch. This was a bad rebase (and would have broken -assumevalid if it’d been merged).

jnewbery force-pushed on Oct 15, 2019

jnewbery commented at 7:06 pm on October 15, 2019: member

Fixed the bad rebase pointed out by @ryanofsky (https://github.com/bitcoin/bitcoin/pull/15921#discussion_r335106673): https://github.com/bitcoin/bitcoin/compare/6fec91c1212c41698d1c9464379d17627cccf42a..b68a33829827c51048784a6f83f8acea3351d3c0

jnewbery force-pushed on Oct 15, 2019

jnewbery commented at 7:20 pm on October 15, 2019: member

rebased: https://github.com/bitcoin/bitcoin/compare/b68a33829827c51048784a6f83f8acea3351d3c0..735e86fac20adee2c42e34ee382aaea95fbdbbb3

ryanofsky approved

ryanofsky commented at 7:34 pm on October 15, 2019: member

Code review ACK 735e86fac20adee2c42e34ee382aaea95fbdbbb3. There was a pretty big rebase since my last ACK. I rereviewed the first state subclasses commit and dug into ConnectBlock check calls more to verify no behavior should be changing. For the later commits I rebased and compared the changes against the previous PR. Only changes were various things discussed in comments: bugfixes, adding back log ERROR prefixes, and err_string setting in ATMP.

jnewbery commented at 7:47 pm on October 15, 2019: member

Thanks for the rereview @ryanofsky !

DrahtBot added the label Needs rebase on Oct 23, 2019

jnewbery force-pushed on Oct 23, 2019

jnewbery commented at 9:27 pm on October 23, 2019: member

rebased

DrahtBot removed the label Needs rebase on Oct 23, 2019

DrahtBot added the label Needs rebase on Oct 24, 2019

jnewbery force-pushed on Oct 24, 2019

jnewbery commented at 3:57 pm on October 24, 2019: member

Rebased on master. Also tidied up a few small details (no functional changes)

jnewbery force-pushed on Oct 24, 2019

ryanofsky approved

ryanofsky commented at 5:36 pm on October 24, 2019: member

Code review ACK 9ec6b4fb30142c182212fb24a581774a18fca00c. Not substantial changes since last review, mainly rebase. Also new assert, RECENT_CONSENSUS enums, some more TX_ and BLOCK_ enum prefixes, and comments in the first commit. And more removed error() calls in the third commit, I think restoring a removed ERROR: log prefix.

DrahtBot removed the label Needs rebase on Oct 24, 2019

jnewbery force-pushed on Oct 24, 2019

jnewbery commented at 7:21 pm on October 24, 2019: member

The linter caught a log that wasn’t terminated with a newline. Pushed a fix.

jnewbery removed the label RPC/REST/ZMQ on Oct 24, 2019

jnewbery removed the label Wallet on Oct 24, 2019

ryanofsky approved

ryanofsky commented at 8:57 pm on October 24, 2019: member

Code review ACK 72d303bf60a87a6ee559ca06627fff4f2d3457a0. Changes since last review: fixed log newline and increased linter happiness

DrahtBot added the label Needs rebase on Oct 25, 2019

mzumsande commented at 1:00 pm on October 25, 2019: member

The fuzzing harness src/test/fuzz/transaction.cpp added in #17076 uses CValidationState and needs an update.

laanwj added this to the milestone 0.20.0 on Oct 25, 2019

jnewbery force-pushed on Oct 25, 2019

jnewbery commented at 2:26 pm on October 25, 2019: member

The fuzzing harness src/test/fuzz/transaction.cpp added in #17076 uses CValidationState and needs an update.

Thanks @mzumsande . Now fixed. I’ve also rebased on master.

DrahtBot removed the label Needs rebase on Oct 25, 2019

ryanofsky approved

ryanofsky commented at 2:21 pm on October 26, 2019: member

Code review ACK dcf5003ca0d8927f3e2457a32394a70ed683f01e. Changes since last review: rebase after fCheckDuplicateInputs removal and fuzz test compile fix

laanwj commented at 6:44 pm on October 27, 2019: member

ACK dcf5003ca0d8927f3e2457a32394a70ed683f01e ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf

in src/consensus/validation.h:104 in bd6ac0e1cc outdated

133         mode = MODE_INVALID;
134         return ret;
135     }
136+public:
137+    // We use ValidationState polymorphically. Have a virtual destructor
138+    virtual ~ValidationState() {}

mzumsande commented at 1:52 am on October 29, 2019:

Your old answer to to an old comment indicates that at some point you wanted to make the base class ValidationState abstract. However, the class has a (non-pure) virtual destructor and no pure virtual functions, so instantiation of ValidationState should still be possible. Is this working as intended?

in src/consensus/tx_check.cpp:10 in bd6ac0e1cc outdated

 6@@ -7,28 +7,28 @@
 7 #include <primitives/transaction.h>
 8 #include <consensus/validation.h>
 9 
10-bool CheckTransaction(const CTransaction& tx, CValidationState& state)
11+bool CheckTransaction(const CTransaction& tx, TxValidationState &state)

fjahr commented at 12:16 pm on October 29, 2019:

nit: the & was moved from the type to the variable name. For consistency I would keep it at end of the type. That’s also how it is still declared in the header file.

fjahr approved

fjahr commented at 12:18 pm on October 29, 2019: member

Code review ACK dcf5003. Also built and ran tests locally.

I agree with @JeremyRubin that Invalid should return void ideally. Also, have you thought about moving the punishment logic into its own class and file? It’s not much of a difference in terms of LoC but reducing net_processing.cpp in size would not be a bad thing and it feels like that logic could deserve its own place due to its importance.

jnewbery commented at 2:07 pm on October 29, 2019: member

Thanks for the reviews @mzumsande and @fjahr . I’ve added a couple of fixup commits that address your comments. Let me know if you like them and I’ll squash into the appropriate commits. @mzumsande - I had to add a consensus/validation.cpp file to define the ValidationState destructor. Let me know if you know of a better way to do this.

mzumsande commented at 3:55 pm on October 29, 2019: member

For me, inline ValidationState::~ValidationState() {}; in the header works as well to make the class abstract (the inline prevents multiple definition linker errors). I am not sure about the cause for the MSVC/AppVeyor error of the current implementation.

jnewbery force-pushed on Oct 29, 2019

jnewbery commented at 5:00 pm on October 29, 2019: member

For me, inline ValidationState::~ValidationState() {}; in the header works

Thanks. Much better!

fjahr commented at 5:08 pm on October 29, 2019: member

Thanks for the reviews @mzumsande and @fjahr . I’ve added a couple of fixup commits that address your comments. Let me know if you like them and I’ll squash into the appropriate commits.

@mzumsande - I had to add a consensus/validation.cpp file to define the ValidationState destructor. Let me know if you know of a better way to do this.

Looks good to me!

[validation] Add CValidationState subclasses

Split CValidationState into TxValidationState and BlockValidationState
to store validation results for transactions and blocks respectively.

a27a2957ed

[validation] Tidy Up ValidationResult class

Minor style fixups and comment updates.

This is purely a style change. There is no change in behavior.

067981e492

[validation] Remove error() calls from Invalid() calls

This is in preparation for the next commit, which removes the useless
`ret` parameter from ValidationState::Invalid().

error() is simply a convenience wrapper that calls LogPrintf and returns
false. Call LogPrintf explicitly and substitute the error() call for a
false bool literal.

1a37de4b31

[validation] Remove useless ret parameter from Invalid()

ValidationState::Invalid() takes a parameter `ret` which is returned to
the caller. All call sites set this to false. Remove the `ret` parameter
and just return false always.

7204c6434b

[validation] Remove unused first_invalid parameter from ProcessNewBlockHeaders()

No callers use the returned value in first_invalid. Remove it from the
function signature and don't set it in the function.

c428622a5b

[validation] Remove fMissingInputs from AcceptToMemoryPool()

Handle this failure in the same way as all other failures: call Invalid()
with the reasons for the failure.

3004d5a12d

jnewbery force-pushed on Oct 29, 2019

jnewbery commented at 7:46 pm on October 29, 2019: member

squashed fixup commits.

ryanofsky approved

ryanofsky commented at 10:25 pm on October 29, 2019: member

Code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf. Just whitespace change and pure virtual destructor added since last review.

amitiuttarwar commented at 11:41 pm on October 29, 2019: contributor

code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf. Also built & ran tests locally.

fjahr commented at 11:52 pm on October 29, 2019: member

Code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf . Only nit style change and pure virtual destructor added since my last review.

laanwj referenced this in commit 3c40bc6726 on Oct 30, 2019

laanwj merged this on Oct 30, 2019

laanwj closed this on Oct 30, 2019

jnewbery deleted the branch on Oct 30, 2019

jonatack commented at 2:44 pm on October 30, 2019: member

There oughta be a rule about merging review club PRs right before the meeting :)

laanwj commented at 2:46 pm on October 30, 2019: member

@jonatack whoops … nah, reviewing doesn’t necessarily stop after merging :smile:

mzumsande commented at 2:50 pm on October 30, 2019: member

Ok, will still mention my code-review ACK for 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf that I was just about to write when this got merged :-)

MarcoFalke commented at 2:55 pm on October 30, 2019: member

nah, reviewing doesn’t necessarily stop after merging

Agree. The only downside is that review comments (with the reviewer’s name) can not be included in the merge commit.

jonatack commented at 4:05 pm on October 30, 2019: member

Could possibly use a review club tag :label:

jonatack commented at 6:41 pm on October 30, 2019: member

Built at 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf, ran tests, running bitcoind tailing the log.

Code review progress:

[validation] Add CValidationState subclasses
[validation] Tidy Up ValidationResult class
[validation] Remove error() calls from Invalid() calls
[validation] Remove useless ret parameter from Invalid()
[validation] Remove unused first_invalid parameter from ProcessNewBlo…
[validation] Remove fMissingInputs from AcceptToMemoryPool()

Interesting reviewer tip in the PR description; result here (without the right colors).

laanwj added the label Review club on Oct 30, 2019

laanwj commented at 6:47 pm on October 30, 2019: member

Could possibly use a review club tag label

Done

jonatack commented at 7:02 pm on October 30, 2019: member

Perhaps helpful context for future readers/reviewers, here are two comments motivating a27a2957ed9afbe5a96caa5f0f4cbec730d27460 (CValidationState subclasses) from discussion in #15141:

sipa: #15141 (comment)
theBlueMatt: #15141 (review)

jonatack commented at 7:04 pm on October 30, 2019: member

Could possibly use a review club tag label

Done

Thanks!

practicalswift commented at 10:20 pm on November 27, 2019: contributor

Reviewers of this PR are encouraged to review PR #17624 (“net: Fix an uninitialized read in ProcessMessage(…, “tx”, …) when receiving a transaction we already have”) which fixes a quite serious bug introduced in this PR. Luckily it was caught before being part of any release :)

laanwj referenced this in commit 114e89e596 on Nov 28, 2019

jasonbcox referenced this in commit feee46d6ef on Jul 9, 2020

jasonbcox referenced this in commit 754a9d81cf on Jul 9, 2020

jasonbcox referenced this in commit 0b39bc3f5a on Jul 10, 2020

jasonbcox referenced this in commit 99589edae4 on Jul 13, 2020

jasonbcox referenced this in commit 0704a943d9 on Jul 14, 2020

MarcoFalke locked this on Dec 16, 2021

validation: Tidy up ValidationState interface #15921

Conflicts