From 0652dc53b291bd295caff4093ec2854fd4b34645 Mon Sep 17 00:00:00 2001 From: Jeremy Rubin Date: Tue, 16 Aug 2022 12:09:27 -0700 Subject: [PATCH 1/6] [BugFix]: Do not allow deserializing PSBT with empty PSBT_OUT_TAP_TREE --- src/psbt.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/psbt.h b/src/psbt.h index d5c67802c7e..b8c35c99707 100644 --- a/src/psbt.h +++ b/src/psbt.h @@ -862,6 +862,9 @@ struct PSBTOutput std::vector 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"); + } while (!s_tree.empty()) { uint8_t depth; uint8_t leaf_ver; From 7df6e1bb77a96eac4fbcba424bbe780636b86650 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Thu, 6 Oct 2022 15:19:30 -0400 Subject: [PATCH 2/6] psbt: Fix merging of m_tap_tree Merging should be checking that the current PSBTOutput doesn't have a taptree and the other one's is copied over. The original merging had this inverted and would remove m_tap_tree if the other did not have it. --- src/psbt.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/psbt.cpp b/src/psbt.cpp index 36fec74bc94..6b75de91f82 100644 --- a/src/psbt.cpp +++ b/src/psbt.cpp @@ -265,7 +265,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.has_value() && output.m_tap_tree.has_value()) m_tap_tree = output.m_tap_tree; } bool PSBTInputSigned(const PSBTInput& input) { From 22c051ca70bae73e0430b05fb9d879591df27699 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Thu, 6 Oct 2022 15:32:33 -0400 Subject: [PATCH 3/6] tests: Test that PSBT_OUT_TAP_TREE is combined correctly --- test/functional/rpc_psbt.py | 22 ++++++++++++++++++++-- test/functional/test_framework/psbt.py | 9 +++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index 4583ca25cf4..1fe3b21542d 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -27,6 +27,7 @@ from test_framework.psbt import ( 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 @@ class PSBTTest(BitcoinTestFramework): 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 @@ class PSBTTest(BitcoinTestFramework): 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) diff --git a/test/functional/test_framework/psbt.py b/test/functional/test_framework/psbt.py index 68945e7e843..3a5b4ec74db 100644 --- a/test/functional/test_framework/psbt.py +++ b/test/functional/test_framework/psbt.py @@ -123,6 +123,15 @@ class PSBT: 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") From 0577d423adda8e719d7611d03355680c8fbacab8 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 16 Aug 2022 21:17:43 -0400 Subject: [PATCH 4/6] psbt: Change m_tap_tree to store just the tuples Instead of having an entire TaprootBuilder which may or may not be complete, and could potentially have future changes that interact oddly with taproot tree tuples, have m_tap_tree be just the tuples. When needed in other a TaprootBuilder for actual use, the tuples will be added to a a TaprootBuilder that, in the future, can take in whatever other data is needed as well. --- src/psbt.cpp | 14 ++++++++++---- src/psbt.h | 22 +++++++--------------- src/rpc/rawtransaction.cpp | 8 ++------ 3 files changed, 19 insertions(+), 25 deletions(-) diff --git a/src/psbt.cpp b/src/psbt.cpp index 6b75de91f82..6c6ea6a555e 100644 --- a/src/psbt.cpp +++ b/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); @@ -244,7 +250,7 @@ void PSBTOutput::FromSignatureData(const SignatureData& sigdata) m_tap_internal_key = sigdata.tr_spenddata.internal_key; } if (sigdata.tr_builder.has_value()) { - m_tap_tree = sigdata.tr_builder; + 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) { diff --git a/src/psbt.h b/src/psbt.h index b8c35c99707..ddcdb8c68dc 100644 --- a/src/psbt.h +++ b/src/psbt.h @@ -713,7 +713,7 @@ struct PSBTOutput CScript witness_script; std::map hd_keypaths; XOnlyPubKey m_tap_internal_key; - std::optional m_tap_tree; + std::vector> m_tap_tree; std::map, KeyOriginInfo>> m_tap_bip32_paths; std::map, std::vector> unknown; std::set 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 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,13 +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 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; @@ -878,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; @@ -934,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"); } diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 7ffb4993307..b640e5d4ba4 100644 --- a/src/rpc/rawtransaction.cpp +++ b/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); From 30ff25cf37eec4b09ab40424eb5d6a4a80410955 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 16 Aug 2022 15:26:19 -0400 Subject: [PATCH 5/6] psbt: Only include m_tap_tree if it has scripts --- src/psbt.cpp | 2 +- src/script/standard.h | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/psbt.cpp b/src/psbt.cpp index 6c6ea6a555e..cbf2f887884 100644 --- a/src/psbt.cpp +++ b/src/psbt.cpp @@ -249,7 +249,7 @@ 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()) { + 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) { diff --git a/src/script/standard.h b/src/script/standard.h index 1e6769782a3..966a52b2c78 100644 --- a/src/script/standard.h +++ b/src/script/standard.h @@ -315,6 +315,8 @@ public: TaprootSpendData GetSpendData() const; /** Returns a vector of tuples representing the depth, leaf version, and script */ std::vector> 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. From 9e386afb67bf8fa71b72f730da1695eeb11828cd Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 16 Aug 2022 15:38:14 -0400 Subject: [PATCH 6/6] tests: Test that PSBT_OUT_TAP_TREE is included correctly --- test/functional/data/rpc_psbt.json | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/functional/data/rpc_psbt.json b/test/functional/data/rpc_psbt.json index 657faebffca..31273508728 100644 --- a/test/functional/data/rpc_psbt.json +++ b/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" : [