Expose a transaction's weight via RPC

TheBlueMatt commented at 3:22 pm on March 26, 2018: member

This seems like an obvious oversight.

TheBlueMatt force-pushed on Mar 26, 2018

fanquake added the label RPC/REST/ZMQ on Mar 26, 2018

practicalswift commented at 3:38 pm on March 26, 2018: contributor

Concept ACK

Nice find!

jamesob commented at 4:01 pm on March 26, 2018: member

utACK https://github.com/bitcoin/bitcoin/pull/12791/commits/4fef4096cb1ad62d4b2a9c4cf821cff0406d3757

MarcoFalke commented at 4:11 pm on March 26, 2018: member

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

laanwj commented at 4:14 pm on March 26, 2018: member

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

promag commented at 5:15 pm on March 26, 2018: member

utACK 4fef409.

TheBlueMatt commented at 5:20 pm on March 26, 2018: member

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 commented at 5:33 pm on March 26, 2018: member

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 commented at 9:10 pm on March 26, 2018: member

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: https://github.com/bitcoin/bips/commit/c2213ed1fdabc671c8b3c46b3f7dcfedacf7dafb#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: https://github.com/bitcoin/bitcoin/blob/0a018431c447bbf18bdaa6a1037aad6a87c1294a/test/functional/p2p_segwit.py#L934

jnewbery commented at 1:23 pm on March 27, 2018: member

Tested ACK 4fef4096cb1ad62d4b2a9c4cf821cff0406d3757.

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

promag commented at 1:41 pm on March 27, 2018: member

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 commented at 9:24 pm on March 27, 2018: member

Added release notes, and p2p_segwit update.

in test/functional/p2p_segwit.py:939 in 3b2bb97383 outdated

937+        assert_equal(raw_tx["vsize"], math.ceil(weight / 4))
938+        assert_equal(raw_tx["weight"], weight)
939         assert_equal(len(raw_tx["vin"][0]["txinwitness"]), 1)
940         assert_equal(raw_tx["vin"][0]["txinwitness"][0], hexlify(witness_program).decode('ascii'))
941-        assert(vsize != raw_tx["size"])
942+        assert(weight / 4 != raw_tx["size"])

jnewbery commented at 9:29 pm on March 27, 2018:

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:

0assert weight / 4 != raw_tx["size"]

in test/functional/p2p_segwit.py:15 in 3b2bb97383 outdated

11@@ -12,6 +12,7 @@
12 from test_framework.key import CECKey, CPubKey
13 import time
14 import random
15+import math

promag commented at 9:41 pm on March 27, 2018:

nit, sort.

in doc/release-notes.md:80 in 3b2bb97383 outdated

73@@ -74,6 +74,11 @@ RPC changes
74   add `label` fields to returned JSON objects that previously only had
75   `account` fields.
76 - `sendmany` now shuffles outputs to improve privacy, so any previously expected behavior with regards to output ordering can no longer be relied upon.
77+- JSON transaction decomposition now includes a `weight` field which provides
78+  the transaction's exact weight. This is included in REST /rest/tx/ and
79+  /rest/block/ endpoints when in json mode. This is also included in getblock
80+  (with verbosity=2), listsinceblock, listtransactions, and getrawtransaction

promag commented at 9:43 pm on March 27, 2018:

Nit, endpoints and comments could be inside back ticks.

promag commented at 9:45 pm on March 27, 2018: member

utACK 3b2bb973.

TheBlueMatt force-pushed on Mar 28, 2018

ken2812221 approved

in src/rpc/rawtransaction.cpp:97 in e3c355c09d outdated

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

jnewbery commented at 6:47 pm on March 29, 2018:

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)*

jnewbery commented at 6:50 pm on March 29, 2018: member

One comment on the help text.

laanwj commented at 5:04 pm on April 7, 2018: member

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

Expose a transaction's weight via RPC 2874709a9f

Test new weight field in p2p_segwit d0d9112b79

Note new weight field in release-notes. 9e50c337c7

TheBlueMatt force-pushed on Apr 13, 2018

TheBlueMatt commented at 7:18 pm on April 13, 2018: member

Fixed @jnewbery’s bug and rebased.

laanwj commented at 8:07 pm on April 13, 2018: member

re-utACK 9e50c337c7126e05dad2f2f5a53ef882ab0b0330

jonasschnelli commented at 6:04 pm on April 17, 2018: contributor

utACK 9e50c337c7126e05dad2f2f5a53ef882ab0b0330

jonasschnelli merged this on Apr 17, 2018

jonasschnelli closed this on Apr 17, 2018

jonasschnelli referenced this in commit 3a8a4dc4a1 on Apr 17, 2018

in src/rpc/rawtransaction.cpp:97 in 9e50c337c7

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

jnewbery commented at 6:29 pm on April 17, 2018:

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)

sipa added the label Needs release notes on Aug 14, 2018

preminem commented at 9:51 am on August 27, 2018: none

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

luke-jr commented at 9:53 am on August 27, 2018: member

In 0.17?

preminem commented at 10:13 am on August 27, 2018: none

it is 0.16.1

fanquake removed the label Needs release note on Mar 20, 2019

DrahtBot locked this on Dec 16, 2021

Expose a transaction’s weight via RPC #12791