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

Expose a transaction's weight via RPC #12791

Merged
merged 3 commits into from Apr 17, 2018

Conversation

Projects
None yet
@TheBlueMatt
Copy link
Contributor

TheBlueMatt commented Mar 26, 2018

This seems like an obvious oversight.

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2018-03-weight branch Mar 26, 2018

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Mar 26, 2018

Concept ACK

Nice find!

@jamesob

This comment has been minimized.

Copy link
Member

jamesob commented Mar 26, 2018

utACK 4fef409

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Mar 26, 2018

utACK 4fef4096cb1ad62d4b2a9c4cf821cff0406d3757. Can't hurt to include, I guess.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Mar 26, 2018

Strange that this wasn't included yet. Does anyone know the reason? utACK.

@promag

This comment has been minimized.

Copy link
Member

promag commented Mar 26, 2018

utACK 4fef409.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

TheBlueMatt commented Mar 26, 2018

I assume because its somewhat redundant with vsize? I just ran into it trying to get ground-truth data for some unit tests in weight calculation and realized you cant get at it.

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Mar 26, 2018

Yeah, I think the idea was to treat weight as an internal accounting only, and vsize as what users would be exposed to.

utACK 4fef4096cb1ad62d4b2a9c4cf821cff0406d3757

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Mar 26, 2018

Strange that this wasn't included yet. Does anyone know the reason? utACK.

The original BIP text did not include the definition for transaction weight or vsize since they're not consensus rules. They were added here: bitcoin/bips@c2213ed#diff-c0db26883ccab057aaa394db5e50e4b1 . Transaction vsize was only added consistently to rpc outputs at around the same time: #8824

Concept ACK. Could also update p2p_segwit.py to assert the transaction weight around here:

assert_equal(raw_tx["vsize"], vsize)

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Mar 27, 2018

Tested ACK 4fef4096cb1ad62d4b2a9c4cf821cff0406d3757.

p2p_segwit.py test could be updated, otherwise new code path from getrawtransaction isn't tested.

@promag

This comment has been minimized.

Copy link
Member

promag commented Mar 27, 2018

This also affects /rest/tx/:txid.json and /rest/block/:block.json REST calls and also getblock verbosity=true, listsinceblock and listtransactions RPC calls. I don't see tests comparing REST calls with RPC calls but maybe we could do that in new PR.

Needs update to release notes since a new field is added.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

TheBlueMatt commented Mar 27, 2018

Added release notes, and p2p_segwit update.

test/functional/p2p_segwit.py Outdated
assert_equal(len(raw_tx["vin"][0]["txinwitness"]), 1)
assert_equal(raw_tx["vin"][0]["txinwitness"][0], hexlify(witness_program).decode('ascii'))
assert(vsize != raw_tx["size"])
assert(weight / 4 != raw_tx["size"])

This comment has been minimized.

@jnewbery

jnewbery Mar 27, 2018

Member

I think this can stay as it was (if you add a vsize variable above).

assert is a statement, not a function, so if you're going to touch this line, then can you change to:

assert weight / 4 != raw_tx["size"]
@promag
Copy link
Member

promag left a comment

utACK 3b2bb973.

test/functional/p2p_segwit.py Outdated
@@ -12,6 +12,7 @@
from test_framework.key import CECKey, CPubKey
import time
import random
import math

This comment has been minimized.

@promag

promag Mar 27, 2018

Member

nit, sort.

doc/release-notes.md Outdated
- JSON transaction decomposition now includes a `weight` field which provides
the transaction's exact weight. This is included in REST /rest/tx/ and
/rest/block/ endpoints when in json mode. This is also included in getblock
(with verbosity=2), listsinceblock, listtransactions, and getrawtransaction

This comment has been minimized.

@promag

promag Mar 27, 2018

Member

Nit, endpoints and comments could be inside back ticks.

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2018-03-weight branch Mar 28, 2018

@jnewbery
Copy link
Member

jnewbery left a comment

One comment on the help text.

src/rpc/rawtransaction.cpp Outdated
@@ -94,6 +94,7 @@ UniValue getrawtransaction(const JSONRPCRequest& request)
" \"hash\" : \"id\", (string) The transaction hash (differs from txid for witness transactions)\n"
" \"size\" : n, (numeric) The serialized transaction size\n"
" \"vsize\" : n, (numeric) The virtual transaction size (differs from size for witness transactions)\n"
" \"weight\" : n, (numeric) The transaction's weight (between vsize*4 and vsize*4 + 3)\n"

This comment has been minimized.

@jnewbery

jnewbery Mar 29, 2018

Member

I think this is wrong. From BIP 141: Virtual transaction size is defined as Transaction weight / 4 (rounded up to the next integer). So weight is between vsize*4 - 3 and vsize*4. Same for help text below.

It may be better to switch the order and then change the help text for vsize to be weight / 4 (rounded up to the next integer)*

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Apr 7, 2018

Needs rebase (oh, release note only) and reply to @jnewbery's comment

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2018-03-weight branch to 9e50c33 Apr 13, 2018

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

TheBlueMatt commented Apr 13, 2018

Fixed @jnewbery's bug and rebased.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Apr 13, 2018

re-utACK 9e50c33

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Apr 17, 2018

utACK 9e50c33

@jonasschnelli jonasschnelli merged commit 9e50c33 into bitcoin:master Apr 17, 2018

1 check passed

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

jonasschnelli added a commit that referenced this pull request Apr 17, 2018

Merge #12791: Expose a transaction's weight via RPC
9e50c33 Note new weight field in release-notes. (Matt Corallo)
d0d9112 Test new weight field in p2p_segwit (Matt Corallo)
2874709 Expose a transaction's weight via RPC (Matt Corallo)

Pull request description:

  This seems like an obvious oversight.

Tree-SHA512: defd047de34fb06a31f589e1a4eef68fcae85095cc67b7c8fb434237bb40300d7f3f97e852d3e7226330e26b96943846b7baf6da0cfc79db8d56e9c1f7848ad9
@@ -94,6 +94,7 @@ UniValue getrawtransaction(const JSONRPCRequest& request)
" \"hash\" : \"id\", (string) The transaction hash (differs from txid for witness transactions)\n"
" \"size\" : n, (numeric) The serialized transaction size\n"
" \"vsize\" : n, (numeric) The virtual transaction size (differs from size for witness transactions)\n"
" \"weight\" : n, (numeric) The transaction's weight (between vsize*4-3 and vsize*4)\n"

This comment has been minimized.

@jnewbery

jnewbery Apr 17, 2018

Member

Would be clearer with a bit of spacing around the - operator (as you've done for the help text in decoderawtransaction)

Would also be clearer to return weight first and then define vsize based on weight (since vsize is derived from weight and not vice versa)

@preminem

This comment has been minimized.

Copy link

preminem commented Aug 27, 2018

Now still can't get weight through rpc. Anyone know the reason?

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Aug 27, 2018

In 0.17?

@preminem

This comment has been minimized.

Copy link

preminem commented Aug 27, 2018

it is 0.16.1

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.