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; }
bool HasScripts() const { return !m_branch.empty(); }
Done
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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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();
maybe this should just be a function in TaprootBuilder?
approach ACK, this seems OK.
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;
nit: What's the reason for keeping it optional now it's a vector?
Dropped the optional
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 | +
Looks like you actually didn't commit this test?
Removed. I changed how I wanted to do the test and forgot to drop the log line.
Code looks good to me. Looks like part of the functional test is missing.
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);
for (const auto& [depth, leaf_ver, script] : m_tap_tree.value()) {
Done
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);
for (const auto& [depth, leaf_ver, script] : m_tap_tree.value()) {
Done
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);
for (const auto& [depth, leaf_ver, script] : output.m_tap_tree.value()) {
Done
Concept ACK
ACK e04f5b93fb
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".
The logic is still "clear this m_tap_tree if merging output has none set". It's just that "has value" is the opposite of "is empty" so it had to be inversed.
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
if (m_tap_internal_key.IsNull() && !output.m_tap_internal_key.IsNull()) m_tap_internal_key = output.m_tap_internal_key;
Oops, got my booleans backwards, good catch.
There should be a test covering this stuff as well?
Added a test.
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?
Not really? It's part of the change of 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
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.
Ah, I see. I've split the fix and it's test into two separate commits.
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.
diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py
index 1fe3b21542..131ffe85e0 100755
--- a/test/functional/rpc_psbt.py
+++ b/test/functional/rpc_psbt.py
@@ -790,6 +790,8 @@ class PSBTTest(BitcoinTestFramework):
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()
+ # make_blank shouldn't have removed PSBT_GLOBAL_UNSIGNED_TX, but it has.
+ assert parsed_psbt.g == PSBTMap()
comb_psbt = self.nodes[0].combinepsbt([psbt, parsed_psbt.to_base64()])
assert_equal(comb_psbt, psbt)
diff --git a/test/functional/test_framework/psbt.py b/test/functional/test_framework/psbt.py
index 6bb899be53..6fb81ba4cf 100644
--- a/test/functional/test_framework/psbt.py
+++ b/test/functional/test_framework/psbt.py
@@ -132,6 +132,7 @@ class PSBT:
old_g = self.g
self.g = PSBTMap()
+ assert old_g == self.g == PSBTMap()
self.g.map[0] = old_g.map[0]
def to_base64(self):
Fixed.
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
diff --git a/src/psbt.cpp b/src/psbt.cpp
index cbf2f88788..1567998039 100644
--- a/src/psbt.cpp
+++ b/src/psbt.cpp
@@ -271,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.empty() && !output.m_tap_tree.empty()) 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)
{
Should work now with the 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()) {
let's add a test for this specific case
I think this is not really testable since we would not have a 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.
seems this would only be hit internally, resulting in a psbt with less data. not too troubling
ACK 9e386afb67bf8fa71b72f730da1695eeb11828cd
post ack, looks correct to me now