Merge pull request #5283 from str4d/fix-v5-tx-format

Fix v5 transaction format

qa/rpc-tests/mempool_nu_activation.py

@@ -4,6 +4,7 @@
 # file COPYING or https://www.opensource.org/licenses/mit-license.php .
 
 from test_framework.test_framework import BitcoinTestFramework
+from test_framework.mininode import NU5_PROTO_VERSION
 from test_framework.util import (
     assert_equal, assert_true,
     start_node, connect_nodes, wait_and_assert_operationid_status,
@@ -25,6 +26,9 @@ class MempoolUpgradeActivationTest(BitcoinTestFramework):
     def setup_network(self):
         args = ["-checkmempool", "-debug=mempool", "-blockmaxsize=4000",
             "-nuparams=2bb40e60:200", # Blossom
+            "-nuparams=f5b9230b:210", # Heartwood
+            "-nuparams=e9ff75a6:220", # Canopy
+            "-nuparams=f919a198:230", # NU5
         ]
         self.nodes = []
         self.nodes.append(start_node(0, self.options.tmpdir, args))
@@ -172,10 +176,28 @@ class MempoolUpgradeActivationTest(BitcoinTestFramework):
             self.nodes[1].generate(6)
             self.sync_all()
 
+        net_version = self.nodes[0].getnetworkinfo()["protocolversion"]
+
         print('Testing Sapling -> Blossom activation boundary')
         # Current height = 195
         nu_activation_checks()
         # Current height = 205
 
+        print('Testing Blossom -> Heartwood activation boundary')
+        nu_activation_checks()
+        # Current height = 215
+
+        print('Testing Heartwood -> Canopy activation boundary')
+        nu_activation_checks()
+        # Current height = 225
+
+        if net_version < NU5_PROTO_VERSION:
+            print("Node's block index is not NU5-aware, skipping remaining tests")
+            return
+
+        print('Testing Canopy -> NU5 activation boundary')
+        nu_activation_checks()
+        # Current height = 235
+
 if __name__ == '__main__':
     MempoolUpgradeActivationTest().main()

qa/rpc-tests/remove_sprout_shielding.py

@@ -8,9 +8,14 @@ from test_framework.authproxy import JSONRPCException
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
+    assert_raises_message,
     start_nodes, get_coinbase_address,
     wait_and_assert_operationid_status,
-    nuparams, BLOSSOM_BRANCH_ID, HEARTWOOD_BRANCH_ID, CANOPY_BRANCH_ID
+    nuparams,
+    BLOSSOM_BRANCH_ID,
+    HEARTWOOD_BRANCH_ID,
+    CANOPY_BRANCH_ID,
+    NU5_BRANCH_ID,
 )
 
 import logging
@@ -19,6 +24,7 @@ HAS_CANOPY = ['-nurejectoldversions=false',
     nuparams(BLOSSOM_BRANCH_ID, 205),
     nuparams(HEARTWOOD_BRANCH_ID, 210),
     nuparams(CANOPY_BRANCH_ID, 220),
+    nuparams(NU5_BRANCH_ID, 225),
 ]
 
 class RemoveSproutShieldingTest (BitcoinTestFramework):
@@ -73,32 +79,29 @@ class RemoveSproutShieldingTest (BitcoinTestFramework):
         self.sync_all()
 
         # Shield coinbase to Sprout on node 0. Should fail
-        errorString = ''
+        sprout_addr = self.nodes[0].z_getnewaddress('sprout')
-        try:
+        assert_raises_message(
-            sprout_addr = self.nodes[0].z_getnewaddress('sprout')
+            JSONRPCException,
-            self.nodes[0].z_shieldcoinbase(get_coinbase_address(self.nodes[0]), sprout_addr, 0)
+            "Sprout shielding is not supported after Canopy",
-        except JSONRPCException as e:
+            self.nodes[0].z_shieldcoinbase,
-            errorString = e.error['message']
+            get_coinbase_address(self.nodes[0]), sprout_addr, 0)
-        assert("Sprout shielding is not supported after Canopy" in errorString)
         print("taddr -> Sprout z_shieldcoinbase tx rejected at Canopy activation on node 0")
 
         # Create taddr -> Sprout z_sendmany transaction on node 0. Should fail
-        errorString = ''
+        sprout_addr = self.nodes[1].z_getnewaddress('sprout')
-        try:
+        assert_raises_message(
-            sprout_addr = self.nodes[1].z_getnewaddress('sprout')
+            JSONRPCException,
-            self.nodes[0].z_sendmany(taddr_0, [{"address": sprout_addr, "amount": 1}])
+            "Sprout shielding is not supported after Canopy",
-        except JSONRPCException as e:
+            self.nodes[0].z_sendmany,
-            errorString = e.error['message']
+            taddr_0, [{"address": sprout_addr, "amount": 1}])
-        assert("Sprout shielding is not supported after Canopy" in errorString)
         print("taddr -> Sprout z_sendmany tx rejected at Canopy activation on node 0")
 
         # Create z_mergetoaddress [taddr, Sprout] -> Sprout transaction on node 0. Should fail
-        errorString = ''
+        assert_raises_message(
-        try:
+            JSONRPCException,
-            self.nodes[0].z_mergetoaddress(["ANY_TADDR", "ANY_SPROUT"], self.nodes[1].z_getnewaddress('sprout'))
+            "Sprout shielding is not supported after Canopy",
-        except JSONRPCException as e:
+            self.nodes[0].z_mergetoaddress,
-            errorString = e.error['message']
+            ["ANY_TADDR", "ANY_SPROUT"], self.nodes[1].z_getnewaddress('sprout'))
-        assert("Sprout shielding is not supported after Canopy" in errorString)
         print("[taddr, Sprout] -> Sprout z_mergetoaddress tx rejected at Canopy activation on node 0")
 
         # Create z_mergetoaddress Sprout -> Sprout transaction on node 0. Should pass
@@ -115,5 +118,17 @@ class RemoveSproutShieldingTest (BitcoinTestFramework):
         wait_and_assert_operationid_status(self.nodes[0], myopid)
         print("taddr -> Sapling z_shieldcoinbase tx accepted after Canopy on node 0")
 
+        # Mine to one block before NU5 activation.
+        self.nodes[0].generate(4)
+        self.sync_all()
+
+        # Create z_mergetoaddress Sprout -> Sprout transaction on node 1. Should pass
+        merge_tx_2 = self.nodes[1].z_mergetoaddress(["ANY_SPROUT"], self.nodes[2].z_getnewaddress('sprout'))
+        wait_and_assert_operationid_status(self.nodes[1], merge_tx_2['opid'])
+        print("Sprout -> Sprout z_mergetoaddress tx accepted at NU5 activation on node 1")
+
+        self.nodes[1].generate(1)
+        self.sync_all()
+
 if __name__ == '__main__':
     RemoveSproutShieldingTest().main()

qa/rpc-tests/test_framework/mininode.py

@@ -51,6 +51,7 @@ SPROUT_PROTO_VERSION = 170002  # past bip-31 for ping/pong
 OVERWINTER_PROTO_VERSION = 170003
 SAPLING_PROTO_VERSION = 170006
 BLOSSOM_PROTO_VERSION = 170008
+NU5_PROTO_VERSION = 170014
 
 MY_SUBVERSION = b"/python-mininode-tester:0.0.3/"
 

src/main.cpp

@@ -1102,11 +1102,24 @@ bool ContextualCheckTransaction(
                     REJECT_INVALID, "bad-tx-zip225-version-too-high");
             }
 
-            // tx.nConsensusBranchId must match the current consensus branch id
+            if (!tx.GetConsensusBranchId().has_value()) {
-            if (!(tx.GetConsensusBranchId() && *tx.GetConsensusBranchId() == consensusBranchId)) {
+                // NOTE: This is an internal zcashd consistency
+                // check; it does not correspond to a consensus rule in the
+                // protocol specification, but is instead an artifact of the
+                // internal zcashd transaction representation.
                 return state.DoS(
                     dosLevelPotentiallyRelaxing,
-                    error("ContextualCheckTransaction(): transaction's consensus branch id does not match the current consensus branch"),
+                    error("ContextualCheckTransaction(): transaction does not have consensus branch id field set"),
+                    REJECT_INVALID, "bad-tx-consensus-branch-id-missing");
+            }
+
+            // tx.nConsensusBranchId must match the current consensus branch id
+            if (tx.GetConsensusBranchId().value() != consensusBranchId) {
+                return state.DoS(
+                    dosLevelPotentiallyRelaxing,
+                    error(
+                        "ContextualCheckTransaction(): transaction's consensus branch id (%08x) does not match the current consensus branch (%08x)",
+                        tx.GetConsensusBranchId().value(), consensusBranchId),
                     REJECT_INVALID, "bad-tx-consensus-branch-id-mismatch");
             }
 
@@ -7631,16 +7644,23 @@ public:
 
 
 // Set default values of new CMutableTransaction based on consensus rules at given height.
-CMutableTransaction CreateNewContextualCMutableTransaction(const Consensus::Params& consensusParams, int nHeight)
+CMutableTransaction CreateNewContextualCMutableTransaction(
+    const Consensus::Params& consensusParams,
+    int nHeight,
+    bool requireSprout)
 {
     CMutableTransaction mtx;
 
-    auto txVersionInfo = CurrentTxVersionInfo(consensusParams, nHeight);
+    auto txVersionInfo = CurrentTxVersionInfo(consensusParams, nHeight, requireSprout);
     mtx.fOverwintered   = txVersionInfo.fOverwintered;
     mtx.nVersionGroupId = txVersionInfo.nVersionGroupId;
     mtx.nVersion        = txVersionInfo.nVersion;
 
     if (mtx.fOverwintered) {
+        if (mtx.nVersion >= ZIP225_TX_VERSION) {
+            mtx.nConsensusBranchId = CurrentEpochBranchId(nHeight, consensusParams);
+        }
+
         bool blossomActive = consensusParams.NetworkUpgradeActive(nHeight, Consensus::UPGRADE_BLOSSOM);
         unsigned int defaultExpiryDelta = blossomActive ? DEFAULT_POST_BLOSSOM_TX_EXPIRY_DELTA : DEFAULT_PRE_BLOSSOM_TX_EXPIRY_DELTA;
         mtx.nExpiryHeight = nHeight + (expiryDeltaArg ? expiryDeltaArg.value() : defaultExpiryDelta);

src/main.h

@@ -567,7 +567,10 @@ uint64_t CalculateCurrentUsage();
  * Return a CMutableTransaction with contextual default values based on set of consensus rules at nHeight. The expiryDelta will
  * either be based on the command-line argument '-txexpirydelta' or derived from consensusParams.
  */
-CMutableTransaction CreateNewContextualCMutableTransaction(const Consensus::Params& consensusParams, int nHeight);
+CMutableTransaction CreateNewContextualCMutableTransaction(
+    const Consensus::Params& consensusParams,
+    int nHeight,
+    bool requireSprout = false);
 
 std::pair<std::map<CBlockIndex*, std::list<CTransaction>>, uint64_t> DrainRecentlyConflicted();
 void SetChainNotifiedSequence(const CChainParams& chainparams, uint64_t recentlyConflictedSequence);

src/primitives/transaction.cpp

@@ -402,7 +402,8 @@ std::string CTransaction::ToString() const
             vShieldedSpend.size(),
             vShieldedOutput.size());
         if (nVersion >= ZIP225_MIN_TX_VERSION) {
-            str += strprintf(", valueBalanceOrchard=%u, vOrchardAction.size=%u",
+            str += strprintf(", nConsensusBranchId=%08x, valueBalanceOrchard=%u, vOrchardAction.size=%u",
+                nConsensusBranchId.value_or(0),
                 orchardBundle.GetValueBalance(),
                 orchardBundle.GetNumActions());
         }
@@ -429,13 +430,23 @@ std::string CTransaction::ToString() const
  * Returns the most recent supported transaction version and version group id,
  * as of the specified activation height and active features.
  */
-TxVersionInfo CurrentTxVersionInfo(const Consensus::Params& consensus, int nHeight) {
+TxVersionInfo CurrentTxVersionInfo(
+    const Consensus::Params& consensus,
+    int nHeight,
+    bool requireSprout)
+{
     if (consensus.NetworkUpgradeActive(nHeight, Consensus::UPGRADE_ZFUTURE)) {
         return {
             .fOverwintered =   true,
             .nVersionGroupId = ZFUTURE_VERSION_GROUP_ID,
             .nVersion =        ZFUTURE_TX_VERSION
         };
+    } else if (consensus.NetworkUpgradeActive(nHeight, Consensus::UPGRADE_NU5) && !requireSprout) {
+        return {
+            .fOverwintered =   true,
+            .nVersionGroupId = ZIP225_VERSION_GROUP_ID,
+            .nVersion =        ZIP225_TX_VERSION
+        };
     } else if (consensus.NetworkUpgradeActive(nHeight, Consensus::UPGRADE_SAPLING)) {
         return {
             .fOverwintered =   true,

src/primitives/transaction.h

@@ -77,7 +77,8 @@ struct TxVersionInfo {
  * Returns the current transaction version and version group id,
  * based upon the specified activation height and active features.
  */
-TxVersionInfo CurrentTxVersionInfo(const Consensus::Params& consensus, int nHeight);
+TxVersionInfo CurrentTxVersionInfo(
+    const Consensus::Params& consensus, int nHeight, bool requireSprout);
 
 struct TxParams {
     unsigned int expiryDelta;
@@ -781,7 +782,14 @@ public:
 
         if (isZip225V5) {
             // Common Transaction Fields (plus version bytes above)
-            READWRITE(*nConsensusBranchId);
+            if (ser_action.ForRead()) {
+                uint32_t consensusBranchId;
+                READWRITE(consensusBranchId);
+                *const_cast<std::optional<uint32_t>*>(&nConsensusBranchId) = consensusBranchId;
+            } else {
+                uint32_t consensusBranchId = nConsensusBranchId.value();
+                READWRITE(consensusBranchId);
+            }
             READWRITE(*const_cast<uint32_t*>(&nLockTime));
             READWRITE(*const_cast<uint32_t*>(&nExpiryHeight));
 
@@ -1001,7 +1009,14 @@ struct CMutableTransaction
 
         if (isZip225V5) {
             // Common Transaction Fields (plus version bytes above)
-            READWRITE(*nConsensusBranchId);
+            if (ser_action.ForRead()) {
+                uint32_t consensusBranchId;
+                READWRITE(consensusBranchId);
+                nConsensusBranchId = consensusBranchId;
+            } else {
+                uint32_t consensusBranchId = nConsensusBranchId.value();
+                READWRITE(consensusBranchId);
+            }
             READWRITE(nLockTime);
             READWRITE(nExpiryHeight);
 

src/transaction_builder.cpp

@@ -153,6 +153,7 @@ TransactionBuilder::TransactionBuilder(
     CKeyStore* keystore,
     CCoinsViewCache* coinsView,
     CCriticalSection* cs_coinsView) :
+    usingSprout(std::nullopt),
     consensusParams(consensusParams),
     nHeight(nHeight),
     keystore(keystore),
@@ -229,6 +230,8 @@ void TransactionBuilder::AddSproutInput(
     libzcash::SproutNote note,
     SproutWitness witness)
 {
+    CheckOrSetUsingSprout();
+
     // Consistency check: all anchors must equal the first one
     if (!jsInputs.empty()) {
         if (jsInputs[0].witness.root() != witness.root()) {
@@ -244,6 +247,8 @@ void TransactionBuilder::AddSproutOutput(
     CAmount value,
     std::array<unsigned char, ZC_MEMO_SIZE> memo)
 {
+    CheckOrSetUsingSprout();
+
     libzcash::JSOutput jsOutput(to, value);
     jsOutput.memo = memo;
     jsOutputs.push_back(jsOutput);
@@ -504,6 +509,22 @@ TransactionBuilderResult TransactionBuilder::Build()
     return TransactionBuilderResult(CTransaction(mtx));
 }
 
+void TransactionBuilder::CheckOrSetUsingSprout()
+{
+    if (usingSprout.has_value()) {
+        if (!usingSprout.value()) {
+            throw JSONRPCError(RPC_WALLET_ERROR, "Can't use Sprout with a v5 transaction.");
+        }
+    } else {
+        usingSprout = true;
+
+        // Switch if necessary to a Sprout-supporting transaction format.
+        auto txVersionInfo = CurrentTxVersionInfo(consensusParams, nHeight, usingSprout.value());
+        mtx.nVersionGroupId = txVersionInfo.nVersionGroupId;
+        mtx.nVersion        = txVersionInfo.nVersion;
+    }
+}
+
 void TransactionBuilder::CreateJSDescriptions()
 {
     // Copy jsInputs and jsOutputs to more flexible containers

src/transaction_builder.h

@@ -107,6 +107,7 @@ public:
 class TransactionBuilder
 {
 private:
+    std::optional<bool> usingSprout;
     Consensus::Params consensusParams;
     int nHeight;
     const CKeyStore* keystore;
@@ -178,6 +179,8 @@ public:
     TransactionBuilderResult Build();
 
 private:
+    void CheckOrSetUsingSprout();
+
     void CreateJSDescriptions();
 
     void CreateJSDescription(

src/wallet/rpcwallet.cpp

@@ -3673,7 +3673,8 @@ UniValue z_sendmany(const UniValue& params, bool fHelp)
 
     // Contextual transaction we will build on
     // (used if no Sapling addresses are involved)
-    CMutableTransaction contextualTx = CreateNewContextualCMutableTransaction(Params().GetConsensus(), nextBlockHeight);
+    CMutableTransaction contextualTx = CreateNewContextualCMutableTransaction(
+        Params().GetConsensus(), nextBlockHeight, !noSproutAddrs);
     bool isShielded = !fromTaddr || zaddrRecipients.size() > 0;
     if (contextualTx.nVersion == 1 && isShielded) {
         contextualTx.nVersion = 2; // Tx format should support vJoinSplits
@@ -4442,11 +4443,12 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp)
         }
     }
 
+    bool isSproutShielded = sproutNoteInputs.size() > 0 || isToSproutZaddr;
     // Contextual transaction we will build on
     CMutableTransaction contextualTx = CreateNewContextualCMutableTransaction(
         Params().GetConsensus(),
-        nextBlockHeight);
+        nextBlockHeight,
-    bool isSproutShielded = sproutNoteInputs.size() > 0 || isToSproutZaddr;
+        isSproutShielded);
     if (contextualTx.nVersion == 1 && isSproutShielded) {
         contextualTx.nVersion = 2; // Tx format should support vJoinSplit
     }