wallet: allow transaction without change if keypool is empty by Sjors · Pull Request #17219 · bitcoin/bitcoin

Sjors · 2019-10-22T19:12:19Z

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.

src/wallet/wallet.cpp

instagibbs · 2019-10-22T19:16:31Z

appreciated, will review

DrahtBot · 2019-10-22T22:14:47Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

promag · 2019-10-23T10:18:48Z

Looks like you could avoid failed_to_get_change_address by applying

--- 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

laanwj · 2019-10-23T10:22:16Z

Concept ACK

Sjors · 2019-10-23T13:21:20Z

Updated with @promag's suggestion to avoid failed_to_get_change_address and @instagibbs suggestion to simplify the change address check.

src/wallet/wallet.cpp

Sjors · 2019-10-24T15:04:03Z

@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:

when coins are pre-selected, it still uses knapsack
it still excludes dust inputs based on their value minus the fees needed to spend them, but it does not use their effective value in BnB. I don't think this matters in practice, because 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 · 2019-10-24T20:23:10Z

@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.

Sjors · 2019-10-25T08:32:15Z

@achow101 done, I moved that commit into #17246

fjahr

Code review ACK 388b153

Two nits that would be worthwhile changing IMO.

src/wallet/wallet.cpp

fjahr · 2019-10-30T13:40:32Z

src/wallet/wallet.cpp

+                if (reservedest.GetReservedDestination(change_type, dest, true)) {
+                    scriptChange = GetScriptForDestination(dest);
+                } else {
+                    strFailReason = _("Can't generate a transaction without change. Please call keypoolrefill first.").translated;


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!".

Changed to: Transaction needs a change address, but we can't generate it. Please call keypoolrefill first.

Sjors · 2020-01-30T14:23:00Z

@instagibbs @achow101 @fjahr @luke-jr this should be ready for another review. @gwillen you may also find it useful.

fjahr · 2020-02-02T18:15:08Z

src/wallet/wallet.cpp

I think L2634-2635 can be removed again. They are already declared in L2578-2579.

Odd, removed again.

fjahr · 2020-02-05T10:49:57Z

Code review ACK 92bcd70

meshcollider

Code review ACK 92bcd70

jonasschnelli · 2020-04-16T06:19:53Z

Thanks @Sjors. Clever way.
utACK 92bcd70

jonasschnelli · 2020-04-16T06:21:01Z

This should have its release notes part (added label).

achow101 · 2020-04-16T16:18:17Z

src/wallet/wallet.cpp

+                    strFailReason = _("Transaction needs a change address, but we can't generate it. Please call keypoolrefill first.").translated;
                }
                scriptChange = GetScriptForDestination(dest);
+                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?

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.

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.

achow101 · 2020-04-17T16:34:47Z

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

fanquake · 2020-05-23T07:30:12Z

@achow101 can you add any relevant release notes to https://github.com/bitcoin-core/bitcoin-devwiki/wiki/0.20.0-Release-Notes-Draft ?

Sjors · 2020-05-27T09:28:33Z

@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

Sjors mentioned this pull request Oct 22, 2019

gui: create PSBT with watch-only wallet #16944

Merged

fanquake added the Wallet label Oct 22, 2019

Sjors commented Oct 22, 2019

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

Sjors force-pushed the 2019/10/change-without-keypool branch from b2da2d9 to b22f425 Compare October 23, 2019 13:21

Sjors force-pushed the 2019/10/change-without-keypool branch 2 times, most recently from bbef48b to 3deb73c Compare October 23, 2019 13:22

promag reviewed Oct 23, 2019

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

Sjors force-pushed the 2019/10/change-without-keypool branch 2 times, most recently from 6ecead7 to db2e400 Compare October 23, 2019 14:35

achow101 reviewed Oct 23, 2019

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

Sjors force-pushed the 2019/10/change-without-keypool branch 3 times, most recently from 62b4b6b to 02a153a Compare October 24, 2019 15:03

Sjors force-pushed the 2019/10/change-without-keypool branch 3 times, most recently from 370c382 to 6c73d97 Compare October 24, 2019 18:48

Sjors mentioned this pull request Oct 25, 2019

wallet: avoid knapsack when there's no change #17246

Closed

Sjors force-pushed the 2019/10/change-without-keypool branch from 6c73d97 to bf234b2 Compare October 25, 2019 08:31

DrahtBot added the Needs rebase label Oct 29, 2019

Sjors force-pushed the 2019/10/change-without-keypool branch from bf234b2 to 388b153 Compare October 29, 2019 15:37

DrahtBot removed the Needs rebase label Oct 29, 2019

fjahr approved these changes Oct 30, 2019

View reviewed changes

Sjors force-pushed the 2019/10/change-without-keypool branch from 1b2a650 to 007512d Compare January 9, 2020 05:26

DrahtBot removed the Needs rebase label Jan 9, 2020

Sjors force-pushed the 2019/10/change-without-keypool branch from 007512d to 570a38e Compare January 16, 2020 14:42

Sjors force-pushed the 2019/10/change-without-keypool branch from 570a38e to e593111 Compare January 30, 2020 13:47

fjahr reviewed Feb 2, 2020

View reviewed changes

Sjors added 3 commits February 4, 2020 11:18

[wallet] translate "Keypool ran out" message

5efc25f

[wallet] CreateTransaction: simplify change address check

709f868

[wallet] allow transaction without change if keypool is empty

92bcd70

Sjors force-pushed the 2019/10/change-without-keypool branch from e593111 to 92bcd70 Compare February 4, 2020 10:20

DrahtBot mentioned this pull request Mar 26, 2020

Use shared pointers only in validation interface #18354

Closed

Sjors mentioned this pull request Mar 28, 2020

Native Descriptor Wallets using DescriptorScriptPubKeyMan #16528

Merged

meshcollider reviewed Apr 16, 2020

View reviewed changes

jonasschnelli added the Needs release note label Apr 16, 2020

achow101 reviewed Apr 16, 2020

View reviewed changes

meshcollider merged commit bbb1ba1 into bitcoin:master Apr 18, 2020

Sjors deleted the 2019/10/change-without-keypool branch April 18, 2020 12:38

fanquake mentioned this pull request May 3, 2020

wallet: Fix typo in assert that is compile-time true #18853

Merged

maflcko removed the Needs release note label May 26, 2020

random-zebra mentioned this pull request Jun 4, 2020

[Wallet][RPC] FundTransaction - fundrawtransaction PIVX-Project/PIVX#1663

Merged

1 task

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Conversation

Sjors commented Oct 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

instagibbs commented Oct 22, 2019

Uh oh!

DrahtBot commented Oct 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag commented Oct 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Oct 23, 2019

Uh oh!

Sjors commented Oct 23, 2019

Uh oh!

Uh oh!

Uh oh!

Sjors commented Oct 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Oct 24, 2019

Uh oh!

Sjors commented Oct 25, 2019

Uh oh!

fjahr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fjahr Oct 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

Sjors commented Jan 30, 2020

Uh oh!

fjahr Feb 2, 2020

Choose a reason for hiding this comment

Uh oh!

Sjors Feb 4, 2020

Choose a reason for hiding this comment

Uh oh!

fjahr commented Feb 5, 2020

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Apr 16, 2020

Uh oh!

jonasschnelli commented Apr 16, 2020

Uh oh!

achow101 Apr 16, 2020

Choose a reason for hiding this comment

Uh oh!

Sjors Apr 17, 2020

Choose a reason for hiding this comment

Uh oh!

achow101 Apr 17, 2020

Choose a reason for hiding this comment

Uh oh!

achow101 commented Apr 17, 2020

Uh oh!

fanquake commented May 23, 2020

Uh oh!

Sjors commented May 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

Sjors commented Oct 22, 2019 •

edited

Loading

DrahtBot commented Oct 22, 2019 •

edited

Loading

promag commented Oct 23, 2019 •

edited

Loading

Sjors commented Oct 24, 2019 •

edited

Loading

fjahr Oct 30, 2019 •

edited

Loading