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

Import key origin data through descriptors in importmulti #14021

Merged
merged 9 commits into from Feb 14, 2019

Conversation

@achow101
Copy link
Member

commented Aug 22, 2018

This PR allows for key origin data as defined by the descriptors document to be imported to the wallet when importing a descriptor using importmulti. This allows the walletprocesspsbt to include the BIP 32 derivation paths for keys that it is watching that are from a different HD wallet.

In order to make this easier to use, a new field hdmasterkeyfingerprint has been added to getaddressinfo. Additionally I have removed hdmasterkeyid as was planned. I think that this API change is fine since it was going to be removed in 0.18 anyways. CKeyMetadata has also been extended to store key origin info to facilitate this.

@instagibbs

This comment has been minimized.

Copy link
Member

commented Aug 22, 2018

Nice! strong concept ACK

@achow101 achow101 force-pushed the achow101:import-multi-hd branch Aug 23, 2018

@achow101 achow101 force-pushed the achow101:import-multi-hd branch Aug 25, 2018

@achow101

This comment has been minimized.

Copy link
Member Author

commented Aug 25, 2018

Fixed a bug where the keymetadata wasn't being written to the wallet file. Also added a test for that case.

@achow101 achow101 force-pushed the achow101:import-multi-hd branch 2 times, most recently Aug 26, 2018

@achow101 achow101 force-pushed the achow101:import-multi-hd branch Aug 28, 2018

@achow101

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2018

Rebased and also changed the keymeta stuff to add the hd master key id at key generation and when the wallet is first loaded.

@achow101 achow101 force-pushed the achow101:import-multi-hd branch Aug 28, 2018

@DrahtBot DrahtBot removed the Needs rebase label Aug 28, 2018

@DrahtBot DrahtBot referenced this pull request Aug 29, 2018

Closed

[wallet] Kill accounts #13825

@achow101 achow101 force-pushed the achow101:import-multi-hd branch Aug 29, 2018

src/utilstrencodings.cpp Outdated
@@ -560,6 +560,14 @@ bool ParseHDKeypath(const std::string& keypath_str, std::vector<uint32_t>& keypa
// Finds whether it is hardened
uint32_t path = 0;
size_t pos = item.find("'");
if (pos == std::string::npos) {
// Look for h

This comment has been minimized.

Copy link
@luke-jr

This comment has been minimized.

Copy link
@instagibbs

instagibbs Aug 29, 2018

Member

why not? This is quite standard.

This comment has been minimized.

Copy link
@achow101

achow101 Aug 29, 2018

Author Member

It's fairly standard to use h or H as the indicator, not just '. In fact, the BIP uses H, but as a subscript. ' is not actually used as the indicator for hardened in the BIP even though that is what everyone uses.

The main reason for allowing this is so that you don't have to do any annoying escaping stuff for the ' when entering the keypaths on the command line.

@achow101 achow101 force-pushed the achow101:import-multi-hd branch to 4a9be42 Aug 29, 2018

@achow101

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2018

Rebased

@achow101 achow101 force-pushed the achow101:import-multi-hd branch from 4a9be42 to 44b1725 Sep 4, 2018

@DrahtBot DrahtBot removed the Needs rebase label Sep 4, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

Concept ACK, thanks for working on this

achow101 added some commits Nov 6, 2018

Add WriteHDKeypath function and move *HDKeypath to util/bip32.{h,cpp}
Creates new files util/bip32.h and util/bip32.cpp for containing
BIP 32 stuff.
Moves FormatKeyPath from descriptor.cpp to util/bip32.
Adds a wrapper around it to prepent the 'm' for when just the
BIP 32 style keypath is needed.

@achow101 achow101 force-pushed the achow101:import-multi-hd branch from f442219 to 8fff73c Feb 14, 2019

@instagibbs

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

utACK 8fff73c

@Sjors
Copy link
Member

left a comment

utACK 8fff73c modulo Travis; that was smaller change than I expected

I can't reproduce the failing QT test on Travis on macOS 10.14.3, so I restarted the job.

@@ -135,6 +135,9 @@ enum WalletFlags : uint64_t {
// wallet flags in the upper section (> 1 << 31) will lead to not opening the wallet if flag is unknown

This comment has been minimized.

Copy link
@Sjors

Sjors Feb 14, 2019

Member

Should this say upper section (> 0 << 31) ?

This comment has been minimized.

Copy link
@achow101

achow101 Feb 14, 2019

Author Member

Not my problem :p

Also, no, it shouldn't. Shifting 0 is still 0.

@@ -135,6 +135,9 @@ enum WalletFlags : uint64_t {
// wallet flags in the upper section (> 1 << 31) will lead to not opening the wallet if flag is unknown
// unknown wallet flags in the lower section <= (1 << 31) will be tolerated

// Indicates that the metadata has already been upgraded to contain key origins
WALLET_FLAG_KEY_ORIGIN_METADATA = (1ULL << 0),

This comment has been minimized.

Copy link
@achow101

achow101 Feb 14, 2019

Author Member

Done

@achow101 achow101 force-pushed the achow101:import-multi-hd branch from 8fff73c to 1b1c6aa Feb 14, 2019

@instagibbs

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

only change is WALLET_FLAG_KEY_ORIGIN_METADATA flag from 0 to 1.

re-utACK 1b1c6aa

@Sjors

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

utACK 1b1c6aa

@ajtowns

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

utACK 1b1c6aa

@sipa
Copy link
Member

left a comment

utACK, only nits

CKeyID master_id = masterKey.key.GetPubKey().GetID();
std::copy(master_id.begin(), master_id.begin() + 4, meta.key_origin.fingerprint);
if (!ParseHDKeypath(meta.hdKeypath, meta.key_origin.path)) {
throw std::runtime_error("Invalid stored hdKeypath");

This comment has been minimized.

Copy link
@sipa

sipa Feb 14, 2019

Member

That's correct. If you don't deserialize things at the end of a stream, they just get ignored.

CKeyMetadata& meta = meta_pair.second;
if (!meta.hd_seed_id.IsNull() && meta.key_origin.IsNull() && meta.hdKeypath != "s") { // If the hdKeypath is "s", that's the seed and it doesn't have a key origin
CKey key;
GetKey(meta.hd_seed_id, key);

This comment has been minimized.

Copy link
@sipa

sipa Feb 14, 2019

Member

It seems there is an IsLocked() at the top now, so you've picked the first solution (not upgrading when locked)? If so, I prefer that.

@@ -1261,6 +1262,8 @@ static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, con
if (!pwallet->GetPubKey(id, temp) && !pwallet->AddWatchOnly(GetScriptForRawPubKey(pubkey), timestamp)) {
throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet");
}
pwallet->AddKeyOrigin(pubkey, import_data.key_origins[id]);

This comment has been minimized.

Copy link
@sipa

sipa Feb 14, 2019

Member

Should this be called when id is not a member of import_data.key_origins? The non-descriptor imports won't have this map entries created.

This comment has been minimized.

Copy link
@achow101

achow101 Feb 14, 2019

Author Member

It should check that. I've added the check and a test for this.

Miscellaneous RPC Changes
-------------------------
- Descriptors with key origin information imported through `importmulti` will have their key origin information stored in the wallet for use with creating PSBTs.
- If `bip32derivs` of both `walletprocesspsbt` and `walletcreatefundedpsbt` is set to true but the key metadata for a public key has not been updated yet, then that key will have a derivation path as if it were just an independent key (i.e. no derivation path and it's master fingerprint is itself)

This comment has been minimized.

Copy link
@sipa

sipa Feb 14, 2019

Member

Typo: it's -> its

This comment has been minimized.

Copy link
@achow101

achow101 Feb 14, 2019

Author Member

Done

@achow101 achow101 force-pushed the achow101:import-multi-hd branch from 1b1c6aa to 2e3acea Feb 14, 2019

@achow101

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

oops, screwed up that most recent push, don't merge yet

@achow101 achow101 force-pushed the achow101:import-multi-hd branch from 2e3acea to 1b1c6aa Feb 14, 2019

achow101 added some commits Nov 6, 2018

Store key origin info in key metadata
Store the master key fingerprint and derivation path in the
key metadata. hdKeypath is kept to indicate the seed and for
backwards compatibility, but all key derivation path output
uses the key origin info instead of hdKeypath.

@achow101 achow101 force-pushed the achow101:import-multi-hd branch from 1b1c6aa to cb3511b Feb 14, 2019

@meshcollider

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

Changes since @sipa's are just nit-fixes, a new test, and a missing brace bugfix 👍

@meshcollider meshcollider merged commit cb3511b into bitcoin:master Feb 14, 2019

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

meshcollider added a commit that referenced this pull request Feb 14, 2019

Merge #14021: Import key origin data through descriptors in importmulti
cb3511b Add release notes for importing key origin info change (Andrew Chow)
4c75a69 Test importing descriptors with key origin information (Andrew Chow)
02d6586 Import KeyOriginData when importing descriptors (Andrew Chow)
3d235df Implement a function to add KeyOriginInfo to a wallet (Andrew Chow)
eab63bc Store key origin info in key metadata (Andrew Chow)
345bff6 Remove hdmasterkeyid (Andrew Chow)
bac8c67 Add a method to CWallet to write just CKeyMetadata (Andrew Chow)
e7652d3 Add WriteHDKeypath function and move *HDKeypath to util/bip32.{h,cpp} (Andrew Chow)
c45415f Refactor keymetadata writing to a separate method (Andrew Chow)

Pull request description:

  This PR allows for key origin data as defined by the descriptors document to be imported to the wallet when importing a descriptor using `importmulti`. This allows the `walletprocesspsbt` to include the BIP 32 derivation paths for keys that it is watching that are from a different HD wallet.

  In order to make this easier to use, a new field `hdmasterkeyfingerprint` has been added to `getaddressinfo`. Additionally I have removed `hdmasterkeyid` as was planned. I think that this API change is fine since it was going to be removed in 0.18 anyways. `CKeyMetadata` has also been extended to store key origin info to facilitate this.

Tree-SHA512: 9c7794f3c793da57e23c5abbdc3d58779ee9dea3d53168bb86c0643a4ad5a11a446264961e2f772f35eea645048cb60954ed58050002caee4e43cd9f51215097

@meshcollider meshcollider removed this from Blockers in High-priority for review Feb 14, 2019

meshcollider added a commit to meshcollider/bitcoin that referenced this pull request Feb 18, 2019

Merge bitcoin#15433: Use a single wallet batch for UpgradeKeyMetadata
0bedcba Use a single wallet batch for UpgradeKeyMetadata (Jonas Schnelli)

Pull request description:

  Opening wallets (the first time) after bitcoin#14021 took on my end around 30 seconds due to the keymetadata migration (tested on regtest).

  Using a single wallet batch reduces the required time for the migration down to <1s on my system for a default 2k keypool wallet.

Tree-SHA512: f68739e452d382f5294186f47511b94884a1a0868688dd3179034a7e091a67f93bc9dd45cdfc9fa6b1fe90362772b719278012f2f56b752b803c87db8597a7b0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.