As part of BIP 141, transaction sizes have been replaced by virtual size (with witness data discounted) as the metric for measuring the fee rate of a transaction. RPC now returns the virtual size of the transaction instead of the serialized size. However, code comments and RPC help text have not been updated everywhere to reflect this change.
This PR fixes various comments and RPC help text.
utACK 2ff29a7
330 | @@ -331,18 +331,18 @@ UniValue getdifficulty(const UniValue& params, bool fHelp) 331 | 332 | std::string EntryDescriptionString() 333 | { 334 | - return " \"size\" : n, (numeric) transaction size in bytes\n" 335 | + return " \"size\" : n, (numeric) virtual transaction size as defined in BIP 141. This is different from actual serialized size for witness transactions as witness data is discounted.\n"
This is a bug. :(
This commit updates the RPC help text to reflect what the function call actually returns. If you think there's a problem with the RPC function, I suggest you raise an issue against that code.
Well, NACK changing documentation to match buggy behaviour rather than fixing the bug...
1235 | @@ -1236,7 +1236,7 @@ UniValue getmempoolinfo(const UniValue& params, bool fHelp) 1236 | "\nResult:\n" 1237 | "{\n" 1238 | " \"size\": xxxxx, (numeric) Current tx count\n" 1239 | - " \"bytes\": xxxxx, (numeric) Sum of all tx sizes\n" 1240 | + " \"bytes\": xxxxx, (numeric) Sum of all virtual transaction sizes as defined in BIP 141. Differs from actual serialized size because witness data is discounted\n"
This also shouldn't be virtual.
See above
The reasonable/expected behaviour in at least these cases is to give real sizes, not "virtual size" nonsense.
I disagree this is a bug. Apart from miners, nobody should care about the total serialized size.
If we're not showing the size in bytes, then just get rid of the size/bytes fields and add weight...
Removing fields from objects returned by RPC functions is a non-backwards compatible change, which may break clients using the API. I agree that adding weight may be useful for some users and we should consider extending the RPC return objects to include it. If you feel strongly about having weight returned RPC functions, let me know and I'll open a new PR.
This PR is simply to fix up the help text so it reflects what the function call is actually doing.
Strinctly speaking, this is wrong...BIP 141 defines virtual size as "Transaction weight / 4 (rounded up to nearest integer)." with weight being " Base transaction size * 3 + Total transaction size (ie. the same method as calculating Block weight from Base size and Total size).".
However, this method actually returns that iff sigops per byte of the tx is lower than sigops per byte of the block consensus rule. Not sure if it matters since that should be rare, but it is, strictly speaking, different.
@TheBlueMatt - you're correct. The actual value returned will be higher than the virtual size if the sigsops per byte in the tx is greater the nBytesPerSigOp due to https://github.com/bitcoin/bitcoin/commit/ab942c15bd3854650afa810d7c22d7fd30d346c1.
I could expand the RPC text from:
(numeric) Sum of all virtual transaction sizes as defined in BIP 141. Differs from actual serialized size because witness data is discounted\n"
to
(numeric) Sum of all virtual transaction sizes as defined in BIP 141. Differs from actual serialized size because witness data is discounted. If the sigsops per byte in the transaction is greater nBytesPerSigOp, then returns a size based on the cost of the sigops rather than the serialized size of the transaction.\n"
I don't think that actually makes things clearer, but I'm happy to update the text if you think that's preferable.
Its relatively strange to me that it would be inconsistent with the BIP. If we want to keep it this way, I'd say changing the BIP is sufficient as the help text refers to that. Alternatively, we could change the return value to be consistent with the BIP. Either way, I agree, expanding the RPC help text is strange.
Closing this, as there seems to be strong disagreement about this and the discussion has bled out.
@laanwj please don't close this. I don't think anyone (other than possibly @luke-jr) disagrees that this is an clear improvement. The existing help text is confusing and this is far more accurate. @TheBlueMatt 's comments are a more obscure technical refinement, but I don't think meant as a blocker to merging this.
I think its important that we make an effort to properly document and educate our users about how the software works. The whole concept of "virtual size" is a bit unfortunate but I don't see any way around keeping it unless we want to radically change how people think about feerates. We can't be held up from making clear improvements to the software just because one person vehemently disagrees.
utACK from me
Travis appeared to fail because of an unrelated environment setup issue. I've rebased this PR which will kick off another travis run.
There seems to be a conflict - getrawmempool defines "size" as the virtual size, whereas getrawtransaction defines it as the serialized size (and using "vsize" instead for virtual size). This doc change helps to clarify the difference, but to avoid confusion perhaps the term "size" should be made to mean the same thing in all RPC calls.
Oh and in getblock RPC, "size" also refers to serialized size and not "weight / 4".
@bitcoinfees - yes, I completely agree with everything you say, and that it's confusing that 'size' means different things in different RPCs.
I didn't make any functional changes to the API since there may be applications that rely on the current behaviour. This PR simply updates the self-documentation in the RPCs so that it accurately describes what is being returned.
Perhaps we should change those RPC definitions, but that's a discussion for another PR and I don't think it should block this documentation fix.
It still doesn't make sense to document a bug rather than fixing it.
@luke-jr but that is not the choice at hand. The choice presented by this PR is to document the existing behavior or leave it improperly documented. Choosing to document it does not preclude changing it in the future. Although I think most of us do not consider it a bug.
I can't understand why this is even a question. How can we argue against more clearly explaining to users what the behavior is? We should always be willing to document existing behavior instead of stalling on that while arguing about whether to change the behavior.
@sipa @TheBlueMatt This PR has several ACK's (and @luke-jr's objection), but I think its held up from merging b/c lack of clear feedback from you two. If you are OK with it could you say?
ACK 2ddb53054579c70de061d95386e9a5a02255d53f
840 | @@ -840,7 +841,8 @@ UniValue estimatesmartfee(const JSONRPCRequest& request) 841 | "\nWARNING: This interface is unstable and may disappear or change!\n" 842 | "\nEstimates the approximate fee per kilobyte needed for a transaction to begin\n" 843 | "confirmation within nblocks blocks if possible and return the number of blocks\n" 844 | - "for which the estimate is valid.\n" 845 | + "for which the estimate is valid. Uses virtual transaction size as defined\n" 846 | + "in BIP 141 (witness data is discounted)\n"
Dot at the end of sentence missing.
Thanks @paveljanik . Comment fixed.
