[backport#17719] Document better -keypool as a look-ahead safety mech…

…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

src/wallet/init.cpp

@@ -57,10 +57,14 @@ void WalletInit::AddWalletOptions() const {
                            "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). 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.",
+                  DEFAULT_KEYPOOL_SIZE),
+        ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
     gArgs.AddArg(
         "-maxapsfee=<n>",
         strprintf(

src/wallet/scriptpubkeyman.h

@@ -79,6 +79,11 @@ std::vector<CKeyID> GetAffectedKeys(const CScript &spk,
  * keys (by default 1000) ahead of the last used key and scans for the
  * addresses of those keys.  This avoids the risk of not seeing transactions
  * involving the wallet's addresses, or of re-using the same address.
+ * 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.
  *
  * The HD-split wallet feature added a second keypool (commit: 02592f4c). There
  * is an external keypool (for addresses to hand out) and an internal keypool