rpc: set default bip32derivs to true for psbt methods

Sjors commented at 10:10 am on October 26, 2019: member

In #13557#pullrequestreview-135905054 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).

[rpc] set default bip32derivs to true for psbt methods 29a21c9061

fanquake added the label RPC/REST/ZMQ on Oct 26, 2019

fanquake renamed this:
~~[rpc] set default bip32derivs to true for psbt methods~~
rpc: set default bip32derivs to true for psbt methods
on Oct 26, 2019

DrahtBot commented at 11:55 am on October 26, 2019: 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:

#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 commented at 12:10 pm on October 27, 2019: member

Concept/Approach ACK 29a21c90610aed88b796a7a5900e42e9048b990e

instagibbs commented at 4:28 pm on November 14, 2019: member

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 commented at 7:17 pm on December 4, 2019: member

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 commented at 7:31 pm on December 4, 2019: member

ACK 29a21c90610aed88b796a7a5900e42e9048b990e

sipa commented at 2:43 am on January 31, 2020: member

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

[test] PSBT RPC: check that bip32_derivs are present by default 5bad7921d0

Sjors commented at 9:39 am on January 31, 2020: member

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 commented at 3:30 pm on February 20, 2020: member

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

jonatack commented at 4:31 pm on February 20, 2020: member

ACK 5bad7921d0 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 @ eb3c6b09

jonatack commented at 4:35 pm on February 20, 2020: member

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

in test/functional/rpc_psbt.py:195 in 5bad7921d0

191@@ -192,12 +192,20 @@ def run_test(self):
192         psbt_orig = self.nodes[0].createpsbt([{"txid":txid1,  "vout":vout1}, {"txid":txid2, "vout":vout2}], {self.nodes[0].getnewaddress():25.999})
193 
194         # Update psbts, should only have data for one input and not the other
195-        psbt1 = self.nodes[1].walletprocesspsbt(psbt_orig)['psbt']
196+        psbt1 = self.nodes[1].walletprocesspsbt(psbt_orig, False, "ALL")['psbt']

instagibbs commented at 5:45 pm on February 20, 2020:

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

Sjors commented at 7:57 pm on February 20, 2020:

Because otherwise the test runs into #18039

instagibbs commented at 5:46 pm on February 20, 2020: member

logic ACK, leaving a question about the test

instagibbs commented at 9:21 pm on February 20, 2020: member

utACK https://github.com/bitcoin/bitcoin/pull/17264/commits/5bad7921d0b33b62c0a59a478c2e8c869fc5e3b5

meshcollider commented at 10:48 am on February 25, 2020: contributor

utACK 5bad7921d0b33b62c0a59a478c2e8c869fc5e3b5

meshcollider merged this on Feb 25, 2020

meshcollider closed this on Feb 25, 2020

Sjors deleted the branch on Feb 25, 2020

sidhujag referenced this in commit 421889f107 on Feb 25, 2020

jasonbcox referenced this in commit c6f37464e3 on Oct 25, 2020

sidhujag referenced this in commit 5813be17ef on Nov 10, 2020

DrahtBot locked this on Aug 16, 2022

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

Conflicts