Skip to content

RPC: add weight to mempool entry output #11256

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.

Sign up for GitHub

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

Jump to bottom
Closed
esotericnonsense wants to merge 4 commits into from
Closed

RPC: add weight to mempool entry output #11256

esotericnonsense wants to merge 4 commits into from

Conversation

@esotericnonsense
Copy link
Contributor

@esotericnonsense esotericnonsense commented Sep 6, 2017 •
edited
Loading

Tested against master using the REST api (/rest/mempool/contents), simple addition of a field.

Personal use case is for fee analysis software.

@promag
Copy link
Contributor

promag commented Sep 6, 2017

No test affected 😞 care to improve by asserting the new field in the relevant RPC's 😉?

@laanwj
Copy link
Member

laanwj commented Sep 6, 2017

Concept ACK

@maflcko
Copy link
Member

maflcko commented Sep 6, 2017

I guess you need to update the documentation as well.

@esotericnonsense esotericnonsense force-pushed the 2017-09-add-weight-to-mempool-entry branch from 375dfca to ed75a33 Compare September 7, 2017 00:54
@esotericnonsense
Copy link
Contributor Author

esotericnonsense commented Sep 7, 2017 •
edited
Loading

Tests are failing.
txid2 and txid3 work as expected.
txid1 fails on both my test and #11203 (add wtxid to mempool entry output).

Do not merge as is.

@ajtowns
Copy link
Contributor

ajtowns commented Sep 8, 2017

txid1 is failing because at that point "tx" is actually referring to the input to txid1, not the transaction for txid1. It works fine for me if I add

tx = FromHex(CTransaction(), self.nodes[0].gettransaction(txid1)['hex'])

prior to the assert_equal lines (and uncomment them obviously).

@esotericnonsense esotericnonsense force-pushed the 2017-09-add-weight-to-mempool-entry branch from c0e9c8c to d4b0d81 Compare September 8, 2017 19:28
@esotericnonsense
Copy link
Contributor Author

esotericnonsense commented Sep 8, 2017 •
edited
Loading

Doh. You're absolutely right. Fixed.

The final commit 'Refactor segwit 3-tx-chain' changes all references to 'tx' to 'tx/tx1/tx2/tx3' in order to clarify that. It has a large diff and can be dropped if necessary (only affects code style).

I have also rebased on master at 3255d63.

Should be good to go now.

@morcos
Copy link
Contributor

morcos commented Sep 11, 2017

Concept ACK, but it turns out GetTxSize is not actually what we claim it is. See the calculation of GetVirtualTransactionSize which potentially includes number of sig ops in the calculation.

I thinke the right path forward is:

  • a small BIP documenting this usage of virtual transaction size (which is used almost everywhere in our code
  • documentation update pointing to this new BIP instead of 141 for defining virtual size
  • this PR makes even more sense given this lack of easy conversion, but I think the newly added tests should at least be documented to note that they only work in the case that sig ops don't factor in, and perhaps we should add a test with a tx that has more sigops to show what happens in that case.

@luke-jr
Copy link
Member

luke-jr commented Nov 10, 2017

@morcos AFAIK that's exclusively used for node policy, and as such isn't a topic for standardisation...?

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Nov 11, 2017
@esotericnonsense @luke-jr
RPC: add weight to mempool entry output
2a88c1e 
Github-Pull: bitcoin#11256
Rebased-From: 8385101
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Nov 11, 2017
@esotericnonsense @luke-jr
qa: Add RPC tests for weight in mempool entry
812bf31 
Github-Pull: bitcoin#11256
Rebased-From: 9e338bc
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Nov 11, 2017
@esotericnonsense @luke-jr
qa: Add more RPC tests for wtxid in mempool entry, clarify comment
a36907e 
Github-Pull: bitcoin#11256
Rebased-From: 22086cc
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Nov 11, 2017
@esotericnonsense @luke-jr
qa: Refactor segwit 3-tx-chain functional test to clarify use of 'tx'…
30b2696 
… variable

Github-Pull: bitcoin#11256
Rebased-From: d4b0d81
@ajtowns ajtowns mentioned this pull request Jan 16, 2018
Merged
luke-jr
luke-jr reviewed Mar 11, 2018
View reviewed changes
test/functional/segwit.py

# Check that weight and sizei (actually vsize) are properly reported in mempool entry (txid1)
assert_equal(self.nodes[0].getmempoolentry(txid1)["size"], (self.nodes[0].getmempoolentry(txid1)["weight"] + 3) // 4)
assert_equal(self.nodes[0].getmempoolentry(txid1)["weight"], len(tx1.serialize())*3 + len(tx1.serialize_with_witness()))
Copy link
Member

@luke-jr luke-jr Mar 11, 2018

Choose a reason for hiding this comment

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

serialize needs to be made serialize_without_witness

@laanwj
Copy link
Member

laanwj commented Aug 7, 2018

Needs rebase and review comments addressed.

@luke-jr luke-jr mentioned this pull request Nov 3, 2018
Closed
@meshcollider
Copy link
Contributor

meshcollider commented Nov 4, 2018

Closing this, please see #14649

@fanquake fanquake mentioned this pull request Aug 19, 2019
Merged
maflcko pushed a commit that referenced this pull request Aug 20, 2019
Merge #16647: rpc: add weight to getmempoolentry output
70b12af 
17d178f doc: add release-notes for getmempoolentry weight field addition (fanquake)
9c9cc2b qa: Add RPC tests for weight in mempool entry (Daniel Edgecumbe)
54aaa78 RPC: add weight to mempool entry output (Daniel Edgecumbe)

Pull request description:

  Rebase of #14649 (which itself was a rebase of #11256).

  Squash the two test related commits, and swapped out `size` usage for `vsize`.

  Added a commit with release notes.

ACKs for top commit:
  emilengler:
    Concept ACK 17d178f
  instagibbs:
    utACK 17d178f
  meshcollider:
    utACK 17d178f

Tree-SHA512: 1d354c9837e0ad0afa40325de9329b9e62688d5eab4d9e1cf9b46d8ae29d08f42d903ab37a41751c2ea8f9034231b21095881b1f5d911cb542b8b06bc85dc7cd
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 22, 2019
@sidhujag
Merge bitcoin#16647: rpc: add weight to getmempoolentry output
4bc7011 
17d178f doc: add release-notes for getmempoolentry weight field addition (fanquake)
9c9cc2b qa: Add RPC tests for weight in mempool entry (Daniel Edgecumbe)
54aaa78 RPC: add weight to mempool entry output (Daniel Edgecumbe)

Pull request description:

  Rebase of bitcoin#14649 (which itself was a rebase of bitcoin#11256).

  Squash the two test related commits, and swapped out `size` usage for `vsize`.

  Added a commit with release notes.

ACKs for top commit:
  emilengler:
    Concept ACK 17d178f
  instagibbs:
    utACK bitcoin@17d178f
  meshcollider:
    utACK 17d178f

Tree-SHA512: 1d354c9837e0ad0afa40325de9329b9e62688d5eab4d9e1cf9b46d8ae29d08f42d903ab37a41751c2ea8f9034231b21095881b1f5d911cb542b8b06bc85dc7cd
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@luke-jr luke-jr luke-jr left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

RPC/REST/ZMQ

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@esotericnonsense @promag @laanwj @maflcko @ajtowns @morcos @luke-jr @meshcollider @fanquake