-
Notifications
You must be signed in to change notification settings - Fork 38.7k
rpc: Return total fee in getmempoolinfo #20944
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
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
fa19a80 to
fa1c01e
Compare
|
Concept ACK, light code review. Looks good. |
|
Concept ACK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK fa1c01e16c3e168960a1cf6c816f05e1bbaaf8ad
I think the test is too weak however, it doesn't test behaviour at all, e.g. the test would pass even if total_fee always returned 0.
fa878bb to
fa79be7
Compare
|
Good point. Force pushed to make the test more useful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested ACK fa79be7aef9c75c78a4f6177051f867cbb623a9e. Tested both the RPC call and the REST interface (/rest/mempool/info.json).
Could have some total_fee documentation in the REST-interface.md here.
EDIT: Unrelated to this PR, but it seems like documentation for unbroadcastcount is missing from REST-interface.md too.
I'll leave fixing the docs as a follow-up. Ideally the docs are auto-generated, which is on my roadmap. |
Also, add missing lock annotations
|
Didn't realize this had only one ACK after my force push, so I went ahead and fixed the documentation. |
fa79be7 to
fa36206
Compare
Github-Pull: bitcoin#20944 Rebased-From: fa36206 (partial) Diff-minimised
glozow
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK fa36206 🧸
I would probably split the mempool and rpc commits, but nbd. Mempool code looks correct. I sanity-checked by adding some stuff to functional tests like
old_fee = node.getmempoolinfo()['total_fee']
node.sendrawtransaction(raw_tx_0)
assert_equal(old_fee + fee, node.getmempoolinfo()['total_fee'])
assert_equal(node.getmempoolinfo()['total_fee'], sum(v['fees']['base'] for k, v in self.nodes[0].getrawmempool(verbose=True).items()))
print(node.getmempoolinfo()['total_fee'])Tried it out with RBFing in mempool_accept.py, after expired txns in mempool_expiry, and expired txns in mempool_expiry.py, all worked for me.
jnewbery
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK fa36206
One suggestion for the test if you happen to touch this branch again. Not at all important - feel free to ignore.
| assert_equal(len(self.nodes[0].getrawmempool()), 5) | ||
| assert_equal(len(self.nodes[1].getrawmempool()), 5) | ||
|
|
||
| total_fee_old = self.nodes[0].getmempoolinfo()['total_fee'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps could do a check here that total_fee_old is a positive integer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could also be checked via CHECK_NONFATAL on the server side.
|
ACK fa36206 |
This avoids having to loop over the whole mempool to query each entry's fee