RPC: Return permitbaremultisig and maxdatacarriersize in getmempoolinfo #29954

pull kristapsk wants to merge 1 commits into bitcoin:master from kristapsk:getmempoolinfo-permitbaremultisig-maxdatacarriersize changing 3 files +18 −1
  1. 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.
  2. 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 & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29954.

    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

    No conflicts as of last run.

  3. DrahtBot added the label RPC/REST/ZMQ on Apr 24, 2024
  4. 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"},
  5. in test/functional/mempool_accept.py:77 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:

    1. permitbaremultisig disabled
    2. 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
  6. 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.

  7. DrahtBot added the label CI failed on Apr 28, 2024
  8. DrahtBot removed the label CI failed on May 4, 2024
  9. 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
  10. 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.
  11. 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?)
  12. 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
  13. 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)

  14. 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?

  15. 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.
  16. 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.

  17. DrahtBot added the label Needs rebase on May 15, 2024
  18. kristapsk force-pushed on May 16, 2024
  19. kristapsk commented at 1:09 am on May 16, 2024: contributor
    Rebased.
  20. DrahtBot removed the label Needs rebase on May 16, 2024
  21. DrahtBot added the label CI failed on May 16, 2024
  22. 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

  23. kristapsk force-pushed on May 16, 2024
  24. DrahtBot removed the label CI failed on May 16, 2024
  25. theStack commented at 4:21 pm on May 17, 2024: contributor

    Concept ACK

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

  26. kristapsk force-pushed on May 17, 2024
  27. DrahtBot added the label CI failed on Jun 18, 2024
  28. DrahtBot removed the label CI failed on Jun 18, 2024
  29. DrahtBot added the label CI failed on Sep 10, 2024
  30. DrahtBot removed the label CI failed on Sep 14, 2024
  31. achow101 requested review from tdb3 on Oct 15, 2024
  32. achow101 removed review request from tdb3 on Oct 15, 2024
  33. achow101 requested review from theStack on Oct 15, 2024
  34. achow101 commented at 2:52 pm on October 15, 2024: member
    Please rebase for cmake if you are still interested in working on this.
  35. kristapsk force-pushed on Oct 23, 2024
  36. kristapsk commented at 9:20 am on October 23, 2024: contributor
    Rebased.
  37. in src/rpc/mempool.cpp:687 in ff808d80d6 outdated 
    682@@ -683,6 +683,8 @@ UniValue MempoolInfoToJSON(const CTxMemPool& pool)
683     ret.pushKV("incrementalrelayfee", ValueFromAmount(pool.m_opts.incremental_relay_feerate.GetFeePerK()));
684     ret.pushKV("unbroadcastcount", uint64_t{pool.GetUnbroadcastTxs().size()});
685     ret.pushKV("fullrbf", pool.m_opts.full_rbf);
686+    ret.pushKV("permitbaremultisig", pool.m_opts.permit_bare_multisig);
687+    ret.pushKV("maxdatacarriersize", (pool.m_opts.max_datacarrier_bytes ? *pool.m_opts.max_datacarrier_bytes : 0));
    theStack commented at 10:11 pm on October 23, 2024:

    nit, a little shorter:

    0    ret.pushKV("maxdatacarriersize", pool.m_opts.max_datacarrier_bytes.value_or(0));
  38. in test/functional/mempool_datacarrier.py:16 in ff808d80d6 outdated 
    12@@ -13,7 +13,7 @@
13 )
14 from test_framework.test_framework import BitcoinTestFramework
15 from test_framework.test_node import TestNode
16-from test_framework.util import assert_raises_rpc_error
17+from test_framework.util import assert_equal, assert_raises_rpc_error
    theStack commented at 10:12 pm on October 23, 2024: 
    0from test_framework.util import (
1    assert_equal,
2    assert_raises_rpc_error,
3)

    (we prefer multi-line imports, see style guidelines in ./test/functional/README.md)

  39. theStack approved
  40. theStack commented at 10:16 pm on October 23, 2024: contributor

    ACK modulo nits

    As others have expressed before, I’d also slightly prefer to name the options exactly like their command line option (i.e. “datacarriersize” without the “max” prefix), but no blocker.

  41. maflcko commented at 8:38 am on October 24, 2024: member
    Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
  42. DrahtBot added the label Needs rebase on Nov 8, 2024
  43. kristapsk force-pushed on Nov 14, 2024
  44. RPC: Return permitbaremultisig and maxdatacarriersize in getmempoolinfo 
    Co-authored-by: Andrew Toth <andrewstoth@gmail.com>

Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
    d165ac8779
  45. kristapsk force-pushed on Nov 14, 2024
  46. kristapsk commented at 6:42 am on November 14, 2024: contributor
    Rebased and squashed commits.
  47. DrahtBot removed the label Needs rebase on Nov 14, 2024


kristapsk DrahtBot andrewtoth tdb3 ajtowns luke-jr glozow theStack achow101 maflcko

Labels
RPC/REST/ZMQ

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-22 00:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me