rpc: Rename size to vsize in mempool related calls

IPGlider commented at 6:17 pm on April 17, 2018: contributor

In getmempoolancestors, getmempooldescendants, getmempoolentry and getrawmempool RPCs size returns the virtual transaction size as defined in BIP 141. Renaming it to vsize makes it consistent with returned value and other calls such as getrawtransaction.

Related to #11218.

fanquake added the label RPC/REST/ZMQ on Apr 17, 2018

fanquake commented at 6:10 am on April 18, 2018: member

@IPGlider You’ll also need to update test/functional/mining_prioritisetransaction.py. See the Travis build failure.

IPGlider commented at 11:34 am on April 18, 2018: contributor

@fanquake Thanks for letting me know. I have now fixed the tests. I will squash the commits if requested.

Empact commented at 1:54 pm on April 18, 2018: member

Squash please.

in src/rpc/blockchain.cpp:402 in da4b5f2570 outdated

382@@ -383,7 +383,7 @@ void entryToJSON(UniValue &info, const CTxMemPoolEntry &e)
383 {
384     AssertLockHeld(mempool.cs);
385 
386-    info.pushKV("size", (int)e.GetTxSize());
387+    info.pushKV("vsize", (int)e.GetTxSize());

MarcoFalke commented at 1:57 pm on April 18, 2018:

I imagine this key-value pair is heavily relied upon. I’d prefer if we included both keys “size” and “vsize” with the same value for at least one release.

promag commented at 2:05 pm on April 18, 2018:

Agree with @MarcoFalke, this is a breaking change.

If you feel that field size should be removed then you must deprecate it first, and wait for the next release to actually remove it. Not sure it’s worth it as the description is clear.

IPGlider force-pushed on Apr 18, 2018

IPGlider commented at 2:02 pm on April 19, 2018: contributor

I have kept the size field for compatibility as requested by @MarcoFalke. I also added a deprecation message and squashed the commits. Let me know if anything else is needed.

in doc/release-notes.md:73 in 8a6d5fe065 outdated

69@@ -70,6 +70,7 @@ RPC changes
70   /rest/block/ endpoints when in json mode. This is also included in `getblock`
71   (with verbosity=2), `listsinceblock`, `listtransactions`, and
72   `getrawtransaction` RPC commands.
73+- In `getmempoolancestors`, `getmempooldescendants`, `getmempoolentry` and `getrawmempool` RPCs `size` has been renamed to `vsize` to be consistent with the returned value and other RPCs such as `getrawtransaction`.

promag commented at 2:11 pm on April 19, 2018:

Please update note, something like In ... `vsize` was added and `size` is now deprecated ...

IPGlider force-pushed on Apr 19, 2018

jnewbery commented at 6:14 pm on May 8, 2018: member

Concept ACK. A couple of nitty suggestions:

return vsize before size (since the definition for size depends on vsize)
hide size behind a deprecation flag so it can be removed cleanly in the next version.

Needs rebase.

IPGlider force-pushed on May 12, 2018

IPGlider commented at 9:59 am on May 12, 2018: contributor

@jnewbery Changed the order and rebased.

Could you please point me towards an example of how to hide size behind a deprecation flag?

promag commented at 10:17 am on May 12, 2018: member

@IPGlider See https://github.com/bitcoin/bitcoin/blob/81d0d565ab5551110ef465484b061922452aabe9/src/wallet/rpcwallet.cpp#L1763

IPGlider force-pushed on May 12, 2018

IPGlider commented at 11:39 am on May 12, 2018: contributor

Thanks @promag.

Hidden size behind a deprecation flag. Any further suggestions?

laanwj commented at 5:20 pm on July 5, 2018: member

I’m not sure about this anymore - this came up in some discussions at the building on bitcoin conference. In practice almost no one knows what vsize is, and everyone that knows what it is likely already knows that all the sizes are virtual now.

luke-jr commented at 11:57 am on July 7, 2018: member

People probably shouldn’t use "size" if they don’t know about vsize, so I think renaming it here still makes sense…

MarcoFalke commented at 1:48 pm on July 9, 2018: member

utACK 52606162ae1cc3c954de7a37411910784317315a

DrahtBot added the label Needs rebase on Jul 9, 2018

IPGlider force-pushed on Jul 10, 2018

DrahtBot removed the label Needs rebase on Jul 10, 2018

MarcoFalke commented at 8:31 pm on July 10, 2018: member

re-utACK 305330e818a410d5bb1f8fc2b6fea0442c3a06a0

Empact commented at 5:03 am on July 11, 2018: member

Concept ACK - I’m in favor since we show virtual size in the gui as of at least: #12580

in src/rpc/blockchain.cpp:355 in 305330e818 outdated

350@@ -351,7 +351,8 @@ static UniValue getdifficulty(const JSONRPCRequest& request)
351 
352 static std::string EntryDescriptionString()
353 {
354-    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"
355+    return "    \"vsize\" : 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"
356+           "    \"size\" : n,             (numeric) same as vsize (DEPRECATED)\n"

jnewbery commented at 12:33 pm on July 11, 2018:

I think this should document that size is only returned if deprecatedrpc=size is in the config or passed as an argument to bitcoind. Perhaps:

0           "    \"size\" : n,             (numeric) (DEPRECATED) same as vsize. Only returned if bitcoind is started with -deprecatedrpc=size\n"
1           "                              `size` will be completely removed in v0.18.\n"

in doc/release-notes.md:87 in 305330e818 outdated

80@@ -81,6 +81,10 @@ RPC changes
81    `fee`, `modifiedfee`, `ancestorfee` and `descendantfee`.
82 - The new RPC `getzmqnotifications` returns information about active ZMQ
83   notifications.
84+- In `getmempoolancestors`, `getmempooldescendants`, `getmempoolentry` and
85+  `getrawmempool` RPCs, to be consistent with the returned value and other RPCs
86+  such as `getrawtransaction`, `vsize` has been added and `size` is now
87+  deprecated.

jnewbery commented at 12:33 pm on July 11, 2018:

Should document that size will only be returned if bitcoind is started with -deprecatedrpc=size.

jnewbery commented at 12:34 pm on July 11, 2018: member

Looks fine, but documentation could be improved for the deprecatedrpc option.

IPGlider force-pushed on Jul 11, 2018

DrahtBot added the label Needs rebase on Aug 13, 2018

rpc: Rename size to vsize in mempool related calls 3bc922d79c

IPGlider force-pushed on Nov 17, 2018

DrahtBot removed the label Needs rebase on Nov 17, 2018

DrahtBot commented at 4:34 am on November 17, 2018: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14802 (rpc: faster getblockstats using BlockUndo data by FelixWeis)
#14649 (RPC: add weight to mempool entry output by luke-jr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jnewbery commented at 9:57 pm on November 21, 2018: member

tACK 3bc922d79c43113ad9face78b2c6208e38c72d47.

Thanks for addressing my review comments. Sorry I didn’t ACK earlier. I didn’t notice that you’d updated the branch.

MarcoFalke commented at 6:32 pm on November 22, 2018: member

re-utACK 3bc922d79c

DrahtBot commented at 2:18 pm on March 2, 2019: member

DrahtBot added the label Needs rebase on Mar 2, 2019

in doc/release-notes.md:205 in 3bc922d79c

197@@ -198,6 +198,12 @@ RPC
198 - A new `submitheader` RPC allows submitting block headers independently
199   from their block.  This is likely only useful for testing.
200 
201+- In getmempoolancestors, getmempooldescendants, getmempoolentry and
202+  getrawmempool RPCs, to be consistent with the returned value and other
203+  RPCs such as getrawtransaction, vsize has been added and size is now
204+  deprecated. size will only be returned if bitcoind is started with
205+  -deprecatedrpc=size.

MarcoFalke commented at 2:20 pm on March 2, 2019:

Should be moved to a separate file ./doc/release-notes-13008.md

fanquake commented at 1:04 am on March 22, 2019: member

I’ve rebased and fixed up the release notes in #15637.

fanquake closed this on Mar 22, 2019

MarcoFalke referenced this in commit e14cd04abb on Mar 26, 2019

laanwj removed the label Needs rebase on Oct 24, 2019

vijaydasmp referenced this in commit 68c792220a on Oct 5, 2021

vijaydasmp referenced this in commit 72b7ecd663 on Oct 12, 2021

vijaydasmp referenced this in commit 7627e2b014 on Nov 10, 2021

vijaydasmp referenced this in commit 893504beeb on Dec 5, 2021

vijaydasmp referenced this in commit 68c20f08c1 on Dec 6, 2021

vijaydasmp referenced this in commit 7b82200393 on Dec 6, 2021

vijaydasmp referenced this in commit 4b95057642 on Dec 13, 2021

vijaydasmp referenced this in commit 6f422f961a on Dec 13, 2021

DrahtBot locked this on Dec 16, 2021

rpc: Rename size to vsize in mempool related calls #13008

Conflicts