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

achow101 · 2022-08-16T20:15:01Z

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

JeremyRubin · 2022-08-17T00:46:48Z

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.

achow101 · 2022-08-17T00:52:01Z

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.

achow101 · 2022-08-17T01:20:48Z

Perhaps the more correct way to implement m_tap_tree is to make it the vector of tuples rather than a TaprootBuilder.

Added a commit that does this.

DrahtBot · 2022-08-17T07:47:37Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26238 (clang-tidy: fixup named argument comments by fanquake)
#25877 (refactor: Do not use CScript for tapleaf scripts until the tapleaf version is known by roconnor-blockstream)

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.

JeremyRubin · 2022-08-17T14:58:59Z

src/psbt.cpp

+        }
+        assert(builder.IsComplete());
+        builder.Finalize(m_tap_internal_key);
+        TaprootSpendData spenddata = builder.GetSpendData();


maybe this should just be a function in TaprootBuilder?

JeremyRubin · 2022-08-17T14:59:32Z

approach ACK, this seems OK.

darosior

Code looks good to me. Looks like part of the functional test is missing.

src/psbt.h

test/functional/rpc_psbt.py

src/script/standard.h

src/psbt.cpp

src/psbt.h

src/rpc/rawtransaction.cpp

S3RK · 2022-09-14T06:42:11Z

Concept ACK

darosior · 2022-09-14T10:16:52Z

ACK e04f5b9

src/psbt.cpp

test/functional/test_framework/psbt.py

test/functional/rpc_psbt.py

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.

instagibbs · 2022-10-10T20:19:20Z

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()) {


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

darosior

ACK 9e386af

instagibbs · 2022-10-13T13:06:48Z

ACK 9e386af

fanquake · 2022-10-13T15:48:09Z

Added to #26133 for backport to 24.x.

Github-Pull: bitcoin#25858 Rebased-From: 0652dc5

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. Github-Pull: bitcoin#25858 Rebased-From: 7df6e1b

Github-Pull: bitcoin#25858 Rebased-From: 22c051c

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. Github-Pull: bitcoin#25858 Rebased-From: 0577d42

Github-Pull: bitcoin#25858 Rebased-From: 30ff25c

Github-Pull: bitcoin#25858 Rebased-From: 9e386af

e2e4c29 tests: Test that PSBT_OUT_TAP_TREE is included correctly (Andrew Chow) 4d42c3a psbt: Only include m_tap_tree if it has scripts (Andrew Chow) d810fde psbt: Change m_tap_tree to store just the tuples (Andrew Chow) a9419ef tests: Test that PSBT_OUT_TAP_TREE is combined correctly (Andrew Chow) 4abd2ab psbt: Fix merging of m_tap_tree (Andrew Chow) 1390c96 [BugFix]: Do not allow deserializing PSBT with empty PSBT_OUT_TAP_TREE (Jeremy Rubin) 9b438f0 refactor: revert m_next_resend to not be std::atomic (stickies-v) 43ced0b wallet: only update m_next_resend when actually resending (stickies-v) fc8f2bf refactor: carve out tx resend timer logic into ShouldResend (stickies-v) a6fb674 refactor: remove unused locks for ResubmitWalletTransactions (stickies-v) 5ad82a0 index: Improve BaseIndex::BlockUntilSyncedToCurrentChain reliability (Ryan Ofsky) 997faf6 contrib: Fix capture_output in getcoins.py (willcl-ark) 7e0bcfb p2p: ProcessHeadersMessage(): fix received_new_header (Larry Ruane) c97d924 Correct sanity-checking script_size calculation (Pieter Wuille) da6fba6 docs: Add 371 to bips.md (Andrew Chow) Pull request description: Will collect backports for rc2 as they become available. Currently: * #25858 * #26124 * #26149 * #26172 * #26205 * #26212 * #26215 ACKs for top commit: dergoegge: ACK e2e4c29 achow101: ACK e2e4c29 instagibbs: ACK e2e4c29 Tree-SHA512: b6374fe202561057dbe1430d4c40f06f721eb568f91e7275ae1ee7747edf780ce64620382d13ecc4b9571d931dc25d226af8284987cf35ff6a6182c5f64eb10c

JeremyRubin · 2022-10-14T13:30:18Z

post ack, looks correct to me now

…tput 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 bitcoin#25856 ACKs for top commit: instagibbs: ACK bitcoin@9e386af darosior: ACK 9e386af Tree-SHA512: ce5c02a69752d176dbd967c1e8d30129b1905c8f186aeeef034576c1de82059271a1ee846bd040f5be4e66bb77ba711dcf14ac1e597c5707d7e7e2293f6cfefb

[BugFix]: Do not allow deserializing PSBT with empty PSBT_OUT_TAP_TREE

0652dc5

achow101 added this to the 24.0 milestone Aug 16, 2022

achow101 mentioned this pull request Aug 16, 2022

[BugFix] Make PSBT serializations 174 compliant by excluding empty taptree from serialization #25856

Closed

DrahtBot added the PSBT label Aug 16, 2022

achow101 force-pushed the psbt-no-empty-taptree branch from bac92f9 to 9e74569 Compare August 17, 2022 01:20

JeremyRubin reviewed Aug 17, 2022

View reviewed changes

DrahtBot mentioned this pull request Aug 20, 2022

refactor: Do not use CScript for tapleaf scripts until the tapleaf version is known #25877

Merged

fanquake requested review from instagibbs and darosior September 1, 2022 06:57

darosior reviewed Sep 13, 2022

View reviewed changes

src/psbt.h Outdated Show resolved Hide resolved

test/functional/rpc_psbt.py Outdated Show resolved Hide resolved

aureleoules reviewed Sep 13, 2022

View reviewed changes

src/script/standard.h Outdated Show resolved Hide resolved

src/psbt.cpp Outdated Show resolved Hide resolved

src/psbt.h Outdated Show resolved Hide resolved

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

achow101 force-pushed the psbt-no-empty-taptree branch from 9e74569 to e04f5b9 Compare September 13, 2022 16:25

fanquake mentioned this pull request Sep 14, 2022

Fix PSBT Deserialization to 174 Compliance over empty Taproot Tree rust-bitcoin/rust-bitcoin#1194

Closed

glozow requested review from JeremyRubin and S3RK September 15, 2022 10:23

JeremyRubin reviewed Sep 15, 2022

View reviewed changes

src/psbt.cpp Outdated Show resolved Hide resolved

achow101 force-pushed the psbt-no-empty-taptree branch 3 times, most recently from f97e0a6 to 5e2b8d1 Compare September 16, 2022 16:15

glozow requested review from darosior and JeremyRubin September 19, 2022 08:36

darosior reviewed Sep 27, 2022

View reviewed changes

src/psbt.cpp Show resolved Hide resolved

test/functional/test_framework/psbt.py Outdated Show resolved Hide resolved

test/functional/rpc_psbt.py Show resolved Hide resolved

DrahtBot mentioned this pull request Oct 3, 2022

clang-tidy: fixup named argument comments #26238

Merged

achow101 force-pushed the psbt-no-empty-taptree branch from 5e2b8d1 to 1f4faeb Compare October 6, 2022 17:14

achow101 and others added 5 commits October 6, 2022 15:19

psbt: Fix merging of m_tap_tree

7df6e1b

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.

tests: Test that PSBT_OUT_TAP_TREE is combined correctly

22c051c

psbt: Only include m_tap_tree if it has scripts

30ff25c

tests: Test that PSBT_OUT_TAP_TREE is included correctly

9e386af

achow101 force-pushed the psbt-no-empty-taptree branch from 1f4faeb to 9e386af Compare October 6, 2022 19:33

instagibbs reviewed Oct 10, 2022

View reviewed changes

glozow requested review from instagibbs and darosior October 12, 2022 20:04

darosior approved these changes Oct 13, 2022

View reviewed changes

glozow merged commit 147d64d into bitcoin:master Oct 13, 2022

glozow added the Needs backport (24.x) label Oct 13, 2022

fanquake removed the Needs backport (24.x) label Oct 13, 2022

fanquake mentioned this pull request Oct 13, 2022

[24.x] Backports for rc2 #26133

Merged

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Oct 13, 2022

[BugFix]: Do not allow deserializing PSBT with empty PSBT_OUT_TAP_TREE

1390c96

Github-Pull: bitcoin#25858 Rebased-From: 0652dc5

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Oct 13, 2022

tests: Test that PSBT_OUT_TAP_TREE is combined correctly

a9419ef

Github-Pull: bitcoin#25858 Rebased-From: 22c051c

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Oct 13, 2022

psbt: Only include m_tap_tree if it has scripts

4d42c3a

Github-Pull: bitcoin#25858 Rebased-From: 30ff25c

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Oct 13, 2022

tests: Test that PSBT_OUT_TAP_TREE is included correctly

e2e4c29

Github-Pull: bitcoin#25858 Rebased-From: 9e386af

bitcoin locked and limited conversation to collaborators Oct 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

achow101 commented Aug 16, 2022

JeremyRubin commented Aug 17, 2022

achow101 commented Aug 17, 2022

achow101 commented Aug 17, 2022

DrahtBot commented Aug 17, 2022 •

edited

Loading

JeremyRubin Aug 17, 2022

JeremyRubin commented Aug 17, 2022

darosior left a comment

S3RK commented Sep 14, 2022

darosior commented Sep 14, 2022

instagibbs Oct 10, 2022

achow101 Oct 10, 2022

instagibbs Oct 11, 2022

darosior left a comment

instagibbs commented Oct 13, 2022

fanquake commented Oct 13, 2022

JeremyRubin commented Oct 14, 2022

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

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

Conversation

achow101 commented Aug 16, 2022

JeremyRubin commented Aug 17, 2022

achow101 commented Aug 17, 2022

achow101 commented Aug 17, 2022

DrahtBot commented Aug 17, 2022 • edited Loading

Conflicts

JeremyRubin Aug 17, 2022

Choose a reason for hiding this comment

JeremyRubin commented Aug 17, 2022

darosior left a comment

Choose a reason for hiding this comment

S3RK commented Sep 14, 2022

darosior commented Sep 14, 2022

instagibbs Oct 10, 2022

Choose a reason for hiding this comment

achow101 Oct 10, 2022

Choose a reason for hiding this comment

instagibbs Oct 11, 2022

Choose a reason for hiding this comment

darosior left a comment

Choose a reason for hiding this comment

instagibbs commented Oct 13, 2022

fanquake commented Oct 13, 2022

JeremyRubin commented Oct 14, 2022

DrahtBot commented Aug 17, 2022 •

edited

Loading