Merge bitcoin/bitcoin#25858: psbt: Only include PSBT_OUT_TAP_TREE when the output has a script path

9e386af tests: Test that PSBT_OUT_TAP_TREE is included correctly (Andrew Chow)
30ff25c psbt: Only include m_tap_tree if it has scripts (Andrew Chow)
0577d42 psbt: Change m_tap_tree to store just the tuples (Andrew Chow)
22c051c tests: Test that PSBT_OUT_TAP_TREE is combined correctly (Andrew Chow)
7df6e1b psbt: Fix merging of m_tap_tree (Andrew Chow)
0652dc5 [BugFix]: Do not allow deserializing PSBT with empty PSBT_OUT_TAP_TREE (Jeremy Rubin)

Pull request description:

  PSBT_OUT_TAP_TREE should not be included for outputs that do not have such a tree. This should be disallowed during parsing, as well as prior to serialization when the field is populated during updating.

  Also added some test cases.

  Alternative to #25856

ACKs for top commit:
  instagibbs:
    ACK bitcoin/bitcoin@9e386af
  darosior:
    ACK 9e386af

Tree-SHA512: ce5c02a69752d176dbd967c1e8d30129b1905c8f186aeeef034576c1de82059271a1ee846bd040f5be4e66bb77ba711dcf14ac1e597c5707d7e7e2293f6cfefb

Author: glozow

src/psbt.cpp

@@ -218,8 +218,14 @@ void PSBTOutput::FillSignatureData(SignatureData& sigdata) const
     for (const auto& key_pair : hd_keypaths) {
         sigdata.misc_pubkeys.emplace(key_pair.first.GetID(), key_pair);
     }
-    if (m_tap_tree.has_value() && m_tap_internal_key.IsFullyValid()) {
-        TaprootSpendData spenddata = m_tap_tree->GetSpendData();
+    if (!m_tap_tree.empty() && m_tap_internal_key.IsFullyValid()) {
+        TaprootBuilder builder;
+        for (const auto& [depth, leaf_ver, script] : m_tap_tree) {
+            builder.Add((int)depth, script, (int)leaf_ver, /*track=*/true);
+        }
+        assert(builder.IsComplete());
+        builder.Finalize(m_tap_internal_key);
+        TaprootSpendData spenddata = builder.GetSpendData();
 
         sigdata.tr_spenddata.internal_key = m_tap_internal_key;
         sigdata.tr_spenddata.Merge(spenddata);
@@ -243,8 +249,8 @@ void PSBTOutput::FromSignatureData(const SignatureData& sigdata)
     if (!sigdata.tr_spenddata.internal_key.IsNull()) {
         m_tap_internal_key = sigdata.tr_spenddata.internal_key;
     }
-    if (sigdata.tr_builder.has_value()) {
-        m_tap_tree = sigdata.tr_builder;
+    if (sigdata.tr_builder.has_value() && sigdata.tr_builder->HasScripts()) {
+        m_tap_tree = sigdata.tr_builder->GetTreeTuples();
     }
     for (const auto& [pubkey, leaf_origin] : sigdata.taproot_misc_pubkeys) {
         m_tap_bip32_paths.emplace(pubkey, leaf_origin);
@@ -265,7 +271,7 @@ void PSBTOutput::Merge(const PSBTOutput& output)
     if (redeem_script.empty() && !output.redeem_script.empty()) redeem_script = output.redeem_script;
     if (witness_script.empty() && !output.witness_script.empty()) witness_script = output.witness_script;
     if (m_tap_internal_key.IsNull() && !output.m_tap_internal_key.IsNull()) m_tap_internal_key = output.m_tap_internal_key;
-    if (m_tap_tree.has_value() && !output.m_tap_tree.has_value()) m_tap_tree = output.m_tap_tree;
+    if (m_tap_tree.empty() && !output.m_tap_tree.empty()) m_tap_tree = output.m_tap_tree;
 }
 bool PSBTInputSigned(const PSBTInput& input)
 {

src/psbt.h

@@ -713,7 +713,7 @@ struct PSBTOutput
     CScript witness_script;
     std::map<CPubKey, KeyOriginInfo> hd_keypaths;
     XOnlyPubKey m_tap_internal_key;
-    std::optional<TaprootBuilder> m_tap_tree;
+    std::vector<std::tuple<uint8_t, uint8_t, CScript>> m_tap_tree;
     std::map<XOnlyPubKey, std::pair<std::set<uint256>, KeyOriginInfo>> m_tap_bip32_paths;
     std::map<std::vector<unsigned char>, std::vector<unsigned char>> unknown;
     std::set<PSBTProprietary> m_proprietary;
@@ -754,15 +754,11 @@ struct PSBTOutput
         }
 
         // Write taproot tree
-        if (m_tap_tree.has_value()) {
+        if (!m_tap_tree.empty()) {
             SerializeToVector(s, PSBT_OUT_TAP_TREE);
             std::vector<unsigned char> value;
             CVectorWriter s_value(s.GetType(), s.GetVersion(), value, 0);
-            const auto& tuples = m_tap_tree->GetTreeTuples();
-            for (const auto& tuple : tuples) {
-                uint8_t depth = std::get<0>(tuple);
-                uint8_t leaf_ver = std::get<1>(tuple);
-                CScript script = std::get<2>(tuple);
+            for (const auto& [depth, leaf_ver, script] : m_tap_tree) {
                 s_value << depth;
                 s_value << leaf_ver;
                 s_value << script;
@@ -858,10 +854,13 @@ struct PSBTOutput
                     } else if (key.size() != 1) {
                         throw std::ios_base::failure("Output Taproot tree key is more than one byte type");
                     }
-                    m_tap_tree.emplace();
                     std::vector<unsigned char> tree_v;
                     s >> tree_v;
                     SpanReader s_tree(s.GetType(), s.GetVersion(), tree_v);
+                    if (s_tree.empty()) {
+                        throw std::ios_base::failure("Output Taproot tree must not be empty");
+                    }
+                    TaprootBuilder builder;
                     while (!s_tree.empty()) {
                         uint8_t depth;
                         uint8_t leaf_ver;
@@ -875,9 +874,10 @@ struct PSBTOutput
                         if ((leaf_ver & ~TAPROOT_LEAF_MASK) != 0) {
                             throw std::ios_base::failure("Output Taproot tree has a leaf with an invalid leaf version");
                         }
-                        m_tap_tree->Add((int)depth, script, (int)leaf_ver, true /* track */);
+                        m_tap_tree.push_back(std::make_tuple(depth, leaf_ver, script));
+                        builder.Add((int)depth, script, (int)leaf_ver, true /* track */);
                     }
-                    if (!m_tap_tree->IsComplete()) {
+                    if (!builder.IsComplete()) {
                         throw std::ios_base::failure("Output Taproot tree is malformed");
                     }
                     break;
@@ -931,11 +931,6 @@ struct PSBTOutput
             }
         }
 
-        // Finalize m_tap_tree so that all of the computed things are computed
-        if (m_tap_tree.has_value() && m_tap_tree->IsComplete() && m_tap_internal_key.IsFullyValid()) {
-            m_tap_tree->Finalize(m_tap_internal_key);
-        }
-
         if (!found_sep) {
             throw std::ios_base::failure("Separator is missing at the end of an output map");
         }

src/rpc/rawtransaction.cpp

@@ -1241,13 +1241,9 @@ static RPCHelpMan decodepsbt()
         }
 
         // Taproot tree
-        if (output.m_tap_tree.has_value()) {
+        if (!output.m_tap_tree.empty()) {
             UniValue tree(UniValue::VARR);
-            const auto& tuples = output.m_tap_tree->GetTreeTuples();
-            for (const auto& tuple : tuples) {
-                uint8_t depth = std::get<0>(tuple);
-                uint8_t leaf_ver = std::get<1>(tuple);
-                CScript script = std::get<2>(tuple);
+            for (const auto& [depth, leaf_ver, script] : output.m_tap_tree) {
                 UniValue elem(UniValue::VOBJ);
                 elem.pushKV("depth", (int)depth);
                 elem.pushKV("leaf_ver", (int)leaf_ver);

src/script/standard.h

@@ -315,6 +315,8 @@ class TaprootBuilder
     TaprootSpendData GetSpendData() const;
     /** Returns a vector of tuples representing the depth, leaf version, and script */
     std::vector<std::tuple<uint8_t, uint8_t, CScript>> GetTreeTuples() const;
+    /** Returns true if there are any tapscripts */
+    bool HasScripts() const { return !m_branch.empty(); }
 };
 
 /** Given a TaprootSpendData and the output key, reconstruct its script tree.

test/functional/data/rpc_psbt.json

@@ -44,6 +44,10 @@
         [
             "cHNidP8BAKOro2MDAwMDA5ggCAAA////CQAtAAD+///1AAAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAJAAAAAAAAAAAAAAAAAAAAAAAAAD+///1Zm9ybmV3nWx1Y2vmelLmegAAAAAAAAAAAAAAAAAAAAMKAwMDAwMDAwMDAwMACvMBA3FkAAAAAAAAAAAABAAlAAAAAAAAACEWDQ0zDQ0NDQ0NDQ0NCwEAAH9/f39/fwMAAABNo6P///kAAA==",
             "Input Taproot BIP32 keypath has an invalid length"
+        ],
+        [
+            "cHNidP8BAIkCAAAAAapfm08b0MipBvW9thL06f8rMbeazW7TIa0W9plHj4WoAAAAAAD9////AoCWmAAAAAAAIlEgC+blBlIP1iijRWxqjw1u9H02sqr7y8fno6/LdnvGqPl895x2AAAAACJRIM5wyjSexMbADl4K+AI1/68zyaDlE7guKvrEDUAjwqU1AAAAAAABASsAlDV3AAAAACJRIDfCpO/CIAqc0JKgBhsCfaPGdyroYtmH+4gQK/Mnn72UIRZGOixxmh9h2gqDIecYHcQHRa8w+Sokc//iDiqXz7uMGRkAHzYIzlYAAIABAACAAAAAgAAAAABhAAAAARcgRjoscZofYdoKgyHnGB3EB0WvMPkqJHP/4g4ql8+7jBkAAQUg1YCB33LpmkGemw3ncz7fcnjhL/bBG/PjH8vpgr2L3cUBBgAhB9WAgd9y6ZpBnpsN53M+33J44S/2wRvz4x/L6YK9i93FGQAfNgjOVgAAgAEAAIAAAACAAAAAAGIAAAAAAQUg9jMNus8cd+GAosBk9wn+pNP9wn7A+jy2Vq0cy+siJ8wBBgAhB/YzDbrPHHfhgKLAZPcJ/qTT/cJ+wPo8tlatHMvrIifMGQAfNgjOVgAAgAEAAIAAAACAAQAAAFEBAAAA",
+            "Output Taproot tree must not be empty"
         ]
     ],
     "valid" : [

test/functional/rpc_psbt.py

@@ -27,6 +27,7 @@
     PSBT_IN_SHA256,
     PSBT_IN_HASH160,
     PSBT_IN_HASH256,
+    PSBT_OUT_TAP_TREE,
 )
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
@@ -779,9 +780,18 @@ def test_psbt_input_keys(psbt_input, keys):
             self.generate(self.nodes[0], 1)
             self.nodes[0].importdescriptors([{"desc": descsum_create("tr({})".format(privkey)), "timestamp":"now"}])
 
-            psbt = watchonly.sendall([wallet.getnewaddress()])["psbt"]
+            psbt = watchonly.sendall([wallet.getnewaddress(), addr])["psbt"]
             psbt = self.nodes[0].walletprocesspsbt(psbt)["psbt"]
-            self.nodes[0].sendrawtransaction(self.nodes[0].finalizepsbt(psbt)["hex"])
+            txid = self.nodes[0].sendrawtransaction(self.nodes[0].finalizepsbt(psbt)["hex"])
+            vout = find_vout_for_address(self.nodes[0], txid, addr)
+
+            # Make sure tap tree is in psbt
+            parsed_psbt = PSBT.from_base64(psbt)
+            assert_greater_than(len(parsed_psbt.o[vout].map[PSBT_OUT_TAP_TREE]), 0)
+            assert "taproot_tree" in self.nodes[0].decodepsbt(psbt)["outputs"][vout]
+            parsed_psbt.make_blank()
+            comb_psbt = self.nodes[0].combinepsbt([psbt, parsed_psbt.to_base64()])
+            assert_equal(comb_psbt, psbt)
 
             self.log.info("Test that walletprocesspsbt both updates and signs a non-updated psbt containing Taproot inputs")
             addr = self.nodes[0].getnewaddress("", "bech32m")
@@ -793,6 +803,14 @@ def test_psbt_input_keys(psbt_input, keys):
             self.nodes[0].sendrawtransaction(rawtx)
             self.generate(self.nodes[0], 1)
 
+            # Make sure tap tree is not in psbt
+            parsed_psbt = PSBT.from_base64(psbt)
+            assert PSBT_OUT_TAP_TREE not in parsed_psbt.o[0].map
+            assert "taproot_tree" not in self.nodes[0].decodepsbt(psbt)["outputs"][0]
+            parsed_psbt.make_blank()
+            comb_psbt = self.nodes[0].combinepsbt([psbt, parsed_psbt.to_base64()])
+            assert_equal(comb_psbt, psbt)
+
         self.log.info("Test decoding PSBT with per-input preimage types")
         # note that the decodepsbt RPC doesn't check whether preimages and hashes match
         hash_ripemd160, preimage_ripemd160 = random_bytes(20), random_bytes(50)

test/functional/test_framework/psbt.py

@@ -123,6 +123,15 @@ def serialize(self):
         psbt = [x.serialize() for x in [self.g] + self.i + self.o]
         return b"psbt\xff" + b"".join(psbt)
 
+    def make_blank(self):
+        """
+        Remove all fields except for PSBT_GLOBAL_UNSIGNED_TX
+        """
+        for m in self.i + self.o:
+            m.map.clear()
+
+        self.g = PSBTMap(map={0: self.g.map[0]})
+
     def to_base64(self):
         return base64.b64encode(self.serialize()).decode("utf8")