Implement BIP 370 PSBTv2

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.

DrahtBot added the label GUI on Feb 23, 2021

DrahtBot added the label RPC/REST/ZMQ on Feb 23, 2021

DrahtBot added the label Wallet on Feb 23, 2021

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

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
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:

#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)
#26732 ([WIP] wallet: tx creation, don’t select outputs from txes that are being replaced by furszy)
#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.

achow101 force-pushed on Mar 5, 2021

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.

achow101 force-pushed on Mar 5, 2021

DrahtBot added the label Needs rebase on Mar 17, 2021

achow101 force-pushed on Mar 17, 2021

DrahtBot removed the label Needs rebase on Mar 17, 2021

DrahtBot added the label Needs rebase on Mar 29, 2021

achow101 force-pushed on Apr 1, 2021

DrahtBot removed the label Needs rebase on Apr 1, 2021

achow101 force-pushed on Apr 5, 2021

DrahtBot added the label Needs rebase on Apr 19, 2021

achow101 force-pushed on Apr 19, 2021

DrahtBot removed the label Needs rebase on Apr 19, 2021

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'

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.

achow101 force-pushed on May 11, 2021

DrahtBot added the label Needs rebase on May 30, 2021

achow101 force-pushed on May 30, 2021

DrahtBot removed the label Needs rebase on May 30, 2021

DrahtBot added the label Needs rebase on Jun 17, 2021

achow101 force-pushed on Jun 18, 2021

DrahtBot removed the label Needs rebase on Jun 19, 2021

DrahtBot added the label Needs rebase on Aug 2, 2021

achow101 force-pushed on Aug 2, 2021

DrahtBot removed the label Needs rebase on Aug 2, 2021

DrahtBot added the label Needs rebase on Sep 23, 2021

achow101 force-pushed on Sep 23, 2021

DrahtBot removed the label Needs rebase on Sep 23, 2021

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

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>?

achow101 force-pushed on Oct 1, 2021

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?

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?

achow101 force-pushed on Oct 3, 2021

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.

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.

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

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

achow101 force-pushed on Oct 5, 2021

DrahtBot added the label Needs rebase on Nov 16, 2021

achow101 force-pushed on Nov 16, 2021

DrahtBot removed the label Needs rebase on Nov 16, 2021

achow101 force-pushed on Nov 23, 2021

achow101 force-pushed on Nov 24, 2021

DrahtBot added the label Needs rebase on Nov 29, 2021

achow101 force-pushed on Dec 5, 2021

DrahtBot removed the label Needs rebase on Dec 5, 2021

DrahtBot added the label Needs rebase on Dec 8, 2021

achow101 force-pushed on Dec 8, 2021

DrahtBot removed the label Needs rebase on Dec 8, 2021

DrahtBot added the label Needs rebase on Dec 9, 2021

achow101 force-pushed on Dec 9, 2021

DrahtBot removed the label Needs rebase on Dec 9, 2021

achow101 force-pushed on Dec 9, 2021

achow101 marked this as a draft on Dec 9, 2021

achow101 force-pushed on Dec 10, 2021

achow101 marked this as ready for review on Dec 10, 2021

DrahtBot added the label Needs rebase on Dec 13, 2021

achow101 force-pushed on Dec 13, 2021

DrahtBot removed the label Needs rebase on Dec 13, 2021

achow101 force-pushed on Feb 2, 2022

DrahtBot added the label Needs rebase on Mar 30, 2022

achow101 force-pushed on Mar 30, 2022

DrahtBot removed the label Needs rebase on Mar 30, 2022

DrahtBot added the label Needs rebase on Mar 31, 2022

achow101 force-pushed on Mar 31, 2022

DrahtBot removed the label Needs rebase on Mar 31, 2022

DrahtBot added the label Needs rebase on Apr 15, 2022

achow101 force-pushed on Apr 15, 2022

DrahtBot removed the label Needs rebase on Apr 15, 2022

DrahtBot added the label Needs rebase on Jun 28, 2022

achow101 force-pushed on Jun 29, 2022

DrahtBot removed the label Needs rebase on Jun 29, 2022

achow101 force-pushed on Jul 5, 2022

achow101 force-pushed on Jul 6, 2022

DrahtBot added the label Needs rebase on Jul 13, 2022

achow101 force-pushed on Jul 13, 2022

DrahtBot removed the label Needs rebase on Jul 13, 2022

achow101 force-pushed on Jul 18, 2022

achow101 force-pushed on Jul 19, 2022

DrahtBot added the label Needs rebase on Jul 27, 2022

achow101 force-pushed on Jul 28, 2022

DrahtBot removed the label Needs rebase on Jul 28, 2022

DrahtBot added the label Needs rebase on Aug 1, 2022

achow101 force-pushed on Aug 1, 2022

DrahtBot removed the label Needs rebase on Aug 1, 2022

achow101 force-pushed on Sep 20, 2022

DrahtBot added the label Needs rebase on Oct 20, 2022

achow101 force-pushed on Oct 21, 2022

DrahtBot removed the label Needs rebase on Oct 21, 2022

achow101 force-pushed on Nov 30, 2022

DrahtBot added the label Needs rebase on Dec 9, 2022

achow101 force-pushed on Dec 9, 2022

DrahtBot removed the label Needs rebase on Dec 9, 2022

DrahtBot added the label Needs rebase on Jan 17, 2023

achow101 force-pushed on Jan 18, 2023

DrahtBot removed the label Needs rebase on Jan 18, 2023

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.

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

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.

in src/rpc/rawtransaction.cpp:1827 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

in src/rpc/rawtransaction.cpp:1809 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

achow101 force-pushed on Jan 27, 2023

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.

DrahtBot added the label Needs rebase on Feb 16, 2023

achow101 force-pushed on Feb 17, 2023

DrahtBot removed the label Needs rebase on Feb 17, 2023

DrahtBot added the label Needs rebase on Mar 16, 2023

achow101 force-pushed on Mar 17, 2023

DrahtBot removed the label Needs rebase on Mar 17, 2023

in src/rpc/rawtransaction.cpp:1811 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.

DrahtBot added the label Needs rebase on Apr 21, 2023

achow101 force-pushed on May 1, 2023

DrahtBot removed the label Needs rebase on May 1, 2023

DrahtBot added the label CI failed on May 23, 2023

achow101 force-pushed on May 27, 2023

DrahtBot removed the label CI failed on May 27, 2023

DrahtBot added the label Needs rebase on Jun 1, 2023

achow101 force-pushed on Jun 1, 2023

DrahtBot removed the label Needs rebase on Jun 1, 2023

DrahtBot added the label CI failed on Aug 6, 2023

maflcko commented at 9:28 am on August 17, 2023: member

Needs rebase, if still relevant.

DrahtBot added the label Needs rebase on Aug 17, 2023

achow101 force-pushed on Aug 17, 2023

DrahtBot removed the label Needs rebase on Aug 17, 2023

DrahtBot removed the label CI failed on Aug 18, 2023

DrahtBot added the label Needs rebase on Sep 12, 2023

achow101 force-pushed on Sep 12, 2023

DrahtBot removed the label Needs rebase on Sep 12, 2023

DrahtBot added the label Needs rebase on Oct 2, 2023

achow101 force-pushed on Oct 3, 2023

DrahtBot removed the label Needs rebase on Oct 3, 2023

scgbckbone commented at 2:01 pm on October 11, 2023: none

ACK https://github.com/bitcoin/bitcoin/pull/21283/commits/93086378db3d0e959cf2b73e9a7a9880022043c1

DrahtBot added the label Needs rebase on Oct 16, 2023

achow101 force-pushed on Oct 16, 2023

DrahtBot removed the label Needs rebase on Oct 16, 2023

DrahtBot added the label Needs rebase on Oct 25, 2023

achow101 force-pushed on Oct 25, 2023

DrahtBot removed the label Needs rebase on Oct 25, 2023

DrahtBot added the label Needs rebase on Nov 15, 2023

achow101 force-pushed on Nov 15, 2023

DrahtBot removed the label Needs rebase on Nov 15, 2023

DrahtBot added the label CI failed on Nov 27, 2023

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      |                                   ^~~~~~~~~

achow101 force-pushed on Nov 27, 2023

DrahtBot removed the label CI failed on Nov 28, 2023

DrahtBot added the label Needs rebase on Dec 11, 2023

achow101 force-pushed on Dec 11, 2023

DrahtBot removed the label Needs rebase on Dec 11, 2023

DrahtBot added the label CI failed on Jan 24, 2024

achow101 commented at 3:00 pm on April 9, 2024: member

Will split this up

DrahtBot added the label Needs rebase on Apr 22, 2024

achow101 force-pushed on Apr 25, 2024

DrahtBot removed the label Needs rebase on Apr 25, 2024

DrahtBot removed the label CI failed on Apr 26, 2024

DrahtBot added the label Needs rebase on May 23, 2024

achow101 force-pushed on May 29, 2024

DrahtBot removed the label Needs rebase on May 29, 2024

DrahtBot added the label Needs rebase on Jun 12, 2024

achow101 force-pushed on Jun 13, 2024

DrahtBot removed the label Needs rebase on Jun 13, 2024

DrahtBot added the label Needs rebase on Jul 8, 2024

achow101 force-pushed on Jul 10, 2024

DrahtBot removed the label Needs rebase on Jul 10, 2024

DrahtBot added the label Needs rebase on Jul 11, 2024

achow101 force-pushed on Jul 22, 2024

DrahtBot removed the label Needs rebase on Jul 22, 2024

hebasto added the label Needs CMake port on Aug 16, 2024

DrahtBot added the label Needs rebase on Sep 2, 2024

maflcko removed the label GUI on Sep 3, 2024

maflcko removed the label Needs CMake port on Sep 3, 2024

Define psbtv2 field numbers 8d8a1a8506

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.

09c8656aaa

achow101 force-pushed on Sep 3, 2024

DrahtBot removed the label Needs rebase on Sep 3, 2024

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.

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!

in src/wallet/rpc/spend.cpp:101 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

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

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!

Implement PSBTv2 fields de/ser 6a73e374fb

Have PSBTInput and PSBTOutput know the PSBT's version 2dc96bd4dc

Enforce PSBT version constraints

With PSBTv2, some fields are not allowed in PSBTv2, and some are
required. Enforce those.

c6743bf713

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.

0c459e43ba

Call CacheUnsignedTxPieces in PSBT constructor cef9de00f4

Convert PSBTv0 unsigned tx to PSBTv2 fields

This is just a convenience and doesn't effect serialization.

ae8662918c

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.

c2ac3e8dd7

Add PSBT::ComputeLockTime()

Function to compute the lock time for the transaction

5c7473cb19

Add PSBT::GetUnsignedTx

A helper function for getting the unsigned transaction regardless of
psbt version.

a600eba36d

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.

162d091677

Change PSBT::AddInput to take just PSBTInput aeb9e44bc5

Change PSBT::AddOutput to take just PSBTOutput abf485c27c

Implement PSBTv2 field merging a79e519323

Add PSBTInput::GetOutPoint

Helper for getting the PSBTInput COutPoint

99f6aa7d70

Update SignPSBTInput for PSBTv2 a30b99d976

Change PrecomputePSBTData to use GetUnsignedTx 85b52e9030

Update FinalizeAndExtract for v2 3c2db8c855

Update AnalyzePSBT for PSBTv2 519238b9ff

Update PSBT Operations Dialog for v2 d32e7af60b

Update RPCs for PSBTv2 06f2b15be7

Update wallet for PSBTv2 d41b0494d8

Implement PSBTv2 AddInput and AddOutput 3fe17931ae

Update PSBTInputSignedAndVerified for PSBTv2 f2d8092c3d

Update test_framework/psbt.py for PSBTv2 7eea10fda3

Implement PSBTv2 in decodepsbt 86137a37b2

Allow specifying PSBT version in constructor 5b68efaf7e

Update PSBT::UpdatePSBTOutput to use GetUnsignedTx 4993953a1d

Restrict joinpsbts to PSBTv0 only b1e7ed17a6

Allow createpsbt and walletcreatefundedpsbt to take psbt version

Use v2 for other RPCs. For some tests to work, PSBTv0 is set explicitly.

a29888643d

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.

c0a7c232a8

psbt: Change default psbt version to 2 a01f66080b

tests: Add test vectors from BIP 370 f1619206a1

tests: Add PSBT unit test for ComputeTimeLock 58fe093a6a

fixup! psbt: Change default psbt version to 2 4abe180242

doc: Release notes for psbtv2 16d734e30e

achow101 force-pushed on Sep 19, 2024

DrahtBot added the label CI failed on Nov 1, 2024

DrahtBot removed the label CI failed on Nov 1, 2024

Implement BIP 370 PSBTv2 #21283

Code Coverage

Reviews

Conflicts