rpc: Return total fee in getmempoolinfo

MarcoFalke commented at 7:57 pm on January 15, 2021: member

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

MarcoFalke added the label RPC/REST/ZMQ on Jan 15, 2021

MarcoFalke renamed this:
~~rpc: Return total fee in mempool~~
rpc: Return total fee in getmempoolinfo
on Jan 15, 2021

DrahtBot commented at 2:56 am on January 16, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20383 (Avoid signed integer overflow and invalid integer negation when loading malformed mempool.dat files by practicalswift)
#18017 (txmempool: split epoch logic into class by ajtowns)

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.

MarcoFalke force-pushed on Jan 16, 2021

in src/txmempool.h:731 in fa1c01e16c outdated

724@@ -724,6 +725,12 @@ class CTxMemPool
725         return totalTxSize;
726     }
727 
728+    CAmount GetTotalFee() const EXCLUSIVE_LOCKS_REQUIRED(cs)
729+    {
730+        AssertLockHeld(cs);
731+        return m_total_fee;

jonasschnelli commented at 8:37 am on January 16, 2021:

wouldn’t this also work as a std::atomic<CAmount> and avoid the locking in the getter?

MarcoFalke commented at 9:16 am on January 16, 2021:

Yes, but for the rpc to return atomic results, the lock needs to be taken. Also, it is already taken in the current code, so this is cheaper than an atomic.

jonasschnelli commented at 8:38 am on January 16, 2021: contributor

Concept ACK, light code review. Looks good.

0xB10C commented at 5:26 pm on January 17, 2021: member

Concept ACK

meshcollider commented at 10:30 am on January 22, 2021: contributor

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.

MarcoFalke force-pushed on Jan 22, 2021

MarcoFalke commented at 11:36 am on January 22, 2021: member

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

0xB10C commented at 9:23 am on January 28, 2021: member

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.

MarcoFalke commented at 9:38 am on January 28, 2021: member

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.

rpc: Return total fee in mempool

Also, add missing lock annotations

fa362064e3

MarcoFalke commented at 9:44 am on January 28, 2021: member

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

MarcoFalke force-pushed on Jan 28, 2021

luke-jr referenced this in commit dc33e1b2bb on Jan 29, 2021

laanwj added this to the "Blockers" column in a project

in src/rpc/blockchain.cpp:1524 in fa362064e3

1520@@ -1520,6 +1521,7 @@ static RPCHelpMan getmempoolinfo()
1521                         {RPCResult::Type::NUM, "size", "Current tx count"},
1522                         {RPCResult::Type::NUM, "bytes", "Sum of all virtual transaction sizes as defined in BIP 141. Differs from actual serialized size because witness data is discounted"},
1523                         {RPCResult::Type::NUM, "usage", "Total memory usage for the mempool"},
1524+                        {RPCResult::Type::STR_AMOUNT, "total_fee", "Total fees for the mempool in " + CURRENCY_UNIT + ", ignoring modified fees through prioritizetransaction"},

glozow commented at 11:49 pm on February 4, 2021:

nit: I’d perhaps call this total_base_fees if there’s a possibility of reporting modified totals in the future

0                        {RPCResult::Type::STR_AMOUNT, "total_base_fees", "Total base fees for the mempool in " + CURRENCY_UNIT + ", ignoring modified fees through prioritizetransaction"},

MarcoFalke commented at 7:18 am on February 5, 2021:

I don’t think the sum of the modified fees has any meaning. They are used to prioritize individual transactions (and thus their packages) for mining. It is not a fee that can be redeemed in any way and in most cases it will be an arbitrary number.

Though, if they are added they can be called total_modified_fee, without breaking any clients. I’ll leave as is for now.

glozow commented at 5:48 pm on February 5, 2021:

Yes, makes sense!

in doc/REST-interface.md:114 in fa362064e3

115-* size : (numeric) the number of transactions in the TX mempool
116-* bytes : (numeric) size of the TX mempool in bytes
117-* usage : (numeric) total TX mempool memory usage
118-* maxmempool : (numeric) maximum memory usage for the mempool in bytes
119-* mempoolminfee : (numeric) minimum feerate (BTC per KB) for tx to be accepted
120+Refer to the `getmempoolinfo` RPC for documentation of the fields.

glozow commented at 0:11 am on February 5, 2021:

nit: perhaps add a link?

MarcoFalke commented at 7:18 am on February 5, 2021:

Happy to take a patch if you provide one, either in this pull or as a follow-up.

glozow commented at 0:13 am on February 5, 2021: member

ACK https://github.com/bitcoin/bitcoin/pull/20944/commits/fa362064e383163a2585ffbc71ac1ea3bcc92663 🧸

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

0old_fee = node.getmempoolinfo()['total_fee']
1node.sendrawtransaction(raw_tx_0)
2assert_equal(old_fee + fee, node.getmempoolinfo()['total_fee'])
3assert_equal(node.getmempoolinfo()['total_fee'], sum(v['fees']['base'] for k, v in self.nodes[0].getrawmempool(verbose=True).items()))
4print(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.

in test/functional/mempool_persist.py:72 in fa362064e3

68@@ -69,13 +69,19 @@ def run_test(self):
69         assert_equal(len(self.nodes[0].getrawmempool()), 5)
70         assert_equal(len(self.nodes[1].getrawmempool()), 5)
71 
72+        total_fee_old = self.nodes[0].getmempoolinfo()['total_fee']

jnewbery commented at 11:21 am on February 5, 2021:

Perhaps could do a check here that total_fee_old is a positive integer?

MarcoFalke commented at 3:52 pm on February 5, 2021:

Could also be checked via CHECK_NONFATAL on the server side.

jnewbery commented at 11:22 am on February 5, 2021: member

ACK fa362064e383163a2585ffbc71ac1ea3bcc92663

One suggestion for the test if you happen to touch this branch again. Not at all important - feel free to ignore.

achow101 commented at 7:19 pm on February 8, 2021: member

ACK fa362064e383163a2585ffbc71ac1ea3bcc92663

MarcoFalke merged this on Feb 8, 2021

MarcoFalke closed this on Feb 8, 2021

MarcoFalke deleted the branch on Feb 8, 2021

sidhujag referenced this in commit 0ba269cf76 on Feb 8, 2021

laanwj removed this from the "Blockers" column in a project

DrahtBot locked this on Aug 16, 2022

rpc: Return total fee in getmempoolinfo #20944

Conflicts