Additional utility RPCs for PSBT

achow101 commented at 9:55 pm on August 9, 2018: member

This PR adds 3 new utility RPCs for interacting with PSBTs.

utxoupdatepsbt updates a PSBT with UTXO information from the node. It only works with witness UTXOs because full transactions (as would be needed for non-witness UTXOs) are not available unless txindex is enabled.

joinpsbts joins the inputs from multiple distinct PSBTs into one PSBT. e.g. if PSBT 1 has inputs 1 and 2, and PSBT 2 has inputs 3 and 4, joinpsbts would create a new PSBT with inputs 1, 2, 3, and 4.

analyzepsbt analyzes a PSBT and determines the current state of it and all of its inputs, and the next step that needs to be done.

DrahtBot commented at 10:37 pm on August 9, 2018: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15404 ([test] Remove -txindex to start nodes by amitiuttarwar)

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.

MarcoFalke added the label RPC/REST/ZMQ on Aug 10, 2018

MarcoFalke added the label Feature on Aug 10, 2018

MarcoFalke added this to the milestone 0.18.0 on Aug 10, 2018

gmaxwell commented at 5:53 am on August 10, 2018: contributor

It only works with witness UTXOs because full transactions (as would be needed for non-witness UTXOs) are not available unless txindex is enabled.

It doesn’t look in the wallet?

sipa commented at 6:07 am on August 10, 2018: member

@gmaxwell walletprocesspsbt already exists for that. This is a node RPC that works without a wallet.

DrahtBot added the label Needs rebase on Aug 14, 2018

achow101 force-pushed on Aug 14, 2018

achow101 commented at 8:48 pm on August 14, 2018: member

Rebased

DrahtBot removed the label Needs rebase on Aug 14, 2018

DrahtBot added the label Needs rebase on Aug 28, 2018

achow101 commented at 9:53 pm on August 28, 2018: member

Rebased

achow101 force-pushed on Aug 28, 2018

DrahtBot removed the label Needs rebase on Aug 28, 2018

in src/rpc/rawtransaction.cpp:1799 in f29d03c08e outdated

1791@@ -1791,6 +1792,320 @@ UniValue converttopsbt(const JSONRPCRequest& request)
1792     return EncodeBase64((unsigned char*)ssTx.data(), ssTx.size());
1793 }
1794 
1795+UniValue utxoupdatepsbt(const JSONRPCRequest& request)
1796+{
1797+    if (request.fHelp || request.params.size() != 1)
1798+        throw std::runtime_error(
1799+                            "utxoupdatepsbt \"psbt\"\n"

promag commented at 11:19 pm on August 28, 2018:

Could reduce indentation?

achow101 commented at 11:51 pm on September 13, 2018:

Done

in src/rpc/rawtransaction.cpp:1888 in f29d03c08e outdated

1896+
1897+    // Merge
1898+    for (auto it = std::next(psbtxs.begin()); it != psbtxs.end(); ++it) {
1899+        for (unsigned int i = 0; i < it->tx->vin.size(); ++i) {
1900+            if (!merged_psbt.AddInput(it->tx->vin[i], it->inputs[i])) {
1901+                throw JSONRPCError(RPC_INVALID_PARAMETER, "An input exists in multiple PSBTs");

promag commented at 11:29 pm on August 28, 2018:

Could be more informative?

achow101 commented at 11:21 pm on September 13, 2018:

What else could be said?

promag commented at 0:06 am on November 20, 2018:

Like the conflicting input index?

in src/rpc/rawtransaction.cpp:1892 in f29d03c08e outdated

1900+            if (!merged_psbt.AddInput(it->tx->vin[i], it->inputs[i])) {
1901+                throw JSONRPCError(RPC_INVALID_PARAMETER, "An input exists in multiple PSBTs");
1902+            }
1903+        }
1904+        for (unsigned int i = 0; i < it->tx->vout.size(); ++i) {
1905+            merged_psbt.AddOutput(it->tx->vout[i], it->outputs[i]);

promag commented at 11:30 pm on August 28, 2018:

Should allow duplicate outputs? Or should sum values into one output?

achow101 commented at 11:23 pm on September 13, 2018:

Duplicate outputs should be allowed. The idea is that there are two distinct transactions with separate inputs and outputs. They are just being combined into one transaction. Thus you can have duplicate outputs as outputs are still unique. However the inputs must be enforced to be unique.

promag commented at 0:09 am on November 20, 2018:

@achow101 could you ack/nack on #12419, esp @MeshCollider #12419 (comment)

in src/rpc/rawtransaction.cpp:1881 in f29d03c08e outdated

1889+
1890+    PartiallySignedTransaction merged_psbt(psbtxs[0]); // Copy the first one
1891+
1892+    // Clear signatures from all inputs
1893+    for (auto& input : merged_psbt.inputs) {
1894+        input.partial_sigs.clear();

promag commented at 11:36 pm on August 28, 2018:

So this is necessary because the first is a copy and the remaining psbt inputs are cleared because of AddInput. Maybe remove this “optimization” and merge all psbts to an empty psbt?

achow101 commented at 11:24 pm on September 13, 2018:

I don’t see how that is better.

gwillen commented at 3:05 am on November 30, 2018:

It would be less code, and less complexity, which I would say is always better, absent a reason to write more code. Why write more code?

gwillen commented at 3:14 am on December 4, 2018:

(Also, this is copied from combinepsbt, where it is easiest to treat the first tx specially, because we need something to compare all the others to, to make sure the underlying transaction is the same. That restriction doesn’t exist here, so the need for the extra step is gone.)

in src/rpc/rawtransaction.cpp:1794 in f29d03c08e outdated

1876+    RPCTypeCheck(request.params, {UniValue::VARR}, true);
1877+
1878+    // Unserialize the transactions
1879+    std::vector<PartiallySignedTransaction> psbtxs;
1880+    UniValue txs = request.params[0].get_array();
1881+    for (unsigned int i = 0; i < txs.size(); ++i) {

promag commented at 11:48 pm on August 28, 2018:

Should force txs.size() > 1?

achow101 commented at 11:51 pm on September 13, 2018:

Done

in src/rpc/rawtransaction.cpp:2059 in f29d03c08e outdated

2054+
2055+        // Get the fee
2056+        CAmount fee = in_amt - out_amt;
2057+
2058+        // Estimate the size
2059+        size_t size;

practicalswift commented at 7:11 am on September 5, 2018:

The scope of size could be reduced?

achow101 commented at 11:51 pm on September 13, 2018:

Done

DrahtBot added the label Needs rebase on Sep 10, 2018

achow101 force-pushed on Sep 13, 2018

achow101 commented at 11:51 pm on September 13, 2018: member

Rebased

DrahtBot removed the label Needs rebase on Sep 13, 2018

in test/functional/rpc_psbt.py:10 in 9004ce4969 outdated

 6@@ -7,6 +7,7 @@
 7 
 8 from test_framework.test_framework import BitcoinTestFramework
 9 from test_framework.util import assert_equal, assert_raises_rpc_error, find_output
10+from decimal import Decimal

practicalswift commented at 5:14 am on October 2, 2018:

Nit: Sort imports :-)

DrahtBot added the label Needs rebase on Nov 1, 2018

achow101 force-pushed on Nov 1, 2018

sipa added this to the "For backport" column in a project

MarcoFalke moved this from the "For backport" to the "Blockers" column in a project

sipa moved this from the "For backport" to the "Blockers" column in a project

DrahtBot removed the label Needs rebase on Nov 1, 2018

achow101 force-pushed on Nov 1, 2018

in src/rpc/rawtransaction.cpp:1983 in ba5f9058f6 outdated

1898+            // Figure out what is missing
1899+            std::vector<CKeyID> missing_pubkeys;
1900+            std::vector<CKeyID> missing_sigs;
1901+            uint160 missing_redeem_script;
1902+            uint256 missing_witness_script;
1903+            SignatureData sigdata;

luke-jr commented at 8:06 pm on November 5, 2018:

Unused?

in src/rpc/rawtransaction.cpp:2053 in ba5f9058f6 outdated

1968+        CCoinsViewCache view(&view_dummy);
1969+        bool success = true;
1970+
1971+        for (unsigned int i = 0; i < psbtx.tx->vin.size(); ++i) {
1972+            PSBTInput& input = psbtx.inputs[i];
1973+            SignatureData sigdata;

luke-jr commented at 8:06 pm on November 5, 2018:

Unused?

in src/rpc/rawtransaction.cpp:2039 in ba5f9058f6 outdated

1954+    }
1955+    if (calc_fee) {
1956+        // Get the output amount
1957+        CAmount out_amt = 0;
1958+        for (const CTxOut& out : psbtx.tx->vout) {
1959+            out_amt += out.nValue;

practicalswift commented at 6:27 pm on November 9, 2018:

Nit: Looks like a case for std::accumulate perhaps? :-)

DrahtBot added the label Needs rebase on Nov 10, 2018

achow101 force-pushed on Nov 17, 2018

achow101 commented at 1:38 am on November 17, 2018: member

Rebased

DrahtBot removed the label Needs rebase on Nov 17, 2018

in src/rpc/rawtransaction.cpp:1780 in f2a2f06fed outdated

1772@@ -1773,6 +1773,67 @@ UniValue converttopsbt(const JSONRPCRequest& request)
1773     return EncodeBase64((unsigned char*)ssTx.data(), ssTx.size());
1774 }
1775 
1776+UniValue utxoupdatepsbt(const JSONRPCRequest& request)
1777+{
1778+    if (request.fHelp || request.params.size() != 1)
1779+        throw std::runtime_error(
1780+            "utxoupdatepsbt \"psbt\"\n"

promag commented at 0:12 am on November 20, 2018:

Should use RPCHelpMan from #14530.

in src/rpc/rawtransaction.cpp:1720 in f2a2f06fed outdated

1791+
1792+    // Unserialize the transactions
1793+    PartiallySignedTransaction psbtx;
1794+    std::string error;
1795+    if (!DecodePSBT(psbtx, request.params[0].get_str(), error)) {
1796+        throw JSONRPCError(RPC_DESERIALIZATION_ERROR, strprintf("TX decode failed %s", error));

promag commented at 0:22 am on November 20, 2018:

Could add test for this error.

achow101 commented at 3:17 am on January 25, 2019:

DecodePSBT has its own tests.

in src/rpc/rawtransaction.cpp:1699 in f2a2f06fed outdated

1772@@ -1773,6 +1773,67 @@ UniValue converttopsbt(const JSONRPCRequest& request)
1773     return EncodeBase64((unsigned char*)ssTx.data(), ssTx.size());
1774 }
1775 
1776+UniValue utxoupdatepsbt(const JSONRPCRequest& request)
1777+{
1778+    if (request.fHelp || request.params.size() != 1)

promag commented at 0:22 am on November 20, 2018:

nit, {

achow101 commented at 3:18 am on January 25, 2019:

Put the bracket where?

sipa commented at 10:51 pm on February 15, 2019:

Technically this violates the style guide, as it’s not putting the then clause on the same line as the if, so it needs braces + indenting on the next line.

achow101 commented at 5:17 am on February 16, 2019:

Done

promag commented at 0:22 am on November 20, 2018: member

Could have release note.

DrahtBot added the label Needs rebase on Nov 23, 2018

jnewbery removed this from the "Blockers" column in a project

in src/rpc/rawtransaction.cpp:1925 in afdec9a0ef outdated

1920+            "        \"signatures\" : [          (array)\n"
1921+            "          \"keyid\"                 (string) Public key ID, hash160 of the public key, of a public key whose signature is missing\n"
1922+            "        ]\n"
1923+            "        \"redeemscript\" : \"hash\"   (string) Hash160 of the redeemScript that is missing\n"
1924+            "        \"witnessscript\" : \"hash\"  (string) SHA256 of the witnessScript that is missing\n"
1925+            "        \"next\" : \"role\"           (string) Role of the next person that this input needs to go to\n"

gwillen commented at 1:43 am on January 16, 2019:

This doesn’t match the code, right? “next” goes inside the input, not inside “missing”?

gwillen commented at 1:46 am on January 16, 2019: contributor

It looks like I’m going to depend on this for my offline-signing GUI work, so we can display nice messages in the GUI about the status of PSBTs that users hand us. Can I therefore encourage its revival? :-) @achow101

Note that, assuming #14978 goes in relatively soon, a little refactoring will be needed – mostly just changing #includes I expect. The other PSBT RPCs have had their guts moved to another file, but I’m happy to do that for these RPCs as a followup PR to this one, if you like, so you don’t have to modify this one too much.

achow101 force-pushed on Jan 25, 2019

achow101 commented at 3:18 am on January 25, 2019: member

Rebased and addressed comments.

DrahtBot removed the label Needs rebase on Jan 25, 2019

in src/script/sign.cpp:251 in e65b4a373b outdated

247@@ -237,7 +248,7 @@ bool PSBTInputSigned(PSBTInput& input)
248     return !input.final_script_sig.empty() || !input.final_script_witness.IsNull();
249 }
250 
251-bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& psbt, int index, int sighash)
252+bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& psbt, int index, int sighash, std::vector<CKeyID>* missing_pubkeys, std::vector<CKeyID>* missing_sigs, uint160* missing_redeem_script, uint256* missing_witness_script, bool use_dummy)

gwillen commented at 1:20 am on January 29, 2019:

What about just having SignPSBTInput take an (optional) output-parameter sigdata, just like ProduceSignature? This function signature is getting unwieldy.

achow101 commented at 10:03 pm on February 9, 2019:

Done

in src/script/sign.cpp:682 in e65b4a373b outdated

678@@ -629,6 +679,18 @@ bool PSBTInput::IsSane() const
679     return true;
680 }
681 
682+bool PSBTInput::GetUTXO(CTxOut& utxo, int prevout_index) const

gwillen commented at 1:35 am on January 29, 2019:

This interface feels fragile to me. Could this instead be “bool PartiallySignedTransaction::GetInputUTXO(CTxOut& utxo, int input_index)”? That version would be resistant to misuse, whereas this version will do non-obvious crazy things if you accidentally give it the wrong prevout_index for your input (which seems easy to do.)

achow101 commented at 10:04 pm on February 9, 2019:

Done

in src/rpc/rawtransaction.cpp:1904 in e65b4a373b outdated

1940+        if (input.GetUTXO(utxo, psbtx.tx->vin[i].prevout.n)) {
1941+            in_amt += utxo.nValue;
1942+            input_univ.pushKV("has_utxo", true);
1943+        } else {
1944+            input_univ.pushKV("has_utxo", false);
1945+            input_univ.pushKV("is_final", false);

gwillen commented at 1:39 am on January 29, 2019:

Are there cases where this isn’t overwritten by a later check? If so, you should set all_final = false here. (If not you should remove this.)

gwillen commented at 1:42 am on January 29, 2019:

Oh, I see, it’s handled by calc_fee = false? The flow is a little hard to follow.

achow101 commented at 10:06 pm on February 9, 2019:

I’m not quite sure you are asking.

gwillen commented at 4:32 am on February 10, 2019:

Yeah, I think I just have trouble following the state machine of this function, which is a little complicated, and so I was confused about whether all_final was actually getting set correctly in every case. Since nobody’s relying on it yet, and I haven’t found any actual issues, I think it should probably go in as-is, and I may propose some refactoring when I go to actually use it.

gwillen commented at 1:45 am on January 29, 2019: contributor

utACK modulo comments above. I have a general sense that analyzepsbt is a little hard to follow the logic of, but I don’t immediately have suggestions for improving that.

in src/rpc/rawtransaction.cpp:1751 in b2c85b14f1 outdated

1789+
1790+        const Coin& coin = view.AccessCoin(psbtx.tx->vin[i].prevout);
1791+
1792+        std::vector<std::vector<unsigned char>> solutions_data;
1793+        txnouttype which_type = Solver(coin.out.scriptPubKey, solutions_data);
1794+        if (which_type == TX_WITNESS_V0_SCRIPTHASH || which_type == TX_WITNESS_V0_KEYHASH || which_type == TX_WITNESS_UNKNOWN) {

sipa commented at 2:19 am on February 9, 2019:

Not P2SH? There’s no guarantee that P2SH outputs are witness of course, but there is no real harm in including too much.

achow101 commented at 8:55 pm on February 9, 2019:

I think that would be a problem right now since providing a witness utxo requires a witness signature and if a P2SH output was not witness, then it would never be signed.

sipa commented at 11:00 pm on February 15, 2019:

I guess there’s an obvious solution which can be added later, namely letting the RPC take in descriptor(s) that apply to its inputs. I’ll look into that after this PR is merged.

achow101 force-pushed on Feb 9, 2019

in src/script/sign.h:591 in 1193ced586 outdated

575@@ -572,9 +576,12 @@ struct PartiallySignedTransaction
576     bool IsNull() const;
577     void Merge(const PartiallySignedTransaction& psbt);
578     bool IsSane() const;
579+    bool AddInput(const CTxIn& txin, PSBTInput& psbtin);
580+    bool AddOutput(const CTxOut& txout, const PSBTOutput& psbtout);
581     PartiallySignedTransaction() {}
582     PartiallySignedTransaction(const PartiallySignedTransaction& psbt_in) : tx(psbt_in.tx), inputs(psbt_in.inputs), outputs(psbt_in.outputs), unknown(psbt_in.unknown) {}
583     explicit PartiallySignedTransaction(const CMutableTransaction& tx);
584+    bool GetInputUTXO(CTxOut& utxo, int input_index) const;

laanwj commented at 7:53 am on February 12, 2019:

Please add a short doxygen documentation for the new calls. (I know, the current ones don’t, but every day is a good day to get started)

Edit: also, what is our normal argument ordering here? input then output arguments or vice versa? Let’s try to be consistent.

laanwj commented at 8:43 am on February 12, 2019:

Maybe we can avoid the input/output order completely by returning an Optional<CTxOut> instead. We have this in optional.h, after all.

achow101 commented at 5:08 pm on February 12, 2019:

Added doxygen comment

in test/functional/rpc_psbt.py:320 in 1193ced586 outdated

315+        assert "witness_utxo" in decoded['inputs'][0] and "non_witness_utxo" not in decoded['inputs'][0]
316+        assert "witness_utxo" not in decoded['inputs'][1] and "non_witness_utxo" not in decoded['inputs'][1]
317+        assert "witness_utxo" not in decoded['inputs'][2] and "non_witness_utxo" not in decoded['inputs'][2]
318+
319+        # Two PSBTs with a common input should not be joinable
320+        psbt1 = self.nodes[1].createpsbt([{"txid":txid1, "vout":vout1}], {self.nodes[0].getnewaddress():10.999})

laanwj commented at 8:56 am on February 12, 2019:

Decimal("10.999") I guess? (there are some more occurrences of floats for monetary values, won’t comment them individually)

achow101 commented at 5:08 pm on February 12, 2019:

Done

achow101 force-pushed on Feb 12, 2019

achow101 force-pushed on Feb 14, 2019

DrahtBot added the label Needs rebase on Feb 14, 2019

achow101 force-pushed on Feb 14, 2019

DrahtBot removed the label Needs rebase on Feb 14, 2019

laanwj commented at 4:20 pm on February 15, 2019: member

very strange!-one of the travis runs seems to be failing with

0/bin/bash: test/test_bitcoin: No such file or directory
1Makefile:12966: recipe for target 'test/addrman_tests.cpp.test' failed
2make[3]: *** [test/addrman_tests.cpp.test] Error 1
3make[3]: *** Waiting for unfinished jobs....
4/bin/bash: test/test_bitcoin: No such file or directory
5Makefile:12966: recipe for target 'test/arith_uint256_tests.cpp.test' failed
6make[3]: *** [test/arith_uint256_tests.cpp.test] Error 1
7PASS: qt/test/test_bitcoin-qt

MarcoFalke closed this on Feb 15, 2019

MarcoFalke reopened this on Feb 15, 2019

laanwj commented at 8:31 pm on February 15, 2019: member

utACK 0c35f0e802274c27736b16a4542ad44bc3c7f794

in src/rpc/rawtransaction.cpp:1907 in 0c35f0e802 outdated

1902+            input_univ.pushKV("next", "updater");
1903+            calc_fee = false;
1904+        }
1905+
1906+        // Check if it is final
1907+        if (input.final_script_sig.empty() && input.final_script_witness.IsNull()) {

sipa commented at 11:08 pm on February 15, 2019:

Use PSBTInputSigned here?

sipa commented at 11:10 pm on February 15, 2019:

I think you may want to add a check here for whether the input UTXO is present (if not, it’s possible that you first set next=“updater”, and then overwrite it in this conditional).

achow101 commented at 5:16 am on February 16, 2019:

Done

in src/rpc/rawtransaction.cpp:1945 in 0c35f0e802 outdated

1940+                }
1941+                input_univ.pushKV("missing", missing);
1942+
1943+                // If we are only missing signatures and nothing else, then next is signer
1944+                if (outdata.missing_pubkeys.empty() && outdata.missing_redeem_script.IsNull() && outdata.missing_witness_script.IsNull() && !outdata.missing_sigs.empty()) {
1945+                    only_missing_sigs = true;

sipa commented at 11:12 pm on February 15, 2019:

I think this (cross-input) variable can’t be set to true here. If this branch is executed for the last input in a transaction, the global “next” will report “signer”, even if keys/script or even UTXOs are missing for other inputs.

You probably want to have the variable start at true, and then set it to false if anything anywhere more than a signature is missing.

achow101 commented at 5:16 am on February 16, 2019:

Done

Implement utxoupdatepsbt RPC and tests 7344a7b998

Implement joinpsbts RPC and tests

Adds a joinpsbts RPC which combines multiple distinct PSBTs into
one PSBT.

08f749c914

Figure out what is missing during signing

When signing an input, figure out what was requested for but was unable
to be found and store it in a SignatureData.

Return this information in SignPSBTInput.

cb40b3abd4

Move PSBT UTXO fetching to a separate method 77542cf2a5

achow101 force-pushed on Feb 16, 2019

Implement analyzepsbt RPC and tests 540729ef4b

achow101 force-pushed on Feb 16, 2019

laanwj commented at 7:44 pm on February 16, 2019: member

re-utACK 540729ef4bf1b6c6da1ec795e441d2ce56a9a58b

laanwj merged this on Feb 16, 2019

laanwj closed this on Feb 16, 2019

laanwj referenced this in commit d5b929c813 on Feb 16, 2019

deadalnix referenced this in commit b991a6b45c on May 13, 2020

deadalnix referenced this in commit f283c36d3a on May 13, 2020

deadalnix referenced this in commit 1915fdf30f on May 13, 2020

deadalnix referenced this in commit 63fa706bb6 on May 13, 2020

deadalnix referenced this in commit 397f2c9316 on May 16, 2020

kittywhiskers referenced this in commit 106455d791 on Aug 5, 2021

kittywhiskers referenced this in commit fd724a17d5 on Aug 5, 2021

kittywhiskers referenced this in commit bd4fa2355f on Aug 5, 2021

kittywhiskers referenced this in commit 8a1291648f on Aug 5, 2021

kittywhiskers referenced this in commit a9a2d0d850 on Aug 5, 2021

kittywhiskers referenced this in commit 419bcd395f on Aug 9, 2021

UdjinM6 referenced this in commit 7aebf156e9 on Aug 10, 2021

5tefan referenced this in commit e71f0e5e31 on Aug 12, 2021

kittywhiskers referenced this in commit 72504d171f on Dec 4, 2021

kittywhiskers referenced this in commit 46a56592bc on Dec 8, 2021

kittywhiskers referenced this in commit 55386acde8 on Dec 12, 2021

kittywhiskers referenced this in commit 1b6570894d on Dec 13, 2021

kittywhiskers referenced this in commit eb63905395 on Dec 13, 2021

MarcoFalke locked this on Dec 16, 2021

Additional utility RPCs for PSBT #13932

Conflicts