PSBT key path cleanups by sipa · Pull Request #13723 · bitcoin/bitcoin

sipa · 2018-07-20T07:15:14Z

This PR adds "key origin" (master fingeprint + key path) information to what is exposed from SigningProviders, allowing this information to be used by the generic PSBT code instead of having the RPC pull it directly from the wallet.

This is also a preparation to having PSBT interact with output descriptors, which can then directly expose key origin information for the scripts they generate.

practicalswift · 2018-07-20T11:21:15Z

Concept ACK

Thanks for taking another look at this code

promag

Partial utACK. Nice refactors that improve code readability. Some nits regarding code style.

promag · 2018-07-20T09:58:26Z

src/script/sign.cpp

Commit "Make SigningProvider expose key origin information"

nit CKeyID& keyid.

promag · 2018-07-20T09:59:16Z

src/wallet/wallet.cpp

Commit "Make SigningProvider expose key origin information"

nit, remove.

promag · 2018-07-20T09:59:53Z

src/script/sign.h

Commit "Make SigningProvider expose key origin information"

nit, CKeyID& address.

promag · 2018-07-20T10:01:43Z

src/script/sign.cpp

Commit "Generalize PublicOnlySigningProvider into HidingSigningProvider"

nit, CKeyID& keyid.

promag · 2018-07-20T10:09:20Z

src/script/sign.h

Commit "Generalize PublicOnlySigningProvider into HidingSigningProvider"

nit, CScriptID& scriptid and some more below.

achow101

Concept ACK

achow101 · 2018-07-20T18:58:35Z

src/wallet/rpcwallet.cpp

Shouldn't this change be part of the earlier " Generalize PublicOnlySigningProvider into HidingSigningProvider" commit?

It could be, but doesn't matter. There is no bip32 information in SignatureData until this commit.

achow101 · 2018-07-20T23:11:58Z

src/script/sign.cpp

In commit "Make SigningProvider expose key origin information"

You should change address to keyid in this commit instead of "Generalize PublicOnlySigningProvider into HidingSigningProvider"

DrahtBot · 2018-07-22T13:19:15Z

Note to reviewers: This pull request conflicts with the following ones:

Import key origin data through descriptors in importmulti #14021 (Import key origin data through importmulti by achow101)
Additional utility RPCs for PSBT #13932 (Additional utility RPCs for PSBT by achow101)
util: Add [[nodiscard]] to all {Decode,Parse}[...](...) functions returning bool #13815 (util: Add [[nodiscard]] to all {Decode,Parse}... functions returning bool by practicalswift)
Remove the boost/algorithm/string/case_conv.hpp dependency #13671 (Remove the boost/algorithm/string/case_conv.hpp dependency by 251Labs)

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.

laanwj · 2018-07-23T14:05:33Z

utACK f6ba1ca1e51d044b6fc8398edfa4dd03061572c4
verified that [MOVEONLY] Move ParseHDKeypair is move-only

sipa · 2018-07-24T22:13:36Z

Rebased.

Empact · 2018-08-20T22:26:37Z

src/wallet/rpcwallet.cpp

-}
-
-void AddKeypathToMap(const CWallet* pwallet, const CKeyID& keyID, std::map<CPubKey, std::vector<uint32_t>>& hd_keypaths)
+void AddKeypathToMap(const CWallet* pwallet, const CKeyID& keyID, std::map<CPubKey, KeyOriginInfo>& hd_keypaths)


Looks like this is no longer called.

laanwj · 2018-08-28T14:22:53Z

re-utACK 917353c

917353c Make SignPSBTInput operate on a private SignatureData object (Pieter Wuille) cad5dd2 Pass HD path data through SignatureData (Pieter Wuille) 03a9958 Implement key origin lookup in CWallet (Pieter Wuille) 3b01efa [MOVEONLY] Move ParseHDKeypath to utilstrencodings (Pieter Wuille) 81e1dd5 Generalize PublicOnlySigningProvider into HidingSigningProvider (Pieter Wuille) 84f1f1b Make SigningProvider expose key origin information (Pieter Wuille) 611ab30 Introduce KeyOriginInfo for fingerprint + path (Pieter Wuille) Pull request description: This PR adds "key origin" (master fingeprint + key path) information to what is exposed from `SigningProvider`s, allowing this information to be used by the generic PSBT code instead of having the RPC pull it directly from the wallet. This is also a preparation to having PSBT interact with output descriptors, which can then directly expose key origin information for the scripts they generate. Tree-SHA512: c718382ba8ba2d6fc9a32c062bd4cff08b6f39b133838aa03115c39aeca0f654c7cc3ec72d87005bf8306e550824cd8eb9d60f0bd41784a3e22e17b2afcfe833

merge bitcoin#13269, bitcoin#13425, bitcoin#13557, bitcoin#13721, bitcoin#13666, bitcoin#13723: BIP 174 PSBT Serializations and RPCs

sipa force-pushed the 201807_key_origin_provider branch from db01d22 to 2585c57 Compare July 20, 2018 07:28

promag reviewed Jul 20, 2018

View reviewed changes

sipa force-pushed the 201807_key_origin_provider branch 5 times, most recently from 8afec13 to 8cd99d5 Compare July 20, 2018 18:58

achow101 reviewed Jul 20, 2018

View reviewed changes

sipa force-pushed the 201807_key_origin_provider branch from 8cd99d5 to 8bfa106 Compare July 20, 2018 20:44

achow101 reviewed Jul 20, 2018

View reviewed changes

sipa force-pushed the 201807_key_origin_provider branch from 8bfa106 to f6ba1ca Compare July 20, 2018 23:40

This was referenced Jul 22, 2018

Remove the boost/algorithm/string/case_conv.hpp dependency #13671

Merged

Add CMerkleTx::IsImmatureCoinBase method #13631

Merged

[bugfix] Fix encoding issue for Windows #13426

Closed

laanwj added the RPC/REST/ZMQ label Jul 23, 2018

DrahtBot added the Needs rebase label Jul 24, 2018

sipa force-pushed the 201807_key_origin_provider branch from f6ba1ca to 8df185b Compare July 24, 2018 21:58

DrahtBot removed the Needs rebase label Jul 24, 2018

This was referenced Jul 25, 2018

[WIP] Full unicode support on Windows #13755

Closed

Test for Windows encoding issue #13787

Merged

wallet: -avoidreuse with destination filters #13801

Closed

achow101 and others added 8 commits August 13, 2018 08:41

More tests of signer checks

5df6f08

Introduce KeyOriginInfo for fingerprint + path

611ab30

Make SigningProvider expose key origin information

84f1f1b

Generalize PublicOnlySigningProvider into HidingSigningProvider

81e1dd5

[MOVEONLY] Move ParseHDKeypath to utilstrencodings

3b01efa

Implement key origin lookup in CWallet

03a9958

Pass HD path data through SignatureData

cad5dd2

Make SignPSBTInput operate on a private SignatureData object

917353c

sipa force-pushed the 201807_key_origin_provider branch from 669d72f to 917353c Compare August 13, 2018 15:46

DrahtBot removed the Needs rebase label Aug 13, 2018

Empact reviewed Aug 20, 2018

View reviewed changes

DrahtBot mentioned this pull request Aug 22, 2018

Import key origin data through descriptors in importmulti #14021

Merged

laanwj merged commit 917353c into bitcoin:master Aug 28, 2018

Bushstar mentioned this pull request Sep 4, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#359

Merged

meshcollider mentioned this pull request Dec 6, 2018

Require a public key to be retrieved when signing a P2PKH input #14689

Merged

kwvg mentioned this pull request Jun 4, 2021

merge #13269, #13425, #13557, #13721, #13666, #13723: BIP 174 PSBT Serializations and RPCs dashpay/dash#4186

Merged

UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request Jun 7, 2021

Merge bitcoin#13723: PSBT key path cleanups

e356d8b

kwvg added a commit to kwvg/dash that referenced this pull request Jul 13, 2021

Merge bitcoin#13723: PSBT key path cleanups

8b891c2

PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Jul 13, 2021

Merge pull request #4186 from kittywhiskers/psbt

e98241d

merge bitcoin#13269, bitcoin#13425, bitcoin#13557, bitcoin#13721, bitcoin#13666, bitcoin#13723: BIP 174 PSBT Serializations and RPCs

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

sipa commented Jul 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Jul 20, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jul 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Jul 23, 2018

Uh oh!

sipa commented Jul 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Aug 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

sipa commented Jul 20, 2018 •

edited

Loading

DrahtBot commented Jul 22, 2018 •

edited

Loading