From 5ce1e649d376367a743fdbeaeb5e2fb0f76175fc Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sun, 13 Jun 2021 00:28:25 +0100 Subject: [PATCH] Throw an exception instead of asserting if Rust tx parser fails The Rust parser is stricter than the C++ parser, so we can reach errors now non-contextually that previously were thrown by the consensus rules. Various tests have been updated to check for these exceptions, as they can no longer instantiate these transactions to pass to the consensus rules. The tests use an unsafe constructor so they can still check the consensus rules. --- src/gtest/test_checkblock.cpp | 11 ++++++++- src/gtest/test_checktransaction.cpp | 37 ++++++++++++++++++----------- src/gtest/test_mempool.cpp | 14 +++++++++-- src/primitives/transaction.cpp | 21 +++++++++++----- src/test/transaction_tests.cpp | 27 +++++++++++++++++---- 5 files changed, 82 insertions(+), 28 deletions(-) diff --git a/src/gtest/test_checkblock.cpp b/src/gtest/test_checkblock.cpp index b92f9ca76..9f41d8088 100644 --- a/src/gtest/test_checkblock.cpp +++ b/src/gtest/test_checkblock.cpp @@ -36,6 +36,14 @@ TEST(CheckBlock, VersionTooLow) { } +// Subclass of CTransaction which doesn't call UpdateHash when constructing +// from a CMutableTransaction. This enables us to create a CTransaction +// with bad values which normally trigger an exception during construction. +class UNSAFE_CTransaction : public CTransaction { + public: + UNSAFE_CTransaction(const CMutableTransaction &tx) : CTransaction(tx, true) {} +}; + // Test that a Sprout tx with negative version is still rejected // by CheckBlock under Sprout consensus rules. TEST(CheckBlock, BlockSproutRejectsBadVersion) { @@ -55,7 +63,8 @@ TEST(CheckBlock, BlockSproutRejectsBadVersion) { mtx.nVersion = -1; mtx.nVersionGroupId = 0; - CTransaction tx {mtx}; + EXPECT_THROW((CTransaction(mtx)), std::ios_base::failure); + UNSAFE_CTransaction tx {mtx}; CBlock block; block.vtx.push_back(tx); diff --git a/src/gtest/test_checktransaction.cpp b/src/gtest/test_checktransaction.cpp index 6361c29b5..6e0aa854a 100644 --- a/src/gtest/test_checktransaction.cpp +++ b/src/gtest/test_checktransaction.cpp @@ -11,6 +11,14 @@ #include #include +// Subclass of CTransaction which doesn't call UpdateHash when constructing +// from a CMutableTransaction. This enables us to create a CTransaction +// with bad values which normally trigger an exception during construction. +class UNSAFE_CTransaction : public CTransaction { + public: + UNSAFE_CTransaction(const CMutableTransaction &tx) : CTransaction(tx, true) {} +}; + TEST(ChecktransactionTests, CheckVpubNotBothNonzero) { CMutableTransaction tx; tx.nVersion = 2; @@ -124,7 +132,8 @@ TEST(ChecktransactionTests, BadVersionTooLow) { CMutableTransaction mtx = GetValidTransaction(); mtx.nVersion = 0; - CTransaction tx(mtx); + EXPECT_THROW((CTransaction(mtx)), std::ios_base::failure); + UNSAFE_CTransaction tx(mtx); MockCValidationState state; EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "bad-txns-version-too-low", false)).Times(1); CheckTransactionWithoutProofVerification(tx, state); @@ -273,7 +282,8 @@ TEST(ChecktransactionTests, BadTxnsVoutNegative) { CMutableTransaction mtx = GetValidTransaction(); mtx.vout[0].nValue = -1; - CTransaction tx(mtx); + EXPECT_THROW((CTransaction(mtx)), std::ios_base::failure); + UNSAFE_CTransaction tx(mtx); MockCValidationState state; EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "bad-txns-vout-negative", false)).Times(1); @@ -284,7 +294,8 @@ TEST(ChecktransactionTests, BadTxnsVoutToolarge) { CMutableTransaction mtx = GetValidTransaction(); mtx.vout[0].nValue = MAX_MONEY + 1; - CTransaction tx(mtx); + EXPECT_THROW((CTransaction(mtx)), std::ios_base::failure); + UNSAFE_CTransaction tx(mtx); MockCValidationState state; EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "bad-txns-vout-toolarge", false)).Times(1); @@ -787,7 +798,8 @@ TEST(ChecktransactionTests, SproutTxVersionTooLow) { mtx.fOverwintered = false; mtx.nVersion = -1; - CTransaction tx(mtx); + EXPECT_THROW((CTransaction(mtx)), std::ios_base::failure); + UNSAFE_CTransaction tx(mtx); MockCValidationState state; EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "bad-txns-version-too-low", false)).Times(1); CheckTransactionWithoutProofVerification(tx, state); @@ -795,14 +807,6 @@ TEST(ChecktransactionTests, SproutTxVersionTooLow) { -// Subclass of CTransaction which doesn't call UpdateHash when constructing -// from a CMutableTransaction. This enables us to create a CTransaction -// with bad values which normally trigger an exception during construction. -class UNSAFE_CTransaction : public CTransaction { - public: - UNSAFE_CTransaction(const CMutableTransaction &tx) : CTransaction(tx, true) {} -}; - TEST(ChecktransactionTests, SaplingSproutInputSumsTooLarge) { CMutableTransaction mtx = GetValidTransaction(); mtx.vJoinSplit.resize(0); @@ -866,6 +870,7 @@ TEST(ChecktransactionTests, OverwinterVersionNumberLow) { mtx.nVersionGroupId = OVERWINTER_VERSION_GROUP_ID; mtx.nExpiryHeight = 0; + EXPECT_THROW((CTransaction(mtx)), std::ios_base::failure); UNSAFE_CTransaction tx(mtx); MockCValidationState state; EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "bad-tx-overwinter-version-too-low", false)).Times(1); @@ -884,6 +889,7 @@ TEST(ChecktransactionTests, OverwinterVersionNumberHigh) { mtx.nVersionGroupId = OVERWINTER_VERSION_GROUP_ID; mtx.nExpiryHeight = 0; + EXPECT_THROW((CTransaction(mtx)), std::ios_base::failure); UNSAFE_CTransaction tx(mtx); MockCValidationState state; EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "bad-tx-overwinter-version-too-high", false)).Times(1); @@ -903,6 +909,7 @@ TEST(ChecktransactionTests, OverwinterBadVersionGroupId) { mtx.nExpiryHeight = 0; mtx.nVersionGroupId = 0x12345678; + EXPECT_THROW((CTransaction(mtx)), std::ios_base::failure); UNSAFE_CTransaction tx(mtx); MockCValidationState state; EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "bad-tx-version-group-id", false)).Times(1); @@ -1250,7 +1257,8 @@ TEST(ChecktransactionTests, HeartwoodEnforcesSaplingRulesOnShieldedCoinbase) { // out of range. { mtx.valueBalance = MAX_MONEY + 1; - CTransaction tx(mtx); + EXPECT_THROW((CTransaction(mtx)), std::ios_base::failure); + UNSAFE_CTransaction tx(mtx); EXPECT_TRUE(tx.IsCoinBase()); MockCValidationState state; @@ -1259,7 +1267,8 @@ TEST(ChecktransactionTests, HeartwoodEnforcesSaplingRulesOnShieldedCoinbase) { } { mtx.valueBalance = -MAX_MONEY - 1; - CTransaction tx(mtx); + EXPECT_THROW((CTransaction(mtx)), std::ios_base::failure); + UNSAFE_CTransaction tx(mtx); EXPECT_TRUE(tx.IsCoinBase()); MockCValidationState state; diff --git a/src/gtest/test_mempool.cpp b/src/gtest/test_mempool.cpp index 2b8ad9763..ced90a2f6 100644 --- a/src/gtest/test_mempool.cpp +++ b/src/gtest/test_mempool.cpp @@ -175,6 +175,14 @@ TEST(Mempool, SproutV3TxWhenOverwinterActive) { } +// Subclass of CTransaction which doesn't call UpdateHash when constructing +// from a CMutableTransaction. This enables us to create a CTransaction +// with bad values which normally trigger an exception during construction. +class UNSAFE_CTransaction : public CTransaction { + public: + UNSAFE_CTransaction(const CMutableTransaction &tx) : CTransaction(tx, true) {} +}; + // Sprout transaction with negative version, rejected by the mempool in CheckTransaction // under Sprout consensus rules, should still be rejected under Overwinter consensus rules. // 1. fails CheckTransaction (specifically CheckTransactionWithoutProofVerification) @@ -198,7 +206,8 @@ TEST(Mempool, SproutNegativeVersionTxWhenOverwinterActive) { mtx.nVersion = -3; EXPECT_EQ(mtx.nVersion, static_cast(0xfffffffd)); - CTransaction tx1(mtx); + EXPECT_THROW((CTransaction(mtx)), std::ios_base::failure); + UNSAFE_CTransaction tx1(mtx); EXPECT_EQ(tx1.nVersion, -3); CValidationState state1; @@ -215,7 +224,8 @@ TEST(Mempool, SproutNegativeVersionTxWhenOverwinterActive) { mtx.nVersion = static_cast((1 << 31) | 3); EXPECT_EQ(mtx.nVersion, static_cast(0x80000003)); - CTransaction tx1(mtx); + EXPECT_THROW((CTransaction(mtx)), std::ios_base::failure); + UNSAFE_CTransaction tx1(mtx); EXPECT_EQ(tx1.nVersion, -2147483645); CValidationState state1; diff --git a/src/primitives/transaction.cpp b/src/primitives/transaction.cpp index e5d5ee4ff..ec6e9a759 100644 --- a/src/primitives/transaction.cpp +++ b/src/primitives/transaction.cpp @@ -143,11 +143,14 @@ uint256 CMutableTransaction::GetHash() const CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); ss << *this; uint256 hash; - assert(zcash_transaction_digests( + if (!zcash_transaction_digests( reinterpret_cast(ss.data()), ss.size(), hash.begin(), - nullptr)); + nullptr)) + { + throw std::ios_base::failure("CMutableTransaction::GetHash: Invalid transaction format"); + } return hash; } @@ -156,11 +159,14 @@ uint256 CMutableTransaction::GetAuthDigest() const CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); ss << *this; uint256 authDigest; - assert(zcash_transaction_digests( + if (!zcash_transaction_digests( reinterpret_cast(ss.data()), ss.size(), nullptr, - authDigest.begin())); + authDigest.begin())) + { + throw std::ios_base::failure("CMutableTransaction::GetAuthDigest: Invalid transaction format"); + } return authDigest; } @@ -168,11 +174,14 @@ void CTransaction::UpdateHash() const { CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); ss << *this; - assert(zcash_transaction_digests( + if (!zcash_transaction_digests( reinterpret_cast(ss.data()), ss.size(), const_cast(&hash)->begin(), - const_cast(&authDigest)->begin())); + const_cast(&authDigest)->begin())) + { + throw std::ios_base::failure("CTransaction::UpdateHash: Invalid transaction format"); + } } CTransaction::CTransaction() : nVersion(CTransaction::SPROUT_MIN_CURRENT_VERSION), diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp index 744ae94ae..8451fd31c 100644 --- a/src/test/transaction_tests.cpp +++ b/src/test/transaction_tests.cpp @@ -43,6 +43,14 @@ using namespace std; +// Subclass of CTransaction which doesn't call UpdateHash when constructing +// from a CMutableTransaction. This enables us to create a CTransaction +// with bad values which normally trigger an exception during construction. +class UNSAFE_CTransaction : public CTransaction { + public: + UNSAFE_CTransaction(const CMutableTransaction &tx) : CTransaction(tx, true) {} +}; + BOOST_FIXTURE_TEST_SUITE(transaction_tests, JoinSplitTestingSetup) BOOST_AUTO_TEST_CASE(tx_valid) @@ -188,7 +196,12 @@ BOOST_AUTO_TEST_CASE(tx_invalid) string transaction = test[1].get_str(); CDataStream stream(ParseHex(transaction), SER_NETWORK, PROTOCOL_VERSION); CTransaction tx; - stream >> tx; + try { + stream >> tx; + } catch (std::ios_base::failure) { + // Invalid transaction was caught at parse time by the Rust logic. + continue; + } CValidationState state; fValid = CheckTransaction(tx, state, verifier) && state.IsValid(); @@ -463,23 +476,27 @@ void test_simple_joinsplit_invalidity(uint32_t consensusBranchId, CMutableTransa JSDescription *jsdesc = &newTx.vJoinSplit[0]; jsdesc->vpub_old = -1; - BOOST_CHECK(!CheckTransaction(newTx, state, verifier)); + BOOST_CHECK_THROW((CTransaction(newTx)), std::ios_base::failure); + BOOST_CHECK(!CheckTransaction(UNSAFE_CTransaction(newTx), state, verifier)); BOOST_CHECK(state.GetRejectReason() == "bad-txns-vpub_old-negative"); jsdesc->vpub_old = MAX_MONEY + 1; - BOOST_CHECK(!CheckTransaction(newTx, state, verifier)); + BOOST_CHECK_THROW((CTransaction(newTx)), std::ios_base::failure); + BOOST_CHECK(!CheckTransaction(UNSAFE_CTransaction(newTx), state, verifier)); BOOST_CHECK(state.GetRejectReason() == "bad-txns-vpub_old-toolarge"); jsdesc->vpub_old = 0; jsdesc->vpub_new = -1; - BOOST_CHECK(!CheckTransaction(newTx, state, verifier)); + BOOST_CHECK_THROW((CTransaction(newTx)), std::ios_base::failure); + BOOST_CHECK(!CheckTransaction(UNSAFE_CTransaction(newTx), state, verifier)); BOOST_CHECK(state.GetRejectReason() == "bad-txns-vpub_new-negative"); jsdesc->vpub_new = MAX_MONEY + 1; - BOOST_CHECK(!CheckTransaction(newTx, state, verifier)); + BOOST_CHECK_THROW((CTransaction(newTx)), std::ios_base::failure); + BOOST_CHECK(!CheckTransaction(UNSAFE_CTransaction(newTx), state, verifier)); BOOST_CHECK(state.GetRejectReason() == "bad-txns-vpub_new-toolarge"); jsdesc->vpub_new = (MAX_MONEY / 2) + 10;