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
314@@ -315,6 +315,8 @@ class TaprootBuilder
315 TaprootSpendData GetSpendData() const;
316 /** Returns a vector of tuples representing the depth, leaf version, and script */
317 std::vector<std::tuple<uint8_t, uint8_t, CScript>> GetTreeTuples() const;
318+ /** Returns true if there are any tapscripts */
319+ bool HasScripts() const { return m_branch.size() > 0; }
0 bool HasScripts() const { return !m_branch.empty(); }
Either approach should work.
Not as big a fan of this approach because we may, in the future, wish to have access to the TaprootBuilder even HasScripts is false. E.g., were the TaprootBuilder to be extended to contain information about MuSig derivations, in which case the changes would be more straightforward IMO.
Of course, it’s silly to predict how future changes might be made, but I just personally find it conceptually cleaner to hang onto whatever data the sigdata passed us, and to use it correctly on output.
Not as big a fan of this approach because we may, in the future, wish to have access to the TaprootBuilder even HasScripts is false. E.g., were the TaprootBuilder to be extended to contain information about MuSig derivations, in which case the changes would be more straightforward IMO.
I highly doubt that would be the case. And if it were, it’s not particularly difficult to deal with it then. Furthermore, the general structure of the PSBT implementation is that each field has its own explicit member inside its struct, so any future things would get their own fields, which will then be un/packed from TaprootBuilder as needed.
Perhaps the more correct way to implement m_tap_tree is to make it the vector of tuples rather than a TaprootBuilder.
Perhaps the more correct way to implement
m_tap_treeis to make it the vector of tuples rather than a TaprootBuilder.
Added a commit that does this.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
227+ CScript script = std::get<2>(tuple);
228+ builder.Add((int)depth, script, (int)leaf_ver, /*track=*/true);
229+ }
230+ assert(builder.IsComplete());
231+ builder.Finalize(m_tap_internal_key);
232+ TaprootSpendData spenddata = builder.GetSpendData();
712@@ -713,7 +713,7 @@ struct PSBTOutput
713 CScript witness_script;
714 std::map<CPubKey, KeyOriginInfo> hd_keypaths;
715 XOnlyPubKey m_tap_internal_key;
716- std::optional<TaprootBuilder> m_tap_tree;
717+ std::optional<std::vector<std::tuple<uint8_t, uint8_t, CScript>>> m_tap_tree;
846@@ -834,6 +847,9 @@ def test_psbt_input_keys(psbt_input, keys):
847 assert_raises_rpc_error(-8, "PSBTs not compatible (different transactions)", self.nodes[0].combinepsbt, [psbt1, psbt2])
848 assert_equal(self.nodes[0].combinepsbt([psbt1, psbt1]), psbt1)
849
850+ if self.options.descriptors:
851+ self.log.info("Test that PSBT_OUT_TAPROOT_TREE is only present for Taproot outputs that have script paths")
852+
218@@ -219,7 +219,16 @@ void PSBTOutput::FillSignatureData(SignatureData& sigdata) const
219 sigdata.misc_pubkeys.emplace(key_pair.first.GetID(), key_pair);
220 }
221 if (m_tap_tree.has_value() && m_tap_internal_key.IsFullyValid()) {
222- TaprootSpendData spenddata = m_tap_tree->GetSpendData();
223+ TaprootBuilder builder;
224+ for (const auto& tuple : m_tap_tree.value()) {
225+ uint8_t depth = std::get<0>(tuple);
226+ uint8_t leaf_ver = std::get<1>(tuple);
227+ CScript script = std::get<2>(tuple);
0 for (const auto& [depth, leaf_ver, script] : m_tap_tree.value()) {
757@@ -758,8 +758,7 @@ struct PSBTOutput
758 SerializeToVector(s, PSBT_OUT_TAP_TREE);
759 std::vector<unsigned char> value;
760 CVectorWriter s_value(s.GetType(), s.GetVersion(), value, 0);
761- const auto& tuples = m_tap_tree->GetTreeTuples();
762- for (const auto& tuple : tuples) {
763+ for (const auto& tuple : m_tap_tree.value()) {
764 uint8_t depth = std::get<0>(tuple);
765 uint8_t leaf_ver = std::get<1>(tuple);
766 CScript script = std::get<2>(tuple);
0 for (const auto& [depth, leaf_ver, script] : m_tap_tree.value()) {
1242@@ -1243,8 +1243,7 @@ static RPCHelpMan decodepsbt()
1243 // Taproot tree
1244 if (output.m_tap_tree.has_value()) {
1245 UniValue tree(UniValue::VARR);
1246- const auto& tuples = output.m_tap_tree->GetTreeTuples();
1247- for (const auto& tuple : tuples) {
1248+ for (const auto& tuple : output.m_tap_tree.value()) {
1249 uint8_t depth = std::get<0>(tuple);
1250 uint8_t leaf_ver = std::get<1>(tuple);
1251 CScript script = std::get<2>(tuple);
0 for (const auto& [depth, leaf_ver, script] : output.m_tap_tree.value()) {
270@@ -265,7 +271,7 @@ void PSBTOutput::Merge(const PSBTOutput& output)
271 if (redeem_script.empty() && !output.redeem_script.empty()) redeem_script = output.redeem_script;
272 if (witness_script.empty() && !output.witness_script.empty()) witness_script = output.witness_script;
273 if (m_tap_internal_key.IsNull() && !output.m_tap_internal_key.IsNull()) m_tap_internal_key = output.m_tap_internal_key;
274- if (m_tap_tree.has_value() && !output.m_tap_tree.has_value()) m_tap_tree = output.m_tap_tree;
275+ if (!m_tap_tree.empty() && output.m_tap_tree.empty()) m_tap_tree = output.m_tap_tree;
isn’t this the opposite of what we want?
why is the logic switched to “clear this m_tap_tree if merging output has none set” from “if unset, set m_tap_tree if output has one set”.
hmmm, even if not inverted, is the original behavior the intended behavior and why?
Other lines seem to be opposite, e.g. 273 has set m_tap_key_internal from non-null output.m_tap_key_internal semantics, not unset m_tap_key_internal from null output.m_tap_key_internal
0if (m_tap_internal_key.IsNull() && !output.m_tap_internal_key.IsNull()) m_tap_internal_key = output.m_tap_internal_key;
270@@ -265,7 +271,7 @@ void PSBTOutput::Merge(const PSBTOutput& output)
271 if (redeem_script.empty() && !output.redeem_script.empty()) redeem_script = output.redeem_script;
272 if (witness_script.empty() && !output.witness_script.empty()) witness_script = output.witness_script;
273 if (m_tap_internal_key.IsNull() && !output.m_tap_internal_key.IsNull()) m_tap_internal_key = output.m_tap_internal_key;
274- if (m_tap_tree.has_value() && !output.m_tap_tree.has_value()) m_tap_tree = output.m_tap_tree;
275+ if (m_tap_tree.empty() && !output.m_tap_tree.empty()) m_tap_tree = output.m_tap_tree;
Good catch Jeremy that the existing behaviour was incorrect, i had missed that by just checking the diff preserved the same behaviour.
nit: since this is a bugfix unrelated to this commit maybe it could have its own commit?
m_tap_tree’s type.
i think what @darosior is suggesting is that this PR should have a “parent” PR which first fixes this bug?
the original line
0 if (m_tap_tree.has_value() && !output.m_tap_tree.has_value()) m_tap_tree = output.m_tap_tree;
reads
replace the m_tap_tree if the incoming value is Empty and the current value is Something
which is clearly incorrect. now the PR is really two independent fixes, both of which might due with test coverage.
130+ for m in self.i + self.o:
131+ m.map.clear()
132+
133+ old_g = self.g
134+ self.g = PSBTMap()
135+ self.g.map[0] = old_g.map[0]
You need to copy self.g in order to really keep PSBT_GLOBAL_UNSIGNED_TX. This code will wipe the whole map since old_g is just a reference to self.g.
0diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py
1index 1fe3b21542..131ffe85e0 100755
2--- a/test/functional/rpc_psbt.py
3+++ b/test/functional/rpc_psbt.py
4@@ -790,6 +790,8 @@ class PSBTTest(BitcoinTestFramework):
5 assert_greater_than(len(parsed_psbt.o[vout].map[PSBT_OUT_TAP_TREE]), 0)
6 assert "taproot_tree" in self.nodes[0].decodepsbt(psbt)["outputs"][vout]
7 parsed_psbt.make_blank()
8+ # make_blank shouldn't have removed PSBT_GLOBAL_UNSIGNED_TX, but it has.
9+ assert parsed_psbt.g == PSBTMap()
10 comb_psbt = self.nodes[0].combinepsbt([psbt, parsed_psbt.to_base64()])
11 assert_equal(comb_psbt, psbt)
12
13diff --git a/test/functional/test_framework/psbt.py b/test/functional/test_framework/psbt.py
14index 6bb899be53..6fb81ba4cf 100644
15--- a/test/functional/test_framework/psbt.py
16+++ b/test/functional/test_framework/psbt.py
17@@ -132,6 +132,7 @@ class PSBT:
18
19 old_g = self.g
20 self.g = PSBTMap()
21+ assert old_g == self.g == PSBTMap()
22 self.g.map[0] = old_g.map[0]
23
24 def to_base64(self):
791+ parsed_psbt = PSBT.from_base64(psbt)
792+ assert_greater_than(len(parsed_psbt.o[vout].map[PSBT_OUT_TAP_TREE]), 0)
793+ assert "taproot_tree" in self.nodes[0].decodepsbt(psbt)["outputs"][vout]
794+ parsed_psbt.make_blank()
795+ comb_psbt = self.nodes[0].combinepsbt([psbt, parsed_psbt.to_base64()])
796+ assert_equal(comb_psbt, psbt)
This does not quite test the bug Jeremy found. You can reintroduce it and the test won’t fail
0diff --git a/src/psbt.cpp b/src/psbt.cpp
1index cbf2f88788..1567998039 100644
2--- a/src/psbt.cpp
3+++ b/src/psbt.cpp
4@@ -271,7 +271,7 @@ void PSBTOutput::Merge(const PSBTOutput& output)
5 if (redeem_script.empty() && !output.redeem_script.empty()) redeem_script = output.redeem_script;
6 if (witness_script.empty() && !output.witness_script.empty()) witness_script = output.witness_script;
7 if (m_tap_internal_key.IsNull() && !output.m_tap_internal_key.IsNull()) m_tap_internal_key = output.m_tap_internal_key;
8- if (m_tap_tree.empty() && !output.m_tap_tree.empty()) m_tap_tree = output.m_tap_tree;
9+ if (!m_tap_tree.empty() && output.m_tap_tree.empty()) m_tap_tree = output.m_tap_tree;
10 }
11 bool PSBTInputSigned(const PSBTInput& input)
12 {
self.g fix.
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.
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.
248@@ -249,7 +249,7 @@ void PSBTOutput::FromSignatureData(const SignatureData& sigdata)
249 if (!sigdata.tr_spenddata.internal_key.IsNull()) {
250 m_tap_internal_key = sigdata.tr_spenddata.internal_key;
251 }
252- if (sigdata.tr_builder.has_value()) {
tr_builder for change that doesn’t have scripts. This could only have been reached through a bad psbt, which have now been disallowed through serialization, and this change is more of a belt-and-suspenders.
Labels
PSBT
Milestone
24.0