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:
0 File "/path/to/bitcoin/test/functional/test_framework/test_framework.py", line 131, in main
1 self.run_test()
2 File "/path/to/bitcoin/test/functional/wallet_keypool.py", line 114, in run_test
3 assert_equal(kp_size_before, kp_size_after)
4 File "/path/to/bitcoin/test/functional/test_framework/util.py", line 57, in assert_equal
5 raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
6AssertionError: 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.
