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

meshcollider commented at 10:52 AM on September 25, 2021: contributor

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.

meshcollider added the label Wallet on Sep 25, 2021

achow101 commented at 4:47 PM on September 25, 2021: member

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 commented at 7:13 PM on September 25, 2021: contributor

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

achow101 commented at 7:16 PM on September 25, 2021: member

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 commented at 9:14 PM on September 25, 2021: contributor

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.

Add newkeypool RPC to flush the keypool 22cc797ca5

Fix outdated keypool size default 2434b10781

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

Add test for flushing keypool with newkeypool f9603ee4e0

Add release notes for keypool flush changes 84fa19c77a

achow101 commented at 4:52 AM on September 26, 2021: member

ACK 84fa19c77a2c8d0d01add2daf18b42af07c17710

laanwj commented at 11:42 AM on September 27, 2021: member

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 commented at 12:18 PM on September 27, 2021: contributor

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 commented at 2:40 PM on September 28, 2021: member

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

Concept ACK then!

in src/wallet/rpcwallet.cpp:1898 in 84fa19c77a outdated

1891 | @@ -1892,6 +1892,33 @@ static RPCHelpMan keypoolrefill()
1892 |  }
1893 |  
1894 |  
1895 | +static RPCHelpMan newkeypool()
1896 | +{
1897 | +    return RPCHelpMan{"newkeypool",
1898 | +                "\nEntirely clears and refills the keypool."+

laanwj commented at 2:43 PM on September 28, 2021:

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)

katesalazar commented at 3:42 PM on September 28, 2021:

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)?

meshcollider commented at 8:49 PM on September 28, 2021:

@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.

katesalazar commented at 1:27 PM on September 29, 2021:

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

yea

sipa commented at 1:54 PM on September 29, 2021:

@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.

katesalazar commented at 5:49 PM on September 29, 2021:

OK, I understand Samuel better now. Thanks.

instagibbs commented at 2:17 AM on October 14, 2021:

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

laanwj commented at 1:49 PM on October 13, 2021: member

Code review ACK 84fa19c77a2c8d0d01add2daf18b42af07c17710

re-ACK 6531599f422524fbbcc43816121e7536cf79d66c

in test/functional/wallet_upgradewallet.py:238 in 6f6f7bb36c outdated

 239 | -        for i in range(0, 2):
 240 | -            info = wallet.getaddressinfo(wallet.getnewaddress())
 241 | -            assert 'hdkeypath' not in info
 242 | -            assert 'hdseedid' not in info
 243 | -        # Next key should be HD
 244 | +        # New keys should be HD (the two old keys have been flushed)

instagibbs commented at 2:20 AM on October 14, 2021:

do we test that change addresses have also flushed?

meshcollider commented at 3:56 AM on October 14, 2021:

Yep, clarified comment :)

in test/functional/wallet_keypool.py:143 in f9603ee4e0 outdated

 137 | @@ -138,6 +138,16 @@ def run_test(self):
 138 |              assert_equal(wi['keypoolsize_hd_internal'], 100)
 139 |              assert_equal(wi['keypoolsize'], 100)
 140 |  
 141 | +        if not self.options.descriptors:
 142 | +            # Check that newkeypool entirely flushes the keypool
 143 | +            start_keypath = nodes[0].getaddressinfo(nodes[0].getnewaddress())['hdkeypath']

instagibbs commented at 2:21 AM on October 14, 2021:

let's test for change address flushing too

meshcollider commented at 3:56 AM on October 14, 2021:

Done

instagibbs commented at 2:22 AM on October 14, 2021: member

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

ghost commented at 3:44 AM on October 14, 2021: none

Concept ACK

test: Add check that newkeypool flushes change addresses too 6531599f42

meshcollider commented at 3:55 AM on October 14, 2021: contributor

Added new commit 6531599f422524fbbcc43816121e7536cf79d66c to avoid invalidating previous ACKs.

New commit adds test that newkeypool flushes change addresses too.

meshcollider requested review from instagibbs on Oct 14, 2021

instagibbs approved

instagibbs commented at 4:00 AM on October 14, 2021: member

ACK https://github.com/bitcoin/bitcoin/pull/23093/commits/6531599f422524fbbcc43816121e7536cf79d66c

laanwj merged this on Oct 14, 2021

laanwj closed this on Oct 14, 2021

sidhujag referenced this in commit a909f678d2 on Oct 14, 2021

harding commented at 4:18 AM on October 18, 2021: contributor

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 commented at 1:39 AM on October 23, 2021: contributor

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

meshcollider deleted the branch on Dec 20, 2021

achow101 referenced this in commit 887796a5ff on Dec 20, 2021

sidhujag referenced this in commit 673b1dcd79 on Dec 20, 2021

DrahtBot locked this on Dec 20, 2022