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

kristapsk commented at 8:29 pm on April 24, 2024: contributor

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

DrahtBot commented at 8:29 pm on April 24, 2024: contributor

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, theStack

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:

#30592 (Remove mempoolfullrbf by instagibbs)
#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.

DrahtBot added the label RPC/REST/ZMQ on Apr 24, 2024

in src/rpc/mempool.cpp:704 in 5b4c7a2212 outdated

700@@ -699,6 +701,8 @@ static RPCHelpMan getmempoolinfo()
701                 {RPCResult::Type::NUM, "incrementalrelayfee", "minimum fee rate increment for mempool limiting or replacement in " + CURRENCY_UNIT + "/kvB"},
702                 {RPCResult::Type::NUM, "unbroadcastcount", "Current number of transactions that haven't passed initial broadcast yet"},
703                 {RPCResult::Type::BOOL, "fullrbf", "True if the mempool accepts RBF without replaceability signaling inspection"},
704+                {RPCResult::Type::BOOL, "permitbaremultisig", "True if the mempool accepts bare multisig transactions"},

andrewtoth commented at 1:39 pm on April 25, 2024:

To avoid any confusion

0                {RPCResult::Type::BOOL, "permitbaremultisig", "True if the mempool accepts transactions with bare multisig outputs"},

in test/functional/mempool_accept.py:75 in 5b4c7a2212 outdated

69@@ -70,6 +70,8 @@ def run_test(self):
70         node = self.nodes[0]
71         self.wallet = MiniWallet(node)
72 
73+        assert_equal(node.getmempoolinfo()['permitbaremultisig'], False)

andrewtoth commented at 1:40 pm on April 25, 2024:

Could have a test with the True case?

tdb3 commented at 9:49 pm on April 28, 2024:

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.

ajtowns commented at 6:33 pm on May 20, 2024:

Just adding the true check in mempool_datacarrier seems like it would be easy?

kristapsk commented at 1:32 am on May 21, 2024:

@ajtowns Good idea, can add test simple way without adding much more code. 84e03ab48e0c832ab2c0d8e6babb6563a0ee92dc

tdb3 commented at 9:51 pm on April 28, 2024: contributor

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 (review) 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:

 0$ src/bitcoin-cli getmempoolinfo
 1{
 2  "loaded": true,
 3  "size": 0,
 4  "bytes": 0,
 5  "usage": 32,
 6  "total_fee": 0.00000000,
 7  "maxmempool": 300000000,
 8  "mempoolminfee": 0.00001000,
 9  "minrelaytxfee": 0.00001000,
10  "incrementalrelayfee": 0.00001000,
11  "unbroadcastcount": 0,
12  "fullrbf": false,
13  "permitbaremultisig": true,
14  "maxdatacarriersize": 83
15}
16$ curl --user __cookie__ --data-binary '{"id": "curltest", "method": "getmempoolinfo", "params": []}' -H 'content-type: text/plain;' http://127.0.0.1:38332/
17Enter host password for user '__cookie__':
18{"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 (3aaf7328eb656b642e5f0f74f3e4d51645a1d0ab at the the time):

 0$ src/bitcoin-cli getmempoolinfo
 1{
 2  "loaded": true,
 3  "size": 0,
 4  "bytes": 0,
 5  "usage": 32,
 6  "total_fee": 0.00000000,
 7  "maxmempool": 300000000,
 8  "mempoolminfee": 0.00001000,
 9  "minrelaytxfee": 0.00001000,
10  "incrementalrelayfee": 0.00001000,
11  "unbroadcastcount": 0,
12  "fullrbf": false
13}
14$ curl --user __cookie__ --data-binary '{"id": "curltest", "method": "getmempoolinfo", "params": []}' -H 'content-type: text/plain;' http://127.0.0.1:38332/
15Enter host password for user '__cookie__':
16{"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.

DrahtBot added the label CI failed on Apr 28, 2024

DrahtBot removed the label CI failed on May 4, 2024

in src/rpc/mempool.cpp:681 in f562477973 outdated

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

luke-jr commented at 3:46 pm on May 7, 2024:

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

kristapsk commented at 6:39 pm on May 7, 2024:

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.

luke-jr commented at 2:31 pm on May 8, 2024:

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.

glozow commented at 5:05 pm on May 8, 2024:

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

luke-jr commented at 2:30 pm on May 8, 2024: member

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 commented at 3:02 pm on May 8, 2024: member

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

glozow commented at 5:06 pm on May 8, 2024: member

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

tdb3 commented at 0:08 am on May 10, 2024: contributor

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.

See also: #29845 (review)

tdb3 commented at 0:11 am on May 10, 2024: contributor

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

This option also makes sense to me. Maybe something like getmempoolpolicy or similar?

kristapsk commented at 6:07 pm on May 10, 2024: contributor

IMHO #29086 should be merged first, it’s a good refactor, I will rebase then, so please review that one.

kristapsk commented at 6:08 pm on May 10, 2024: contributor

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

I’m not sure it’s worth breaking compatibility here. Stuff like minrelaytxfee and mempoolminfee is checked by a lots of wallet software, Lightning nodes, etc.

DrahtBot added the label Needs rebase on May 15, 2024

kristapsk force-pushed on May 16, 2024

kristapsk commented at 1:09 am on May 16, 2024: contributor

Rebased.

DrahtBot removed the label Needs rebase on May 16, 2024

DrahtBot added the label CI failed on May 16, 2024

DrahtBot commented at 4:55 am on May 16, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/25029460423

kristapsk force-pushed on May 16, 2024

DrahtBot removed the label CI failed on May 16, 2024

theStack commented at 4:21 pm on May 17, 2024: contributor

Concept ACK

Can you squash the two commits? (Currently compiling d06227fd6b87b5a427441a212c04a0ba64edc142 fails).

RPC: Return permitbaremultisig and maxdatacarriersize in getmempoolinfo

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

3a6560527e

kristapsk force-pushed on May 17, 2024

Test that permitbaremultisig is enabled by default 84e03ab48e

DrahtBot added the label CI failed on Jun 18, 2024

DrahtBot removed the label CI failed on Jun 18, 2024

DrahtBot added the label CI failed on Sep 10, 2024

DrahtBot removed the label CI failed on Sep 14, 2024

RPC: Return permitbaremultisig and maxdatacarriersize in getmempoolinfo #29954

Code Coverage

Reviews

Conflicts

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