Skip to content

feat: DatagramMonitor and consensus-testing admin RPCs - #8218

Draft
dangell7 wants to merge 1 commit into
developfrom
dangell7/datagram
Draft

dangell7 wants to merge 1 commit into
developfrom
dangell7/datagram

Conversation

@dangell7

@dangell7 dangell7 commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

High Level Overview of Change

Adds DatagramMonitor, a [datagram_monitor] config section that makes the node emit a fixed-layout UDP datagram every interval with the numbers we otherwise scrape from server_info and the logs: server state, ledger and consensus timing, proposer count, cache hit rates, node store IO, tree cache size, job queue overflow. Adds the consensus-testing admin RPCs node_stall, inject and unl_set, and ByzantinePartitionRecovery_test for the recovery path. Bumps conan openssl to 3.6.3, which is what conan.xrplf.org serves.

Context of Change

The perf network and alphanet need per-node metrics at a fixed cadence without polling the admin RPC from outside the box: on the perf network the loadtester's collector reads the datagram, on alphanet the status sampler behind /status/ does. A push datagram is a single listener per node and no admin port exposure; polling server_info at the same cadence costs a JSON round trip per node per second and does not carry the cache and node store counters at all.

The admin RPCs exist so a DR drill or a consensus test can fault one node (stall it, inject a transaction, swap its UNL) from the tooling instead of by hand on the host.

This branch is part of the alphanet integration set (label alphanet) and is opened here so it is tracked and kept current against develop.

API Impact

  • Public API: New feature (new methods and/or new fields): admin methods node_stall, inject, unl_set; config section [datagram_monitor]
  • Public API: Breaking change (in general, breaking changes should only impact the next api_version)
  • libxrpl change (any change that may affect libxrpl or dependents of libxrpl): new jss fields
  • Peer protocol change (must be backward compatible or bump the peer protocol version)

Test Plan

ByzantinePartitionRecovery_test covers the stall and recovery path. The datagram layout is exercised end to end by the perf network's collector (xrpld-perfnet) and the alphanet status sampler, both of which decode it.

@codecov

codecov Bot commented Sep 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@xrplf-ai-reviewer xrplf-ai-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing return path on unsupported platforms causes undefined behavior.

if (sysctlbyname("hw.physicalcpu", &value, &size, NULL, 0) == 0)
count = value;
return count > 0 ? count : (count = 1);
#endif

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing #else clause — non-Linux/non-Apple platforms fall through without return, causing undefined behavior. Add:

Suggested change
#endif
#else
return std::thread::hardware_concurrency();
#endif

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Multiple critical build, shutdown, telemetry, and consensus-control issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds UDP node telemetry, consensus-testing admin RPCs, supporting hooks, a Byzantine recovery test, and an OpenSSL 3.6.3 update.

Changes:

  • Adds configurable DatagramMonitor telemetry.
  • Adds node_stall, inject, and unl_set admin RPCs.
  • Adds consensus recovery testing and related interfaces/configuration.
File summaries
File Change and final review notes
src/xrpld/rpc/handlers/transaction/Submit.cpp Adds transaction injection handling.
src/xrpld/rpc/handlers/Handlers.h Declares the new handlers.
src/xrpld/rpc/handlers/admin/UnlList.cpp Implements unl_set. Critical (2 votes), L71: validator changes are not propagated to validation and amendment state. Critical (1 vote), L65: quorum overrides are overwritten on the next update. Critical (1 vote), L42: empty validator sets produce quorum zero and can accept unsigned ledgers. Moderate (2 votes), L65: invalid quorum values can assert or silently fall back.
src/xrpld/rpc/handlers/admin/server_control/NodeStall.cpp Implements node_stall. Moderate (2 votes), L28: large unsigned durations can assert or throw instead of returning invalid parameters.
src/xrpld/rpc/detail/Handler.cpp Registers the new RPCs.
src/xrpld/core/detail/Config.cpp Loads datagram monitor configuration.
src/xrpld/core/Config.h Stores monitor endpoint configuration.
src/xrpld/app/misc/ValidatorList.h Declares debug UNL mutation support.
src/xrpld/app/misc/TxQ.h Declares transaction injection queue support.
src/xrpld/app/misc/NetworkOPs.cpp Adds stall and telemetry accessors. Moderate (1 vote), L1113: current-mode duration is omitted from the counter snapshot.
src/xrpld/app/misc/detail/ValidatorList.cpp Implements debug validator mutation. Critical (1 vote), L2001 and L2016: injected keys use listing count one and can be removed when the threshold exceeds one.
src/xrpld/app/misc/detail/TxQ.cpp Applies injected transactions.
src/xrpld/app/misc/DatagramMonitor.h Defines UDP telemetry collection and serialization. Critical (3 votes), L670: debug counters remain zero. Moderate (2 votes), L684: milliseconds are reported as microseconds. Moderate (1 vote), L690: current-state duration is omitted. Critical (3 votes), L247: ring-buffer wraparound produces incorrect rates. Moderate (3 votes), L766: Linux memory is underreported by 1024. Critical (3 votes), L824: malformed configuration can terminate the node. Moderate (1 vote), L373: IPv6 address storage is uninitialized. Moderate (1 vote), L841: persistent errors cause a tight retry loop. Nit (3 votes), L188: job-queue overflow is missing from the packet. Moderate (1 vote), L715: load factor is not encoded according to the contract. Moderate (2 votes), L544: Linux network-out parsing reads the wrong field. Moderate (1 vote), L597: NVMe devices can be incorrectly filtered. Nit (1 vote), L839: collection time causes cadence drift.
src/xrpld/app/main/Application.cpp Starts the monitor. Critical (3 votes), L1535: the monitor is not stopped before its dependencies, creating a shutdown race.
src/test/consensus/ByzantinePartitionRecovery_test.cpp Adds partition recovery scenarios. Critical (2 votes), L17, L128, and L277: CSF headers and sources are not wired to the target, preventing compilation/linking. Moderate (1 vote), L344: the injection check has no assertion. Moderate (1 vote), L368: recovery does not assert transaction acceptance. Moderate (1 vote), L886: the test does not invoke the new RPCs or construct DatagramMonitor. Moderate (1 vote), L457: post-recovery attacker ledgers can make the comparison pass falsely.
include/xrpl/server/NetworkOPs.h Extends the network operations interface.
include/xrpl/protocol/jss.h Adds injection response fields.
include/xrpl/config/Constants.h Adds the datagram monitor configuration constant.
.cspell.config.yaml Allows the new product name.
Review details

Suppressed comments (14)

src/test/consensus/ByzantinePartitionRecovery_test.cpp:346

  • This purported injection check only binds and prints attackerLedger; it never inspects its transactions or asserts anything. The injection can be ignored and this phase still passes, so the test does not cover the behavior it claims to verify.
        // Check that attackers included the injected tx
        Ledger const& attackerLedger = attackers[0]->lastClosedLedger;
        std::cout << "Attacker ledger seq: " << attackerLedger.seq() << "\n";

src/test/consensus/ByzantinePartitionRecovery_test.cpp:285

  • The comments describe the recovery safety assertion, but this code only prints branches and trust counts; it never asserts that a legitimate peer rejected the attacker FVL or that the network converged. A regression where recovery accepts the attacker chain would therefore still pass this test.
        // 2. Check if the network re-converges or stays forked
        std::size_t const branches = sim.branches();
        std::cout << "  Final branch count: " << branches << "\n";

        // The attacker's chain (validated with only 3/7 trust)
        // should not be accepted by the full network.
        // After reconnection, with 7 validators all trusting each other,
        // quorum is 6. The attacker's old validations only had 3.
        // The network should eventually converge on a new chain.

src/test/consensus/ByzantinePartitionRecovery_test.cpp:372

  • This test similarly has no assertion after the recovery phase: the injected transaction is never inspected in any legitimate ledger, despite the following comments claiming that property is checked. The test can pass even if the injected fork is accepted.
        // The legitimate peers should NOT have the injected transaction
        // in their fully validated ledger chain. The attacker's fork
        // (containing the injected tx) should be rejected because it
        // lacks sufficient validations from the full UNL.
    }

src/test/consensus/ByzantinePartitionRecovery_test.cpp:890

  • The suite invokes only CSF simulation APIs and synthetic transaction injections; it never invokes the new admin RPCs or constructs a DatagramMonitor. Thus it does not cover the new RPCs or the claimed stall/recovery path described in the test plan.
        testPartitionNoQuorumBypass();
        testPartitionWithQuorumBypass();
        testTxInjectionDuringPartition();
        testAttackSurfaceSweep();
        testLongPartitionDeepChain();

src/test/consensus/ByzantinePartitionRecovery_test.cpp:128

  • sim.run(int) resets every peer's target to completedLedgers + ledgers (see src/tests/libxrpl/csf/impl/Sim.cpp:14-20), so the earlier targetLedgers = completedLedgers assignments do not stop the legitimate validators. This partition therefore does not model the documented crash/offline phase.
        sim.run(4);

src/test/consensus/ByzantinePartitionRecovery_test.cpp:460

  • This comparison uses each attacker's FVL after recovery rather than the attacker's pre-recovery ledger. If the network correctly converges onto the legitimate chain, the attackers also end on that chain and this expression becomes true, falsely reporting that the legitimate peers accepted the attack. Preserve and compare against the pre-recovery attacker ledger ID or ancestry.
                        for (Peer* ap : attackers)
                        {
                            if (lp->fullyValidatedLedger.id() == ap->fullyValidatedLedger.id())
                                return true;

src/xrpld/app/misc/DatagramMonitor.h:694

  • The snapshot's current mode and start are ignored, so the duration for the currently active state never accumulates between transitions. server_info includes that elapsed interval, but this datagram reports only the last completed interval and can remain stale for the lifetime of a state.
        auto const [counters, mode, start, initialSync] = ops.getStateAccountingData();
        for (size_t i = 0; i < 5; ++i)
        {
            header->state_transitions[i] = counters[i].transitions;
            header->state_durations[i] = counters[i].dur.count();

src/xrpld/app/misc/DatagramMonitor.h:373

  • sockaddr_storage is left uninitialized; for IPv6 the unset sin6_flowinfo and sin6_scope_id bytes are passed to sendto, making IPv6 destinations nondeterministic (and link-local destinations can use a garbage scope). Value-initialize the structure before filling it.
        struct sockaddr_storage addr;

src/xrpld/app/misc/DatagramMonitor.h:844

  • If packet generation or sending keeps throwing, this catch immediately retries without any delay, creating a tight CPU/logging loop. Add a backoff or sleep in the error path so one persistent monitor failure cannot consume a core.
            catch (std::exception const& e)
            {
                // Log error but continue monitoring
                JLOG(j_.error()) << "Server info monitor error: " << e.what();

src/xrpld/app/misc/DatagramMonitor.h:715

  • ServerInfoHeader::load_factor is documented as fixed-point x1,000,000, but this stores the raw LoadFeeTrack fee level (normally 256, with load_base also 256). A decoder following the packet contract will report the wrong load factor; normalize the value or change the wire contract consistently.
        header->load_factor = static_cast<std::uint64_t>(app_.getFeeTrack().getLoadFactor());

src/xrpld/app/misc/DatagramMonitor.h:600

  • The partition filter drops every device whose name ends in a digit, but Linux NVMe namespaces such as nvme0n1 are whole block devices and also end in a digit. On nodes using NVMe storage this makes the disk counters (and their rates) remain zero; identify partitions using the block-device metadata (or handle NVMe namespace names) instead of this suffix test.
                    // Skip partitions (usually have a number at the end)
                    if (std::isdigit(device_name.back()))
                    {
                        continue;

src/xrpld/app/misc/DatagramMonitor.h:839

  • Because the sleep occurs after collection and transmission, each packet period is one second plus the time spent gathering metrics and sending to all endpoints; the schedule also drifts after any slow iteration. That does not provide the fixed one-second cadence promised by this exporter.
                std::this_thread::sleep_for(std::chrono::seconds(1));

src/xrpld/app/misc/NetworkOPs.cpp:1117

  • getCounterData() returns the accumulated duration only up to the last state transition. This snapshot is copied directly, while the returned mode and start are ignored, so the datagram underreports time in the current operating mode until the next transition; server_info explicitly adds that elapsed time.
    auto const data = accounting_.getCounterData();
    std::array<NetworkOPs::AccountingCounter, 5> out;
    for (std::size_t i = 0; i < out.size(); ++i)
        out[i] = {data.counters[i].transitions, data.counters[i].dur};
    return {out, data.mode, data.start, data.initialSyncUs};

src/xrpld/app/misc/detail/ValidatorList.cpp:2023

  • The override is applied only to the current quorum_ value; it is not stored anywhere. At the next consensus round updateTrusted() calls calculateQuorum() and silently discards a quorum supplied to unl_set, so a requested test quorum cannot reliably remain in effect.
    if (quorumOverride)
    {
        quorum_ = *quorumOverride;
    }
    else
    {
        auto const unlSize = trustedMasterKeys_.size();
        quorum_ = calculateQuorum(unlSize, unlSize, unlSize);
  • Files reviewed: 19/19 changed files
  • Comments generated: 15
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

* - Question: does the attacker chain get accepted?
*/

#include <test/csf.h>
Comment on lines +1535 to +1536
datagramMonitor_ = std::make_unique<DatagramMonitor>(*this);
datagramMonitor_->start();
Comment on lines +247 to +254
uint64_t actual_window_micros = current.timestamp - samples[0].timestamp;
double window_scale =
std::min(1.0, static_cast<double>(actual_window_micros) / expected_window_micros);

// Get the oldest valid sample
size_t oldest_index =
(current_index >= max_samples) ? ((current_index + 1) % max_samples) : 0;
auto const& oldest = samples[oldest_index];
Comment on lines +667 to +675
auto currentMetrics = collectSystemMetrics();
metrics_tracker_.addSample(currentMetrics);

// Slimmed for this fork (3.2.0-b0): ledger ranges, DB debug-counters and
// the object-count map are omitted (divergent accessors). The packet is
// just the fixed header with core node + OS metrics.
std::vector<uint8_t> buffer(sizeof(ServerInfoHeader));
auto* header = reinterpret_cast<ServerInfoHeader*>(buffer.data());
memset(header, 0, sizeof(ServerInfoHeader));
Comment on lines +824 to +827
for (auto const& epStr : app_.config().DATAGRAM_MONITOR)
{
auto endpoint = parseEndpoint(epStr);
endpoints.push_back(std::make_pair(endpoint, createSocket(endpoint)));
std::chrono::system_clock::now().time_since_epoch())
.count();
header->uptime = UptimeClock::now().time_since_epoch().count();
header->io_latency_us = app_.getIOLatency().count();
Comment on lines +763 to +766
// Get process memory usage
struct rusage usage;
getrusage(RUSAGE_SELF, &usage);
header->process_memory_pages = usage.ru_maxrss;
Comment on lines +65 to +69
std::optional<std::size_t> quorumOverride;
if (context.params.isMember("quorum") && context.params["quorum"].isIntegral())
{
quorumOverride = context.params["quorum"].asUInt();
}
Comment on lines +28 to +32
if (context.params.isMember("duration_ms") && context.params["duration_ms"].isIntegral())
{
durationMs = context.params["duration_ms"].asInt();
if (durationMs <= 0)
return rpcError(RpcInvalidParams);
MetricRates disk_write;
} rates;

DebugCounters dbg_counters;
@dangell7
dangell7 marked this pull request as draft September 12, 2026 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants