From 7462870d354a205333c446d88c040a7d3982c3c4 Mon Sep 17 00:00:00 2001
From: Ava Chow <github@achow101.com>
Date: Thu, 29 Feb 2024 13:16:12 -0500
Subject: [PATCH] Merge bitcoin/bitcoin#29510: wallet: `getrawchangeaddress`
 and `getnewaddress` failures should not affect keypools for descriptor
 wallets

e073f1dfda7a2a2cb2be9fe2a1d576f122596021 test: make sure keypool sizes do not change on `getrawchangeaddress`/`getnewaddress` failures (UdjinM6)
367bb7a80cc71130995672c853d4a6e0134721d6 wallet: Avoid updating `ReserveDestination::nIndex` when `GetReservedDestination` fails (UdjinM6)

Pull request description:

  I think the expected behaviour of `getrawchangeaddress` and `getnewaddress` RPCs is that their failure should not affect keypool in any way. At least that's how legacy wallets work, you can confirm this behaviour by running `wallet_keypool.py --legacy-wallet` on master with e073f1dfda7a2a2cb2be9fe2a1d576f122596021 applied on top. However running `wallet_keypool.py --descriptors` on the same commit results in the following failure:
  ```
    File "/path/to/bitcoin/test/functional/test_framework/test_framework.py", line 131, in main
      self.run_test()
    File "/path/to/bitcoin/test/functional/wallet_keypool.py", line 114, in run_test
      assert_equal(kp_size_before, kp_size_after)
    File "/path/to/bitcoin/test/functional/test_framework/util.py", line 57, in assert_equal
      raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
  AssertionError: not([18, 24] == [19, 24])
  ```

  This happens because we pass `nIndex` (which is a class member) into `GetReservedDestination` and since it's passed by reference we get an updated value back, so `nIndex` won't be equal `-1` anymore, no matter if the function failed or succeeded. This means that `ReturnDestination` (called by dtor of `ReserveDestination`) will try to return something we did not actually reserve.

  The fix is to simply use a temporary variable instead of a class member and only update `nIndex` when `op_address` actually has value, basically do it the same way we do for other class members (`address` and `fInternal`) already.

ACKs for top commit:
  achow101:
    ACK e073f1dfda7a2a2cb2be9fe2a1d576f122596021
  josibake:
    ACK https://github.com/bitcoin/bitcoin/pull/29510/commits/e073f1dfda7a2a2cb2be9fe2a1d576f122596021

Tree-SHA512: 1128288a60dd4d8f306ef6f7ac66cdfeae3c9cc35c66ecada2d78fa61ac759f2a757b70fc3976ba8b5081200942b58dfabc184c01ccf911af40ba8c145344651
---
 src/wallet/wallet.cpp                |  4 +++-
 test/functional/wallet_keypool_hd.py | 14 ++++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index c459ec14125a1b..6974dc3b4cb4f8 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -4206,9 +4206,11 @@ bool ReserveDestination::GetReservedDestination(CTxDestination& dest, bool fInte
         m_spk_man->TopUp();
 
         CKeyPool keypool;
-        if (!m_spk_man->GetReservedDestination(fInternalIn, address, nIndex, keypool)) {
+        int64_t index;
+        if (!m_spk_man->GetReservedDestination(fInternalIn, address, index, keypool)) {
             return false;
         }
+        nIndex = index;
         fInternal = keypool.fInternal;
     }
     dest = address;
diff --git a/test/functional/wallet_keypool_hd.py b/test/functional/wallet_keypool_hd.py
index 0acf8737a9a43a..f4cc5fed8f319f 100755
--- a/test/functional/wallet_keypool_hd.py
+++ b/test/functional/wallet_keypool_hd.py
@@ -101,12 +101,19 @@ def run_test(self):
         nodes[0].getrawchangeaddress()
         nodes[0].getrawchangeaddress()
         nodes[0].getrawchangeaddress()
+        # remember keypool sizes
+        wi = nodes[0].getwalletinfo()
+        kp_size_before = [wi['keypoolsize_hd_internal'], wi['keypoolsize']]
         # the next one should fail
         try:
             nodes[0].getrawchangeaddress()
             raise AssertionError('Keypool should be exhausted after six addresses')
         except JSONRPCException as e:
             assert e.error['code']==-12
+        # check that keypool sizes did not change
+        wi = nodes[0].getwalletinfo()
+        kp_size_after = [wi['keypoolsize_hd_internal'], wi['keypoolsize']]
+        assert_equal(kp_size_before, kp_size_after)
 
         addr = set()
         # drain the external keys
@@ -117,12 +124,19 @@ def run_test(self):
         addr.add(nodes[0].getnewaddress())
         addr.add(nodes[0].getnewaddress())
         assert len(addr) == 6
+        # remember keypool sizes
+        wi = nodes[0].getwalletinfo()
+        kp_size_before = [wi['keypoolsize_hd_internal'], wi['keypoolsize']]
         # the next one should fail
         try:
             addr = nodes[0].getnewaddress()
             raise AssertionError('Keypool should be exhausted after six addresses')
         except JSONRPCException as e:
             assert e.error['code']==-12
+        # check that keypool sizes did not change
+        wi = nodes[0].getwalletinfo()
+        kp_size_after = [wi['keypoolsize_hd_internal'], wi['keypoolsize']]
+        assert_equal(kp_size_before, kp_size_after)
 
         # refill keypool with three new addresses
         nodes[0].walletpassphrase('test', 1)