rpc: Return fee and vsize from testmempoolaccept #19940

pull glozow wants to merge 2 commits into bitcoin:master from glozow:rpc-testmempoolaccept-fee changing 7 files +53 −15

glozow commented at 2:08 PM on September 11, 2020: member

From #19093 and resolves #19057.

Difference from #19093: return vsize and fees object (similar to getmempoolentry) when the test accept is successful. Updates release-notes.md.
glozow commented at 2:11 PM on September 11, 2020: member

@rajarshimaitra @MarcoFalke @jnewbery as we discussed on #19093 :)

in doc/release-notes.md:106 in 4943a1392c outdated

 101 | @@ -102,6 +102,9 @@ will trigger BIP 125 (replace-by-fee) opt-in. (#11413)
 102 |    option `-deprecatedrpc=banscore` is used. The `banscore` field will be fully
 103 |    removed in the next major release. (#19469)
 104 |  
 105 | +- The `testmempoolaccept` RPC returns `vsize` and a `fee` object with the `base` fee
 106 | +  if the transaction passes validation. (#19093)

glozow commented at 2:12 PM on September 11, 2020:

I should update this now that it'd be a different PR number. Will incorporate with any review comments so feel free to nit :)

glozow commented at 1:35 PM on September 16, 2020:

updated

fanquake added the label RPC/REST/ZMQ on Sep 11, 2020
fanquake added the label Validation on Sep 11, 2020

in test/functional/mempool_accept.py:95 in 4943a1392c outdated

  91 | @@ -91,7 +92,7 @@ def run_test(self):
  92 |          tx.deserialize(BytesIO(hex_str_to_bytes(raw_tx_0)))
  93 |          txid_0 = tx.rehash()
  94 |          self.check_mempool_result(
  95 | -            result_expected=[{'txid': txid_0, 'allowed': True}],
  96 | +            result_expected=[{'txid': txid_0, 'allowed': True, 'vsize': 110, 'fees': {'base': Decimal(str(fee))}}],

instagibbs commented at 3:06 PM on September 11, 2020:

I'd rather the vsize was computed rather than hard-coded. Should be simple enough to make a helper function to do this in python, and would be nice to use for tests going forward.

in test/functional/mempool_accept.py:108 in 4943a1392c outdated

 104 | @@ -104,7 +105,7 @@ def run_test(self):
 105 |          ))['hex']
 106 |          tx.deserialize(BytesIO(hex_str_to_bytes(raw_tx_final)))
 107 |          self.check_mempool_result(
 108 | -            result_expected=[{'txid': tx.rehash(), 'allowed': True}],
 109 | +            result_expected=[{'txid': tx.rehash(), 'allowed': True, 'vsize': 188, 'fees': { 'base': Decimal(str(int(coin['amount']) - 0.025))}}],

instagibbs commented at 3:08 PM on September 11, 2020:

Can we also try to make these fee calculations more programmatic? Otherwise it's a burden on future test writers.

instagibbs commented at 3:08 PM on September 11, 2020: member

concept ACK, would like less-magical tests
jnewbery commented at 3:14 PM on September 11, 2020: member

utACK 4943a1392c33cd5ac985e753d9cc728b794c5752 (once release notes have been updated)

in src/rpc/rawtransaction.cpp:881 in 4943a1392c outdated

 877 | @@ -878,6 +878,11 @@ static UniValue testmempoolaccept(const JSONRPCRequest& request)
 878 |                          {
 879 |                              {RPCResult::Type::STR_HEX, "txid", "The transaction hash in hex"},
 880 |                              {RPCResult::Type::BOOL, "allowed", "If the mempool allows this tx to be inserted"},
 881 | +                            {RPCResult::Type::NUM, "vsize", "virtual transaction size as defined in BIP 141. This is different from actual serialized size for witness transactions as witness data is discounted."},

fjahr commented at 10:55 PM on September 15, 2020:

nit: Should maybe add a comment that these are not always present. See below for example. Also, capitalize the first letter.

glozow commented at 1:34 PM on September 16, 2020:

Done

fjahr commented at 11:01 PM on September 15, 2020: member

utACK 4943a1392c33cd5ac985e753d9cc728b794c5752

I agree that tests could be better but I would also be fine with merging as is.
[rpc] Return fee and vsize from testmempoolaccept
Return fee and vsize if tx would pass ATMP.
2233a93a10
glozow force-pushed on Sep 16, 2020
[test] add get_vsize util for more programmatic testing 23c35bf005
instagibbs commented at 2:26 PM on September 16, 2020: member

ACK https://github.com/bitcoin/bitcoin/pull/19940/commits/418958892d22b552c44f7801fd2f233c92b8f5a8

thanks it's a solid improvement for the next contributor wanting to touch it :)
glozow force-pushed on Sep 16, 2020
glozow commented at 2:31 PM on September 16, 2020: member

thank you @instagibbs! I'm so sorry but I just realized I forgot to apply it to p2p_segwit.py 😅 need you to take another look oops... @jnewbery @fjahr this is ready for review again - updated release notes + rpc and added a util for getting vsize in test_framework/messages.py
instagibbs commented at 2:40 PM on September 16, 2020: member

reACK https://github.com/bitcoin/bitcoin/pull/19940/commits/23c35bf0059bd6270218e0b732959e9c754f9812
fjahr commented at 3:14 PM on September 16, 2020: member

utACK 23c35bf
jnewbery commented at 4:46 PM on September 16, 2020: member

utACK 23c35bf0059bd6270218e0b732959e9c754f9812
fanquake merged this on Sep 19, 2020
fanquake closed this on Sep 19, 2020

in test/functional/mempool_accept.py:108 in 23c35bf005

 105 | -            outputs=[{node.getnewaddress(): 0.025}],
 106 | +            outputs=[{node.getnewaddress(): output_amount}],
 107 |              locktime=node.getblockcount() + 2000,  # Can be anything
 108 |          ))['hex']
 109 |          tx.deserialize(BytesIO(hex_str_to_bytes(raw_tx_final)))
 110 | +        fee_expected = int(coin['amount']) - output_amount

MarcoFalke commented at 7:37 AM on September 19, 2020:

why does this need to truncate the digits of the btc amount?

in test/functional/mempool_accept.py:95 in 23c35bf005

  91 | @@ -91,20 +92,22 @@ def run_test(self):
  92 |          tx.deserialize(BytesIO(hex_str_to_bytes(raw_tx_0)))
  93 |          txid_0 = tx.rehash()
  94 |          self.check_mempool_result(
  95 | -            result_expected=[{'txid': txid_0, 'allowed': True}],
  96 | +            result_expected=[{'txid': txid_0, 'allowed': True, 'vsize': tx.get_vsize(), 'fees': {'base': Decimal(str(fee))}}],

MarcoFalke commented at 7:38 AM on September 19, 2020:

unrelated nit: What happens if fee was made a Decimal as opposed to float when defined?

in test/functional/mempool_accept.py:110 in 23c35bf005

 108 |          ))['hex']
 109 |          tx.deserialize(BytesIO(hex_str_to_bytes(raw_tx_final)))
 110 | +        fee_expected = int(coin['amount']) - output_amount
 111 |          self.check_mempool_result(
 112 | -            result_expected=[{'txid': tx.rehash(), 'allowed': True}],
 113 | +            result_expected=[{'txid': tx.rehash(), 'allowed': True, 'vsize': tx.get_vsize(), 'fees': {'base': Decimal(str(fee_expected))}}],

MarcoFalke commented at 7:39 AM on September 19, 2020:

Same

(This is my fault for using float in the first place, but now that Decimal is imported, might as well use it everywhere)

MarcoFalke commented at 7:42 AM on September 19, 2020: member

Nice. Concept ACK! Left some feedback on the tests.
sidhujag referenced this in commit 4d146837e5 on Sep 20, 2020
fanquake referenced this in commit e21b824386 on Oct 14, 2020
sidhujag referenced this in commit 6d323e260d on Oct 15, 2020
deadalnix referenced this in commit d5381a5e68 on Oct 8, 2021
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

RPC/REST/ZMQ Validation

Linked (view graph)

#19057 testmempoolaccept should return the fee #19093 RPC: Return transaction fee from testmempoolaccept #20011 test: Convert amounts in testmempoolaccept test from float to decimal #20109 Release notes and followups from 19339 #20164 [test] undo truncation of digits in btc amount