Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Additional utility RPCs for PSBT #13932

Merged
merged 5 commits into from Feb 16, 2019

Conversation

@achow101
Copy link
Member

commented Aug 9, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2018

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.

@gmaxwell

This comment has been minimized.

Copy link
Member

commented Aug 10, 2018

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

This comment has been minimized.

Copy link
Member

commented Aug 10, 2018

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

@achow101 achow101 force-pushed the achow101:psbt-util-rpcs branch Aug 14, 2018

@achow101

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2018

Rebased

@achow101

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2018

Rebased

@achow101 achow101 force-pushed the achow101:psbt-util-rpcs branch 2 times, most recently Aug 28, 2018

@DrahtBot DrahtBot removed the Needs rebase label Aug 28, 2018

src/rpc/rawtransaction.cpp Outdated
{
if (request.fHelp || request.params.size() != 1)
throw std::runtime_error(
"utxoupdatepsbt \"psbt\"\n"

This comment has been minimized.

Copy link
@promag

promag Aug 28, 2018

Member

Could reduce indentation?

This comment has been minimized.

Copy link
@achow101

achow101 Sep 13, 2018

Author Member

Done

src/rpc/rawtransaction.cpp Outdated
for (auto it = std::next(psbtxs.begin()); it != psbtxs.end(); ++it) {
for (unsigned int i = 0; i < it->tx->vin.size(); ++i) {
if (!merged_psbt.AddInput(it->tx->vin[i], it->inputs[i])) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "An input exists in multiple PSBTs");

This comment has been minimized.

Copy link
@promag

promag Aug 28, 2018

Member

Could be more informative?

This comment has been minimized.

Copy link
@achow101

achow101 Sep 13, 2018

Author Member

What else could be said?

This comment has been minimized.

Copy link
@promag

promag Nov 20, 2018

Member

Like the conflicting input index?

src/rpc/rawtransaction.cpp Outdated
}
}
for (unsigned int i = 0; i < it->tx->vout.size(); ++i) {
merged_psbt.AddOutput(it->tx->vout[i], it->outputs[i]);

This comment has been minimized.

Copy link
@promag

promag Aug 28, 2018

Member

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

This comment has been minimized.

Copy link
@achow101

achow101 Sep 13, 2018

Author Member

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.

This comment has been minimized.

Copy link
@promag

promag Nov 20, 2018

Member

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

src/rpc/rawtransaction.cpp Outdated

// Clear signatures from all inputs
for (auto& input : merged_psbt.inputs) {
input.partial_sigs.clear();

This comment has been minimized.

Copy link
@promag

promag Aug 28, 2018

Member

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?

This comment has been minimized.

Copy link
@achow101

achow101 Sep 13, 2018

Author Member

I don't see how that is better.

This comment has been minimized.

Copy link
@gwillen

gwillen Nov 30, 2018

Contributor

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?

This comment has been minimized.

Copy link
@gwillen

gwillen Dec 4, 2018

Contributor

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

// Unserialize the transactions
std::vector<PartiallySignedTransaction> psbtxs;
UniValue txs = request.params[0].get_array();
for (unsigned int i = 0; i < txs.size(); ++i) {

This comment has been minimized.

Copy link
@promag

promag Aug 28, 2018

Member

Should force txs.size() > 1?

This comment has been minimized.

Copy link
@achow101

achow101 Sep 13, 2018

Author Member

Done

src/rpc/rawtransaction.cpp Outdated
CAmount fee = in_amt - out_amt;

// Estimate the size
size_t size;

This comment has been minimized.

Copy link
@practicalswift

practicalswift Sep 5, 2018

Member

The scope of size could be reduced?

This comment has been minimized.

Copy link
@achow101

achow101 Sep 13, 2018

Author Member

Done

@achow101 achow101 force-pushed the achow101:psbt-util-rpcs branch to 9004ce4 Sep 13, 2018

@achow101

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2018

Rebased

@@ -7,6 +7,7 @@

from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import assert_equal, assert_raises_rpc_error, find_output
from decimal import Decimal

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 2, 2018

Member

Nit: Sort imports :-)

@DrahtBot DrahtBot removed the Needs rebase label Jan 25, 2019

@@ -237,7 +248,7 @@ bool PSBTInputSigned(PSBTInput& input)
return !input.final_script_sig.empty() || !input.final_script_witness.IsNull();
}

bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& psbt, int index, int sighash)
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)

This comment has been minimized.

Copy link
@gwillen

gwillen Jan 29, 2019

Contributor

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

This comment has been minimized.

Copy link
@achow101

achow101 Feb 9, 2019

Author Member

Done

@@ -629,6 +679,18 @@ bool PSBTInput::IsSane() const
return true;
}

bool PSBTInput::GetUTXO(CTxOut& utxo, int prevout_index) const

This comment has been minimized.

Copy link
@gwillen

gwillen Jan 29, 2019

Contributor

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.)

This comment has been minimized.

Copy link
@achow101

achow101 Feb 9, 2019

Author Member

Done

input_univ.pushKV("has_utxo", true);
} else {
input_univ.pushKV("has_utxo", false);
input_univ.pushKV("is_final", false);

This comment has been minimized.

Copy link
@gwillen

gwillen Jan 29, 2019

Contributor

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.)

This comment has been minimized.

Copy link
@gwillen

gwillen Jan 29, 2019

Contributor

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

This comment has been minimized.

Copy link
@achow101

achow101 Feb 9, 2019

Author Member

I'm not quite sure you are asking.

This comment has been minimized.

Copy link
@gwillen

gwillen Feb 10, 2019

Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2019

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.


std::vector<std::vector<unsigned char>> solutions_data;
txnouttype which_type = Solver(coin.out.scriptPubKey, solutions_data);
if (which_type == TX_WITNESS_V0_SCRIPTHASH || which_type == TX_WITNESS_V0_KEYHASH || which_type == TX_WITNESS_UNKNOWN) {

This comment has been minimized.

Copy link
@sipa

sipa Feb 9, 2019

Member

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

This comment has been minimized.

Copy link
@achow101

achow101 Feb 9, 2019

Author Member

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.

This comment has been minimized.

Copy link
@sipa

sipa Feb 15, 2019

Member

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 achow101 force-pushed the achow101:psbt-util-rpcs branch from e65b4a3 to 1193ced Feb 9, 2019

PartiallySignedTransaction() {}
PartiallySignedTransaction(const PartiallySignedTransaction& psbt_in) : tx(psbt_in.tx), inputs(psbt_in.inputs), outputs(psbt_in.outputs), unknown(psbt_in.unknown) {}
explicit PartiallySignedTransaction(const CMutableTransaction& tx);
bool GetInputUTXO(CTxOut& utxo, int input_index) const;

This comment has been minimized.

Copy link
@laanwj

laanwj Feb 12, 2019

Member

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.

This comment has been minimized.

Copy link
@laanwj

laanwj Feb 12, 2019

Member

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

This comment has been minimized.

Copy link
@achow101

achow101 Feb 12, 2019

Author Member

Added doxygen comment

assert "witness_utxo" not in decoded['inputs'][2] and "non_witness_utxo" not in decoded['inputs'][2]

# Two PSBTs with a common input should not be joinable
psbt1 = self.nodes[1].createpsbt([{"txid":txid1, "vout":vout1}], {self.nodes[0].getnewaddress():10.999})

This comment has been minimized.

Copy link
@laanwj

laanwj Feb 12, 2019

Member

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

This comment has been minimized.

Copy link
@achow101

achow101 Feb 12, 2019

Author Member

Done

@achow101 achow101 force-pushed the achow101:psbt-util-rpcs branch 2 times, most recently from 3f34247 to bef5f3b Feb 12, 2019

@achow101 achow101 force-pushed the achow101:psbt-util-rpcs branch from bef5f3b to 0c35f0e Feb 14, 2019

@DrahtBot DrahtBot removed the Needs rebase label Feb 14, 2019

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

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

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

@MarcoFalke MarcoFalke closed this Feb 15, 2019

@MarcoFalke MarcoFalke reopened this Feb 15, 2019

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

utACK 0c35f0e

@@ -1773,6 +1773,67 @@ UniValue converttopsbt(const JSONRPCRequest& request)
return EncodeBase64((unsigned char*)ssTx.data(), ssTx.size());
}

UniValue utxoupdatepsbt(const JSONRPCRequest& request)
{
if (request.fHelp || request.params.size() != 1)

This comment has been minimized.

Copy link
@sipa

sipa Feb 15, 2019

Member

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.


std::vector<std::vector<unsigned char>> solutions_data;
txnouttype which_type = Solver(coin.out.scriptPubKey, solutions_data);
if (which_type == TX_WITNESS_V0_SCRIPTHASH || which_type == TX_WITNESS_V0_KEYHASH || which_type == TX_WITNESS_UNKNOWN) {

This comment has been minimized.

Copy link
@sipa

sipa Feb 15, 2019

Member

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.

}

// Check if it is final
if (input.final_script_sig.empty() && input.final_script_witness.IsNull()) {

This comment has been minimized.

Copy link
@sipa

sipa Feb 15, 2019

Member

Use PSBTInputSigned here?

This comment has been minimized.

Copy link
@sipa

sipa Feb 15, 2019

Member

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).

This comment has been minimized.

Copy link
@achow101

achow101 Feb 16, 2019

Author Member

Done


// If we are only missing signatures and nothing else, then next is signer
if (outdata.missing_pubkeys.empty() && outdata.missing_redeem_script.IsNull() && outdata.missing_witness_script.IsNull() && !outdata.missing_sigs.empty()) {
only_missing_sigs = true;

This comment has been minimized.

Copy link
@sipa

sipa Feb 15, 2019

Member

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.

This comment has been minimized.

Copy link
@achow101

achow101 Feb 16, 2019

Author Member

Done

achow101 added some commits Jul 21, 2018

Implement joinpsbts RPC and tests
Adds a joinpsbts RPC which combines multiple distinct PSBTs into
one PSBT.
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.

@achow101 achow101 force-pushed the achow101:psbt-util-rpcs branch 2 times, most recently from 310b395 to 43bbc2c Feb 16, 2019

@achow101 achow101 force-pushed the achow101:psbt-util-rpcs branch from 43bbc2c to 540729e Feb 16, 2019

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 16, 2019

re-utACK 540729e

@laanwj laanwj merged commit 540729e into bitcoin:master Feb 16, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Feb 16, 2019

Merge #13932: Additional utility RPCs for PSBT
540729e Implement analyzepsbt RPC and tests (Andrew Chow)
77542cf Move PSBT UTXO fetching to a separate method (Andrew Chow)
cb40b3a Figure out what is missing during signing (Andrew Chow)
08f749c Implement joinpsbts RPC and tests (Andrew Chow)
7344a7b Implement utxoupdatepsbt RPC and tests (Andrew Chow)

Pull request description:

  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.

Tree-SHA512: 3c1fa302201abca76a8901d0c2be7b4ccbce334d989533c215f8b3e50e22f2f018ce6209544b26789f58f5980a253c0655111e1e20d47d5656e0414c64891a5c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.