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

IPGlider · 2018-04-17T18:17:54Z

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 · 2018-04-18T06:10:29Z

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

IPGlider · 2018-04-18T11:34:44Z

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

Empact · 2018-04-18T13:54:13Z

Squash please.

maflcko · 2018-04-18T13:57:37Z

src/rpc/blockchain.cpp

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.

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 · 2018-04-19T14:02:24Z

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.

promag · 2018-04-19T14:11:34Z

doc/release-notes.md

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

jnewbery · 2018-05-08T18:14:31Z

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 · 2018-05-12T09:59:32Z

@jnewbery Changed the order and rebased.

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

promag · 2018-05-12T10:17:02Z

@IPGlider See

bitcoin/src/wallet/rpcwallet.cpp

Line 1763 in 81d0d56

    
           if (IsDeprecatedRPCEnabled("accounts")) entry.pushKV("account", strSentAccount);

IPGlider · 2018-05-12T11:39:25Z

Thanks @promag.

Hidden size behind a deprecation flag. Any further suggestions?

laanwj · 2018-07-05T17:20:04Z

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 · 2018-07-07T11:57:00Z

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

maflcko · 2018-07-09T13:48:44Z

utACK 52606162ae1cc3c954de7a37411910784317315a

maflcko · 2018-07-10T20:31:35Z

re-utACK 305330e818a410d5bb1f8fc2b6fea0442c3a06a0

Empact · 2018-07-11T05:03:57Z

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

jnewbery

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

jnewbery · 2018-07-11T12:33:19Z

src/rpc/blockchain.cpp

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:

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

jnewbery · 2018-07-11T12:33:45Z

doc/release-notes.md

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

DrahtBot · 2018-11-17T04:34:40Z

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 · 2018-11-21T21:57:16Z

tACK 3bc922d.

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

maflcko · 2018-11-22T18:32:49Z

re-utACK 3bc922d

DrahtBot · 2019-03-02T14:18:19Z

Needs rebase

maflcko · 2019-03-02T14:20:14Z

doc/release-notes.md

+  getrawmempool RPCs, to be consistent with the returned value and other
+  RPCs such as getrawtransaction, vsize has been added and size is now
+  deprecated. size will only be returned if bitcoind is started with
+  -deprecatedrpc=size.


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

fanquake · 2019-03-22T01:04:00Z

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

e16b6a7 rpc: Rename size to vsize in mempool related calls (Miguel Herranz) Pull request description: #13008 rebased on `master`, with release notes split out. > 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. ACKs for commit e16b6a: MarcoFalke: re-utACK e16b6a7 jnewbery: utACK e16b6a7 Tree-SHA512: ce95260fe7f280eacf4ff70bfffe02315c3a521b3b462a34e72a05b90733f40cc473319ac2df05d3e3c12cb7b1fbf2a1bbea632a8f979fff94207854cdbd494d

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

maflcko reviewed Apr 18, 2018

View reviewed changes

IPGlider force-pushed the rename-size-to-vsize branch from 0252836 to 8a6d5fe Compare April 18, 2018 15:19

promag reviewed Apr 19, 2018

View reviewed changes

doc/release-notes.md Outdated

Copy link

Contributor

promag Apr 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

IPGlider force-pushed the rename-size-to-vsize branch 3 times, most recently from 2038f18 to 98e9e9a Compare April 19, 2018 14:55

IPGlider force-pushed the rename-size-to-vsize branch from 98e9e9a to 1aaad6d Compare May 12, 2018 09:58

IPGlider force-pushed the rename-size-to-vsize branch from 1aaad6d to 5260616 Compare May 12, 2018 11:35

DrahtBot mentioned this pull request Jun 30, 2018

RPC: Add new "getzmqnotifications" method #13570

Merged

DrahtBot added the Needs rebase label Jul 9, 2018

IPGlider force-pushed the rename-size-to-vsize branch from 5260616 to 305330e Compare July 10, 2018 17:31

DrahtBot removed the Needs rebase label Jul 10, 2018

jnewbery reviewed Jul 11, 2018

View reviewed changes

IPGlider force-pushed the rename-size-to-vsize branch from 305330e to 9271166 Compare July 11, 2018 17:50

DrahtBot added the Needs rebase label Aug 13, 2018

rpc: Rename size to vsize in mempool related calls

3bc922d

IPGlider force-pushed the rename-size-to-vsize branch from 9271166 to 3bc922d Compare November 17, 2018 00:00

DrahtBot removed the Needs rebase label Nov 17, 2018

DrahtBot added the Needs rebase label Mar 2, 2019

maflcko reviewed Mar 2, 2019

View reviewed changes

fanquake mentioned this pull request Mar 22, 2019

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

Merged

fanquake closed this Mar 22, 2019

laanwj removed the Needs rebase label Oct 24, 2019

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

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

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

Uh oh!

Conversation

IPGlider commented Apr 17, 2018

Uh oh!

fanquake commented Apr 18, 2018

Uh oh!

IPGlider commented Apr 18, 2018

Uh oh!

Empact commented Apr 18, 2018

Uh oh!

maflcko Apr 18, 2018

Choose a reason for hiding this comment

Uh oh!

promag Apr 18, 2018

Choose a reason for hiding this comment

Uh oh!

IPGlider commented Apr 19, 2018

Uh oh!

promag Apr 19, 2018

Choose a reason for hiding this comment

Uh oh!

jnewbery commented May 8, 2018

Uh oh!

IPGlider commented May 12, 2018

Uh oh!

promag commented May 12, 2018

Uh oh!

IPGlider commented May 12, 2018

Uh oh!

laanwj commented Jul 5, 2018

Uh oh!

luke-jr commented Jul 7, 2018

Uh oh!

maflcko commented Jul 9, 2018

Uh oh!

maflcko commented Jul 10, 2018

Uh oh!

Empact commented Jul 11, 2018

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery Jul 11, 2018

Choose a reason for hiding this comment

Uh oh!

jnewbery Jul 11, 2018

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Nov 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jnewbery commented Nov 21, 2018

Uh oh!

maflcko commented Nov 22, 2018

Uh oh!

DrahtBot commented Mar 2, 2019

Uh oh!

maflcko Mar 2, 2019

Choose a reason for hiding this comment

Uh oh!

fanquake commented Mar 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

DrahtBot commented Nov 17, 2018 •

edited

Loading