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

ariard · 2019-12-11T04:25:41Z

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)

jonatack

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

jonatack · 2019-12-11T09:15:00Z

src/wallet/init.cpp

@@ -46,7 +46,7 @@ void WalletInit::AddWalletOptions() const

    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)",
                                                               CURRENCY_UNIT, FormatMoney(DEFAULT_FALLBACK_FEE)), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
-    gArgs.AddArg("-keypool=<n>", strprintf("Set key pool size to <n> (default: %u)", DEFAULT_KEYPOOL_SIZE), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
+    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);


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

maybe want to clarify that the original keypool was larger

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 ?

@ariard who are you asking here? sorry

@instagibbs what your comment that seems unclear to me

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

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.

jonatack · 2019-12-11T09:26:29Z

src/wallet/scriptpubkeyman.h

+ * 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.


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.

ariard · 2019-12-17T21:05:04Z

@jonatack, thanks for review, took your suggestions!

laanwj · 2019-12-18T10:57:23Z

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

jonatack

ACK 8af8063

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.

jonatack · 2019-12-18T12:02:23Z

src/wallet/init.cpp

@@ -46,7 +46,7 @@ void WalletInit::AddWalletOptions() const

    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)",
                                                               CURRENCY_UNIT, FormatMoney(DEFAULT_FALLBACK_FEE)), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
-    gArgs.AddArg("-keypool=<n>", strprintf("Set key pool size to <n> (default: %u)", 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);


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

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.

Good points @ryanofsky; I agree.

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.

ryanofsky

Code review ACK 46b54df. 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.

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.

ariard · 2019-12-18T18:43:49Z

@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 · 2019-12-18T18:59:41Z

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 · 2020-01-01T13:24:16Z

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

DrahtBot · 2020-01-08T12:51:52Z

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

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

jonatack

ACK f41d589 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 · 2020-01-29T19:56:44Z

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

LGTM f41d589

f41d589 Document better -keypool as a look-ahead safety mechanism (Antoine Riard) Pull request description: 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) ACKs for top commit: ryanofsky: Code review ACK f41d589. Just "Warning:" prefix added since the last review jonatack: ACK f41d589 code review and build/test. The added `Warning:` since last review is a good addition. Tree-SHA512: d3d0ee88fcdfc5c8841a2bd4bada0e4eeb412a0dce5054e5fb023643c2fa57206a0f3efb06890c245528dc4431413ed2fd5645b9319d26245d044c490b7f0db0

…mechanism f41d589 Document better -keypool as a look-ahead safety mechanism (Antoine Riard) Pull request description: 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 bitcoin#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 bitcoin#17681 (comment) ACKs for top commit: ryanofsky: Code review ACK f41d589. Just "Warning:" prefix added since the last review jonatack: ACK f41d589 code review and build/test. The added `Warning:` since last review is a good addition. Tree-SHA512: d3d0ee88fcdfc5c8841a2bd4bada0e4eeb412a0dce5054e5fb023643c2fa57206a0f3efb06890c245528dc4431413ed2fd5645b9319d26245d044c490b7f0db0

…anism Summary: f41d58966995fe69df433fa684117fae74a56e66 Document better -keypool as a look-ahead safety mechanism (Antoine Riard) Pull request description: 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 bitcoin/bitcoin#17681 (comment) ACKs for top commit: ryanofsky: Code review ACK f41d58966995fe69df433fa684117fae74a56e66. Just "Warning:" prefix added since the last review jonatack: ACK f41d58966995fe69df433fa684117fae74a56e66 code review and build/test. The added `Warning:` since last review is a good addition. --- Backport of Core [[bitcoin/bitcoin#17719 | PR17719]] Test Plan: ninja read it Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7836

…mechanism f41d589 Document better -keypool as a look-ahead safety mechanism (Antoine Riard) Pull request description: 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 bitcoin#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 bitcoin#17681 (comment) ACKs for top commit: ryanofsky: Code review ACK f41d589. Just "Warning:" prefix added since the last review jonatack: ACK f41d589 code review and build/test. The added `Warning:` since last review is a good addition. Tree-SHA512: d3d0ee88fcdfc5c8841a2bd4bada0e4eeb412a0dce5054e5fb023643c2fa57206a0f3efb06890c245528dc4431413ed2fd5645b9319d26245d044c490b7f0db0

DrahtBot added the Wallet label Dec 11, 2019

jonatack reviewed Dec 11, 2019

View reviewed changes

ariard force-pushed the 2019-12-improve-keypool-doc branch from 46b54df to 8af8063 Compare December 17, 2019 21:04

jonatack reviewed Dec 18, 2019

View reviewed changes

ryanofsky approved these changes Dec 18, 2019

View reviewed changes

ariard force-pushed the 2019-12-improve-keypool-doc branch from 8af8063 to f41d589 Compare December 18, 2019 18:38

ryanofsky approved these changes Jan 8, 2020

View reviewed changes

jonatack reviewed Jan 9, 2020

View reviewed changes

meshcollider reviewed Jan 29, 2020

View reviewed changes

meshcollider merged commit f41d589 into bitcoin:master Jan 29, 2020

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

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

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

Uh oh!

Conversation

ariard commented Dec 11, 2019

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard Dec 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard commented Dec 17, 2019

Uh oh!

laanwj commented Dec 18, 2019

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack Dec 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ariard commented Dec 18, 2019

Uh oh!

ryanofsky commented Dec 18, 2019

Uh oh!

leto commented Jan 1, 2020

Uh oh!

DrahtBot commented Jan 8, 2020

Conflicts

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Jan 29, 2020

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ariard Dec 17, 2019 •

edited

Loading

jonatack left a comment •

edited

Loading

jonatack Dec 18, 2019 •

edited

Loading