Implement BIP 370 PSBTv2 #21283

pull achow101 wants to merge 34 commits into bitcoin:master from achow101:psbt2 changing 25 files +1048 −248
  1. achow101 commented at 6:29 pm on February 23, 2021: member
    BIP 370 PSBTv2 introduces several new fields and different invariants for PSBT. This PR implements those new fields and restructures the PSBT implementation to match PSBTv2 but still remain compatible with PSBTv0.
  2. DrahtBot added the label GUI on Feb 23, 2021
  3. DrahtBot added the label RPC/REST/ZMQ on Feb 23, 2021
  4. DrahtBot added the label Wallet on Feb 23, 2021
  5. DrahtBot commented at 0:38 am on February 24, 2021: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/21283.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK rkrux
    Stale ACK scgbckbone

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #31622 (psbt: add non-default sighash types to PSBTs and unify sighash type match checking by achow101)
    • #31247 (psbt: MuSig2 Fields by achow101)
    • #30886 (rpc: Add support to populate PSBT input utxos via rpc by instagibbs)
    • #30341 (WIP: Permit Combiner to strip bip32_deriv information by willcl-ark)
    • #29675 (wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys by achow101)
    • #24963 (RPC/Wallet: Convert walletprocesspsbt to use options parameter by luke-jr)

    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.

  6. achow101 force-pushed on Mar 5, 2021
  7. in src/rpc/rawtransaction.cpp:1193 in 6e4c04e3a2 outdated 
    1192+        }
1193+        if (psbtx.fallback_locktime != std::nullopt) {
1194+            result.pushKV("fallback_locktime", static_cast<uint64_t>(*psbtx.fallback_locktime));
1195+        }
1196+        result.pushKV("input_count", psbtx.inputs.size());
1197+        result.pushKV("output_count", psbtx.inputs.size());
    adamjonas commented at 3:50 pm on March 5, 2021:

    Getting a compile error consistent with the CI for these two lines:

     0rpc/rawtransaction.cpp:1192:16: error: call to member function 'pushKV' is ambiguous
 1        result.pushKV("input_count", psbtx.inputs.size());
 2        ~~~~~~~^~~~~~
 3./univalue/include/univalue.h:127:10: note: candidate function
 4    bool pushKV(const std::string& key, int64_t val_) {
 5         ^
 6./univalue/include/univalue.h:131:10: note: candidate function
 7    bool pushKV(const std::string& key, uint64_t val_) {
 8         ^
 9./univalue/include/univalue.h:135:10: note: candidate function
10    bool pushKV(const std::string& key, bool val_) {
11         ^
12./univalue/include/univalue.h:139:10: note: candidate function
13    bool pushKV(const std::string& key, int val_) {
14         ^
15./univalue/include/univalue.h:143:10: note: candidate function
16    bool pushKV(const std::string& key, double val_) {
17         ^
18./univalue/include/univalue.h:118:10: note: candidate function
19    bool pushKV(const std::string& key, const UniValue& val);
20         ^
21./univalue/include/univalue.h:119:10: note: candidate function not viable: no known conversion from 'std::__1::vector<PSBTInput, std::__1::allocator<PSBTInput> >::size_type' (aka 'unsigned long') to 'const std::string' (aka 'const basic_string<char, char_traits<char>, allocator<char> >') for 2nd argument
22    bool pushKV(const std::string& key, const std::string& val_) {
23         ^
24./univalue/include/univalue.h:123:10: note: candidate function not viable: no known conversion from 'std::__1::vector<PSBTInput, std::__1::allocator<PSBTInput> >::size_type' (aka 'unsigned long') to 'const char *' for 2nd argument
25    bool pushKV(const std::string& key, const char *val_) {
26         ^
27rpc/rawtransaction.cpp:1193:16: error: call to member function 'pushKV' is ambiguous
28        result.pushKV("output_count", psbtx.inputs.size());
29        ~~~~~~~^~~~~~
30./univalue/include/univalue.h:127:10: note: candidate function
31    bool pushKV(const std::string& key, int64_t val_) {
32         ^
33./univalue/include/univalue.h:131:10: note: candidate function
34    bool pushKV(const std::string& key, uint64_t val_) {
35         ^
36./univalue/include/univalue.h:135:10: note: candidate function
37    bool pushKV(const std::string& key, bool val_) {
38         ^
39./univalue/include/univalue.h:139:10: note: candidate function
40    bool pushKV(const std::string& key, int val_) {
41         ^
42./univalue/include/univalue.h:143:10: note: candidate function
43    bool pushKV(const std::string& key, double val_) {
44         ^
45./univalue/include/univalue.h:118:10: note: candidate function
46    bool pushKV(const std::string& key, const UniValue& val);
47         ^
48./univalue/include/univalue.h:119:10: note: candidate function not viable: no known conversion from 'std::__1::vector<PSBTInput, std::__1::allocator<PSBTInput> >::size_type' (aka 'unsigned long') to 'const std::string' (aka 'const basic_string<char, char_traits<char>, allocator<char> >') for 2nd argument
49    bool pushKV(const std::string& key, const std::string& val_) {
50         ^
51./univalue/include/univalue.h:123:10: note: candidate function not viable: no known conversion from 'std::__1::vector<PSBTInput, std::__1::allocator<PSBTInput> >::size_type' (aka 'unsigned long') to 'const char *' for 2nd argument
52    bool pushKV(const std::string& key, const char *val_) {
53         ^
542 errors generated.
    achow101 commented at 5:51 pm on March 5, 2021:
    I can’t replicate this error at all, but I think I have fixed it.
  8. achow101 force-pushed on Mar 5, 2021
  9. DrahtBot added the label Needs rebase on Mar 17, 2021
  10. achow101 force-pushed on Mar 17, 2021
  11. achow101 force-pushed on Mar 17, 2021
  12. DrahtBot removed the label Needs rebase on Mar 17, 2021
  13. DrahtBot added the label Needs rebase on Mar 29, 2021
  14. achow101 force-pushed on Apr 1, 2021
  15. DrahtBot removed the label Needs rebase on Apr 1, 2021
  16. achow101 force-pushed on Apr 5, 2021
  17. achow101 force-pushed on Apr 5, 2021
  18. achow101 force-pushed on Apr 5, 2021
  19. achow101 force-pushed on Apr 5, 2021
  20. achow101 force-pushed on Apr 5, 2021
  21. achow101 force-pushed on Apr 5, 2021
  22. DrahtBot added the label Needs rebase on Apr 19, 2021
  23. achow101 force-pushed on Apr 19, 2021
  24. DrahtBot removed the label Needs rebase on Apr 19, 2021
  25. in src/psbt.cpp:27 in 776419b72e outdated 
    18@@ -19,6 +19,14 @@ PartiallySignedTransaction::PartiallySignedTransaction(const CMutableTransaction
19     SetupFromTx(tx);
20 }
21 
22+PartiallySignedTransaction::PartiallySignedTransaction(uint32_t version) :
23+    m_version(version)
24+{
25+    if (GetVersion() >= 2) {
26+        tx_version = CTransaction::CURRENT_VERSION;
    adamjonas commented at 2:33 pm on April 21, 2021:
    This is erroring for me: psbt.cpp:(.text+0x4ca): undefined reference to 'CTransaction::CURRENT_VERSION'
  26. in src/psbt.h:808 in dd0ea68a16 outdated 
    803@@ -661,6 +804,10 @@ struct PartiallySignedTransaction
804 
805         // Read global data
806         bool found_sep = false;
807+        uint32_t input_count = 0;
808+        uint32_t output_count = 0;
    sanket1729 commented at 2:49 am on May 11, 2021:
    @achow101, the BIP says that this should CompactSize which I assume means VarInt in bitcoin context? But the implementation serializes this as uint32_t
    achow101 commented at 5:52 pm on May 11, 2021:
    Oops, fixed.
  27. achow101 force-pushed on May 11, 2021
  28. DrahtBot added the label Needs rebase on May 30, 2021
  29. achow101 force-pushed on May 30, 2021
  30. DrahtBot removed the label Needs rebase on May 30, 2021
  31. DrahtBot added the label Needs rebase on Jun 17, 2021
  32. achow101 force-pushed on Jun 18, 2021
  33. DrahtBot removed the label Needs rebase on Jun 19, 2021
  34. DrahtBot added the label Needs rebase on Aug 2, 2021
  35. achow101 force-pushed on Aug 2, 2021
  36. DrahtBot removed the label Needs rebase on Aug 2, 2021
  37. DrahtBot added the label Needs rebase on Sep 23, 2021
  38. achow101 force-pushed on Sep 23, 2021
  39. DrahtBot removed the label Needs rebase on Sep 23, 2021
  40. in src/psbt.h:437 in 015f133e9a outdated 
    432@@ -428,6 +433,12 @@ struct PartiallySignedTransaction
433         OverrideStream<Stream> os(&s, s.GetType(), s.GetVersion() | SERIALIZE_TRANSACTION_NO_WITNESS);
434         SerializeToVector(os, *tx);
435 
436+        // PSBT version
437+        if (m_version != std::nullopt && m_version > 0) {
    kiminuo commented at 11:37 am on October 1, 2021:

    I think it should be:

    if (m_version != std::nullopt && m_version > 0) { -> if (m_version != std::nullopt && *m_version > 0) {

    at least your next commit makes me think so (https://github.com/bitcoin/bitcoin/pull/21283/commits/92ff609c1ef9d3cd848d08b4bbec1cdbf4ae4f0b#diff-2fb519d85c6ae99cd87c94e5b7cba43ff2ddd031b2c523dd2ffb12e991c16494R438)

    achow101 commented at 5:02 pm on October 1, 2021:
    Done
  41. kiminuo commented at 11:52 am on October 1, 2021: contributor
    In c8b879609f02288975e3a6f714fbcfa1a93edcbc (moveonly: Move (Un)Serialize(To/From)Vector, (De)SerializeHDKeypaths to psbt module), should you possibly move/copy #include <streams.h>, #include <span.h> and #include <script/keyorigin.h>?
  42. achow101 force-pushed on Oct 1, 2021
  43. achow101 force-pushed on Oct 1, 2021
  44. kiminuo commented at 7:20 pm on October 2, 2021: contributor

    In commit 0bd656b64e72e29f255a860c71c542f5a9f18c17 (Separate individual HD Keypath serialization into separate functions)

    Should line:

    0if (value_len % 4 || value_len == 0) {

    be

    0if (value_len % 4 != 0 || value_len == 0) {

    to make it more explicit?

  45. kiminuo commented at 7:41 pm on October 2, 2021: contributor

    Is it possible that in commit 459fd3b0cc646f5ab02dd3cfd239c7a0418e6a32 (*Implement operator< for KeyOriginInfo and CExtPubKey *)

    0else if (a.pubkey > b.pubkey) {
1    return false;
2}

    is missing?

  46. achow101 force-pushed on Oct 3, 2021
  47. achow101 commented at 5:34 pm on October 3, 2021: member

    @kiminuo

    In commit 0bd656b (Separate individual HD Keypath serialization into separate functions)

    Should line:

    0if (value_len % 4 || value_len == 0) {

    be

    0if (value_len % 4 != 0 || value_len == 0) {

    to make it more explicit?

    This is moved code, so I will not be changing it.

    Is it possible that in commit 459fd3b (*Implement operator< for KeyOriginInfo and CExtPubKey *)

    0else if (a.pubkey > b.pubkey) {
1    return false;
2}

    is missing?

    Done.

    In the future, please leave such comments on the code as inline comments rather than comments like this. It makes it much easier to respond directly to specific questions such as these.

  48. in src/pubkey.h:368 in 4130efb9cd outdated 
    306@@ -305,8 +307,20 @@ struct CExtPubKey {
307         return !(a == b);
308     }
309 
310+    friend bool operator<(const CExtPubKey &a, const CExtPubKey &b)
311+    {
312+        if (a.pubkey < b.pubkey) {
313+            return true;
314+        } else if (a.pubkey > b.pubkey) {
    kiminuo commented at 7:13 am on October 5, 2021:

    I believe that operator> implementation is missing for CPubKey class:

    0    friend bool operator>(const CPubKey& a, const CPubKey& b)
1    {
2        return a.vch[0] > b.vch[0] ||
3               (a.vch[0] == b.vch[0] && memcmp(a.vch, b.vch, a.size()) > 0);
4    }

    for this line to work and that’s why tests fail at the moment.

    achow101 commented at 6:13 pm on October 5, 2021:
    Done. Keep in mind this commit (and the first 15 of this PR) are part of #17034 so comments on them are better suited for that PR.
  49. in src/psbt.h:220 in 4130efb9cd outdated 
    183@@ -56,68 +184,109 @@ struct PSBTInput
184     CScriptWitness final_script_witness;
185     std::map<CPubKey, KeyOriginInfo> hd_keypaths;
186     std::map<CKeyID, SigPair> partial_sigs;
187+    uint256 prev_txid;
    kiminuo commented at 7:15 am on October 5, 2021:
    Should #include <uint256.h> be added?
    achow101 commented at 6:13 pm on October 5, 2021:
    Done
  50. in src/psbt.h:256 in 4130efb9cd outdated 
    198     bool IsNull() const;
199     void FillSignatureData(SignatureData& sigdata) const;
200     void FromSignatureData(const SignatureData& sigdata);
201     void Merge(const PSBTInput& input);
202-    PSBTInput() {}
203+    bool GetUTXO(CTxOut& utxo) const;
    kiminuo commented at 7:19 am on October 5, 2021:
    Should the original comment be preserved in some way?
    achow101 commented at 6:13 pm on October 5, 2021:
    Done
  51. achow101 force-pushed on Oct 5, 2021
  52. DrahtBot added the label Needs rebase on Nov 16, 2021
  53. achow101 force-pushed on Nov 16, 2021
  54. DrahtBot removed the label Needs rebase on Nov 16, 2021
  55. achow101 force-pushed on Nov 23, 2021
  56. achow101 force-pushed on Nov 24, 2021
  57. achow101 force-pushed on Nov 24, 2021
  58. achow101 force-pushed on Nov 24, 2021
  59. DrahtBot added the label Needs rebase on Nov 29, 2021
  60. achow101 force-pushed on Dec 5, 2021
  61. DrahtBot removed the label Needs rebase on Dec 5, 2021
  62. DrahtBot added the label Needs rebase on Dec 8, 2021
  63. achow101 force-pushed on Dec 8, 2021
  64. DrahtBot removed the label Needs rebase on Dec 8, 2021
  65. DrahtBot added the label Needs rebase on Dec 9, 2021
  66. achow101 force-pushed on Dec 9, 2021
  67. DrahtBot removed the label Needs rebase on Dec 9, 2021
  68. achow101 force-pushed on Dec 9, 2021
  69. achow101 marked this as a draft on Dec 9, 2021
  70. achow101 force-pushed on Dec 10, 2021
  71. achow101 marked this as ready for review on Dec 10, 2021
  72. DrahtBot added the label Needs rebase on Dec 13, 2021
  73. achow101 force-pushed on Dec 13, 2021
  74. DrahtBot removed the label Needs rebase on Dec 13, 2021
  75. achow101 force-pushed on Feb 2, 2022
  76. DrahtBot added the label Needs rebase on Mar 30, 2022
  77. achow101 force-pushed on Mar 30, 2022
  78. DrahtBot removed the label Needs rebase on Mar 30, 2022
  79. DrahtBot added the label Needs rebase on Mar 31, 2022
  80. achow101 force-pushed on Mar 31, 2022
  81. DrahtBot removed the label Needs rebase on Mar 31, 2022
  82. DrahtBot added the label Needs rebase on Apr 15, 2022
  83. achow101 force-pushed on Apr 15, 2022
  84. DrahtBot removed the label Needs rebase on Apr 15, 2022
  85. DrahtBot added the label Needs rebase on Jun 28, 2022
  86. achow101 force-pushed on Jun 29, 2022
  87. DrahtBot removed the label Needs rebase on Jun 29, 2022
  88. achow101 force-pushed on Jul 5, 2022
  89. achow101 force-pushed on Jul 6, 2022
  90. DrahtBot added the label Needs rebase on Jul 13, 2022
  91. achow101 force-pushed on Jul 13, 2022
  92. DrahtBot removed the label Needs rebase on Jul 13, 2022
  93. achow101 force-pushed on Jul 18, 2022
  94. achow101 force-pushed on Jul 19, 2022
  95. DrahtBot added the label Needs rebase on Jul 27, 2022
  96. achow101 force-pushed on Jul 28, 2022
  97. DrahtBot removed the label Needs rebase on Jul 28, 2022
  98. DrahtBot added the label Needs rebase on Aug 1, 2022
  99. achow101 force-pushed on Aug 1, 2022
  100. DrahtBot removed the label Needs rebase on Aug 1, 2022
  101. achow101 force-pushed on Sep 20, 2022
  102. DrahtBot added the label Needs rebase on Oct 20, 2022
  103. achow101 force-pushed on Oct 21, 2022
  104. DrahtBot removed the label Needs rebase on Oct 21, 2022
  105. achow101 force-pushed on Nov 30, 2022
  106. DrahtBot added the label Needs rebase on Dec 9, 2022
  107. achow101 force-pushed on Dec 9, 2022
  108. DrahtBot removed the label Needs rebase on Dec 9, 2022
  109. DrahtBot added the label Needs rebase on Jan 17, 2023
  110. achow101 force-pushed on Jan 18, 2023
  111. DrahtBot removed the label Needs rebase on Jan 18, 2023
  112. in src/psbt.cpp:23 in be9e558f3e outdated 
    19@@ -20,6 +20,14 @@ PartiallySignedTransaction::PartiallySignedTransaction(const CMutableTransaction
20     SetupFromTx(tx);
21 }
22 
23+PartiallySignedTransaction::PartiallySignedTransaction(uint32_t version) :
    instagibbs commented at 2:52 pm on January 27, 2023:
    does this constructor ever get used? I don’t see it being used
    achow101 commented at 9:00 pm on January 27, 2023:
    Dropped that commit.
  113. in src/wallet/rpc/spend.cpp:1698 in d4f0cdc8e9 outdated 
    1694+    }
1695+    if (psbt_version != 2 && psbt_version != 0) {
1696+        throw JSONRPCError(RPC_INVALID_PARAMETER, "The PSBT version can only be 2 or 0");
1697+    }
1698+
1699+    PartiallySignedTransaction psbtx(rawTx, /* version=*/ 2);
    instagibbs commented at 2:59 pm on January 27, 2023:
    should this be using psbt_version?
    achow101 commented at 9:00 pm on January 27, 2023:
    Yes, done
  114. in src/psbt.h:1172 in d4f0cdc8e9 outdated 
    1178-     * @param[in] input_index Index of the input to retrieve the UTXO of
1179-     * @return Whether the UTXO for the specified input was found
1180-     */
1181-    bool GetInputUTXO(CTxOut& utxo, int input_index) const;
1182+    PartiallySignedTransaction(uint32_t version);
1183+    explicit PartiallySignedTransaction(const CMutableTransaction& tx, uint32_t version = 0);
    instagibbs commented at 3:01 pm on January 27, 2023:
    If we’re internally defaulting to v2, probably better just to have constructor default for that value as well?
    achow101 commented at 9:00 pm on January 27, 2023:
    Added a commit changing it. It’s 0 when introduced in order to not break everything.
  115. in src/rpc/rawtransaction.cpp:1813 in d4f0cdc8e9 outdated 
    1784 
1785     // Create a blank psbt where everything will be added
1786     PartiallySignedTransaction merged_psbt;
1787+    merged_psbt.tx_version = best_version;
1788+    merged_psbt.fallback_locktime = best_locktime;
1789+    // TODO: Remove for PSBTv2
    instagibbs commented at 3:05 pm on January 27, 2023:
    this function is only for v0, why would we remove something for v2?
    achow101 commented at 8:40 pm on January 27, 2023:
    The intent is to enable this rpc for v2 in the future.
    instagibbs commented at 9:41 pm on January 27, 2023:
    ah, wasn’t clear to me
  116. in src/rpc/rawtransaction.cpp:1795 in d4f0cdc8e9 outdated 
    1762         PartiallySignedTransaction psbtx;
1763         std::string error;
1764         if (!DecodeBase64PSBT(psbtx, txs[i].get_str(), error)) {
1765             throw JSONRPCError(RPC_DESERIALIZATION_ERROR, strprintf("TX decode failed %s", error));
1766         }
1767+        if (psbtx.GetVersion() != 0) {
    instagibbs commented at 3:07 pm on January 27, 2023:
    Please add a test for this case
    achow101 commented at 8:42 pm on January 27, 2023:
    One was added in 318d818df7de30ac00551b655640512598ac1595
  117. achow101 force-pushed on Jan 27, 2023
  118. instagibbs commented at 2:42 pm on January 30, 2023: member

    Looking good so far.

    Ran this against CLN test suite and got no unexpected behavior.

  119. DrahtBot added the label Needs rebase on Feb 16, 2023
  120. achow101 force-pushed on Feb 17, 2023
  121. DrahtBot removed the label Needs rebase on Feb 17, 2023
  122. DrahtBot added the label Needs rebase on Mar 16, 2023
  123. achow101 force-pushed on Mar 17, 2023
  124. DrahtBot removed the label Needs rebase on Mar 17, 2023
  125. in src/rpc/rawtransaction.cpp:1797 in 6433fe1d16 outdated 
    1769@@ -1770,6 +1770,9 @@ static RPCHelpMan joinpsbts()
1770         if (!DecodeBase64PSBT(psbtx, txs[i].get_str(), error)) {
1771             throw JSONRPCError(RPC_DESERIALIZATION_ERROR, strprintf("TX decode failed %s", error));
1772         }
1773+        if (psbtx.GetVersion() != 0) {
1774+            throw JSONRPCError(RPC_INVALID_PARAMETER, "joinpsbts only operates on version 0 PSBTs");
1775+        }
    unknown commented at 7:43 pm on April 12, 2023:
    Is there a reason for this restriction?
    achow101 commented at 7:51 pm on April 12, 2023:
    IIRC updating this RPC would require pretty significant changes to it that felt out of scope for this PR. I’ve left it as something to do in a followup in order to keep the scope of the PR limited to just the bare minimum to work with PSBTv2.
  126. DrahtBot added the label Needs rebase on Apr 21, 2023
  127. achow101 force-pushed on May 1, 2023
  128. achow101 force-pushed on May 1, 2023
  129. DrahtBot removed the label Needs rebase on May 1, 2023
  130. DrahtBot added the label CI failed on May 23, 2023
  131. achow101 force-pushed on May 27, 2023
  132. DrahtBot removed the label CI failed on May 27, 2023
  133. DrahtBot added the label Needs rebase on Jun 1, 2023
  134. achow101 force-pushed on Jun 1, 2023
  135. DrahtBot removed the label Needs rebase on Jun 1, 2023
  136. DrahtBot added the label CI failed on Aug 6, 2023
  137. maflcko commented at 9:28 am on August 17, 2023: member
    Needs rebase, if still relevant.
  138. DrahtBot added the label Needs rebase on Aug 17, 2023
  139. achow101 force-pushed on Aug 17, 2023
  140. DrahtBot removed the label Needs rebase on Aug 17, 2023
  141. DrahtBot removed the label CI failed on Aug 18, 2023
  142. DrahtBot added the label Needs rebase on Sep 12, 2023
  143. achow101 force-pushed on Sep 12, 2023
  144. DrahtBot removed the label Needs rebase on Sep 12, 2023
  145. DrahtBot added the label Needs rebase on Oct 2, 2023
  146. achow101 force-pushed on Oct 3, 2023
  147. DrahtBot removed the label Needs rebase on Oct 3, 2023
  149. DrahtBot added the label Needs rebase on Oct 16, 2023
  150. achow101 force-pushed on Oct 16, 2023
  151. DrahtBot removed the label Needs rebase on Oct 16, 2023
  152. DrahtBot added the label Needs rebase on Oct 25, 2023
  153. achow101 force-pushed on Oct 25, 2023
  154. DrahtBot removed the label Needs rebase on Oct 25, 2023
  155. DrahtBot added the label Needs rebase on Nov 15, 2023
  156. achow101 force-pushed on Nov 15, 2023
  157. DrahtBot removed the label Needs rebase on Nov 15, 2023
  158. DrahtBot added the label CI failed on Nov 27, 2023
  159. maflcko commented at 1:29 pm on November 27, 2023: member 
    0psbt.cpp: In member function ‘CMutableTransaction PartiallySignedTransaction::GetUnsignedTx() const’:
1psbt.cpp:107:35: error: ‘transaction_identifier<has_witness>::transaction_identifier(const uint256&) [with bool has_witness = false]’ is private within this context
2  107 |         txin.prevout.hash = input.prev_txid;
3      |                                   ^~~~~~~~~
  160. achow101 force-pushed on Nov 27, 2023
  161. DrahtBot removed the label CI failed on Nov 28, 2023
  162. DrahtBot added the label Needs rebase on Dec 11, 2023
  163. achow101 force-pushed on Dec 11, 2023
  164. DrahtBot removed the label Needs rebase on Dec 11, 2023
  165. DrahtBot added the label CI failed on Jan 24, 2024
  166. achow101 commented at 3:00 pm on April 9, 2024: member
    Will split this up
  167. DrahtBot added the label Needs rebase on Apr 22, 2024
  168. achow101 force-pushed on Apr 25, 2024
  169. DrahtBot removed the label Needs rebase on Apr 25, 2024
  170. DrahtBot removed the label CI failed on Apr 26, 2024
  171. DrahtBot added the label Needs rebase on May 23, 2024
  172. achow101 force-pushed on May 29, 2024
  173. DrahtBot removed the label Needs rebase on May 29, 2024
  174. DrahtBot added the label Needs rebase on Jun 12, 2024
  175. achow101 force-pushed on Jun 13, 2024
  176. achow101 force-pushed on Jun 13, 2024
  177. DrahtBot removed the label Needs rebase on Jun 13, 2024
  178. DrahtBot added the label Needs rebase on Jul 8, 2024
  179. achow101 force-pushed on Jul 10, 2024
  180. DrahtBot removed the label Needs rebase on Jul 10, 2024
  181. DrahtBot added the label Needs rebase on Jul 11, 2024
  182. achow101 force-pushed on Jul 22, 2024
  183. DrahtBot removed the label Needs rebase on Jul 22, 2024
  184. hebasto added the label Needs CMake port on Aug 16, 2024
  185. DrahtBot added the label Needs rebase on Sep 2, 2024
  186. maflcko removed the label GUI on Sep 3, 2024
  187. maflcko removed the label Needs CMake port on Sep 3, 2024
  188. achow101 force-pushed on Sep 3, 2024
  189. DrahtBot removed the label Needs rebase on Sep 3, 2024
  190. in src/psbt.cpp:130 in ce9d05fa4f outdated 
    127+
128+    // Get the unsigned transaction
129+    CMutableTransaction mtx = GetUnsignedTx();
130+    // Compute the locktime
131+    bool locktime_success = ComputeTimeLock(mtx.nLockTime);
132+    assert(locktime_success);
    tcharding commented at 11:21 pm on September 18, 2024:

    Is there a reason to compute the locktime again? It is done in the call to GetUnsignedTx just above.

    (In 0cf1a9febb0205dadc03cd87a7af074e7b4cfd8c)

    achow101 commented at 0:27 am on September 19, 2024:
    Good point, removed.
  191. in src/rpc/rawtransaction.cpp:1801 in ce9d05fa4f outdated 
    1797@@ -1740,40 +1798,46 @@ static RPCHelpMan joinpsbts()
1798         throw JSONRPCError(RPC_INVALID_PARAMETER, "At least two PSBTs are required to join PSBTs.");
1799     }
1800 
1801-    uint32_t best_version = 1;
1802+    int32_t best_version = 1;
    tcharding commented at 11:30 pm on September 18, 2024:
    Why change to a signed int? IIUC this is checked against the version field in transaction which is unsigned.
    achow101 commented at 0:27 am on September 19, 2024:

    It was unsigned when this PR was written 3 years ago lol.

    Updated to be unsigned.

    tcharding commented at 0:45 am on September 19, 2024:
    Nice!
  192. in src/wallet/rpc/spend.cpp:98 in ce9d05fa4f outdated 
     97@@ -98,7 +98,7 @@ std::set<int> InterpretSubtractFeeFromOutputInstructions(const UniValue& sffo_in
 98 static UniValue FinishTransaction(const std::shared_ptr<CWallet> pwallet, const UniValue& options, const CMutableTransaction& rawTx)
 99 {
100     // Make a blank psbt
101-    PartiallySignedTransaction psbtx(rawTx);
102+    PartiallySignedTransaction psbtx(rawTx, /*version=*/2);
    tcharding commented at 11:40 pm on September 18, 2024:
    Whitespace is different here compared to below (mtx, /* version=*/ 2). I don’t know if you guys worry about that sort of thing.
    achow101 commented at 0:28 am on September 19, 2024:
    Fixed the whitespace to be consistent with our style guidelines
  193. in src/psbt.h:1175 in ce9d05fa4f outdated 
    1181-     * @param[out] utxo The UTXO of the input if found
1182-     * @param[in] input_index Index of the input to retrieve the UTXO of
1183-     * @return Whether the UTXO for the specified input was found
1184-     */
1185-    bool GetInputUTXO(CTxOut& utxo, int input_index) const;
1186+    explicit PartiallySignedTransaction(const CMutableTransaction& tx, uint32_t version = 2);
    tcharding commented at 11:46 pm on September 18, 2024:
    In 7d46c2488679ccca70cdd1dc71100724347a47e8 this is a pretty big change behavour, right? I would have expected more in the patch that just a one line change (eg docs somewhere or perhaps a test somewhere that checks default behavour).
    achow101 commented at 0:29 am on September 19, 2024:

    I’ve added a basic test for this and a release note fragment. The bulk of the test changes in previous commits is to setup for this though.

    Changing to PSBTv2 is should largely be transparent to the user, so there shouldn’t be many changes in the functional tests. Most of the testing for it is in the unit tests.

    tcharding commented at 0:45 am on September 19, 2024:
    Cheers
  194. tcharding commented at 11:47 pm on September 18, 2024: contributor

    Caveat: I’m not a C++ dev and don’t know much about Core development.

    I did however check the changes here against my Rust implementation of the same, and found some bugs in mine, so cheers!

  195. achow101 force-pushed on Sep 19, 2024
  196. DrahtBot added the label CI failed on Nov 1, 2024
  197. DrahtBot removed the label CI failed on Nov 1, 2024
  198. Sjors commented at 10:22 am on January 16, 2025: member
    Aside from the existential question, it would be useful to have some projects to test this PR against: https://bitcoin.stackexchange.com/questions/125384/who-uses-or-wants-to-use-psbtv2-bip370
  199. tcharding commented at 10:18 pm on January 16, 2025: contributor
    cc @dangould
  200. whitslack commented at 0:19 am on January 17, 2025: contributor
    @Sjors: You want to know who wants this? I’ve been wanting to use Bitcoin Core to decode and manipulate the PSBTs that Core Lightning stores in its database, but it only works for the older, first-gen PSBTs. Core Lightning moved on to PSBTv2 some time ago, so now all the newer PSBTs fail to parse in Bitcoin Core. I’ve been having to hack together various half-baked tools I’ve found around the Internet, but it would be so good if Bitcoin Core would be upgraded to the current spec.
  201. instagibbs commented at 0:36 am on January 17, 2025: member
    @whitslack yes I authored the migration, and v2<->v0 roundtripping is supported in libwally as well. Would be nice to move the industry a bit closer to making v2 the defacto default.
  202. Sjors commented at 8:50 am on January 17, 2025: member
    @whitslack @instagibbs thanks for mentioning CLN. Is it possible to turn off its round tripping so I can test this PR against it? And are there any good nodes on signet or testnet4?
  203. Sjors commented at 10:14 am on January 17, 2025: member
    @bigspider thanks for your reply on the mailinglist. So I guess another way to test this PR is to patch HWI to not do the conversion between PSBT 0 and 2, and then make (any?) transaction with a Ledger that runs at least 2.0.0.
  204. bigspider commented at 1:08 pm on January 17, 2025: none

    @bigspider thanks for your reply on the mailinglist. So I guess another way to test this PR is to patch HWI to not do the conversion between PSBT 0 and 2, and then make (any?) transaction with a Ledger that runs at least 2.0.0.

    I suppose so, although it feels somewhat circular as bitcoin-core was always the reference for the tests of the Ledger Bitcoin app!

  205. scgbckbone commented at 2:34 pm on January 17, 2025: none

    Coldcard Mk4/Q also support PSBT v2 if you need to test ( from October 2023 https://github.com/Coldcard/firmware/blob/master/releases/History-Mk4.md#520---2023-10-10)

    but as @bigspider, I also used core PR to implement this

  206. Sjors commented at 7:12 pm on January 17, 2025: member

    IIUC the recently merged Silent Payments PSBT BIP only works with PSBTv2? https://github.com/bitcoin/bips/blob/master/bip-0375.mediawiki

    Having a (rough) draft PR implementing those fields on top of this PR could also aid in review. cc @josibake @andrewtoth

  207. jonatack commented at 7:33 pm on January 17, 2025: member

    IIUC the recently merged Silent Payments PSBT BIP only works with PSBTv2?

    Suggested to consider renaming BIP375 to be more clear about this, unless there’s a reason I’m missing: https://github.com/bitcoin/bips/pull/1687#discussion_r1913779155

  208. Sjors commented at 12:36 pm on January 20, 2025: member

    I briefly tested a Ledger NanoS with Bitcoin Testnet app 2.2.5 by disabling its v0 <-> v2 conversion: https://github.com/Sjors/HWI/commit/28bfb53e136918e5c6279b6bc6ceb33e1421b934

    On master this triggers a “failed to sign” error. With this PR (rebased) it works.

    it feels somewhat circular

    Agreed, an independent implementation would be nice to test against.

    (I also read BIP 370 again, so should be in a better position now to review this PR)

  209. instagibbs commented at 6:14 pm on January 23, 2025: member

    Is it possible to turn off its round tripping so I can test this PR against it?

    Internally it only uses v2, but it will output v0 if you ask for it.

  210. Sjors commented at 8:20 am on January 24, 2025: member

    The fundchannel_start -> fundchannel_complete id psbt workflow is the easiest to test. If I give it a v2 PSBT, it’s not going to be smart and try to fix encoding mistakes?

    Tested that flow, and also the sendpsbt RPC.

    So this PR makes the GUI produce v2 PSBTs by default. We might want to change that, or at least have a downgrade checkbox. Similarly the send RPC and friends should have a way to force a lower version.

    Here’s a branch that:

    • makes v0 the GUI default: it’s too much work to add a checkbox everywhere. Later on for Silent Payments we can just make v2 the default if an sp address is involved.
    • adds a psbt_version option to send, sendall, converttopsbt, bumpfee and psbtbumpfee

    https://github.com/Sjors/bitcoin/tree/2025/01/psbtv2

  211. instagibbs commented at 2:17 pm on January 24, 2025: member

    If I give it a v2 PSBT, it’s not going to be smart and try to fix encoding mistakes?

    That would be a libwally question. I don’t recall.

  212. Aldocapurro approved
  213. Aldocapurro approved
  214. in test/functional/data/rpc_psbt.json:197 in 09c8656aaa outdated 
    155@@ -156,10 +156,10 @@
156         },
157         {
158             "combine" : [
159-                "cHNidP8BAD8CAAAAAf//////////////////////////////////////////AAAAAAD/////AQAAAAAAAAAAA2oBAAAAAAAKDwECAwQFBgcICQ8BAgMEBQYHCAkKCwwNDg8ACg8BAgMEBQYHCAkPAQIDBAUGBwgJCgsMDQ4PAAoPAQIDBAUGBwgJDwECAwQFBgcICQoLDA0ODwA=",
160-                "cHNidP8BAD8CAAAAAf//////////////////////////////////////////AAAAAAD/////AQAAAAAAAAAAA2oBAAAAAAAKDwECAwQFBgcIEA8BAgMEBQYHCAkKCwwNDg8ACg8BAgMEBQYHCBAPAQIDBAUGBwgJCgsMDQ4PAAoPAQIDBAUGBwgQDwECAwQFBgcICQoLDA0ODwA="
161+                "cHNidP8BAD8CAAAAAf//////////////////////////////////////////AAAAAAD/////AQAAAAAAAAAAA2oBAAAAAAAK8AECAwQFBgcICQ8BAgMEBQYHCAkKCwwNDg8ACvABAgMEBQYHCAkPAQIDBAUGBwgJCgsMDQ4PAArwAQIDBAUGBwgJDwECAwQFBgcICQoLDA0ODwA=",
162+                "cHNidP8BAD8CAAAAAf//////////////////////////////////////////AAAAAAD/////AQAAAAAAAAAAA2oBAAAAAAAK8AECAwQFBgcIEA8BAgMEBQYHCAkKCwwNDg8ACvABAgMEBQYHCBAPAQIDBAUGBwgJCgsMDQ4PAArwAQIDBAUGBwgQDwECAwQFBgcICQoLDA0ODwA="
    Sjors commented at 10:19 am on January 28, 2025:
    09c8656aaa27d18e2d14a52cdd67ee46f6a5cae6: note to other reviewers: you can run both through decodepsbt and notice the “unknown” key f0010203040506070809 in the first vector, f0010203040506070810 in the second and both appear in the combined “result” below.
  215. in src/psbt.h:1274 in 6a73e374fb outdated 
    1218@@ -1063,6 +1219,10 @@ struct PartiallySignedTransaction
1219 
1220         // Read global data
1221         bool found_sep = false;
1222+        uint64_t input_count = 0;
1223+        uint64_t output_count = 0;
1224+        bool found_input_count = false;
    Sjors commented at 10:26 am on January 28, 2025:

    6a73e374fb0835dd06433e079c7c9300d6412a11 nit: variable 'found_input_count' set but not used [-Werror,-Wunused-but-set-variable]

    Same for found_output_count.

    The “test-each-commit” job doesn’t catch this, because it’s limited to the last 6 commits.

    Although PSBT_GLOBAL_INPUT_COUNT case does set it, it’s not enough for the compiler to be happy. It wants you to introduce these variables in c6743bf7137615eb20145c6eb3e2da5c82903b1e. Doing so also increases clarity imo.

    Fwiw all later commits compile fine.

  216. in src/psbt.h:635 in 6a73e374fb outdated 
    613@@ -584,6 +614,70 @@ struct PSBTInput
614                     hash256_preimages.emplace(hash, std::move(preimage));
615                     break;
616                 }
617+                case PSBT_IN_PREVIOUS_TXID:
618+                {
619+                    if (!key_lookup.emplace(key).second) {
620+                        throw std::ios_base::failure("Duplicate Key, previous txid is already provided");
621+                    } else if (key.size() != 1) {
622+                        throw std::ios_base::failure("Previous txid key is more than one byte type");
    Sjors commented at 1:53 pm on January 28, 2025:
    6a73e374fb0835dd06433e079c7c9300d6412a11: as I noted in #31247 (review) it would be really nice if we can abstract this duplicate and error prone size checking stuff into e.g. a DSL that defines it for all fields.
    achow101 commented at 11:15 pm on January 31, 2025:
    This PR is big enough already, I’d prefer to do that separately.
    Sjors commented at 9:42 am on February 3, 2025:
    Agreed.
  217. in src/psbt.h:639 in 6a73e374fb outdated 
    619+                    if (!key_lookup.emplace(key).second) {
620+                        throw std::ios_base::failure("Duplicate Key, previous txid is already provided");
621+                    } else if (key.size() != 1) {
622+                        throw std::ios_base::failure("Previous txid key is more than one byte type");
623+                    }
624+                    UnserializeFromVector(s, prev_txid);
    Sjors commented at 2:09 pm on January 28, 2025:

    6a73e374fb0835dd06433e079c7c9300d6412a11: IIUC UnserializeFromVector checks that the first byte of s represents a size and that it matches the number of bytes left in in s.

    So that protects against passing in too many bytes. But does it protect against too few bytes?

    The UnserializeMany method uses std::make_shared which calls the constructor for Txid (in this case), which in return, I think, picks the uint256(Span<const unsigned char> vch) constructor. Which doesn’t check the length.

    Maybe UnserializeMany could do a sizeof on its args?

    achow101 commented at 11:27 pm on January 31, 2025:

    No… that’s not at all how deserialization works. There’s no constructors called, prev_txid is an already initialized uint256. Unserializing into it means that the data is overwritten. There’s no std::make_shared called anywhere, nothing is constructed or initialized.

    The call chain is UnserializeFromVector -> UnserializeMany -> uint256::Unserialize() -> Unserialize(Stream& s, Span<B> span) -> <template Stream> read(), and every read(Span) function checks that the stream has at least span.size() bytes, and reads exactly span.size() bytes into the span.

    Unserializing uint256 always reads exactly 32 bytes, that’s why no hashes ever need to be length prefixed. The same applies here.

    Sjors commented at 9:44 am on February 3, 2025:
    Thanks for clarifying. So in that case both too many and too few bytes are prevented.
  218. Sjors commented at 2:12 pm on January 28, 2025: member
    Some feedback on the first three commits.
  219. Aldocapurro approved
  220. bitcoin deleted a comment on Jan 30, 2025
  221. rkrux commented at 10:45 am on February 6, 2025: none

    Definite Concept ACK

    This one is a biggie, I should get around to reviewing this PR sometime later this month and will leave a review then.

  222. in src/psbt.h:844 in 2dc96bd4dc outdated 
    840     bool IsNull() const;
841     void FillSignatureData(SignatureData& sigdata) const;
842     void FromSignatureData(const SignatureData& sigdata);
843     void Merge(const PSBTOutput& output);
844-    PSBTOutput() = default;
845+    PSBTOutput(uint32_t version) : m_psbt_version(version) {}
    Sjors commented at 10:15 am on February 7, 2025:
    In 2dc96bd4dc5169f692b94ae98eeb6bcd6b0f1ee9 “Have PSBTInput and PSBTOutput know the PSBT’s version”, nit: calling this param psbt_version makes it clear that inputs and outputs don’t have their own versioning. And consistent with the eventual variable name.
    achow101 commented at 7:59 pm on February 10, 2025:
    Done
  223. in src/psbt.h:1411 in 2dc96bd4dc outdated 
    1407@@ -1404,10 +1408,12 @@ struct PartiallySignedTransaction
1408             throw std::ios_base::failure("No unsigned transaction was provided");
1409         }
1410 
1411+        uint32_t psbt_ver = GetVersion();
    Sjors commented at 10:18 am on February 7, 2025:
    In https://github.com/bitcoin/bitcoin/commit/2dc96bd4dc5169f692b94ae98eeb6bcd6b0f1ee9 “Have PSBTInput and PSBTOutput know the PSBT’s version”, μnit const.
    achow101 commented at 8:00 pm on February 10, 2025:
    Done
  224. in src/rpc/rawtransaction.cpp:1588 in 2dc96bd4dc outdated 
    1584@@ -1585,10 +1585,10 @@ static RPCHelpMan createpsbt()
1585     PartiallySignedTransaction psbtx;
1586     psbtx.tx = rawTx;
1587     for (unsigned int i = 0; i < rawTx.vin.size(); ++i) {
1588-        psbtx.inputs.emplace_back();
1589+        psbtx.inputs.emplace_back(0);
    Sjors commented at 10:23 am on February 7, 2025:
    In 2dc96bd4dc5169f692b94ae98eeb6bcd6b0f1ee9 “Have PSBTInput and PSBTOutput know the PSBT’s version”, nit: /*psbt_version=*/ would be useful here, but this goes away in a later commit (and before the default is changed to 2).
  225. in src/test/fuzz/deserialize.cpp:200 in 2dc96bd4dc outdated 
    196@@ -197,11 +197,11 @@ FUZZ_TARGET_DESERIALIZE(prefilled_transaction_deserialize, {
197     DeserializeFromFuzzingInput(buffer, prefilled_transaction);
198 })
199 FUZZ_TARGET_DESERIALIZE(psbt_input_deserialize, {
200-    PSBTInput psbt_input;
201+    PSBTInput psbt_input(0);
    Sjors commented at 10:27 am on February 7, 2025:

    In 2dc96bd4dc5169f692b94ae98eeb6bcd6b0f1ee9 “Have PSBTInput and PSBTOutput know the PSBT’s version”: somewhere later in the PR, we should add a target for version 2?

    Or better: any version, which would detect that the PSBTInput constructor doesn’t prevent version 1 (not currently a problem, but could creep in if we build functionality to add inputs to a PSBT).

  226. in src/psbt.h:1503 in c6743bf713 outdated 
    1498@@ -1419,10 +1499,14 @@ struct PartiallySignedTransaction
1499 
1500             // Make sure the non-witness utxo matches the outpoint
1501             if (input.non_witness_utxo) {
1502-                if (input.non_witness_utxo->GetHash() != tx->vin[i].prevout.hash) {
1503+                if ((tx != std::nullopt && input.non_witness_utxo->GetHash() != tx->vin[i].prevout.hash)
1504+                    || (!input.prev_txid.IsNull() && input.non_witness_utxo->GetHash() != input.prev_txid)
    Sjors commented at 11:19 am on February 7, 2025:
    In c6743bf7137615eb20145c6eb3e2da5c82903b1e “Enforce PSBT version constraints”: this is hard to read. Would prefer two separate checks based on the PSBT version.
    achow101 commented at 8:00 pm on February 10, 2025:
    Done
  227. in src/psbt.cpp:17 in cef9de00f4 outdated 
    13@@ -14,6 +14,7 @@ PartiallySignedTransaction::PartiallySignedTransaction(const CMutableTransaction
14 {
15     inputs.resize(tx.vin.size(), PSBTInput(GetVersion()));
16     outputs.resize(tx.vout.size(), PSBTOutput(GetVersion()));
17+    CacheUnsignedTxPieces();
    Sjors commented at 11:28 am on February 7, 2025:
    In cef9de00f4c2d4c9c033e7dce2b7ffac2e694a26 “Call CacheUnsignedTxPieces in PSBT constructor”: might as well do this in the previous commit. Ditto for the next commit.
    achow101 commented at 8:00 pm on February 10, 2025:
    Done
  228. in src/psbt.h:1170 in 0c459e43ba outdated 
    1157@@ -1158,6 +1158,8 @@ struct PartiallySignedTransaction
1158     [[nodiscard]] bool Merge(const PartiallySignedTransaction& psbt);
1159     bool AddInput(const CTxIn& txin, PSBTInput& psbtin);
1160     bool AddOutput(const CTxOut& txout, const PSBTOutput& psbtout);
1161+    void SetupFromTx(const CMutableTransaction& tx);
1162+    void CacheUnsignedTxPieces();
    Sjors commented at 11:33 am on February 7, 2025:

    In 0c459e43ba1ce74d2e88dde352e28a63743f4746 “Add PSBT::CacheUnsignedTxPieces”: this seems fine, but it would be good to have a method that verifies that for v0 PSBTs these two representations are in sync. You could then enable that method in debug builds and call it in a bunch of places.

    The could e.g call GetUnsignedTx() (introduced later), skip its if (tx != std::nullopt) early return, and compare.

  229. in src/psbt.cpp:115 in 162d091677 outdated 
    109@@ -110,6 +110,21 @@ CMutableTransaction PartiallySignedTransaction::GetUnsignedTx() const
110     return mtx;
111 }
112 
113+uint256 PartiallySignedTransaction::GetUniqueID() const
114+{
115+    if (tx != std::nullopt) {
    Sjors commented at 11:40 am on February 7, 2025:

    In 162d0916774c719f1e3749b0d4d204e296ee4ac8 “Add PSBT::GetUniqueID”: I think this should check the version number, and then Assume(version == 0 || tx == std::nullopt)

    The way it’s written now suggests tx is just a cache that might be stale.

    achow101 commented at 8:00 pm on February 10, 2025:
    Done
  230. in src/psbt.cpp:140 in aeb9e44bc5 outdated 
    142-    psbtin.final_script_witness.SetNull();
143-    inputs.push_back(psbtin);
144-    return true;
145+
146+    if (tx != std::nullopt) {
147+        // This is a v0 psbt, so do the v0 AddInput
    Sjors commented at 11:51 am on February 7, 2025:
    In aeb9e44bc5a221553d2010752c76f8f16205551c “Change PSBT::AddInput to take just PSBTInput”: I think it’s more readable to just check the PSBT version directly.
    achow101 commented at 8:00 pm on February 10, 2025:
    Done
  231. in src/psbt.cpp:152 in aeb9e44bc5 outdated 
    155+        psbtin.final_script_witness.SetNull();
156+        inputs.push_back(psbtin);
157+        return true;
158+    }
159+
160+    // TODO: Do PSBTv2
    Sjors commented at 11:53 am on February 7, 2025:
    In https://github.com/bitcoin/bitcoin/commit/aeb9e44bc5a221553d2010752c76f8f16205551c “Change PSBT::AddInput to take just PSBTInput”: assert(false) ?
    achow101 commented at 8:00 pm on February 10, 2025:
    Done
  232. in src/psbt.cpp:142 in aeb9e44bc5 outdated 
    144-    return true;
145+
146+    if (tx != std::nullopt) {
147+        // This is a v0 psbt, so do the v0 AddInput
148+        CTxIn txin(COutPoint(psbtin.prev_txid, *psbtin.prev_out));
149+        if (std::find(tx->vin.begin(), tx->vin.end(), txin) != tx->vin.end()) {
    Sjors commented at 11:56 am on February 7, 2025:
    In https://github.com/bitcoin/bitcoin/commit/aeb9e44bc5a221553d2010752c76f8f16205551c “Change PSBT::AddInput to take just PSBTInput”: could add // Prevent duplicate input
    achow101 commented at 8:00 pm on February 10, 2025:
    Done
  233. in src/psbt.cpp:131 in aeb9e44bc5 outdated 
    124@@ -125,17 +125,32 @@ uint256 PartiallySignedTransaction::GetUniqueID() const
125     return mtx.GetHash();
126 }
127 
128-bool PartiallySignedTransaction::AddInput(const CTxIn& txin, PSBTInput& psbtin)
129+bool PartiallySignedTransaction::AddInput(PSBTInput& psbtin)
130 {
131-    if (std::find(tx->vin.begin(), tx->vin.end(), txin) != tx->vin.end()) {
132+    if (std::find_if(inputs.begin(), inputs.end(),
133+        [psbtin](const PSBTInput& psbt) {
    Sjors commented at 12:01 pm on February 7, 2025:

    In https://github.com/bitcoin/bitcoin/commit/aeb9e44bc5a221553d2010752c76f8f16205551c “Change PSBT::AddInput to take just PSBTInput”:

    0// Avoid duplicate input
1[psbtin](const PSBTInput& psbt_input) {

    (the comment about duplicate inputs is introduced in 3fe17931ae2d0e1b83cbe9c376f5d255ac9de68d “Implement PSBTv2 AddInput and AddOutput”)

    But this only works for v2, since prev_txid is nullptr otherwise. That’s why the duplicate input check for v0 is maintained below.

    It’s more clear if you explicitly check the PSBT version in both places.

    achow101 commented at 8:00 pm on February 10, 2025:
    Done
  234. in src/psbt.cpp:33 in a79e519323 outdated 
    29@@ -30,6 +30,7 @@ bool PartiallySignedTransaction::Merge(const PartiallySignedTransaction& psbt)
30         return false;
31     }
32 
33+    assert(*tx_version == psbt.tx_version);
    Sjors commented at 12:06 pm on February 7, 2025:
    In a79e519323ee35544e854efd6cbf7ce8577fd4e3 “Implement PSBTv2 field merging”: Assume(...) || return false;?
    achow101 commented at 8:00 pm on February 10, 2025:
    Done
  235. Sjors commented at 12:19 pm on February 7, 2025: member

    Studied the other commits as well.

    Perhaps there’s an opportunity to split this PR by first introducing the internal refactoring that anticipates PSBTv2. But other that I don’t think it can be made much smaller.

  236. Define psbtv2 field numbers 36669995bd
  237. Change PSBT unknown fields test to use higher numbers 
    Previously these tests were using 0x0f as the field number. Changed to
use 0xf0 instead as it is unlikely we will hit that anytime soon.
    7e7fc1c45e
  238. Implement PSBTv2 fields de/ser b8697f7cf6
  239. Have PSBTInput and PSBTOutput know the PSBT's version 5e60bd68a0
  240. Enforce PSBT version constraints 
    With PSBTv2, some fields are not allowed in PSBTv2, and some are
required. Enforce those.
    ec87d48292
  241. Add PSBT::CacheUnsignedTxPieces 
    Fetches the PSBTv2 fields from PSBTv0's global unsigned tx. This allows
us to pretend everything internally is a PSBTv2 and makes things easier
to work with.
    4c96da90ac
  242. Replace PSBT::GetInputUTXO with PSBTInput::GetUTXO 
    Now that PSBTInput's track their own prevouts, there's no need for a
PSBT global function to fetch input specific data.
    7d4d768554
  243. Add PSBT::ComputeLockTime() 
    Function to compute the lock time for the transaction
    d670dbfc10
  244. Add PSBT::GetUnsignedTx 
    A helper function for getting the unsigned transaction regardless of
psbt version.
    962490ce1d
  245. Add PSBT::GetUniqueID 
    The unique ID for PSBTv2 is different from v0. Use this function to get
the ID without requiring the caller to know the version number.
    b0e30bbdca
  246. Change PSBT::AddInput to take just PSBTInput 28066c39f1
  247. Change PSBT::AddOutput to take just PSBTOutput 6c003b18aa
  248. Implement PSBTv2 field merging 078ef612c9
  249. Add PSBTInput::GetOutPoint 
    Helper for getting the PSBTInput COutPoint
    8037eb704b
  250. Update SignPSBTInput for PSBTv2 76178c2664
  251. Change PrecomputePSBTData to use GetUnsignedTx e842ace5f3
  252. Update FinalizeAndExtract for v2 df50eed67e
  253. Update AnalyzePSBT for PSBTv2 8992a213a2
  254. Update PSBT Operations Dialog for v2 2b6d18b1aa
  255. Update RPCs for PSBTv2 7d890eb8bd
  256. Update wallet for PSBTv2 b3b21a1758
  257. Implement PSBTv2 AddInput and AddOutput 662ae838e4
  258. Update PSBTInputSignedAndVerified for PSBTv2 71439d14e8
  259. Update test_framework/psbt.py for PSBTv2 961442e342
  260. Implement PSBTv2 in decodepsbt bc573a4c2a
  261. Allow specifying PSBT version in constructor 66558b5bf7
  262. Update PSBT::UpdatePSBTOutput to use GetUnsignedTx 33cd69932d
  263. Restrict joinpsbts to PSBTv0 only 13f4ae61d2
  264. Allow createpsbt and walletcreatefundedpsbt to take psbt version 
    Use v2 for other RPCs. For some tests to work, PSBTv0 is set explicitly.
    a530dbb745
  265. Use GetUnsignedTx when serializing in PSBTv0 
    If we are asked to make a PSBTv0, we may not necessarily have made an
unsigned transaction. So instead use GetUnsignedTx which will either
fetch one that already exists, or construct a new one from the stored
data. Internally we may be storing a PSBTv0 like a PSBTv2, but still
want to serialize those as v0.
    07f62d0dab
  266. psbt: Change default psbt version to 2 4d981352c4
  267. tests: Add test vectors from BIP 370 135c8be73f
  268. tests: Add PSBT unit test for ComputeTimeLock a487dee098
  269. doc: Release notes for psbtv2 1137807299
  270. achow101 force-pushed on Feb 10, 2025


achow101 DrahtBot adamjonas sanket1729 kiminuo instagibbs maflcko scgbckbone tcharding Sjors whitslack bigspider jonatack Aldocapurro rkrux

Labels
Wallet RPC/REST/ZMQ

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-02-23 06:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me