mirrors/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-01-19 05:45:05 +01:00
Code Issues Projects Releases Wiki Activity

Merge bitcoin/bitcoin#29510: wallet: getrawchangeaddress and getnewaddress failures should not affect keypools for descriptor wallets

Browse Source 
e073f1dfda test: make sure keypool sizes do not change on `getrawchangeaddress`/`getnewaddress` failures (UdjinM6)
367bb7a80c 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 e073f1dfda 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 e073f1dfda
  josibake:
    ACK e073f1dfda

Tree-SHA512: 1128288a60dd4d8f306ef6f7ac66cdfeae3c9cc35c66ecada2d78fa61ac759f2a757b70fc3976ba8b5081200942b58dfabc184c01ccf911af40ba8c145344651
This commit is contained in:
Ava Chow 2024-02-29 13:16:12 -05:00
commit 22a5ccfb06
No known key found for this signature in database
GPG Key ID: 17565732E08E5E41
2 changed files with 18 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
src/wallet/wallet.cpp
View File

@ -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;
}

16
test/functional/wallet_keypool.py
View File

@ -103,11 +103,18 @@ class KeyPoolTest(BitcoinTestFramework):
nodes[0].getrawchangeaddress()
nodes[0].getrawchangeaddress()
nodes[0].getrawchangeaddress()
addr = set()
# remember keypool sizes
wi = nodes[0].getwalletinfo()
kp_size_before = [wi['keypoolsize_hd_internal'], wi['keypoolsize']]
# the next one should fail
assert_raises_rpc_error(-12, "Keypool ran out", nodes[0].getrawchangeaddress)
# 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)
# drain the external keys
addr = set()
addr.add(nodes[0].getnewaddress(address_type="bech32"))
addr.add(nodes[0].getnewaddress(address_type="bech32"))
addr.add(nodes[0].getnewaddress(address_type="bech32"))
@ -115,8 +122,15 @@ class KeyPoolTest(BitcoinTestFramework):
addr.add(nodes[0].getnewaddress(address_type="bech32"))
addr.add(nodes[0].getnewaddress(address_type="bech32"))
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
assert_raises_rpc_error(-12, "Error: Keypool ran out, please call keypoolrefill first", nodes[0].getnewaddress)
# 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)