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

rpc: set default bip32derivs to true for psbt methods #17264

Merged
merged 2 commits into from Feb 25, 2020

Conversation

@Sjors
Copy link
Member

Sjors commented Oct 26, 2019

In #13557 (review) I recommended not including bip32 deriviation by default in PSBTs:

Bit of a privacy issue: let's say person A and B are about to spend from a multisig address, sending everything to person A. Person A gives their address to person B, their wallet wallet creates a PSBT, but doesn't sign it. Wallet A then calls walletprocesspsbt which signs it and spontaneously adds the master_fingerprint and bip32 path. Same issue with walletcreatefundedpsbt.

Adding bip32_derivs should probably be opt-in.

In practice I find this default quite annoying because I forget it and end up with a confused hardware wallet.

More importantly, in the multisig example I provided, it's actually essential for the other side to know the derivation details (in addition to an xpub). This allows them to check that change is going to an address you can still co-sign for (because the spending policy is unchanged except for an index).

@fanquake fanquake changed the title [rpc] set default bip32derivs to true for psbt methods rpc: set default bip32derivs to true for psbt methods Oct 26, 2019
@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Oct 26, 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:

  • #18202 (refactor: consolidate sendmany and sendtoaddress code by Sjors)
  • #18115 (wallet: Pass in transactions and messages for signing instead of exporting the private keys by achow101)
  • #18027 ("PSBT Operations" dialog by gwillen)
  • #17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101)
  • #16549 ([WIP] UI external signer support (e.g. hardware wallet) by Sjors)
  • #16546 ([WIP] External signer support - Wallet Box edition by Sjors)
  • #16528 (Native Descriptor Wallets using DescriptorScriptPubKeyMan by achow101)

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.

@jb55

This comment has been minimized.

Copy link
Contributor

jb55 commented Oct 27, 2019

Concept/Approach ACK 29a21c9

@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Nov 14, 2019

imo the privacy ship has sailed on PSBTs, we have no functionality to remove data from PSBTs yet, and no conceptual workflow to manage such a thing. We have thought about this in context of Confidential Assets as well dealing with blinded amounts/assets. It's complicated.

Therefore, concept ACK

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Dec 4, 2019

Privacy in PSBTs is hard; if done properly, it should probably mean having different domains of participants/signers, and filtering out private information when crossing boundaries. I don't think it's simple, or something that can be delegated to users using a flag at signing time (because usually you'd want to reveal that derivation information to e.g. your hardware wallet but not to other signers).

Concept ACK.

@achow101

This comment has been minimized.

Copy link
Member

achow101 commented Dec 4, 2019

ACK 29a21c9

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Jan 31, 2020

Code review ACK 29a21c9. The changes look straightforward, but a test that these fields are filled would be nice.

@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Jan 31, 2020

I added a commit with a test demonstrating the default behavior. Good call, because I discovered #18039 in the process. Update: it's a feature, not a bug :-)

@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Feb 20, 2020

I won't rebase, to preserve ACKs, but I did run the test suite locally on a rebased version.

Copy link
Member

jonatack left a comment

ACK 5bad792 code review, built, ran tests, inspected/messed around with/pprinted values from the new tests. Thanks for adding the tests.

Edit: also built and ran tests after rebasing onto current master @ eb3c6b0

@jonatack

This comment has been minimized.

Copy link
Member

jonatack commented Feb 20, 2020

I wonder if a deprecation process would be good here. Edit: probably not; this fixes an annoyance.

Copy link
Member

instagibbs left a comment

logic ACK, leaving a question about the test

@@ -192,12 +192,20 @@ def run_test(self):
psbt_orig = self.nodes[0].createpsbt([{"txid":txid1, "vout":vout1}, {"txid":txid2, "vout":vout2}], {self.nodes[0].getnewaddress():25.999})

# Update psbts, should only have data for one input and not the other
psbt1 = self.nodes[1].walletprocesspsbt(psbt_orig)['psbt']
psbt1 = self.nodes[1].walletprocesspsbt(psbt_orig, False, "ALL")['psbt']

This comment has been minimized.

Copy link
@instagibbs

instagibbs Feb 20, 2020

Member

why is it important that signing is False and specified sighash flag??

This comment has been minimized.

Copy link
@Sjors

Sjors Feb 20, 2020

Author Member

Because otherwise the test runs into #18039

@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Feb 20, 2020

utACK 5bad792

Copy link
Member

meshcollider left a comment

utACK 5bad792

@meshcollider meshcollider merged commit 31c0006 into bitcoin:master Feb 25, 2020
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Sjors Sjors deleted the Sjors:2019/10/bip32_derivs branch Feb 25, 2020
sidhujag added a commit to syscoin/syscoin that referenced this pull request Feb 25, 2020
…thods

5bad792 [test] PSBT RPC: check that bip32_derivs are present by default (Sjors Provoost)
29a21c9 [rpc] set default bip32derivs to true for psbt methods (Sjors Provoost)

Pull request description:

  In bitcoin#13557 (review) I recommended not including bip32 deriviation by default in PSBTs:

  > _Bit of a privacy issue_: let's say person A and B are about to spend from a multisig address, sending everything to person A. Person A gives their address to person B, their wallet wallet creates a PSBT, but doesn't sign it. Wallet A then calls `walletprocesspsbt` which signs it and _spontaneously adds the master_fingerprint and bip32 path_. Same issue with `walletcreatefundedpsbt`.
  >
  > Adding `bip32_derivs` should probably be opt-in.

  In practice I find this default quite annoying because I forget it and end up with a confused hardware wallet.

  More importantly, in the multisig example I provided, it's actually essential for the other side to know the derivation details (in addition to an xpub). This allows them to check that change is going to an address you can still co-sign for (because the spending policy is unchanged except for an index).

ACKs for top commit:
  instagibbs:
    utACK bitcoin@5bad792
  jonatack:
    ACK 5bad792 code review, built, ran tests, inspected/messed around with/pprinted values from the new tests. Thanks for adding the tests.
  meshcollider:
    utACK 5bad792

Tree-SHA512: 22ad71dda96856060a96758c4ae7aafa22d5e9efba30e0c8287c711e7579849bd72593cbc0f41a2e9e8821315d78bda04e848dbb006283b841b2795e2faebcfd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.