mimblewimble / grin-wallet

Grin Wallet
Apache License 2.0
182 stars 133 forks source link

Ability to "send all" or "send max" with auto-calculation of fees #601

Closed trevyn closed 2 years ago

trevyn commented 3 years ago

Feature request by @nthrow in #582. Something like:

grin-wallet send -e -m all
cliik commented 2 years ago

I don't think the -e or -m options should be necessary for this feature.

I propose the following:

I volunteer to implement this, if there's agreement on the feature request.

phyro commented 2 years ago

Correct me if I'm wrong as I've never used this, but it seems we already have an option from this argument https://github.com/mimblewimble/grin-wallet/blob/master/src/bin/grin-wallet.yml#L120-L126

cliik commented 2 years ago

Oh, perhaps I misunderstood that option. I thought -s all selected all outputs for a transaction, but did not necessarily compute the max amount you could spend less fees. i.e. you can send a transaction with -s all to someone, but you still need to specify the amount to send (vs receive back as change).

I think what @trevyn is requesting is a bit different. He is requesting a way to send the maximum amount to a recipient, accounting for fees. This is very similar to the -s all option, except there is no change output, and the wallet must calculate the transaction fees for the user.

I may be wrong though. Documentation for the -s all feature is slim, and I also have not used it myself.

phyro commented 2 years ago

Thanks for clarifying, I think I now see what I missed. I wrongfully assumed the intent was to use the opportunity to sweep all outputs into a change output, but if you want to send max to someone else you have to specify the amount in which case you'd have to set the amount to max_balance - fees(n_inputs, n_outputs, n_kernels). The user can't easily know the fee calculation. If we introduced all for the amount, we should be careful how we do that. If I recall correctly, the SendTxArgs is the CLI argument specification while we have InitTxArgs for the API. The question would be if this is a CLI option or also an option on the API. The former would mean you parse SendTxArgs.amount and compute the actual amount if all and do the api call on the computed amount. The latter would be a breaking change for the API. There's also something yeastplume mentioned to me the other day on keybase regarding transactions where you have no change outputs:

like they need to scan kernels to check if the transaction completed, as opposed to waiting for a change output. So perhaps always having a change output would be cleaner, if we had a sweep strategy

I do wonder if introducing an option where the send command does not explicitly set the exact amount is a good thing though.

cc: @yeastplume

cliik commented 2 years ago

@yeastplume do you know why transaction confirmation is determined by tracking change outputs instead of kernels? If either approach would work, but tracking kernels would make sweeps more efficient, I'd propose the idea of using kernels instead of change outputs for all transaction types.

because It's much more expensive to look up kernels on a node than outputs. So we've generally used chance outputs to track transaction completion unless there aren't any.

phyro commented 2 years ago

Let me clarify the "always have a change output" part. What I had in mind is that every party in a transaction receives an output. If I do a self-spend (sweep) of 10 coins, I'll only create a single output holding 10-fee. Thought this makes me wonder if we should make two (also one with 0-value) to make self-spends indistinguishable from other transactions. I think making transactions look alike would be a good goal to have. It's one of the reasons I'm in favor of payjoining by default in case we can conclude that the outcome isn't a much worse privacy due to another "change output chain".

cliik commented 2 years ago

I think making transactions look alike would be a good goal to have

I strongly agree with this goal. Transaction uniformity is important for strong privacy.

Initially I was in favor of coming up with a single output solution, to prevent leaving behind unused outputs in and cluttering the UTXO set. But, as I think about it more, I realize this will be an infrequent occurrence: a user is unlikely to sweep more than once, and many/most users will never sweep at all. So the cost of the extra 0-value output in a sweep is minimal, IMO.

This, combined with the simplicity / ease of implementation, and the transaction uniformity benefit, lead me to think creating a 0-value change output is the best way to implement this feature. I will take a look into the implementation.

cliik commented 2 years ago

FYI, I think this may be all that is necessary: https://github.com/mimblewimble/grin-wallet/pull/653

Still need to think through edge cases and test, but I figured I'd share a draft PR in case anyone wanted to give early feedback to the approach I am taking.

cliik commented 2 years ago

FYI, I think this may be all that is necessary: https://github.com/mimblewimble/grin-wallet/pull/653

This was a failed attempt. After considering it further, I do not think 'hacking' the amount value in SendArgs is the correct approach for a few reasons.

Instead, I'm considering modifing init_send_tx() API to have an amount_includes_fees option. This allows creating a transaction that debits an exact amount from the sender's wallet (as opposed to crediting an exact amount to the recipients's wallet). Similar to letting the recipient pay the fees. This enables easily sweeping a wallet for both the API and the CLI.

The CLI will still need an extra step to parse the special 'max' value and set the appropriate option when calling init_send_tx(). This part is similar to what I attempted in #653.

For the API to sweep the full wallet balance:

For CLI to sweep the full wallet balance:

Additionally, CLI wallet will have a new --amount_includes_fees option, for generic use of this feature.

The advantages to this approach are:

Ok, I've probably over-thought this by now... but what I had before was under-thought :shrug: I'll get started on this implementation now, but I welcome any feedback.

cliik commented 2 years ago

I'm not sure if --amount_includes_fees is the most ergonomic name for this option. If anyone has any better idea, please share. Maybe --exact_debit instead?

phyro commented 2 years ago

I agree this is a better approach. I think it may even keep the API backwards compatible if we make the field optional and select the right default value. Regarding the name, I think the one you picked is quite descriptive.

P.S. it's usually the practice to have the command arg use dashes rather than underscores i.e. --amount-includes-fees

cliik commented 2 years ago

P.S. it's usually the practice to have the command arg use dashes rather than underscores i.e. --amount-includes-fees

This is usually my preference too, but I tried to be consistent with the rest of the grin-wallet options, which seem to use underscores. e.g. --show_spent, --api_server_address, --top_level_dir, etc

cliik commented 2 years ago

Second attempt at this feature: https://github.com/mimblewimble/grin-wallet/pull/657