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

[wallet] Upgrade path for non-HD wallets to HD #12560

Merged
merged 6 commits into from
May 14, 2018

Conversation

achow101
Copy link
Member

@achow101 achow101 commented Feb 27, 2018

Revival/rebase of #11085

Adds a new command sethdseed which allows you to either set or generate a new HD seed to be used. A new keypool can be generated or the original one kept and new keys added to the keypool will come from the new HD seed.

Wallets that are not HD will be upgraded to be version FEATURE_HD_SPLIT when the sethdseed RPC command is used.

I have also add some tests for this.

Additionally -upgradewallet can now be used to upgrade a wallet from non-HD to HD. When it is used for such an upgrade, the keypool will be regenerated.

"\nNote that you will need to MAKE A NEW BACKUP of your wallet after setting the HD wallet seed.\n"
+ HelpRequiringPassphrase(pwallet) +
"\nArguments:\n"
"1. \"flushkeypool\" (boolean, optional, default=true) Whether to flush old unused addresses from the keypool.\n"
Copy link
Member

Choose a reason for hiding this comment

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

Text should also tell user that change addresses will be removed(or not)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Also included some more text explaining hd chain split stuff.

CPubKey master_pub_key;
if (!request.params[1].isNull()) {
CBitcoinSecret secret;
if (!secret.SetString(request.params[1].get_str())) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid private key encoding");
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: IMO this inline throw style makes the code less readable, and it's not used elsewhere in the file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed this and other places that have similar inline ifs that are hard to read.

@jonasschnelli
Copy link
Contributor

I think this can be useful, though the following potential risks may be harmful in certain scenarios:

  • Missing key rotation: generating a new hd seed may imply for novice users that this protects from a compromised seed/masterkey.
  • seed option: if someone uses the optional seed parameter, it's possible that the child keys have already been used to send funds to. Eventually a rescan or a UTXO-set scan should be considered?
  • "Upgrading" existing non-hd wallets: leads to a mix of HD non HD-keys. Hard to "untangle" the non HD keys in case one wants to forward all funds to the new hd keyspace.

@achow101
Copy link
Member Author

achow101 commented Mar 1, 2018

Missing key rotation: generating a new hd seed may imply for novice users that this protects from a compromised seed/masterkey.

The help text could be updated to indicate that only coins sent to newly generated addresses will have keys generated with the new seed.

seed option: if someone uses the optional seed parameter, it's possible that the child keys have already been used to send funds to. Eventually a rescan or a UTXO-set scan should be considered?

I could add a rescan option that is disabled by default. It could only be enabled if explicitly set and if the keypool is regenerated.

"Upgrading" existing non-hd wallets: leads to a mix of HD non HD-keys. Hard to "untangle" the non HD keys in case one wants to forward all funds to the new hd keyspace.

At the time of upgrade, one could send all coins to a new address.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

OK

if (!request.params[1].isNull()) {
CBitcoinSecret secret;
if (!secret.SetString(request.params[1].get_str())) {
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid private key encoding");
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing test for this error and another for params[1].get_str().

Copy link
Member Author

Choose a reason for hiding this comment

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

Done


CKey key = secret.GetKey(), key2;
if (!key.IsValid()) {
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Private key outside allowed range");
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing test for this error.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done


bool flush_key_pool = true;
if (!request.params[0].isNull()) {
flush_key_pool = request.params[0].get_bool();
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing test get_bool().

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

// check that we don't already have this key, compressed or otherwise
key2.Set(key.begin(), key.end(), !key.IsCompressed());
if (pwallet->HaveKey(key.GetPubKey().GetID()) || pwallet->HaveKey(key2.GetPubKey().GetID())) {
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Already have this key (either as an HD seed or as a loose private key)");
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing test for this error.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

// Upgrade the wallet to version FEATURE_HD_SPLIT if it isn't aleady
pwallet->SetMinVersion(FEATURE_HD_SPLIT);

if (!pwallet->SetHDMasterKey(master_pub_key)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This always returns true so it's not possible to test the error raised below 🙄

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed the if block

}
if (flush_key_pool) pwallet->NewKeyPool();

return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should test return value (never false). However consider returning an object as per developer notes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to NullUniValue

Copy link
Member

Choose a reason for hiding this comment

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

No, not changed.

"\nNote that you will need to MAKE A NEW BACKUP of your wallet after setting the HD wallet seed.\n"
+ HelpRequiringPassphrase(pwallet) +
"\nArguments:\n"
"1. \"flushkeypool\" (boolean, optional, default=true) Whether to flush old unused addresses, including change addresses, from the keypool.\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe should be newkeypool?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

if (!pwallet->SetHDMasterKey(master_pub_key)) {
throw JSONRPCError(RPC_WALLET_ERROR, "Storing master key failed");
}
if (flush_key_pool) pwallet->NewKeyPool();
Copy link
Contributor

@promag promag Mar 16, 2018

Choose a reason for hiding this comment

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

Return value of NewkeyPool ignored and nothing is logged. Maybe that's fine?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's fine. It only returns false when the wallet is locked, and that is caught earlier with EnsureWalletIsUnlocked

return NullUniValue;
}

if (request.fHelp || request.params.size() > 2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could have a test for extra parameters. See example.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

}

CPubKey master_pub_key;
if (!request.params[1].isNull()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe:

if (request.params[1].isNull()) {
    master_pub_key = pwallet->GenerateNewHDMasterKey();
} else {

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@achow101 achow101 force-pushed the sethdseed branch 3 times, most recently from e277429 to 1bc2bf2 Compare March 18, 2018 19:30
@achow101
Copy link
Member Author

Had to rebase this.

new_masterkeyid = self.nodes[1].getwalletinfo()['hdmasterkeyid']
assert orig_masterkeyid != new_masterkeyid
addr = self.nodes[1].getnewaddress()
assert_equal(self.nodes[1].getaddressinfo(addr)['hdkeypath'], 'm/0\'/0\'/0\'') # Make sure the new address is the first from the keypool
Copy link
Member

Choose a reason for hiding this comment

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

nit: also check it's not the same key(not just path)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

assert_equal(self.nodes[1].getaddressinfo(addr)['hdkeypath'], 'm/0\'/0\'/0\'') # Make sure the new address continues previous keypool

# Sethdseed parameter validity
assert_raises_rpc_error(-1, 'sethdseed', self.nodes[0].sethdseed, False, new_seed, 0)
Copy link
Member

Choose a reason for hiding this comment

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

is this for invalid named arguments? Without running this I'm not sure what it's doing

Copy link
Member Author

Choose a reason for hiding this comment

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

This just tests that an error is thrown when there are too many arguments

" If true, the next address from getnewaddress and change address from getrawchangeaddress will be from this new seed.\n"
" If false, addresses (including change addresses if the wallet already had HD Chain Split enabled) from the existing \n"
" keypool will be used until it has been depleted.\n"
"2. \"seed\" (string, optional) The WIF private key to use as the new HD seed;\n"
Copy link
Member

@instagibbs instagibbs Mar 23, 2018

Choose a reason for hiding this comment

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

I think it stands to be noted that Core software will never(?) return this value on a currently running wallet. You can get address' keys, but not the seed itself. I can easily see users getting confused and thinking that backing up some address key will save their wallet in the future.

You may want to leave a note here saying how this specific key can be extracted from the wallet, through dumpwallet->hdmaster=1 key

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid private key");
}

// check that we don't already have this key, compressed or otherwise
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason for disallowing master keys with opposite compressedness? Unless another party knows the master xpubkey, they would not be able to tell that any child keys are related through a shared master key.

Copy link
Member Author

Choose a reason for hiding this comment

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

The seed value is interpreted and saved as private key. We don't want to have a seed where we already have that seed value as a key in the wallet. The only way to check whether we have a given private key is to derive its public key, get the id of that public key, and look it up. So to ensure that we don't have the seed value already, we need to derive both compressed and uncompressed public keys, get their ids, and look them up.

It's important to note that the seed is not actually a private key so it doesn't have a corresponding public key. We just use WIF because its a common format.

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the problem with having a seed that is the same private key with opposite compressedness? It wouldn't have an existing entry in the the wallet file because the CKeyID is different.

If there is a good reason to protect against this, it might make sense to add a bool HaveKey(CKey& key) const method on CKeyStore.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we use a key that already exists in the wallet but with opposite compressedness, that key could be exportable with dumpprivkey. But the user would not necessarily know that the key that is exported is their seed just with opposite compressedness. I don't think that we want the seed to be exportable like that, and if we do want the seed to be exported in some way, it should be via its own RPC and not through dumpprivkey.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. I'm still not convinced (I could make similar arguments about rejecting keys with the lowest bit toggled), but it doesn't really matter.

I would prefer to see the lookups moved over to a HaveKey(CKey& key) method on KeyStore though.

Copy link
Member

Choose a reason for hiding this comment

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

@jimpo I don't understand what HaveKey(CKey) would do. There is a 1-to-1 correspondence both ways between CKeys and CPubKeys.

Copy link
Contributor

@jimpo jimpo Apr 12, 2018

Choose a reason for hiding this comment

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

@sipa It would replicate the logic here that checks whether there is a CPubKey corresponding to either the provided CKey or the one with opposite compressedness. Basically, I don't think that this RPC function is the best place for that logic. Maybe the method would be better named HaveKeyIgnoringCompressedness.

Copy link
Member

Choose a reason for hiding this comment

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

@jimpo Ah, I see. My concern is adding extra unnecessary functions to the CKeyStore interface (which need to be implemented by all implementations, even though there's just one now). What would you think about having a function (not method) in keystore.h/cpp that does this, which takes a const CKeyStore& as argument?

Copy link
Member Author

Choose a reason for hiding this comment

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

I implemented @sipa's suggestion

@achow101 achow101 force-pushed the sethdseed branch 2 times, most recently from 42372f2 to bade3fc Compare March 28, 2018 18:17
@TheBlueMatt
Copy link
Contributor

At the risk of expanding the scope, can we fix -upgradewallet to work for HD as well here? The fact that upgradewallet will work to upgrade your wallet to HD-1 and then you have to call sethdseed to upgrade seems....strange.
Separately, can we either check that we're fully synced before allowing an HD master rotate or keep around old HD keys for key derivation? I'd prefer the second, but its obviously a ton more complicated, so just ensuring that we're at least synced first is likely sufficient to ensure people dont rotate and miss some payments.

@achow101 achow101 changed the title [wallet][RPC] Set or generate a new HD seed [wallet] Upgrade path for non-HD wallets to HD Mar 31, 2018
@achow101
Copy link
Member Author

I changed -upgradewallet so it upgrades non-HD wallets to HD and HD chain split. I've updated the OP and title to indicate as such.

Unfortunately automated tests can't be done with -upgradewallet.

Separately, can we either check that we're fully synced before allowing an HD master rotate

This seems incompatible with -upgradewallet as an upgrade method.

or keep around old HD keys for key derivation? I'd prefer the second, but its obviously a ton more complicated, so just ensuring that we're at least synced first is likely sufficient to ensure people dont rotate and miss some payments.

I think this should be done as a separate PR. It's quite a bit of scope creep.

@TheBlueMatt
Copy link
Contributor

This seems incompatible with -upgradewallet as an upgrade method.

I dont care as much about it for first-upgrade (first-upgrade-to-HD is actually no different from just using your existing wallet from a backup/keypool perspective, as long as you dont flush keypool, its just a new way to generate keypool entries), but for HD master rotation, I think making sure we dont need any future new keys is pretty easy to add, no?

@achow101
Copy link
Member Author

achow101 commented Apr 5, 2018

I dont care as much about it for first-upgrade (first-upgrade-to-HD is actually no different from just using your existing wallet from a backup/keypool perspective

Right, duh.


I rebased this and added a check for IBD so it won't set a new seed if we are still in IBD.

Copy link
Contributor

@kallewoof kallewoof left a comment

Choose a reason for hiding this comment

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

re-utACK ae810ff48650a07160877199f9283edfda1b3632

"2. \"seed\" (string, optional) The WIF private key to use as the new HD seed; if not provided a random seed will be used.\n"
" The seed value can be retrieved using the dumpwallet command. It is the private key marked hdmaster=1\n"
"\nResult:\n"
"null (boolean) Returns null if successful\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

Just leave the result section out -- e.g. like in setlabel (L326).

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@jonasschnelli jonasschnelli added this to the 0.15.2 milestone May 12, 2018
{
CKey key2;
key2.Set(key.begin(), key.end(), !key.IsCompressed());
return store.HaveKey(key.GetPubKey().GetID()) || store.HaveKey(key2.GetPubKey().GetID());
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this have performance impacts?
What are the downside of not checking the non-compressed (or compressed if non-compressed)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think there are performance impacts, or if there are, they are not noticeable. The reason to check for both is to avoid having a seed value which was already in the wallet before and thus could have been revealed.

Copy link
Member

Choose a reason for hiding this comment

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

If there's a noticable performance impact (it should perhaps be 50-100 microseconds extra), we can create a function to simultaneously compute the compressed and uncompressed pubkey for a secret. Can be done later, and it's probably not necessarily.

}

if (IsInitialBlockDownload()) {
throw JSONRPCError(RPC_CLIENT_IN_INITIAL_DOWNLOAD, "Cannot set a new HD seed while still in Initial Block Download");
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you elaborate why a IBD check is necessary and why the wallet-key-state is connected to IBD?

Copy link
Member Author

Choose a reason for hiding this comment

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

From Matt's earlier comments:

Separately, can we either check that we're fully synced before allowing an HD master rotate or keep around old HD keys for key derivation? I'd prefer the second, but its obviously a ton more complicated, so just ensuring that we're at least synced first is likely sufficient to ensure people dont rotate and miss some payments.

@jonasschnelli
Copy link
Contributor

Concept ACK, will test more later.

dooglus and others added 5 commits May 12, 2018 13:15
Changes the maximum upgradewallet version to the latest wallet version
number, 159900. Non-HD wallets will be upgraded to use HD derivation.
Non HD chain split wallets will be upgraded to HD chain split.

If a non-HD wallet is upgraded to HD, the keypool will be entirely
regenerated.

Since upgradewallet is effectively run during a first run, all of the
first run initial setup stuff is combined with the upgrade to HD
After upgrading to HD chain split, we want to continue to use keys
from the old keypool. To do this, before we generate any new keys after
upgrading, we mark all of the keypool entries as being pre-chain
split and move them to a separate pre chain split keypool. Keys are
fetched from that keypool until it is emptied. Only then are the new
internal and external keypools used.
Bump the wallet version to indicate support for the pre split keypool.
Also prevents any wallets from upgrading to versions between HD_SPLIT
and PRE_SPLIT_KEYPOOL.
Copy link
Member

@sipa sipa left a comment

Choose a reason for hiding this comment

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

utACK a8da482

{
CKey key2;
key2.Set(key.begin(), key.end(), !key.IsCompressed());
return store.HaveKey(key.GetPubKey().GetID()) || store.HaveKey(key2.GetPubKey().GetID());
Copy link
Member

Choose a reason for hiding this comment

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

If there's a noticable performance impact (it should perhaps be 50-100 microseconds extra), we can create a function to simultaneously compute the compressed and uncompressed pubkey for a secret. Can be done later, and it's probably not necessarily.

+ HelpExampleCli("sethdseed", "")
+ HelpExampleCli("sethdseed", "false")
+ HelpExampleCli("sethdseed", "true \"wifkey\"")
+ HelpExampleRpc("sethdseed", "true, \"wifkey\"")
Copy link
Member

Choose a reason for hiding this comment

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

Really? That's not even valid JSON :)

@@ -95,7 +95,7 @@ enum WalletFeature

FEATURE_NO_DEFAULT_KEY = 159900, // Wallet without a default key written

FEATURE_LATEST = FEATURE_COMPRPUBKEY // HD is optional, use FEATURE_COMPRPUBKEY as latest version
FEATURE_LATEST = FEATURE_NO_DEFAULT_KEY
Copy link
Member

Choose a reason for hiding this comment

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

@TheBlueMatt Does that matter? This is just the default for new wallets or explicit upgrades. I don't really care about gratuitous incompatibility when you're explicitly not caring about that.

UPDATE: especially with the new PRE_SPLIT type added afterwards this is probably inevitable.

@maflcko
Copy link
Member

maflcko commented May 13, 2018

@jonasschnelli You added this to the 15.2 milestone without the "needs backport" label. Was this done in mistake? I think a backport should be discussed shortly on irc

@laanwj
Copy link
Member

laanwj commented May 14, 2018 via email

@laanwj laanwj merged commit a8da482 into bitcoin:master May 14, 2018
laanwj added a commit that referenced this pull request May 14, 2018
a8da482 Bump wallet version for pre split keypool (Andrew Chow)
dfcd9f3 Use a keypool of presplit keys after upgrading to hd chain split (Andrew Chow)
5c50e93 Allow -upgradewallet to upgradewallets to HD (Andrew Chow)
2bcf2b5 Test sethdseed (Andrew Chow)
b5ba01a Add 'sethdseed' RPC to initialize or replace HD seed (Chris Moore)
dd3c07a Separate HaveKey function that checks whether a key is in a keystore (Andrew Chow)

Pull request description:

  Revival/rebase of #11085

  Adds a new command `sethdseed` which allows you to either set or generate a new HD seed to be used. A new keypool can be generated or the original one kept and new keys added to the keypool will come from the new HD seed.

  Wallets that are not HD will be upgraded to be version FEATURE_HD_SPLIT when the `sethdseed` RPC command is used.

  I have also add some tests for this.

  Additionally `-upgradewallet` can now be used to upgrade a wallet from non-HD to HD. When it is used for such an upgrade, the keypool will be regenerated.

Tree-SHA512: e56c792e150590429ac4a1061e8d6f7b20cca06366e184eb9bbade4cd6ae82699a28fe84f87031eadba97ad2c1606517a105f00fb7b45779c979243020071adb
meshcollider added a commit to bitcoin-core/gui that referenced this pull request Nov 16, 2021
…ated with bitcoin-wallet

5b6b5ef util: Use FEATURE_LATEST for wallets created with bitcoin-wallet (Hennadii Stepanov)

Pull request description:

  Since the 49d2374 commit was athored by **jonasschnelli** in 2016, the wallet version was bumped twice: in 2017 (bitcoin/bitcoin#11250) and in 2018 (bitcoin/bitcoin#12560).

  This PR bumps the version of wallets created with `bitcoin-wallet` offline tool.

  On master (04437ee) -- `"walletversion": 139900`:
  ```
  $ src/bitcoin-wallet -signet -wallet=211025-test-master create
  Topping up keypool...
  Wallet info
  ===========
  Name: 211025-test-master
  Format: sqlite
  Descriptors: yes
  Encrypted: no
  HD (hd seed available): yes
  Keypool Size: 6000
  Transactions: 0
  Address Book: 0
  $ src/bitcoin-cli -signet -rpcwallet=211025-test-master getwalletinfo
  {
    "walletname": "211025-test-master",
    "walletversion": 139900,
    "format": "sqlite",
    "balance": 0.00000000,
    "unconfirmed_balance": 0.00000000,
    "immature_balance": 0.00000000,
    "txcount": 0,
    "keypoolsize": 3000,
    "keypoolsize_hd_internal": 3000,
    "paytxfee": 0.00000000,
    "private_keys_enabled": true,
    "avoid_reuse": false,
    "scanning": false,
    "descriptors": true
  }
  ```

  With this PR -- `"walletversion": 169900`:
  ```
  $ src/bitcoin-wallet -signet -wallet=211025-test-pr create
  Topping up keypool...
  Wallet info
  ===========
  Name: 211025-test-pr
  Format: sqlite
  Descriptors: yes
  Encrypted: no
  HD (hd seed available): yes
  Keypool Size: 6000
  Transactions: 0
  Address Book: 0
  $ src/bitcoin-cli -signet -rpcwallet=211025-test-pr getwalletinfo
  {
    "walletname": "211025-test-pr",
    "walletversion": 169900,
    "format": "sqlite",
    "balance": 0.00000000,
    "unconfirmed_balance": 0.00000000,
    "immature_balance": 0.00000000,
    "txcount": 0,
    "keypoolsize": 3000,
    "keypoolsize_hd_internal": 3000,
    "paytxfee": 0.00000000,
    "private_keys_enabled": true,
    "avoid_reuse": false,
    "scanning": false,
    "descriptors": true
  }
  ```

ACKs for top commit:
  lsilva01:
    Code Review ACK 5b6b5ef
  stratospher:
    ACK 5b6b5ef.
  rajarshimaitra:
    ACK bitcoin/bitcoin@5b6b5ef
  meshcollider:
    Code review ACK 5b6b5ef

Tree-SHA512: 0221e76fa8f29037920d0a483c742bf270ecaead45f30230943b78775aaea63ac052e43fe712d15c2326e515dea2d2ac82de0924882598421c1874f2e6f442a6
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 16, 2021
…h bitcoin-wallet

5b6b5ef util: Use FEATURE_LATEST for wallets created with bitcoin-wallet (Hennadii Stepanov)

Pull request description:

  Since the 49d2374 commit was athored by **jonasschnelli** in 2016, the wallet version was bumped twice: in 2017 (bitcoin#11250) and in 2018 (bitcoin#12560).

  This PR bumps the version of wallets created with `bitcoin-wallet` offline tool.

  On master (04437ee) -- `"walletversion": 139900`:
  ```
  $ src/bitcoin-wallet -signet -wallet=211025-test-master create
  Topping up keypool...
  Wallet info
  ===========
  Name: 211025-test-master
  Format: sqlite
  Descriptors: yes
  Encrypted: no
  HD (hd seed available): yes
  Keypool Size: 6000
  Transactions: 0
  Address Book: 0
  $ src/bitcoin-cli -signet -rpcwallet=211025-test-master getwalletinfo
  {
    "walletname": "211025-test-master",
    "walletversion": 139900,
    "format": "sqlite",
    "balance": 0.00000000,
    "unconfirmed_balance": 0.00000000,
    "immature_balance": 0.00000000,
    "txcount": 0,
    "keypoolsize": 3000,
    "keypoolsize_hd_internal": 3000,
    "paytxfee": 0.00000000,
    "private_keys_enabled": true,
    "avoid_reuse": false,
    "scanning": false,
    "descriptors": true
  }
  ```

  With this PR -- `"walletversion": 169900`:
  ```
  $ src/bitcoin-wallet -signet -wallet=211025-test-pr create
  Topping up keypool...
  Wallet info
  ===========
  Name: 211025-test-pr
  Format: sqlite
  Descriptors: yes
  Encrypted: no
  HD (hd seed available): yes
  Keypool Size: 6000
  Transactions: 0
  Address Book: 0
  $ src/bitcoin-cli -signet -rpcwallet=211025-test-pr getwalletinfo
  {
    "walletname": "211025-test-pr",
    "walletversion": 169900,
    "format": "sqlite",
    "balance": 0.00000000,
    "unconfirmed_balance": 0.00000000,
    "immature_balance": 0.00000000,
    "txcount": 0,
    "keypoolsize": 3000,
    "keypoolsize_hd_internal": 3000,
    "paytxfee": 0.00000000,
    "private_keys_enabled": true,
    "avoid_reuse": false,
    "scanning": false,
    "descriptors": true
  }
  ```

ACKs for top commit:
  lsilva01:
    Code Review ACK 5b6b5ef
  stratospher:
    ACK 5b6b5ef.
  rajarshimaitra:
    ACK bitcoin@5b6b5ef
  meshcollider:
    Code review ACK 5b6b5ef

Tree-SHA512: 0221e76fa8f29037920d0a483c742bf270ecaead45f30230943b78775aaea63ac052e43fe712d15c2326e515dea2d2ac82de0924882598421c1874f2e6f442a6
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.