Dependency on #15719
Actually we lock the chain every-time a resend or transaction is broadcast to evaluate if node is currently in-or-out of IBD. This PR proposes instead to rely on updatedBlockTip to refresh IBD status, therefore increasing asynchronicity between wallet-node and should be a slight performance improvement.
It let call
sethseedwhile being disconnected, which may be blameworthty but ascreatewalletlet you already generate a HD wallet without checking for IBD, we should go either way or the other, i.e can generate HD-seed without out-of-IBD or can’t generate HD-seed without out-of-IBD. Thoughts ?
The reason that you had to be out of IBD in order to set an HD seed is because by setting a new HD seed, no new keys would ever be generated from the old seed. So if there were a transaction that involved a key that hadn’t been generated yet and was in a block after where you were currently synced, setting a new HD seed would mean that you don’t detect that transaction. That would be bad.
With createwallet, the difference is that it’s a completely new wallet with no history at all so it is safe to make a new wallet when you are behind.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
So if there were a transaction that involved a key that hadn’t been generated yet and was in a block after where you were currently synced
You’re talking about a key derived from the old seed here ? I didn’t dig in code but if I understand when we set a new seed we discard checking keys for old one right ? Node is getting out of IBD at tip N - DEFAULT_TIP_AGE(~144), which means you can set a new seed at N - 144, but you’re not going to see a tx with an old key in block N - 100 for example, so is this mechanism reliable ?
You’re talking about a key derived from the old seed here ? I didn’t dig in code but if I understand when we set a new seed we discard checking keys for old one right ?
Yes, I’m talking about keys derived from the old seed. Any key that has already been derived and added to the wallet (i.e. it was in the keypool or already marked as used) will still be checked for. Setting a new seed doesn’t change that. Setting a new seed only makes it so that no new keys will be derived from the old seed.
Node is getting out of IBD at tip N - DEFAULT_TIP_AGE(~144), which means you can set a new seed at N - 144, but you’re not going to see a tx with an old key in block N - 100 for example, so is this mechanism reliable ?
I think it’s reasonably reliable as I think it is unlikely that someone used the entire keypool in 144 blocks.
1164@@ -1158,6 +1165,11 @@ class CWallet final : public WalletStorage, private interfaces::Chain::Notificat
1165 m_last_block_processed_height = block_height;
1166 m_last_block_processed = block_hash;
1167 };
1168+ /** Get if chain is initial block download */
698@@ -699,6 +699,13 @@ class CWallet final : public WalletStorage, private interfaces::Chain::Notificat
699 */
700 int m_last_block_processed_height GUARDED_BY(cs_wallet) = -1;
701
702+ /* Flag to indicate chain is still in initial block download. Set it to
703+ * false to let wallet generating new seed without being connected to a node.
704+ * Othersaid, a wallet evaluates chain being out of initial block download
705+ * until it's proven false.
Thanks for adding this documentation. Suggestion (if I understand correctly that othersaid means autrement dit, which translates to in other words):
0- /* Flag to indicate chain is still in initial block download. Set it to
1- * false to let wallet generating new seed without being connected to a node.
2- * Othersaid, a wallet evaluates chain being out of initial block download
3- * until it's proven false.
4+ /* Flag to indicate whether or not the chain is still in initial block
5+ * download. Defaults to false to allow the wallet to generate new seeds
6+ * without being connected to a node. In other words, the wallet assumes the
7+ * chain is out of initial block download unless shown otherwise.
3931@@ -3932,17 +3932,17 @@ UniValue sethdseed(const JSONRPCRequest& request)
3932
3933 LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(*pwallet);
3934
3935- if (pwallet->chain().isInitialBlockDownload()) {
Perhaps make this change in the last commit where the other removals of isInitialBlockDownload take place?
IIUC, the commit is misnamed and could better be called something like “Remove Chain::isInitialBlockDownload`
1950@@ -1948,6 +1951,8 @@ void CWallet::ResendWalletTransactions()
1951 auto locked_chain = chain().lock();
1952 LOCK(cs_wallet);
1953
1954+ if (m_is_ibd) return;
re: #17484 (review)
In commit “Remove Chain::isReadyToBroadcast method” (a5a674857d0ae6d535e4725337968492c821c635)
Perhaps make this change in the commit “Add m_is_ibd in CWallet` (or merge the two commits, IIUC)
Change does make sense (and is necessary) in this commit due to isReadyToBroadcast call a few lines up. But it would be good to add a comment here mentioning this is also needed to avoid spamming nodes with old transactions.
Concept ACK.
First review pass to get started before digging deeper into the approach. Built, ran tests and bitcoind.
Some implementation thoughts below; please ignore them until the PR has more Concept/Approach ACKs. Additional thoughts:
const
Thanks @achow101 for your insights, I’m going to hold-on this PR once the wallet init its state asynchronously like thanks to change here https://github.com/ariard/bitcoin/commits/2019-08-rescan-index-refactor so it would avoid behavior change on the point you raised.
Thanks for your review @jonatack, will fix comments once PR gets Concept ACK.
Concept ACK. @achow101 to be clear, is the failure scenario as follows:
sethdseed before the node has synced. The payments to the keys handed out after the initial keypool was drained are lost.(1) must be an HD wallet because restoring a non-HD wallet backup that has drained its initial keypool keys always results in funds loss (3) must drain the initial keypool because setting a new HD seed does not remove the initial keypool from the wallet. It simply marks them as not in the keypool
Not allowing sethdseed to be called before we’re out of IBD isn’t perfect protection against this. If many addresses (more than in the initial keypool) have been handed out, the user could still receive a payment to one of those addresses after they’ve changed their hdseed and lose the payment.
@ariard - is it possible to change this so m_is_ibd initializes to true and then call Chain::isInitialBlockDownload() once on wallet start-up?
For some context about the IBD and sethdseed stuff: #12560 (comment)
is the failure scenario as follows:
Yes.
@achow101 Thanks for the cross-reference :pray:
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
Keeping the old key for key derivation definitely seems like the correct fix. How complex is that (and does descriptor wallets do that automatically?)
Keeping the old key for key derivation definitely seems like the correct fix. How complex is that
I don’t think it would be too complex. Although I think it would be a fairly large change. We would need to keep track of the old hd seeds and keep track of where we’ve derived up to for a particular seed. And a bunch of things that do things on the seed would need to be changed to do things on any seed. We would have to add several more records to the wallet.
Alternatively we could have sethdseed make a new LegacyScriptPubKeyMan with the seed, but that may also require a bit of re-architecting as the primary assumption about LegacyScriptPubKeyMans will change. But we would have to wait for the full ScriptPubKeyMan and CWallet separation before that can happen.
and does descriptor wallets do that automatically?
Not the current implementation. But it also would be pretty trivial to do it.
is it possible to change this so m_is_ibd initializes to true and then call Chain::isInitialBlockDownload() once on wallet start-up?
Yes but that removes PR interest of drying up the Chain interface. I think best it to wait wallet registering its sync state with the Chain::interface (https://github.com/ariard/bitcoin/commits/2019-08-rescan-index-refactor) at start-up so after rescan done wallet knows it’s out of IBD.
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
I think by robustness we should go for the second option. Reading https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Class-Structure-Changes, best option is to have multiple Scri[tPubKeyMan or one with multiple hdChain ?
Reading https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Class-Structure-Changes, best option is to have multiple Scri[tPubKeyMan or one with multiple hdChain ?
I think you could have separate LegacyScriptPubKeyMan instances for each hdseed, but this would probably be more work than necessary, and not very related to what this PR is trying to do.
I think the minimal fix to prevent running out of keys for old hd seeds would be to do some extra checking and topping up inside LegacyScriptPubKeyMan::MarkUnusedAddresses. Add a check there to see if the affected key has metadata associated with an inactive hd seed. If it does and the key number in the metadata is close enough to the highest generated key on that hd path, generate more keys on the path and add them to the wallet. I don’t think this would require very much code. To work efficiently the LegacyScriptPubKeyMan instance would need to have a new map from KeyOriginInfo hd path to the maximum generated key number on that path.
Actually, I think you would need to do what I just described even if you did want to instantiate different LegacyScriptPubKeyMan instances for each seed, because inactive hd seeds aren’t going to have any pool data, so m_pool_key_to_index data will be empty and existing topup code won’t work for them. So you’d need this new LegacyScriptPubKeyMan::MarkUnusedAddresses code anyway.
If it is desirable to have separate LegacyScriptPubKeyMan instances in the wallet, I think that might not be too hard to implement. The trickiest part would just be loading them. In walletdb.cpp as keys and scripts are loaded you would have to examine their metadata to figure out what seed they were associated with and load them into the appropriate keyman instance based on that seed.
1104@@ -1105,6 +1105,9 @@ void CWallet::BlockDisconnected(const CBlock& block, int height)
1105 void CWallet::UpdatedBlockTip(bool is_ibd)
1106 {
1107 m_best_block_time = GetTime();
1108+ auto locked_chain = chain().lock();
In commit “Add m_is_ibd in CWallet” (dca4246dac0f4fa6369a71ee787b2bf1f6415c6d)
Calling chain lock not unnecessary here (but harmless)
190@@ -191,9 +191,6 @@ class Chain
191 //! Check if the node is ready to broadcast transactions.
192 virtual bool isReadyToBroadcast() = 0;
In commit “Remove Chain::isReadyToBroadcast method” (a5a674857d0ae6d535e4725337968492c821c635)
Implementation and meaning of isReadyToBroadcast method is changing a lot, so I would rename it to something like “isInitializing” or “isLoading” and update the comment.
Also commit title could be changed to mention removing isInitialBlockDownload
Code review ACK a5a674857d0ae6d535e4725337968492c821c635 with caveat that I only did cursory check to verify that CMainSignals::UpdatedBlockTip fInitialDownload value is the same value given by ChainstateActive().IsInitialBlockDownload()
Would be great to see this PR and #17443 rebased and merged as followup to #16426!
3942
3943 auto locked_chain = pwallet->chain().lock();
3944 LOCK(pwallet->cs_wallet);
3945
3946+ if (pwallet->IsIBD()) {
3947+ throw JSONRPCError(RPC_CLIENT_IN_INITIAL_DOWNLOAD, "Cannot set a new HD seed while still in Initial Block Download");
In commit “Add m_is_ibd in CWallet” (dca4246dac0f4fa6369a71ee787b2bf1f6415c6d)
Is the commit message correct? It says there is a behavior change here, but there doesn’t seem to be one if this is still raising an error.
This is only used in next commit.
Rebased the PR but this is not worthy to review now, after thinking a bit more I want to avoid behavior changes and for so, it would be better to get #15719 and its follow-up first to always ensure we are in-sync with node wrt to IBD after wallet startup.
This current PR may provoke issue if a privacy leak if user broadcast a transaction without being at tip.
Updated PR description in consequence.
2015@@ -2014,6 +2016,8 @@ void CWallet::ResendWalletTransactions()
2016 { // cs_wallet scope
2017 LOCK(cs_wallet);
2018
2019+ if (m_is_ibd) return;
In commit “[wallet] Add m_is_ibd in CWallet” (797ec9dc2b4c1b4fbf2ac3377a3c47f406383248)
I do think it would be helpful to have comment here saying this is skipped during ibd to avoid spam.
Also the comment above for isReadyToBroadcast() is incorrect after commit “[interfaces] Remove Chain::isReadyToBroadcast” (20eb0dacdd6d698b02d5b151dd2ca7f9bfba4f8c), and should be updated to not mention IBD since the IBD check isn’t there anymore
re: #17484 (comment) Seems reasonable to delay this PR until wallet startup works better. In case it’s not clear to others, the change in behavior here is that with this PR the wallet may now incorrectly assume the node is in IBD when it’s loaded (before it gets an updatedBlockTip notification indicating otherwise). This can cause it to choose transaction lock times differently and be a privacy leak.
But code review ACK 20eb0dacdd6d698b02d5b151dd2ca7f9bfba4f8c aside from this concern. Changes since last review: rebase, adding m_is_ibd comment, moving resend ibd check to earlier commit.
🐙 This pull request conflicts with the target branch and needs rebase.
Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.
