Document better -keypool as a look-ahead safety mechanism #17719

ariard commented at 4:25 AM on December 11, 2019: member

If after a backup, an address is issued beyond the initial keypool range and none of the addresses in this range is seen onchain, if a wallet is restored from backup, even in case of rescan, funds may be loss due to the look-ahead buffer not being incremented and so restored wallet not detecting onchain out-of-range address as derived from its seed.

This scenario is theoretically unavoidable due to the requirement of the keypool to have a max size. However, given the default keypool size, this is unlikely. Document better keypool size implications to avoid user setting a too low value.

While reviewing #17681, it took me a while to figure out the safety implications of keypool, I find it would be better to document this a bit farther to avoid users shooting themselves in the foot. For further context & discussion, see #17681 (comment)

DrahtBot added the label Wallet on Dec 11, 2019

in src/wallet/init.cpp:49 in 46b54df2a9 outdated

  45 | @@ -46,7 +46,7 @@ void WalletInit::AddWalletOptions() const
  46 |  
  47 |      gArgs.AddArg("-fallbackfee=<amt>", strprintf("A fee rate (in %s/kB) that will be used when fee estimation has insufficient data. 0 to entirely disable the fallbackfee feature. (default: %s)",
  48 |                                                                 CURRENCY_UNIT, FormatMoney(DEFAULT_FALLBACK_FEE)), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
  49 | -    gArgs.AddArg("-keypool=<n>", strprintf("Set key pool size to <n> (default: %u)", DEFAULT_KEYPOOL_SIZE), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
  50 | +    gArgs.AddArg("-keypool=<n>", strprintf("Set key pool size to <n> (default: %u). Decreasing keypool size to a low value may increase the risk of losing funds in case of restoring from an old backup if none of the addresses in the original keypool have been used.", DEFAULT_KEYPOOL_SIZE), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);

jonatack commented at 9:15 AM on December 11, 2019:

Consider:

-    gArgs.AddArg("-keypool=<n>", strprintf("Set key pool size to <n> (default: %u). Decreasing keypool size to a low value may increase the risk of losing funds in case of restoring from an old backup if none of the addresses in the original keypool have been used.", DEFAULT_KEYPOOL_SIZE), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
+    gArgs.AddArg("-keypool=<n>", strprintf("Set key pool size to <n> (default: %u). "
+                                           "Smaller sizes may increase the risk of losing funds when restoring from an "
+                                           "old backup, if none of the addresses in the original keypool have been used.",
+                                           DEFAULT_KEYPOOL_SIZE), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);

instagibbs commented at 3:00 PM on December 12, 2019:

maybe want to clarify that the original keypool was larger

ariard commented at 9:08 PM on December 17, 2019:

My understanding is if none of the addresses is detected as used on-chain, the keypool is not going to be top-up. If size is N keypool stays at range 0..N instead of being shifted by I, with I being index of address detected. Size is always going to be constant ?

Or do you mean original keypool as keypool set by default ?

instagibbs commented at 9:10 PM on December 17, 2019:

@ariard who are you asking here? sorry

ariard commented at 9:16 PM on December 17, 2019:

@instagibbs what your comment that seems unclear to me

instagibbs commented at 9:19 PM on December 17, 2019:

I'm saying that the explanation should explicitly mention that the value for the "original keypool" was larger in this loss-risk case.

ariard commented at 9:26 PM on December 17, 2019:

The original keypool as set by a mindless user before any detection in the loss-risk case or the default keypool ? Sorry first option doesn't make sense to me as AFAII keypool keeps the same size.

in src/wallet/scriptpubkeyman.h:79 in 46b54df2a9 outdated

  71 | @@ -72,6 +72,11 @@ std::vector<CKeyID> GetAffectedKeys(const CScript& spk, const SigningProvider& p
  72 |   * keys (by default 1000) ahead of the last used key and scans for the
  73 |   * addresses of those keys.  This avoids the risk of not seeing transactions
  74 |   * involving the wallet's addresses, or of re-using the same address.
  75 | + * In the unlikely case where none of the addresses in the `gap limit` would
  76 | + * be used onchain, the look-ahead wouldn't be incremented to keep
  77 | + * a constant-size and addresses beyond this range wouldn't be detected by an
  78 | + * old backup, that's why it's not recommended to decrease keypool size lower
  79 | + * than default value.

jonatack commented at 9:26 AM on December 11, 2019:

Consider:

- * In the unlikely case where none of the addresses in the `gap limit` would
- * be used onchain, the look-ahead wouldn't be incremented to keep
- * a constant-size and addresses beyond this range wouldn't be detected by an
- * old backup, that's why it's not recommended to decrease keypool size lower
- * than default value.
+
+ * In the unlikely case where none of the addresses in the 'gap limit' are used
+ * on-chain, the look-ahead will not be incremented to keep a constant size and
+ * addresses beyond this range will not be detected by an old backup. For this
+ * reason, it is not recommended to decrease keypool size lower than default
+ * value.

jonatack commented at 9:29 AM on December 11, 2019: member

Concept ACK. Thanks for documenting this @ariard. Some writing suggestions below that build on what you wrote; feel free to pick and choose.

ariard force-pushed on Dec 17, 2019

ariard commented at 9:05 PM on December 17, 2019: member

@jonatack, thanks for review, took your suggestions!

laanwj commented at 10:57 AM on December 18, 2019: member

ACK. I think at some point it'd make sense to rename "keypoolsize" to something like "gap limit" or "lookahead" as other wallets do.

in src/wallet/init.cpp:49 in 8af8063272 outdated

  45 | @@ -46,7 +46,7 @@ void WalletInit::AddWalletOptions() const
  46 |  
  47 |      gArgs.AddArg("-fallbackfee=<amt>", strprintf("A fee rate (in %s/kB) that will be used when fee estimation has insufficient data. 0 to entirely disable the fallbackfee feature. (default: %s)",
  48 |                                                                 CURRENCY_UNIT, FormatMoney(DEFAULT_FALLBACK_FEE)), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
  49 | -    gArgs.AddArg("-keypool=<n>", strprintf("Set key pool size to <n> (default: %u)", DEFAULT_KEYPOOL_SIZE), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
  50 | +    gArgs.AddArg("-keypool=<n>", strprintf("Set key pool size to <n> (default: %u). Smaller sizes may increase the risk of losing funds when restoring from an old backup, if none of the addresses in the original keypool have been used.", DEFAULT_KEYPOOL_SIZE), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);

jonatack commented at 12:02 PM on December 18, 2019:

Now that I test it on the command line, perhaps:

- Smaller sizes may increase the risk...
+ Warning: A size lower than the default may increase the risk...

Happy to re-ACK if you agree

ryanofsky commented at 4:48 PM on December 18, 2019:

Now that I test it on the command line, perhaps:
- Smaller sizes may increase the risk...
+ Warning: A size lower than the default may increase the risk...

Adding the Warning: prefix could be a good idea, but it's misleading to say that "size lower than the default may increase the risk", because that implies the default value has the right amount of risk, even though it could easily be unsafe if you generate a lot of addresses. "Smaller sizes may increase the risk" is more accurate because smaller sizes decrease the risk and larger sizes increase it, and the default value is actually pretty arbitrary.

jonatack commented at 5:28 PM on December 18, 2019:

Good points @ryanofsky; I agree.

ariard commented at 6:40 PM on December 18, 2019:

Thanks @jonatack for the suggestion took it on f41d589, should be only diff!

"Smaller sizes may increase the risk" is more accurate because smaller sizes decrease the risk and larger sizes increase it

I think you mean the inverse here, at least that's what the updated comment aims to convey.

jonatack commented at 12:03 PM on December 18, 2019: member

ACK 8af806327254086a27ed478c4b7316116132927e

before

bitcoin ((HEAD detached at origin/pr/17719)) $ bitcoind -help | grep -C1 keypool

  -keypool=<n>
       Set key pool size to <n> (default: 1000)

after

bitcoin/src ((HEAD detached at origin/pr/17719)) $ bitcoind -help | grep -C1 keypool

  -keypool=<n>
       Set key pool size to <n> (default: 1000). Smaller sizes may increase the
       risk of losing funds when restoring from an old backup, if none
       of the addresses in the original keypool have been used.

ryanofsky approved

ryanofsky commented at 4:57 PM on December 18, 2019: member

Code review ACK 46b54df2a9ff20355e6cc9a7127f2b19200935eb. New documentation is an improvement. I wonder if there's a good way to convey that "risk of losing funds" means list of "risk of irrecoverably losing funds" in the case of non-HD wallets and "risk of funds not showing up when recovering with a small pool size" in the case of HD wallets.

Document better -keypool as a look-ahead safety mechanism

If after a backup, an address is issued beyond the initial
keypool range and none of the addresses in this range
is seen onchain, if a wallet is restored from backup, even in
case of rescan, funds may be loss due to the look-ahead
buffer not being incremented and so restored wallet not detecting
onchain out-of-range address as derived from its seed.

This scenario is theoretically unavoidable due to the requirement
of the keypool to have a max size. However, given the default
keypool size, this is unlikely. Document better keypool size
implications to avoid user setting a too low value.

f41d589669

ariard force-pushed on Dec 18, 2019

ariard commented at 6:43 PM on December 18, 2019: member

@ryanofsky, thanks for review, I think than both HD/non-HD wallets are subjected to risk of loss of funds in case of look-ahead misses. Deriving all your keys by backing up all of them or just the seed, and detecting if derived range has been used are too tied problems but still different?

Overall, I agree security assumptions of wallet operations could be more documented.

ryanofsky commented at 6:59 PM on December 18, 2019: member

Yeah, this PR is already an improvement. I think there is an important distinction between irrecoverable loss off funds restoring an non-hd backup vs fixable loss off funds restoring an HD wallet backup with too-small -keypool value. But it seems ok for this documentation not to go into the details if there isn't a simple way to describe them.

leto commented at 1:24 PM on January 1, 2020: contributor

Concept ACK. This will hopefully prevent some people from losing funds and has no downside.

DrahtBot commented at 12:51 PM on January 8, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14582 (wallet: always do avoid partial spends if fees are within a specified range by kallewoof)

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.

ryanofsky approved

ryanofsky commented at 10:09 PM on January 8, 2020: member

Code review ACK f41d58966995fe69df433fa684117fae74a56e66. Just "Warning:" prefix added since the last review

jonatack commented at 7:52 PM on January 9, 2020: member

ACK f41d58966995fe69df433fa684117fae74a56e66 code review and build/test. The added Warning: since last review is a good addition.

bitcoin/src ((HEAD detached at origin/pr/17719))$ bitcoind -help | grep -C1 keypool

  -keypool=<n>
       Set key pool size to <n> (default: 1000). Warning: Smaller sizes may
       increase the risk of losing funds when restoring from an old
       backup, if none of the addresses in the original keypool have
       been used.

ryanofsky commented at 7:56 PM on January 29, 2020: member

Maintainers, it seems like this might be ready to be merged. There are just two ACKs, but it is a pretty straightforward documentation update

meshcollider commented at 8:08 PM on January 29, 2020: contributor

LGTM f41d58966995fe69df433fa684117fae74a56e66

meshcollider referenced this in commit aabec94541 on Jan 29, 2020

meshcollider merged this on Jan 29, 2020

meshcollider closed this on Jan 29, 2020

sidhujag referenced this in commit 7aec48ac80 on Feb 1, 2020

jasonbcox referenced this in commit 9df09227a2 on Oct 10, 2020

sidhujag referenced this in commit 93a20c3437 on Nov 10, 2020

DrahtBot locked this on Feb 15, 2022