Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[qt] Improved "custom fee" explanation in tooltip #12501

Merged
merged 1 commit into from Mar 5, 2018

Conversation

randolf
Copy link
Contributor

@randolf randolf commented Feb 21, 2018

Thanks to @dooglus for asking about this tooltip in Issue 12500.
Reference: https://www.github.com/bitcoin/bitcoin/issues/12500

I would also appreciate it if someone can confirm that 1 kilobyte in this field indeed represents 1,000 bytes rather than 1,024 bytes (if it's supposed to be 1,024, then I'll gladly make the necessary changes to reflect this).

@dooglus
Copy link
Contributor

dooglus commented Feb 21, 2018

It's 1000 bytes.

But I don't think there's any logic about "transactions larger than 1 kilobyte". It's a simple BTC-per-1000-bytes calculation, as follows:

The value in the field is used to construct a CFeeRate object:

ctrl.m_feerate = CFeeRate(ui->customFee->value());

The constructor sets nSatoshisPerK:

CFeeRate(const I _nSatoshisPerK): nSatoshisPerK(_nSatoshisPerK) {

nSatoshisPerK is multiplied by the tx size and divided by 1000:

CAmount nFee = nSatoshisPerK * nSize / 1000;

So the tooltip really doesn't need to say anything other than that it's per 1000 bytes I guess.

@dooglus
Copy link
Contributor

dooglus commented Feb 21, 2018

Although, it might make sense to also mention that it's not really per 1000 bytes, but per 4000 weight units. Or in other words per 1000 bytes of virtual transaction size.

@randolf
Copy link
Contributor Author

randolf commented Feb 21, 2018

@dooglus Thanks for the clarification. I think it's important to mention 1 kilobyte in the tooltip since the field's label mentions "per kilobyte" specifically.

My feeling on weight units is that since they're not mentioned in the field's label that inclusion in the tooltip would be more likely to introduce confusion for those less-knowledgeable users, and so I'm inclined to think that this concept is probably best left in documentation.

@dooglus
Copy link
Contributor

dooglus commented Feb 21, 2018

Fair enough, but mention that it's per 1000 bytes of virtual transaction size, not actual transaction size.

int64_t GetVirtualTransactionSize(int64_t nWeight, int64_t nSigOpCost)
{
    return (std::max(nWeight, nSigOpCost * nBytesPerSigOp) + WITNESS_SCALE_FACTOR - 1) / WITNESS_SCALE_FACTOR;
}

@@ -848,7 +848,7 @@
<item>
<widget class="QLabel" name="labelCustomPerKilobyte">
<property name="toolTip">
<string>If the custom fee is set to 1000 satoshis and the transaction is only 250 bytes, then &quot;per kilobyte&quot; only pays 250 satoshis in fee, while &quot;total at least&quot; pays 1000 satoshis. For transactions bigger than a kilobyte both pay by kilobyte.</string>
<string>For the transaction fee, you can specify the exact number of "satoshis per kilobyte (1,000 bytes)." For transactions larger than 1 kilobyte, the fee will be calculated on a per-kilobyte basis. For transactions smaller than 1 kilobyte, the fee will be apportioned according to the size of the transaction (e.g., for a transaction that is 500 bytes {half of 1 kilobyte}, a fee of 0.00000010 would therefore be reduced to 0.00000005).</string>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no longer a distinction between less-than-1kB and other txes. You might as well remove that distinction altogether.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarcoFalke Oh, so to clarify, that means that for transactions of 1 kilobyte or less that the fee would still be calculated on a per-kilobyte basis too then, and I should squash it down to the following only?

For the transaction fee, you can specify the exact number of "satoshis per kilobyte (1,000 bytes)."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it is always a fee rate (it just happens to be denominated in BTC/kB, and not satoshis/Byte)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct. But we already know it's for the transaction fee, and that you're specifying it. You aren't specifying satoshis per kB, you can specify it in BTC, mBTC, or uBTC. So you could just squash it down to "per kilobyte (1000 bytes)" - but that's almost what the label itself already says.

I prefer something like:

Specify a custom fee per 1000 bytes of virtual transaction size."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So all fee rates (set on the command line or the gui) are denominated in BTC/kB

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The gui allows me to specify the fee rate in BTC/kB, mBTC/kB, and uBTC/kB.

It's kind of unfortunate that we can't specify it in sat/B, since that seems to be emerging as a standard way to talk about fee rates.

@randolf
Copy link
Contributor Author

randolf commented Feb 21, 2018

@dooglus Thanks, I like the more condensed sentence you suggested and so I added a new commit to reflect that. (For a tooltip, simpler is usually better too.)

Note: It appears that @dooglus' suggestion disappeared since I added the new commit.

@@ -848,7 +848,7 @@
<item>
<widget class="QLabel" name="labelCustomPerKilobyte">
<property name="toolTip">
<string>For the transaction fee, you can specify the exact number of "satoshis per kilobyte (1,000 bytes)." For transactions larger than 1 kilobyte, the fee will be calculated on a per-kilobyte basis. For transactions smaller than 1 kilobyte, the fee will be apportioned according to the size of the transaction (e.g., for a transaction that is 500 bytes {half of 1 kilobyte}, a fee of 0.00000010 would therefore be reduced to 0.00000005).</string>
<string>Specify a custom fee per 1,000 bytes of virtual transaction size.</string>
Copy link
Member

@maflcko maflcko Feb 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just my opinion, but we shouldn't need to specify that the size is virtual. Either the user knows what virtual means, so they know that all sizes that involve transaction fees are virtual. Or they don't, in which case it just adds confusion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that there are two sizes: virtual and actual. I didn't know (until reading the source) which of the two this custom setting was specifying. It would have been useful for me if the tooltip had cleared up the ambiguity.

When I double-click on a transaction in the transaction list it shows the actual size and not the virtual size. That's the only other place I've seen any mention of transaction size in the GUI.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears that @dooglus' suggestion disappeared since I added the new commit.

I didn't remove anything. You may need to click "view outdated" since my comment was in response to an old commit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that should probably be changed to virtual size.

@maflcko
Copy link
Member

maflcko commented Feb 21, 2018

utACK, seems like an improvement.

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

@fanquake fanquake added the GUI label Feb 22, 2018
@randolf randolf force-pushed the patch-2 branch 3 times, most recently from f3b4294 to a1605f0 Compare February 22, 2018 07:11
@randolf
Copy link
Contributor Author

randolf commented Feb 22, 2018

@MarcoFalke The squash/rebase is complete. Thanks.

@Willtech
Copy link
Contributor

Willtech commented Feb 22, 2018

The tooltip is not clear. It can be understood that the fee is per every 1000 bytes virtual transaction size to mean a transaction of 1001 bytes virtual transaction size will pay 2x the entered fee, the same as 2000 bytes virtual transaction size.

It would be better IMHO to say something like Specify a custom fee per 1,000 bytes of the virtual transaction's size. If the custom fee is set to 1000 satoshis and the transaction is only 250 bytes virtual size, then &quot;per kilobyte&quot; only pays 250 satoshis in fee.

Specify a custom fee per 1,000 bytes of the virtual transaction's size.

@randolf
Copy link
Contributor Author

randolf commented Feb 22, 2018

@Willtech We did have some wording describing the apportioned amount, but @MarcoFalke explained that this isn't how it works anymore so I removed it (I don't know where his comments disappeared to though as it seems that GitHub is losing parts of the conversation here a few times now).

So, I'd be happy to put the wording back in, but I would like consensus first on whether there is support for an apportioned amount (e.g., a transaction size of 250 bytes reduces the fee to one quarter).

By the way, the wording I added (then later removed) was:

For the transaction fee, you can specify the exact number of "satoshis per kilobyte (1,000 bytes)." For transactions larger than 1 kilobyte, the fee will be calculated on a per-kilobyte basis. For transactions smaller than 1 kilobyte, the fee will be apportioned according to the size of the transaction (e.g., for a transaction that is 500 bytes {half of 1 kilobyte}, a fee of 0.00000010 would therefore be reduced to 0.00000005).

@dooglus
Copy link
Contributor

dooglus commented Feb 23, 2018

It might also be worth noting that if you specify a value that is less than either of the -mintxfee or -minrelaytxfee values, then the greater of those two values will silently be used instead:

CAmount GetRequiredFee(unsigned int nTxBytes)
{
    return std::max(CWallet::minTxFee.GetFee(nTxBytes), ::minRelayTxFee.GetFee(nTxBytes));
}

I only found this out accidentally when I tried to make a transaction that pays 333 satoshis per 1000 bytes, but it silently changed my custom fee to 1000 satoshis per 1000 bytes, due to those two settings both defaulting to 1000 satoshis.

@dooglus
Copy link
Contributor

dooglus commented Feb 23, 2018

So, I'd be happy to put the wording back in

We don't need that wording back in - it was talking about how things behave differently if the tx size is less than 1000 bytes, and that's no longer the case.

The point that maybe needs clarifying is that the feerate of x per 1000 bytes applies on a pro rata basis. If the fee is x per 1000 bytes, and the tx is y bytes, then the fee is x*y/1000. That seems pretty intuitive to me. If a tx that was 1001 bytes cost 2x, then that would be described as "x per 1000 bytes or part thereof", I should think. The fees work the same way as buying cheese or gasoline. If the cheese costs $x per pound, and you're buying 1.1 pounds of it, you pay $1.1x, not $2x.

@@ -848,7 +848,7 @@
<item>
<widget class="QLabel" name="labelCustomPerKilobyte">
<property name="toolTip">
<string>If the custom fee is set to 1000 satoshis and the transaction is only 250 bytes, then &quot;per kilobyte&quot; only pays 250 satoshis in fee, while &quot;total at least&quot; pays 1000 satoshis. For transactions bigger than a kilobyte both pay by kilobyte.</string>
<string>Specify a custom fee per 1,000 bytes of the virtual transaction's size.</string>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The transaction is not virtual. The size is...?

transation's virtual size.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without the typo:

Specify a custom fee per 1,000 bytes of the transaction's virtual size.

@randolf
Copy link
Contributor Author

randolf commented Feb 27, 2018

@dooglus I discussed this a few days ago in irc.freenode.net#bitcoin-core-dev and based on that conversation I added some "Notes" to this tooltip. Now the full text reads as follows, and I would like to know if this looks reasonable (this is my current attempt at trying to make it easier to understand):

Specify a custom fee per kB (1,000 bytes) of the virtual transaction's size.

Note: Since the fee is calculated on a per-byte basis, a fee of "100 satoshis per kB" for a transaction size of 500 bytes (half of 1 kB) would ultimately yield a fee of only 50 satoshis.

@Empact
Copy link
Member

Empact commented Feb 27, 2018

Nit that "virtual transaction" is not currently a concept in the UI, and I don't think it has value for the user. I think in the source code "virtual transaction" refers to a transaction that has not been broadcast, which is potentially meaningful if you're distinguishing between transaction states in-flight but adds unnecessary complexity for the user. To be clear, I would drop "virtual".

@Empact
Copy link
Member

Empact commented Feb 27, 2018

Also how are translations dealt with? There are ~30 translations of this string that would need to be updated. Maybe it's better to go with a simple removal so that the translation work would be negligible? Note I'm a noob so not knowledgeable on this point.

@dooglus
Copy link
Contributor

dooglus commented Feb 27, 2018

As said here (click 'show outdated'), it should be "transaction's virtual size", not "virtual transaction's size".

@Empact
Copy link
Member

Empact commented Feb 27, 2018

Should "virtual size" be revealed to the end user? It shows up in the transaction unival but IMO the end-user interface should be higher-level, where distinctions between "serialized size" and "virtual size" are not relevant.

@dooglus
Copy link
Contributor

dooglus commented Feb 27, 2018

@Empact How is the user meant to decide how much to pay per virtual byte if they don't know what a virtual byte is? If I ask to pay 1000 satoshis "per 1000 bytes", then later see that the transaction's size is reported to be 216 bytes, but I only paid 134 satoshis aren't I going to think that the GUI got it wrong and paid less than I asked it to?

We should report the serialized and the virtual size so the user at least has a chance of making an informed decision.

@randolf
Copy link
Contributor Author

randolf commented Feb 27, 2018

@dooglus I'm glad that @Empact raised the point about the transaction's virtual size possibly being confusing to end-users. Your point that it needs to be mentioned seems to make sense, which leads me to ask this question: Is the GUI showing the transaction's total (non-virtual?) size while instead the code is calculating the "transaction's virtual size" behind-the-scenes?

@@ -848,7 +848,9 @@
<item>
<widget class="QLabel" name="labelCustomPerKilobyte">
<property name="toolTip">
<string>If the custom fee is set to 1000 satoshis and the transaction is only 250 bytes, then &quot;per kilobyte&quot; only pays 250 satoshis in fee, while &quot;total at least&quot; pays 1000 satoshis. For transactions bigger than a kilobyte both pay by kilobyte.</string>
<string>Specify a custom fee per kB (1,000 bytes) of the virtual transaction's size.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The size is virtual, not the transaction.

@Empact
Copy link
Member

Empact commented Feb 27, 2018

IMO the difference between virtual and serialized size is an implementation detail irrelevant to users. Here are a few blockchain explorer transaction views - they include "size" and sometimes "weight" but never a separate "virtual size".
https://blockchain.info/tx/ee72a85ab4ced151bf26d3317c3ec41ace4e523cd2f8764dc089ac94383eae74
https://blockexplorer.com/tx/ee72a85ab4ced151bf26d3317c3ec41ace4e523cd2f8764dc089ac94383eae74
https://live.blockcypher.com/btc/tx/ee72a85ab4ced151bf26d3317c3ec41ace4e523cd2f8764dc089ac94383eae74/
https://www.blocktrail.com/BTC/tx/ee72a85ab4ced151bf26d3317c3ec41ace4e523cd2f8764dc089ac94383eae74

Where is virtual size exposed in your experience? The important thing in my experience is having a value that can be compared with mempool viewers for informed fee selection:
https://jochen-hoenicke.de/queue/#1,24h

@dooglus
Copy link
Contributor

dooglus commented Feb 27, 2018

Where is virtual size exposed in your experience? The important thing in my experience is having a value that can be compared with mempool viewers for informed fee selection:
https://jochen-hoenicke.de/queue/#1,24h

That's a good example. All the sizes and rates shown there are virtual sizes, and fee per virtual byte. He doesn't use the word "virtual", but talks about how "sizes include the segwit discount". That's just another way of saying they are the virtual sizes.

the lower diagram already shows the size minus three quarter of the witness size. The segwit discount is also included when computing the fee level for a transaction

So the very site you mention is exposing the virtual size (only).

@Empact
Copy link
Member

Empact commented Feb 27, 2018

Great, so if the virtual size is the meaningful value, shared everywhere as the size, why not just refer to it as the size as it's the only relevant size to the user?

@dooglus
Copy link
Contributor

dooglus commented Feb 28, 2018

@Empact Because the GUI itself already refers to the serialization size as the size.

Double-click on any transaction and it reports only the serialization size, not the virtual size.

For example, this is a transaction where I asked to pay 1000 satoshis per 1000 bytes:

Transaction fee: -0.00000134 BTC
[...]
Transaction total size: 216 bytes

The virtual size was 134 bytes, but that isn't reported anywhere.

(I'm not sure why the fee shows up as a negative amount, but that's a separate issue).

@randolf
Copy link
Contributor Author

randolf commented Feb 28, 2018

@dooglus It seems that perhaps you've discovered a bug. Although this clearly is not a detrimental bug given that the resulting transaction fee is lower than what you expected it to be (which I suspect is unlikely to yield negative complaints), it still should probably be addressed at the very least.

Updating the tooltip may not the ideal way of going about remedying this discrepancy though, and so perhaps raising a separate issue entitled "transaction fee discrepancies" that focuses on getting the underlying mechanics of this sorted out would be better? (Feel free to reference this PR if you do raise this as a separate issue.)

I'm favouring what @Empact suggested because I do think that specifying "virtual size" is likely to be confusing to at least some end users (particularly those who aren't technical). If the tooltip were to reference documentation in a "see also ... for more detailed information" manner, I certainly think that would be a good option as it would not detract from keeping the tooltip text simple, straight-forward, and easy to understand, while also satisfying those who do want more technical detail.

@randolf
Copy link
Contributor Author

randolf commented Mar 1, 2018

@dooglus Thanks again for raising this issue. I just rebased all the commits after reviewing the "virtual size" wording in today's Bitcoin Core Developer meeting...

The consensus seems to be that we're going to "bite the bullet" so-to-speak and simply use the wording in the current commit which refers to the "virtual size" of transactions. It was also noted that this is a term that people can search for on Google to find out more if they need/want to (we considered possibly using other ways to describe this, which we did not favour because that would be introducing newer terminology, which in turn would create confusion).

@dooglus
Copy link
Contributor

dooglus commented Mar 1, 2018

Thanks for letting me know.

I just made a pull request (#12580) to add the virtual size to the details modal.

maflcko pushed a commit that referenced this pull request Mar 5, 2018
ee04119 Show a transaction's virtual size in its details dialog. (Chris Moore)

Pull request description:

  #12501 looks like it is going to mention transaction's "virtual size" in the custom fee tooltip, so let's display the virtual size when the user double-clicks a transaction.

Tree-SHA512: c60ae23c9f86edfba086b840519941d8e8ee1be9da5987ffe6dee3255943ea5d215708ce57464f109a1d1c612c4c0eeb11f8f3e203d8a8cfc1f8ec753a8aac27
@dooglus
Copy link
Contributor

dooglus commented Mar 5, 2018

@MarcoFalke just merged my PR, so the transaction details dialog shows both total size and virtual size.

Example tx spending a SegWit utxo:

screenshot_2018-03-05_07-51-38

Example tx spending legacy utxos:

screenshot_2018-03-05_07-51-12

So now it makes sense to refer to a transaction's virtual size and have some expectation that the user knows what this means.

@laanwj
Copy link
Member

laanwj commented Mar 5, 2018

utACK 0bc095e, this is a clear improvement, I think it's been bikeshedded enough.

@laanwj laanwj merged commit 0bc095e into bitcoin:master Mar 5, 2018
laanwj added a commit that referenced this pull request Mar 5, 2018
0bc095e [qt] Improved "custom fee" explanation in tooltip (Randolf Richardson)

Pull request description:

  Thanks to @dooglus for asking about this tooltip in Issue 12500.
  Reference:  https://www.github.com/bitcoin/bitcoin/issues/12500

  I would also appreciate it if someone can confirm that 1 kilobyte in this field indeed represents 1,000 bytes rather than 1,024 bytes (if it's supposed to be 1,024, then I'll gladly make the necessary changes to reflect this).

Tree-SHA512: da2fe0128411b5ef6f0a26382a80601efcf823c3f3591bdd83a7fe7e25777728e7eb89e2e8b175b991566e63838aca12d204792f981031b86e7b2ba28ca50021
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants