-
Notifications
You must be signed in to change notification settings - Fork 36.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
wallet: allow transaction without change if keypool is empty #17219
wallet: allow transaction without change if keypool is empty #17219
Conversation
appreciated, will review |
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
Looks like you could avoid --- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -2968,7 +2968,7 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
// TODO: pass in scriptChange instead of reservedest so
// change transaction isn't always pay-to-bitcoin-address
CScript scriptChange;
- bool failed_to_get_change_address{false};
+ assert(scriptChange.empty());
// coin control: send change to custom address
if (!boost::get<CNoDestination>(&coin_control.destChange)) {
@@ -2984,7 +2984,6 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
// Reserve a new key pair from key pool. If it fails, provide a dummy
// destination in case we don't need change.
if (!CanGetAddresses(true)) {
- failed_to_get_change_address = true;
strFailReason = _("Can't generate a change-address key. No keys in the internal keypool and can't generate any keys.").translated;
} else {
CTxDestination dest;
@@ -2993,7 +2992,6 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
if (ret) {
scriptChange = GetScriptForDestination(dest);
} else {
- failed_to_get_change_address = true;
strFailReason = _("Keypool ran out, please call keypoolrefill first").translated;
}
@@ -3210,10 +3208,9 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
}
// Give up if change keypool ran out and we failed to find a solution without change:
- if (failed_to_get_change_address && nChangePosInOut != -1) {
+ if (scriptChange.empty() && nChangePosInOut != -1) {
return false;
}
-
}
// Shuffle selected coins and fill in final vin |
Concept ACK |
b2da2d9
to
b22f425
Compare
Updated with @promag's suggestion to avoid |
bbef48b
to
3deb73c
Compare
6ecead7
to
db2e400
Compare
62b4b6b
to
02a153a
Compare
@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. It does this by enabling BnB when subtracting fees from outputs. Gotchas:
I tested #16944 on top of this, in particular: GUI coin selection still works with a watch-only wallet without keypool. |
370c382
to
6c73d97
Compare
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. |
6c73d97
to
bf234b2
Compare
bf234b2
to
388b153
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK 388b153
Two nits that would be worthwhile changing IMO.
src/wallet/wallet.cpp
Outdated
if (reservedest.GetReservedDestination(change_type, dest, true)) { | ||
scriptChange = GetScriptForDestination(dest); | ||
} else { | ||
strFailReason = _("Can't generate a transaction without change. Please call keypoolrefill first.").translated; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this error message is ambiguous: It could mean we can't generate a transaction with change without a change address because we can't get the dest. But it could also mean we can not generate a transaction that does not need a change at all. I could see my self thinking here: "What do you mean? I have change in this tx!".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to: Transaction needs a change address, but we can't generate it. Please call keypoolrefill first.
1b2a650
to
007512d
Compare
007512d
to
570a38e
Compare
570a38e
to
e593111
Compare
@instagibbs @achow101 @fjahr @luke-jr this should be ready for another review. @gwillen you may also find it useful. |
src/wallet/wallet.cpp
Outdated
{ | ||
strFailReason = _("Keypool ran out, please call keypoolrefill first").translated; | ||
const OutputType change_type = TransactionChangeType(coin_control.m_change_type ? *coin_control.m_change_type : m_default_change_type, vecSend); | ||
ReserveDestination reservedest(this, change_type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think L2634-2635 can be removed again. They are already declared in L2578-2579.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Odd, removed again.
e593111
to
92bcd70
Compare
Code review ACK 92bcd70 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK 92bcd70
This should have its release notes part (added label). |
} | ||
scriptChange = GetScriptForDestination(dest); | ||
assert(!dest.empty() || scriptChange.empty()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't remember why I wrote this, but I think the idea was to ensure 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's fine, but it just seems like useless code that might be confusing to future readers. I would suggest that it be removed in a followup that touches this code.
ACK 92bcd70 |
…ool is empty 92bcd70 [wallet] allow transaction without change if keypool is empty (Sjors Provoost) 709f868 [wallet] CreateTransaction: simplify change address check (Sjors Provoost) 5efc25f [wallet] translate "Keypool ran out" message (Sjors Provoost) Pull request description: Extracted from bitcoin#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. ACKs for top commit: fjahr: Code review ACK 92bcd70 jonasschnelli: utACK 92bcd70 achow101: ACK 92bcd70 meshcollider: Code review ACK 92bcd70 Tree-SHA512: 07b8c8251f57061c58a85ebf0359be63583c23bac7a2c4cefdc14820c0cdebcc90a2bb218e5ede0db11d1e204cda149e056dfd18614642070b3d56efe2735006
@achow101 can you add any relevant release notes to https://github.com/bitcoin-core/bitcoin-devwiki/wiki/0.20.0-Release-Notes-Draft ? |
@fanquake note that this isn't in 0.20 |
fc81158 [QA] Add test_change_position case to rpc_fundrawtransaction.py (random-zebra) dd35760 [QA] Add test_option_feerate to rpc_fundrawtransaction functional test (random-zebra) 5bca4f4 Add more clear interface for CoinControl.h regarding individual feerate (random-zebra) 169bc3b [RPC] add feerate option to fundrawtransaction (random-zebra) 87dbdf8 [QA] Test new options in rpc_fundrawtransaction functional test (random-zebra) bc9dc67 Add lockUnspents option to fundrawtransaction (random-zebra) a3ac191 Add change options to fundrawtransaction (random-zebra) 0c1f7ba Add strict flag to RPCTypeCheckObj (random-zebra) d655b42 Use CCoinControl selection in CWallet::FundTransaction (random-zebra) 76c8d54 [QA] Test watchonly addrs in fundrawtransaction tests (random-zebra) 134c5d2 Implement watchonly support in fundrawtransaction (random-zebra) 1b153e5 Update importaddress help to push its use to script-only (random-zebra) 7b4eb6d Add importpubkey method to import a watch-only pubkey (random-zebra) 816dabb Add p2sh option to importaddress to import redeemScripts (random-zebra) 60a20a4 Split up importaddress into helper functions (random-zebra) cbffa80 Add logic to track pubkeys as watch-only, not just scripts (random-zebra) 12b38b0 Add have-pubkey distinction to ISMINE flags (random-zebra) fab6556 Exempt unspendable transaction outputs from dust checks (random-zebra) ab407ff [Tests] Fix and enable fundrawtransaction functional tests (random-zebra) bc44ba0 [wallet] allow transaction without change if keypool is empty (random-zebra) a2f8071 [wallet] CreateTransaction: simplify change address check (random-zebra) 761e60e Add fundrawtransaction RPC method (random-zebra) ccb18dd Add FundTransaction method to wallet (random-zebra) 692b827 Add DummySignatureCreator which just creates zeroed sigs (random-zebra) Pull request description: based on top of - [x] #1662 This introduces a new wallet function, `CWallet::FundTransaction()` (and exposes it via RPC with `fundrawtransaction`), to fill a tx containing only vouts (or not enough vins to cover the vouts) with unspent coins from the wallet. `fundrawtransaction` will not modify existing inputs, and will add one change output (if needed) to the outputs. It will not sign the inputs (so can include also watch-only or multi-sig inputs, if enabled). backported from: - bitcoin#6088 - bitcoin#17219 [`*`] - bitcoin#6417 - bitcoin#6444 - bitcoin#6415 - bitcoin#6828 - bitcoin#7296 (only bebe58b) - bitcoin#7506 - bitcoin#7518 - bitcoin#7967 adapting the tests for the (more recent) framework. [`*`] Note: this has been included to be able to call `fundrawtransaction` without the need for an unencrypted wallet (for the change address key) ACKs for top commit: furszy: re ACK fc81158 . Fuzzbawls: ACK fc81158 Tree-SHA512: 10235ce6e672a1cfd4ae2cad9312864c82971f6a4aa1a8ed9489d85156f5c4126c293180a7f1b86b7c65d07caab484e9a6d7a87ebf032bee55adb98d3e08e7b9
Summary: * [wallet] translate "Keypool ran out" message * [wallet] CreateTransaction: simplify change address check * [wallet] allow transaction without change if keypool is empty This is a backport of Core [[bitcoin/bitcoin#17219 | PR17219]] Test Plan: ninja alll check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D8667
Extracted from #16944
First this PR simplifies the check when generating a change address, by dropping
CanGetAddresses
and just lettingreservedest.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.