aboutsummaryrefslogtreecommitdiff
path: root/src
diff options
context:
space:
mode:
authorAva Chow <github@achow101.com>2024-02-29 13:16:12 -0500
committerAva Chow <github@achow101.com>2024-02-29 13:25:38 -0500
commit22a5ccfb06429c3b0e111a191e7ba71d98a1d198 (patch)
tree58205b133d8968fb72d9214c7d9df9fac95b0ed8 /src
parent61aa981b8c40eda39fbab8c7cd105c236b0f33bd (diff)
parente073f1dfda7a2a2cb2be9fe2a1d576f122596021 (diff)
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
Diffstat (limited to 'src')
-rw-r--r--src/wallet/wallet.cpp4
1 files changed, 3 insertions, 1 deletions
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 26c5256f6f..3ac09430d8 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -2607,8 +2607,10 @@ util::Result<CTxDestination> ReserveDestination::GetReservedDestination(bool int
if (nIndex == -1) {
CKeyPool keypool;
- auto op_address = m_spk_man->GetReservedDestination(type, internal, nIndex, keypool);
+ int64_t index;
+ auto op_address = m_spk_man->GetReservedDestination(type, internal, index, keypool);
if (!op_address) return op_address;
+ nIndex = index;
address = *op_address;
fInternal = keypool.fInternal;
}