From e598e405bdd4303e187fcc241500287ec458efff Mon Sep 17 00:00:00 2001 From: Nicholas Dudfield Date: Fri, 27 Feb 2026 13:38:26 +0700 Subject: [PATCH] fix: harden RuntimeConfig validation and add startup diagnostics - Error on unknown message_types instead of silently widening scope - Make messageCategories optional so per-peer can override global filter to "all categories" (nullopt=inherit, empty set=explicitly all) - Clamp send_drop_pct to 0-100% range - Add STARTDIAG: logging for consensus startup diagnostics - Add 3 test cases (11 total, 58 assertions) --- src/test/rpc/RuntimeConfig_test.cpp | 101 ++++++++++++++++++++++- src/xrpld/app/consensus/RCLConsensus.cpp | 15 +++- src/xrpld/app/misc/RuntimeConfig.h | 13 +-- src/xrpld/consensus/Consensus.h | 32 ++++++- src/xrpld/rpc/handlers/RuntimeConfig.cpp | 36 ++++++-- 5 files changed, 177 insertions(+), 20 deletions(-) diff --git a/src/test/rpc/RuntimeConfig_test.cpp b/src/test/rpc/RuntimeConfig_test.cpp index 3940473c8..3aa533caa 100644 --- a/src/test/rpc/RuntimeConfig_test.cpp +++ b/src/test/rpc/RuntimeConfig_test.cpp @@ -285,13 +285,109 @@ class RuntimeConfig_test : public beast::unit_test::suite if (!BEAST_EXPECT(cfg.has_value())) return; - BEAST_EXPECT(cfg->messageCategories.empty()); + BEAST_EXPECT(!cfg->messageCategories.has_value()); BEAST_EXPECT(cfg->appliesTo(TrafficCount::category::proposal)); BEAST_EXPECT(cfg->appliesTo(TrafficCount::category::validation)); BEAST_EXPECT(cfg->appliesTo(TrafficCount::category::transaction)); BEAST_EXPECT(cfg->appliesTo(TrafficCount::category::base)); } + void + testInvalidMessageType() + { + testcase("Invalid message type returns error"); + using namespace test::jtx; + Env env{*this}; + + Json::Value params; + params["set"] = Json::objectValue; + params["set"]["*"] = Json::objectValue; + params["set"]["*"]["send_delay_ms"] = 100; + params["set"]["*"]["message_types"] = Json::arrayValue; + params["set"]["*"]["message_types"].append("proposals"); // typo + auto result = runtimeConfig(env, params); + + BEAST_EXPECT(result.isMember("error")); + BEAST_EXPECT(result["error"].asString() == "invalidParams"); + // Config should NOT have been applied + BEAST_EXPECT(!env.app().getRuntimeConfig().active()); + } + + void + testDropPctClamping() + { + testcase("send_drop_pct clamped to 0-100"); + using namespace test::jtx; + Env env{*this}; + + // Over 100 + { + Json::Value params; + params["set"] = Json::objectValue; + params["set"]["*"] = Json::objectValue; + params["set"]["*"]["send_drop_pct"] = 200.0; + runtimeConfig(env, params); + } + auto cfg = env.app().getRuntimeConfig().getConfig("*"); + BEAST_EXPECT(cfg.has_value()); + BEAST_EXPECT(cfg->sendDropPctX100 == 10000); // clamped to 100% + + // Negative + { + Json::Value params; + params["set"] = Json::objectValue; + params["set"]["*"] = Json::objectValue; + params["set"]["*"]["send_drop_pct"] = -50.0; + runtimeConfig(env, params); + } + cfg = env.app().getRuntimeConfig().getConfig("*"); + BEAST_EXPECT(cfg.has_value()); + BEAST_EXPECT(cfg->sendDropPctX100 == 0); // clamped to 0% + } + + void + testPerPeerClearInheritedFilter() + { + testcase("Per-peer can override global filter to all"); + using namespace test::jtx; + Env env{*this}; + + // Global: only proposals + { + Json::Value params; + params["set"] = Json::objectValue; + params["set"]["*"] = Json::objectValue; + params["set"]["*"]["send_delay_ms"] = 100; + params["set"]["*"]["message_types"] = Json::arrayValue; + params["set"]["*"]["message_types"].append("proposal"); + runtimeConfig(env, params); + } + + // Per-peer: message_types = [] (explicitly all) + { + Json::Value params; + params["set"] = Json::objectValue; + params["set"]["10.0.0.2:51235"] = Json::objectValue; + params["set"]["10.0.0.2:51235"]["message_types"] = Json::arrayValue; + runtimeConfig(env, params); + } + + auto& rc = env.app().getRuntimeConfig(); + + // Per-peer should apply to all categories (empty set override) + auto peerCfg = rc.getConfig("10.0.0.2:51235"); + BEAST_EXPECT(peerCfg.has_value()); + BEAST_EXPECT(peerCfg->appliesTo(TrafficCount::category::proposal)); + BEAST_EXPECT(peerCfg->appliesTo(TrafficCount::category::validation)); + BEAST_EXPECT(peerCfg->appliesTo(TrafficCount::category::transaction)); + + // Other peers still only get proposal filter from global + auto otherCfg = rc.getConfig("10.0.0.3:51235"); + BEAST_EXPECT(otherCfg.has_value()); + BEAST_EXPECT(otherCfg->appliesTo(TrafficCount::category::proposal)); + BEAST_EXPECT(!otherCfg->appliesTo(TrafficCount::category::validation)); + } + public: void run() override @@ -304,6 +400,9 @@ public: testPerPeerWithoutGlobal(); testMessageTypeFilter(); testMessageTypeFilterEmpty(); + testInvalidMessageType(); + testDropPctClamping(); + testPerPeerClearInheritedFilter(); } }; diff --git a/src/xrpld/app/consensus/RCLConsensus.cpp b/src/xrpld/app/consensus/RCLConsensus.cpp index a2ebc6834..7a499eae9 100644 --- a/src/xrpld/app/consensus/RCLConsensus.cpp +++ b/src/xrpld/app/consensus/RCLConsensus.cpp @@ -1168,8 +1168,17 @@ RCLConsensus::Adaptor::preStartRound( !nowTrusted.empty()) nUnlVote_.newValidators(prevLgr.seq() + 1, nowTrusted); + bool const proposing = validating_ && synced; + + JLOG(j_.info()) << "STARTDIAG: preStartRound" + << " mode=" << app_.getOPs().strOperatingMode() + << " synced=" << (synced ? "yes" : "no") + << " validating=" << (validating_ ? "yes" : "no") + << " proposing=" << (proposing ? "yes" : "no") + << " seq=" << (prevLgr.seq() + 1); + // propose only if we're in sync with the network (and validating) - return validating_ && synced; + return proposing; } bool @@ -1208,7 +1217,11 @@ void RCLConsensus::Adaptor::updateOperatingMode(std::size_t const positions) const { if (!positions && app_.getOPs().isFull()) + { + JLOG(j_.warn()) << "STARTDIAG: updateOperatingMode demoting" + << " FULL->CONNECTED positions=" << positions; app_.getOPs().setMode(OperatingMode::CONNECTED); + } } //------------------------------------------------------------------------------ diff --git a/src/xrpld/app/misc/RuntimeConfig.h b/src/xrpld/app/misc/RuntimeConfig.h index 4477e8afc..45cd23505 100644 --- a/src/xrpld/app/misc/RuntimeConfig.h +++ b/src/xrpld/app/misc/RuntimeConfig.h @@ -39,16 +39,17 @@ struct ConfigVals std::optional sendDelayMs; std::optional sendDelayJitterMs; std::optional sendDropPctX100; // 0-10000 (pct * 100, avoids float) - // If set, only apply to these TrafficCount::category values. - // Empty set = no filter (apply to all messages). - std::set messageCategories; + // If set (non-nullopt), only apply to these TrafficCount::category values. + // nullopt = not specified (inherit from global on merge). + // Empty set = explicitly "all categories" (overrides global filter). + std::optional> messageCategories; /** Check if this config applies to a given message category. */ bool appliesTo(std::size_t category) const { - return messageCategories.empty() || - messageCategories.count(category) > 0; + return !messageCategories || messageCategories->empty() || + messageCategories->count(category) > 0; } bool @@ -70,7 +71,7 @@ struct ConfigVals result.sendDelayJitterMs = other.sendDelayJitterMs; if (other.sendDropPctX100) result.sendDropPctX100 = other.sendDropPctX100; - if (!other.messageCategories.empty()) + if (other.messageCategories) result.messageCategories = other.messageCategories; return result; } diff --git a/src/xrpld/consensus/Consensus.h b/src/xrpld/consensus/Consensus.h index e598a1671..5b6749123 100644 --- a/src/xrpld/consensus/Consensus.h +++ b/src/xrpld/consensus/Consensus.h @@ -829,8 +829,12 @@ Consensus::peerProposalInternal( if (newPeerProp.prevLedger() != prevLedgerID_) { - JLOG(j_.debug()) << "Got proposal for " << newPeerProp.prevLedger() - << " but we are on " << prevLedgerID_; + JLOG(j_.info()) << "STARTDIAG: peerProposal REJECTED prevLedger" + << " peer=" << newPeerProp.nodeID() + << " theirPrev=" << newPeerProp.prevLedger() + << " ourPrev=" << prevLedgerID_ + << " phase=" << to_string(phase_) + << " seq=" << newPeerProp.proposeSeq(); return false; } @@ -871,9 +875,18 @@ Consensus::peerProposalInternal( } if (peerPosIt != currPeerPositions_.end()) + { peerPosIt->second = newPeerPos; + } else + { currPeerPositions_.emplace(peerID, newPeerPos); + JLOG(j_.info()) << "STARTDIAG: peerProposal ACCEPTED" + << " peer=" << peerID + << " peerPositions=" << currPeerPositions_.size() + << " seq=" << newPeerProp.proposeSeq() + << " phase=" << to_string(phase_); + } } // Harvest RNG data from proposal if adaptor supports it @@ -1698,6 +1711,11 @@ Consensus::phaseEstablish( } //@@end rng-phase-establish-substates + JLOG(j_.info()) << "STARTDIAG: converge cutoff" + << " peerPositions=" << currPeerPositions_.size() + << " roundTime=" << result_->roundTime.read().count() + << "ms" + << " mode=" << to_string(mode_.get()); JLOG(j_.info()) << "Converge cutoff (" << currPeerPositions_.size() << " participants)"; CLOG(clog) << "Converge cutoff (" << currPeerPositions_.size() @@ -2013,8 +2031,14 @@ Consensus::haveConsensus( auto currentFinished = adaptor_.proposersFinished(previousLedger_, prevLedgerID_); - JLOG(j_.debug()) << "Checking for TX consensus: agree=" << agree - << ", disagree=" << disagree; + JLOG(j_.info()) << "STARTDIAG: haveConsensus" + << " agree=" << agree << " disagree=" << disagree + << " total=" << (agree + disagree) + << " peerPositions=" << currPeerPositions_.size() + << " prevProposers=" << prevProposers_ + << " roundTime=" << result_->roundTime.read().count() + << "ms" + << " mode=" << to_string(mode_.get()); // Determine if we actually have consensus or not result_->state = checkConsensus( diff --git a/src/xrpld/rpc/handlers/RuntimeConfig.cpp b/src/xrpld/rpc/handlers/RuntimeConfig.cpp index 32982ed8c..be1c0aa5e 100644 --- a/src/xrpld/rpc/handlers/RuntimeConfig.cpp +++ b/src/xrpld/rpc/handlers/RuntimeConfig.cpp @@ -82,14 +82,34 @@ doRuntimeConfig(RPC::JsonContext& context) if (v.isMember("send_delay_jitter_ms")) cfg.sendDelayJitterMs = v["send_delay_jitter_ms"].asInt(); if (v.isMember("send_drop_pct")) - cfg.sendDropPctX100 = - static_cast(v["send_drop_pct"].asDouble() * 100); - if (v.isMember("message_types") && v["message_types"].isArray()) { - for (auto const& mt : v["message_types"]) + auto pct = v["send_drop_pct"].asDouble(); + if (pct < 0.0) + pct = 0.0; + else if (pct > 100.0) + pct = 100.0; + cfg.sendDropPctX100 = static_cast(pct * 100); + } + if (v.isMember("message_types")) + { + auto const& mts = v["message_types"]; + cfg.messageCategories.emplace(); // set to empty = "all" + if (mts.isArray()) { - if (auto cat = categoryFromName(mt.asString())) - cfg.messageCategories.insert(*cat); + for (auto const& mt : mts) + { + auto const name = mt.asString(); + auto cat = categoryFromName(name); + if (!cat) + { + Json::Value err{Json::objectValue}; + err["error"] = "invalidParams"; + err["error_message"] = + "Unknown message_type: " + name; + return err; + } + cfg.messageCategories->insert(*cat); + } } } rc.setConfig(target, cfg); @@ -123,10 +143,10 @@ doRuntimeConfig(RPC::JsonContext& context) entry["send_delay_jitter_ms"] = *cfg.sendDelayJitterMs; if (cfg.sendDropPctX100) entry["send_drop_pct"] = *cfg.sendDropPctX100 / 100.0; - if (!cfg.messageCategories.empty()) + if (cfg.messageCategories) { Json::Value types{Json::arrayValue}; - for (auto cat : cfg.messageCategories) + for (auto cat : *cfg.messageCategories) types.append(categoryToName(cat)); entry["message_types"] = types; }