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

getrawmempool verbose mode should return "size" and "disksize" for segwit transactions #11218

Open
junderw opened this Issue Sep 2, 2017 · 6 comments

Comments

Projects
None yet
7 participants
@junderw
Copy link

junderw commented Sep 2, 2017

Describe the issue

getrawmempool verbose mode only returns "size" rather than "size" and "disksize" (the size on disk for segwit transactions)

Can you reliably reproduce the issue?

If so, please list the steps to reproduce below:

  1. Run getrawmempool with verbose set to true
  2. Notice it only gives size

Expected behaviour

It should show:

  1. effective feerate "size" (ceil(weight/4))
  2. disk size "disksize" (total number of bytes in the serialized segwit transaction. (since "disksize" and "size" will be equal for non-segwit transactions, "disksize" should only appear for segwit transactions)

Actual behaviour

It shows size only (effective size for fees ceil(weight/4))

Screenshots.

If the issue is related to the GUI, screenshots can be added to this issue via drag & drop.

What version of bitcoin-core are you using?

0.14.2

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Sep 2, 2017

The only thing that counts towards the block limit (and by extension, what matters for feerate) is vsize. That's also what is reported, as it's the only thing the mempool cares about.

@junderw

This comment has been minimized.

Copy link
Author

junderw commented Sep 3, 2017

I guess I'll take that as an "out of scope" response.
The mempool only cares about vsize, so "size" is always vsize... makes sense.

However, this is a "verbose" mode which imo should "give as much information as possible so the user/coder can decide what's useful and what's not"...

In regards to segwit transactions, the only missing info regarding the possible effect on the blockchain (for sites looking to record or watch these statistics) is how much disk space it will take up.

Thinking it through. You would only really need 2 of the 3, and the other one can be calculated.

Perhaps segwit transactions can have an extra key which is "disksize"... since non-segwit disksize == size, it can be removed.

@junderw junderw changed the title getrawmempool verbose mode should return "size" "vsize" (or weight) and "bsize" getrawmempool verbose mode should return "size" and "disksize" for segwit transactions Sep 3, 2017

@sdaftuar

This comment has been minimized.

Copy link
Member

sdaftuar commented Sep 6, 2017

I'm skeptical of disksize being a useful quantity, because that is implementation specific and not protocol-enforced; for instance if we implemented compression to store the transaction on disk, would we then return a smaller value for disksize?

Perhaps you mean "serialized size", which is well-defined because the serialization is part of the protocol, but we don't cache this in the mempool because we don't need it for anything. I'd prefer not to cache this in the mempool to avoid wasting memory, and I'm not sure it's worth reserializing the mempool contents to calculate this value whenever a user calls getrawmempool, because that rpc is already slow enough, and I don't know what the use case here would be... Note that users can always calculate the serialized size themselves by asking for the serialized transaction directly, eg with getrawtransaction.

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Sep 6, 2017

Perhaps we should however rename the "size" field to "vsize" there, for consistency?

@FelixWeis

This comment has been minimized.

Copy link
Contributor

FelixWeis commented Nov 9, 2017

+1 renaming it vsize

maybe +/-0.5 splitting it to weigth and adding disksize

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Mar 27, 2018

Agree that we should rename size -> vsize (perhaps behind deprecation flag).

There's a PR to expose transaction weight in getrawtransaction here: #12791. Perhaps we could do the same for getrawmempool

No need for disksize IMO.

Suggest tagging this as 'good first issue'

MarcoFalke added a commit that referenced this issue Mar 26, 2019

Merge #15637: rpc: Rename size to vsize in mempool related calls
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
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.