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

@@ -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);
 

src/gtest/test_checktransaction.cpp

@@ -11,6 +11,14 @@
 #include <librustzcash.h>
 #include <rust/ed25519.h>
 
+// 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;

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<int32_t>(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<int32_t>((1 << 31) | 3);
         EXPECT_EQ(mtx.nVersion, static_cast<int32_t>(0x80000003));
 
-        CTransaction tx1(mtx);
+        EXPECT_THROW((CTransaction(mtx)), std::ios_base::failure);
+        UNSAFE_CTransaction tx1(mtx);
         EXPECT_EQ(tx1.nVersion, -2147483645);
 
         CValidationState state1;

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<const unsigned char*>(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<const unsigned char*>(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<const unsigned char*>(ss.data()),
         ss.size(),
         const_cast<uint256*>(&hash)->begin(),
-        const_cast<uint256*>(&authDigest)->begin()));
+        const_cast<uint256*>(&authDigest)->begin()))
+    {
+        throw std::ios_base::failure("CTransaction::UpdateHash: Invalid transaction format");
+    }
 }
 
 CTransaction::CTransaction() : nVersion(CTransaction::SPROUT_MIN_CURRENT_VERSION),

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;
+            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;