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

ericallam · 2018-01-15T13:06:10Z

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 · 2018-01-15T15:43:10Z

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 · 2018-01-17T12:32:42Z

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

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

ericallam · 2018-01-17T12:44:52Z

The SendCoinsDialog confirmation screen now looks like this:

jonasschnelli · 2018-02-12T10:34:21Z

src/qt/sendcoinsdialog.cpp

        // append transaction size
-        questionString.append(" (" + QString::number((double)currentTransaction.getTransactionSize() / 1000) + " kB)");
+        questionString.append(" (");
+        questionString.append(QString::number(txSize / 1000) + " kB");


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

jonasschnelli · 2018-02-12T10:34:36Z

Concept ACK

PierreRochard · 2018-03-28T23:22:30Z

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

jonasschnelli · 2018-04-10T18:36:24Z

src/qt/sendcoinsdialog.cpp

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


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

Fixed in e8485d1

jonasschnelli · 2018-04-10T18:44:43Z

src/qt/sendcoinsdialog.cpp

+        double txSize = (double)currentTransaction.getTransactionSize();
+
+        // safe to assume that txSize cannot be 0 at this point?
+        double satPerVByte = txFee / txSize; // txFee is already in satoshis


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.

Moved sat/vbyte calculation to the WalletModelTransaction class in e8485d1

jonasschnelli · 2018-04-10T18:53:40Z

src/qt/sendcoinsdialog.cpp

        // append transaction size
-        questionString.append(" (" + QString::number((double)currentTransaction.getTransactionSize() / 1000) + " kB)");
+        questionString.append(" (");
+        questionString.append(GUIUtil::formatBytes(txSize));


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.

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

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.

Sjors

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.

f08a385 [qt]: changes sendcoinsdialog's box layout for improved readability. (marcoagner) Pull request description: I'm addressing two (probably duplicate) issues: #11606 and #10613. Some points worth noting: - I've tried to balance the proposed changes on both issues without going too far and remaining a bit conservative. It will be easier to improve based on suggestions where necessary. - I preferred to maintain a layout that doesn't ask for an address truncation because, in my view, this wallet should be conservative on this. - I didn't follow the idea of aligning the amounts to the right for finding it more natural (and minimalist) to read the information without having to map alignments. Additionally, that approach seems to need more `<hr />`'s (or similar) in order to help the user to map information, which ended up cluttering the box too much (specially with multiple recipients). Thus, I preferred to just give some more space between recipients. Let me know if there are better ideas on this. Visually, I went from this (current): ![screenshot from 2018-05-03 15-11-30](https://user-images.githubusercontent.com/5016303/39581859-16abec82-4edc-11e8-86d3-eb722f8a7ed6.png) To this: ![screenshot from 2018-05-03 15-15-41](https://user-images.githubusercontent.com/5016303/39582066-96856adc-4edc-11e8-804c-468aec44cc8d.png) As a side note, while doing this, I thought about a better way to show fees and found there's already a PR on this (#12189) and thought it is Tree-SHA512: e94b740fab6c1babd853a97be65c3b6f86ec174c975a926fde66b147f7a47e0cf0fa10f7255ba92aaba68c76a80dde8c688008179a34705a9799bf24d3c5cd46

maflcko · 2018-05-23T15:30:13Z

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)

DrahtBot · 2018-11-08T23:13:08Z

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.

Summary: f08a385 [qt]: changes sendcoinsdialog's box layout for improved readability. (marcoagner) Pull request description: I'm addressing two (probably duplicate) issues: bitcoin/bitcoin#11606 and bitcoin/bitcoin#10613. Some points worth noting: - I've tried to balance the proposed changes on both issues without going too far and remaining a bit conservative. It will be easier to improve based on suggestions where necessary. - I preferred to maintain a layout that doesn't ask for an address truncation because, in my view, this wallet should be conservative on this. - I didn't follow the idea of aligning the amounts to the right for finding it more natural (and minimalist) to read the information without having to map alignments. Additionally, that approach seems to need more `<hr />`'s (or similar) in order to help the user to map information, which ended up cluttering the box too much (specially with multiple recipients). Thus, I preferred to just give some more space between recipients. Let me know if there are better ideas on this. Visually, I went from this (current): ![screenshot from 2018-05-03 15-11-30](https://user-images.githubusercontent.com/5016303/39581859-16abec82-4edc-11e8-86d3-eb722f8a7ed6.png) To this: ![screenshot from 2018-05-03 15-15-41](https://user-images.githubusercontent.com/5016303/39582066-96856adc-4edc-11e8-804c-468aec44cc8d.png) As a side note, while doing this, I thought about a better way to show fees and found there's already a PR on this (bitcoin/bitcoin#12189) and thought it is Tree-SHA512: e94b740fab6c1babd853a97be65c3b6f86ec174c975a926fde66b147f7a47e0cf0fa10f7255ba92aaba68c76a80dde8c688008179a34705a9799bf24d3c5cd46 Backport of Core PR13158 bitcoin/bitcoin#13158 Test Plan: make check bitcoin-qt -> Send -> View confirmation window Before: {F4069070} After: {F4069069} Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: deadalnix, Fabien, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D4154

f08a385 [qt]: changes sendcoinsdialog's box layout for improved readability. (marcoagner) Pull request description: I'm addressing two (probably duplicate) issues: bitcoin#11606 and bitcoin#10613. Some points worth noting: - I've tried to balance the proposed changes on both issues without going too far and remaining a bit conservative. It will be easier to improve based on suggestions where necessary. - I preferred to maintain a layout that doesn't ask for an address truncation because, in my view, this wallet should be conservative on this. - I didn't follow the idea of aligning the amounts to the right for finding it more natural (and minimalist) to read the information without having to map alignments. Additionally, that approach seems to need more `<hr />`'s (or similar) in order to help the user to map information, which ended up cluttering the box too much (specially with multiple recipients). Thus, I preferred to just give some more space between recipients. Let me know if there are better ideas on this. Visually, I went from this (current): ![screenshot from 2018-05-03 15-11-30](https://user-images.githubusercontent.com/5016303/39581859-16abec82-4edc-11e8-86d3-eb722f8a7ed6.png) To this: ![screenshot from 2018-05-03 15-15-41](https://user-images.githubusercontent.com/5016303/39582066-96856adc-4edc-11e8-804c-468aec44cc8d.png) As a side note, while doing this, I thought about a better way to show fees and found there's already a PR on this (bitcoin#12189) and thought it is Tree-SHA512: e94b740fab6c1babd853a97be65c3b6f86ec174c975a926fde66b147f7a47e0cf0fa10f7255ba92aaba68c76a80dde8c688008179a34705a9799bf24d3c5cd46

Display fee sat/vbyte in SendCoinsDialog

515f2bd

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

ericallam force-pushed the sat_vbyte_fee_rate_in_qt branch from de69d36 to 515f2bd Compare January 17, 2018 12:43

fanquake added the GUI label Jan 19, 2018

jonasschnelli reviewed Feb 12, 2018

View reviewed changes

Using formatBytes() to standardize the size output

111288c

jonasschnelli suggested changes Apr 10, 2018

View reviewed changes

Sjors approved these changes Apr 21, 2018

View reviewed changes

marcoagner mentioned this pull request May 3, 2018

[Qt]: Improve sendcoinsdialog readability #13158

Merged

maflcko added the Needs rebase label Jun 6, 2018

DrahtBot added the Up for grabs label Nov 8, 2018

DrahtBot closed this Nov 8, 2018

laanwj removed the Needs rebase label Oct 24, 2019

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

fanquake removed the Up for grabs label Aug 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ericallam commented Jan 15, 2018

Sjors commented Jan 15, 2018

ericallam commented Jan 17, 2018

ericallam commented Jan 17, 2018

jonasschnelli Feb 12, 2018

jonasschnelli commented Feb 12, 2018

PierreRochard commented Mar 28, 2018

jonasschnelli Apr 10, 2018

ericallam Apr 14, 2018

jonasschnelli Apr 10, 2018

ericallam Apr 14, 2018

jonasschnelli Apr 10, 2018

ericallam Apr 14, 2018

Sjors left a comment

maflcko commented May 23, 2018

DrahtBot commented Nov 8, 2018

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

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

Conversation

ericallam commented Jan 15, 2018

Sjors commented Jan 15, 2018

ericallam commented Jan 17, 2018

ericallam commented Jan 17, 2018

jonasschnelli Feb 12, 2018

Choose a reason for hiding this comment

jonasschnelli commented Feb 12, 2018

PierreRochard commented Mar 28, 2018

jonasschnelli Apr 10, 2018

Choose a reason for hiding this comment

ericallam Apr 14, 2018

Choose a reason for hiding this comment

jonasschnelli Apr 10, 2018

Choose a reason for hiding this comment

ericallam Apr 14, 2018

Choose a reason for hiding this comment

jonasschnelli Apr 10, 2018

Choose a reason for hiding this comment

ericallam Apr 14, 2018

Choose a reason for hiding this comment

Sjors left a comment

Choose a reason for hiding this comment

maflcko commented May 23, 2018

DrahtBot commented Nov 8, 2018