rpc: Return fee and vsize from testmempoolaccept

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

rpc: Return fee and vsize from testmempoolaccept #19940