Add 'sendall' RPC née sweep

Xekyo commented at 10:20 pm on January 20, 2022: member

Add sendall RPC née sweep

Motivation Currently, the wallet uses a fSubtractFeeAmount (SFFO) flag on the recipients objects for all forms of sending calls. According to the commit discussion, this flag was chiefly introduced to permit sweeping without manually calculating the fees of transactions. However, the flag leads to unintuitive behavior and makes it more complicated to test many wallet RPCs exhaustively. We proposed to introduce a dedicated sendall RPC with the intention to cover this functionality.

Since the proposal, it was discovered in further discussion that our proposed sendall rpc and SFFO have subtly different scopes of operation. • sendall: Use given UTXOs to pay a destination the remainder after fees. • SFFO: Use a given budget to pay an address the remainder after fees.

While sendall will simplify cases of spending a given set of UTXOs such as paying the value from one or more specific UTXOs, emptying a wallet, or burning dust, we realized that there are some cases in which SFFO is used to pay other parties from a limited budget, which can often lead to the creation of change outputs. This cannot be easily replicated using sendall as it would require manual computation of the appropriate change amount.

As such, sendall cannot replace all uses of SFFO, but it still has a different use case and will aid in simplifying some wallet calls and numerous wallet tests.

Sendall call details The proposed sendall call builds a transaction from a specific subset of the wallet’s UTXO pool (by default all of them) and assigns the funds to one or more receivers. Receivers can either be specified with a given amount or receive an equal share of the remaining unassigned funds. At least one recipient must be provided without assigned amount to collect the remainder. The sendall call will never create change. The call has a send_max option that changes the default behavior of spending all UTXOs (“no UTXO left behind”), to maximizing the output amount of the transaction by skipping uneconomic UTXOs. The send_max option is incompatible with providing a specific set of inputs.

Edit: Replaced OP with latest commit message to reflect my updated motivation of the proposal.

Xekyo force-pushed on Jan 20, 2022

Xekyo marked this as ready for review on Jan 20, 2022

Xekyo force-pushed on Jan 20, 2022

JeremyRubin commented at 10:42 pm on January 20, 2022: contributor

Interesting idea. Can this be exposed as a testing only RPC? Or do real users need it?

There are privacy implications of using sweep as well – if it’s just to be used for e.g. taking an old potentially compromised wallet and migrating to a new one is it better to do a sweep that splits into a number of normal-ish looking txns?

jamesob commented at 10:43 pm on January 20, 2022: member

Cool, concept ACK

Tests look nice

Xekyo commented at 10:50 pm on January 20, 2022: member

Subtract fee from amount has historically been used for sweeping and there exist some issues on this repo that illustrate that there is usage.

You could achieve a “normal” looking transaction by specifying two recipients and setting an amount on one of them. There are examples to illustrate this usage in the RPC help text.

Xekyo force-pushed on Jan 20, 2022

DrahtBot added the label RPC/REST/ZMQ on Jan 20, 2022

DrahtBot added the label Wallet on Jan 20, 2022

DrahtBot commented at 7:54 am on January 21, 2022: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22751 (rpc/wallet: add simulaterawtransaction RPC by kallewoof)
#21576 (rpc, gui: bumpfee signer support by Sjors)
#21283 (Implement BIP 370 PSBTv2 by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Successahead approved

brunoerg commented at 2:16 pm on January 21, 2022: member

Concept ACK

glozow commented at 3:15 pm on January 21, 2022: member

Concept ACK, I prefer this to #23534.

w0xlt commented at 9:58 am on January 22, 2022: contributor

Concept ACK

in src/wallet/rpc/spend.cpp:1188 in 669c52287d outdated

1183+                        "       \"" + FeeModes("\"\n\"") + "\""},
1184+            {"fee_rate", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"not set, fall back to wallet fee estimation"}, "Specify a fee rate in " + CURRENCY_ATOM + "/vB."},
1185+            {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "",
1186+                {
1187+                    {"add_to_wallet", RPCArg::Type::BOOL, RPCArg::Default{true}, "When false, returns a serialized transaction which will not be added to the wallet or broadcast"},
1188+                    {"fee_rate", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"not set, fall back to wallet fee estimation"}, "Specify a fee rate in " + CURRENCY_ATOM + "/vB."},

shaavan commented at 1:23 pm on January 22, 2022:

The fee_rate is defined twice:

As an argument
As an option object

I presume this is an intentional design choice. But I can’t seem to grasp the reason behind doing so.

Xekyo commented at 5:01 pm on January 25, 2022:

This is consistent with how all other transaction creating RPCs permit setting the fee_rate, not offering both options may surprise some of the users.

shaavan commented at 11:39 am on January 28, 2022:

This is consistent with how all other transaction creating RPCs permit setting the fee_rate.

I checked with other RPCs where "fee_rate", RPCArg::Type::AMOUNT,... is used. It seems like the situation you are describing is true only for the send RPC (see https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpc/spend.cpp#L1050-L1062) and isn’t a norm per se.

Other RPCs that uses fee_rate but doesn’t define it twice are:

sendtoaddress
sendmany
fundrawtransaction
bumpfee_helper, and,
walletcreatefundedpsbt

Xekyo commented at 0:34 am on February 10, 2022:

Okay, I’ll take another look at that. Thank you.

Xekyo commented at 7:06 pm on February 17, 2022:

After talking to a few people, my sentiment is that this is the expected behavior. In the minimal use case, the most important arguments are available as positional, making it easy to use. When people start using options, they may want to define everything as options, though, for consistency and readability.

in src/wallet/rpc/spend.cpp:1261 in 669c52287d outdated

1232+            if (!pwallet) return NullUniValue;
1233+
1234+            UniValue options{request.params[4].isNull() ? UniValue::VOBJ : request.params[4]};
1235+            if (options.exists("conf_target") || options.exists("estimate_mode")) {
1236+                if (!request.params[1].isNull() || !request.params[2].isNull()) {
1237+                    throw JSONRPCError(RPC_INVALID_PARAMETER, "Pass conf_target and estimate_mode either as arguments or in the options object, but not both");

shaavan commented at 1:26 pm on January 22, 2022:

Two Questions:

If I pass the conf_target argument and estimate_mode under the options object, this will cause an error. But should there be an error in such a scenario?
conf_target and estimate_mode can be options objects, but they are not explicitly defined in the RPC help message under the options column as is done with the fee_rate object. Is it a deliberate decision?

Xekyo commented at 5:23 pm on January 25, 2022:

Good catch, thanks. I’ll make sure that the conf_target and estimate_mode appear in the documentation of the options.

Regarding the error when conf_target and estimate_mode are passed in different ways, this is consistent with what send does.

shaavan commented at 11:55 am on January 28, 2022:

Regarding the error when conf_target and estimate_mode are passed in different ways, this is consistent with what send does.

I checked the code, and it seems like you are right. I can also see that the sweep RPC is modeled after the send RPC. However, I am not sure if it is an optimal behavior. I can’t understand why the user is compelled to input both of these options as either options or as arguments. Why not allow them to pass the first conf_target in one way and estimate_mode in another. Similar to what is done with fee_rate.

in src/wallet/rpc/spend.cpp:1265 in 669c52287d outdated

1171+                "Optionally some receivers can be specified with an amount, but at least one address must appear without a specified amount.\n",
1172+                {
1173+                    {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "A bitcoin address which receives an equal share of the unspecified amount."},
1174+                    {"", RPCArg::Type::OBJ_USER_KEYS, RPCArg::Optional::OMITTED, "",
1175+                        {
1176+                            {"address", RPCArg::Type::AMOUNT, RPCArg::Optional::NO, "A key-value pair. The key (string) is the bitcoin address, the value (float or string) is the amount in " + CURRENCY_UNIT + ""},

shaavan commented at 1:29 pm on January 22, 2022:

nit: I think it’s better to replace key-value -> key:value, because that’s how the pair is originally expressed in arguments.

Xekyo commented at 5:24 pm on January 25, 2022:

“key-value” is the prevalent way of describing this sort of datum.

shaavan commented at 11:29 am on January 28, 2022:

“key-value” is the prevalent way of describing this sort of datum.

I had checked with various RPC commands, and I think it is correct.

in test/functional/wallet_sweep.py:107 in 669c52287d outdated

102+        assert_raises_rpc_error(-6, "Dynamically assigned remainder results in dust output.", self.wallet.sweep, [{self.return_addr_with_amount: wallet_balance_before_sweep - fee}, self.return_addr_remainder])
103+        assert_raises_rpc_error(-6, "Dynamically assigned remainder results in dust output.", self.wallet.sweep, [{self.return_addr_with_amount: wallet_balance_before_sweep - fee - Decimal(0.00000010)}, self.return_addr_remainder])
104+
105+        self.clean_up()
106+
107+    def sweep_negative_effective_value(self):

shaavan commented at 1:30 pm on January 22, 2022:

I think you can add a self.log.info(..) statement here.

Xekyo commented at 5:28 pm on January 25, 2022:

Done

in test/functional/wallet_sweep.py:121 in 669c52287d outdated

115+
116+        assert_raises_rpc_error(-6, "Total value of UTXO pool too low to pay for sweep. Try using lower feerate or excluding uneconomic UTXOs with 'sendmax' option.", dust_wallet.sweep, receivers=[self.return_addr_remainder], fee_rate=300)
117+
118+        dust_wallet.unloadwallet()
119+
120+    def sweep_with_sendmax(self):

shaavan commented at 1:31 pm on January 22, 2022:

Here also, a self.log.info(..) statement can be added.

Xekyo commented at 5:28 pm on January 25, 2022:

Done

in test/functional/wallet_sweep.py:130 in 669c52287d outdated

122+        self.def_wallet.sendtoaddress(self.wallet.getnewaddress(), 0.00000300)
123+        self.def_wallet.sendtoaddress(self.wallet.getnewaddress(), 1)
124+        self.generate(self.nodes[0], 1)
125+        assert_greater_than(self.wallet.getbalances()["mine"]["trusted"], 0)
126+
127+        sweep_tx_receipt = self.wallet.sweep(receivers=[self.return_addr_remainder], fee_rate=300, options={"sendmax": True})

shaavan commented at 1:34 pm on January 22, 2022:

Though it’s not very difficult grasping the meaning of each section, I think it would be better if you added a comment:

0sweep with sendmax enabled.

Before line 127. And statement:

0sweep with sendmax disabled.

Before line 138.

Xekyo commented at 5:30 pm on January 25, 2022:

I’ve added comments as suggested.

shaavan commented at 1:37 pm on January 22, 2022: contributor

Concept ACK

The added sweep() RPC function looks good. I went through the code and couldn’t find any breaking points. The same goes for the added test, which is logically sound. Kudos for this amazing work, @Xekyo! :clinking_glasses:

Here’s a screenshot of the displayed message when passing the command:

0./src/bitcoin-cli -signet sweep

Screenshot:

The added test passed successfully, which intends that the added function works correctly. I shall do a manual test by creating a new wallet and sweeping it in some time. Meanwhile, I have some doubts and suggestions that might help make this PR even better.

in src/wallet/rpc/spend.cpp:1166 in 669c52287d outdated

1159@@ -1159,6 +1160,267 @@ RPCHelpMan send()
1160     };
1161 }
1162 
1163+RPCHelpMan sweep()
1164+{
1165+    return RPCHelpMan{"sweep",
1166+        "\nEXPERIMENTAL warning: this call may be changed in future releases.\n"

MarcoFalke commented at 1:44 pm on January 22, 2022:

0        "EXPERIMENTAL warning: this call may be changed in future releases.\n"

nit: no need for newline

Xekyo commented at 5:52 pm on January 25, 2022:

Removed the newline. (I was copying the style of send which has one.)

in src/wallet/rpc/spend.cpp:1407 in 669c52287d outdated

1402+                ssTx << psbtx;
1403+                result.pushKV("psbt", EncodeBase64(ssTx.str()));
1404+            }
1405+
1406+            if (complete) {
1407+                std::string err_string;

brunoerg commented at 10:08 pm on January 22, 2022:

Is there a place where this string is used? I could see only this declaration.

Xekyo commented at 5:52 pm on January 25, 2022:

Thanks. I also removed it from send where it also wasn’t used.

in src/wallet/rpc/spend.cpp:1308 in 669c52287d outdated

1279+            if (options.exists("lock_unspents")) {
1280+                lock_unspents = options["lock_unspents"].get_bool();
1281+            }
1282+
1283+            bool rbf = pwallet->m_signal_rbf;
1284+            if (options.exists("replaceable")) {

achow101 commented at 8:42 pm on January 24, 2022:

replaceable is not documented

Xekyo commented at 5:52 pm on January 25, 2022:

Fixed, thanks.

in src/wallet/rpc/spend.cpp:1312 in 669c52287d outdated

1307+            for (const COutput& output : all_the_utxos) {
1308+                CHECK_NONFATAL(output.nInputBytes > 0);
1309+                if (sendmax && fee_rate.GetFee(output.nInputBytes) > output.tx->tx->vout[output.i].nValue) {
1310+                    continue;
1311+                }
1312+                CTxIn input(output.tx->GetHash(), output.i);

achow101 commented at 8:43 pm on January 24, 2022:

Needs to handle rbf and set the right sequence number

0                CTxIn input(output.tx->GetHash(), output.i, CScript(), rbf ? MAX_BIP125_RBF_SEQUENCE : CTxIn::SEQUENCE_FINAL);

Xekyo commented at 5:52 pm on January 25, 2022:

Added, thanks.

achow101 commented at 10:28 pm on January 24, 2022: member

The GUI will need access to this functionality too. It would be nice if the sweeping logic was refactored into a function in src/wallet/spend.cpp so that the GUI can access it.

Edit: Actually that can wait until we figure out what the GUI is going to do for sweep.

Xekyo force-pushed on Jan 25, 2022

glozow added the label Review club on Jan 26, 2022

Sjors commented at 12:01 pm on January 28, 2022: member

I have no objection to a dedicated sweep RPC call, but I don’t agree with the premise that subtracting fee from output is only useful for sweeping a full wallet. See #24142 (comment)

ryanofsky commented at 3:42 pm on January 28, 2022: member

I have no objection to a dedicated sweep RPC call, but I don’t agree with the premise that subtracting fee from output is only useful for sweeping a full wallet. See #24142 (comment)

Either approach seems fine to me, but I do think it is potentially more confusing to have two completely different RPCs for sending funds: “send” vs" sweep" than to have one sending option that means “Send approximate amount. I don’t care about exact amount of BTC received, and am happy if it’s a little less or a little more to economize on fees and avoid change.” Obviously you shouldn’t use this option if you are trying to send an exact amount to someone, but it seems like it would be generally useful whenever you are sending money to one of your own wallets, or exchange accounts, or paying for any service that can be incrementally topped up.

I know one of achow101’s recent PR was adding more complexity to subtract from output implementation to try to do something to help sweeping, but I don’t think the original (current?) semantics inherently had to add much complexity. It is true the code has been complex at different points but I think that was mostly a result of code shittiness and duplication, which have generally improved recently.

Sjors commented at 4:19 pm on January 28, 2022: member

Indeed I suspect that a sweep RPC will be useless if we decide to keep ‘subtracting fee from output’ functionality. In that case adding a sweep feature to the send RPC would make more sense. C-lightling has a special case amount all for that purpose.

w0xlt commented at 4:59 pm on January 28, 2022: contributor

Concept ACK

I have no objections to this RPC either, but something like bitcoin-cli -named sendtoaddress address="...." sweep=true might be simpler for users instead of two commands to send funds.

achow101 commented at 5:09 pm on January 28, 2022: member

I have no objection to a dedicated sweep RPC call, but I don’t agree with the premise that subtracting fee from output is only useful for sweeping a full wallet. See #24142 (comment)

That use case is what I call generalized sweep. Sweep can be viewed as spending all UTXOs in a given list without needing to specify the output amount. Whether that list is all the UTXOs in the wallet or a specific list of some UTXOs doesn’t matter. So sweep can be extended to cover your use case by allowing for inputs to be specified.

but it seems like it would be generally useful whenever you are sending money to one of your own wallets, or exchange accounts, or paying for any service that can be incrementally topped up.

Can you describe why SFFO would be used in those cases? @Xekyo and I have had discussions with many people about SFFO use cases and even though the concept of “send no more than X” usually comes up, no one can express why that behavior would ever actually be useful. Just looking through many of the issues about SFFO show that its primary use case is to sweep an entire wallet, and sometimes spend just a preset list of coins without having to calculate the fee manually. Both of these cases can be covered by a sweep function without needing to maintain SFFO in coin selection logic.

I know one of achow101’s recent PR was adding more complexity to subtract from output implementation to try to do something to help sweeping, but I don’t think the original (current?) semantics inherently had to add much complexity. It is true the code has been complex at different points but I think that was mostly a result of code shittiness and duplication, which have generally improved recently.

I find that SFFO makes it much harder to reason about our coin selection because we now use effective values for selection, but with SFFO, we don’t actually want the effective values.

luke-jr commented at 8:32 am on January 29, 2022: member

Another SFFO use case is when your recipient is legitimately taking on the cost of business. For example, it can be used when sending a loan.

ryanofsky commented at 2:16 pm on January 29, 2022: member

but it seems like it would be generally useful whenever you are sending money to one of your own wallets, or exchange accounts, or paying for any service that can be incrementally topped up.

Can you describe why SFFO would be used in those cases? @Xekyo and I have had discussions with many people about SFFO use cases and even though the concept of “send no more than X” usually comes up, no one can express why that behavior would ever actually be useful.

I think, mostly, it is useful conceptually as a way to simplify creating transactions. It’s a way of expressing the intention behind a transaction, and letting the implementation take care of the details, instead of being having to think about what the transaction will look like, and choose between sweep and send APIs to create it.

Also the concept here is not exactly “spend no more than X.” If there are two amounts associated with transaction: X is amount spent by sender, Y is amount received by receiver, then option lets you choose between fixing X and letting Y vary, or fixing Y and letting X vary. If you have 12 BTC, and want to budget exactly 1 BTC per month to spend on a service, the option let you do that. And in general, it lets you just say what your intention is instead of having to think at a lower level.

Additionally, there is my main practical concern:

This is duplicating existing CreateTransaction(Internal) logic instead of just calling CreateTransaction with the right options. The claim is that this will be a code simplification, but the followup PR #24142 is +588/−148 lines. And this is while REMOVING features which I think we agree are useful: providing GUI support for sweeping, and being able to select coins to sweep. The new code and tests are going to grow and become even more complex after adding these features.

My medium level practical concern:

Deprecating subtract from output in #24142 presumably is going to break existing workflows, and there doesn’t seem to be a good release notes or FAQ style item saying what that the problem is with subtract from output, why its removal is justified, and how to transition to the sweep API.

And my mini practical concern:

This sweep API might be less safe than send APIs because it doesn’t force you to specify amount you are trying to send. It’s easier to fat-finger by accidentally sweeping the wrong wallet (or when manual coin selection is added) sweeping the wrong coin and spending an amount larger than you intended. Maybe this is not a very big concern. If it is a concern, it could also be addresed by adding a mandatory amount argument requiring you to specify total amount of the sweep or an override value like "unchecked" or "yolo"

ryanofsky commented at 2:46 pm on January 29, 2022: member

I find that SFFO makes it much harder to reason about our coin selection because we now use effective values for selection, but with SFFO, we don’t actually want the effective values.

Could you explain this more? There is a m_subtract_fee_outputs ? m_value : effective_value line but that hardly seems terrible

EDIT: I see there is a counterpart to this in CreateTransactionInternal where the option is used to set tx_noinput_size. This seems pretty straightforward as well. Is this the extent of the problem or is there more under the surface?

Sjors commented at 3:00 pm on January 29, 2022: member

This discussion is getting a little mixed between here and #24142 (comment). I don’t need automatic coin selection for the use case I describe above. I do still need the feature to work for manual coin selection. Having to use a separate “sweep” RPC for that seems a bit odd, especially when it needs to support most of the send kitchen sink, like using hardware wallets, specifying the fee rate, RBF, etc. And it still has to work in the GUI send screen, which touches much of the same CWallet code.

achow101 commented at 9:02 pm on February 1, 2022: member

I find that SFFO makes it much harder to reason about our coin selection because we now use effective values for selection, but with SFFO, we don’t actually want the effective values.

Could you explain this more? There is a m_subtract_fee_outputs ? m_value : effective_value line but that hardly seems terrible

EDIT: I see there is a counterpart to this in CreateTransactionInternal where the option is used to set tx_noinput_size. This seems pretty straightforward as well. Is this the extent of the problem or is there more under the surface?

It makes the reasoning harder because the new coin selection code is almost entirely predicated on using effective values, particularly positive effective values. For BnB and SRD, we assume there are no negative ev inputs. For Knapsack and SRD, we assume we will make change and require a minimum change value. SFFO violates these assumptions, and that leads to bugs like #23026 (which happens to be a sweep use case).

and choose between sweep and send APIs to create it. … This sweep API might be less safe than send APIs because it doesn’t force you to specify amount you are trying to send. It’s easier to fat-finger by accidentally sweeping the wrong wallet (or when manual coin selection is added) sweeping the wrong coin and spending an amount larger than you intended. Maybe this is not a very big concern. If it is a concern, it could also be addresed by adding a mandatory amount argument requiring you to specify total amount of the sweep or an override value like "unchecked" or "yolo"

There was some discussion about combining this with send where there would be some option for sweeping and some interpretation of magic values, but we felt that would end up being too clunky to use correctly so it was abandoned.

This is duplicating existing CreateTransaction(Internal) logic instead of just calling CreateTransaction with the right options. The claim is that this will be a code simplification, but the followup PR [Deprecate SubtractFeeFromOutputs #24142](https://github.com/bitcoin/bitcoin/pull/24142) is +588/−148 lines.

The followup only has more additions because it is adding -deprecatedrpc=sffo. It does not remove the behavior yet.

And this is while REMOVING features which I think we agree are useful: providing GUI support for sweeping, and being able to select coins to sweep.

There will be GUI support in the future.

The new code and tests are going to grow and become even more complex after adding these features.

This new code is unlikely to be expanded further given it’s limited scope. However I agree there could be some refactors for deduplication between sweep and CreateTransactionInternal.

Deprecating subtract from output in [Deprecate SubtractFeeFromOutputs #24142](https://github.com/bitcoin/bitcoin/pull/24142) presumably is going to break existing workflows, and there doesn’t seem to be a good release notes or FAQ style item saying what that the problem is with subtract from output, why its removal is justified, and how to transition to the sweep API.

The same could be said about every other time -deprecatedrpc has been used. Needing docs is not a reason to block a deprecation.

Xekyo commented at 10:30 pm on February 3, 2022: member

I think, mostly, it is useful conceptually as a way to simplify creating transactions. It’s a way of expressing the intention behind a transaction, and letting the implementation take care of the details, instead of being having to think about what the transaction will look like, and choose between sweep and send APIs to create it.

After talking to some people in the last week and reading all comments on #24142 and here, I think I have a better understanding of how to distinguish the two domains of use cases between sweep and SFFO:

SFFO: Send to a recipient using a given budget deducting fees
Sweep: Send sum of inputs to recipient(s) deducting fees

We realize now, that SFFO is used more widely than we anticipated, and that the non-sweeping case is not covered by sweep. The problems and bugs occur when people use SFFO to specify the full wallet balance in order to arrive at a sweep functionality. Therefore, I do think that sweep is still useful, especially if we allow specifying an input list smaller than everything. We will work on that and GUI support.

This is duplicating existing CreateTransaction(Internal) logic instead of just calling CreateTransaction with the right options. The claim is that this will be a code simplification, but the followup PR #24142 is +588/−148 lines. And this is while REMOVING features which I think we agree are useful: providing GUI support for sweeping, and being able to select coins to sweep. The new code and tests are going to grow and become even more complex after adding these features.

We looked into this, but found that we need to know the feerate to determine which inputs are eligible, need the inputs to be able to determine the receiver amounts, and when we have all the parameters to call CreateTransaction(…), we already have used all of the duplicated code.

Xekyo force-pushed on Feb 4, 2022

Xekyo commented at 0:42 am on February 4, 2022: member

Added the inputs option to sweep in 2664d3a.

Sjors commented at 9:30 am on February 4, 2022: member

I’m still a bit worried about the amount of duplicated code here, also in the RPC parameter parsing.

Can you add some test coverage for external signer use? The easiest way is probably to tweak wallet_signer.py such that the send example results in exactly the same coin selection as the new sweep example. In that case you can just duplicate the hww.send line and compare res[hex] between the two calls.

If you end up with two separate transactions because the RPC calls can’t produce identical results, then it’s useful to know the mechanism that test uses: it has one wallet with private keys and a watch-only clone. A PSBT is generated on the watch-only wallet. The private key enabled wallet is then used to sign it, with the result stored in the file system. Once the test node calls mocks/signer.py it reads the stored signed PSBT and returns it, thus faking a hardware wallet that actually signed something. This can of course be done for two separate PSBT’s with some refactoring, but having an identical PSBT to sign is easier.

Xekyo force-pushed on Feb 4, 2022

Xekyo renamed this:
~~Add 'sweep' RPC~~
Add 'sweepwallet' RPC
on Feb 4, 2022

Xekyo commented at 9:54 pm on February 4, 2022: member

Renamed the proposed RPC sweepwallet to clarify that we are operating on the wallet’s UTXO pool rather than a wallet-foreign private key as “sweep” may imply.

Rewrote the commit description. @Sjors: I will look into your feedback to reduce the code duplication and test external signer use.

Xekyo force-pushed on Feb 8, 2022

Xekyo commented at 9:18 pm on February 8, 2022: member

Deduplicated the parameter processing, still working on the test in wallet_signer.py.

Xekyo force-pushed on Feb 8, 2022

Xekyo commented at 9:43 pm on February 8, 2022: member

Now with wallet_signer.py test.

Xekyo force-pushed on Feb 8, 2022

Xekyo commented at 10:31 pm on February 8, 2022: member

Now with Release Notes.

in src/wallet/rpc/spend.cpp:119 in b2b1d6b543 outdated

114+    result.pushKV("complete", complete);
115+
116+    return result;
117+}
118+
119+static void PreventOutdatedOptions(const UniValue& options) {

jonatack commented at 9:13 pm on February 9, 2022:

92b248498 nit, clang-format

0-static void PreventOutdatedOptions(const UniValue& options) {
1+static void PreventOutdatedOptions(const UniValue& options)
2+{

74e3135 idem for ParseFeeEstimationInstructions() 7ba3faa and for FinishTransaction()

Xekyo commented at 10:36 pm on February 10, 2022:

Done.

in src/wallet/rpc/spend.cpp:1162 in b2b1d6b543 outdated

1193-                throw JSONRPCError(RPC_INVALID_PARAMETER, "Use lock_unspents");
1194-            }
1195-            if (options.exists("subtractFeeFromOutputs")) {
1196-                throw JSONRPCError(RPC_INVALID_PARAMETER, "Use subtract_fee_from_outputs");
1197-            }
1198+            ParseFeeEstimationInstructions(/*conf_target*/ request.params[1], /*estimate_mode*/ request.params[2], /*fee_rate*/ request.params[3], options);

jonatack commented at 9:18 pm on February 9, 2022:

74e3135ba0721f62c2bc0cbb9bcab88a34003d67 if you retouch, we’ve been converging on this named arg format for clang-tidy verification

0            ParseFeeEstimationInstructions(/*conf_target=*/request.params[1], /*estimate_mode=*/request.params[2], /*fee_rate=*/request.params[3], options);

b2b1d6b idem, line 1265, line 1290, etc.

Xekyo commented at 10:35 pm on February 10, 2022:

Got em.

in src/wallet/rpc/spend.cpp:76 in b2b1d6b543 outdated

71+    if (!options["conf_target"].isNull() && (options["estimate_mode"].isNull() || (options["estimate_mode"].get_str() == "unset"))) {
72+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Specify estimate_mode");
73+    }
74+}
75+
76+static UniValue FinishTransaction(const std::shared_ptr<CWallet> pwallet, const UniValue& options, const CMutableTransaction& rawTx) {

jonatack commented at 9:43 pm on February 9, 2022:

7ba3faa4 minor style suggestions (initiialize localvar next to first use, const, braced initialization) if you retouch and are so inclined, these are already much improved

 0 static UniValue FinishTransaction(const std::shared_ptr<CWallet> pwallet, const UniValue& options, const CMutableTransaction& rawTx) {
 1-    bool add_to_wallet = options.exists("add_to_wallet") ? options["add_to_wallet"].get_bool() : true;
 2-
 3     // Make a blank psbt
 4     PartiallySignedTransaction psbtx(rawTx);
 5 
 6@@ -94,7 +92,9 @@ static UniValue FinishTransaction(const std::shared_ptr<CWallet> pwallet, const
 7 
 8     UniValue result(UniValue::VOBJ);
 9 
10-    const bool psbt_opt_in = options.exists("psbt") && options["psbt"].get_bool();
11+    const bool psbt_opt_in{options.exists("psbt") && options["psbt"].get_bool()};
12+    const bool add_to_wallet{options.exists("add_to_wallet") ? options["add_to_wallet"].get_bool() : true};
13     if (psbt_opt_in || !complete || !add_to_wallet) {
14         // Serialize the PSBT
15         CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION);
16@@ -1231,7 +1231,7 @@ RPCHelpMan send()
17 
18             CAmount fee;
19             int change_position;
20-            bool rbf = options.exists("replaceable") ? options["replaceable"].get_bool() : pwallet->m_signal_rbf;
21+            const bool rbf{options.exists("replaceable") ? options["replaceable"].get_bool() : pwallet->m_signal_rbf};
22             CMutableTransaction rawTx = ConstructTransaction(options["inputs"], request.params[0], options["locktime"], rbf);
23             CCoinControl coin_control;

Xekyo commented at 10:34 pm on February 10, 2022:

Done

in doc/release-notes-24118.md:1 in b2b1d6b543 outdated

0@@ -0,0 +1,24 @@
1+Add `sweepwallet` RPC

jonatack commented at 10:15 pm on February 9, 2022:

b2b1d6b

I think you can add your release note directly to the file doc/release-notes.md in this section:

0New RPCs
1--------

s/without unspecified/without a specified/
add (#24118) to the end

Xekyo commented at 10:32 pm on February 10, 2022:

Moved and amended as suggested

in src/wallet/rpc/spend.cpp:10 in b2b1d6b543 outdated

 6@@ -7,11 +7,13 @@
 7 #include <policy/policy.h>
 8 #include <rpc/rawtransaction_util.h>
 9 #include <rpc/util.h>
10+#include <util/rbf.h>

jonatack commented at 10:16 pm on February 9, 2022:

b2b1d6b nit, sort

Xekyo commented at 10:28 pm on February 10, 2022:

Done

in src/wallet/rpc/spend.cpp:1188 in b2b1d6b543 outdated

1227+    return RPCHelpMan{"sweepwallet",
1228+        "EXPERIMENTAL warning: this call may be changed in future releases.\n"
1229+        "\nSpend all (or specific) confirmed UTXOs in the wallet to one or more recipients.\n"
1230+        "Unconfirmed inbound UTXOs and locked UTXOs will not be spent. Sweepwallet will respect the wallet flag for avoid_reuse.\n",
1231+        {
1232+            {"receivers", RPCArg::Type::ARR, RPCArg::Optional::NO, "The destinations of the sweep, each address may only appear once.\n"

jonatack commented at 10:20 pm on February 9, 2022:

b2b1d6b use a semi-colon here (";") or a new sentence

0            {"receivers", RPCArg::Type::ARR, RPCArg::Optional::NO, "The sweep destinations. Each address may only appear once.\n"

Xekyo commented at 10:32 pm on February 10, 2022:

Thanks, sounds much better.

in src/wallet/rpc/spend.cpp:1222 in b2b1d6b543 outdated

1261+                            },
1262+                        },
1263+                        {"locktime", RPCArg::Type::NUM, RPCArg::Default{0}, "Raw locktime. Non-0 value also locktime-activates inputs"},
1264+                        {"lock_unspents", RPCArg::Type::BOOL, RPCArg::Default{false}, "Lock selected unspent outputs"},
1265+                        {"psbt", RPCArg::Type::BOOL,  RPCArg::DefaultHint{"automatic"}, "Always return a PSBT, implies add_to_wallet=false."},
1266+                        {"sendmax", RPCArg::Type::BOOL, RPCArg::Default{false}, "When true, only sweep UTXOs that can pay for their own fees to maximize the output amount. When 'false' (default), no UTXO is left behind. Sendmax is incompatible with providing specific inputs."},

jonatack commented at 10:22 pm on February 9, 2022:

b2b1d6b per the developer notes, snake case is used for new RPC arguments: send_max?

Xekyo commented at 10:32 pm on February 10, 2022:

Done

in src/wallet/rpc/spend.cpp:1469 in b2b1d6b543 outdated

1463+                }
1464+            }
1465+
1466+            if (lock_unspents) {
1467+                for (const CTxIn& txin : rawTx.vin) {
1468+                    pwallet->LockCoin(txin.prevout);

jonatack commented at 10:40 pm on February 9, 2022:

b2b1d6b minor suggestions if you retouch and are so inclined

 0@@ -1353,17 +1353,11 @@ RPCHelpMan sweepwallet()
 1             }
 2 
 3             CCoinControl coin_control;
 4-
 5-            SetFeeEstimateMode(*pwallet, coin_control, options["conf_target"], options["estimate_mode"], options["fee_rate"], /* override_min_fee */ false);
 6+            SetFeeEstimateMode(*pwallet, coin_control, options["conf_target"], options["estimate_mode"], options["fee_rate"], /*override_min_fee=*/false);
 7 
 8             coin_control.fAllowWatchOnly = ParseIncludeWatchonly(options["include_watching"], *pwallet);
 9 
10-            bool lock_unspents = false;
11-            if (options.exists("lock_unspents")) {
12-                lock_unspents = options["lock_unspents"].get_bool();
13-            }
14-
15-            bool rbf = options.exists("replaceable") ? options["replaceable"].get_bool() : pwallet->m_signal_rbf;
16+            const bool rbf{options.exists("replaceable") ? options["replaceable"].get_bool() : pwallet->m_signal_rbf};
17 
18             FeeCalculation fee_calc_out;
19             CFeeRate fee_rate = GetMinimumFeeRate(*pwallet, coin_control, &fee_calc_out);
20@@ -1422,7 +1416,7 @@ RPCHelpMan sweepwallet()
21                 }
22             }
23 
24-            CAmount output_amounts_claimed(0);
25+            CAmount output_amounts_claimed{0};
26             for (CTxOut out : rawTx.vout) {
27                 output_amounts_claimed += out.nValue;
28             }
29@@ -1431,12 +1425,12 @@ RPCHelpMan sweepwallet()
30                 throw JSONRPCError(RPC_INVALID_PARAMETER, "Assigned more value to outputs than available funds.");
31             }
32 
33-            CAmount remainder = effective_value - output_amounts_claimed;
34+            const CAmount remainder{effective_value - output_amounts_claimed};
35             if (remainder < 0) {
36                 throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Insufficient funds for fees after creating specified outputs.");
37             }
38 
39-            CAmount per_output_without_amount = remainder / addresses_without_amount.size();
40+            const CAmount per_output_without_amount = remainder / addresses_without_amount.size();
41 
42             bool gave_remaining_to_first = false;
43             for (CTxOut& out : rawTx.vout) {
44@@ -1461,6 +1455,7 @@ RPCHelpMan sweepwallet()
45                 }
46             }
47 
48+            const bool lock_unspents{options.exists("lock_unspents") ? options["lock_unspents"].get_bool() : false};
49             if (lock_unspents) {
50                 for (const CTxIn& txin : rawTx.vin) {
51                     pwallet->LockCoin(txin.prevout);

Xekyo commented at 10:31 pm on February 10, 2022:

I think I got all these.

jonatack commented at 10:46 pm on February 9, 2022: member

WIP review, looks mostly good so far. A few minor comments if you retouch.

Xekyo commented at 10:25 pm on February 10, 2022: member

• Addressed @jonatack’s comments • Added test for duplicate destinations • Added test for sweeping to multiple recipients sharing the remainder • Added @cleanup decorator to tests • Cleaned up extremely long lines in tests

Open questions: • Should this new RPC only allow one method (either options or positional args) to provide fee_rate, feeconf_target and estimate_mode?

Xekyo force-pushed on Feb 10, 2022

ghost commented at 10:47 pm on February 10, 2022: none

Concept ACK

luke-jr commented at 3:49 am on February 11, 2022: member

Hate to nitpick the name, but “sweepwallet” to me sounds too easily confused with sweeping funds into the wallet.

in test/functional/test_runner.py:282 in 27f8fb4c83 outdated

277@@ -278,6 +278,8 @@
278     'wallet_create_tx.py --legacy-wallet',
279     'wallet_send.py --legacy-wallet',
280     'wallet_send.py --descriptors',
281+    'wallet_sweep.py --legacy-wallet',
282+    'wallet_sweep.py --descriptors',

jonatack commented at 10:57 am on February 11, 2022:

Not sure, but maybe rpc_sweepwallet.py would be the most coherent filename if the call is named “sweepwallet”.

Xekyo commented at 6:08 pm on February 14, 2022:

I see that some other wallet related calls do start with rpc_… as well, such as rpc_rawtransaction.py, rpc_fundrawtransaction.py, rpc_signrawtransaction.py, and rpc_psbt.py, but all the other send related calls are under wallet_…. I’m kinda thinking that it fits the pattern as it is currently. It should probably be wallet_sweepwallet.py, though.

Xekyo commented at 9:17 pm on February 14, 2022:

Renamed to wallet_sweepwallet.py.

jonatack commented at 11:12 am on February 11, 2022: member

Looks like test/lint/lint-python.sh needs appeasing

0test/functional/wallet_sweep.py:28:1: E115 expected an indented block (comment)

in doc/release-notes.md:151 in 27f8fb4c83 outdated

146+  or to spend specific UTXOs in full.
147+
148+  The `sweepwallet` RPC therefore provides a less cumbersome way of spending
149+  specific UTXOs or emptying wallets than subtract fee from output/amount (SFFO).
150+  If the user wishes to specify a budget rather than a set of UTXOs to delimit a
151+  transaction, they should continue to use SFFO. (#24118)

jonatack commented at 11:19 am on February 11, 2022:

I wonder if the release note isn’t a bit long and whether the how-to info would be best documented elsewhere more permanently like in the sweepwallet help or in one of the doc/ files.

Xekyo commented at 9:16 pm on February 14, 2022:

I rewrote the release notes.

Xekyo commented at 5:49 pm on February 11, 2022: member

Hate to nitpick the name, but “sweepwallet” to me sounds too easily confused with sweeping funds into the wallet.

I changed it from sweep after multiple people stated that they first thought it was a feature to sweep a private key into the wallet. I’m open to suggestions, if you have an idea for a better name.

in doc/release-notes.md:131 in 27f8fb4c83 outdated

125@@ -126,6 +126,30 @@ Updated RPCs
126 New RPCs
127 --------
128 
129+- Add `sweepwallet` RPC
130+
131+  The `sweepwallet` RPC spends some given UTXOs' complete balance to one or more

luke-jr commented at 6:43 pm on February 11, 2022:

nit: s/balance/value

Xekyo commented at 9:16 pm on February 14, 2022:

Amended.

luke-jr commented at 6:45 pm on February 11, 2022: member

idk, I guess if you don’t mind ignoring the advanced use cases in naming, “emptywallet” might work.

DrahtBot added the label Needs rebase on Feb 14, 2022

flack commented at 3:50 pm on February 14, 2022: contributor

or maybe “drainwallet”. “emptywallet” sounds a lot like empty wallet, i.e. it’s not immediately clear that “empty” is a verb in this case

Xekyo force-pushed on Feb 14, 2022

Xekyo force-pushed on Feb 15, 2022

Xekyo commented at 10:47 pm on February 15, 2022: member

Moved the release notes back to their own file to avoid merge conflict (I think I had misunderstood some feedback I got about release notes).

DrahtBot removed the label Needs rebase on Feb 16, 2022

Xekyo force-pushed on Feb 17, 2022

Xekyo renamed this:
~~Add 'sweepwallet' RPC~~
Add 'sendall' RPC née sweep
on Feb 17, 2022

Xekyo commented at 8:43 pm on February 17, 2022: member

Renamed RPC to sendall, after discussion with other contributors. All review comments should have been addressed. Please let me know if I have missed something.

in src/wallet/rpc/spend.cpp:1312 in 5a663b2a67 outdated

1307+                // eventually allow a fallback fee
1308+                throw JSONRPCError(RPC_WALLET_ERROR, "Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable -fallbackfee.");
1309+            }
1310+
1311+            CMutableTransaction rawTx = ConstructTransaction(options["inputs"], receiver_key_value_pairs, options["locktime"], rbf);
1312+            LOCK(pwallet->cs_wallet); // Lock automatically released at end of function

achow101 commented at 9:30 pm on February 17, 2022:

in 5a663b2a676ed7ec24d5d4b1690d2639aeebab86 “Add sendall RPC née sweep”

nit: The comment on this line is unnecessary.

Xekyo commented at 10:54 pm on February 17, 2022:

Thanks, removed

in src/wallet/rpc/spend.cpp:1331 in 5a663b2a67 outdated

1326+                        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Input not found. UTXO (%s:%d) is not part of wallet.", input.prevout.hash.ToString(), input.prevout.n));
1327+                    }
1328+                    total_input_value += tx->tx->vout[input.prevout.n].nValue;
1329+                }
1330+            } else {
1331+                AvailableCoins(*pwallet, all_the_utxos, &coin_control, /*spend 0-value utxos=*/0);

achow101 commented at 9:31 pm on February 17, 2022:

in 5a663b2a676ed7ec24d5d4b1690d2639aeebab86 “Add sendall RPC née sweep”

nit: the inline comment should have the parameter name

0                AvailableCoins(*pwallet, all_the_utxos, &coin_control, /*nMinimumAmount=*/0);

Xekyo commented at 10:54 pm on February 17, 2022:

Thanks, updated

in test/functional/wallet_sendall.py:220 in 5a663b2a67 outdated

215+        assert_greater_than(self.wallet.getbalances()["mine"]["trusted"], 0)
216+
217+        # Clean up remaining UTXO
218+        self.wallet.sendall(receivers=[self.remainder_target])
219+        self.generate(self.nodes[0], 1)
220+        assert_equal(0, self.wallet.getbalances()["mine"]["trusted"]) # wallet is empty

achow101 commented at 9:41 pm on February 17, 2022:

In 5a663b2a676ed7ec24d5d4b1690d2639aeebab86 “Add sendall RPC née sweep”

This cleanup is no longer necessary with the @cleanup decorator.

Xekyo commented at 10:55 pm on February 17, 2022:

Indeed. I removed the duplicate cleanup. Thanks.

Xekyo force-pushed on Feb 17, 2022

in doc/release-notes-24118.md:9 in 9bda5234bc outdated

0@@ -0,0 +1,10 @@
1+New RPCs
2+--------
3+
4+- The `sendall` RPC spends given UTXOs to one or more receivers
5+  without creating change. By default, the `sendall` RPC will send
6+  everything in the wallet leaving no UTXOs behind. `sendall` is
7+  useful to empty wallets or to create a changeless payment from select
8+  UTXOs. Continue to use the "subtract fee from output" option to create
9+  transactions delimited via a budget (in contrast to a concrete set of

Sjors commented at 9:01 am on February 18, 2022:

We can clean up the docs later, but I assume “delimited via a budget” refers to a scenario where a service pays a users an amount X and the user can decide what fee to pay?

Maybe say:

Continue to use the “subtract fee from output” option in send, sendtoaddress or sendmany if the intention is to allocate a specific amount to a recipient for which they incur the fee.

stickies-v commented at 2:07 pm on February 20, 2022:

I agree with Sjors’ rephrasing - currently one could understand this to mean using SFFO with the sendall RPC. Slight iterative alteration which includes using the actual option name:

If the intention is to allocate a specific amount to a recipient for which the recipient incurs the fee, continue to use the send, sendtoaddress or sendmany methods with subtractfeefromamount option.

Xekyo commented at 9:57 pm on March 9, 2022:

Thanks for the suggestions. I’m changing the sentence to:

When creating a payment from a specific amount for which the recipient incurs the transaction fee, continue to use the subtractfeefromamount option via the send, sendtoaddress, or sendmany RPCs.

stickies-v commented at 2:14 pm on March 16, 2022:

That works for me, although I don’t think “from a specific amount” is grammatically correct, I think that should be “of a specific amount”.

in src/rpc/client.cpp:145 in 9bda5234bc outdated

141@@ -142,6 +142,10 @@ static const CRPCConvertParam vRPCConvertParams[] =
142     { "send", 1, "conf_target" },
143     { "send", 3, "fee_rate"},
144     { "send", 4, "options" },
145+    { "sendall", 0, "receivers" },

Sjors commented at 9:08 am on February 18, 2022:

We use “recipient(s)” in most places (though receiver too). Both are valid, but “recipient” seems better: https://www.quora.com/What-is-the-difference-between-a-receiver-and-a-recipient

Xekyo commented at 9:57 pm on March 9, 2022:

Changing receiver to recipient everywhere.

in src/wallet/rpc/spend.cpp:54 in c4d6cf5fac outdated

50@@ -51,6 +51,28 @@ static void ParseRecipients(const UniValue& address_amounts, const UniValue& sub
51     }
52 }
53 
54+static void ParseFeeEstimationInstructions(const UniValue& positional_conf_target, const UniValue& positional_estimate_mode, const UniValue& positional_fee_rate, UniValue& options)

Sjors commented at 9:43 am on February 18, 2022:

c4d6cf5facf08cf4b1fba512ec30800f650a9c0b: maybe drop positional_

stickies-v commented at 10:33 am on February 28, 2022:

I agree with Sjors, it would also make the hints on the call site align with the (shorter) parameter name.

stickies-v commented at 10:39 am on February 28, 2022:

Additional nit: I’m not sure Parse best captures the function behaviour. Since the function has side effects and I don’t really see any parsing, I think a function name like UpdateFeeEstimationInstructions would better reflect that?

Xekyo commented at 9:57 pm on March 9, 2022:

Dropping the positional_ and renaming function to InterpretFeeEstimationInstructions

in test/functional/wallet_sendall.py:241 in 9bda5234bc outdated

236+
237+        # fails because UTXO is unknown, while other UTXOs exist
238+        foreign_utxo = self.def_wallet.listunspent()[0]
239+        assert_raises_rpc_error(-8, "Input not found. UTXO ({}:{}) is not part of wallet.".format(foreign_utxo["txid"],
240+            foreign_utxo["vout"]), self.wallet.sendall, receivers=[self.remainder_target],
241+            options={"inputs": [foreign_utxo]})

Sjors commented at 10:10 am on February 18, 2022:

Some of the times when I run the test it raises “Total value of UTXO pool too low to pay for transaction.” and then during the handling it said “No exception raised” for this line.

Xekyo commented at 9:57 pm on March 9, 2022:

I have been able to reproduce this, but I haven’t figured out what the issue is, yet.

achow101 commented at 11:44 am on March 14, 2022:

The issue is that foreign_utxo might be the change output for a transaction that funded the test wallet. Because the transaction has something belonging to the test wallet, the test wallet knows the transaction and does not detect it to be foreign. I will add a suggestion at the relevant line to fix this issue.

Xekyo commented at 2:39 pm on March 14, 2022:

Thanks, fixed working from achow101’s suggestion

Sjors commented at 10:13 am on February 18, 2022: member

Review hint for the first three commits: git show --color-moved=dimmed-zebra --color-moved-ws=allow-indentation-change

It would be nice if for a single recipient you can omit the array, like so: sendall bc1q09...hufdl (instead of sendall '["bc1q09...hufdl"]').

The first 3 commits are useful refactors regardless, just in case this PR doesn’t make it (ACK on those, but still need to review the meat and potatoes commit).

In the description of 9bda5234bc41b7477b0224930177c6dfcd8cb9fc maybe replace “given” with “specific”.

Let’s add a quick warning to the help text:

If your wallet contains many small inputs, either because it received tiny payments or as a result of accumulating change, consider using “send_max” to exclude those inputs that are worth less than the fees needed to spend them.

Thoughts that can wait for followups…

Name-wise sendall implies that you can’t send specific UTXO’s. We can think of a better name later, since it’s marked experimental. One suggestion: sendcoins. That said, I doubt anyone uses the RPC to manually perform coin selection. The GUI or some external tool is far easier to use for that. So that leaves the use case of emptying out the wallet, and for that the current name is very accurate.

We could potentially make sending specific coins less tedious on the RPC, but that’s outside the scope of this PR. Ideally you would want a new RPC call that returns a concise list of outpoints with their amount, like so:

016a9...97f:7 0.000100
134fe...e8d:0 0.00200

We could even use transaction short ids, if there’s no duplicate in any given wallet (return the full id if there is).

And then rather than the tedious options dictionary with an object for each input, we’d add one argument which takes a simple array of strings, so you can do sendall bc1q09...hufdl null unset 1.1 '["16a9...97f:7"]' in order to spend the first coin. Still not super pretty, but when copy-pasted and adjusted from the help example, it should be easy enough to use.

To burn dust we could add an option burn that, if zero addresses are provided, will send to OP_RETURN. That way you might sweep economic coins with a slightly higher fee and then make a burn transaction for the rest at 1 sat/vbyte (or 0 if you have a way to get those mined).

in src/wallet/rpc/spend.cpp:1471 in 9bda5234bc outdated

1395+            const bool lock_unspents{options.exists("lock_unspents") ? options["lock_unspents"].get_bool() : false};
1396+            if (lock_unspents) {
1397+                for (const CTxIn& txin : rawTx.vin) {
1398+                    pwallet->LockCoin(txin.prevout);
1399+                }
1400+            }

glozow commented at 3:40 pm on February 18, 2022:

General organization question: Did you consider putting some of this in a CWallet member function? A lot of it, like grabbing available coins and checking the effective values, locking unspents, etc. seem to be things that the wallet should take care of internally, rather than in the rpc handler code.

Sjors commented at 5:28 pm on February 18, 2022:

That seems like a good idea, but maybe too big a refactor for this PR?

glozow commented at 8:11 am on February 22, 2022:

This is all newly added code?

Sjors commented at 8:56 am on February 22, 2022:

Oops yes, this is not part of the refactoring commits.

Xekyo commented at 5:13 pm on March 11, 2022:

That sounds like a good idea. Especially getting subsets of a wallet’s UTXO pool does seem like a good candidate. I’ll consult with my wallet expert on this matter.

achow101 commented at 4:01 pm on March 25, 2022:

I think some things like unspent locking could be refactored further, but the bulk of this PR is new code rather than copied from somewhere else. There isn’t much here that I think can be reused elsewhere.

w.r.t getting a subset of the UTXO pool, it doesn’t make sense for this to be part of AvailableCoins as that function is used for more than transaction creation. Not all situations where ti is used have access to information such as feerates.

in src/wallet/rpc/spend.cpp:1402 in 9bda5234bc outdated

1326+                        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Input not found. UTXO (%s:%d) is not part of wallet.", input.prevout.hash.ToString(), input.prevout.n));
1327+                    }
1328+                    total_input_value += tx->tx->vout[input.prevout.n].nValue;
1329+                }
1330+            } else {
1331+                AvailableCoins(*pwallet, all_the_utxos, &coin_control, /*nMinimumAmount=*/0);

glozow commented at 3:53 pm on February 18, 2022:

Given that AvailableCoins filters much more than based on effective value, I’m wondering what the expected behavior should be for these types of UTXOs:

UTXOs that are not yet final or are immature coinbases
UTXOs for which the address has been marked as already used
outputs from replacement transactions etc.

Maybe it’s fine that these are excluded, but just want to clarify that this behavior is what we want. Also leads me to wonder if the RPC should be returning a list of UTXOs that weren’t included in the constructed transaction or something, but maybe that’s overkill.

Xekyo commented at 5:09 pm on March 11, 2022:

This RPC will only operate on confirmed spendable UTXOs. It also respects the wallet-wide avoid_reuse configuration.

UTXOs that are not yet final or are immature coinbases

I would expect them to get ignored since they’re not spendable at this time.

UTXOs for which the address has been marked as already used

If address reuse has been disabled, it should ignore funds on previously used addresses, if it only permits spending all of the funds on one address at once, it should either use all or none, and if it’s not restricted, it should just spend either all or the economically viable UTXOs as determined by the options on this RPC.

outputs from replacement transactions

We don’t spend unconfirmed UTXOs in this call.

glozow commented at 3:57 pm on February 18, 2022: member

Still reviewing, but posting some questions I had about the expected behavior

in doc/release-notes-24118.md:6 in 9bda5234bc outdated

0@@ -0,0 +1,10 @@
1+New RPCs
2+--------
3+
4+- The `sendall` RPC spends given UTXOs to one or more receivers
5+  without creating change. By default, the `sendall` RPC will send
6+  everything in the wallet leaving no UTXOs behind. `sendall` is

stickies-v commented at 1:56 pm on February 20, 2022:

nit: slightly more concise

0  without creating change. By default, the `sendall` RPC will spend
1  every UTXO in the wallet. `sendall` is

Xekyo commented at 5:09 pm on March 11, 2022:

Adopted your suggestion.

in src/wallet/rpc/spend.cpp:141 in 9bda5234bc outdated

135+    if (options.exists("lockUnspents")) {
136+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Use lock_unspents");
137+    }
138+    if (options.exists("subtractFeeFromOutputs")) {
139+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Use subtract_fee_from_outputs");
140+    }

stickies-v commented at 10:23 am on February 28, 2022:

nyfnit (not-your-fault nit): would now be a good time to align the error messages and either have them all (preferred imo) or none report on the “instead of” like is done for feeRate/fee_rate? Might make more sense in a separate commit to not break the --color-moved heuristics.

Xekyo commented at 5:11 pm on March 11, 2022:

I will look into this

Xekyo commented at 2:39 pm on March 14, 2022:

Amended all the error messages to fully explain the issue as in the fee_rate. Added the change as a separate commit.

in src/wallet/rpc/spend.cpp:1171 in 9bda5234bc outdated

1207             int change_position;
1208-            bool rbf = pwallet->m_signal_rbf;
1209-            if (options.exists("replaceable")) {
1210-                rbf = options["replaceable"].get_bool();
1211-            }
1212+            bool rbf = options.exists("replaceable") ? options["replaceable"].get_bool() : pwallet->m_signal_rbf;

stickies-v commented at 11:30 am on February 28, 2022:

nit: maybe use brace initialization?

0            bool rbf { options.exists("replaceable") ? options["replaceable"].get_bool() : pwallet->m_signal_rbf };

Xekyo commented at 5:11 pm on March 11, 2022:

Done

in src/wallet/rpc/spend.cpp:1274 in 9bda5234bc outdated

1313+            PreventOutdatedOptions(options);
1314+
1315+
1316+            std::set<std::string> addresses_without_amount;
1317+            UniValue receiver_key_value_pairs(UniValue::VARR);
1318+            const UniValue& receivers = request.params[0];

stickies-v commented at 11:31 am on February 28, 2022:

nit: maybe use brace initialization?

0            const UniValue& receivers { request.params[0] };

Xekyo commented at 5:11 pm on March 11, 2022:

Done

in src/wallet/rpc/spend.cpp:107 in 9bda5234bc outdated

102+        ssTx << psbtx;
103+        result.pushKV("psbt", EncodeBase64(ssTx.str()));
104+    }
105+
106+    if (complete) {
107+        std::string hex = EncodeHexTx(CTransaction(mtx));

stickies-v commented at 11:32 am on February 28, 2022:

nit: maybe use brace initialization?

0        std::string hex { EncodeHexTx(CTransaction(mtx)) };

Xekyo commented at 5:11 pm on March 11, 2022:

Done

in src/wallet/rpc/spend.cpp:1264 in 9bda5234bc outdated

1303+                UniValueType(), // fee_rate, will be checked by AmountFromValue() in SetFeeEstimateMode()
1304+                UniValue::VOBJ, // options
1305+                }, true
1306+            );
1307+
1308+            std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);

stickies-v commented at 11:32 am on February 28, 2022:

nit: maybe use brace initialization?

0            std::shared_ptr<CWallet> const pwallet { GetWalletForJSONRPCRequest(request) };

Xekyo commented at 5:11 pm on March 11, 2022:

Done

in src/wallet/rpc/spend.cpp:1276 in 9bda5234bc outdated

1315+
1316+            std::set<std::string> addresses_without_amount;
1317+            UniValue receiver_key_value_pairs(UniValue::VARR);
1318+            const UniValue& receivers = request.params[0];
1319+            for (unsigned int i = 0; i < receivers.size(); ++i) {
1320+                const UniValue& receiver = receivers[i];

stickies-v commented at 11:33 am on February 28, 2022:

nit: maybe use brace initialization?

0                const UniValue& receiver { receivers[i] };

Xekyo commented at 5:13 pm on March 11, 2022:

Done

in src/wallet/rpc/spend.cpp:1300 in 9bda5234bc outdated

1348+            coin_control.fAllowWatchOnly = ParseIncludeWatchonly(options["include_watching"], *pwallet);
1349+
1350+            const bool rbf{options.exists("replaceable") ? options["replaceable"].get_bool() : pwallet->m_signal_rbf};
1351+
1352+            FeeCalculation fee_calc_out;
1353+            CFeeRate fee_rate = GetMinimumFeeRate(*pwallet, coin_control, &fee_calc_out);

stickies-v commented at 11:33 am on February 28, 2022:

nit: maybe use brace initialization?

0            CFeeRate fee_rate { GetMinimumFeeRate(*pwallet, coin_control, &fee_calc_out) };

Xekyo commented at 5:11 pm on March 11, 2022:

Done

in src/wallet/rpc/spend.cpp:1311 in 9bda5234bc outdated

1361+                throw JSONRPCError(RPC_WALLET_ERROR, "Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable -fallbackfee.");
1362             }
1363 
1364-            CMutableTransaction mtx;
1365-            complete = FinalizeAndExtractPSBT(psbtx, mtx);
1366+            CMutableTransaction rawTx = ConstructTransaction(options["inputs"], receiver_key_value_pairs, options["locktime"], rbf);

stickies-v commented at 11:33 am on February 28, 2022:

nit: maybe use brace initialization?

0            CMutableTransaction rawTx { ConstructTransaction(options["inputs"], receiver_key_value_pairs, options["locktime"], rbf) };

Xekyo commented at 5:11 pm on March 11, 2022:

Done

in src/wallet/rpc/spend.cpp:1324 in 9bda5234bc outdated

1374+            } else if (options.exists("inputs")) {
1375+                for (const CTxIn& input : rawTx.vin) {
1376+                    if (pwallet->IsSpent(input.prevout.hash, input.prevout.n)) {
1377+                        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Input not available. UTXO (%s:%d) was already spent.", input.prevout.hash.ToString(), input.prevout.n));
1378+                    }
1379+                    const CWalletTx* tx = pwallet->GetWalletTx(input.prevout.hash);

stickies-v commented at 11:34 am on February 28, 2022:

nit: maybe use list initialization?

0                    const CWalletTx* tx { pwallet->GetWalletTx(input.prevout.hash) };

Xekyo commented at 5:11 pm on March 11, 2022:

Done

in src/wallet/rpc/spend.cpp:1346 in 9bda5234bc outdated

1397 
1398-            UniValue result(UniValue::VOBJ);
1399+            // estimate final size of tx
1400+            TxSize tx_size = CalculateMaximumSignedTxSize(CTransaction(rawTx), pwallet.get());
1401+            CAmount fee_from_size = fee_rate.GetFee(tx_size.vsize);
1402+            CAmount effective_value = total_input_value - fee_from_size;

stickies-v commented at 11:41 am on February 28, 2022:

nit: maybe use brace initialization?

0            TxSize tx_size { CalculateMaximumSignedTxSize(CTransaction(rawTx), pwallet.get()) };
1            CAmount fee_from_size { fee_rate.GetFee(tx_size.vsize) };
2            CAmount effective_value { total_input_value - fee_from_size };

Xekyo commented at 5:11 pm on March 11, 2022:

Done

in src/wallet/rpc/spend.cpp:1370 in 9bda5234bc outdated

1433+            const CAmount remainder{effective_value - output_amounts_claimed};
1434+            if (remainder < 0) {
1435+                throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Insufficient funds for fees after creating specified outputs.");
1436+            }
1437+
1438+            const CAmount per_output_without_amount = remainder / addresses_without_amount.size();

stickies-v commented at 11:42 am on February 28, 2022:

nit: maybe use brace initialization?

0            const CAmount per_output_without_amount { remainder / addresses_without_amount.size() };

Xekyo commented at 5:11 pm on March 11, 2022:

Done

Xekyo commented at 5:53 pm on March 11, 2022:

This one actually doesn’t like being a brace initialization:

0wallet/rpc/spend.cpp: In lambda function:
1wallet/rpc/spend.cpp:1371:63: warning: narrowing conversion of ‘(((long unsigned int)((long int)remainder)) / addresses_without_amount.std::set<std::__cxx11::basic_string<char> >::size())’ from ‘long unsigned int’ to ‘CAmount’ {aka ‘long int’} inside { } [-Wnarrowing]
2             const CAmount per_output_without_amount{remainder / addresses_without_amount.size()};
3                                                     ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Xekyo commented at 7:27 pm on March 11, 2022:

Fixed by casting the size to (long) explicitly.

in src/wallet/rpc/spend.cpp:1372 in 9bda5234bc outdated

1435+                throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Insufficient funds for fees after creating specified outputs.");
1436+            }
1437+
1438+            const CAmount per_output_without_amount = remainder / addresses_without_amount.size();
1439+
1440+            bool gave_remaining_to_first = false;

stickies-v commented at 11:42 am on February 28, 2022:

nit: maybe use brace initialization?

0            bool gave_remaining_to_first { false };

Xekyo commented at 5:13 pm on March 11, 2022:

Done

in src/wallet/rpc/spend.cpp:1376 in 9bda5234bc outdated

1439+
1440+            bool gave_remaining_to_first = false;
1441+            for (CTxOut& out : rawTx.vout) {
1442+                CTxDestination dest;
1443+                ExtractDestination(out.scriptPubKey, dest);
1444+                std::string addr = EncodeDestination(dest);

stickies-v commented at 11:45 am on February 28, 2022:

nit: maybe use brace initialization?

0                std::string addr { EncodeDestination(dest) };

Xekyo commented at 5:11 pm on March 11, 2022:

Done

in src/wallet/rpc/spend.cpp:1189 in 9bda5234bc outdated

1228+RPCHelpMan sendall()
1229+{
1230+    return RPCHelpMan{"sendall",
1231+        "EXPERIMENTAL warning: this call may be changed in future releases.\n"
1232+        "\nSpend the value of all (or specific) confirmed UTXOs in the wallet to one or more recipients.\n"
1233+        "Unconfirmed inbound UTXOs and locked UTXOs will not be spent. Sendall will respect the wallet flag for avoid_reuse.\n",

stickies-v commented at 11:53 am on February 28, 2022:

Wording nit/suggestion:

0        "Unconfirmed inbound UTXOs and locked UTXOs will not be spent. sendall respects the avoid_reuse wallet flag.\n",

Xekyo commented at 5:11 pm on March 11, 2022:

Amended

in src/wallet/rpc/spend.cpp:1212 in 9bda5234bc outdated

1251+                "options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "",
1252+                Cat<std::vector<RPCArg>>(
1253+                    {
1254+                        {"add_to_wallet", RPCArg::Type::BOOL, RPCArg::Default{true}, "When false, returns a serialized transaction which will not be added to the wallet or broadcast"},
1255+                        {"fee_rate", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"not set, fall back to wallet fee estimation"}, "Specify a fee rate in " + CURRENCY_ATOM + "/vB."},
1256+                        {"include_watching", RPCArg::Type::BOOL, RPCArg::DefaultHint{"true for watch-only wallets, otherwise false"}, "Also select inputs which are watch only.\n"

stickies-v commented at 12:00 pm on February 28, 2022:

Consistency nit

0                        {"include_watching", RPCArg::Type::BOOL, RPCArg::DefaultHint{"true for watch-only wallets, otherwise false"}, "Also select inputs which are watch-only.\n"

Xekyo commented at 5:12 pm on March 11, 2022:

Thanks

in src/wallet/rpc/spend.cpp:1210 in 9bda5234bc outdated

1249+            {"fee_rate", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"not set, fall back to wallet fee estimation"}, "Specify a fee rate in " + CURRENCY_ATOM + "/vB."},
1250+            {
1251+                "options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "",
1252+                Cat<std::vector<RPCArg>>(
1253+                    {
1254+                        {"add_to_wallet", RPCArg::Type::BOOL, RPCArg::Default{true}, "When false, returns a serialized transaction which will not be added to the wallet or broadcast"},

stickies-v commented at 12:00 pm on February 28, 2022:

Wording nit/suggestion:

0                        {"add_to_wallet", RPCArg::Type::BOOL, RPCArg::Default{true}, "When false, returns the serialized transaction without broadcasting or adding it to the wallet"},

Xekyo commented at 5:12 pm on March 11, 2022:

That’s better, thanks

stickies-v commented at 12:23 pm on February 28, 2022: member

Approach ACK 9bda523

A couple of nits and wording suggestions, all of them optional. I support the sendall name, I think it’s neutral and well describes what’s happening.

Xekyo commented at 8:51 pm on March 9, 2022: member

@Sjors: Thanks for the thoughtful suggestions. I’ve adopted your suggestions for phrasing and discussed whether overloading the receivers/recipients array with a single string with some other developers. While I agree that it would ease the (probably) most likely use case of sweeping everything into a single address, some people didn’t like the idea of allowing multiple types for one parameter. @glozow, @stickies-v: Thank you for the review, I’m working on answers to each of your comments.

Xekyo commented at 10:00 pm on March 9, 2022: member

Sorry if this is confusing, but I’ve started responding to comments, but haven’t finished making all the code changes and rebasing yet (gotta switch devices and didn’t want to lose what I’ve already written).

Xekyo force-pushed on Mar 11, 2022

Xekyo commented at 5:20 pm on March 11, 2022: member

Addressed most comments by Sjors, Gloria and stickies-v.

Still looking into/open for feedback on:

@sjors’s suggestion of overloading recipients parameter to take a single string in the case of a single recipient instead of an array
The inconsistent bug @Sjors noticed in the sendall_fails_on_missing_input test
@glozow’s suggestion of moving parts of the UTXO filtering into CWallet
@stickies-v’s suggestion to align the error messages

I’ve also added another testcase that was authored by @ishaanam.

Xekyo force-pushed on Mar 11, 2022

in test/functional/wallet_signer.py:197 in 5c0a1b3302 outdated

191@@ -192,6 +192,12 @@ def test_valid_signer(self):
192         assert(res["complete"])
193         assert_equal(res["hex"], mock_tx)
194 
195+        self.log.info('Test sendall using hww1')
196+
197+        res = hww.sendall(receivers=[{dest:0.5}, hww.getrawchangeaddress()],options={"add_to_wallet": False})

achow101 commented at 11:01 am on March 14, 2022:

In 5c0a1b330294d40112af79af19414b913d0dce8c “Add sendall RPC née sweep”

0        res = hww.sendall(recipients=[{dest:0.5}, hww.getrawchangeaddress()],options={"add_to_wallet": False})

Xekyo commented at 2:39 pm on March 14, 2022:

Fixed, thank you

in test/functional/wallet_sendall.py:53 in 5c0a1b3302 outdated

48+    def assert_tx_has_outputs(self, tx, expected_outputs):
49+        assert_equal(len(expected_outputs), len(tx["decoded"]["vout"]))
50+        for eo in expected_outputs:
51+            self.assert_tx_has_output(tx, eo["address"], eo["value"])
52+
53+    def add_uxtos(self, amounts):

achow101 commented at 11:24 am on March 14, 2022:

In 5c0a1b330294d40112af79af19414b913d0dce8c “Add sendall RPC née sweep”

0    def add_utxos(self, amounts):

Xekyo commented at 2:39 pm on March 14, 2022:

Fixed everywhere, thanks

in src/wallet/rpc/spend.cpp:1326 in 5c0a1b3302 outdated

1321+                for (const CTxIn& input : rawTx.vin) {
1322+                    if (pwallet->IsSpent(input.prevout.hash, input.prevout.n)) {
1323+                        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Input not available. UTXO (%s:%d) was already spent.", input.prevout.hash.ToString(), input.prevout.n));
1324+                    }
1325+                    const CWalletTx* tx{pwallet->GetWalletTx(input.prevout.hash)};
1326+                    if (!tx) {

achow101 commented at 11:51 am on March 14, 2022:

We may accidentally allow an external UTXO here if the transaction had both outputs that belonged to us, and outputs that did not. So we should check that the specified output itself actually belongs to this wallet.

0                    if (!tx || pwallet->IsMine(tx->tx->vout[input.prevout.n]) != ISMINE_SPENDABLE | (coin_control.fAllowWatchOnly ? ISMINE_WATCHONLY : 0)) {

(Did not test it, but something similar to that should work. The output needs to be in the wallet, and we should also respect coin_control.fAllowWatchonly)

Fixes https://github.com/bitcoin/bitcoin/pull/24118/commits/5c0a1b330294d40112af79af19414b913d0dce8c#r809858573

Xekyo commented at 2:39 pm on March 14, 2022:

Thanks! I’ve used

if (!tx || pwallet->IsMine(tx->tx->vout[input.prevout.n]) != (coin_control.fAllowWatchOnly ? ISMINE_ALL : ISMINE_SPENDABLE))

Xekyo force-pushed on Mar 14, 2022

Xekyo commented at 2:41 pm on March 14, 2022: member

Addressed review comments from @achow101, fixed the bug that @Sjors discovered, enacted the suggestion of @stickies-v to update the remaining error messages for outdated options to be more helpful.

Still open for comments on:

@sjors’s suggestion of overloading recipients parameter to take a single string in the case of a single recipient instead of an array
@glozow’s suggestion of moving parts of the UTXO filtering into CWallet

Xekyo force-pushed on Mar 16, 2022

Xekyo commented at 1:29 am on March 16, 2022: member

Added a second test provided by @ishaanam. Thanks!

in src/wallet/rpc/spend.cpp:1311 in cab7a4bf86 outdated

1306+                    {RPCResult::Type::STR, "psbt", /*optional=*/true, "If more signatures are needed, or if add_to_wallet is false, the base64-encoded (partially) signed transaction"}
1307+                }
1308+        },
1309+        RPCExamples{""
1310+        "\nSpend all UTXOs from the wallet with a fee rate of 1 " + CURRENCY_ATOM + "/vB using named arguments\n"
1311+        + HelpExampleCli("-named sendall", "recipients='{\"" + EXAMPLE_ADDRESS[0] + "\"}' fee_rate=1\n") +

mzumsande commented at 2:04 pm on March 16, 2022:

In the first help example, use […] instead of {..} for a valid json array.

Xekyo commented at 4:17 pm on March 16, 2022:

Thank you, fixed

Xekyo force-pushed on Mar 16, 2022

achow101 commented at 7:18 pm on March 23, 2022: member

ACK 08be21737e1b38a8171ca216c18a7bbf94511c49

w0xlt approved

w0xlt commented at 8:38 pm on March 23, 2022: contributor

re-ACK 08be217

MarcoFalke added the label Needs rebase on Mar 25, 2022

DrahtBot removed the label Needs rebase on Mar 25, 2022

achow101 commented at 2:49 pm on March 25, 2022: member

Needs rebase?

Yes, there’s a silent merge conflict.

Extract prevention of outdated option names

This will be reused in `sendall` so we extract it to avoid
duplication.

35ed094e4b

Elaborate error messages for outdated options a31d75e5fb

Extract interpretation of fee estimation arguments

This will be reused in `sendall`, so we extract a method to prevent
duplication.

6d2208a3f6

Extract FinishTransaction from send()

The final step of send either produces a PSBT or the final transaction.
We extract these steps to a new helper function `FinishTransaction()` to
reuse them in `sendall`.

902793c777

Xekyo force-pushed on Mar 25, 2022

Xekyo commented at 3:45 pm on March 25, 2022: member

Rebased to master due to silent merge conflict.

achow101 commented at 4:11 pm on March 25, 2022: member

re-ACK 78aab643af1c1afbaf82e1269b55a617f7fda6f6

Changes since last were just the rebase to resolving the silent merge conflict with renaming variables.

w0xlt approved

w0xlt commented at 2:44 am on March 26, 2022: contributor

re-ACK 78aab64

achow101 commented at 2:28 pm on March 28, 2022: member

@sjors @stickies-v @glozow re-review this?

in src/wallet/rpc/spend.cpp:24 in 35ed094e4b outdated

20@@ -21,7 +21,8 @@
21 
22 
23 namespace wallet {
24-static void ParseRecipients(const UniValue& address_amounts, const UniValue& subtract_fee_outputs, std::vector<CRecipient> &recipients) {
25+static void ParseRecipients(const UniValue& address_amounts, const UniValue& subtract_fee_outputs, std::vector<CRecipient> &recipients)

glozow commented at 5:18 pm on March 28, 2022:

nit in 35ed094e4b0e0554e609709f6ca1f7d17096882c since you’re touching

0static void ParseRecipients(const UniValue& address_amounts, const UniValue& subtract_fee_outputs, std::vector<CRecipient>& recipients)

Xekyo commented at 8:41 pm on March 29, 2022:

Fixed

in src/wallet/rpc/spend.cpp:111 in 902793c777 outdated

106+    if (complete) {
107+        std::string hex{EncodeHexTx(CTransaction(mtx))};
108+        CTransactionRef tx(MakeTransactionRef(std::move(mtx)));
109+        result.pushKV("txid", tx->GetHash().GetHex());
110+        if (add_to_wallet && !psbt_opt_in) {
111+            pwallet->CommitTransaction(tx, {}, {} /* orderForm */);

glozow commented at 5:32 pm on March 28, 2022:

nit in 902793c7772e5bdd5aae5b0d20a32c02a1a6dc7c (for clang-tidy, see https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c-named-arguments)

0            pwallet->CommitTransaction(tx, {}, /*orderForm=*/ {});

Xekyo commented at 8:41 pm on March 29, 2022:

Fixed

in src/wallet/rpc/spend.cpp:453 in 3813ba1b19 outdated

449@@ -449,31 +450,43 @@ RPCHelpMan settxfee()
450 
451 
452 // Only includes key documentation where the key is snake_case in all RPC methods. MixedCase keys can be added later.
453-static std::vector<RPCArg> FundTxDoc()
454+static std::vector<RPCArg> FundTxDoc(bool solving_data = true)

glozow commented at 5:57 pm on March 28, 2022:

nit in 3813ba1b196f8dd89de1f8b959c179dd28c05d4e: would prefer to not have default values

Xekyo commented at 8:37 pm on March 29, 2022:

Using this default value in this instance replicates the previous behavior.

in src/wallet/rpc/spend.cpp:1414 in 78aab643af outdated

1424 
1425-            UniValue result(UniValue::VOBJ);
1426+            // estimate final size of tx
1427+            TxSize tx_size{CalculateMaximumSignedTxSize(CTransaction(rawTx), pwallet.get())};
1428+            CAmount fee_from_size{fee_rate.GetFee(tx_size.vsize)};
1429+            CAmount effective_value{total_input_value - fee_from_size};

glozow commented at 6:11 pm on March 28, 2022:

nit in 3813ba1b196f8dd89de1f8b959c179dd28c05d4e

0            const TxSize tx_size{CalculateMaximumSignedTxSize(CTransaction(rawTx), pwallet.get())};
1            const CAmount fee_from_size{fee_rate.GetFee(tx_size.vsize)};
2            const CAmount effective_value{total_input_value - fee_from_size};

in src/wallet/rpc/spend.cpp:1384 in 78aab643af outdated

1393+            CMutableTransaction rawTx{ConstructTransaction(options["inputs"], recipient_key_value_pairs, options["locktime"], rbf)};
1394+            LOCK(pwallet->cs_wallet);
1395+            std::vector<COutput> all_the_utxos;
1396+
1397+            CAmount total_input_value(0);
1398+            bool send_max{options.exists("send_max") && options["send_max"].get_bool()};

glozow commented at 6:12 pm on March 28, 2022:

nit in 3813ba1b196f8dd89de1f8b959c179dd28c05d4e

0            const bool send_max{options.exists("send_max") && options["send_max"].get_bool()};

MarcoFalke commented at 6:05 am on March 29, 2022:

Generally I’d prefer to keep code logic and constants (defaults) separate. Otherwise changing the constant (default) requires changing the code logic. So this should be bool a{option.exists() ? option.bool() : option_default};

Xekyo commented at 8:39 pm on March 29, 2022:

Changed this line to match @MarcoFalke’s suggestion.

in src/wallet/rpc/spend.cpp:1439 in 78aab643af outdated

1458+            }
1459+
1460+            const CAmount remainder{effective_value - output_amounts_claimed};
1461+            if (remainder < 0) {
1462+                throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Insufficient funds for fees after creating specified outputs.");
1463+            }

glozow commented at 6:19 pm on March 28, 2022:

What makes one of these RPC_INVALID_PARAMETER and the other RPC_WALLET_INSUFFICIENT_FUNDS ?

Xekyo commented at 8:39 pm on March 29, 2022:

Changed to RPC_WALLET_INSUFFICIENT_FUNDS

in src/wallet/rpc/spend.cpp:1337 in 78aab643af outdated

1332+                }, true
1333+            );
1334+
1335+            std::shared_ptr<CWallet> const pwallet{GetWalletForJSONRPCRequest(request)};
1336+            if (!pwallet) return NullUniValue;
1337+

glozow commented at 6:31 pm on March 28, 2022:

Do you need to pwallet->BlockUntilSyncedToCurrentChain(); here?

Xekyo commented at 4:08 pm on March 29, 2022:

Yes! Thanks.

glozow commented at 6:34 pm on March 28, 2022: member

code review ACK 78aab643af1c1afbaf82e1269b55a617f7fda6f6, a few non-blocking comments/questions

Add sendall RPC née sweep

_Motivation_
Currently, the wallet uses a fSubtractFeeAmount (SFFO) flag on the
recipients objects for all forms of sending calls. According to the
commit discussion, this flag was chiefly introduced to permit sweeping
without manually calculating the fees of transactions. However, the flag
leads to unintuitive behavior and makes it more complicated to test
many wallet RPCs exhaustively. We proposed to introduce a dedicated
`sendall` RPC with the intention to cover this functionality.

Since the proposal, it was discovered in further discussion that our
proposed `sendall` rpc and SFFO have subtly different scopes of
operation.
• sendall:
  Use _specific UTXOs_ to pay a destination the remainder after fees.
• SFFO:
  Use a _specific budget_ to pay an address the remainder after fees.

While `sendall` will simplify cases of spending from specific UTXOs,
emptying a wallet, or burning dust, we realized that there are some
cases in which SFFO is used to pay other parties from a limited budget,
which can often lead to the creation of change outputs. This cannot be
easily replicated using `sendall` as it would require manual computation
of the appropriate change amount.

As such, sendall cannot replace all uses of SFFO, but it still has a
different use case and will aid in simplifying some wallet calls and
numerous wallet tests.

_Sendall call details_
The proposed sendall call builds a transaction from a specific subset of
the wallet's UTXO pool (by default all of them) and assigns the funds to
one or more receivers. Receivers can either be specified with a specific
amount or receive an equal share of the remaining unassigned funds. At
least one recipient must be provided without assigned amount to collect
the remainder. The `sendall` call will never create change. The call has
a `send_max` option that changes the default behavior of spending all
UTXOs ("no UTXO left behind"), to maximizing the output amount of the
transaction by skipping uneconomic UTXOs. The `send_max` option is
incompatible with providing a specific set of inputs.

49090ec402

add tests for no recipient and using send_max while inputs are specified bb84b7145b

Xekyo force-pushed on Mar 29, 2022

Xekyo commented at 8:40 pm on March 29, 2022: member

Addressed all of the comments by @glozow and @MarcoFalke

achow101 commented at 10:02 pm on March 29, 2022: member

re-ACK bb84b7145b31dbfdcb4cf0b9b6e612a57e573993

fanquake requested review from glozow on Mar 30, 2022

fanquake requested review from stickies-v on Mar 30, 2022

fanquake requested review from Sjors on Mar 30, 2022

MarcoFalke commented at 1:04 pm on March 30, 2022: member

This had 3 ACKs and I checked the range-diff. Did not review myself.

MarcoFalke merged this on Mar 30, 2022

MarcoFalke closed this on Mar 30, 2022

Sjors commented at 1:22 pm on March 30, 2022: member

Sorry I didn’t get around to reviewing this. I’ll see if I can test it later.

This remains on my wish list (for a future PR):

a new RPC call that returns a concise list of outpoints with their amount, like so:
016a9...97f:7 0.000100
134fe...e8d:0 0.00200
We could even use transaction short ids, if there’s no duplicate in any given wallet (return the full id if there is).

MarcoFalke commented at 1:54 pm on March 30, 2022: member

Probably wouldn’t hurt to also return metadata like depth etc..

sidhujag referenced this in commit 59fd959efc on Apr 3, 2022

luke-jr referenced this in commit a956b22295 on May 21, 2022

luke-jr referenced this in commit 96c63418db on May 21, 2022

luke-jr referenced this in commit 231924d911 on May 21, 2022

luke-jr referenced this in commit e2deb9b3d6 on May 21, 2022

luke-jr referenced this in commit 60ec3336fe on May 21, 2022

luke-jr referenced this in commit f025826fe0 on May 21, 2022

achow101 referenced this in commit a56876e6b9 on Sep 15, 2022

sidhujag referenced this in commit e91fcc5403 on Sep 15, 2022

DrahtBot locked this on Mar 30, 2023

Add ‘sendall’ RPC née sweep #24118

Conflicts