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

[rpc] Fix transaction size comments and RPC help text. #8747

Merged
merged 1 commit into from Jan 5, 2017

Conversation

Projects
None yet
@jnewbery
Copy link
Member

commented Sep 16, 2016

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.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Sep 18, 2016

utACK 2ff29a7

@laanwj

laanwj approved these changes Sep 19, 2016

Copy link
Member

left a comment

utACK 2ff29a7

@luke-jr
Copy link
Member

left a comment

The reasonable/expected behaviour in at least these cases is to give real sizes, not "virtual size" nonsense.

@@ -331,18 +331,18 @@ UniValue getdifficulty(const UniValue& params, bool fHelp)

std::string EntryDescriptionString()
{
return " \"size\" : n, (numeric) transaction size in bytes\n"
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 comment has been minimized.

Copy link
@luke-jr

luke-jr Sep 20, 2016

Member

This is a bug. :(

This comment has been minimized.

Copy link
@jnewbery

jnewbery Sep 20, 2016

Author Member

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.

This comment has been minimized.

Copy link
@luke-jr

luke-jr Sep 20, 2016

Member

Well, NACK changing documentation to match buggy behaviour rather than fixing the bug...

@@ -1236,7 +1236,7 @@ UniValue getmempoolinfo(const UniValue& params, bool fHelp)
"\nResult:\n"
"{\n"
" \"size\": xxxxx, (numeric) Current tx count\n"
" \"bytes\": xxxxx, (numeric) Sum of all tx sizes\n"
" \"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 comment has been minimized.

Copy link
@luke-jr

luke-jr Sep 20, 2016

Member

This also shouldn't be virtual.

This comment has been minimized.

Copy link
@jnewbery

jnewbery Sep 20, 2016

Author Member

See above

@sipa

This comment has been minimized.

Copy link
Member

commented Sep 20, 2016

@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 21, 2016

@sipa so do you agree on this documentation change?

@sipa

This comment has been minimized.

Copy link
Member

commented Sep 21, 2016

@laanwj I agree with it, though there was some discussion in one of the former IRC meetings that we shouldn't call this virtual size.

@luke-jr

This comment has been minimized.

Copy link
Member

commented Sep 22, 2016

If we're not showing the size in bytes, then just get rid of the size/bytes fields and add weight...

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Sep 22, 2016

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.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2016

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.

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Sep 27, 2016

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

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.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2016

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.

@laanwj

This comment has been minimized.

Copy link
Member

commented Nov 16, 2016

Closing this, as there seems to be strong disagreement about this and the discussion has bled out.

@laanwj laanwj closed this Nov 16, 2016

@morcos

This comment has been minimized.

Copy link
Member

commented Nov 16, 2016

@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

@laanwj laanwj reopened this Nov 16, 2016

@jnewbery jnewbery force-pushed the jnewbery:rpc_comments branch from 2ff29a7 Nov 16, 2016

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Nov 16, 2016

Travis appeared to fail because of an unrelated environment setup issue. I've rebased this PR which will kick off another travis run.

@bitcoinfees

This comment has been minimized.

Copy link

commented Nov 17, 2016

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

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Nov 17, 2016

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

@luke-jr

This comment has been minimized.

Copy link
Member

commented Nov 17, 2016

It still doesn't make sense to document a bug rather than fixing it.

@morcos

This comment has been minimized.

Copy link
Member

commented Nov 17, 2016

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

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Nov 17, 2016

@luke-jr - I agree with @morcos. Nothing in this documentation fix precludes you from opening an issue or PR to change the API.

@morcos

This comment has been minimized.

Copy link
Member

commented Dec 7, 2016

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

@sipa

This comment has been minimized.

Copy link
Member

commented Dec 7, 2016

ACK 2ddb53054579c70de061d95386e9a5a02255d53f

src/rpc/mining.cpp Outdated
@@ -840,7 +841,8 @@ UniValue estimatesmartfee(const JSONRPCRequest& request)
"\nWARNING: This interface is unstable and may disappear or change!\n"
"\nEstimates the approximate fee per kilobyte needed for a transaction to begin\n"
"confirmation within nblocks blocks if possible and return the number of blocks\n"
"for which the estimate is valid.\n"
"for which the estimate is valid. Uses virtual transaction size as defined\n"
"in BIP 141 (witness data is discounted)\n"

This comment has been minimized.

Copy link
@paveljanik

paveljanik Dec 8, 2016

Contributor

Dot at the end of sentence missing.

@jnewbery jnewbery force-pushed the jnewbery:rpc_comments branch Dec 8, 2016

@jnewbery jnewbery force-pushed the jnewbery:rpc_comments branch to d29505d Dec 8, 2016

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Dec 8, 2016

Thanks @paveljanik . Comment fixed.

@paveljanik

This comment has been minimized.

Copy link
Contributor

commented Dec 9, 2016

ACK d29505d

@laanwj laanwj merged commit d29505d into bitcoin:master Jan 5, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jan 5, 2017

Merge #8747: [rpc] Fix transaction size comments and RPC help text.
d29505d Fix transaction size comments. Size now refers to virtual size as defined in BIP141. (jonnynewbs)

@jnewbery jnewbery deleted the jnewbery:rpc_comments branch Jan 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.