Extracted from #16944
First this PR simplifies the check when generating a change address, by dropping CanGetAddresses and just letting reservedest.GetReservedDestination do this check.
Second, when the keypool is empty, instead of immediately giving up, we create a dummy change address and pass that to coin selection. If we didn’t need the change address (e.g. when spending the entire balance), then it’s all good. If we did need a change address, we throw the original error.
2980@@ -2980,21 +2981,23 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
2981 // rediscover unknown transactions that were written with keys of ours to recover
2982 // post-backup change.
2983
2984- // Reserve a new key pair from key pool
2985+ // Reserve a new key pair from key pool. If it fails, provide a dummy
@instagibbs wrote:
At the risk of making the error message less precise, I think this whole block is easier to read if it just relies on
GetReservedDestinationto check ifCanGetAddressis true or not. You also can just not storeretand use it directly in the conditional.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
Looks like you could avoid failed_to_get_change_address by applying
0--- a/src/wallet/wallet.cpp
1+++ b/src/wallet/wallet.cpp
2@@ -2968,7 +2968,7 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
3 // TODO: pass in scriptChange instead of reservedest so
4 // change transaction isn't always pay-to-bitcoin-address
5 CScript scriptChange;
6- bool failed_to_get_change_address{false};
7+ assert(scriptChange.empty());
8
9 // coin control: send change to custom address
10 if (!boost::get<CNoDestination>(&coin_control.destChange)) {
11@@ -2984,7 +2984,6 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
12 // Reserve a new key pair from key pool. If it fails, provide a dummy
13 // destination in case we don't need change.
14 if (!CanGetAddresses(true)) {
15- failed_to_get_change_address = true;
16 strFailReason = _("Can't generate a change-address key. No keys in the internal keypool and can't generate any keys.").translated;
17 } else {
18 CTxDestination dest;
19@@ -2993,7 +2992,6 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
20 if (ret) {
21 scriptChange = GetScriptForDestination(dest);
22 } else {
23- failed_to_get_change_address = true;
24 strFailReason = _("Keypool ran out, please call keypoolrefill first").translated;
25 }
26
27@@ -3210,10 +3208,9 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
28 }
29
30 // Give up if change keypool ran out and we failed to find a solution without change:
31- if (failed_to_get_change_address && nChangePosInOut != -1) {
32+ if (scriptChange.empty() && nChangePosInOut != -1) {
33 return false;
34 }
35-
36 }
37
38 // Shuffle selected coins and fill in final vin
failed_to_get_change_address and @instagibbs suggestion to simplify the change address check.
3200@@ -3200,6 +3201,12 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
3201 coin_selection_params.use_bnb = false;
3202 continue;
3203 }
3204+
3205+ // Give up if change keypool ran out and we failed to find a solution without change:
3206+ if (scriptChange.empty() && nChangePosInOut != -1) {
3207+ return false;
3208+ }
3209+
2985+ // Reserve a new key pair from key pool. If it fails, provide a dummy
2986+ // destination in case we don't need change.
2987 CTxDestination dest;
2988 const OutputType change_type = TransactionChangeType(coin_control.m_change_type ? *coin_control.m_change_type : m_default_change_type, vecSend);
2989 if (reservedest.GetReservedDestination(change_type, dest, true)) {
2990 scriptChange = GetScriptForDestination(dest);
change_prototype_txout and change_output_size setting into here?. Then set change_output_size to 0 below in the else block.
@achow101 suggested in #16944 (review) to avoid knapsack when there’s no change. I added a commit that achieves that, but it’s non-trivial.
It does this by enabling BnB when subtracting fees from outputs. Gotchas:
subtractFeesFromOutputs is generally used to spend either all coins or a selected bunch. But there is an edge case where e.g. it can choose between two 0.01 BTC inputs, one SegWit and one legacy, and it won’t care.I tested #16944 on top of this, in particular: GUI coin selection still works with a watch-only wallet without keypool.
@achow101 suggested in #16944 (comment) to avoid knapsack when there’s no change. I added a commit that achieves that, but it’s non-trivial.
In the interest of reducing complexity, I think that change should be done as a separate PR. I do plan on working on the coin selection logic again soon and clean this up as well.
2548+ scriptChange = GetScriptForDestination(dest);
2549+ } else {
2550+ strFailReason = _("Can't generate a transaction without change. Please call keypoolrefill first.").translated;
2551 }
2552-
2553 scriptChange = GetScriptForDestination(dest);
GetScriptForDestination(dest) is called twice here if we can reserve a dest. I would optimize this slightly:
0 if (!reservedest.GetReservedDestination(change_type, dest, true)) {
1 strFailReason = _("Can't generate a transaction without change. Please call keypoolrefill first.").translated;
2 }
3 scriptChange = GetScriptForDestination(dest);
The suggestion should replace L2537 - 2542. Seems I can not edit that part of my suggestion.
2545- strFailReason = "Keypool ran out, please call keypoolrefill first";
2546- return false;
2547+ if (reservedest.GetReservedDestination(change_type, dest, true)) {
2548+ scriptChange = GetScriptForDestination(dest);
2549+ } else {
2550+ strFailReason = _("Can't generate a transaction without change. Please call keypoolrefill first.").translated;
Transaction needs a change address, but we can't generate it. Please call keypoolrefill first.
Code review ACK 388b153f9ea7ae9cc47e928e08d4f244ee624571
Two nits that would be worthwhile changing IMO.
2516@@ -2517,6 +2517,7 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
2517 // TODO: pass in scriptChange instead of reservedest so
2518 // change transaction isn't always pay-to-bitcoin-address
2519 CScript scriptChange;
2520+ assert(scriptChange.empty());
scriptChange = GetScriptForDestination(dest);
2638- bool ret = reservedest.GetReservedDestination(dest, true);
2639- if (!ret)
2640- {
2641- strFailReason = _("Keypool ran out, please call keypoolrefill first").translated;
2642+ const OutputType change_type = TransactionChangeType(coin_control.m_change_type ? *coin_control.m_change_type : m_default_change_type, vecSend);
2643+ ReserveDestination reservedest(this, change_type);
2637- strFailReason = _("Can't generate a change-address key. Please call keypoolrefill first.").translated;
2638- return false;
2639+ strFailReason = _("Transaction needs a change address, but we can't generate it. Please call keypoolrefill first.").translated;
2640 }
2641 scriptChange = GetScriptForDestination(dest);
2642+ assert(!dest.empty() || scriptChange.empty());
It’s not clear to me what this assert is checking for.
If dest is not empty, we got a change destination and so scriptChange is not empty. If dest is empty, then scriptChange will be empty. So this just covers all cases and can’t fail?
GetScriptForDestination() returns an empty script for an empty destination. Perhaps that’s overkill; I can drop it if there’s any other changes needed in this PR.
