Subtract fee from amount

cozz commented at 11:04 pm on June 11, 2014: contributor

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

subtractfeefromamount7

Without checkbox subtractfeefromamount4

With checkbox subtractfeefromamount3

Dust edge case subtractfeefromamount5

rpc subtractfeefromamount6

ghost commented at 6:17 am on June 12, 2014: none

Untested ACK

laanwj added the label Windows on Jun 13, 2014

laanwj removed the label Windows on Jun 19, 2014

laanwj added the label Wallet on Jun 19, 2014

laanwj commented at 8:35 am on June 19, 2014: member

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

laanwj commented at 8:04 am on July 1, 2014: member

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 commented at 12:43 pm on July 1, 2014: none

Needs to be rebased first.

in src/core.h: in fe07479750 outdated

186@@ -187,6 +187,12 @@ class CTxOut
187         return (nValue < 3*minRelayTxFee.GetFee(nSize));
188     }
189 
190+    int64_t getDustThreshold(CFeeRate minRelayTxFee) const

Diapolo commented at 1:42 pm on July 1, 2014:

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

in src/qt/forms/sendcoinsentry.ui: in fe07479750 outdated

145+       <widget class="BitcoinAmountField" name="payAmount"/>
146+      </item>
147+      <item>
148+       <widget class="QCheckBox" name="checkboxSubtractFeeFromAmount">
149+        <property name="toolTip">
150+         <string>The fee will be automatically deducted from the amount being sent. The recipient will receive less bitcoins than you enter in the amount field.</string>

Diapolo commented at 2:03 pm on July 1, 2014:

Nit: More a question, should Bitcoins be uppercase in general?

cozz commented at 6:56 pm on July 1, 2014:

I guess lowercase is ok here.

See https://bitcoin.org/en/faq Looks like they say bitcoins when talking about coins. But Bitcoin when talking about Bitcoin.

Or here also lowercase https://github.com/bitcoin/bitcoin/blob/master/src/rpcwallet.cpp#L1590

Diapolo commented at 7:23 pm on July 1, 2014:

That’s fine with me then :).

in src/qt/sendcoinsdialog.cpp: in fe07479750 outdated

140@@ -137,6 +141,39 @@ void SendCoinsDialog::on_sendButton_clicked()
141         return;
142     }
143 
144+    fNewRecipientAllowed = false;
145+

Diapolo commented at 2:06 pm on July 1, 2014:

Nit: While you are at it, can you remove this new-line ;)?

in src/qt/sendcoinsentry.cpp: in fe07479750 outdated

72@@ -73,11 +73,10 @@ void SendCoinsEntry::setModel(WalletModel *model)
73         connect(model->getOptionsModel(), SIGNAL(displayUnitChanged(int)), this, SLOT(updateDisplayUnit()));
74 
75     connect(ui->payAmount, SIGNAL(textChanged()), this, SIGNAL(payAmountChanged()));
76+    connect(ui->checkboxSubtractFeeFromAmount, SIGNAL(toggled(bool)), this, SIGNAL(subtractFeeFromAmountChanged()));
77     connect(ui->deleteButton, SIGNAL(clicked()), this, SLOT(deleteClicked()));
78     connect(ui->deleteButton_is, SIGNAL(clicked()), this, SLOT(deleteClicked()));
79     connect(ui->deleteButton_s, SIGNAL(clicked()), this, SLOT(deleteClicked()));
80-
81-    clear();

Diapolo commented at 2:07 pm on July 1, 2014:

Why was this removed?

Diapolo commented at 2:12 pm on July 1, 2014: none

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

cozz commented at 6:46 pm on July 1, 2014: contributor

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 commented at 0:31 am on July 8, 2014: contributor

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

Earlz commented at 0:52 am on July 8, 2014: contributor

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 commented at 2:41 am on July 8, 2014: contributor

@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 commented at 9:26 pm on July 8, 2014: contributor

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 commented at 7:02 pm on July 23, 2014: contributor

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.

subtractfeefromamount9

laanwj commented at 7:35 am on July 24, 2014: member

@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 commented at 9:13 am on July 24, 2014: member

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

laanwj commented at 9:49 am on July 24, 2014: member

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

sipa commented at 9:58 am on July 24, 2014: member

EIther prevent it, or use equal split?

cozz commented at 1:31 pm on July 24, 2014: contributor

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 commented at 1:41 pm on July 24, 2014: member

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 commented at 1:47 pm on July 24, 2014: contributor

Ok, I will look into it, choosing individually.

cozz commented at 0:02 am on July 25, 2014: contributor

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.

subtractfeefromamount10

in src/qt/coincontroldialog.cpp: in 94e2d7cd2c outdated

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

laanwj commented at 7:34 am on July 25, 2014:

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 commented at 7:42 am on July 25, 2014: member

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 commented at 8:16 am on September 8, 2014: member

Works for me.

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

laanwj referenced this in commit 2ae206452e on Sep 8, 2014

cozz force-pushed on Sep 9, 2014

BitcoinPullTester commented at 1:54 am on September 9, 2014: none

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 commented at 2:12 am on September 9, 2014: contributor

Added a quick RPC test.

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

Subtract fee from amount 74a6b3e255

Subtract fee from amount: RPC-tests f156d37604

Subtract fee from amount: CAmount 55e774db5b

cozz force-pushed on Oct 3, 2014

cozz commented at 3:47 am on October 3, 2014: contributor

rebased

Diapolo commented at 4:23 pm on November 21, 2014: none

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

gmaxwell commented at 8:40 pm on November 21, 2014: contributor

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 commented at 8:26 am on November 22, 2014: contributor

@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 commented at 3:19 pm on November 22, 2014: member

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 commented at 4:38 pm on November 22, 2014: contributor

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

laanwj commented at 1:16 pm on February 26, 2015: member

Rebased as #5831

laanwj closed this on Feb 26, 2015

MarcoFalke locked this on Sep 8, 2021

Subtract fee from amount #4331