Add ability to flush keypool and always flush when upgrading non-HD to HD #23093

meshcollider · 2021-09-25T10:52:18Z

This PR makes two main changes:

Adds a new RPC newkeypool which will entirely flush and refill the keypool.
When upgradewallet is called on old, non-HD wallets upgrading them to HD, we now always flush the keypool and generate a new one, to immediately start using the HD generated keys.

This PR is motivated by a number of users with old, pre-compressed-key wallets upgrading them and being confused about why they still can't generate p2sh-segwit or bech32 addresses -- this is due to uncompressed keys remaining in the keypool post-upgrade and being illegal in these newer address formats. There is currently no easy way to flush the keypool other than to call getnewaddress a hundred/thousand times or an ugly hack of using a sethdseed call.

achow101 · 2021-09-25T16:47:27Z

I would really prefer that we just add a new parameter rather overloading newsize with special behavior for 0. newsize=0 should really just do nothing rather than be special behavior.

Instead of checking the versions and then calling NewKeyPool, it would be simpler and more robust to replace the TopUp call in LegacyScriptPubKeyMan::Upgrade with NewKeyPool(). Especially since that won't potentially throw away all of the newly generated HD keys.

IMO the automatic keypool flush should be optional behavior though. I think it would be better if that were an option to upgradewallet. Doing this defeats the purpose of having the pre-split keypool.

meshcollider · 2021-09-25T19:13:18Z

@achow101 Sure, do you think the option should default to flushing or not though?

achow101 · 2021-09-25T19:16:59Z

I would prefer to keep the existing behavior as default, so default to not flushing.

Edit: Discussed briefly on IRC, flushing after upgrading to HD is fine. Flushing after every upgrade is not IMO.

katesalazar · 2021-09-25T21:14:05Z

There is currently no easy way to flush the keypool other than to call getnewaddress a hundred times or an ugly hack of using a sethdseed call.

#10831

A thousand times.

achow101 · 2021-09-26T04:52:50Z

ACK 84fa19c

laanwj · 2021-09-27T11:42:26Z

Edit: Discussed briefly on IRC, flushing after upgrading to HD is fine. Flushing after every upgrade is not IMO.

Agree; only once, so that users upgrading can use the new address tyeps. Definitely not for every upgrade.

meshcollider · 2021-09-27T12:18:06Z

Agree; only once, so that users upgrading can use the new address tyeps. Definitely not for every upgrade.

👍 (note this is already the behaviour in this PR)

laanwj · 2021-09-28T14:40:27Z

+1 (note this is already the behaviour in this PR)

Concept ACK then!

laanwj · 2021-09-28T14:43:07Z

src/wallet/rpcwallet.cpp

+static RPCHelpMan newkeypool()
+{
+    return RPCHelpMan{"newkeypool",
+                "\nEntirely clears and refills the keypool."+


We might want to be precise in documenting what "clears" means. I understand the keys will not be dealed out anymore by getnewaddress. But what happens to the keys in the old keypool? Are they deleted from the wallet, or still considered for incoming transactions? (I guess the latter)

what happens to the keys in the old keypool? Are they deleted from the wallet, or still considered for incoming transactions? (I guess the latter)

FWIW; if the keys in the pool, not extracted using getnewaddress, can be queried using some wallet software other than Core, then concept NACK to make it the default behavior (I think I've read that was going to be the topic of a different future PR).

OTOH, if they can't be queried, are somehow sealed and inaccessible, then why at all hold them (they are non-HD)?

@laanwj they're marked as used and remain stored but won't be returned from getnewaddress. I'll try and improve the helptext.

@katesalazar I'm not sure what you mean about other software retrieving them - do you mean another piece of software that opens Core wallet.dat files? Otherwise no, without opening the wallet file there is no way an external program could "guess" what the addresses were going to be? Sorry if I'm misunderstanding your point but I don't see anything wrong.

do you mean another piece of software that opens Core wallet.dat files?

yea

@katesalazar Perhaps I'm not understanding you, because I don't see the relevance.

For context, Bitcoin Core wallets distinguish between the set of addresses/keys that are considered part of the wallet (i.e., transactions to them will be treated as incoming payments), and addresses that are given out as new addresses for receiving.

What this PR does is permit refreshing the new addresses that will be given out, but the old ones remain valid for incoming transactions. They can also be queried still by various RPCs that dump things; they just won't be given out by getnewaddress and related procedures.

OK, I understand Samuel better now. Thanks.

I think "consumes" is clearer than "clears".

laanwj · 2021-10-13T13:49:32Z

Code review ACK 84fa19c

re-ACK 6531599

test/functional/wallet_upgradewallet.py

test/functional/wallet_keypool.py

instagibbs · 2021-10-14T02:22:12Z

I'd like coverage of change addresses being flushed and the rewording, otherwise looks correct to me

ghost · 2021-10-14T03:44:32Z

Concept ACK

meshcollider · 2021-10-14T03:55:49Z

Added new commit 6531599 to avoid invalidating previous ACKs.

New commit adds test that newkeypool flushes change addresses too.

instagibbs

ACK 6531599

…en upgr…

harding · 2021-10-18T04:18:28Z

Sorry for the late comment, but I'm curious about the impact of using the newkeypool RPC when it comes to wallet recovery. If Alice creates a new wallet, backs up the wallet, runs newkeypool, runs getnewaddress, receives a payment to that address, and then recovers from the earlier wallet backup, will rescanning find the previously received payment? What if she runs newkeypool twice in a row between backup and recovery?

If there's a possible footgun here, it might be worth documenting it in the RPC help.

meshcollider · 2021-10-23T01:39:29Z

@harding added in #23341. Thanks for pointing this out.

…nd and wallet backups a2a9231 rpc: Add warning to user about newkeypool command (Samuel Dobson) Pull request description: This PR prevents `newkeypool` from being run on non-HD wallets, because this would require a new backup every time, so it isn't very safe. David Harding also suggested [here](bitcoin/bitcoin#23093 (comment)) that the RPC help text should include a warning to the users about the interaction between newkeypool. ACKs for top commit: achow101: ACK a2a9231 Tree-SHA512: 0aa497900f1d179764bce13ffce0bb081ba2ca354492bf2e04b21d0212e960b3ed13a386fbf65602b6b50461f4056a0285752ef707d312da28e82449cd8ea048

…allet backups a2a9231 rpc: Add warning to user about newkeypool command (Samuel Dobson) Pull request description: This PR prevents `newkeypool` from being run on non-HD wallets, because this would require a new backup every time, so it isn't very safe. David Harding also suggested [here](bitcoin#23093 (comment)) that the RPC help text should include a warning to the users about the interaction between newkeypool. ACKs for top commit: achow101: ACK a2a9231 Tree-SHA512: 0aa497900f1d179764bce13ffce0bb081ba2ca354492bf2e04b21d0212e960b3ed13a386fbf65602b6b50461f4056a0285752ef707d312da28e82449cd8ea048

meshcollider added the Wallet label Sep 25, 2021

meshcollider added 5 commits September 26, 2021 14:57

Add newkeypool RPC to flush the keypool

22cc797

Fix outdated keypool size default

2434b10

Make legacy wallet upgrades from non-HD to HD always flush the keypool

6f6f7bb

Add test for flushing keypool with newkeypool

f9603ee

Add release notes for keypool flush changes

84fa19c

laanwj reviewed Sep 28, 2021

View reviewed changes

instagibbs reviewed Oct 14, 2021

View reviewed changes

test/functional/wallet_upgradewallet.py Outdated Show resolved Hide resolved

instagibbs reviewed Oct 14, 2021

View reviewed changes

test/functional/wallet_keypool.py Show resolved Hide resolved

test: Add check that newkeypool flushes change addresses too

6531599

meshcollider requested a review from instagibbs October 14, 2021 03:57

instagibbs approved these changes Oct 14, 2021

View reviewed changes

laanwj merged commit 6419bdf into bitcoin:master Oct 14, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 14, 2021

Merge bitcoin#23093: Add ability to flush keypool and always flush wh…

a909f67

…en upgr…

meshcollider mentioned this pull request Oct 23, 2021

RPC: Better safety with newkeypool command and wallet backups #23341

Merged

meshcollider deleted the 202109_keypoolrefill branch December 20, 2021 07:53

bitcoin locked and limited conversation to collaborators Dec 20, 2022

Add ability to flush keypool and always flush when upgrading non-HD to HD #23093

Add ability to flush keypool and always flush when upgrading non-HD to HD #23093

Uh oh!

Conversation

meshcollider commented Sep 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Sep 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meshcollider commented Sep 25, 2021

Uh oh!

achow101 commented Sep 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

katesalazar commented Sep 25, 2021

Uh oh!

achow101 commented Sep 26, 2021

Uh oh!

laanwj commented Sep 27, 2021

Uh oh!

meshcollider commented Sep 27, 2021

Uh oh!

laanwj commented Sep 28, 2021

Uh oh!

laanwj Sep 28, 2021

Choose a reason for hiding this comment

Uh oh!

katesalazar Sep 28, 2021

Choose a reason for hiding this comment

Uh oh!

meshcollider Sep 28, 2021

Choose a reason for hiding this comment

Uh oh!

katesalazar Sep 29, 2021

Choose a reason for hiding this comment

Uh oh!

sipa Sep 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

katesalazar Sep 29, 2021

Choose a reason for hiding this comment

Uh oh!

instagibbs Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

laanwj commented Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

instagibbs commented Oct 14, 2021

Uh oh!

ghost commented Oct 14, 2021

Uh oh!

meshcollider commented Oct 14, 2021

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

harding commented Oct 18, 2021

Uh oh!

meshcollider commented Oct 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

meshcollider commented Sep 25, 2021 •

edited

Loading

achow101 commented Sep 25, 2021 •

edited

Loading

achow101 commented Sep 25, 2021 •

edited

Loading

sipa Sep 29, 2021 •

edited

Loading

laanwj commented Oct 13, 2021 •

edited

Loading