RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo` #29954

kristapsk · 2024-04-24T20:29:37Z

Other node relay settings like fullrbf and minrelaytxfee are already returned, makes sense to add these two too.

DrahtBot · 2024-04-24T20:29:40Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	tdb3

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29086 (refactor: Simply include CTxMemPool::Options in CTxMemPool directly rather than duplicating definition by luke-jr)
#28676 ([WIP] Cluster mempool implementation by sdaftuar)

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.

src/rpc/mempool.cpp

andrewtoth · 2024-04-25T13:40:49Z

test/functional/mempool_accept.py

@@ -70,6 +70,8 @@ def run_test(self):
        node = self.nodes[0]
        self.wallet = MiniWallet(node)

+        assert_equal(node.getmempoolinfo()['permitbaremultisig'], False)


Could have a test with the True case?

I second this. It would add value to have tests that cover:

permitbaremultisig disabled

permitbaremultisig enabled

This might be accomplished through the use of a second node (self.num_nodes) or perhaps through stopping the node, modifying configuration with replace_in_config(), and restarting it. Typically num_nodes is to be minimized and start/stops avoided when possible, but I do not immediately see how different permitbaremultisig states could be tested without doing one of the two (see https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#general-test-writing-advice). Maybe it's a bit overkill, but we would want to know that the code behind the RPC on bitcoind still works if there are changes in the future.

Co-authored-by: Andrew Toth <andrewstoth@gmail.com>

tdb3

Concept ACK.
Thank you. Appears to work well. Seems helpful to me for clients to have this information provided through RPC. I took a quick look at other RPC calls (in https://bitcoincore.org/en/doc/27.0.0/), but didn't see any that had permitbaremultisig and maxdatacarriersize.

Recently, Issue #29912 was opened, which discusses implementing a formal specification for the RPC API. RPC API changes have the potential to adjust the meaning, fields/types, and/or values used in or returned by the API (see also #29845 (comment) for a recent discussion on a type change). Overall, it's a reasonable idea to be very careful when changing the RPC API, as there is a ripple effect to downstream clients and can cause issues/bugs for them.

That being said, the specific change in this PR seems pretty low risk to me, since it is adding two items to the existing result object returned, rather than changing the type returned, the meaning/value, or the other fields. Per https://github.com/bitcoin/bitcoin/blob/master/doc/JSON-RPC-interface.md#versioning, we already declare that RPC can change in major releases. I wrote this comment to increase awareness in case others have input.

This PR:

$ src/bitcoin-cli getmempoolinfo
{
  "loaded": true,
  "size": 0,
  "bytes": 0,
  "usage": 32,
  "total_fee": 0.00000000,
  "maxmempool": 300000000,
  "mempoolminfee": 0.00001000,
  "minrelaytxfee": 0.00001000,
  "incrementalrelayfee": 0.00001000,
  "unbroadcastcount": 0,
  "fullrbf": false,
  "permitbaremultisig": true,
  "maxdatacarriersize": 83
}
$ curl --user __cookie__ --data-binary '{"id": "curltest", "method": "getmempoolinfo", "params": []}' -H 'content-type: text/plain;' http://127.0.0.1:38332/
Enter host password for user '__cookie__':
{"result":{"loaded":true,"size":0,"bytes":0,"usage":32,"total_fee":0.00000000,"maxmempool":300000000,"mempoolminfee":0.00001000,"minrelaytxfee":0.00001000,"incrementalrelayfee":0.00001000,"unbroadcastcount":0,"fullrbf":false,"permitbaremultisig":true,"maxdatacarriersize":83},"error":null,"id":"curltest"}

Master branch (3aaf732 at the the time):

$ src/bitcoin-cli getmempoolinfo
{
  "loaded": true,
  "size": 0,
  "bytes": 0,
  "usage": 32,
  "total_fee": 0.00000000,
  "maxmempool": 300000000,
  "mempoolminfee": 0.00001000,
  "minrelaytxfee": 0.00001000,
  "incrementalrelayfee": 0.00001000,
  "unbroadcastcount": 0,
  "fullrbf": false
}
$ curl --user __cookie__ --data-binary '{"id": "curltest", "method": "getmempoolinfo", "params": []}' -H 'content-type: text/plain;' http://127.0.0.1:38332/
Enter host password for user '__cookie__':
{"result":{"loaded":true,"size":0,"bytes":0,"usage":32,"total_fee":0.00000000,"maxmempool":300000000,"mempoolminfee":0.00001000,"minrelaytxfee":0.00001000,"incrementalrelayfee":0.00001000,"unbroadcastcount":0,"fullrbf":false},"error":null,"id":"curltest"}

Also left some inline comments/suggestions.

tdb3 · 2024-04-28T21:49:03Z

test/functional/mempool_accept.py

@@ -70,6 +70,8 @@ def run_test(self):
        node = self.nodes[0]
        self.wallet = MiniWallet(node)

+        assert_equal(node.getmempoolinfo()['permitbaremultisig'], False)


I second this. It would add value to have tests that cover:

permitbaremultisig disabled

permitbaremultisig enabled

This might be accomplished through the use of a second node (self.num_nodes) or perhaps through stopping the node, modifying configuration with replace_in_config(), and restarting it. Typically num_nodes is to be minimized and start/stops avoided when possible, but I do not immediately see how different permitbaremultisig states could be tested without doing one of the two (see https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#general-test-writing-advice). Maybe it's a bit overkill, but we would want to know that the code behind the RPC on bitcoind still works if there are changes in the future.

luke-jr · 2024-05-07T15:46:21Z

src/rpc/mempool.cpp

@@ -677,6 +677,8 @@ UniValue MempoolInfoToJSON(const CTxMemPool& pool)
    ret.pushKV("incrementalrelayfee", ValueFromAmount(pool.m_incremental_relay_feerate.GetFeePerK()));
    ret.pushKV("unbroadcastcount", uint64_t{pool.GetUnbroadcastTxs().size()});
    ret.pushKV("fullrbf", pool.m_full_rbf);
+    ret.pushKV("permitbaremultisig", pool.m_permit_bare_multisig);
+    ret.pushKV("maxdatacarriersize", (pool.m_max_datacarrier_bytes ? *pool.m_max_datacarrier_bytes : 0));


Is there a reason to name it something different here? Especially while the option is still broken and doesn't actually limit datacarriers?

I was expecting this comment from you. :)

Named it same way as existing startup / config option (-datacarriersize), but I'm open to other names, as in fact it only limits OP_RETURN. What name seems to be good from Knots perspective? It would be good to not unnecessary break compatibility between Core and Knots.

Well, you added "max" in front. I guess it makes sense, but might be surprising.

It's not clear what compatibility would mean before Core has fixed it to actually work.

I slightly prefer it being called datacarriersize to match the config option, but don't feel strongly

luke-jr · 2024-05-08T14:30:05Z

I wonder if a sub-object would be a good idea before we start adding more settings here. There's at least also m_expiry that would make sense to add too.

luke-jr · 2024-05-08T15:02:48Z

Actually, since these don't change on their own... maybe a separate RPC altogether? (later extended to allow some changes?)

glozow

This seems useful, and makes sense as a field of getmempoolinfo like the others

glozow · 2024-05-08T17:05:10Z

src/rpc/mempool.cpp

@@ -677,6 +677,8 @@ UniValue MempoolInfoToJSON(const CTxMemPool& pool)
    ret.pushKV("incrementalrelayfee", ValueFromAmount(pool.m_incremental_relay_feerate.GetFeePerK()));
    ret.pushKV("unbroadcastcount", uint64_t{pool.GetUnbroadcastTxs().size()});
    ret.pushKV("fullrbf", pool.m_full_rbf);
+    ret.pushKV("permitbaremultisig", pool.m_permit_bare_multisig);
+    ret.pushKV("maxdatacarriersize", (pool.m_max_datacarrier_bytes ? *pool.m_max_datacarrier_bytes : 0));


I slightly prefer it being called datacarriersize to match the config option, but don't feel strongly

tdb3 · 2024-05-10T00:08:54Z

I wonder if a sub-object would be a good idea before we start adding more settings here. There's at least also m_expiry that would make sense to add too.

Implementing as a sub-object makes sense to me as well. If it is decided to adjust this RPC, then I'm thinking we'd also need a -deprecatedrpc option to retain the existing behavior, so we don't break RPC for downstream clients.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo` #29954

RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo` #29954

kristapsk commented Apr 24, 2024

DrahtBot commented Apr 24, 2024 •

edited

andrewtoth Apr 25, 2024

tdb3 Apr 28, 2024 •

edited

tdb3 left a comment •

edited

tdb3 Apr 28, 2024 •

edited

luke-jr May 7, 2024

kristapsk May 7, 2024

luke-jr May 8, 2024

glozow May 8, 2024

luke-jr commented May 8, 2024

luke-jr commented May 8, 2024

glozow left a comment

glozow May 8, 2024

tdb3 commented May 10, 2024

tdb3 commented May 10, 2024

kristapsk commented May 10, 2024

kristapsk commented May 10, 2024

RPC: Return permitbaremultisig and maxdatacarriersize in getmempoolinfo #29954

Are you sure you want to change the base?

RPC: Return permitbaremultisig and maxdatacarriersize in getmempoolinfo #29954

Conversation

kristapsk commented Apr 24, 2024

DrahtBot commented Apr 24, 2024 • edited

Code Coverage

Reviews

Conflicts

andrewtoth Apr 25, 2024

Choose a reason for hiding this comment

tdb3 Apr 28, 2024 • edited

Choose a reason for hiding this comment

tdb3 left a comment • edited

Choose a reason for hiding this comment

tdb3 Apr 28, 2024 • edited

Choose a reason for hiding this comment

luke-jr May 7, 2024

Choose a reason for hiding this comment

kristapsk May 7, 2024

Choose a reason for hiding this comment

luke-jr May 8, 2024

Choose a reason for hiding this comment

glozow May 8, 2024

Choose a reason for hiding this comment

luke-jr commented May 8, 2024

luke-jr commented May 8, 2024

glozow left a comment

Choose a reason for hiding this comment

glozow May 8, 2024

Choose a reason for hiding this comment

tdb3 commented May 10, 2024

tdb3 commented May 10, 2024

kristapsk commented May 10, 2024

kristapsk commented May 10, 2024

RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo` #29954

RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo` #29954

DrahtBot commented Apr 24, 2024 •

edited

tdb3 Apr 28, 2024 •

edited

tdb3 left a comment •

edited

tdb3 Apr 28, 2024 •

edited