rpc: Return fee and vsize from testmempoolaccept#19940
Merged
fanquake merged 2 commits intobitcoin:masterfrom Sep 19, 2020
Merged
rpc: Return fee and vsize from testmempoolaccept#19940fanquake merged 2 commits intobitcoin:masterfrom
fanquake merged 2 commits intobitcoin:masterfrom
Conversation
Member
Author
|
@rajarshimaitra @MarcoFalke @jnewbery as we discussed on #19093 :) |
glozow
commented
Sep 11, 2020
instagibbs
reviewed
Sep 11, 2020
Member
instagibbs
left a comment
instagibbs
left a comment
There was a problem hiding this comment.
concept ACK, would like less-magical tests
Contributor
|
utACK 4943a1392c33cd5ac985e753d9cc728b794c5752 (once release notes have been updated) |
fjahr
reviewed
Sep 15, 2020
Contributor
fjahr
left a comment
fjahr
left a comment
There was a problem hiding this comment.
utACK 4943a1392c33cd5ac985e753d9cc728b794c5752
I agree that tests could be better but I would also be fine with merging as is.
Return fee and vsize if tx would pass ATMP.
glozow
force-pushed
the
rpc-testmempoolaccept-fee
branch
from
4943a13 to
2233a93
Compare
Member
|
ACK 4189588 thanks it's a solid improvement for the next contributor wanting to touch it :) |
glozow
force-pushed
the
rpc-testmempoolaccept-fee
branch
from
4189588 to
23c35bf
Compare
Member
Author
|
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 |
Member
|
reACK 23c35bf |
Contributor
|
utACK 23c35bf |
Contributor
|
utACK 23c35bf |
maflcko
reviewed
Sep 19, 2020
Member
maflcko
left a comment
maflcko
left a comment
There was a problem hiding this comment.
Nice. Concept ACK! Left some feedback on the tests.
| locktime=node.getblockcount() + 2000, # Can be anything | ||
| ))['hex'] | ||
| tx.deserialize(BytesIO(hex_str_to_bytes(raw_tx_final))) | ||
| fee_expected = int(coin['amount']) - output_amount |
Member
There was a problem hiding this comment.
why does this need to truncate the digits of the btc amount?
| txid_0 = tx.rehash() | ||
| self.check_mempool_result( | ||
| result_expected=[{'txid': txid_0, 'allowed': True}], | ||
| result_expected=[{'txid': txid_0, 'allowed': True, 'vsize': tx.get_vsize(), 'fees': {'base': Decimal(str(fee))}}], |
Member
There was a problem hiding this comment.
unrelated nit: What happens if fee was made a Decimal as opposed to float when defined?
| fee_expected = int(coin['amount']) - output_amount | ||
| self.check_mempool_result( | ||
| result_expected=[{'txid': tx.rehash(), 'allowed': True}], | ||
| result_expected=[{'txid': tx.rehash(), 'allowed': True, 'vsize': tx.get_vsize(), 'fees': {'base': Decimal(str(fee_expected))}}], |
Member
There was a problem hiding this comment.
Same
(This is my fault for using float in the first place, but now that Decimal is imported, might as well use it everywhere)
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Sep 20, 2020
23c35bf [test] add get_vsize util for more programmatic testing (gzhao408) 2233a93 [rpc] Return fee and vsize from testmempoolaccept (codeShark149) Pull request description: From bitcoin#19093 and resolves bitcoin#19057. Difference from bitcoin#19093: return `vsize` and `fees` object (similar to `getmempoolentry`) when the test accept is successful. Updates release-notes.md. ACKs for top commit: jnewbery: utACK 23c35bf fjahr: utACK 23c35bf instagibbs: reACK bitcoin@23c35bf Tree-SHA512: dcb81b7b817a4684e9076bc5d427a6f2d549d2edc66544e718260c4b5f8f1d5ae1d47b754175e9f0c8a3bd8371ce116c2dca0583588d513a7d733d5d614f2b04
This was referenced Sep 26, 2020
fanquake
added a commit
that referenced
this pull request
Oct 14, 2020
88197b0 [doc] release notes for max fee checking (gzhao408) c201d73 style and nits for fee-checking in BroadcastTransaction (gzhao408) Pull request description: Pretty trivial... addresses some tiny comments from #19339. Also fixes a docs typo from #19940 and adds a release note about the error message change for testmempoolaccept. ACKs for top commit: achow101: ACK 88197b0 MarcoFalke: cr re-ACK 88197b0 Tree-SHA512: fff16d731426b9b4db5222df02633983402f4c7241551eec98bb1554145dbdc132f40ed8ca4abd5edcebe1f4d1e879fb6d11bd91730604f6552c10cdf65706a1
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Oct 15, 2020
88197b0 [doc] release notes for max fee checking (gzhao408) c201d73 style and nits for fee-checking in BroadcastTransaction (gzhao408) Pull request description: Pretty trivial... addresses some tiny comments from bitcoin#19339. Also fixes a docs typo from bitcoin#19940 and adds a release note about the error message change for testmempoolaccept. ACKs for top commit: achow101: ACK 88197b0 MarcoFalke: cr re-ACK 88197b0 Tree-SHA512: fff16d731426b9b4db5222df02633983402f4c7241551eec98bb1554145dbdc132f40ed8ca4abd5edcebe1f4d1e879fb6d11bd91730604f6552c10cdf65706a1
deadalnix
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Oct 8, 2021
Summary: Return fee and size if tx would pass ATMP. This is a backport of [[bitcoin/bitcoin#19940 | core#19940]] D1635 introduced `CTransaction.billable_size`, so use it instead of adding a new identical `get_size` method. Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10279
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
From #19093 and resolves #19057.
Difference from #19093: return
vsizeandfeesobject (similar togetmempoolentry) when the test accept is successful. Updates release-notes.md.