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

Add support for descriptors to utxoupdatepsbt #15427

Merged
merged 4 commits into from Jul 2, 2019

Conversation

@sipa
Copy link
Member

commented Feb 17, 2019

This adds a descriptors argument to the utxoupdatepsbt RPC. This means:

  • Input and output scripts and keys will be filled in when known.
  • P2SH-witness inputs will be filled in from the UTXO set when a descriptor is provided that shows they're spending segwit outputs.

This also moves some (newly) shared code to separate functions: UpdatePSBTOutput (an analogue to SignPSBTInput), IsSegWitOutput, and EvalDescriptorStringOrObject (implementing the string or object notation parsing used in scantxoutset).

@sipa

This comment has been minimized.

Copy link
Member Author

commented Feb 17, 2019

@achow101 @gwillen You may be interested in this.

@sipa sipa force-pushed the sipa:201902_utxoupdatepsbtdesc branch from ad6facf to e90a5c7 Feb 17, 2019

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #16240 (JSONRPCRequest-aware RPCHelpMan by kallewoof)
  • #16227 (Refactor CWallet's inheritance chain by achow101)
  • #15996 (rpc: Deprecate totalfee argument in bumpfee by instagibbs)

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
Copy link
Member

left a comment

Conecpt ACK

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved
@promag
Copy link
Member

left a comment

Concept ACK, beside comment below, abstracted code looks good. Could include release note.

src/script/sign.cpp Show resolved Hide resolved

@sipa sipa force-pushed the sipa:201902_utxoupdatepsbtdesc branch from e90a5c7 to e767de4 Feb 24, 2019

@sipa

This comment has been minimized.

Copy link
Member Author

commented Feb 24, 2019

Rebased.

@DrahtBot DrahtBot added Needs rebase and removed Needs rebase labels Feb 24, 2019

@sipa sipa force-pushed the sipa:201902_utxoupdatepsbtdesc branch from e767de4 to cdabfae Mar 2, 2019

@sipa

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2019

Rebased after merge of #15497.

@DrahtBot DrahtBot removed the Needs rebase label Mar 2, 2019

@meshcollider

This comment has been minimized.

Copy link
Member

commented Apr 14, 2019

Concept ACK

@laanwj laanwj added this to Blockers in High-priority for review Apr 18, 2019

@jnewbery
Copy link
Member

left a comment

utACk cdabfae

I left a bunch of comments, mostly around comments.

src/psbt.h Outdated Show resolved Hide resolved
@@ -210,6 +210,20 @@ bool PSBTInputSigned(PSBTInput& input)
return !input.final_script_sig.empty() || !input.final_script_witness.IsNull();
}

void UpdatePSBTOutput(const SigningProvider& provider, PartiallySignedTransaction& psbt, int index)
{

This comment has been minimized.

Copy link
@jnewbery

jnewbery May 2, 2019

Member

I know this is move-only, but I found this functionality pretty inscrutable, and think it could do with some additional commenting. Things that would be helpful:

  • explain why the caller is instantiating a HidingSigningProvider with nosign set to true
  • explain what the magic 0 and 1 values are in the call to ProduceSignature()
  • explain that ProduceSignature() isn't really signing (or if it does, then that signature gets thrown away)

This comment has been minimized.

Copy link
@sipa

sipa May 9, 2019

Author Member

Added a bunch of comments. Better?

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved
input.witness_utxo = coin.out;
}

// Update the inputs using descriptor data.

This comment has been minimized.

Copy link
@jnewbery

jnewbery May 2, 2019

Member

Comment could be expanded to say what is being updated (redeem_script, witness_script and hd_keypaths are being filled)

SignPSBTInput(HidingSigningProvider(&provider, /* nosign */ true, /* nobip32derivs */ false), psbtx, i, /* sighash_type */ 1);
}

// Update the outputs

This comment has been minimized.

Copy link
@jnewbery

jnewbery May 2, 2019

Member

Comment could be expanded to say what is being updated (redeem_script, witness_script and hd_keypaths are being filled)

This comment has been minimized.

Copy link
@sipa

sipa May 9, 2019

Author Member

I'd rather not be specific here, as it may change over time.

input.witness_utxo = coin.out;
}

// Update the inputs using descriptor data.
SignPSBTInput(HidingSigningProvider(&provider, /* nosign */ true, /* nobip32derivs */ false), psbtx, i, /* sighash_type */ 1);

This comment has been minimized.

Copy link
@jnewbery

jnewbery May 2, 2019

Member

I found using SignPSBTInput() a bit jarring here, since nothing is actually being signed. Can you add a comment to say that SignPSBTInput() won't actually sign when provided with a HidingSigningProvider where nosign is true?

This comment has been minimized.

Copy link
@sipa

sipa May 9, 2019

Author Member

Added some comments.

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved
test/functional/rpc_psbt.py Outdated Show resolved Hide resolved

@sipa sipa force-pushed the sipa:201902_utxoupdatepsbtdesc branch from cdabfae to 535b649 May 9, 2019

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

sipa added some commits Feb 16, 2019

Abstract out IsSegWitOutput from utxoupdatepsbt
This is not a pure refactor; additional functionality is added in
IsSegWitOutput which lets it recurse into P2SH when a
SigningProvider is provided that knows about the inner script.
Add support for descriptors to utxoupdatepsbt
This adds a descriptors argument to the utxoupdatepsbt RPC. This means:
* Input and output scripts and keys will be filled in when known
* P2SH-witness outputs will be filled in from the UTXO set when a descriptor
  is provided to show they're segwit outputs.

@sipa sipa force-pushed the sipa:201902_utxoupdatepsbtdesc branch from 535b649 to 26fe9b9 May 10, 2019

@sipa

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

Rebased.

@jnewbery

This comment has been minimized.

Copy link
Member

commented May 10, 2019

utACK 26fe9b9

@DrahtBot DrahtBot removed the Needs rebase label May 13, 2019

@promag
Copy link
Member

left a comment

A couple of comments.

// Parse descriptors, if any.
FlatSigningProvider provider;
if (!request.params[1].isNull()) {
auto descs = request.params[1].get_array();

This comment has been minimized.

Copy link
@promag

promag May 13, 2019

Member

Use range loop

for (const UniValue& scanobject : request.params[1].get_array().getValues()) {

Like here.

MutableTransactionSignatureCreator creator(psbtx.tx.get_ptr(), 0, out.nValue, 1);
ProduceSignature(HidingSigningProvider(pwallet, true, !bip32derivs), creator, out.scriptPubKey, sigdata);
psbt_out.FromSignatureData(sigdata);
UpdatePSBTOutput(HidingSigningProvider(pwallet, true, !bip32derivs), psbtx, i);

This comment has been minimized.

Copy link
@promag

promag May 13, 2019

Member

Makes sense to instantiate HidingSigningProvider before the loop?

test_psbt_input_keys(decoded['inputs'][2], [])

# Try again, now while providing descriptors, making P2SH-segwit work, and causing bip32_derivs and redeem_script to be filled in
descs = [self.nodes[1].getaddressinfo(addr)['desc'] for addr in [addr1,addr2,addr3]]

This comment has been minimized.

Copy link
@promag

promag May 13, 2019

Member

nit, spaces after , - [addr1, addr2, addr3].

@@ -215,6 +215,25 @@ bool PSBTInputSigned(const PSBTInput& input)
return !input.final_script_sig.empty() || !input.final_script_witness.IsNull();
}

void UpdatePSBTOutput(const SigningProvider& provider, PartiallySignedTransaction& psbt, int index)

This comment has been minimized.

Copy link
@promag

promag May 13, 2019

Member

Could name this UpdatePSBT() and iterate outputs here? - which is what both callers do.

This comment has been minimized.

Copy link
@Empact

Empact May 29, 2019

Member

Alternatively, seems this could be appropriate as PartiallySignedTransaction::UpdateOutput

@laanwj

This comment has been minimized.

Copy link
Member

commented May 20, 2019

utACK 26fe9b9 (will hold merging until response to promag's comments)

@ariard
Copy link
Contributor

left a comment

@sipa I'm trying the RPC with the following command:

bitcoin-cli utxoupdatepsbt 'cHNidP8BAFMCAAAAAYCdwVRx2X3o4KHx5tAMsN1ddp51MbfWsietjfMbl5HtAAAAAAD/////AQDh9QUAAAAAF6kUW+rtEOi4nk9rpw2F5XZl1dd8ehGHAAAAAAAAAA=='  '[{"desc":"sh(wpkh([bd50871a/0h/0h/0h]03895c66337b38699bfafff1084ad35bc347fac4f4e5e5fe5eb7dd81155280db53))"}]'

With psbt built from createpsbt and a desc from listunspent but got a RPC_TYPE_ERROR Expected type array, got string. Is there somehting really basic I'm missing or a real parsing issue ? (given than descriptors hasn't been added as argNames)

@@ -232,4 +232,7 @@ void UpdateInput(CTxIn& input, const SignatureData& data);
* Solvability is unrelated to whether we consider this output to be ours. */
bool IsSolvable(const SigningProvider& provider, const CScript& script);

/** Check whether a scriptPubKey is known to be segwit. */

This comment has been minimized.

Copy link
@ariard

ariard May 20, 2019

Contributor

nit: "native segwit or nested-segwit"?

{"desc", RPCArg::Type::STR, RPCArg::Optional::NO, "An output descriptor"},
{"range", RPCArg::Type::RANGE, "1000", "Up to what index HD chains should be explored (either end or [begin,end])"},
}},
}},
},
RPCResult {
" \"psbt\" (string) The base64-encoded partially signed transaction with inputs updated\n"

This comment has been minimized.

Copy link
@ariard

ariard May 20, 2019

Contributor

nit: "with inputs and outputs updated"

@@ -1490,12 +1490,19 @@ UniValue converttopsbt(const JSONRPCRequest& request)

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

This comment has been minimized.

Copy link
@promag

promag Jun 2, 2019

Member

26fe9b9

nit, could use the "new" way:

    if (request.fHelp || !help.IsValidNumArgs(request.params.size())) {
        throw std::runtime_error(help.ToString());
    }
@moneyball

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

It seems like next steps for this PR are for @sipa to respond to @promag and @ariard comments, then probably back to @laanwj for merge consideration.

@promag

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

My comments aren't blocking FWIW.

@promag

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

@sipa my suggestions are in fb3540a, and rebased here d7a8cf2.

@promag

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

ACK 26fe9b9, checked refactors and tests look comprehensive. Still missing a release note but can be added later.

@laanwj laanwj merged commit 26fe9b9 into bitcoin:master Jul 2, 2019

2 checks passed

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

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

Going to merge this, I think the comments are minor enough to be addressable afterward and don't really need to block the merge.

laanwj added a commit that referenced this pull request Jul 2, 2019

Merge #15427: Add support for descriptors to utxoupdatepsbt
26fe9b9 Add support for descriptors to utxoupdatepsbt (Pieter Wuille)
3135c1a Abstract out UpdatePSBTOutput from FillPSBT (Pieter Wuille)
fb90ec3 Abstract out EvalDescriptorStringOrObject from scantxoutset (Pieter Wuille)
eaf4f88 Abstract out IsSegWitOutput from utxoupdatepsbt (Pieter Wuille)

Pull request description:

  This adds a descriptors argument to the `utxoupdatepsbt` RPC. This means:
  * Input and output scripts and keys will be filled in when known.
  * P2SH-witness inputs will be filled in from the UTXO set when a descriptor is provided that shows they're spending segwit outputs.

  This also moves some (newly) shared code to separate functions: `UpdatePSBTOutput` (an analogue to `SignPSBTInput`), `IsSegWitOutput`, and `EvalDescriptorStringOrObject` (implementing the string or object notation parsing used in `scantxoutset`).

ACKs for top commit:
  jnewbery:
    utACK 26fe9b9
  laanwj:
    utACK 26fe9b9 (will hold merging until response to promag's comments)
  promag:
    ACK 26fe9b9, checked refactors and tests look comprehensive. Still missing a release note but can be added later.

Tree-SHA512: 1d833b7351b59d6c5ded6da399ff371a8a2a6ad04c0a8f90e6e46105dc737fa6f2740b1e5340280d59e01f42896c40b720c042f44417e38dfbee6477b894b245
@jnewbery

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

It's a shame that this got merged without a bunch of review comments being addressed. It seems to me that this: #15427 (review) is a bug, and that the new functionality won't work with bitcoin-cli or using named arguments.

@jnewbery

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

Fixed here: #16326

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

This had 3 ACKs. master is always expected to have bugs and if they are trivially fixed in a follow-up, why not?

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 3, 2019

Merge bitcoin#15427: Add support for descriptors to utxoupdatepsbt
26fe9b9 Add support for descriptors to utxoupdatepsbt (Pieter Wuille)
3135c1a Abstract out UpdatePSBTOutput from FillPSBT (Pieter Wuille)
fb90ec3 Abstract out EvalDescriptorStringOrObject from scantxoutset (Pieter Wuille)
eaf4f88 Abstract out IsSegWitOutput from utxoupdatepsbt (Pieter Wuille)

Pull request description:

  This adds a descriptors argument to the `utxoupdatepsbt` RPC. This means:
  * Input and output scripts and keys will be filled in when known.
  * P2SH-witness inputs will be filled in from the UTXO set when a descriptor is provided that shows they're spending segwit outputs.

  This also moves some (newly) shared code to separate functions: `UpdatePSBTOutput` (an analogue to `SignPSBTInput`), `IsSegWitOutput`, and `EvalDescriptorStringOrObject` (implementing the string or object notation parsing used in `scantxoutset`).

ACKs for top commit:
  jnewbery:
    utACK 26fe9b9
  laanwj:
    utACK 26fe9b9 (will hold merging until response to promag's comments)
  promag:
    ACK 26fe9b9, checked refactors and tests look comprehensive. Still missing a release note but can be added later.

Tree-SHA512: 1d833b7351b59d6c5ded6da399ff371a8a2a6ad04c0a8f90e6e46105dc737fa6f2740b1e5340280d59e01f42896c40b720c042f44417e38dfbee6477b894b245

@laanwj laanwj removed this from Blockers in High-priority for review Jul 3, 2019

@fanquake

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

Release notes for this are being added as part of #16326.

@jnewbery

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

This had 3 ACKs. master is always expected to have bugs and if they are trivially fixed in a follow-up, why not?

Because comments should at least be acknowledged by the author, even if that's just to say "This can be addressed in a follow-up PR".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.