rpc: Export corepolicy settings for headless deployments (dumpsettings/setsettings)#154
Conversation
luke-jr
left a comment
There was a problem hiding this comment.
There's a lot to review here, so this isn't complete comments yet.
|
Thank you for taking a look @luke-jr I greatly appreciate it. This is still a WIP and there's more I'd like to do here, but your comments are really helpful as I work through this to ensure it meets Knots' standards. |
|
One issue is going to be review. This is largely one giant commit, which makes it difficult. Can you break it up into smaller steps? You're also putting a lot in the RPC code. At the end of this, we should ideally have RPC and GUI using the same code for changing settings. So an internal interface for updatesettings called by both would be appropriate. There's a lot of existing parsing code that you can probably reuse too (eg, boolean string to boolean UniValue conversions...) |
|
@luke-jr for review purposes, would it be better to break this into several PRs, or would you rather have everything in one PR? |
|
Multiple PRs could make sense, but there's tradeoffs. As long as it's broken up into logical separate commits, that's probably good enough. |
a542761 to
8abfb47
Compare
a70f6c4 to
85d3d8e
Compare
f4deb0d to
40bead9
Compare
40bead9 to
0bf6977
Compare
7038925 to
88a30c6
Compare
|
Reworked onto current 29.x-knots: kept dumpsettings/setsettings only, routed both the RPC and the GUI through one shared internal interface (node/policy_settings) that applies to live mempool options and persists like the GUI, and added a functional test. Dropped the standalone settings manager, JSON schema/subscribe surface, and security levels. |
88a30c6 to
397e1a4
Compare
397e1a4 to
b77bdc1
Compare
| std::string PermitEphemeralToString(const kernel::MemPoolOptions& o) | ||
| { | ||
| if (!(o.permitephemeral_anchor || o.permitephemeral_send)) return "reject"; | ||
| return std::string(o.permitephemeral_anchor ? "" : "-") + "anchor," + | ||
| (o.permitephemeral_send ? "" : "-") + "send," + | ||
| (o.permitephemeral_dust ? "" : "-") + "dust"; | ||
| } |
There was a problem hiding this comment.
From CodeRabbit, minor:
PermitEphemeralToString drops dust for -anchor,-send,dust setsettings accepts that combination via ApplyPermitEphemeralOption, but dumpsettings collapses it to reject, so dust won’t round-trip.
| args.ModifyRWConfigFile("mempoolreplacement", nv.s); | ||
| } | ||
| return {}; | ||
| } |
There was a problem hiding this comment.
From CodeRabbit, minor:
Use the startup parser for mempoolreplacement
This branch maps never to Never, any string containing optin (but not -optin) to OptIn, and everything else to Always. That diverges from the token-based startup parser, especially for values like fee and other comma lists, so setsettings can resolve the same string differently than CLI/config. Reuse the shared parser/helper here.
| util::Result<void> UpdatePolicySetting(NodeContext& node, std::string_view name, const UniValue& value, bool persist) | ||
| { | ||
| const PolicySettingDef* def = FindPolicySetting(name); | ||
| if (!def) return util::Error{Untranslated(strprintf("unknown setting: %s", name))}; | ||
|
|
||
| auto norm = Normalize(*def, value); | ||
| if (!norm) return util::Error{util::ErrorString(norm)}; | ||
| const Normalized& nv = *norm; | ||
|
|
||
| ArgsManager& args = *Assert(node.args); | ||
| CTxMemPool* pool = MemPool(node); | ||
| const std::string n{name}; | ||
|
|
||
| // Helper: persist a settings.json value only when requested. | ||
| auto json = [&](const std::string& key, const SettingsValue& v) { if (persist) PersistJson(args, key, v); }; | ||
| auto rwconf = [&](const std::string& key, const std::string& v) { if (persist) args.ModifyRWConfigFile(key, v); }; | ||
|
|
||
| // ---- Globals (no mempool required) ---- | ||
| if (n == "datacarriercost") { | ||
| g_weight_per_data_byte = (unsigned int)((nv.hundredths * WITNESS_SCALE_FACTOR + 99) / 100); | ||
| json("datacarriercost", UniValue(double(nv.hundredths) / 100.0)); | ||
| return {}; | ||
| } | ||
| if (n == "bytespersigop") { nBytesPerSigOp = (unsigned int)nv.i; rwconf("bytespersigop", strprintf("%d", nv.i)); return {}; } | ||
| if (n == "bytespersigopstrict") { nBytesPerSigOpStrict = (unsigned int)nv.i; rwconf("bytespersigopstrict", strprintf("%d", nv.i)); return {}; } | ||
| if (n == "maxscriptsize") { g_script_size_policy_limit = (unsigned int)nv.i; json("maxscriptsize", UniValue(nv.i)); return {}; } | ||
| if (n == "spkreuse") { rwconf("spkreuse", nv.s); return {}; } // restart-required; no live field | ||
|
|
||
| if (!pool) return util::Error{Untranslated("mempool not available")}; | ||
| kernel::MemPoolOptions& o = pool->m_opts; | ||
|
|
||
| // ---- Mempool size / expiry (with live trim on shrink) ---- | ||
| if (n == "maxmempool" || n == "mempoolexpiry") { | ||
| bool shrink; | ||
| if (n == "maxmempool") { | ||
| const int64_t old = o.max_size_bytes; | ||
| o.max_size_bytes = nv.i * 1'000'000; | ||
| shrink = o.max_size_bytes < old; | ||
| args.ForceSetArg("-maxmempool", strprintf("%d", nv.i)); | ||
| rwconf("maxmempool", strprintf("%d", nv.i)); | ||
| } else { | ||
| const auto old = o.expiry; | ||
| o.expiry = std::chrono::hours{nv.i}; | ||
| shrink = o.expiry < old; | ||
| args.ForceSetArg("-mempoolexpiry", strprintf("%d", nv.i)); | ||
| rwconf("mempoolexpiry", strprintf("%d", nv.i)); | ||
| } | ||
| if (shrink && node.chainman) { | ||
| LOCK(cs_main); | ||
| LOCK(pool->cs); | ||
| LimitMempoolSize(*pool, node.chainman->ActiveChainstate().CoinsTip()); | ||
| } | ||
| return {}; | ||
| } | ||
|
|
||
| // ---- Ancestor/descendant limits ---- | ||
| if (n == "limitancestorcount") { o.limits.ancestor_count = nv.i; args.ForceSetArg("-limitancestorcount", strprintf("%d", nv.i)); rwconf("limitancestorcount", strprintf("%d", nv.i)); return {}; } | ||
| if (n == "limitancestorsize") { o.limits.ancestor_size_vbytes = nv.i * 1'000; args.ForceSetArg("-limitancestorsize", strprintf("%d", nv.i)); rwconf("limitancestorsize", strprintf("%d", nv.i)); return {}; } | ||
| if (n == "limitdescendantcount") { o.limits.descendant_count = nv.i; args.ForceSetArg("-limitdescendantcount", strprintf("%d", nv.i)); rwconf("limitdescendantcount", strprintf("%d", nv.i)); return {}; } | ||
| if (n == "limitdescendantsize") { o.limits.descendant_size_vbytes = nv.i * 1'000; args.ForceSetArg("-limitdescendantsize", strprintf("%d", nv.i)); rwconf("limitdescendantsize", strprintf("%d", nv.i)); return {}; } | ||
|
|
||
| // ---- Fee rates (satoshis-per-kvB via ParseMoney) ---- | ||
| if (n == "minrelaytxfee") { o.min_relay_feerate = CFeeRate(nv.amount); rwconf("minrelaytxfee", FormatMoney(nv.amount)); return {}; } | ||
| if (n == "incrementalrelayfee") { o.incremental_relay_feerate = CFeeRate(nv.amount); rwconf("incrementalrelayfee", FormatMoney(nv.amount)); return {}; } | ||
| if (n == "dustrelayfee") { | ||
| const CFeeRate feerate{nv.amount}; | ||
| o.dust_relay_feerate_floor = feerate; | ||
| if (o.dust_relay_feerate < feerate || !o.dust_relay_target) { | ||
| o.dust_relay_feerate = feerate; | ||
| } else { | ||
| pool->UpdateDynamicDustFeerate(); | ||
| } | ||
| rwconf("dustrelayfee", FormatMoney(nv.amount)); | ||
| return {}; | ||
| } | ||
|
|
||
| // ---- Simple integer m_opts, persisted to settings.json ---- | ||
| if (n == "minrelaycoinblocks") { o.minrelaycoinblocks = nv.i; json("minrelaycoinblocks", UniValue(nv.i)); return {}; } | ||
| if (n == "minrelaymaturity") { o.minrelaymaturity = nv.i; json("minrelaymaturity", UniValue(nv.i)); return {}; } | ||
| if (n == "maxtxlegacysigops") { o.maxtxlegacysigops = nv.i; json("maxtxlegacysigops", UniValue(nv.i)); return {}; } | ||
|
|
||
| // ---- Boolean policy toggles ---- | ||
| if (n == "rejectparasites") { o.reject_parasites = nv.b; json("rejectparasites", UniValue(nv.b)); return {}; } | ||
| if (n == "rejecttokens") { o.reject_tokens = nv.b; json("rejecttokens", UniValue(nv.b)); return {}; } | ||
| if (n == "subdustfeepenalty") { o.subdustfeepenalty = nv.b; json("subdustfeepenalty", UniValue(nv.b)); return {}; } | ||
| if (n == "acceptunknownwitness") { o.acceptunknownwitness = nv.b; json("acceptunknownwitness", UniValue(nv.b)); return {}; } | ||
| if (n == "permitbarepubkey") { o.permit_bare_pubkey = nv.b; json("permitbarepubkey", UniValue(nv.b)); return {}; } | ||
| if (n == "permitbareanchor") { o.permitbareanchor = nv.b; json("permitbareanchor", UniValue(nv.b)); return {}; } | ||
| if (n == "permitbaredatacarrier") { o.permitbaredatacarrier = nv.b; json("permitbaredatacarrier", UniValue(nv.b)); return {}; } | ||
| if (n == "acceptnonstddatacarrier") { o.accept_non_std_datacarrier = nv.b; json("acceptnonstddatacarrier", UniValue(nv.b)); return {}; } | ||
| if (n == "acceptnonstdtxn") { o.require_standard = !nv.b; rwconf("acceptnonstdtxn", strprintf("%d", nv.b)); return {}; } | ||
| if (n == "permitbaremultisig") { o.permit_bare_multisig = nv.b; rwconf("permitbaremultisig", strprintf("%d", nv.b)); return {}; } | ||
|
|
||
| // ---- Data carrier size (toggles the datacarrier bool key) ---- | ||
| if (n == "datacarriersize") { | ||
| if (nv.i > 0) { | ||
| if (!o.max_datacarrier_bytes.has_value()) rwconf("datacarrier", "1"); | ||
| rwconf("datacarriersize", strprintf("%d", nv.i)); | ||
| o.max_datacarrier_bytes = (unsigned)nv.i; | ||
| } else { | ||
| rwconf("datacarrier", "0"); | ||
| o.max_datacarrier_bytes = std::nullopt; | ||
| } | ||
| return {}; | ||
| } | ||
|
|
||
| // ---- Special string settings ---- | ||
| if (n == "mempoolreplacement") { | ||
| if (nv.s == "never") o.rbf_policy = RBFPolicy::Never; | ||
| else if (nv.s.find("optin") != std::string::npos && nv.s.find("-optin") == std::string::npos) o.rbf_policy = RBFPolicy::OptIn; | ||
| else o.rbf_policy = RBFPolicy::Always; | ||
| if (persist) { | ||
| PersistJson(args, "mempoolfullrbf", UniValue(o.rbf_policy == RBFPolicy::Always ? "1" : "0")); | ||
| args.ModifyRWConfigFile("mempoolreplacement", nv.s); | ||
| } | ||
| return {}; | ||
| } | ||
| if (n == "mempooltruc") { | ||
| o.truc_policy = (nv.s == "reject") ? TRUCPolicy::Reject : (nv.s == "enforce") ? TRUCPolicy::Enforce : TRUCPolicy::Accept; | ||
| json("mempooltruc", UniValue(nv.s)); | ||
| return {}; | ||
| } | ||
| if (n == "permitephemeral") { | ||
| ApplyPermitEphemeralOption(SettingsValue(nv.s), o); | ||
| json("permitephemeral", UniValue(nv.s)); | ||
| return {}; | ||
| } | ||
| if (n == "dustdynamic") { | ||
| auto parsed = ParseDustDynamicOpt(nv.s, /*max_fee_estimate_blocks=*/1008); | ||
| if (!parsed) return util::Error{util::ErrorString(parsed)}; | ||
| o.dust_relay_target = parsed->first; | ||
| o.dust_relay_multiplier = parsed->second; | ||
| json("dustdynamic", UniValue(nv.s)); | ||
| return {}; | ||
| } | ||
|
|
||
| return util::Error{Untranslated(strprintf("unhandled setting: %s", name))}; | ||
| } |
There was a problem hiding this comment.
From CodeRabbit, critical:
Protect live mempool policy updates
UpdatePolicySetting mutates pool->m_opts at runtime, but readers like MaybeSendFeefilter() use m_mempool.GetMinFee() and m_mempool.m_opts.min_relay_feerate without m_mempool.cs. That makes setsettings a data race unless these writes are taken under the same lock or moved to atomics.
|
tACK b77bdc1 I reviewed the code with Code Rabbit, and added the three issues (1 critical, 2 minor) as comments. I tested using the following commands on Ubuntu 24.04 running on ARMv8-A (64-bit): git clone https://github.com/bitcoinknots/bitcoin.git pr154 The test passes. |
pdath
left a comment
There was a problem hiding this comment.
What do you think of this comment I posted yesterday?
#154 (review)
The other comments I made are very minor, and more just for note.
Adds two RPCs so headless deployments (Start9, Umbrel, etc.) can read and change the node's corepolicy/mempool settings without the GUI. Resolves #130.
Changes
dumpsettings ( detailed ): export the current corepolicy/mempool settings as JSON. Withdetailed, each entry also reports its type and help text, so a headless UI can be built from the node instead of hard-coding the option list per release.setsettings {"name": value, ...}: validate the whole batch first (nothing is applied if any value is invalid), then apply and persist the valid changes. The result reports whether any change needs a restart (e.g.spkreuse).RPC and the GUI now change these settings through a single internal interface (
node/policy_settings), soOptionsModeland the RPCs stay consistent; theOptionsModelapply/persist duplication is removed. Setting names match the correspondingbitcoin.confoption names.Includes a functional test (
rpc_settings.py) covering round-trip, live effect viagetmempoolinfo, batch atomicity,detailedmetadata, and persistence across restart, plus release notes.