From 7bbd846f0f5c0d333f5c582692a7beb9e6aa8178 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 6 Feb 2020 14:30:37 +0000 Subject: [PATCH] Apply a consistent ban policy within ContextualCheckTransaction --- src/gtest/test_checktransaction.cpp | 49 ++++++++---- src/gtest/test_transaction_builder.cpp | 8 +- src/main.cpp | 106 +++++++++++++++++-------- src/main.h | 2 +- src/test/transaction_tests.cpp | 4 +- 5 files changed, 115 insertions(+), 54 deletions(-) diff --git a/src/gtest/test_checktransaction.cpp b/src/gtest/test_checktransaction.cpp index 04d69ab19..dd1c93cfc 100644 --- a/src/gtest/test_checktransaction.cpp +++ b/src/gtest/test_checktransaction.cpp @@ -167,7 +167,7 @@ TEST(checktransaction_tests, BadTxnsOversize) { // ... but fails contextual ones! EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "bad-txns-oversize", false)).Times(1); - EXPECT_FALSE(ContextualCheckTransaction(tx, state, Params(), 1, 100)); + EXPECT_FALSE(ContextualCheckTransaction(tx, state, Params(), 1, true)); } { @@ -188,7 +188,7 @@ TEST(checktransaction_tests, BadTxnsOversize) { MockCValidationState state; EXPECT_TRUE(CheckTransactionWithoutProofVerification(tx, state)); - EXPECT_TRUE(ContextualCheckTransaction(tx, state, Params(), 1, 100)); + EXPECT_TRUE(ContextualCheckTransaction(tx, state, Params(), 1, true)); // Revert to default RegtestDeactivateSapling(); @@ -503,11 +503,18 @@ TEST(checktransaction_tests, bad_txns_invalid_joinsplit_signature) { CTransaction tx(mtx); MockCValidationState state; - // during initial block download, DoS ban score should be zero, else 100 + // during initial block download, for transactions being accepted into the + // mempool (and thus not mined), DoS ban score should be zero, else 10 EXPECT_CALL(state, DoS(0, false, REJECT_INVALID, "bad-txns-invalid-joinsplit-signature", false)).Times(1); - ContextualCheckTransaction(tx, state, chainparams, 0, 100, [](const CChainParams&) { return true; }); + ContextualCheckTransaction(tx, state, chainparams, 0, false, [](const CChainParams&) { return true; }); + EXPECT_CALL(state, DoS(10, false, REJECT_INVALID, "bad-txns-invalid-joinsplit-signature", false)).Times(1); + ContextualCheckTransaction(tx, state, chainparams, 0, false, [](const CChainParams&) { return false; }); + // for transactions that have been mined in a block, DoS ban score should + // always be 100. EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "bad-txns-invalid-joinsplit-signature", false)).Times(1); - ContextualCheckTransaction(tx, state, chainparams, 0, 100, [](const CChainParams&) { return false; }); + ContextualCheckTransaction(tx, state, chainparams, 0, true, [](const CChainParams&) { return true; }); + EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "bad-txns-invalid-joinsplit-signature", false)).Times(1); + ContextualCheckTransaction(tx, state, chainparams, 0, true, [](const CChainParams&) { return false; }); } TEST(checktransaction_tests, non_canonical_ed25519_signature) { @@ -520,7 +527,7 @@ TEST(checktransaction_tests, non_canonical_ed25519_signature) { { CTransaction tx(mtx); MockCValidationState state; - EXPECT_TRUE(ContextualCheckTransaction(tx, state, chainparams, 0, 100)); + EXPECT_TRUE(ContextualCheckTransaction(tx, state, chainparams, 0, true)); } // Copied from libsodium/crypto_sign/ed25519/ref10/open.c @@ -540,11 +547,18 @@ TEST(checktransaction_tests, non_canonical_ed25519_signature) { CTransaction tx(mtx); MockCValidationState state; - // during initial block download, DoS ban score should be zero, else 100 + // during initial block download, for transactions being accepted into the + // mempool (and thus not mined), DoS ban score should be zero, else 10 EXPECT_CALL(state, DoS(0, false, REJECT_INVALID, "bad-txns-invalid-joinsplit-signature", false)).Times(1); - ContextualCheckTransaction(tx, state, chainparams, 0, 100, [](const CChainParams&) { return true; }); + ContextualCheckTransaction(tx, state, chainparams, 0, false, [](const CChainParams&) { return true; }); + EXPECT_CALL(state, DoS(10, false, REJECT_INVALID, "bad-txns-invalid-joinsplit-signature", false)).Times(1); + ContextualCheckTransaction(tx, state, chainparams, 0, false, [](const CChainParams&) { return false; }); + // for transactions that have been mined in a block, DoS ban score should + // always be 100. EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "bad-txns-invalid-joinsplit-signature", false)).Times(1); - ContextualCheckTransaction(tx, state, chainparams, 0, 100, [](const CChainParams&) { return false; }); + ContextualCheckTransaction(tx, state, chainparams, 0, true, [](const CChainParams&) { return true; }); + EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "bad-txns-invalid-joinsplit-signature", false)).Times(1); + ContextualCheckTransaction(tx, state, chainparams, 0, true, [](const CChainParams&) { return false; }); } TEST(checktransaction_tests, OverwinterConstructors) { @@ -806,7 +820,7 @@ TEST(checktransaction_tests, OverwinterVersionNumberHigh) { UNSAFE_CTransaction tx(mtx); MockCValidationState state; EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "bad-tx-overwinter-version-too-high", false)).Times(1); - ContextualCheckTransaction(tx, state, Params(), 1, 100); + ContextualCheckTransaction(tx, state, Params(), 1, true); // Revert to default UpdateNetworkUpgradeParameters(Consensus::UPGRADE_OVERWINTER, Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT); @@ -841,11 +855,18 @@ TEST(checktransaction_tests, OverwinterNotActive) { CTransaction tx(mtx); MockCValidationState state; - // during initial block download, DoS ban score should be zero, else 100 + // during initial block download, for transactions being accepted into the + // mempool (and thus not mined), DoS ban score should be zero, else 10 EXPECT_CALL(state, DoS(0, false, REJECT_INVALID, "tx-overwinter-not-active", false)).Times(1); - ContextualCheckTransaction(tx, state, chainparams, 1, 100, [](const CChainParams&) { return true; }); + ContextualCheckTransaction(tx, state, chainparams, 0, false, [](const CChainParams&) { return true; }); + EXPECT_CALL(state, DoS(10, false, REJECT_INVALID, "tx-overwinter-not-active", false)).Times(1); + ContextualCheckTransaction(tx, state, chainparams, 0, false, [](const CChainParams&) { return false; }); + // for transactions that have been mined in a block, DoS ban score should + // always be 100. EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "tx-overwinter-not-active", false)).Times(1); - ContextualCheckTransaction(tx, state, chainparams, 1, 100, [](const CChainParams&) { return false; }); + ContextualCheckTransaction(tx, state, chainparams, 0, true, [](const CChainParams&) { return true; }); + EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "tx-overwinter-not-active", false)).Times(1); + ContextualCheckTransaction(tx, state, chainparams, 0, true, [](const CChainParams&) { return false; }); } // This tests a transaction without the fOverwintered flag set, against the Overwinter consensus rule set. @@ -862,7 +883,7 @@ TEST(checktransaction_tests, OverwinterFlagNotSet) { CTransaction tx(mtx); MockCValidationState state; EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "tx-overwinter-flag-not-set", false)).Times(1); - ContextualCheckTransaction(tx, state, Params(), 1, 100); + ContextualCheckTransaction(tx, state, Params(), 1, true); // Revert to default UpdateNetworkUpgradeParameters(Consensus::UPGRADE_OVERWINTER, Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT); diff --git a/src/gtest/test_transaction_builder.cpp b/src/gtest/test_transaction_builder.cpp index 332c194f8..cd0680d9e 100644 --- a/src/gtest/test_transaction_builder.cpp +++ b/src/gtest/test_transaction_builder.cpp @@ -106,7 +106,7 @@ TEST(TransactionBuilder, TransparentToSapling) EXPECT_EQ(tx.valueBalance, -40000); CValidationState state; - EXPECT_TRUE(ContextualCheckTransaction(tx, state, Params(), 2, 0)); + EXPECT_TRUE(ContextualCheckTransaction(tx, state, Params(), 2, true)); EXPECT_EQ(state.GetRejectReason(), ""); // Revert to default @@ -143,7 +143,7 @@ TEST(TransactionBuilder, SaplingToSapling) { EXPECT_EQ(tx.valueBalance, 10000); CValidationState state; - EXPECT_TRUE(ContextualCheckTransaction(tx, state, Params(), 3, 0)); + EXPECT_TRUE(ContextualCheckTransaction(tx, state, Params(), 3, true)); EXPECT_EQ(state.GetRejectReason(), ""); // Revert to default @@ -181,7 +181,7 @@ TEST(TransactionBuilder, SaplingToSprout) { EXPECT_EQ(tx.valueBalance, 35000); CValidationState state; - EXPECT_TRUE(ContextualCheckTransaction(tx, state, Params(), 3, 0)); + EXPECT_TRUE(ContextualCheckTransaction(tx, state, Params(), 3, true)); EXPECT_EQ(state.GetRejectReason(), ""); // Revert to default @@ -242,7 +242,7 @@ TEST(TransactionBuilder, SproutToSproutAndSapling) { EXPECT_EQ(tx.valueBalance, -5000); CValidationState state; - EXPECT_TRUE(ContextualCheckTransaction(tx, state, Params(), 4, 0)); + EXPECT_TRUE(ContextualCheckTransaction(tx, state, Params(), 4, true)); EXPECT_EQ(state.GetRejectReason(), ""); // Revert to default diff --git a/src/main.cpp b/src/main.cpp index bcf652693..be5d9ef91 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -763,69 +763,96 @@ unsigned int GetP2SHSigOpCount(const CTransaction& tx, const CCoinsViewCache& in * 1. AcceptToMemoryPool calls CheckTransaction and this function. * 2. ProcessNewBlock calls AcceptBlock, which calls CheckBlock (which calls CheckTransaction) * and ContextualCheckBlock (which calls this function). - * 3. The isInitBlockDownload argument is only to assist with testing. + * 3. For consensus rules that relax restrictions (where a transaction that is invalid at + * nHeight can become valid at a later height), we make the bans conditional on not + * being in Initial Block Download mode. + * 4. The isInitBlockDownload argument is a function parameter to assist with testing. */ bool ContextualCheckTransaction( const CTransaction& tx, CValidationState &state, const CChainParams& chainparams, const int nHeight, - const int dosLevel, + const bool isMined, bool (*isInitBlockDownload)(const CChainParams&)) { + const int DOS_LEVEL_BLOCK = 100; + // DoS level set to 10 to be more forgiving. + const int DOS_LEVEL_MEMPOOL = 10; + + // For constricting rules, we don't need to account for IBD mode. + auto dosLevelConstricting = isMined ? DOS_LEVEL_BLOCK : DOS_LEVEL_MEMPOOL; + // For rules that are relaxing (or might become relaxing when a future + // network upgrade is implemented), we need to account for IBD mode. + auto dosLevelPotentiallyRelaxing = isMined ? DOS_LEVEL_BLOCK : ( + isInitBlockDownload(chainparams) ? 0 : DOS_LEVEL_MEMPOOL); + bool overwinterActive = chainparams.GetConsensus().NetworkUpgradeActive(nHeight, Consensus::UPGRADE_OVERWINTER); bool saplingActive = chainparams.GetConsensus().NetworkUpgradeActive(nHeight, Consensus::UPGRADE_SAPLING); bool isSprout = !overwinterActive; // If Sprout rules apply, reject transactions which are intended for Overwinter and beyond if (isSprout && tx.fOverwintered) { - return state.DoS(isInitBlockDownload(chainparams) ? 0 : dosLevel, - error("ContextualCheckTransaction(): overwinter is not active yet"), - REJECT_INVALID, "tx-overwinter-not-active"); + return state.DoS( + dosLevelPotentiallyRelaxing, + error("ContextualCheckTransaction(): overwinter is not active yet"), + REJECT_INVALID, "tx-overwinter-not-active"); } if (saplingActive) { // Reject transactions with valid version but missing overwintered flag if (tx.nVersion >= SAPLING_MIN_TX_VERSION && !tx.fOverwintered) { - return state.DoS(dosLevel, error("ContextualCheckTransaction(): overwintered flag must be set"), - REJECT_INVALID, "tx-overwintered-flag-not-set"); + return state.DoS( + dosLevelConstricting, + error("ContextualCheckTransaction(): overwintered flag must be set"), + REJECT_INVALID, "tx-overwintered-flag-not-set"); } // Reject transactions with non-Sapling version group ID if (tx.fOverwintered && tx.nVersionGroupId != SAPLING_VERSION_GROUP_ID) { - return state.DoS(isInitBlockDownload(chainparams) ? 0 : dosLevel, - error("CheckTransaction(): invalid Sapling tx version"), - REJECT_INVALID, "bad-sapling-tx-version-group-id"); + return state.DoS( + dosLevelPotentiallyRelaxing, + error("CheckTransaction(): invalid Sapling tx version"), + REJECT_INVALID, "bad-sapling-tx-version-group-id"); } // Reject transactions with invalid version if (tx.fOverwintered && tx.nVersion < SAPLING_MIN_TX_VERSION ) { - return state.DoS(100, error("CheckTransaction(): Sapling version too low"), + return state.DoS( + dosLevelConstricting, + error("CheckTransaction(): Sapling version too low"), REJECT_INVALID, "bad-tx-sapling-version-too-low"); } // Reject transactions with invalid version if (tx.fOverwintered && tx.nVersion > SAPLING_MAX_TX_VERSION ) { - return state.DoS(100, error("CheckTransaction(): Sapling version too high"), + return state.DoS( + dosLevelPotentiallyRelaxing, + error("CheckTransaction(): Sapling version too high"), REJECT_INVALID, "bad-tx-sapling-version-too-high"); } } else if (overwinterActive) { // Reject transactions with valid version but missing overwinter flag if (tx.nVersion >= OVERWINTER_MIN_TX_VERSION && !tx.fOverwintered) { - return state.DoS(dosLevel, error("ContextualCheckTransaction(): overwinter flag must be set"), - REJECT_INVALID, "tx-overwinter-flag-not-set"); + return state.DoS( + dosLevelConstricting, + error("ContextualCheckTransaction(): overwinter flag must be set"), + REJECT_INVALID, "tx-overwinter-flag-not-set"); } // Reject transactions with non-Overwinter version group ID if (tx.fOverwintered && tx.nVersionGroupId != OVERWINTER_VERSION_GROUP_ID) { - return state.DoS(isInitBlockDownload(chainparams) ? 0 : dosLevel, - error("CheckTransaction(): invalid Overwinter tx version"), - REJECT_INVALID, "bad-overwinter-tx-version-group-id"); + return state.DoS( + dosLevelPotentiallyRelaxing, + error("CheckTransaction(): invalid Overwinter tx version"), + REJECT_INVALID, "bad-overwinter-tx-version-group-id"); } // Reject transactions with invalid version if (tx.fOverwintered && tx.nVersion > OVERWINTER_MAX_TX_VERSION ) { - return state.DoS(100, error("CheckTransaction(): overwinter version too high"), + return state.DoS( + dosLevelPotentiallyRelaxing, + error("CheckTransaction(): overwinter version too high"), REJECT_INVALID, "bad-tx-overwinter-version-too-high"); } } @@ -834,14 +861,16 @@ bool ContextualCheckTransaction( if (overwinterActive) { // Reject transactions intended for Sprout if (!tx.fOverwintered) { - return state.DoS(dosLevel, error("ContextualCheckTransaction: overwinter is active"), - REJECT_INVALID, "tx-overwinter-active"); + return state.DoS( + dosLevelConstricting, + error("ContextualCheckTransaction: overwinter is active"), + REJECT_INVALID, "tx-overwinter-active"); } // Check that all transactions are unexpired if (IsExpiredTx(tx, nHeight)) { // Don't increase banscore if the transaction only just expired - int expiredDosLevel = IsExpiredTx(tx, nHeight - 1) ? dosLevel : 0; + int expiredDosLevel = IsExpiredTx(tx, nHeight - 1) ? dosLevelConstricting : 0; return state.DoS(expiredDosLevel, error("ContextualCheckTransaction(): transaction is expired"), REJECT_INVALID, "tx-overwinter-expired"); } } @@ -851,8 +880,10 @@ bool ContextualCheckTransaction( // Size limits BOOST_STATIC_ASSERT(MAX_BLOCK_SIZE > MAX_TX_SIZE_BEFORE_SAPLING); // sanity if (::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION) > MAX_TX_SIZE_BEFORE_SAPLING) - return state.DoS(100, error("ContextualCheckTransaction(): size limits failed"), - REJECT_INVALID, "bad-txns-oversize"); + return state.DoS( + dosLevelPotentiallyRelaxing, + error("ContextualCheckTransaction(): size limits failed"), + REJECT_INVALID, "bad-txns-oversize"); } uint256 dataToBeSigned; @@ -867,6 +898,8 @@ bool ContextualCheckTransaction( try { dataToBeSigned = SignatureHash(scriptCode, tx, NOT_AN_INPUT, SIGHASH_ALL, 0, consensusBranchId); } catch (std::logic_error ex) { + // A logic error should never occur because we pass NOT_AN_INPUT and + // SIGHASH_ALL to SignatureHash(). return state.DoS(100, error("CheckTransaction(): error computing signature hash"), REJECT_INVALID, "error-computing-signature-hash"); } @@ -882,9 +915,10 @@ bool ContextualCheckTransaction( dataToBeSigned.begin(), 32, tx.joinSplitPubKey.begin() ) != 0) { - return state.DoS(isInitBlockDownload(chainparams) ? 0 : 100, - error("CheckTransaction(): invalid joinsplit signature"), - REJECT_INVALID, "bad-txns-invalid-joinsplit-signature"); + return state.DoS( + dosLevelPotentiallyRelaxing, + error("CheckTransaction(): invalid joinsplit signature"), + REJECT_INVALID, "bad-txns-invalid-joinsplit-signature"); } } @@ -906,8 +940,10 @@ bool ContextualCheckTransaction( )) { librustzcash_sapling_verification_ctx_free(ctx); - return state.DoS(100, error("ContextualCheckTransaction(): Sapling spend description invalid"), - REJECT_INVALID, "bad-txns-sapling-spend-description-invalid"); + return state.DoS( + dosLevelPotentiallyRelaxing, + error("ContextualCheckTransaction(): Sapling spend description invalid"), + REJECT_INVALID, "bad-txns-sapling-spend-description-invalid"); } } @@ -921,6 +957,9 @@ bool ContextualCheckTransaction( )) { librustzcash_sapling_verification_ctx_free(ctx); + // This should be a non-contextual check, but we check it here + // as we need to pass over the outputs anyway in order to then + // call librustzcash_sapling_final_check(). return state.DoS(100, error("ContextualCheckTransaction(): Sapling output description invalid"), REJECT_INVALID, "bad-txns-sapling-output-description-invalid"); } @@ -934,8 +973,10 @@ bool ContextualCheckTransaction( )) { librustzcash_sapling_verification_ctx_free(ctx); - return state.DoS(100, error("ContextualCheckTransaction(): Sapling binding signature invalid"), - REJECT_INVALID, "bad-txns-sapling-binding-signature-invalid"); + return state.DoS( + dosLevelPotentiallyRelaxing, + error("ContextualCheckTransaction(): Sapling binding signature invalid"), + REJECT_INVALID, "bad-txns-sapling-binding-signature-invalid"); } librustzcash_sapling_verification_ctx_free(ctx); @@ -1249,9 +1290,8 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa if (!CheckTransaction(tx, state, verifier)) return error("AcceptToMemoryPool: CheckTransaction failed"); - // DoS level set to 10 to be more forgiving. // Check transaction contextually against the set of consensus rules which apply in the next block to be mined. - if (!ContextualCheckTransaction(tx, state, Params(), nextBlockHeight, 10)) { + if (!ContextualCheckTransaction(tx, state, Params(), nextBlockHeight, false)) { return error("AcceptToMemoryPool: ContextualCheckTransaction failed"); } @@ -3813,7 +3853,7 @@ bool ContextualCheckBlock( BOOST_FOREACH(const CTransaction& tx, block.vtx) { // Check transaction contextually against consensus rules at block height - if (!ContextualCheckTransaction(tx, state, chainparams, nHeight, 100)) { + if (!ContextualCheckTransaction(tx, state, chainparams, nHeight, true)) { return false; // Failure reason has been set in validation state object } diff --git a/src/main.h b/src/main.h index a7992c69b..8427fdf07 100644 --- a/src/main.h +++ b/src/main.h @@ -341,7 +341,7 @@ bool ContextualCheckInputs(const CTransaction& tx, CValidationState &state, cons /** Check a transaction contextually against a set of consensus rules */ bool ContextualCheckTransaction(const CTransaction& tx, CValidationState &state, - const CChainParams& chainparams, int nHeight, int dosLevel, + const CChainParams& chainparams, int nHeight, bool isMined, bool (*isInitBlockDownload)(const CChainParams&) = IsInitialBlockDownload); /** Apply the effects of this transaction on the UTXO set represented by view */ diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp index 1f1d3e069..fadbc6c29 100644 --- a/src/test/transaction_tests.cpp +++ b/src/test/transaction_tests.cpp @@ -491,7 +491,7 @@ void test_simple_joinsplit_invalidity(uint32_t consensusBranchId, CMutableTransa jsdesc->nullifiers[1] = GetRandHash(); BOOST_CHECK(CheckTransactionWithoutProofVerification(newTx, state)); - BOOST_CHECK(!ContextualCheckTransaction(newTx, state, Params(), 0, 100)); + BOOST_CHECK(!ContextualCheckTransaction(newTx, state, Params(), 0, true)); BOOST_CHECK(state.GetRejectReason() == "bad-txns-invalid-joinsplit-signature"); // Empty output script. @@ -505,7 +505,7 @@ void test_simple_joinsplit_invalidity(uint32_t consensusBranchId, CMutableTransa ) == 0); BOOST_CHECK(CheckTransactionWithoutProofVerification(newTx, state)); - BOOST_CHECK(ContextualCheckTransaction(newTx, state, Params(), 0, 100)); + BOOST_CHECK(ContextualCheckTransaction(newTx, state, Params(), 0, true)); } { // Ensure that values within the joinsplit are well-formed.