Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Jan 15, 2021

This avoids having to loop over the whole mempool to query each entry's fee

@maflcko maflcko changed the title rpc: Return total fee in mempool rpc: Return total fee in getmempoolinfo Jan 15, 2021
@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 16, 2021

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, 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.

@jonasschnelli
Copy link
Contributor

Concept ACK, light code review. Looks good.

@0xB10C
Copy link
Contributor

0xB10C commented Jan 17, 2021

Concept ACK

Copy link
Contributor

@meshcollider meshcollider left a 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.

@maflcko maflcko force-pushed the 2101-rpcMempoolTotalFee branch 2 times, most recently from fa878bb to fa79be7 Compare January 22, 2021 11:35
@maflcko
Copy link
Member Author

maflcko commented Jan 22, 2021

Good point. Force pushed to make the test more useful.

Copy link
Contributor

@0xB10C 0xB10C left a 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.

@maflcko
Copy link
Member Author

maflcko commented Jan 28, 2021

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
@maflcko
Copy link
Member Author

maflcko commented Jan 28, 2021

Didn't realize this had only one ACK after my force push, so I went ahead and fixed the documentation.

@maflcko maflcko force-pushed the 2101-rpcMempoolTotalFee branch from fa79be7 to fa36206 Compare January 28, 2021 09:44
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jan 29, 2021
Github-Pull: bitcoin#20944
Rebased-From: fa36206 (partial)
Diff-minimised
Copy link
Member

@glozow glozow left a 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.

Copy link
Contributor

@jnewbery jnewbery left a 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']
Copy link
Contributor

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?

Copy link
Member Author

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.

@achow101
Copy link
Member

achow101 commented Feb 8, 2021

ACK fa36206

@maflcko maflcko merged commit b09ad73 into bitcoin:master Feb 8, 2021
@maflcko maflcko deleted the 2101-rpcMempoolTotalFee branch February 8, 2021 19:38
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 8, 2021
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants