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: add weight to mempool entry output #11256

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@esotericnonsense
Contributor

esotericnonsense commented Sep 6, 2017

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

Personal use case is for fee analysis software.

@promag

This comment has been minimized.

Show comment
Hide comment
@promag

promag Sep 6, 2017

Member

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

Member

promag commented Sep 6, 2017

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Sep 6, 2017

Member

Concept ACK

Member

laanwj commented Sep 6, 2017

Concept ACK

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Sep 6, 2017

Member

I guess you need to update the documentation as well.

Member

MarcoFalke commented Sep 6, 2017

I guess you need to update the documentation as well.

@esotericnonsense

This comment has been minimized.

Show comment
Hide comment
@esotericnonsense

esotericnonsense Sep 7, 2017

Contributor

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.

Contributor

esotericnonsense commented Sep 7, 2017

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

This comment has been minimized.

Show comment
Hide comment
@ajtowns

ajtowns Sep 8, 2017

Contributor

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).

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

This comment has been minimized.

Show comment
Hide comment
@esotericnonsense

esotericnonsense Sep 8, 2017

Contributor

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.

Contributor

esotericnonsense commented Sep 8, 2017

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

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Sep 11, 2017

Member

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.
Member

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

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Nov 10, 2017

Member

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

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 added a commit to bitcoinknots/bitcoin that referenced this pull request Nov 11, 2017

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Nov 11, 2017

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Nov 11, 2017

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Nov 11, 2017

# 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()))

This comment has been minimized.

@luke-jr

luke-jr Mar 11, 2018

Member

serialize needs to be made serialize_without_witness

@luke-jr

luke-jr Mar 11, 2018

Member

serialize needs to be made serialize_without_witness

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Aug 7, 2018

Member

Needs rebase and review comments addressed.

Member

laanwj commented Aug 7, 2018

Needs rebase and review comments addressed.

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