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: Rename size to vsize in mempool related calls #13008

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@IPGlider
Copy link

IPGlider commented Apr 17, 2018

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

This comment has been minimized.

Copy link
Member

fanquake commented Apr 18, 2018

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

@IPGlider

This comment has been minimized.

Copy link

IPGlider commented Apr 18, 2018

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

@Empact

This comment has been minimized.

Copy link
Member

Empact commented Apr 18, 2018

Squash please.

@@ -383,7 +383,7 @@ void entryToJSON(UniValue &info, const CTxMemPoolEntry &e)
{
AssertLockHeld(mempool.cs);

info.pushKV("size", (int)e.GetTxSize());
info.pushKV("vsize", (int)e.GetTxSize());

This comment has been minimized.

@MarcoFalke

MarcoFalke Apr 18, 2018

Member

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.

This comment has been minimized.

@promag

promag Apr 18, 2018

Member

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 IPGlider force-pushed the IPGlider:rename-size-to-vsize branch from 0252836 to 8a6d5fe Apr 18, 2018

@IPGlider

This comment has been minimized.

Copy link

IPGlider commented Apr 19, 2018

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.

@@ -70,6 +70,7 @@ RPC changes
/rest/block/ endpoints when in json mode. This is also included in `getblock`
(with verbosity=2), `listsinceblock`, `listtransactions`, and
`getrawtransaction` RPC commands.
- 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`.

This comment has been minimized.

@promag

promag Apr 19, 2018

Member

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

@IPGlider IPGlider force-pushed the IPGlider:rename-size-to-vsize branch 3 times, most recently from 2038f18 to 98e9e9a Apr 19, 2018

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented May 8, 2018

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 IPGlider force-pushed the IPGlider:rename-size-to-vsize branch from 98e9e9a to 1aaad6d May 12, 2018

@IPGlider

This comment has been minimized.

Copy link

IPGlider commented May 12, 2018

@jnewbery Changed the order and rebased.

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

@promag

This comment has been minimized.

Copy link
Member

promag commented May 12, 2018

@IPGlider See

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

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

@IPGlider

This comment has been minimized.

Copy link

IPGlider commented May 12, 2018

Thanks @promag.

Hidden size behind a deprecation flag. Any further suggestions?

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Jul 5, 2018

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

This comment has been minimized.

Copy link
Member

luke-jr commented Jul 7, 2018

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Jul 9, 2018

utACK 5260616

@IPGlider IPGlider force-pushed the IPGlider:rename-size-to-vsize branch from 5260616 to 305330e Jul 10, 2018

@DrahtBot DrahtBot removed the Needs rebase label Jul 10, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Jul 10, 2018

re-utACK 305330e

@Empact

This comment has been minimized.

Copy link
Member

Empact commented Jul 11, 2018

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

@jnewbery
Copy link
Member

jnewbery left a comment

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

@@ -351,7 +351,8 @@ static UniValue getdifficulty(const JSONRPCRequest& request)

static std::string EntryDescriptionString()
{
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"
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"
" \"size\" : n, (numeric) same as vsize (DEPRECATED)\n"

This comment has been minimized.

@jnewbery

jnewbery Jul 11, 2018

Member

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"
- In `getmempoolancestors`, `getmempooldescendants`, `getmempoolentry` and
`getrawmempool` RPCs, to be consistent with the returned value and other RPCs
such as `getrawtransaction`, `vsize` has been added and `size` is now
deprecated.

This comment has been minimized.

@jnewbery

jnewbery Jul 11, 2018

Member

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

@IPGlider IPGlider force-pushed the IPGlider:rename-size-to-vsize branch from 305330e to 9271166 Jul 11, 2018

@IPGlider IPGlider force-pushed the IPGlider:rename-size-to-vsize branch from 9271166 to 3bc922d Nov 17, 2018

@DrahtBot DrahtBot removed the Needs rebase label Nov 17, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Nov 17, 2018

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #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

This comment has been minimized.

Copy link
Member

jnewbery commented Nov 21, 2018

tACK 3bc922d.

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 22, 2018

re-utACK 3bc922d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment