[Qt] Display transaction fee with sat/vbyte value in SendCoinsDialog #12189

ericallam commented at 1:06 PM on January 15, 2018: none

Related to issue #11564, this PR designed to provide feedback to the user about their relative transaction fee before they finish broadcasting their tx to the bitcoin network.

Displaying the sat/vbyte is useful for knowing how likely their tx will be included in an upcoming block when using public fee estimation tools like https://jochen-hoenicke.de/queue/#24h, https://estimatefee.com, and https://bitcoinfees.earn.com.

This updates the SendCoinsDialog to look like this:

Sjors commented at 3:43 PM on January 15, 2018: member

I think the transaction confirmation dialog should use the same unit as the send screen. So you could make two pull requests:

1 - add sat / (v)byte to the dropdown on the send screen 2 - add fee per (kilo)byte to the confirmation screen (this PR)

Nit: I would move the fee per size inside brackets at the end, e.g. "0.00002220 BTC added as transaction fee (0.221 kB at 10.0452 sat/byte)".

ericallam commented at 12:32 PM on January 17, 2018: none

@Sjors I agree that it's a good idea to do 2 different PRs for these changes. Adding the ability to specify custom fees in sat/vbyte is a decent chunk of work because of the way the Custom Fee text field reuses the BitcoinAmountField widget which is used in a couple different places in the UI (like when selecting an amount of BTC to send in a transaction, and to receive).

I think the best approach for the custom fee section would be to not use BitcoinAmountField but to create a new widget specifically for the custom fee input that would both:

Allow inputting the fee as sat/vbyte
When not inputting the fee as sat/vbyte, display (either in the UI or as a tooltip) the resulting sat/vbyte.

When this second PR happens, the Recommended section should also probably either show the sat/vbyte instead of BTC/kb, or show both.

I agree with your Nit about the format of the fee per size label, and will be updating this PR to reflect the change in a few minutes.

Display fee sat/vbyte in SendCoinsDialog

Related to issue #11564, this is designed to provide
feedback to the user before they finish broadcasting their tx
to the bitcoin network.

515f2bdea0

ericallam force-pushed on Jan 17, 2018

ericallam commented at 12:44 PM on January 17, 2018: none

The SendCoinsDialog confirmation screen now looks like this:

screen shot 2018-01-17 at 12 44 17

fanquake added the label GUI on Jan 19, 2018

in src/qt/sendcoinsdialog.cpp:332 in 515f2bdea0 outdated

 328 | +        double satPerVByte = txFee / txSize; // txFee is already in satoshis
 329 | +
 330 |          // append transaction size
 331 | -        questionString.append(" (" + QString::number((double)currentTransaction.getTransactionSize() / 1000) + " kB)");
 332 | +        questionString.append(" (");
 333 | +        questionString.append(QString::number(txSize / 1000) + " kB");

jonasschnelli commented at 10:34 AM on February 12, 2018:

Use formatBytes() from GUIUtils or otherwise please use KB / 1024 for consistency reasons.

jonasschnelli commented at 10:34 AM on February 12, 2018: contributor

Concept ACK

Using formatBytes() to standardize the size output 111288cc04

PierreRochard commented at 11:22 PM on March 28, 2018: contributor

Strong Concept ACK, hopefully we coalesce on a solution to issue #11564

in src/qt/sendcoinsdialog.cpp:321 in 111288cc04 outdated

 317 | @@ -318,11 +318,24 @@ void SendCoinsDialog::on_sendButton_clicked()
 318 |          // append fee string if a fee is required
 319 |          questionString.append("<hr /><span style='color:#aa0000;'>");
 320 |          questionString.append(BitcoinUnits::formatHtmlWithUnit(model->getOptionsModel()->getDisplayUnit(), txFee));
 321 | +

jonasschnelli commented at 6:36 PM on April 10, 2018:

Is there a reason for this newline and the double-newline in L333?

ericallam commented at 10:33 AM on April 14, 2018:

Fixed in e8485d10e63c1c66ea85b253ff38ee86fd2a3ea6

in src/qt/sendcoinsdialog.cpp:328 in 111288cc04 outdated

 323 |          questionString.append(tr("added as transaction fee"));
 324 |  
 325 | +        double txSize = (double)currentTransaction.getTransactionSize();
 326 | +
 327 | +        // safe to assume that txSize cannot be 0 at this point?
 328 | +        double satPerVByte = txFee / txSize; // txFee is already in satoshis

jonasschnelli commented at 6:44 PM on April 10, 2018:

From the isolated code part perspective, I'd say either factor out into a function that handles the txSize validity or so an if structure above to ensue txSize is valid.

ericallam commented at 10:34 AM on April 14, 2018:

Moved sat/vbyte calculation to the WalletModelTransaction class in e8485d10e63c1c66ea85b253ff38ee86fd2a3ea6

in src/qt/sendcoinsdialog.cpp:332 in 111288cc04 outdated

 328 | +        double satPerVByte = txFee / txSize; // txFee is already in satoshis
 329 | +
 330 |          // append transaction size
 331 | -        questionString.append(" (" + QString::number((double)currentTransaction.getTransactionSize() / 1000) + " kB)");
 332 | +        questionString.append(" (");
 333 | +        questionString.append(GUIUtil::formatBytes(txSize));

jonasschnelli commented at 6:53 PM on April 10, 2018:

This may loos precision since formatBytes() does show 1.2kb as 1kb... not sure if this is a problem though. Maybe better keep it the way it was.

ericallam commented at 10:34 AM on April 14, 2018:

Thanks for the heads up, moved back to original version in e8485d10e63c1c66ea85b253ff38ee86fd2a3ea6

jonasschnelli changes_requested

Move sat/vbyte calculation to WalletModelTransaction

This allowed isolation of the transaction size validation into the wallet model, to protect against divide by 0 errors.

Also removed use of `GUIUtil::formatBytes` to output the size of the transaction to have better precision in the output.

e8485d10e6

Sjors approved

Sjors commented at 11:17 AM on April 21, 2018: member

tACK e8485d1, but please squash the commits. Looks like @jonasschnelli's items were addressed.

I'd still like the user to be able to enter a fee in sat/byte, but that can be done on top of this work.

I think we were inclined against using the "v" prefix in #12180, i.e. just sat/byte should suffice.

MarcoFalke referenced this in commit 81c533c6f4 on May 14, 2018

MarcoFalke commented at 3:30 PM on May 23, 2018: member

Needs rebase if still relevant. (Personally I think we shouldn't expose gui users to the term "vbyte" unless they request raw data about fee estimates)

MarcoFalke added the label Needs rebase on Jun 6, 2018

DrahtBot added the label Up for grabs on Nov 8, 2018

DrahtBot commented at 11:13 PM on November 8, 2018: member

There hasn't been much activity lately and the patch still needs rebase, so I am closing this for now. Please let me know when you want to continue working on this, so the pull request can be re-opened.

DrahtBot closed this on Nov 8, 2018

jasonbcox referenced this in commit 6b6d0ee541 on Oct 11, 2019

laanwj removed the label Needs rebase on Oct 24, 2019

UdjinM6 referenced this in commit ee8beba049 on Jun 18, 2021

TheArbitrator referenced this in commit 52ad4ff0ab on Jun 21, 2021

UdjinM6 referenced this in commit 9316725eb2 on Jun 24, 2021

UdjinM6 referenced this in commit d8051ec52f on Jun 29, 2021

UdjinM6 referenced this in commit 47b2895954 on Jun 29, 2021

UdjinM6 referenced this in commit 9621d4181b on Jul 2, 2021

UdjinM6 referenced this in commit 984f6bbd18 on Jul 4, 2021

UdjinM6 referenced this in commit a650372830 on Jul 6, 2021

MarcoFalke locked this on Dec 16, 2021

fanquake removed the label Up for grabs on Aug 4, 2022