fullrbf
and minrelaytxfee
are already returned, makes sense to add these two too.
permitbaremultisig
and maxdatacarriersize
in getmempoolinfo
#29954
fullrbf
and minrelaytxfee
are already returned, makes sense to add these two too.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29954.
See the guideline for information on the review process.
Type | Reviewers |
---|---|
ACK | ajtowns, maflcko, theStack |
Concept ACK | tdb3 |
Stale ACK | TheCharlatan |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
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"},
To avoid any confusion
0 {RPCResult::Type::BOOL, "permitbaremultisig", "True if the mempool accepts transactions with bare multisig outputs"},
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)
True
case?
I second this. It would add value to have tests that cover:
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.
mempool_datacarrier
seems like it would be easy?
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.
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));
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.
m_expiry
that would make sense to add too.
getmempoolinfo
like the others
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)
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?
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.
🚧 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.
Concept ACK
Can you squash the two commits? (Currently compiling d06227fd6b87b5a427441a212c04a0ba64edc142 fails).
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));
nit, a little shorter:
0 ret.pushKV("maxdatacarriersize", pool.m_opts.max_datacarrier_bytes.value_or(0));
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
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)
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.
Co-authored-by: Andrew Toth <andrewstoth@gmail.com>
Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
Co-authored-by: Anthony Towns <aj@erisian.com.au>
690@@ -691,6 +691,8 @@ UniValue MempoolInfoToJSON(const CTxMemPool& pool)
691 ret.pushKV("incrementalrelayfee", ValueFromAmount(pool.m_opts.incremental_relay_feerate.GetFeePerK()));
692 ret.pushKV("unbroadcastcount", uint64_t{pool.GetUnbroadcastTxs().size()});
693 ret.pushKV("fullrbf", true);
694+ ret.pushKV("permitbaremultisig", pool.m_opts.permit_bare_multisig);
695+ ret.pushKV("maxdatacarriersize", pool.m_opts.max_datacarrier_bytes.value_or(0));
nit: Seems a bit odd to use 0
and nullopt
interchangeably to mean the same (and require call sites to do the conversion manually). It would be easier to just flatten the optional and have a single dimension, directly translating two-way to the -datacarriersize
option.
Diff:
0diff --git a/src/kernel/mempool_options.h b/src/kernel/mempool_options.h
1index d57dbb393f..cf873dd48d 100644
2--- a/src/kernel/mempool_options.h
3+++ b/src/kernel/mempool_options.h
4@@ -48,9 +48,8 @@ struct MemPoolOptions {
5 * type is designated as TxoutType::NULL_DATA.
6 *
7 * Maximum size of TxoutType::NULL_DATA scripts that this node considers standard.
8- * If nullopt, any size is nonstandard.
9 */
10- std::optional<unsigned> max_datacarrier_bytes{DEFAULT_ACCEPT_DATACARRIER ? std::optional{MAX_OP_RETURN_RELAY} : std::nullopt};
11+ unsigned max_datacarrier_bytes{DEFAULT_ACCEPT_DATACARRIER ? MAX_OP_RETURN_RELAY : 0};
12 bool permit_bare_multisig{DEFAULT_PERMIT_BAREMULTISIG};
13 bool require_standard{true};
14 bool persist_v1_dat{DEFAULT_PERSIST_V1_DAT};
15diff --git a/src/node/mempool_args.cpp b/src/node/mempool_args.cpp
16index 11c77ff561..2f79dd3670 100644
17--- a/src/node/mempool_args.cpp
18+++ b/src/node/mempool_args.cpp
19@@ -93,7 +93,7 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& argsman, const CChainP
20 if (argsman.GetBoolArg("-datacarrier", DEFAULT_ACCEPT_DATACARRIER)) {
21 mempool_opts.max_datacarrier_bytes = argsman.GetIntArg("-datacarriersize", MAX_OP_RETURN_RELAY);
22 } else {
23- mempool_opts.max_datacarrier_bytes = std::nullopt;
24+ mempool_opts.max_datacarrier_bytes = 0;
25 }
26
27 mempool_opts.require_standard = !argsman.GetBoolArg("-acceptnonstdtxn", DEFAULT_ACCEPT_NON_STD_TXN);
28diff --git a/src/policy/policy.cpp b/src/policy/policy.cpp
29index 48f2a6a744..f4a2ee66d1 100644
30--- a/src/policy/policy.cpp
31+++ b/src/policy/policy.cpp
32@@ -96,7 +96,7 @@ bool IsStandard(const CScript& scriptPubKey, TxoutType& whichType)
33 return true;
34 }
35
36-bool IsStandardTx(const CTransaction& tx, const std::optional<unsigned>& max_datacarrier_bytes, bool permit_bare_multisig, const CFeeRate& dust_relay_fee, std::string& reason)
37+bool IsStandardTx(const CTransaction& tx, unsigned max_datacarrier_bytes, bool permit_bare_multisig, const CFeeRate& dust_relay_fee, std::string& reason)
38 {
39 if (tx.version > TX_MAX_STANDARD_VERSION || tx.version < 1) {
40 reason = "version";
41@@ -133,7 +133,7 @@ bool IsStandardTx(const CTransaction& tx, const std::optional<unsigned>& max_dat
42 }
43 }
44
45- unsigned int datacarrier_bytes_left = max_datacarrier_bytes.value_or(0);
46+ unsigned int datacarrier_bytes_left{max_datacarrier_bytes};
47 TxoutType whichType;
48 for (const CTxOut& txout : tx.vout) {
49 if (!::IsStandard(txout.scriptPubKey, whichType)) {
50diff --git a/src/policy/policy.h b/src/policy/policy.h
51index f9a18561bc..0104aaff1c 100644
52--- a/src/policy/policy.h
53+++ b/src/policy/policy.h
54@@ -151,7 +151,7 @@ static constexpr decltype(CTransaction::version) TX_MAX_STANDARD_VERSION{3};
55 * Check for standard transaction types
56 * [@return](/bitcoin-bitcoin/contributor/return/) True if all outputs (scriptPubKeys) use only standard transaction forms
57 */
58-bool IsStandardTx(const CTransaction& tx, const std::optional<unsigned>& max_datacarrier_bytes, bool permit_bare_multisig, const CFeeRate& dust_relay_fee, std::string& reason);
59+bool IsStandardTx(const CTransaction& tx, unsigned max_datacarrier_bytes, bool permit_bare_multisig, const CFeeRate& dust_relay_fee, std::string& reason);
60 /**
61 * Check for standard transaction types
62 * [@param](/bitcoin-bitcoin/contributor/param/)[in] mapInputs Map of previous transactions that have outputs we're spending
63diff --git a/src/test/fuzz/transaction.cpp b/src/test/fuzz/transaction.cpp
64index c9eb11222f..f5cbacfb78 100644
65--- a/src/test/fuzz/transaction.cpp
66+++ b/src/test/fuzz/transaction.cpp
67@@ -61,8 +61,8 @@ FUZZ_TARGET(transaction, .init = initialize_transaction)
68
69 const CFeeRate dust_relay_fee{DUST_RELAY_TX_FEE};
70 std::string reason;
71- const bool is_standard_with_permit_bare_multisig = IsStandardTx(tx, std::nullopt, /* permit_bare_multisig= */ true, dust_relay_fee, reason);
72- const bool is_standard_without_permit_bare_multisig = IsStandardTx(tx, std::nullopt, /* permit_bare_multisig= */ false, dust_relay_fee, reason);
73+ const bool is_standard_with_permit_bare_multisig = IsStandardTx(tx, /*max_datacarrier_bytes=*/0, /* permit_bare_multisig= */ true, dust_relay_fee, reason);
74+ const bool is_standard_without_permit_bare_multisig = IsStandardTx(tx, /*max_datacarrier_bytes=*/0, /* permit_bare_multisig= */ false, dust_relay_fee, reason);
75 if (is_standard_without_permit_bare_multisig) {
76 assert(is_standard_with_permit_bare_multisig);
77 }
78diff --git a/src/test/script_p2sh_tests.cpp b/src/test/script_p2sh_tests.cpp
79index b46411185c..6cb6b682b1 100644
80--- a/src/test/script_p2sh_tests.cpp
81+++ b/src/test/script_p2sh_tests.cpp
82@@ -21,13 +21,13 @@
83 // Helpers:
84 static bool IsStandardTx(const CTransaction& tx, bool permit_bare_multisig, std::string& reason)
85 {
86- return IsStandardTx(tx, std::nullopt, permit_bare_multisig, CFeeRate{DUST_RELAY_TX_FEE}, reason);
87+ return IsStandardTx(tx, /*max_datacarrier_bytes=*/0, permit_bare_multisig, CFeeRate{DUST_RELAY_TX_FEE}, reason);
88 }
89
90 static bool IsStandardTx(const CTransaction& tx, std::string& reason)
91 {
92- return IsStandardTx(tx, std::nullopt, /*permit_bare_multisig=*/true, CFeeRate{DUST_RELAY_TX_FEE}, reason) &&
93- IsStandardTx(tx, std::nullopt, /*permit_bare_multisig=*/false, CFeeRate{DUST_RELAY_TX_FEE}, reason);
94+ return IsStandardTx(tx, /*max_datacarrier_bytes=*/0, /*permit_bare_multisig=*/true, CFeeRate{DUST_RELAY_TX_FEE}, reason) &&
95+ IsStandardTx(tx, /*max_datacarrier_bytes=*/0, /*permit_bare_multisig=*/false, CFeeRate{DUST_RELAY_TX_FEE}, reason);
96 }
97
98 static std::vector<unsigned char> Serialize(const CScript& s)
714@@ -713,6 +715,8 @@ static RPCHelpMan getmempoolinfo()
715 {RPCResult::Type::NUM, "incrementalrelayfee", "minimum fee rate increment for mempool limiting or replacement in " + CURRENCY_UNIT + "/kvB"},
716 {RPCResult::Type::NUM, "unbroadcastcount", "Current number of transactions that haven't passed initial broadcast yet"},
717 {RPCResult::Type::BOOL, "fullrbf", "True if the mempool accepts RBF without replaceability signaling inspection (DEPRECATED)"},
718+ {RPCResult::Type::BOOL, "permitbaremultisig", "True if the mempool accepts transactions with bare multisig outputs"},
719+ {RPCResult::Type::NUM, "maxdatacarriersize", "Maximum number of bytes that can be used by OP_RETURN outputs in the mempool"},