The PR branch HEAD was 218c4f640 at the time of this review club meeting.
Notes
The
sethdseed
RPC creates a new BIP
32 HD seed for
the wallet. The previous keys or seeds are no longer used to generate receive
or change addresses.
If more than 1000 receive addresses have been given out from the old HD seed,
and then the wallet is restored from an old backup, any keys after the first 1000
will not be in the keypool look-ahead.
Prior to this PR, if a new HD seed is generated, and subsequently funds are
sent to the first 1000 addresses given out from the old HD seed, the keypool
for the old HD seed would not top up, and any funds sent to keys after the
first 1000 would not be added to the wallet.
This PR keeps the HD seed in the wallet (as an inactivehdseed), and tops up
the keypool as necessary when new funds are received. New receive addresses
are derived from the new HD seed as before.
How does it help users to top up keypools for inactive HD seeds? What is not
good about the behavior before this PR?
The original motivation for the PR was to remove the restriction that
setting a new HD seed wasn’t allowed during Initial Block
Download.
What was the reason for this restriction?
Why was the sethdseed RPC added? Are there uses for having multiple HD
seeds in the same wallet? Does this change to sethdseed affect new
descriptor
wallets
as well as existing wallets?
The main data structures used in this PR are
CHDChain,
CKeyMetadata,
and
KeyOriginInfo.
What are the purposes of the different data structures?
After the wallet has seen an existing key being used, how does the wallet
decide whether to “top up” and generate new keys? What factors determine
whether the wallet does or does not top up? Are there differences here
between the new
TopUpInactiveHDChain
method and existing
TopUp
method?
When is
AddInactiveHDChain
called? Where do the inactive HD chain objects come from? Is
AddInactiveHDChain called everywhere it needs to be called?
How is the PR structured and divided up? Would it make sense to split up or combine commits?
An earlier
version of
this PR tried (the implementation was buggy) to store
m_inactive_hd_chains
map entries as
"inactivehdchain"
rows in the wallet database. Was it good to stop doing this? What are the advantages of
storing this information? What are the advantages of not storing it?
A previous
version of
this PR had a subtle bug on this
line.
What was the bug and what were the effects?
Do you think this PR has sufficient test coverage? Are there ways the test could be extended?
<ryanofsky> That's good, next question is the purpose of the PR clear? How does it help users to top up keypools for inactive HD seeds? What is not good about the behavior before this PR?
<andrewtoth> it's a bit tricky, but i think the bug is that if a highly used seed is replaced by sethdseed, but lots of new txs come into that seed, and then the entire wallet is restored from an old backup but using the new seed, then txs sent to new addresses in the old seed won't be added to the wallet
<ryanofsky> andrewtoth, yeah that kind of scenario (old backups) seems more likely to cause a bug, than someone intentionally changing hd seed on their wallet and expecting to receive old funds from new keys generated externally
<jnewbery> raj_149: yes, you're right that the keypool tops up when an address is given out. It also tops up if we receive a transaction output to one of the addresses in the keypool. See the MarkUnusedAddresses() function.
<vasild> So, if I create a wallet, generate 1100 addresses, send them to some people, receive funds on 500 of them, replace the seed with sethdseed, for sure the 500 already received will be accounted for, later the remaining 600 addresses also receive funds. Would none of those 600 be accounted or just the last 100 (over 1000)?
<jnewbery> vasild: in that scenario, the keypool would already have up to index 2100 (when you give out address 1100). I think a crucial part is that this happens only when you restore from an old backup
<ryanofsky> oh, misread, yeah, none of the 1100 would be lost, it's the keys that come after those 1100, which might exist because the wallet is old backup, or there's another active wallet out with the same seed
<ryanofsky> 3. The original motivation for the PR was to remove the restriction that setting a new HD seed wasn’t allowed during Initial Block Download. What was the reason for this restriction?
<ryanofsky> raj_149, right, it can happen outside of ibd, but it's more likely keypool will run out during ibd with all the historical blocks being processed
<jnewbery> 7. the first 1000 payments are received, but payment 1001 is not because we're not able to top-up the keypool for the old seed after setting a new seed.
<ryanofsky> 4. Why was the sethdseed RPC added? Are there uses for having multiple HD seeds in the same wallet? Does this change to sethdseed affect new descriptor wallets as well as existing wallets?
<troygiorshev> the fact that it can still happen after IDB is even more motivation for this PR. Part of the point of HD wallets was that you can have a "master" wallet that can spend funds and then put a receive-only wallet on a vulnerable machine. But if you change the hd seed on the master and the receive-only receives a lot of new transactions (and generates more than 1000 new addresses) then
<ryanofsky> troygiorshev, that makes sense in principle, but in practice we don't support a receive only wallet part that can top up, because we use hardened keys
<jkczyz> ryanofsky: yes, I believe so. My main qualm is the reason that "we no longer need to wait for IBD to finish before sethdseed can work" is not given
<ryanofsky> jkczyz, yeah it think that may be phrased as a binary statement when it was always a matter of precaution. but with this change there is no longer any benefit to disabling sethdseed during ibd
<raj_149> ryanofsky: that seems correct. Corollary question, what one needs to do to have a recieve only wallet in a merchant site and still get it tracked by core?
<jonatack> raj_149: i think it will finally become possible to export an xpub to, say, btcpayserver from a bitcoin core wallet, with descriptor wallets
<ryanofsky> just to wrap up question 4, I couldn't think of real uses cases for sethdseed anymore, and didn't check but suspect it was just added before there was multiwallet support
<jonatack> raj_149: for security reasons, bitcoin core legacy wallets don't support xpub key derivation because if an attacker knows the private key of any of the child keys and the xpub, he can compute the private key of all child keys
<raj_149> CHDChain: contans data to construct the full chain. CKeyMetadata: Contains metadata for a particular key KeyOriginInfo: contains origin and path to get to this key.
<ryanofsky> raj_149, that's interesting, yeah i guess it gets to a basic difference that TopUpInactive is told specifically what to top up, while TopUp just figures it out itself
<ryanofsky> 7. When is AddInactiveHDChain called? Where do the inactive HD chain objects come from? Is AddInactiveHDChain called everywhere it needs to be called?
<raj_149> ryanofsky: it adds a chain to m_inactive_hd_chains. But if sethdchain requires also m_inactive_hd_chains to be updated and its not right now. How the test is passing?
<ryanofsky> one question that might be interesting is 10: A previous version of this PR had a subtle bug on this line. What was the bug and what were the effects?
<ryanofsky> raj_149, i'd say the important thing happening is that m_inactive_hd_chains is populated on loading, by looping over all the keys and finding the max internal/external indices
<raj_149> ryanofsky: i need to understand descriptor wallet better. But overall the motivation made sense. The wallets need to handle old chains sensibly.
<raj_149> thanks everybody. It was a great session. For the first time it felt like i understood enough to have meaningful conversation. thats motivating. :)