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

PSBT key path cleanups #13723

Merged
merged 11 commits into from Aug 28, 2018
Merged

PSBT key path cleanups #13723

merged 11 commits into from Aug 28, 2018

Conversation

@sipa
Copy link
Member

@sipa sipa commented Jul 20, 2018

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.

@sipa sipa force-pushed the 201807_key_origin_provider branch from db01d22 to 2585c57 Jul 20, 2018
@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Jul 20, 2018

Concept ACK

Thanks for taking another look at this code

Loading

Copy link
Member

@promag promag left a comment

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

Loading

@@ -624,3 +624,8 @@ bool PublicOnlySigningProvider::GetPubKey(const CKeyID &address, CPubKey& pubkey
{
return m_provider->GetPubKey(address, pubkey);
}

bool PublicOnlySigningProvider::GetKeyOrigin(const CKeyID &keyid, KeyOriginInfo& info) const
Copy link
Member

@promag promag Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit "Make SigningProvider expose key origin information"

nit CKeyID& keyid.

Loading

Copy link
Member Author

@sipa sipa Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Loading

@@ -4378,4 +4378,3 @@ void CWallet::LearnAllRelatedScripts(const CPubKey& key)
// OutputType::P2SH_SEGWIT always adds all necessary scripts for all types.
LearnRelatedScripts(key, OutputType::P2SH_SEGWIT);
}

Copy link
Member

@promag promag Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit "Make SigningProvider expose key origin information"

nit, remove.

Loading

Copy link
Member Author

@sipa sipa Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Loading

@@ -47,6 +48,7 @@ class PublicOnlySigningProvider : public SigningProvider
PublicOnlySigningProvider(const SigningProvider* provider) : m_provider(provider) {}
bool GetCScript(const CScriptID &scriptid, CScript& script) const;
bool GetPubKey(const CKeyID &address, CPubKey& pubkey) const;
bool GetKeyOrigin(const CKeyID &address, KeyOriginInfo& info) const;
Copy link
Member

@promag promag Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit "Make SigningProvider expose key origin information"

nit, CKeyID& address.

Loading

Copy link
Member Author

@sipa sipa Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Loading

return m_provider->GetKey(keyid, key);
}

bool HidingSigningProvider::GetKeyOrigin(const CKeyID &keyid, KeyOriginInfo& info) const
Copy link
Member

@promag promag Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit "Generalize PublicOnlySigningProvider into HidingSigningProvider"

nit, CKeyID& keyid.

Loading

Copy link
Member Author

@sipa sipa Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Loading

bool GetPubKey(const CKeyID &address, CPubKey& pubkey) const;
bool GetKeyOrigin(const CKeyID &address, KeyOriginInfo& info) const;
HidingSigningProvider(const SigningProvider* provider, bool hide_secret, bool hide_origin) : m_hide_secret(hide_secret), m_hide_origin(hide_origin), m_provider(provider) {}
bool GetCScript(const CScriptID &scriptid, CScript& script) const override;
Copy link
Member

@promag promag Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit "Generalize PublicOnlySigningProvider into HidingSigningProvider"

nit, CScriptID& scriptid and some more below.

Loading

Copy link
Member Author

@sipa sipa Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Loading

@sipa sipa force-pushed the 201807_key_origin_provider branch 5 times, most recently from 8afec13 to 8cd99d5 Jul 20, 2018
Copy link
Member

@achow101 achow101 left a comment

Concept ACK

Loading

@@ -4461,21 +4461,14 @@ bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, const C
}

SignatureData sigdata;
complete &= SignPSBTInput(HidingSigningProvider(pwallet, !sign, false), *psbtx.tx, input, sigdata, i, sighash_type);
complete &= SignPSBTInput(HidingSigningProvider(pwallet, !sign, !bip32derivs), *psbtx.tx, input, sigdata, i, sighash_type);
Copy link
Member

@achow101 achow101 Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Member Author

@sipa sipa Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

@sipa sipa force-pushed the 201807_key_origin_provider branch from 8cd99d5 to 8bfa106 Jul 20, 2018
@@ -620,7 +620,12 @@ bool PublicOnlySigningProvider::GetCScript(const CScriptID &scriptid, CScript& s
return m_provider->GetCScript(scriptid, script);
}

bool PublicOnlySigningProvider::GetPubKey(const CKeyID &address, CPubKey& pubkey) const
bool PublicOnlySigningProvider::GetPubKey(const CKeyID& keyid, CPubKey& pubkey) const
{
return m_provider->GetPubKey(address, pubkey);
Copy link
Member

@achow101 achow101 Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "Make SigningProvider expose key origin information"

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

Loading

Copy link
Member Author

@sipa sipa Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Loading

@sipa sipa force-pushed the 201807_key_origin_provider branch from 8bfa106 to f6ba1ca Jul 20, 2018
@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Jul 22, 2018

Note to reviewers: This pull request conflicts with the following ones:
  • #14021 (Import key origin data through importmulti by achow101)
  • #13932 (Additional utility RPCs for PSBT by achow101)
  • #13815 (util: Add [[nodiscard]] to all {Decode,Parse}... functions returning bool by practicalswift)
  • #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.

Loading

@laanwj
Copy link
Member

@laanwj laanwj commented Jul 23, 2018

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

Loading

@sipa sipa force-pushed the 201807_key_origin_provider branch from f6ba1ca to 8df185b Jul 24, 2018
@sipa
Copy link
Member Author

@sipa sipa commented Jul 24, 2018

Rebased.

Loading

@sipa sipa force-pushed the 201807_key_origin_provider branch from 669d72f to 917353c Aug 13, 2018
@laanwj laanwj added this to Blockers in High-priority for review Aug 16, 2018
}

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

@Empact Empact Aug 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is no longer called.

Loading

@laanwj
Copy link
Member

@laanwj laanwj commented Aug 28, 2018

re-utACK 917353c

Loading

@laanwj laanwj merged commit 917353c into bitcoin:master Aug 28, 2018
1 check passed
Loading
laanwj added a commit that referenced this issue Aug 28, 2018
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
@MarcoFalke MarcoFalke removed this from Blockers in High-priority for review Aug 28, 2018
kittywhiskers added a commit to kittywhiskers/dash that referenced this issue Jun 4, 2021
kittywhiskers added a commit to kittywhiskers/dash that referenced this issue Jun 4, 2021
UdjinM6 added a commit to UdjinM6/dash that referenced this issue Jun 7, 2021
barrystyle added a commit to pacprotocol/pacprotocol that referenced this issue Jun 8, 2021
barrystyle added a commit to pacprotocol/pacprotocol that referenced this issue Jun 8, 2021
kittywhiskers added a commit to kittywhiskers/dash that referenced this issue Jun 9, 2021
kittywhiskers added a commit to kittywhiskers/dash that referenced this issue Jul 9, 2021
kittywhiskers added a commit to kittywhiskers/dash that referenced this issue Jul 13, 2021
kittywhiskers added a commit to kittywhiskers/dash that referenced this issue Jul 13, 2021
kittywhiskers added a commit to kittywhiskers/dash that referenced this issue Jul 13, 2021
PastaPastaPasta added a commit to dashpay/dash that referenced this issue Jul 13, 2021
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants