Add HD wallet auto-restore functionality

jonasschnelli commented at 3:30 pm on April 20, 2017: contributor

This PR ensures that we always check the keypool during SyncTransaction and, that the lookup window for HD restore is large enough (currently +20 keys). Once the keypool size falls below the min gap limit (currently 20 keys) and the keypool can’t be extended (locked wallet), the wallet then temporary pauses synchronisation.

unencrypted wallets

For unencrypted wallets, the key pool will always be topped up. During sync (SyncTransaction), we check for used keypool keys and mark all keys up to the matched key as used. Additionally we topup the keypool to ensure we not fall below the gap limit (20 keys).

encrypted wallets in non pruning mode

Same as above, but, If we hit the gap limit with an encrypted wallet, we can’t topup the keypool. In that case, we just pause the sync (not the node, only the wallet). Once the user unlocks the wallet over walletpassphrase, we rescan down to the point where we have stopped previously to ensure we don’t miss possible funds.

encrypted wallets in pruned mode

Same as above, but, we also stop requesting and connecting blocks. The full node will “pause” in this case until the user did unlock his wallet (== topup, rescan, etc.).

This has a little bit of complexity and requires careful reviews.

jonasschnelli force-pushed on Apr 20, 2017

jonasschnelli added the label Wallet on Apr 20, 2017

ryanofsky commented at 6:46 pm on May 1, 2017: member

I know there was discussion about this in IRC last week, but I didn’t follow closely. Would it be better to review this PR now, or hold off for more changes?

jonasschnelli commented at 1:08 pm on May 2, 2017: contributor

I’m currently implementing the changes we discussed in last weeks RIC meeting. A review makes more sense after the overhaul.

jonasschnelli force-pushed on May 3, 2017

jonasschnelli commented at 3:37 pm on May 3, 2017: contributor

Completely rewrote this PR. Keeping the WIP tag for now. The current implementation takes care of encrypted and unencrypted wallets (also in conjunction with pruning).

unencrypted wallets

For unencrypted wallets, the key pool will always be topped up. During sync (SyncTransaction), we check for used keypool keys and mark all keys up to the matched key as used. Additionally we topup the keypool to ensure we not fall below the gap limit (20 keys).

encrypted wallets in non pruning mode

Same as above, but, If we hit the gap limit with an encrypted wallet, we can’t topup the keypool. In that case, we just pause the sync (not the node, only the wallet). Once the user unlocks the wallet over walletpassphrase, we rescan down to the point where we have stopped previously to ensure we don’t miss possible funds.

encrypted wallets in pruned mode

Same as above, but, we also stop requesting and connecting blocks. The full node will “pause” in this case until the user did unlock his wallet (== topup, rescan, etc.).

This has a little bit of complexity and requires careful reviews. Thanks for early feedback.

jonasschnelli force-pushed on May 4, 2017

jonasschnelli renamed this:
~~[WIP] Add basic HD wallet restore functionality~~
Add basic HD wallet restore functionality
on May 4, 2017

jonasschnelli commented at 3:48 pm on May 4, 2017: contributor

All tests are passing now. Removed WIP tag. Overhauled PR description

in src/wallet/wallet.cpp:1009 in cada7263fa outdated

1003@@ -970,6 +1004,22 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const CBlockI
1004         if (fExisted && !fUpdate) return false;
1005         if (fExisted || IsMine(tx) || IsFromMe(tx))
1006         {
1007+            // check if we need to flag used keypool keys
1008+            std::set<CKeyID> setKeyPool;
1009+            GetAllReserveKeys(setKeyPool);

jonasschnelli commented at 3:53 pm on May 4, 2017:

I guess this is very inefficient and something like #10235 should allow to speed up things.

jonasschnelli renamed this:
~~Add basic HD wallet restore functionality~~
Add HD wallet auto-restore functionality
on May 4, 2017

in src/wallet/wallet.cpp:1008 in 11cccadf13 outdated

1003@@ -970,6 +1004,22 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const CBlockI
1004         if (fExisted && !fUpdate) return false;
1005         if (fExisted || IsMine(tx) || IsFromMe(tx))
1006         {
1007+            // check if we need to flag used keypool keys
1008+            std::set<CKeyID> setKeyPool;

ryanofsky commented at 4:03 pm on May 5, 2017:

In commit “Add basic HD wallet restore functionality”

Maybe rename this variable. (It shadows CWallet::setKeyPool, which makes that variable more annoying to grep for.)

ryanofsky commented at 7:24 pm on June 8, 2017:

Maybe rename this variable. (It shadows CWallet::setKeyPool, which makes that variable more annoying to grep for.)

Done in 2a203b73fd2d7b016cd5f55977135a15a982414d

in src/wallet/wallet.cpp:1011 in 11cccadf13 outdated

1003@@ -970,6 +1004,22 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const CBlockI
1004         if (fExisted && !fUpdate) return false;
1005         if (fExisted || IsMine(tx) || IsFromMe(tx))
1006         {
1007+            // check if we need to flag used keypool keys
1008+            std::set<CKeyID> setKeyPool;
1009+            GetAllReserveKeys(setKeyPool);
1010+            // loop though all outputs
1011+            BOOST_FOREACH(const CTxOut& txout, tx.vout) {

ryanofsky commented at 4:32 pm on May 5, 2017:

In commit “Add basic HD wallet restore functionality”

Maybe avoid BOOST_FOREACH here and below.

ryanofsky commented at 7:25 pm on June 8, 2017:

Maybe avoid BOOST_FOREACH here and below.

Done in 2a203b73fd2d7b016cd5f55977135a15a982414d

in src/wallet/wallet.cpp:3469 in b5f9bc2c11 outdated

3376+    for (const int64_t& id : setKeyPool) {
3377+        CKeyPool keypool;
3378+        if (!walletdb.ReadPool(id, keypool))
3379+            throw std::runtime_error(std::string(__func__) + ": read failed");
3380+
3381+        if (keypool.vchPubKey.GetID() == keyId) {

ryanofsky commented at 4:59 pm on May 5, 2017:

In commit “Add basic HD wallet restore functionality”

It seems like this for-loop could be eliminated if you just changed GetAllReserveKeys to return map<CKeyID, int64_t> instead of set<CKeyID>. I think making this change would add less code than is written here.

jonasschnelli commented at 8:15 am on May 12, 2017:

It would also require to keep the fInternal flags in memory (for the address book internal/external change flagging). This is independently addresses in #10238.

ryanofsky commented at 3:23 pm on June 8, 2017:

It would also require to keep the fInternal flags in memory (for the address book internal/external change flagging). This is independently addresses in #10238.

Yeah probably not worth simplifying this if it will be obsoleted by #10238.

in src/wallet/wallet.cpp:3460 in b5f9bc2c11 outdated

3364@@ -3349,14 +3365,62 @@ void CReserveKey::ReturnKey()
3365     vchPubKey = CPubKey();
3366 }
3367 
3368+void CWallet::MarkReserveKeysAsUsed(const CKeyID& keyId)
3369+{
3370+    LOCK(cs_wallet);
3371+    CWalletDB walletdb(*dbw);
3372+    auto it = std::begin(setKeyPool);

ryanofsky commented at 5:01 pm on May 5, 2017:

In commit “Add basic HD wallet restore functionality”

Should move this declaration closer to where it’s actually used way below.

ryanofsky commented at 3:23 pm on June 8, 2017:

Should move this declaration closer to where it’s actually used way below.

(unchanged)

in src/wallet/wallet.cpp:3489 in b5f9bc2c11 outdated

3396+                CKeyPool keypool;
3397+                if (!walletdb.ReadPool(id, keypool))
3398+                    throw std::runtime_error(std::string(__func__) + ": read failed");
3399+
3400+                // only mark keys on the corresponding chain
3401+                if (keypool.fInternal == foundInternal) {

ryanofsky commented at 5:05 pm on May 5, 2017:

In commit “Add basic HD wallet restore functionality”

Shouldn’t it be an error if this condition is not true?

ryanofsky commented at 3:23 pm on June 8, 2017:

Shouldn’t it be an error if this condition is not true?

(unchanged)

in src/wallet/wallet.cpp:3483 in b5f9bc2c11 outdated

3390+
3391+    // mark all keys up to the found key as used
3392+    if (foundIndex >= 0) {
3393+        while (it != std::end(setKeyPool)) {
3394+            const int64_t& id = *(it);
3395+            if (id <= foundIndex) {

ryanofsky commented at 5:09 pm on May 5, 2017:

In commit “Add basic HD wallet restore functionality”

As an simplification (and optimization), you can exit the loop if id > foundIndex, since setKeyPool is an ordered set.

ryanofsky commented at 3:23 pm on June 8, 2017:

As an simplification (and optimization), you can exit the loop if id > foundIndex, since setKeyPool is an ordered set.

(unchanged)

in src/wallet/wallet.cpp:3795 in b5f9bc2c11 outdated

3788@@ -3725,6 +3789,19 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
3789 
3790     RegisterValidationInterface(walletInstance);
3791 
3792+    // HD Restore: Make sure we always have a reasonable keypool size if HD is enabled
3793+    if (walletInstance->IsHDEnabled()) {
3794+        if (walletInstance->IsCrypted()) {
3795+            InitWarning(_("Your are using an encrypted HD wallet. In case you recover a HD wallet, you may miss incomming or outgoing funds."));

ryanofsky commented at 5:32 pm on May 5, 2017:

In commit “Add basic HD wallet restore functionality”

Phrase “In case you recover a HD wallet” seems too vague to be meaningful. I might say something more detailed like, “In the case where you just restored an old wallet.dat backup that was not topped-up with the most recently derived HD keys, transactions using those keys will not show up in the wallet, and funds that are available to spend may not appear. You can avoid this problem by either manually topping up your wallet keypool, or (less reliably) just leaving your wallet decrypted while it scans for relevant transactions.”

ryanofsky commented at 3:24 pm on June 8, 2017:

Phrase “In case you recover a HD wallet” seems too vague to be meaningful.

(unchanged)

in src/wallet/wallet.cpp:3799 in b5f9bc2c11 outdated

3794+        if (walletInstance->IsCrypted()) {
3795+            InitWarning(_("Your are using an encrypted HD wallet. In case you recover a HD wallet, you may miss incomming or outgoing funds."));
3796+        }
3797+        else {
3798+            if (GetArg("-keypool", DEFAULT_KEYPOOL_SIZE) < HD_RESTORE_KEYPOOL_SIZE_MIN ) {
3799+                InitWarning(_("Your keypool size is below the recommended limit for HD rescans. In case you recover a HD wallet, you may miss incomming or outgoing funds."));

ryanofsky commented at 5:46 pm on May 5, 2017:

In commit “Add basic HD wallet restore functionality”

Since this is warning that the requested size is below the recommended limit, it would be good to mention what the recommended limit actually is. And again “recover a HD wallet” seems vague. I’d maybe write this as, “Your keypool configured to store less than 20 keys. The recommended size is -keypool=100. Using a larger keypool will make it less likely that your wallet will be missing transactions and funds if it is restored from an old backup.” (Substituting HD_RESTORE_KEYPOOL_SIZE_MIN and DEFAULT_KEYPOOL_SIZE for the 20 and 100 values.)

ryanofsky commented at 3:25 pm on June 8, 2017:

Since this is warning that the requested size is below the recommended limit, it would be good to mention what the recommended limit actually is.

(unchanged)

in src/wallet/wallet.cpp:1005 in b5f9bc2c11 outdated

1001@@ -1002,6 +1002,22 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const CBlockI
1002         if (fExisted && !fUpdate) return false;
1003         if (fExisted || IsMine(tx) || IsFromMe(tx))
1004         {
1005+            // check if we need to flag used keypool keys

ryanofsky commented at 6:03 pm on May 5, 2017:

In commit “Add basic HD wallet restore functionality”

Would expand this comment. Maybe “Check if any keys in the wallet keypool that were supposed to be unused have appeared in a new transaction. If so, remove those keys from the keypool. This can happen when restoring an old wallet backup that does not contain the mostly recently created transactions from newer versions of the wallet.”

ryanofsky commented at 7:22 pm on June 8, 2017:

Would expand this comment

Done in 2a203b73fd2d7b016cd5f55977135a15a982414d

in src/wallet/wallet.cpp:1177 in 5e672d36d9 outdated

1180@@ -1182,6 +1181,7 @@ void CWallet::BlockConnected(const std::shared_ptr<const CBlock>& pblock, const
1181     }
1182     for (size_t i = 0; i < pblock->vtx.size(); i++) {
1183         SyncTransaction(pblock->vtx[i], pindex, i);
1184+void CWallet::BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex *pindex, const std::vector<CTransactionRef>& vtxConflicted, bool &requestPause) {

ryanofsky commented at 6:28 pm on May 5, 2017:

In commit “Add request-halt flag to BlockConnected signal”.

Compile error here (line needs to be moved up)

ryanofsky commented at 3:26 pm on June 8, 2017:

Compile error here (line needs to be moved up)

Error is still present (commit “Add request-halt flag to BlockConnected signal)

in src/wallet/wallet.cpp:1170 in b9a95b08e5 outdated

1178-    // to abandon a transaction and then have it inadvertently cleared by
1179-    // the notification that the conflicted transaction was evicted.
1180-
1181-    for (const CTransactionRef& ptx : vtxConflicted) {
1182+    {
1183+        LOCK2(cs_main, cs_wallet);

ryanofsky commented at 6:33 pm on May 5, 2017:

In commit “Wallet: allow to eventually pause verification if keypool requires”

No need to add the {} and extra indentation here in TransactionAddedToMempool. (It throws off the github diff and makes the other changes harder to follow).

ryanofsky commented at 7:25 pm on June 8, 2017:

No need to add the {} and extra indentation

(unchanged)

in src/wallet/wallet.cpp:1207 in b9a95b08e5 outdated

1220+    if (fSyncPausedUntilKeypoolExt) return;
1221 
1222-    for (const CTransactionRef& ptx : pblock->vtx) {
1223-        SyncTransaction(ptx);
1224+    {
1225+        LOCK2(cs_main, cs_wallet);

ryanofsky commented at 6:35 pm on May 5, 2017:

In commit “Wallet: allow to eventually pause verification if keypool requires”

Again unnecessary {} and indentation make the diff messy here in BlockDisconnected

ryanofsky commented at 7:25 pm on June 8, 2017:

Again unnecessary {} and indentation make the diff messy here in BlockDisconnected

(unchanged)

in src/wallet/wallet.h:1112 in b9a95b08e5 outdated

1107@@ -1106,6 +1108,9 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
1108        caller must ensure the current wallet version is correct before calling
1109        this function). */
1110     bool SetHDMasterKey(const CPubKey& key);
1111+
1112+    bool IsSyncPausedUntilKeypoolExt() { return fSyncPausedUntilKeypoolExt; }

ryanofsky commented at 6:39 pm on May 5, 2017:

In commit “Wallet: allow to eventually pause verification if keypool requires”

These functions are never called anywhere, and appear to be deleted in the next commit. Would remove them from this commit.

ryanofsky commented at 7:26 pm on June 8, 2017:

These functions are never called anywhere, and appear to be deleted in the next commit. Would remove them from this commit.

Removed in 5b70a2289e96f0ce8c49678e613845572d8203bc

in src/wallet/wallet.cpp:3432 in b9a95b08e5 outdated

3427@@ -3409,6 +3428,7 @@ void CWallet::MarkReserveKeysAsUsed(const CKeyID& keyId)
3428     }
3429 
3430     if (IsHDEnabled() && !TopUpKeyPool()) {
3431+        fSyncPausedUntilKeypoolExt = true;
3432         LogPrintf("%s: Topping up keypool failed (locked wallet)\n", __func__);

ryanofsky commented at 6:41 pm on May 5, 2017:

In commit “Wallet: allow to eventually pause verification if keypool requires”

Would update the printf to say this is pausing downloads / transaction processing.

ryanofsky commented at 3:24 pm on June 8, 2017:

Fixed in 5b70a2289e96f0ce8c49678e613845572d8203bc

in src/wallet/wallet.h:664 in b9a95b08e5 outdated

651@@ -652,6 +652,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
652     static std::atomic<bool> fFlushScheduled;
653     std::atomic<bool> fAbortRescan;
654     std::atomic<bool> fScanningWallet;
655+    std::atomic<bool> fSyncPausedUntilKeypoolExt;

ryanofsky commented at 6:42 pm on May 5, 2017:

In commit “Wallet: allow to eventually pause verification if keypool requires”

This variable should be better documented. I think you could basically take the text from your PR description and paste it here.

ryanofsky commented at 3:26 pm on June 8, 2017:

Added in 5b70a2289e96f0ce8c49678e613845572d8203bc.

in src/wallet/wallet.cpp:1200 in b9a95b08e5 outdated

1208+        }
1209+        for (size_t i = 0; i < pblock->vtx.size(); i++) {
1210+            SyncTransaction(pblock->vtx[i], pindex, i);
1211+        }
1212+        if (fSyncPausedUntilKeypoolExt) {
1213+            requestPause = true;

ryanofsky commented at 6:50 pm on May 5, 2017:

In commit “Add request-halt flag to BlockConnected signal”.

What’s the advantage of requesting a pause in this delayed and indirect way via BlockConnected instead of immediately setting setBlockRequestsPaused(true); setTipUpdatesPaused(true); when the keypool fails to top up in MarkReserveKeysAsUsed()? The indirect approach seems to add a lot of complexity.

jonasschnelli commented at 7:56 am on May 12, 2017:

As you mention, it would be much simpler to call setBlockRequestsPaused(), but, wouldn’t it be a massive layer violation? Communicating through the signals doesn’t require that the wallet needs to know what the full node parts is really doing. But maybe im overcomplicating things here.

ryanofsky commented at 3:22 pm on June 8, 2017:

As you mention, it would be much simpler to call setBlockRequestsPaused(), but, wouldn’t it be a massive layer violation? Communicating through the signals doesn’t require that the wallet needs to know what the full node parts is really doing. But maybe im overcomplicating things here.

I don’t see why it would be a problem to call setBlockRequestsPaused(true) from CWallet::MarkReserveKeysAsUsed, especially if it isn’t a problem to call setBlockRequestsPaused(false) from CWallet::EventuallyRescanAfterKeypoolTopUp which the current code already does. It seems like it would just make pausing simpler, and more consistent with unpausing.

in src/validation.cpp:2536 in 8ac56f8ebd outdated

2532@@ -2533,6 +2533,14 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams,
2533             for (const PerBlockConnectTrace& trace : connectTrace.GetBlocksConnected()) {
2534                 assert(trace.pblock && trace.pindex);
2535                 GetMainSignals().BlockConnected(trace.pblock, trace.pindex, *trace.conflictedTxs, requestPause);
2536+                if (requestPause) {

ryanofsky commented at 6:54 pm on May 5, 2017:

In commit “Ensure we pause wallet sync when the keypool requires extension”

This diff logically seems like it should be part of the “Add request-halt flag to BlockConnected signal” commit. Moving it there would make the commit history easier to understand.

ryanofsky commented at 3:27 pm on June 8, 2017:

This diff logically seems like it should be part of the “Add request-halt flag to BlockConnected signal” commit. Moving it there would make the commit history easier to understand.

(unchanged)

in src/validation.cpp:4327 in 8ac56f8ebd outdated

4323@@ -4316,6 +4324,7 @@ bool isBlockRequestsPaused() {
4324 }
4325 
4326 void setBlockRequestsPaused(bool state) {
4327+    LogPrintf("%s Block Requests\n", (state ? "Pause" : "Resume"));

ryanofsky commented at 6:55 pm on May 5, 2017:

In commit “Ensure we pause wallet sync when the keypool requires extension”

Consider moving this change to the “Add option to pause block requests” commit.

ryanofsky commented at 3:27 pm on June 8, 2017:

Consider moving this change to the “Add option to pause block requests” commit.

(unchanged)

in src/validation.cpp:4336 in 8ac56f8ebd outdated

4332@@ -4324,6 +4333,7 @@ bool isTipUpdatesPaused() {
4333 }
4334 
4335 void setTipUpdatesPaused(bool state) {
4336+    LogPrintf("%s Tip Updates\n", (state ? "Pause" : "Resume"));

ryanofsky commented at 6:55 pm on May 5, 2017:

In commit “Ensure we pause wallet sync when the keypool requires extension”

Consider moving this change to the “Add option to pause tip updates” commit.

ryanofsky commented at 3:27 pm on June 8, 2017:

Consider moving this change to the “Add option to pause tip updates” commit.

(unchanged)

in src/wallet/rpcwallet.cpp:2051 in 8ac56f8ebd outdated

2043@@ -2044,6 +2044,9 @@ UniValue walletpassphrase(const JSONRPCRequest& request)
2044 
2045     pwallet->TopUpKeyPool();
2046 
2047+    // give a hint to the wallet in case we have paused sync (we may have fall bellow the hd gap limit)
2048+    pwallet->EventuallyRescanAfterKeypoolTopUp();

ryanofsky commented at 7:01 pm on May 5, 2017:

In commit “Ensure we pause wallet sync when the keypool requires extension”

Would it make sense to call this automatically inside TopUpKeyPool? Seems strange to have to implement it separately for RPC and Qt wallet interfaces.

ryanofsky commented at 3:27 pm on June 8, 2017:

Would it make sense to call this automatically inside TopUpKeyPool? Seems strange to have to implement it separately for RPC and Qt wallet interfaces

(unchanged)

in src/wallet/wallet.cpp:1458 in 8ac56f8ebd outdated

1455+                    block = block->pprev;
1456+
1457+                if (pindexRescan != block) {
1458+                    const static std::string pruneError = "Prune: last wallet synchronisation goes beyond pruned data. You need to -reindex (download the whole blockchain again in case of pruned node)";
1459+                    uiInterface.ThreadSafeMessageBox(pruneError, "", CClientUIInterface::MSG_ERROR);
1460+                    StartShutdown();

ryanofsky commented at 7:11 pm on May 5, 2017:

In commit “Ensure we pause wallet sync when the keypool requires extension”

It seems kind of abrupt and unfriendly that entering your passphrase to unlock the wallet could suddenly trigger a shutdown. Wouldn’t it be better to detect this condition before unpausing, and if it happens, let the node stay up and paused instead of shutting it down?

jonasschnelli commented at 8:20 am on May 12, 2017:

I guess it already tries to avoid this situation by disabling the block downloads if the wallet is encrypted and the node runes in prune-mode. But there is no guarantee that we won’t prune bellow the wallets best block,.. this why this shutdown/protection is here.

ryanofsky commented at 3:23 pm on June 8, 2017:

But there is no guarantee that we won’t prune bellow the wallets best block,.. this why this shutdown/protection is here.

Wouldn’t just staying paused in this case offer as much protection as briefly unpausing and then shutting down?

in test/functional/test_runner.py:57 in ec5bf97733 outdated

53@@ -54,6 +54,7 @@
54     # Longest test should go first, to favor running tests in parallel
55     'wallet-hd.py',
56     'wallet-hd-restore.py',
57+    'wallet-hd-restore.py',

ryanofsky commented at 7:14 pm on May 5, 2017:

In commit “Extend wallet hd restore functional test”

Already listed

ryanofsky commented at 3:27 pm on June 8, 2017:

Already listed

(unchanged)

in src/wallet/wallet.cpp:3894 in f6f09a8eb2 outdated

3893-            }
3894             walletInstance->TopUpKeyPool();
3895         }
3896-
3897+        if (!walletInstance->CheckKeypoolMinSize()) {
3898+            InitWarning(_("Your keypool size is below the required limit for HD rescans. Wallet synchronisation is now paused until you have refilled the keypool."));

ryanofsky commented at 7:28 pm on May 5, 2017:

In commit “Make sure we check the keypool min size during startup”

Warning should suggest how to fix the problem, and also ideally also include more background information (maybe consider using my previous suggestion for the -keypool limit warning).

ryanofsky commented at 3:28 pm on June 8, 2017:

Warning should suggest how to fix the problem, and also ideally also include more background information (maybe consider using my previous suggestion for the -keypool limit warning).

(unchanged)

in src/wallet/wallet.cpp:3449 in f6f09a8eb2 outdated

3442@@ -3442,6 +3443,18 @@ void CReserveKey::ReturnKey()
3443     vchPubKey = CPubKey();
3444 }
3445 
3446+bool CWallet::CheckKeypoolMinSize() {
3447+    LOCK(cs_wallet);
3448+    size_t extKeypoolSize = KeypoolCountExternalKeys();
3449+    if (IsHDEnabled() && (extKeypoolSize < HD_RESTORE_KEYPOOL_SIZE_MIN || (setKeyPool.size()-extKeypoolSize) < HD_RESTORE_KEYPOOL_SIZE_MIN)) {
3450+        // if the remaining keypool size is below the gap limit, refuse to continue with the sync
3451+        fSyncPausedUntilKeypoolExt = true;

ryanofsky commented at 7:41 pm on May 5, 2017:

In commit “Make sure we check the keypool min size during startup”

This pausing behavior seems like it should be optional, because even in the worst case, you should be able to recover from any problem by topping up your keypool and doing a rescan. Maybe HD_RESTORE_KEYPOOL_SIZE_MIN should be renamed something like PAUSE_SYNC_WHEN_KEYPOOL_SMALLER_THAN and be a config option.

ryanofsky commented at 3:28 pm on June 8, 2017:

This pausing behavior seems like it should be optional

The “-hdignoregaplimit” argument added in the next commit does make this optional (commit “SetSoftArg the keypool always to the minimum gap limit”), though I still think a single “pause sync when keypool smaller than” check would be a little cleaner than separate “keypool size min” and “ignore gap limit” checks.

in src/validation.cpp:2557 in 11cccadf13 outdated

2553@@ -2554,7 +2554,7 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams,
2554         if (pindexFork != pindexNewTip) {
2555             uiInterface.NotifyBlockTip(fInitialDownload, pindexNewTip);
2556         }
2557-    } while (pindexNewTip != pindexMostWork);
2558+    } while (pindexNewTip != pindexMostWork && !isTipUpdatesPaused());

ryanofsky commented at 7:48 pm on May 5, 2017:

In commit “Make sure we pause immediately if we receive a pause-request”

Would squash this commit into the commit “Add request-halt flag to BlockConnected signal” (along with the other change I suggest moving there) so this is understandable in the correct context.

ryanofsky commented at 3:22 pm on June 8, 2017:

Would squash this commit into the commit “Add request-halt flag to BlockConnected signal” (along with the other change I suggest moving there) so this is understandable in the correct context.

(unchanged)

in src/wallet/wallet.cpp:3765 in cada7263fa outdated

3763@@ -3764,6 +3764,7 @@ std::string CWallet::GetWalletHelpString(bool showDebug)
3764         strUsage += HelpMessageOpt("-flushwallet", strprintf("Run a thread to flush wallet periodically (default: %u)", DEFAULT_FLUSHWALLET));
3765         strUsage += HelpMessageOpt("-privdb", strprintf("Sets the DB_PRIVATE flag in the wallet db environment (default: %u)", DEFAULT_WALLET_PRIVDB));
3766         strUsage += HelpMessageOpt("-walletrejectlongchains", strprintf(_("Wallet will not create transactions that violate mempool chain limits (default: %u)"), DEFAULT_WALLET_REJECT_LONG_CHAINS));
3767+        strUsage += HelpMessageOpt("-hdignoregaplimit", strprintf(_("Ignores the minimum keypool-size warning for HD restore (default: %u)"), DEFAULT_IGNORE_HD_MIN_KEYPOOL_SIZE));

ryanofsky commented at 7:58 pm on May 5, 2017:

In commit “SetSoftArg the keypool always to the minimum gap limit if HD is enabled”

It seems like it would be a lot harder to for someone to understand what this “ignore gap limit” option does than the PAUSE_SYNC_WHEN_KEYPOOL_SMALLER_THAN option I suggested in another comment. Would be curious to know what you think about it.

jonasschnelli commented at 8:10 am on May 12, 2017:

The -hdignoregaplimit is for debug purposes only. Our tests run with a keypool size of 1 and therefor would cause troubles running post this PR without a “ignore” option, and I don’t think it matter how we “ignore” the gap limit and over a startup argument seems the be the simplest way.

ryanofsky commented at 3:23 pm on June 8, 2017:

The -hdignoregaplimit is for debug purposes only

Maybe it’s not worth fixing, but even as a developer I think “pause sync when keypool is smaller than” is clearer than “ignore hd gap limit” because one name tells you literally what the option does, while the other refers obliquely to what the option doesn’t do.

jnewbery commented at 10:43 pm on June 12, 2017:

I think “pause sync when keypool is smaller than” is clearer than “ignore hd gap limit”

:+1:

nit: place this argument in the correct alphabetic order.

in src/wallet/wallet.cpp:3893 in cada7263fa outdated

3893         }
3894         else {
3895             walletInstance->TopUpKeyPool();
3896         }
3897-        if (!walletInstance->CheckKeypoolMinSize()) {
3898+        if (!walletInstance->CheckKeypoolMinSize() && !GetBoolArg("-hdignoregaplimit", DEFAULT_IGNORE_HD_MIN_KEYPOOL_SIZE)) {

ryanofsky commented at 7:59 pm on May 5, 2017:

In commit “SetSoftArg the keypool always to the minimum gap limit if HD is enabled”

This seems broken. It’s still pausing but just no longer warning that the pause took place?

ryanofsky commented at 3:29 pm on June 8, 2017:

This seems broken. It’s still pausing but just no longer warning that the pause took place?

On second look, this seems ok because CWallet::CheckKeypoolMinSize now checks for “-hdignoregaplimit” and disables the pause.

in test/functional/wallet-hd.py:78 in cada7263fa outdated

74@@ -75,10 +75,12 @@ def run_test (self):
75 
76         self.log.info("Restore backup ...")
77         self.stop_node(1)
78-        os.remove(self.options.tmpdir + "/node1/regtest/wallet.dat")
79+        # we need to delete the complete regtest directory
80+        # otherwise node1 would auto-recover all funds in flag the keypool keys as used

ryanofsky commented at 8:02 pm on May 5, 2017:

In commit “SetSoftArg the keypool always to the minimum gap limit if HD is enabled”

Is this change meant to be part of this commit? I don’t think there’s an interaction with hdignoregaplimit here.

ryanofsky commented at 3:23 pm on June 8, 2017:

Is this change meant to be part of this commit? I don’t think there’s an interaction with hdignoregaplimit here.

(unchanged)

jnewbery commented at 11:05 pm on June 12, 2017:

I’m also confused about why wallet-hd.py needs to be updated.

in src/wallet/wallet.cpp:1426 in 8ac56f8ebd outdated

1417@@ -1417,6 +1418,57 @@ bool CWallet::IsHDEnabled() const
1418     return !hdChain.masterKeyID.IsNull();
1419 }
1420 
1421+void CWallet::EventuallyRescanAfterKeypoolTopUp() {
1422+    if (fSyncPausedUntilKeypoolExt) {
1423+
1424+        // for now, enable block requests and tip updates via the states
1425+        // this will only be sufficient as long as only a single wallet adjusts these stats
1426+        // otherwise the net-/validation-logic needs to poll the state over a signal (or similar)

ryanofsky commented at 8:08 pm on May 5, 2017:

In commit “Ensure we pause wallet sync when the keypool requires extension”

It should be possible to handle this without any polling or notifications by making fPauseBlockRequests and fPauseTipUpdates counters instead of bools. The counters would need to be incremented whenever fSyncPausedUntilKeypoolExt changed from false to true in any wallet, and decremented when it changed from true to false. Block requesting or tip updating would be paused whenever its respective counter was greater than 0.

jonasschnelli commented at 8:12 am on May 12, 2017:

Yes. Thats a good idea. Do you think this should be done in this PR or can we tackle this once we have multiple places where the sync may get paused? Counters may also introduce the risk of unwillingly increasing it twice (while only decrease it once).

ryanofsky commented at 7:23 pm on June 8, 2017:

Do you think this should be done in this PR or can we tackle this once we have multiple places where the sync may get paused?

Maybe, this PR is already pretty complicated, so I wouldn’t want to tack this on in extra commits, but it could be nice if implemented to replace the existing “Add option to pause…” commits.

laanwj added this to the milestone 0.15.0 on May 11, 2017

[MOVEONLY] move CAffectedKeysVisitor 0024b9c7ed

Add basic HD wallet restore functionality 2a203b73fd

Add option to pause block requests (is/setBlockRequestsPaused()) 68d600b8da

Add option to pause tip updates (is/setTipUpdatesPaused()) 1a0272a3af

Add request-halt flag to BlockConnected signal 5a10cd334a

Wallet: allow to eventually pause verification if keypool requires more key to respect the gap limit 5b70a2289e

Ensure we pause wallet sync when the keypool requires extension 2a8f1f6856

Extend wallet hd restore functional test 09de1cbe5c

Make sure we check the keypool min size during startup and when retriving a key 46060c261c

SetSoftArg the keypool always to the minimum gap limit if HD is enabled bd9e1e8f89

Make sure we pause immediately if we receive a pause-request over the BlockConnected signal 4cbc21689c

jonasschnelli force-pushed on May 12, 2017

jonasschnelli commented at 8:29 am on May 12, 2017: contributor

Needed rebase. Addressed most of @ryanofsky points.

jonasschnelli added this to the "Blockers" column in a project

in src/wallet/wallet.cpp:3886 in bd9e1e8f89 outdated

3882@@ -3882,13 +3883,14 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
3883 
3884     // HD Restore: Make sure we always have a reasonable keypool size if HD is enabled
3885     if (walletInstance->IsHDEnabled()) {
3886-        if (walletInstance->IsCrypted()) {
3887-            InitWarning(_("Your are using an encrypted HD wallet. In case you recover a HD wallet, you may miss incomming or outgoing funds."));

ryanofsky commented at 6:33 pm on June 8, 2017:

In commit “SetSoftArg the keypool always to the minimum gap limit if HD is enabled”

It might be a good idea to keep the warning about missing funds in the case where IsCrypted && size < KEYPOOL_SIZE_MIN && GetBool("-hdignoregaplimit"). I also had a suggestion about this warning in #10240 (review)

in src/wallet/wallet.cpp:3883 in 46060c261c outdated

3885@@ -3879,12 +3886,11 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
3886             InitWarning(_("Your are using an encrypted HD wallet. In case you recover a HD wallet, you may miss incomming or outgoing funds."));
3887         }
3888         else {
3889-            if (GetArg("-keypool", DEFAULT_KEYPOOL_SIZE) < HD_RESTORE_KEYPOOL_SIZE_MIN ) {
3890-                InitWarning(_("Your keypool size is below the recommended limit for HD rescans. In case you recover a HD wallet, you may miss incomming or outgoing funds."));

ryanofsky commented at 6:50 pm on June 8, 2017:

In commit “Make sure we check the keypool min size during startup”

Why drop this warning? It still seems useful to warn about the “-keypool” being too small even if KeypoolCountExternalKeys() is momentarily returning a high number of keys. (I also had another suggestion about this warning at #10240 (review))

in src/wallet/wallet.cpp:1177 in 4cbc21689c

1184-    // to abandon a transaction and then have it inadvertently cleared by
1185-    // the notification that the conflicted transaction was evicted.
1186-
1187-    for (const CTransactionRef& ptx : vtxConflicted) {
1188-        SyncTransaction(ptx);
1189+void CWallet::BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex *pindex, const std::vector<CTransactionRef>& vtxConflicted, bool &requestPause) {

jnewbery commented at 10:06 pm on June 12, 2017:

I’m not going to comment on @ryanofsky’s other comments regarding commit order (although they all sound sensible). This one should definitely be fixed in commit “Add request-halt flag to BlockConnected signal” since it breaks the build and so breaks git bisect.

in src/net_processing.cpp:483 in 4cbc21689c

479@@ -480,6 +480,9 @@ void FindNextBlocksToDownload(NodeId nodeid, unsigned int count, std::vector<con
480     // Make sure pindexBestKnownBlock is up to date, we'll need it.
481     ProcessBlockAvailability(nodeid);
482 
483+    if (isBlockRequestsPaused())

jnewbery commented at 10:06 pm on June 12, 2017:

nit: braces

sipa commented at 11:12 pm on July 12, 2017:

Please address.

in src/validation.h:290 in 4cbc21689c

286@@ -287,6 +287,20 @@ CAmount GetBlockSubsidy(int nHeight, const Consensus::Params& consensusParams);
287 double GuessVerificationProgress(const ChainTxData& data, CBlockIndex* pindex);
288 
289 /**
290+ * Pausing block requests will allow to temporary pause the verification process.

jnewbery commented at 10:11 pm on June 12, 2017:

nit: s/will allow to temporary/will temporarily

in src/validation.h:297 in 4cbc21689c

292+ */
293+bool isBlockRequestsPaused();
294+void setBlockRequestsPaused(bool state);
295+
296+/**
297+ * Pausing tip updates will temporary pause connecting new blocks

jnewbery commented at 10:12 pm on June 12, 2017:

nit: s/will temporary/will temporarily

in src/wallet/rpcwallet.cpp:2050 in 4cbc21689c

2045@@ -2046,6 +2046,10 @@ UniValue walletpassphrase(const JSONRPCRequest& request)
2046 
2047     pwallet->TopUpKeyPool();
2048 
2049+    // give a hint to the wallet in case we have paused sync (we may have fall bellow the hd gap limit)
2050+    // this runs synchronous, at least during the resync, we can be sure the keypool can be topped up

jnewbery commented at 10:14 pm on June 12, 2017:

nit:

// Give a hint to the wallet in case we have paused sync (we may have fallen bellow the hd gap limit).
// This runs synchronously, at least during the resync, we can be sure the keypool can be topped up

in src/wallet/wallet.cpp:1019 in 4cbc21689c

1014+            // loop though all outputs
1015+            for(const CTxOut& txout: tx.vout) {
1016+                // extract addresses and check if they match with an unused keypool key
1017+                std::vector<CKeyID> vAffected;
1018+                CAffectedKeysVisitor(*this, vAffected).Process(txout.scriptPubKey);
1019+                BOOST_FOREACH(const CKeyID &keyid, vAffected) {

jnewbery commented at 10:16 pm on June 12, 2017:

nit: avoid BOOST_FOREACH

in src/wallet/wallet.cpp:1183 in 4cbc21689c

1192+        return;
1193     }
1194-    for (size_t i = 0; i < pblock->vtx.size(); i++) {
1195-        SyncTransaction(pblock->vtx[i], pindex, i);
1196+
1197+    {

jnewbery commented at 10:21 pm on June 12, 2017:

I don’t think there’s any need to change the indentation here

in src/wallet/wallet.cpp:1424 in 4cbc21689c

1415@@ -1350,6 +1416,57 @@ bool CWallet::IsHDEnabled() const
1416     return !hdChain.masterKeyID.IsNull();
1417 }
1418 
1419+void CWallet::EventuallyRescanAfterKeypoolTopUp() {
1420+    if (fSyncPausedUntilKeypoolExt) {
1421+
1422+        // for now, enable block requests and tip updates via the states
1423+        // this will only be sufficient as long as only a single wallet adjusts these stats
1424+        // switch to counters (instead of a single boolean state) could solve this

jnewbery commented at 10:25 pm on June 12, 2017:

As you note, this doesn’t work with multiwallet, which has now been merged. I think the setBlockRequestsPaused() and setTipUpdatesPaused() logic will now need to be updated to store the BlockRequests and TipUpdates state for each of the wallets.

in src/wallet/wallet.cpp:1441 in 4cbc21689c

1436+        }
1437+
1438+        CBlockIndex *pindexRescan = chainActive.Genesis();
1439+        CWalletDB walletdb(*dbw);
1440+        CBlockLocator locator;
1441+        if (walletdb.ReadBestBlock(locator))

jnewbery commented at 10:25 pm on June 12, 2017:

nit: braces

in src/wallet/wallet.cpp:1452 in 4cbc21689c

1447+            //this might happen if a user uses a old wallet within a pruned node
1448+            // or if he ran -disablewallet for a longer time, then decided to re-enable
1449+            if (fPruneMode)
1450+            {
1451+                CBlockIndex *block = chainActive.Tip();
1452+                while (block && block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA) && block->pprev->nTx > 0 && pindexRescan != block)

jnewbery commented at 10:26 pm on June 12, 2017:

nit: braces

sipa commented at 10:31 pm on June 12, 2017: member

Needs rebase.

in src/wallet/wallet.cpp:3500 in 4cbc21689c

3495+            ++it;
3496+        }
3497+    }
3498+
3499+    if (IsHDEnabled() && !TopUpKeyPool()) {
3500+        fSyncPausedUntilKeypoolExt = true;

jnewbery commented at 10:41 pm on June 12, 2017:

I don’t think you need to set fSyncPausedUntilKeypoolExt and log here, since the call to CheckKeypoolMinSize() below will catch when the keypool is too small.

I think this also prevents -hdignoregaplimit from working, since TopUpKeyPool() returns false without checking that argument.

in src/wallet/wallet.cpp:3891 in 4cbc21689c

3886+        if (GetArg("-keypool", DEFAULT_KEYPOOL_SIZE) < HD_RESTORE_KEYPOOL_SIZE_MIN && !GetBoolArg("-hdignoregaplimit", DEFAULT_IGNORE_HD_MIN_KEYPOOL_SIZE)) {
3887+            LogPrintf("Parameter Interaction: set keypool to required minimum for encrypted wallets (%d)\n", HD_RESTORE_KEYPOOL_SIZE_MIN);
3888+            SoftSetArg("-keypool", std::to_string(HD_RESTORE_KEYPOOL_SIZE_MIN));
3889+        }
3890+        else {
3891+            walletInstance->TopUpKeyPool();

jnewbery commented at 10:46 pm on June 12, 2017:

I don’t understand why you’re calling TopUpKeyPool() here only if -keypool >= HD_RESTORE_KEYPOOL_SIZE_MIN or hdignoregaplimit is set.

in src/wallet/wallet.cpp:3893 in 4cbc21689c

3888+            SoftSetArg("-keypool", std::to_string(HD_RESTORE_KEYPOOL_SIZE_MIN));
3889+        }
3890+        else {
3891+            walletInstance->TopUpKeyPool();
3892+        }
3893+        if (!walletInstance->CheckKeypoolMinSize() && !GetBoolArg("-hdignoregaplimit", DEFAULT_IGNORE_HD_MIN_KEYPOOL_SIZE)) {

jnewbery commented at 10:48 pm on June 12, 2017:

-hdignoregaplimit is checked within CheckKeypoolMinSize() so I don’t think you need it here (CheckKeypoolMinSize() will not return false if -hdignoregaplimit is true)

in test/functional/test_framework/test_framework.py:288 in 4cbc21689c

284@@ -285,7 +285,7 @@ def _initialize_chain(self, test_dir, num_nodes, cachedir):
285             # Create cache directories, run bitcoinds:
286             for i in range(MAX_NODES):
287                 datadir = initialize_datadir(cachedir, i)
288-                args = [os.getenv("BITCOIND", "bitcoind"), "-server", "-keypool=1", "-datadir=" + datadir, "-discover=0"]
289+                args = [os.getenv("BITCOIND", "bitcoind"), "-server", "-keypool=1", "-hdignoregaplimit", "-datadir=" + datadir, "-discover=0"]

jnewbery commented at 10:50 pm on June 12, 2017:

Why does this need to be changed for creating the cache directories?

in test/functional/test_framework/util.py:237 in 4cbc21689c

233@@ -234,7 +234,7 @@ def start_node(i, dirname, extra_args=None, rpchost=None, timewait=None, binary=
234     datadir = os.path.join(dirname, "node"+str(i))
235     if binary is None:
236         binary = os.getenv("BITCOIND", "bitcoind")
237-    args = [binary, "-datadir=" + datadir, "-server", "-keypool=1", "-discover=0", "-rest", "-logtimemicros", "-debug", "-debugexclude=libevent", "-debugexclude=leveldb", "-mocktime=" + str(get_mocktime()), "-uacomment=testnode%d" % i]
238+    args = [binary, "-datadir=" + datadir, "-server", "-keypool=1", "-hdignoregaplimit", "-discover=0", "-rest", "-logtimemicros", "-debug", "-debugexclude=libevent", "-debugexclude=leveldb", "-mocktime=" + str(get_mocktime()), "-uacomment=testnode%d" % i]

jnewbery commented at 10:51 pm on June 12, 2017:

I don’t think you should be changing this command line argument for all tests.

in test/functional/test_runner.py:57 in 4cbc21689c

52@@ -53,6 +53,8 @@
53     # Scripts that are run by the travis build process.
54     # Longest test should go first, to favor running tests in parallel
55     'wallet-hd.py',
56+    'wallet-hd-restore.py',
57+    'wallet-hd-restore.py',

jnewbery commented at 10:51 pm on June 12, 2017:

remove duplicate

in test/functional/wallet-hd-restore.py:5 in 4cbc21689c

0@@ -0,0 +1,162 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2017 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+"""Test Hierarchical Deterministic wallet function."""

jnewbery commented at 10:52 pm on June 12, 2017:

Please update docstring to describe what this test is doing (I think you copied this directly from wallet-hd.py)

sipa commented at 11:10 pm on July 12, 2017:

Please address.

in test/functional/wallet-hd-restore.py:7 in 4cbc21689c

0@@ -0,0 +1,162 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2017 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+"""Test Hierarchical Deterministic wallet function."""
6+
7+from test_framework.test_framework import BitcoinTestFramework

jnewbery commented at 10:52 pm on June 12, 2017:

Please order imports in PEP8 ordering (std library, then 3rd party libraries, then local imports)

in test/functional/wallet-hd-restore.py:11 in 4cbc21689c

 6+
 7+from test_framework.test_framework import BitcoinTestFramework
 8+from test_framework.util import *
 9+import os
10+import shutil
11+from pprint import pprint

jnewbery commented at 10:52 pm on June 12, 2017:

import not used

in test/functional/wallet-hd-restore.py:19 in 4cbc21689c

14+
15+    def __init__(self):
16+        super().__init__()
17+        self.setup_clean_chain = True
18+        self.num_nodes = 2
19+        self.node_args = [['-usehd=0'], ['-usehd=1', '-keypool=100', '-hdignoregaplimit=0']]

jnewbery commented at 10:53 pm on June 12, 2017:

you can set self.extra_args here, and then you won’t need to override the setup_network() method below (the test framework will take care of everything if you set the extra_args here.

in test/functional/wallet-hd-restore.py:29 in 4cbc21689c

24+        connect_nodes_bi(self.nodes, 0, 1)
25+
26+    def run_test (self):
27+        tmpdir = self.options.tmpdir
28+
29+        print("Initialize wallet including backups of unencrypted and encrypted wallet")

jnewbery commented at 10:54 pm on June 12, 2017:

Please use self.log.info() instead of print for logging.

in test/functional/wallet-hd-restore.py:47 in 4cbc21689c

42+        self.nodes[1] = start_node(1, self.options.tmpdir, self.node_args[1])
43+        for _ in range(50):
44+            addr_enc_oldpool = self.nodes[1].getnewaddress()
45+
46+        # now make sure we retrive an address in the extended pool
47+        self.nodes[1].walletpassphrase("test", 10)

jnewbery commented at 10:58 pm on June 12, 2017:

Suggest you set the timeout higher, perhaps to 60 seconds. Tests should be robust to bitcoind hanging for several seconds, especially if running in parallel on Travis.

in test/functional/wallet-hd-restore.py:82 in 4cbc21689c

77+        assert_equal(self.nodes[1].getbalance(), 3)
78+        assert_equal(self.nodes[1].listtransactions()[0]['category'], "receive")
79+
80+        print("Testing with unencrypted wallet")
81+        self.stop_node(1)
82+        shutil.rmtree(tmpdir + "/node1/regtest")

jnewbery commented at 11:00 pm on June 12, 2017:

please don’t do this! It erases the debug.log file. Are you able to be more targeted in what you’re erasing?

sipa commented at 11:11 pm on July 12, 2017:

Please address.

in test/functional/wallet-hd-restore.py:65 in 4cbc21689c

60+        self.nodes[0].generate(101)
61+        addr = self.nodes[1].getnewaddress()
62+        assert_equal(self.nodes[1].validateaddress(addr)['hdkeypath'], "m/0'/0'/11'")
63+
64+        rawch = self.nodes[1].getrawchangeaddress()
65+        txid = self.nodes[0].sendtoaddress(addr, 1)

jnewbery commented at 11:02 pm on June 12, 2017:

txid isn’t used. No need to assign it here or below.

jnewbery commented at 11:06 pm on June 12, 2017: member

Needs rebase. I’ve added a lot of mainly nitty comments. The most important thing to address is to make this compatible with multiwallet.

gmaxwell commented at 8:05 pm on June 15, 2017: contributor

Will begin code review post rebase.

Question about the parameters– Why 20? that is very small, I am not aware of any reason to not have it be more like– say– 10,000. With a large number the pause is hopefully seldom/never hit. Electrum uses a very small number because its a single user wallet and has to send all these addresses for processing to a remote server. In a commercial application getting reordering beyond 20 would be very easy. (e.g hand out 30 keys to different users and only the first one makes a payment).

jonasschnelli commented at 8:27 pm on June 15, 2017: contributor

Agree that 20 is probably way to low. It’s just a single constant and I worry more about getting the PR on a level where everything works as expected. Changing the 20 to 1000 then is simple… once the PR is stable, we can also test performance better (maybe between 20 and 10k).

jonasschnelli removed this from the "Blockers" column in a project

in src/wallet/wallet.cpp:3801 in 2a203b73fd outdated

3792@@ -3725,6 +3793,19 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
3793 
3794     RegisterValidationInterface(walletInstance);
3795 
3796+    // HD Restore: Make sure we always have a reasonable keypool size if HD is enabled
3797+    if (walletInstance->IsHDEnabled()) {
3798+        if (walletInstance->IsCrypted()) {
3799+            InitWarning(_("Your are using an encrypted HD wallet. In case you recover a HD wallet, you may miss incomming or outgoing funds."));
3800+        }
3801+        else {

sipa commented at 11:11 pm on July 12, 2017:

Nit: else on the same line as }

sipa commented at 11:18 pm on July 12, 2017: member

Some comments already.

Overall question: why is any of this conditional on HD being enabled? I think we want to mark user and topup any key seen on the network at any time.

laanwj added this to the "Blockers" column in a project

jonasschnelli commented at 9:22 pm on July 14, 2017: contributor

Closing in favor of #10830

jonasschnelli closed this on Jul 14, 2017

laanwj removed this from the "Blockers" column in a project

DrahtBot locked this on Sep 8, 2021

Add HD wallet auto-restore functionality #10240

unencrypted wallets

encrypted wallets in non pruning mode

encrypted wallets in pruned mode

unencrypted wallets

encrypted wallets in non pruning mode

encrypted wallets in pruned mode