Subtract fee from amount #4331

cozz · 2014-06-11T23:04:58Z

Adds the automatically-subtract-the-fee-from-the-amount-and-send-whats-left feature to the GUI and rpc (sendtoaddres,sendfrom,sendmany).

The checkbox is only shown for the first recipient. This is to avoid confusion and to make the implementation
as simple as possible: We deduct the fee from the first recipient, if its not enough -> fail
If whats left to send is dust -> fail

There is an edge case to consider: Instead of moving dust change to fees, we raise the change
until no dust and deduct from the recipient.
Example:
Amount: 10000 satoshis
Fee: 300 satoshis
Selected unspent output: 10001 satoshis

Now normally:
Recipient receives: 9700
Fee: 300
Change: 1
To pay: 10000

But 1 satoshi is dust, so instead we do:
Recipient receives: 9155 (9700 - 545)
Fee: 300
Change: 546 (1 + 545)
To pay: 9455

So you end up paying less than 10000 in this edge case. But if we would move dust-change to fees (or recipient) you would pay 10001 which would be totally unexpected if you enter 10000 and say "please just deduct the fee from the 10000 and send whats left".

Without checkbox

With checkbox

Dust edge case

rpc

ghost · 2014-06-12T06:17:59Z

Untested ACK

laanwj · 2014-06-19T08:35:27Z

Such a large change to the wallet needs much more than an untested ack. If you want this merged, help testing.

laanwj · 2014-07-01T08:04:42Z

I'm a tad surprised that this gets no testing. Lots of complaints about this problem with the wallet, and then someone attempts to solve it and the pull is virtually ignored.

ghost · 2014-07-01T12:43:03Z

Needs to be rebased first.

Diapolo · 2014-07-01T13:42:55Z

src/core.h

@@ -187,6 +187,12 @@ class CTxOut
        return (nValue < 3*minRelayTxFee.GetFee(nSize));
    }

+    int64_t getDustThreshold(CFeeRate minRelayTxFee) const


I'd love to see a comment here, what the 148u are and why 3*minRelayTxFee.

Diapolo · 2014-07-01T14:12:23Z

Code looks good apart from my nits. I'm going to test this tomorrow!

cozz · 2014-07-01T18:46:41Z

Rebased and fixed @Diapolo comments (thanks for reviewing!):

IsDust(..) now calls getDustThreshold(..) to remove duplicate code
readded clear(); in sendcoinsentry. The clear() was removed because it also hides the fee-checkbox. Instead I added now another show() in sendcoinsdialog::setModel()

Earlz · 2014-07-08T00:31:11Z

Has there been any further testing or updates done with this?

Earlz · 2014-07-08T00:52:57Z

Also, there appears to be a bug when sending money to yourself (same input as output). In this scenario, it will subtract the fees from the other recipient(the random change address). It seems a bit misleading, but it is a fairly uncommon scenario for any purpose but testing

cozz · 2014-07-08T02:41:18Z

@Earlz Thanks for testing. I just tried this, I can not reproduce the bug you describe. A change address is inserted at a random position in the raw transaction. The fee is subtracted from the first recipient, but Before the change address is inserted. The fee should never be subtracted from the change address.
There should not be any difference, if you send to yourself or not.

Did you look at the confirmation dialog before sending and did it show the correct total value?

As this would be unexpected, would you mind retesting this?

Earlz · 2014-07-08T21:26:24Z

Yea, I'm not able to reproduce this behavior now either. Maybe I didn't use the subtractfeefromtotal option when sending it or some such. Also, unrelated, but I merged this PR into a downstream coin fork and added a command line option to subtract fees by default, as well as splitting fees evenly among all recipients in a manysend. If you are interested in these changes I can rebase them onto bitcoin and submit a patch against this PR.

cozz · 2014-07-23T19:02:36Z

update:
I added now a commit by @Earlz and changed the design to what he suggested, thanks for helping.
So the fee is now subtracted equally from each recipient.

The checkbox moved to the bottom in the UI, I think this is cleaner than what we had before, only
showing the checkbox for the first recipient.

laanwj · 2014-07-24T07:35:40Z

@cozz It looks better, but with multiple recipients now it's not clear from which of the amounts the fee will get subtracted. ~~Maybe: only allow the option in case of one recipient?~~

Edit: hm no, that makes it impossible to send your entire balance to multiple addresses...

sipa · 2014-07-24T09:13:33Z

Or move the checkbox next to each amount field "subtree fee from this amount"?

laanwj · 2014-07-24T09:49:57Z

@sipa What would happen if it's selected for multiple? Or should that be avoided somehow by providing a kind of radio button.

sipa · 2014-07-24T09:58:11Z

EIther prevent it, or use equal split?

cozz · 2014-07-24T13:31:36Z

So you prefer the checkbox on the top. Then simplest may be,
to show a checkbox for each recipient, but if you click one
checkbox of one recipient, all others are selected automatically. Same for uncheck.
So only allow all or nothing.

If we leave it at the bottom we can simply change the description:
"Subtract the fee equally from the amount of each recipient."

laanwj · 2014-07-24T13:41:54Z

But if you always deduct it equally from all amounts, that means you cannot chose from which amount the fee is deducted. If that is a big loss I don't know, maybe no one cares about that.
The idea of adding a checkbox to every entry is to allow the user to choose which one.

N.B.: in the case of equal division, if you send very large amounts as well as very small amounts, be careful that some amounts without the partial fee deducted may be negative or beneath the dust threshold.

cozz · 2014-07-24T13:47:21Z

Ok, I will look into it, choosing individually.

cozz · 2014-07-25T00:02:17Z

update:

Now showing the checkbox for each recipient, letting the user choose individually,
same for sendmany.

I had to make some small design-changes to CWallet::CreateTransaction

introduce vector<CRecipient> instead of vector<std::pair<CScript, int64_t> >
return the change position in int& nChangePosRet. This is, so that the GUI can just reassign the amounts
and does not need to do any recalculations. So the GUI can show exactly what CreateTransaction did.

laanwj · 2014-07-25T07:34:30Z

src/qt/coincontroldialog.cpp

@@ -31,6 +31,7 @@
 using namespace std;
 QList<qint64> CoinControlDialog::payAmounts;
 CCoinControl* CoinControlDialog::coinControl = new CCoinControl();
+bool CoinControlDialog::fSubtractFeeFromAmount = false;


I think it is strange to have these as a static variable, instead of part of the class - but this is an existing problem, it's not something to be solved in this pull.

laanwj · 2014-07-25T07:42:56Z

UI looks good to me now! I still need to take a better look at the code and test around with functionality on regtest.

laanwj · 2014-09-08T08:16:40Z

Works for me.

This needs RPC tests, though (or an update to the current RPC tests so that the new parameters are tested).

BitcoinPullTester · 2014-09-09T01:54:02Z

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4331_b8956a0de3ae9eb8127ce7f5b995512bb53d2a25/ for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

cozz · 2014-09-09T02:12:52Z

Added a quick RPC test.

cozz · 2014-10-03T03:47:34Z

rebased

Diapolo · 2014-11-21T16:23:54Z

@cozz What about this after your smart fee pull git merged?

gmaxwell · 2014-11-21T20:40:27Z

I'd very much like to have a feature to support this.

There is an related change which I'd also like to see: Round UP the payment to avoid change. E.g. If I say pay 1.0 btc, and I have a 1.001 coin, and I'm just paying another account of my own, it's stupid to take 0.001 btc in change. Interface was do you have an thoughts if this pull would obstruct doing that? (I'd always envisioned the round up working by augmenting values, e.g. 1.0+ would do a configurable amount of round up while 1.0+0.1 would round up upto 0.1 more.). Round up and fee from amount are 2/3rds of why I use raw txn for all my transactions currently.

cozz · 2014-11-22T08:26:31Z

@Diapolo If we wanted this for 0.10 it would have been merged earlier, I guess. But yeah, would be nice to see this merged after branch off, especially as the workflow of sending everything minus the fee, is harder to do now because we removed the rounding of fees.

@gmaxwell Would a simple option "Never create change smaller than x" make you happy?
where x is just an absolute value in bitcoins. We could just add the change to the first recipient. I dont think we need that feature per recipient, do we?

morcos · 2014-11-22T15:19:57Z

The logic around this I find most troublesome was introduced in #85. The current incarnation of it in SelectCoinsMinConf causes you to keep selecting more inputs until you have at least .01 BTC more than you need so that your change is never less than .01 (unless you happened to have exactly the right amount). I haven't played around with it to know if this is more of a problem with smartfees and non-rounded fees, but seems possible. I've had a lot of cases where I'm just adding lots of small inputs together and sending them in order to have enough change.

cozz · 2014-11-22T16:38:23Z

@morcos yes, the sub-cent fee rule has been removed recently, this means that #85 could be reverted now.

laanwj · 2015-02-26T13:16:08Z

Rebased as #5831

laanwj added Windows and removed Windows labels Jun 13, 2014

This was referenced Jul 1, 2014

If there is not enough left in the wallet balance to cover for the fee, to ask if what more is needed to cover for the fee to be subtracted from the transfer #2724

Closed

Bitcoin-Qt: Tx fee weirdness #1570

Closed

Diapolo reviewed Jul 1, 2014
View reviewed changes

laanwj mentioned this pull request Jul 4, 2014

Move a check from SendMoneyToDestination to SendMoney. #4463

Merged

laanwj reviewed Jul 25, 2014
View reviewed changes

cozz force-pushed the cozz15 branch from 94e2d7c to b8956a0 Compare September 9, 2014 01:39

cozz added 4 commits October 3, 2014 04:37

[Qt] Code-movement-only: Format confirmation message in sendcoinsdialog

a4b9b74

Subtract fee from amount

74a6b3e

Subtract fee from amount: RPC-tests

f156d37

Subtract fee from amount: CAmount

55e774d

cozz force-pushed the cozz15 branch from b8956a0 to 55e774d Compare October 3, 2014 03:04

laanwj mentioned this pull request Feb 26, 2015

Subtract fee from amount (rebase) #5831

Merged

laanwj closed this Feb 26, 2015

morcos mentioned this pull request Dec 13, 2016

Don't create change at dust limit #9343

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Subtract fee from amount #4331

Subtract fee from amount #4331

cozz commented Jun 11, 2014

ghost commented Jun 12, 2014

laanwj commented Jun 19, 2014

laanwj commented Jul 1, 2014

ghost commented Jul 1, 2014

Diapolo Jul 1, 2014

Diapolo commented Jul 1, 2014

cozz commented Jul 1, 2014

Earlz commented Jul 8, 2014

Earlz commented Jul 8, 2014

cozz commented Jul 8, 2014

Earlz commented Jul 8, 2014

cozz commented Jul 23, 2014

laanwj commented Jul 24, 2014

sipa commented Jul 24, 2014

laanwj commented Jul 24, 2014

sipa commented Jul 24, 2014

cozz commented Jul 24, 2014

laanwj commented Jul 24, 2014

cozz commented Jul 24, 2014

cozz commented Jul 25, 2014

laanwj Jul 25, 2014

laanwj commented Jul 25, 2014

laanwj commented Sep 8, 2014

BitcoinPullTester commented Sep 9, 2014

cozz commented Sep 9, 2014

cozz commented Oct 3, 2014

Diapolo commented Nov 21, 2014

gmaxwell commented Nov 21, 2014

cozz commented Nov 22, 2014

morcos commented Nov 22, 2014

cozz commented Nov 22, 2014

laanwj commented Feb 26, 2015

Subtract fee from amount #4331

Subtract fee from amount #4331

Conversation

cozz commented Jun 11, 2014

ghost commented Jun 12, 2014

laanwj commented Jun 19, 2014

laanwj commented Jul 1, 2014

ghost commented Jul 1, 2014

Diapolo Jul 1, 2014

Choose a reason for hiding this comment

Diapolo commented Jul 1, 2014

cozz commented Jul 1, 2014

Earlz commented Jul 8, 2014

Earlz commented Jul 8, 2014

cozz commented Jul 8, 2014

Earlz commented Jul 8, 2014

cozz commented Jul 23, 2014

laanwj commented Jul 24, 2014

sipa commented Jul 24, 2014

laanwj commented Jul 24, 2014

sipa commented Jul 24, 2014

cozz commented Jul 24, 2014

laanwj commented Jul 24, 2014

cozz commented Jul 24, 2014

cozz commented Jul 25, 2014

laanwj Jul 25, 2014

Choose a reason for hiding this comment

laanwj commented Jul 25, 2014

laanwj commented Sep 8, 2014

BitcoinPullTester commented Sep 9, 2014

cozz commented Sep 9, 2014

cozz commented Oct 3, 2014

Diapolo commented Nov 21, 2014

gmaxwell commented Nov 21, 2014

cozz commented Nov 22, 2014

morcos commented Nov 22, 2014

cozz commented Nov 22, 2014

laanwj commented Feb 26, 2015