From 251cd181a7a9cc3315a9cbe208e9dc6cb18b446c Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Fri, 21 Aug 2026 12:29:02 +0100 Subject: [PATCH 1/5] fix: Reject unreadable [telemetry] TLS certificate paths at startup With telemetry enabled and use_tls=1, makeTelemetrySetup now reads each non-empty tls_ca_cert / tls_client_cert / tls_client_key path and refuses to start when the file is missing or cannot be read. The message names the config key, the path and the OS error, instead of leaving the problem to surface much later as an opaque TLS handshake failure inside the exporter. Reading the file with getFileContents, as the gRPC server already does for its own ssl_cert and ssl_key pair, proves the file is both present and readable; an existence test alone would miss a permissions problem. The contents are discarded. Both gates are deliberate. The check is skipped when enabled is 0, so a stale cert line still cannot stop a node from booting, and when use_tls is 0, where the exporter never opens the files. An empty path stays valid; for tls_ca_cert it selects the system CA store. Six GTest cases cover the three keys that can fail, the all-readable case, and each gate on its own. --- src/libxrpl/telemetry/TelemetryConfig.cpp | 43 ++++ .../libxrpl/telemetry/TelemetryConfig.cpp | 193 ++++++++++++++++-- 2 files changed, 220 insertions(+), 16 deletions(-) diff --git a/src/libxrpl/telemetry/TelemetryConfig.cpp b/src/libxrpl/telemetry/TelemetryConfig.cpp index 006f062009..f0cc61517f 100644 --- a/src/libxrpl/telemetry/TelemetryConfig.cpp +++ b/src/libxrpl/telemetry/TelemetryConfig.cpp @@ -8,6 +8,7 @@ * See cfg/xrpld-example.cfg for the full list of available options. */ +#include #include #include #include @@ -16,6 +17,7 @@ #include #include #include +#include namespace xrpl::telemetry { @@ -85,6 +87,35 @@ networkTypeFromId(std::uint32_t networkId) } } +/** + * Throw unless the given path names a file this process can read. + * + * An empty path means the option is unset, which every caller allows. Reading + * the file proves it is both present and readable; testing existence alone + * would miss a permissions problem. The contents are discarded — nothing here + * checks that they parse as PEM. + * + * @param path Path taken from the config, possibly empty. + * @param configKey Config key the path came from, named in the message. Not + * called `key`, which would hide the `key` namespace above. + * @throws std::runtime_error If the path is non-empty and cannot be read. + */ +void +requireReadableFile(std::string const& path, char const* configKey) +{ + if (path.empty()) + return; + + std::error_code ec; + getFileContents(ec, path); + if (ec) + { + Throw( + std::string{"[telemetry] "} + configKey + " cannot be read: " + path + " - " + + ec.message()); + } +} + } // namespace Telemetry::Setup @@ -139,6 +170,18 @@ makeTelemetrySetup( "[telemetry] tls_client_cert/tls_client_key require use_tls=1 " "(set use_tls=1 to enable mutual TLS, or remove the cert paths)."); } + + // Still inside the enabled branch. The exporter opens these files only + // when TLS is on, so check them only then: a bad path behind use_tls=0 + // stops nothing. Checking here turns what would otherwise surface much + // later as an opaque handshake failure into a startup error naming the + // key. Each path is optional; an empty one is skipped. + if (setup.useTls) + { + requireReadableFile(setup.tlsCertPath, key::tlsCaCert); + requireReadableFile(setup.tlsClientCertPath, key::tlsClientCert); + requireReadableFile(setup.tlsClientKeyPath, key::tlsClientKey); + } } // Head sampling is intentionally fixed at 1.0 (sample everything) and is diff --git a/src/tests/libxrpl/telemetry/TelemetryConfig.cpp b/src/tests/libxrpl/telemetry/TelemetryConfig.cpp index 2aff850c84..56a3ef4447 100644 --- a/src/tests/libxrpl/telemetry/TelemetryConfig.cpp +++ b/src/tests/libxrpl/telemetry/TelemetryConfig.cpp @@ -1,3 +1,4 @@ +#include #include #include #include @@ -5,10 +6,13 @@ #include #include +#include #include +#include using namespace xrpl; +using ::testing::AllOf; using ::testing::HasSubstr; using ::testing::ThrowsMessage; @@ -25,15 +29,18 @@ namespace { * or an unexpected throw. Tests that never set the key are unaffected. One * source of truth still keeps the two files from drifting apart. * - * clientCert and clientKey are the paths written to those keys. They are + * clientCert and clientKey are the paths written to those keys. They name files + * that do not exist, so they suit only the cases the readability check cannot + * reach: telemetry off, or use_tls off. A case with enabled=1 and use_tls=1 + * must write real files with writeCertFile() below instead. They are * declared as `char const*` so they pass to Section::set() (which takes * `std::string const&`) and compare against the parsed std::string members * without an explicit conversion, exactly as a literal would. * - * pairingError and useTlsError are message fragments. Both guards throw - * std::runtime_error, so the exception type alone cannot tell them apart. - * Each fragment occurs in exactly one of the two messages, so matching it - * proves which guard fired. + * pairingError, useTlsError and readError are message fragments. All three + * guards throw std::runtime_error, so the exception type alone cannot tell + * them apart. Each fragment occurs in exactly one of the three messages, so + * matching it proves which guard fired. */ namespace mtls { constexpr char const* keyClientCert = "tls_client_cert"; @@ -42,6 +49,7 @@ constexpr char const* clientCert = "/etc/ssl/client.pem"; constexpr char const* clientKey = "/etc/ssl/client.key"; constexpr char const* pairingError = "must be set together"; constexpr char const* useTlsError = "require use_tls=1"; +constexpr char const* readError = "cannot be read"; /** * Build a [telemetry] section carrying only the `enabled` key. @@ -75,6 +83,27 @@ parseSection(Section const& section) { return telemetry::makeTelemetrySetup(section, "nHUtest123", "2.0.0", 0); } + +/** + * Write a placeholder certificate file at the given path. + * + * The parser only needs the file to exist and be readable, so the contents are + * irrelevant — nothing checks that they parse as PEM. The stream state is + * asserted, so a failed write shows up as a setup failure here rather than as a + * confusing failure in the case under test. + * + * @param path Where to write the file, typically from TempDir::file(). + * @return The same path, ready to pass to Section::set(). + */ +std::string +writeCertFile(std::string const& path) +{ + std::ofstream out{path}; + out << "placeholder\n"; + out.close(); + EXPECT_TRUE(out.good()) << "could not create " << path; + return path; +} } // namespace mtls } // namespace @@ -121,6 +150,10 @@ TEST(TelemetryConfig, parse_empty_section) TEST(TelemetryConfig, parse_full_section) { + // The CA path has to name a real file: with enabled=1 and use_tls=1 the + // parser opens it, so a placeholder path would make this case throw. + TempDir const dir; + auto const caCert = mtls::writeCertFile(dir.file("ca.pem")); Section section; section.set("enabled", "1"); section.set("service_name", "my-rippled"); @@ -128,7 +161,7 @@ TEST(TelemetryConfig, parse_full_section) section.set("exporter", "otlp_http"); section.set("endpoint", "http://collector:4318/v1/traces"); section.set("use_tls", "1"); - section.set("tls_ca_cert", "/etc/ssl/ca.pem"); + section.set("tls_ca_cert", caCert); section.set("batch_size", "256"); section.set("batch_delay_ms", "3000"); section.set("max_queue_size", "4096"); @@ -145,7 +178,7 @@ TEST(TelemetryConfig, parse_full_section) EXPECT_EQ(setup.serviceInstanceId, "custom-id"); EXPECT_EQ(setup.exporterEndpoint, "http://collector:4318/v1/traces"); EXPECT_TRUE(setup.useTls); - EXPECT_EQ(setup.tlsCertPath, "/etc/ssl/ca.pem"); + EXPECT_EQ(setup.tlsCertPath, caCert); EXPECT_EQ(setup.batchSize, 256u); EXPECT_EQ(setup.batchDelay, std::chrono::milliseconds{3000}); EXPECT_EQ(setup.maxQueueSize, 4096u); @@ -158,17 +191,24 @@ TEST(TelemetryConfig, parse_full_section) TEST(TelemetryConfig, mtls_cert_and_key_both_set) { - // Telemetry on and use_tls=1, so both guards run and neither may fire. + // Telemetry on and use_tls=1, so all three checks run and none may fire. + // Both paths have to name real files, because the parser opens them here. + // No CA bundle is set, which is the case this covers: mTLS against a + // collector whose certificate the system CA store already vouches for. + TempDir const dir; + auto const cert = mtls::writeCertFile(dir.file("client.pem")); + auto const key = mtls::writeCertFile(dir.file("client.key")); Section section = mtls::makeSection(true); section.set("use_tls", "1"); - section.set(mtls::keyClientCert, mtls::clientCert); - section.set(mtls::keyClientKey, mtls::clientKey); + section.set(mtls::keyClientCert, cert); + section.set(mtls::keyClientKey, key); auto const setup = mtls::parseSection(section); EXPECT_TRUE(setup.enabled); EXPECT_TRUE(setup.useTls); - EXPECT_EQ(setup.tlsClientCertPath, mtls::clientCert); - EXPECT_EQ(setup.tlsClientKeyPath, mtls::clientKey); + EXPECT_TRUE(setup.tlsCertPath.empty()); + EXPECT_EQ(setup.tlsClientCertPath, cert); + EXPECT_EQ(setup.tlsClientKeyPath, key); } TEST(TelemetryConfig, mtls_cert_without_key_throws) @@ -252,20 +292,141 @@ TEST(TelemetryConfig, mtls_default_no_client_tls_is_accepted) TEST(TelemetryConfig, mtls_neither_set_is_one_way_tls) { - // Telemetry is on so the guards run, and this config must pass both: - // one-way TLS with a CA bundle and no client certificate. + // Telemetry is on so the checks run, and this config must pass all of + // them: one-way TLS with a CA bundle and no client certificate. The CA + // path has to name a real file, because the parser opens it here. + TempDir const dir; + auto const caCert = mtls::writeCertFile(dir.file("ca.pem")); Section section = mtls::makeSection(true); section.set("use_tls", "1"); - section.set("tls_ca_cert", "/etc/ssl/ca.pem"); + section.set("tls_ca_cert", caCert); auto const setup = mtls::parseSection(section); EXPECT_TRUE(setup.enabled); EXPECT_TRUE(setup.useTls); - EXPECT_EQ(setup.tlsCertPath, "/etc/ssl/ca.pem"); + EXPECT_EQ(setup.tlsCertPath, caCert); EXPECT_TRUE(setup.tlsClientCertPath.empty()); EXPECT_TRUE(setup.tlsClientKeyPath.empty()); } +TEST(TelemetryConfig, tls_missing_client_cert_file_throws) +{ + // Both client paths are set and use_tls=1, so neither contradiction guard + // can fire and the readability check is the only reachable throw. Only the + // certificate is absent, so the message must name that key and that path. + // + // This case and the two below use an absent file. A file that exists but + // denies read permission is deliberately not covered: a test process + // running as root reads it anyway, so the case would not be reliable. + TempDir const dir; + auto const absentCert = dir.file("absent.pem"); + Section section = mtls::makeSection(true); + section.set("use_tls", "1"); + section.set(mtls::keyClientCert, absentCert); + section.set(mtls::keyClientKey, mtls::writeCertFile(dir.file("k.pem"))); + + EXPECT_THAT( + [§ion] { mtls::parseSection(section); }, + ThrowsMessage(AllOf( + HasSubstr(mtls::readError), HasSubstr(mtls::keyClientCert), HasSubstr(absentCert)))); +} + +TEST(TelemetryConfig, tls_missing_client_key_file_throws) +{ + // The mirror image of the case above: the certificate is readable and only + // the private key is absent, so the key's name must appear instead. + TempDir const dir; + auto const absentKey = dir.file("absent.key"); + Section section = mtls::makeSection(true); + section.set("use_tls", "1"); + section.set(mtls::keyClientCert, mtls::writeCertFile(dir.file("c.pem"))); + section.set(mtls::keyClientKey, absentKey); + + EXPECT_THAT( + [§ion] { mtls::parseSection(section); }, + ThrowsMessage(AllOf( + HasSubstr(mtls::readError), HasSubstr(mtls::keyClientKey), HasSubstr(absentKey)))); +} + +TEST(TelemetryConfig, tls_missing_ca_cert_file_throws) +{ + // One-way TLS with no client certificate, so the CA bundle is the only + // path checked. + TempDir const dir; + auto const absentCa = dir.file("absent-ca.pem"); + Section section = mtls::makeSection(true); + section.set("use_tls", "1"); + section.set("tls_ca_cert", absentCa); + + EXPECT_THAT( + [§ion] { mtls::parseSection(section); }, + ThrowsMessage( + AllOf(HasSubstr(mtls::readError), HasSubstr("tls_ca_cert"), HasSubstr(absentCa)))); +} + +TEST(TelemetryConfig, tls_readable_files_are_accepted) +{ + // Full mTLS with all three files present and readable: parsing must + // succeed and keep every path verbatim. + TempDir const dir; + auto const ca = mtls::writeCertFile(dir.file("ca.pem")); + auto const cert = mtls::writeCertFile(dir.file("c.pem")); + auto const key = mtls::writeCertFile(dir.file("k.pem")); + Section section = mtls::makeSection(true); + section.set("use_tls", "1"); + section.set("tls_ca_cert", ca); + section.set(mtls::keyClientCert, cert); + section.set(mtls::keyClientKey, key); + + telemetry::Telemetry::Setup setup; + ASSERT_NO_THROW(setup = mtls::parseSection(section)); + EXPECT_TRUE(setup.enabled); + EXPECT_TRUE(setup.useTls); + EXPECT_EQ(setup.tlsCertPath, ca); + EXPECT_EQ(setup.tlsClientCertPath, cert); + EXPECT_EQ(setup.tlsClientKeyPath, key); +} + +TEST(TelemetryConfig, tls_paths_not_checked_when_telemetry_disabled) +{ + // Telemetry off, so the files are never opened and absent paths must not + // stop the node from booting. use_tls stays 1 here, so the `enabled` gate + // is the only thing that can be suppressing the check. + TempDir const dir; + auto const absentCert = dir.file("absent.pem"); + auto const absentKey = dir.file("absent.key"); + Section section = mtls::makeSection(false); + section.set("use_tls", "1"); + section.set(mtls::keyClientCert, absentCert); + section.set(mtls::keyClientKey, absentKey); + + telemetry::Telemetry::Setup setup; + ASSERT_NO_THROW(setup = mtls::parseSection(section)); + EXPECT_FALSE(setup.enabled); + EXPECT_TRUE(setup.useTls); + EXPECT_EQ(setup.tlsClientCertPath, absentCert); + EXPECT_EQ(setup.tlsClientKeyPath, absentKey); +} + +TEST(TelemetryConfig, tls_ca_cert_not_checked_when_use_tls_off) +{ + // With TLS off the exporter never reads the CA path, so a missing file + // must not stop startup. Telemetry stays on here, so the use_tls gate is + // the only thing that can be suppressing the check. The client-cert keys + // cannot be used for this case: they trip the use_tls contradiction guard + // before any file is opened. + TempDir const dir; + auto const absentCa = dir.file("absent-ca.pem"); + Section section = mtls::makeSection(true); + section.set("tls_ca_cert", absentCa); + + telemetry::Telemetry::Setup setup; + ASSERT_NO_THROW(setup = mtls::parseSection(section)); + EXPECT_TRUE(setup.enabled); + EXPECT_FALSE(setup.useTls); + EXPECT_EQ(setup.tlsCertPath, absentCa); +} + TEST(TelemetryConfig, null_telemetry_factory) { telemetry::Telemetry::Setup setup; From a24db2e995b29ea749b298e522ce7ef1de798541 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Fri, 21 Aug 2026 12:29:35 +0100 Subject: [PATCH 2/5] docs: Document the [telemetry] TLS path readability check Bring the three documentation surfaces in line with the new parse-time check: - The @throws clause on makeTelemetrySetup now names the third failure condition and records that an empty path is skipped. - cfg/xrpld-example.cfg states, under all three TLS keys, that with enabled=1 and use_tls=1 a path that does not exist or cannot be read stops startup. The tls_ca_cert wording still says that empty selects the system CA store, since only a path that is set is checked. - The runbook troubleshooting entry gains a third bullet for the "cannot be read" message, whose remedy is the path or its permissions rather than the certificate and key pairing. Documentation only; no behaviour change. --- cfg/xrpld-example.cfg | 19 ++++++++++++++----- docs/telemetry-runbook.md | 21 ++++++++++++++------- include/xrpl/telemetry/Telemetry.h | 5 ++++- 3 files changed, 32 insertions(+), 13 deletions(-) diff --git a/cfg/xrpld-example.cfg b/cfg/xrpld-example.cfg index a3bd8673ce..9f74c99c82 100644 --- a/cfg/xrpld-example.cfg +++ b/cfg/xrpld-example.cfg @@ -1714,6 +1714,11 @@ validators.txt # Path to a PEM-encoded CA certificate bundle for TLS verification. # Only used when use_tls=1. Default: empty (system CA store). # +# Leaving this empty stays valid and selects the system CA store. A path +# that is set is checked like the client paths below: with enabled=1 and +# use_tls=1, one that does not exist or cannot be read makes xrpld fail +# to start. +# # tls_client_cert= # # Path to this node's PEM-encoded client certificate, presented to the @@ -1723,15 +1728,19 @@ validators.txt # To enable mTLS, both tls_client_cert and tls_client_key must be # specified. If only one is provided, xrpld will fail to start. Providing # them while use_tls=0 also fails to start, rather than being ignored. -# Both checks apply only when enabled=1; with telemetry disabled these -# settings are read but never validated. +# With use_tls=1 each path is opened at startup, so one that does not +# exist or cannot be read fails to start too, rather than failing later +# as an opaque TLS handshake error. All three checks apply only when +# enabled=1; with telemetry disabled these settings are read but never +# validated. # # tls_client_key= # # Path to the PEM-encoded private key for tls_client_cert. Required -# whenever tls_client_cert is set. Requires use_tls=1. Both conditions -# are enforced exactly as described under tls_client_cert above: when -# enabled=1, breaking either one makes xrpld fail to start. +# whenever tls_client_cert is set. Requires use_tls=1, and must be +# readable. All three conditions are enforced exactly as described under +# tls_client_cert above: when enabled=1, breaking any one of them makes +# xrpld fail to start. # Default: empty. # # Head sampling is intentionally fixed at 1.0 (sample everything) and is diff --git a/docs/telemetry-runbook.md b/docs/telemetry-runbook.md index f30b87eaea..eb3a6e9c28 100644 --- a/docs/telemetry-runbook.md +++ b/docs/telemetry-runbook.md @@ -612,12 +612,13 @@ Three dashboards are pre-provisioned in `docker/telemetry/grafana/dashboards/`: exception thrown while the `Application` object is constructed prints the same `Unable to start` prefix, so confirm the text after the colon begins with `[telemetry]` before using this entry -- Cause: the `[telemetry]` mTLS keys (`tls_client_cert` and `tls_client_key`) - contradict each other. Only these two mTLS checks are gated on `enabled=1`; - the rest of the section is still read when telemetry is off, so a malformed - value in any key — including `enabled` itself, which is read before the gate - — still fails startup with a different message -- Fix: the two checks need different remedies, and the printed message says +- Cause: either the `[telemetry]` mTLS keys (`tls_client_cert` and + `tls_client_key`) contradict each other, or one of the TLS certificate paths + cannot be read. Only these three checks are gated on `enabled=1`; the rest of + the section is still read when telemetry is off, so a malformed value in any + key — including `enabled` itself, which is read before the gate — still fails + startup with a different message +- Fix: the three checks need different remedies, and the printed message says which one fired - `tls_client_cert and tls_client_key must be set together` — exactly one of the two paths is set. Either delete the one that is set, or add the missing @@ -626,8 +627,14 @@ Three dashboards are pre-provisioned in `docker/telemetry/grafana/dashboards/`: - `tls_client_cert/tls_client_key require use_tls=1` — both paths are set but TLS is off. Either set `use_tls=1`, or delete **both** paths. Deleting only one of them trips the first check + - ` cannot be read` — the named key (`tls_ca_cert`, `tls_client_cert` or + `tls_client_key`) points at a file the node cannot open; the message also + prints the path and the OS error. Fix the path or its permissions — the + pairing is not what is wrong here. This check runs only when `use_tls=1`, + and an empty `tls_ca_cert` is always accepted (it selects the system CA + store) - If you did not mean to enable telemetry at all, set `enabled=0` — that - clears both checks whichever one fired + clears all three checks whichever one fired ## Performance Tuning diff --git a/include/xrpl/telemetry/Telemetry.h b/include/xrpl/telemetry/Telemetry.h index a34e3cf19b..bf5af0156c 100644 --- a/include/xrpl/telemetry/Telemetry.h +++ b/include/xrpl/telemetry/Telemetry.h @@ -431,7 +431,10 @@ makeTelemetry(Telemetry::Setup const& setup, beast::Journal journal); * @return A populated Setup struct with defaults for missing values. * @throws std::runtime_error If `enabled` is set and the mutual TLS (mTLS) * settings contradict each other: only one of `tls_client_cert`/`tls_client_key` - * is given, or a client certificate is given while `use_tls` is 0. Those two + * is given, or a client certificate is given while `use_tls` is 0. Also if + * `enabled` and `use_tls` are both set and a non-empty `tls_ca_cert`, + * `tls_client_cert` or `tls_client_key` cannot be read; an empty path is skipped, + * so an empty `tls_ca_cert` still means "use the system CA store". All three * checks are skipped when `enabled` is 0. * @throws boost::bad_lexical_cast If any numeric key (`enabled`, `use_tls`, * `batch_size`, the trace switches, ...) holds a value Section::valueOr cannot From 31619219cc514d8a9e0c76f64231f68a4ef61cd1 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Mon, 24 Aug 2026 19:54:15 +0100 Subject: [PATCH 3/5] feat(telemetry): add set-once state attrs to consensus phase spans consensus.phase.open and consensus.establish carried almost no state of their own. Span attributes are not inherited, so the ledger context on the parent consensus.round span does not describe either child, and the few attributes the establish span did carry are rewritten on every iteration and therefore only ever report the final value. Add seven attributes that are read from state already in scope, are written exactly once, and are not duplicates of the parent round span: consensus.phase.open (start) start_reason initial, or recovered on a handleWrongLedger re-entry, which emplaces a SECOND phase.open span under the same round previous_close_agree feeds the sinceClose branch in phaseOpen() peer_positions_at_open positions in hand after playbackProposals(), the head start the round began with early_close_triggered the round skipped the timer because enough peers had already closed consensus.phase.open (end) tx_sets_acquired candidate tx sets held at close, read before our own position is added; a low count against a high peer_positions_at_close means tx-set fetches did not land, not disagreement consensus.establish (start) disputes_count_initial disputes carried in from the positions held at close, as opposed to disputes_count, which is overwritten each iteration consensus.establish (end) avalanche_state terminal close-time convergence regime; the derived avalanche_threshold is a weight and cannot be inverted back to the state The avalanche label is mapped by a new constexpr avalancheStateLabel() in ConsensusSpanNames.h rather than an inline switch, so the four labels stay under the naming check's L1 ownership and are unit-testable. Deliberately not added: ledger_seq and consensus_mode, which would only copy the parent round span's values down; tx set size and position hash, which the TxSet concept does not expose portably across RCLTxSet and the csf simulator; and the peer-unchanged and dead-node counters, whose underlying state is reset mid-round and so would report a misleading value. Behaviour is unchanged. The early-close condition is hoisted into a named local so the annotation happens before timerEntry(), which can reach closeLedger() and end the open-phase span. Tests pin the wire strings for every new key and value and cover all four enumerators of the avalanche mapping. They need no telemetry runtime: the csf simulator returns an invalid round span context and a null Telemetry, so consensus spans there are null guards and attribute writes are no-ops. --- include/xrpl/consensus/Consensus.h | 67 +++++++++++++- include/xrpl/consensus/ConsensusSpanNames.h | 82 ++++++++++++++++- .../libxrpl/telemetry/ConsensusSpanNames.cpp | 90 +++++++++++++++++++ 3 files changed, 234 insertions(+), 5 deletions(-) create mode 100644 src/tests/libxrpl/telemetry/ConsensusSpanNames.cpp diff --git a/include/xrpl/consensus/Consensus.h b/include/xrpl/consensus/Consensus.h index 0baef3acfc..207534012b 100644 --- a/include/xrpl/consensus/Consensus.h +++ b/include/xrpl/consensus/Consensus.h @@ -669,7 +669,8 @@ private: /** * Create the establish-phase span if not yet active. - * Called on each phaseEstablish() invocation; no-op while span is live. + * Called on each phaseEstablish() invocation; no-op while span is live, + * so the entry-state attributes it sets are written exactly once. */ void startEstablishTracing(); @@ -683,6 +684,9 @@ private: /** * End the establish span when transitioning to the accepted phase. + * Records the terminal avalanche_state before ending the span. A round + * that instead loses the establish span to a wrongLedger recovery omits + * the attribute rather than reporting a stale regime. */ void endEstablishTracing(); @@ -783,6 +787,22 @@ Consensus::startRoundInternal( openSpan_.emplace( telemetry::SpanGuard::childSpan( telemetry::consensus::span::phaseOpen, adaptor_.roundSpanContext())); + if (*openSpan_) + { + namespace cs = telemetry::consensus::span; + // Which entry path created this span. A handleWrongLedger recovery + // re-enters startRoundInternal and emplaces a SECOND phase.open span + // under the same round span, so without this the two are + // indistinguishable in a trace. + openSpan_->setAttribute( + cs::attr::startReason, + reason == StartRoundReason::Recovered ? std::string_view{cs::val::startRecovered} + : std::string_view{cs::val::startInitial}); + // Read from the parameter, not previousLedger_, which is not assigned + // until further down this function. Feeds the sinceClose calculation + // in phaseOpen(). + openSpan_->setAttribute(cs::attr::previousCloseAgree, prevLedger.closeAgree()); + } // On the Recovered path, fire phase.open here because startRoundTracing // (which fires it for the Initial path) is not called on re-entry. On // the Initial path this is a no-op because the round span hasn't been @@ -817,10 +837,25 @@ Consensus::startRoundInternal( playbackProposals(); CLOG(clog) << "number of peer proposals,previous proposers: " << currPeerPositions_.size() << ',' << prevProposers_ << ". "; - if (currPeerPositions_.size() > (prevProposers_ / 2)) + // We may be falling behind, don't wait for the timer + // consider closing the ledger immediately + bool const closeImmediately = currPeerPositions_.size() > (prevProposers_ / 2); + // Annotate before the timerEntry() below, which can reach closeLedger() + // and end this span. + if (openSpan_ && *openSpan_) + { + namespace cs = telemetry::consensus::span; + // Positions already in hand once playbackProposals() has replayed the + // buffered ones — the head start this round began with. Pairs with + // peer_positions_at_close on the same span. + openSpan_->setAttribute( + cs::attr::peerPositionsAtOpen, static_cast(currPeerPositions_.size())); + // Whether the round skipped waiting for the timer because enough peers + // had already closed. + openSpan_->setAttribute(cs::attr::earlyCloseTriggered, closeImmediately); + } + if (closeImmediately) { - // We may be falling behind, don't wait for the timer - // consider closing the ledger immediately CLOG(clog) << "consider closing the ledger immediately. "; timerEntry(now_, clog); } @@ -1560,6 +1595,11 @@ Consensus::closeLedger(std::unique_ptr const& clog) cs::attr::openDurationMs, static_cast(openTime_.read().count())); openSpan_->setAttribute( cs::attr::peerPositionsAtClose, static_cast(currPeerPositions_.size())); + // Candidate transaction sets held at close. Read before our own + // position is added below, so this counts only what peers shared. A + // low count against a high peer_positions_at_close means tx-set + // fetches did not land, not that peers disagreed. + openSpan_->setAttribute(cs::attr::txSetsAcquired, static_cast(acquired_.size())); } openSpan_.reset(); phase_ = ConsensusPhase::Establish; @@ -2113,6 +2153,16 @@ Consensus::startEstablishTracing() if (*establishSpan_) { establishSpanContext_ = establishSpan_->spanContext(); + // Disputes carried into the phase from the positions held at close. + // Set once (this function early-returns while the span is live), + // unlike disputes_count, which updateEstablishTracing() overwrites + // every iteration and so only ever reports the final value. + if (result_) + { + establishSpan_->setAttribute( + telemetry::consensus::span::attr::disputesCountInitial, + static_cast(result_->disputes.size())); + } } } @@ -2138,6 +2188,15 @@ template void Consensus::endEstablishTracing() { + // Terminal close-time convergence regime, recorded once before the span + // ends. The derived avalanche_threshold on consensus.update_positions is + // a weight, not the state, and cannot be inverted back to it. + if (establishSpan_ && *establishSpan_) + { + establishSpan_->setAttribute( + telemetry::consensus::span::attr::avalancheState, + telemetry::consensus::span::avalancheStateLabel(closeTimeAvalancheState_)); + } establishSpan_.reset(); establishSpanContext_ = telemetry::SpanContext{}; } diff --git a/include/xrpl/consensus/ConsensusSpanNames.h b/include/xrpl/consensus/ConsensusSpanNames.h index 6c79cd3132..dfc492d962 100644 --- a/include/xrpl/consensus/ConsensusSpanNames.h +++ b/include/xrpl/consensus/ConsensusSpanNames.h @@ -21,6 +21,9 @@ * +-- consensus.phase.open [main thread, child] * | Created: Consensus::startRoundInternal() * | Ended: Consensus::closeLedger() + * | Attrs: start_reason, previous_close_agree, peer_positions_at_open, + * | early_close_triggered (all at start); open_duration_ms, + * | peer_positions_at_close, tx_sets_acquired (all at end) * | * +-- consensus.proposal.send [main thread] * | Created: Adaptor::propose() @@ -33,7 +36,9 @@ * +-- consensus.establish [main thread, child] * | Created: Consensus::startEstablishTracing() * | Ended: Consensus::phaseEstablish() on accept - * | Attrs: converge_percent, establish_count, proposers + * | Attrs: disputes_count_initial (at start); converge_percent, + * | establish_count, proposers, disputes_count (overwritten + * | each iteration); avalanche_state (terminal, at end) * | * +-- consensus.update_positions [main thread] * | Created: Consensus::updateOurPositions() @@ -78,8 +83,11 @@ * +~~~ follows-from link (separate sub-tree, causal link) */ +#include #include +#include + namespace xrpl::telemetry::consensus::span { // ===== Span name segments ==================================================== @@ -169,11 +177,29 @@ inline constexpr auto previousProposers = makeStr("previous_proposers"); inline constexpr auto previousRoundTimeMs = makeStr("previous_round_time_ms"); inline constexpr auto previousLedgerSeq = makeStr("previous_ledger_seq"); inline constexpr auto closeTimeResolutionMs = makeStr("close_time_resolution_ms"); +/** + * Open-phase start metadata (set on consensus.phase.open at creation). + * + * `start_reason` distinguishes a fresh round from a handleWrongLedger + * recovery, which emits a SECOND consensus.phase.open span under the same + * round span; without it the two are indistinguishable in a trace. + * `early_close_triggered` records that startRoundInternal itself forced an + * immediate timerEntry because peers had already closed. + */ +inline constexpr auto startReason = makeStr("start_reason"); +inline constexpr auto previousCloseAgree = makeStr("previous_close_agree"); +inline constexpr auto peerPositionsAtOpen = makeStr("peer_positions_at_open"); +inline constexpr auto earlyCloseTriggered = makeStr("early_close_triggered"); /** * Open-phase end metadata (set on consensus.phase.open before reset). + * + * `tx_sets_acquired` counts the candidate transaction sets held when the + * ledger closed; a low count next to a high peer_positions_at_close points + * at missing tx-set fetches rather than at disagreement. */ inline constexpr auto openDurationMs = makeStr("open_duration_ms"); inline constexpr auto peerPositionsAtClose = makeStr("peer_positions_at_close"); +inline constexpr auto txSetsAcquired = makeStr("tx_sets_acquired"); /** * Ledger-close inputs. */ @@ -182,6 +208,19 @@ inline constexpr auto txCountOpen = makeStr("tx_count_open"); * Establish/check additional state. */ inline constexpr auto proposersFinished = makeStr("proposers_finished"); +/** + * Establish-phase start/end metadata. + * + * `disputes_count_initial` is the dispute count carried into the establish + * phase from the positions held at close. It is set once, unlike + * `disputes_count`, which updateEstablishTracing() overwrites every + * iteration and so only ever reports the final value. + * `avalanche_state` is the terminal close-time convergence regime, set once + * when the establish span ends; the derived `avalanche_threshold` on + * consensus.update_positions cannot be inverted back to it. + */ +inline constexpr auto disputesCountInitial = makeStr("disputes_count_initial"); +inline constexpr auto avalancheState = makeStr("avalanche_state"); /** * Accept/apply enrichment. */ @@ -291,6 +330,47 @@ inline constexpr auto unchanged = makeStr("unchanged"); inline constexpr auto phaseOpen = makeStr("open"); inline constexpr auto phaseEstablish = makeStr("establish"); inline constexpr auto phaseAccepted = makeStr("accepted"); +// start_reason values (how startRoundInternal was entered). +inline constexpr auto startInitial = makeStr("initial"); +inline constexpr auto startRecovered = makeStr("recovered"); +// avalanche_state values, one per ConsensusParms::AvalancheState enumerator. +inline constexpr auto avalancheInit = makeStr("init"); +inline constexpr auto avalancheMid = makeStr("mid"); +inline constexpr auto avalancheLate = makeStr("late"); +inline constexpr auto avalancheStuck = makeStr("stuck"); } // namespace val +/** + * Map a close-time avalanche state to its `avalanche_state` label. + * + * The regime escalates Init -> Mid -> Late -> Stuck as a round takes longer + * to converge, raising the close-time agreement threshold at each step. The + * label is recorded once, when the establish span ends, so the terminal + * regime of the round is queryable. + * + * @param state The state held by Consensus::closeTimeAvalancheState_. + * @return The wire label; one of val::avalanche*. + * + * @note constexpr so the call site costs nothing. Every enumerator is + * handled explicitly; there is no default arm, so adding an enumerator to + * ConsensusParms::AvalancheState makes the switch fall through to the + * unreachable return rather than silently mislabelling a new regime. + */ +[[nodiscard]] constexpr std::string_view +avalancheStateLabel(ConsensusParms::AvalancheState const state) +{ + switch (state) + { + case ConsensusParms::AvalancheState::Init: + return val::avalancheInit; + case ConsensusParms::AvalancheState::Mid: + return val::avalancheMid; + case ConsensusParms::AvalancheState::Late: + return val::avalancheLate; + case ConsensusParms::AvalancheState::Stuck: + return val::avalancheStuck; + } + return val::avalancheInit; +} + } // namespace xrpl::telemetry::consensus::span diff --git a/src/tests/libxrpl/telemetry/ConsensusSpanNames.cpp b/src/tests/libxrpl/telemetry/ConsensusSpanNames.cpp new file mode 100644 index 0000000000..ea8384038b --- /dev/null +++ b/src/tests/libxrpl/telemetry/ConsensusSpanNames.cpp @@ -0,0 +1,90 @@ +#include + +#include + +#include + +#include + +/** + * Contract tests for the consensus phase-span attribute constants. + * + * The keys in ConsensusSpanNames.h are the single source of truth (L1) that + * `.github/scripts/otel-naming/check_otel_naming.py` derives its valid key + * set from, and that the collector's spanmetrics dimensions, the Tempo span + * filters and the Grafana dashboards query by literal string. A silent rename + * here compiles cleanly but blanks panels, so these tests pin the wire values. + * They need no telemetry runtime and run in every build. + * + * Scope: the attributes carried by `consensus.phase.open` and + * `consensus.establish`. The round-level attrs are covered by the + * pre-existing key set and are deliberately NOT duplicated onto the phase + * children (a child span does not inherit parent attributes, but copying + * `ledger_seq` down would store the same value twice per trace). + */ + +using namespace xrpl::telemetry::consensus::span; + +TEST(ConsensusSpanNames, phase_open_start_attribute_keys) +{ + // Set once when the open-phase span is created in startRoundInternal(). + EXPECT_EQ(std::string_view(attr::startReason), "start_reason"); + EXPECT_EQ(std::string_view(attr::previousCloseAgree), "previous_close_agree"); + EXPECT_EQ(std::string_view(attr::peerPositionsAtOpen), "peer_positions_at_open"); + EXPECT_EQ(std::string_view(attr::earlyCloseTriggered), "early_close_triggered"); +} + +TEST(ConsensusSpanNames, phase_open_end_attribute_keys) +{ + // Existing end-of-phase metadata, pinned alongside the new key so a rename + // of either shows up here. + EXPECT_EQ(std::string_view(attr::openDurationMs), "open_duration_ms"); + EXPECT_EQ(std::string_view(attr::peerPositionsAtClose), "peer_positions_at_close"); + EXPECT_EQ(std::string_view(attr::txSetsAcquired), "tx_sets_acquired"); +} + +TEST(ConsensusSpanNames, establish_attribute_keys) +{ + EXPECT_EQ(std::string_view(attr::disputesCountInitial), "disputes_count_initial"); + EXPECT_EQ(std::string_view(attr::avalancheState), "avalanche_state"); +} + +TEST(ConsensusSpanNames, start_reason_values_are_the_two_entry_paths) +{ + // startRoundInternal() is entered fresh, or re-entered by handleWrongLedger + // after acquiring the correct prior ledger. A round that recovers emits a + // SECOND consensus.phase.open span, so the label is what tells them apart. + EXPECT_EQ(std::string_view(val::startInitial), "initial"); + EXPECT_EQ(std::string_view(val::startRecovered), "recovered"); +} + +TEST(ConsensusSpanNames, avalanche_state_values_match_the_parms_enum) +{ + EXPECT_EQ(std::string_view(val::avalancheInit), "init"); + EXPECT_EQ(std::string_view(val::avalancheMid), "mid"); + EXPECT_EQ(std::string_view(val::avalancheLate), "late"); + EXPECT_EQ(std::string_view(val::avalancheStuck), "stuck"); +} + +TEST(ConsensusSpanNames, avalanche_state_label_maps_every_enum_state) +{ + // A missed branch here would silently report the wrong convergence regime + // for the round, so every enumerator is asserted explicitly rather than + // round-tripped through a table. + using AvalancheState = xrpl::ConsensusParms::AvalancheState; + + EXPECT_EQ(avalancheStateLabel(AvalancheState::Init), "init"); + EXPECT_EQ(avalancheStateLabel(AvalancheState::Mid), "mid"); + EXPECT_EQ(avalancheStateLabel(AvalancheState::Late), "late"); + EXPECT_EQ(avalancheStateLabel(AvalancheState::Stuck), "stuck"); +} + +TEST(ConsensusSpanNames, avalanche_state_label_is_usable_at_compile_time) +{ + // The mapping is consteval-safe so the label costs nothing at the call + // site in endEstablishTracing(). + static_assert( + avalancheStateLabel(xrpl::ConsensusParms::AvalancheState::Stuck) == "stuck", + "avalancheStateLabel must be constexpr-evaluable"); + SUCCEED(); +} From 6eaf7316e5a146c3143e04f3c1add45369c70f3c Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Mon, 24 Aug 2026 20:13:39 +0100 Subject: [PATCH 4/5] feat(telemetry): record the ledger close reason on consensus.phase.open The open phase ended for one of four distinct reasons, but shouldCloseLedger() collapsed them into a bool, so a trace could say when a phase ended and never why. "The network closed without us" and "nothing was waiting" are the same span today. Add whyCloseLedger(), which holds the decision and returns LedgerCloseReason. shouldCloseLedger() keeps its exact signature and becomes a one-line delegation, so its callers and unit tests are untouched and the branch logic is not duplicated. phaseOpen() calls whyCloseLedger() directly; both emit the same journal and CLOG output, so only one is called. New attributes on consensus.phase.open, both set once on the closing tick: close_reason anomaly | others_closed | idle | normal proposers_validated trusted peers that had already validated the prior ledger, reusing the value the decision was made on Absent on the simulate() close path, which bypasses the decision rather than having a reason invented for it. Skipped has_open_transactions: hasOpenTransactions() is !getOpenLedger().empty(), which is false on a quiet network for most of a round, and close_reason=idle already implies it. The sibling consensus.ledger_close span carries tx_count_open, which is the same fact with a count instead of a boolean. shouldCloseLedger() now has no production caller; it stays exported so the public API and its tests are unchanged. Tests pin every input vector from should_close_ledger to its literal reason, including that the anomaly check outranks others-closed, and cover the inclusive idle boundary either side by one millisecond. --- include/xrpl/consensus/Consensus.h | 99 ++++++++++++------- include/xrpl/consensus/ConsensusSpanNames.h | 75 +++++++++----- include/xrpl/consensus/ConsensusTypes.h | 35 +++++++ src/libxrpl/consensus/Consensus.cpp | 46 +++++++-- src/tests/libxrpl/consensus/Consensus.cpp | 78 +++++++++++++++ .../libxrpl/telemetry/ConsensusSpanNames.cpp | 33 +++++++ 6 files changed, 297 insertions(+), 69 deletions(-) diff --git a/include/xrpl/consensus/Consensus.h b/include/xrpl/consensus/Consensus.h index 207534012b..5922801e1c 100644 --- a/include/xrpl/consensus/Consensus.h +++ b/include/xrpl/consensus/Consensus.h @@ -31,12 +31,37 @@ namespace xrpl { +/** + * Determines why the current ledger should close at this time. + * + * Holds the close decision that shouldCloseLedger() reduces to a bool. Call + * this one when the deciding branch matters. Both log identically, so call + * one or the other, never both. Parameters match shouldCloseLedger(). + * + * @return The deciding branch, or KeepOpen if no close condition is met. + */ +LedgerCloseReason +whyCloseLedger( + bool anyTransactions, + std::size_t prevProposers, + std::size_t proposersClosed, + std::size_t proposersValidated, + std::chrono::milliseconds prevRoundTime, + std::chrono::milliseconds timeSincePrevClose, + std::chrono::milliseconds openTime, + std::chrono::milliseconds idleInterval, + ConsensusParms const& parms, + beast::Journal j, + std::unique_ptr const& clog = {}); + /** * Determines whether the current ledger should close at this time. * * This function should be called when a ledger is open and there is no close * in progress, or when a transaction is received and no close is in progress. * + * Equivalent to `whyCloseLedger(...) != LedgerCloseReason::KeepOpen`. + * * @param anyTransactions indicates whether any transactions have been received * @param prevProposers proposers in the last closing * @param proposersClosed proposers who have currently closed this ledger @@ -790,17 +815,13 @@ Consensus::startRoundInternal( if (*openSpan_) { namespace cs = telemetry::consensus::span; - // Which entry path created this span. A handleWrongLedger recovery - // re-enters startRoundInternal and emplaces a SECOND phase.open span - // under the same round span, so without this the two are - // indistinguishable in a trace. + // A recovery emplaces a SECOND phase.open span under the same round, + // so this is what tells the two apart. openSpan_->setAttribute( cs::attr::startReason, reason == StartRoundReason::Recovered ? std::string_view{cs::val::startRecovered} : std::string_view{cs::val::startInitial}); - // Read from the parameter, not previousLedger_, which is not assigned - // until further down this function. Feeds the sinceClose calculation - // in phaseOpen(). + // From the parameter: previousLedger_ is not assigned until below. openSpan_->setAttribute(cs::attr::previousCloseAgree, prevLedger.closeAgree()); } // On the Recovered path, fire phase.open here because startRoundTracing @@ -840,18 +861,14 @@ Consensus::startRoundInternal( // We may be falling behind, don't wait for the timer // consider closing the ledger immediately bool const closeImmediately = currPeerPositions_.size() > (prevProposers_ / 2); - // Annotate before the timerEntry() below, which can reach closeLedger() - // and end this span. + // Annotate before the timerEntry() below, which can end this span. if (openSpan_ && *openSpan_) { namespace cs = telemetry::consensus::span; - // Positions already in hand once playbackProposals() has replayed the - // buffered ones — the head start this round began with. Pairs with - // peer_positions_at_close on the same span. + // Head start after playbackProposals() replayed the buffered + // positions. Pairs with peer_positions_at_close. openSpan_->setAttribute( cs::attr::peerPositionsAtOpen, static_cast(currPeerPositions_.size())); - // Whether the round skipped waiting for the timer because enough peers - // had already closed. openSpan_->setAttribute(cs::attr::earlyCloseTriggered, closeImmediately); } if (closeImmediately) @@ -1344,20 +1361,32 @@ Consensus::phaseOpen(std::unique_ptr const& clog) << ", previous ledger close time resolution: " << previousLedger_.closeTimeResolution().count() << "ms. "; - // Decide if we should close the ledger - if (shouldCloseLedger( - anyTransactions, - prevProposers_, - proposersClosed, - proposersValidated, - prevRoundTime_, - sinceClose, - openTime_.read(), - idleInterval, - adaptor_.parms(), - j_, - clog)) + // Decide if we should close the ledger. whyCloseLedger() so the deciding + // branch can be recorded; it logs the same, so only one is called. + LedgerCloseReason const closeReason = whyCloseLedger( + anyTransactions, + prevProposers_, + proposersClosed, + proposersValidated, + prevRoundTime_, + sinceClose, + openTime_.read(), + idleInterval, + adaptor_.parms(), + j_, + clog); + if (closeReason != LedgerCloseReason::KeepOpen) { + // Annotate before closeLedger() ends the span. Set once: the phase + // moves to Establish, so phaseOpen() is not entered again this round. + // Absent on the simulate() path, which bypasses this decision. + if (openSpan_ && *openSpan_) + { + namespace cs = telemetry::consensus::span; + openSpan_->setAttribute(cs::attr::closeReason, cs::closeReasonLabel(closeReason)); + openSpan_->setAttribute( + cs::attr::proposersValidated, static_cast(proposersValidated)); + } CLOG(clog) << "closing ledger. "; closeLedger(clog); } @@ -1595,10 +1624,8 @@ Consensus::closeLedger(std::unique_ptr const& clog) cs::attr::openDurationMs, static_cast(openTime_.read().count())); openSpan_->setAttribute( cs::attr::peerPositionsAtClose, static_cast(currPeerPositions_.size())); - // Candidate transaction sets held at close. Read before our own - // position is added below, so this counts only what peers shared. A - // low count against a high peer_positions_at_close means tx-set - // fetches did not land, not that peers disagreed. + // Read before our own position is added below, so this counts only + // what peers shared. openSpan_->setAttribute(cs::attr::txSetsAcquired, static_cast(acquired_.size())); } openSpan_.reset(); @@ -2153,10 +2180,8 @@ Consensus::startEstablishTracing() if (*establishSpan_) { establishSpanContext_ = establishSpan_->spanContext(); - // Disputes carried into the phase from the positions held at close. - // Set once (this function early-returns while the span is live), - // unlike disputes_count, which updateEstablishTracing() overwrites - // every iteration and so only ever reports the final value. + // Disputes carried in from the positions held at close. Set once: + // this function early-returns while the span is live. if (result_) { establishSpan_->setAttribute( @@ -2188,9 +2213,7 @@ template void Consensus::endEstablishTracing() { - // Terminal close-time convergence regime, recorded once before the span - // ends. The derived avalanche_threshold on consensus.update_positions is - // a weight, not the state, and cannot be inverted back to it. + // Terminal convergence regime, recorded once before the span ends. if (establishSpan_ && *establishSpan_) { establishSpan_->setAttribute( diff --git a/include/xrpl/consensus/ConsensusSpanNames.h b/include/xrpl/consensus/ConsensusSpanNames.h index dfc492d962..cb7d7a8c85 100644 --- a/include/xrpl/consensus/ConsensusSpanNames.h +++ b/include/xrpl/consensus/ConsensusSpanNames.h @@ -84,6 +84,7 @@ */ #include +#include #include #include @@ -180,11 +181,8 @@ inline constexpr auto closeTimeResolutionMs = makeStr("close_time_resolution_ms" /** * Open-phase start metadata (set on consensus.phase.open at creation). * - * `start_reason` distinguishes a fresh round from a handleWrongLedger - * recovery, which emits a SECOND consensus.phase.open span under the same - * round span; without it the two are indistinguishable in a trace. - * `early_close_triggered` records that startRoundInternal itself forced an - * immediate timerEntry because peers had already closed. + * A handleWrongLedger recovery emits a SECOND phase.open span under the same + * round, so `start_reason` is what tells the two apart. */ inline constexpr auto startReason = makeStr("start_reason"); inline constexpr auto previousCloseAgree = makeStr("previous_close_agree"); @@ -193,13 +191,16 @@ inline constexpr auto earlyCloseTriggered = makeStr("early_close_triggered"); /** * Open-phase end metadata (set on consensus.phase.open before reset). * - * `tx_sets_acquired` counts the candidate transaction sets held when the - * ledger closed; a low count next to a high peer_positions_at_close points - * at missing tx-set fetches rather than at disagreement. + * A low `tx_sets_acquired` next to a high peer_positions_at_close points at + * missing tx-set fetches rather than at disagreement. `close_reason` plus + * `proposers_validated` separate "the network moved on without us" from "the + * network was quiet". */ inline constexpr auto openDurationMs = makeStr("open_duration_ms"); inline constexpr auto peerPositionsAtClose = makeStr("peer_positions_at_close"); inline constexpr auto txSetsAcquired = makeStr("tx_sets_acquired"); +inline constexpr auto closeReason = makeStr("close_reason"); +inline constexpr auto proposersValidated = makeStr("proposers_validated"); /** * Ledger-close inputs. */ @@ -211,13 +212,10 @@ inline constexpr auto proposersFinished = makeStr("proposers_finished"); /** * Establish-phase start/end metadata. * - * `disputes_count_initial` is the dispute count carried into the establish - * phase from the positions held at close. It is set once, unlike - * `disputes_count`, which updateEstablishTracing() overwrites every - * iteration and so only ever reports the final value. - * `avalanche_state` is the terminal close-time convergence regime, set once - * when the establish span ends; the derived `avalanche_threshold` on - * consensus.update_positions cannot be inverted back to it. + * Both are set once, unlike `disputes_count`, which + * updateEstablishTracing() overwrites every iteration. + * `avalanche_state` is the terminal regime; the derived + * `avalanche_threshold` cannot be inverted back to it. */ inline constexpr auto disputesCountInitial = makeStr("disputes_count_initial"); inline constexpr auto avalancheState = makeStr("avalanche_state"); @@ -338,23 +336,26 @@ inline constexpr auto avalancheInit = makeStr("init"); inline constexpr auto avalancheMid = makeStr("mid"); inline constexpr auto avalancheLate = makeStr("late"); inline constexpr auto avalancheStuck = makeStr("stuck"); +// close_reason values, one per LedgerCloseReason enumerator. keep_open is +// never emitted: the attribute is only set on the path that closes. +inline constexpr auto closeKeepOpen = makeStr("keep_open"); +inline constexpr auto closeAnomaly = makeStr("anomaly"); +inline constexpr auto closeOthersClosed = makeStr("others_closed"); +inline constexpr auto closeIdle = makeStr("idle"); +inline constexpr auto closeNormal = makeStr("normal"); } // namespace val /** * Map a close-time avalanche state to its `avalanche_state` label. * - * The regime escalates Init -> Mid -> Late -> Stuck as a round takes longer - * to converge, raising the close-time agreement threshold at each step. The - * label is recorded once, when the establish span ends, so the terminal - * regime of the round is queryable. + * The regime escalates Init -> Mid -> Late -> Stuck, raising the close-time + * agreement threshold at each step. * * @param state The state held by Consensus::closeTimeAvalancheState_. * @return The wire label; one of val::avalanche*. * - * @note constexpr so the call site costs nothing. Every enumerator is - * handled explicitly; there is no default arm, so adding an enumerator to - * ConsensusParms::AvalancheState makes the switch fall through to the - * unreachable return rather than silently mislabelling a new regime. + * @note No default arm, so a new enumerator is a compiler warning rather than + * a silently reused label. */ [[nodiscard]] constexpr std::string_view avalancheStateLabel(ConsensusParms::AvalancheState const state) @@ -373,4 +374,32 @@ avalancheStateLabel(ConsensusParms::AvalancheState const state) return val::avalancheInit; } +/** + * Map a ledger-close decision to its `close_reason` label. + * + * @param reason The value returned by whyCloseLedger(). + * @return The wire label; one of val::close*. + * + * @note No default arm, so a new enumerator is a compiler warning rather than + * a silently reused label. `keep_open` is mapped but never emitted. + */ +[[nodiscard]] constexpr std::string_view +closeReasonLabel(LedgerCloseReason const reason) +{ + switch (reason) + { + case LedgerCloseReason::KeepOpen: + return val::closeKeepOpen; + case LedgerCloseReason::Anomaly: + return val::closeAnomaly; + case LedgerCloseReason::OthersClosed: + return val::closeOthersClosed; + case LedgerCloseReason::Idle: + return val::closeIdle; + case LedgerCloseReason::Normal: + return val::closeNormal; + } + return val::closeKeepOpen; +} + } // namespace xrpl::telemetry::consensus::span diff --git a/include/xrpl/consensus/ConsensusTypes.h b/include/xrpl/consensus/ConsensusTypes.h index 11231de40c..74b25e4fa7 100644 --- a/include/xrpl/consensus/ConsensusTypes.h +++ b/include/xrpl/consensus/ConsensusTypes.h @@ -154,6 +154,41 @@ to_string(ConsensusPhase p) } } +/** + * Why the open ledger should, or should not, close right now. + * + * Returned by whyCloseLedger(); shouldCloseLedger() reduces it to a bool. + * + * @note KeepOpen is not a close reason. Compare against it rather than + * treating the enum as a flag. + */ +enum class LedgerCloseReason : std::uint8_t { + /** + * No close condition is met yet. + */ + KeepOpen, + + /** + * Timings out of range; close defensively. + */ + Anomaly, + + /** + * More than half the network has closed or validated. + */ + OthersClosed, + + /** + * Nothing waiting and the idle interval elapsed. + */ + Idle, + + /** + * Transactions waiting and both minimum-open floors met. + */ + Normal, +}; + /** * Measures the duration of phases of consensus */ diff --git a/src/libxrpl/consensus/Consensus.cpp b/src/libxrpl/consensus/Consensus.cpp index 6f398cf66c..50d9a82d5f 100644 --- a/src/libxrpl/consensus/Consensus.cpp +++ b/src/libxrpl/consensus/Consensus.cpp @@ -13,8 +13,8 @@ namespace xrpl { -bool -shouldCloseLedger( +LedgerCloseReason +whyCloseLedger( bool anyTransactions, std::size_t prevProposers, std::size_t proposersClosed, @@ -47,7 +47,7 @@ shouldCloseLedger( JLOG(j.warn()) << ss.str(); CLOG(clog) << "closing ledger: " << ss.str() << ". "; - return true; + return LedgerCloseReason::Anomaly; } if ((proposersClosed + proposersValidated) > (prevProposers / 2)) @@ -55,14 +55,16 @@ shouldCloseLedger( // If more than half of the network has closed, we close JLOG(j.trace()) << "Others have closed"; CLOG(clog) << "closing ledger because enough others have already. "; - return true; + return LedgerCloseReason::OthersClosed; } if (!anyTransactions) { // Only close at the end of the idle interval CLOG(clog) << "no transactions, returning. "; - return timeSincePrevClose >= idleInterval; // normal idle + return timeSincePrevClose >= idleInterval // normal idle + ? LedgerCloseReason::Idle + : LedgerCloseReason::KeepOpen; } // Preserve minimum ledger open time @@ -70,7 +72,7 @@ shouldCloseLedger( { JLOG(j.debug()) << "Must wait minimum time before closing"; CLOG(clog) << "not closing because under ledgerMIN_CLOSE. "; - return false; + return LedgerCloseReason::KeepOpen; } // Don't let this ledger close more than twice as fast as the previous @@ -80,12 +82,40 @@ shouldCloseLedger( { JLOG(j.debug()) << "Ledger has not been open long enough"; CLOG(clog) << "not closing because not open long enough. "; - return false; + return LedgerCloseReason::KeepOpen; } // Close the ledger CLOG(clog) << "no reason to not close. "; - return true; + return LedgerCloseReason::Normal; +} + +bool +shouldCloseLedger( + bool anyTransactions, + std::size_t prevProposers, + std::size_t proposersClosed, + std::size_t proposersValidated, + std::chrono::milliseconds prevRoundTime, + std::chrono::milliseconds timeSincePrevClose, + std::chrono::milliseconds openTime, + std::chrono::milliseconds idleInterval, + ConsensusParms const& parms, + beast::Journal j, + std::unique_ptr const& clog) +{ + return whyCloseLedger( + anyTransactions, + prevProposers, + proposersClosed, + proposersValidated, + prevRoundTime, + timeSincePrevClose, + openTime, + idleInterval, + parms, + j, + clog) != LedgerCloseReason::KeepOpen; } bool diff --git a/src/tests/libxrpl/consensus/Consensus.cpp b/src/tests/libxrpl/consensus/Consensus.cpp index d303d28e89..c0298b64a7 100644 --- a/src/tests/libxrpl/consensus/Consensus.cpp +++ b/src/tests/libxrpl/consensus/Consensus.cpp @@ -66,6 +66,33 @@ shouldCloseLedger( clog); } +LedgerCloseReason +whyCloseLedger( + bool anyTransactions, + std::size_t prevProposers, + std::size_t proposersClosed, + std::size_t proposersValidated, + std::chrono::milliseconds prevRoundTime, + std::chrono::milliseconds timeSincePrevClose, + std::chrono::milliseconds openTime, + std::chrono::milliseconds idleInterval, + ConsensusParms const& parms, + std::unique_ptr const& clog = {}) +{ + return xrpl::whyCloseLedger( + anyTransactions, + prevProposers, + proposersClosed, + proposersValidated, + prevRoundTime, + timeSincePrevClose, + openTime, + idleInterval, + parms, + journal(), + clog); +} + ConsensusState checkConsensus( std::size_t prevProposers, @@ -219,6 +246,57 @@ TEST(ConsensusTest, should_close_ledger) EXPECT_TRUE(shouldCloseLedger(true, 10, 0, 0, 10s, 10s, 10s, 10s, p)); } +TEST(ConsensusTest, why_close_ledger_reports_the_deciding_branch) +{ + using namespace std::chrono_literals; + SCOPED_TRACE("why close ledger"); + + // Same input vectors as should_close_ledger above, pinned to the reason + // rather than the bool, so a branch that starts returning the wrong + // reason is caught even though the close/no-close verdict is unchanged. + ConsensusParms const p{}; + + // Bizarre times forcibly close. These vectors ALSO satisfy the + // others-closed condition (8 > 10/2), so they pin the precedence: the + // anomaly check runs first. + EXPECT_EQ(whyCloseLedger(true, 10, 10, 10, -10s, 10s, 1s, 1s, p), LedgerCloseReason::Anomaly); + EXPECT_EQ(whyCloseLedger(true, 10, 10, 10, 100h, 10s, 1s, 1s, p), LedgerCloseReason::Anomaly); + EXPECT_EQ(whyCloseLedger(true, 10, 10, 10, 10s, 100h, 1s, 1s, p), LedgerCloseReason::Anomaly); + + // Rest of network has closed: 3 closed + 5 validated > 10/2. + EXPECT_EQ( + whyCloseLedger(true, 10, 3, 5, 10s, 10s, 10s, 10s, p), LedgerCloseReason::OthersClosed); + + // No transactions: keep open until the idle interval elapses, then close + // as idle rather than as a normal close. + EXPECT_EQ(whyCloseLedger(false, 10, 0, 0, 1s, 1s, 1s, 10s, p), LedgerCloseReason::KeepOpen); + EXPECT_EQ(whyCloseLedger(false, 10, 0, 0, 1s, 10s, 1s, 10s, p), LedgerCloseReason::Idle); + + // Transactions present, but under ledgerMinClose (2s). + EXPECT_EQ(whyCloseLedger(true, 10, 0, 0, 10s, 10s, 1s, 10s, p), LedgerCloseReason::KeepOpen); + + // Past ledgerMinClose but under prevRoundTime/2 (5s), so still too fast. + EXPECT_EQ(whyCloseLedger(true, 10, 0, 0, 10s, 10s, 3s, 10s, p), LedgerCloseReason::KeepOpen); + + // Both minimum-open constraints satisfied. + EXPECT_EQ(whyCloseLedger(true, 10, 0, 0, 10s, 10s, 10s, 10s, p), LedgerCloseReason::Normal); +} + +TEST(ConsensusTest, why_close_ledger_idle_boundary_is_inclusive) +{ + using namespace std::chrono_literals; + SCOPED_TRACE("idle boundary"); + + // The idle path closes on `timeSincePrevClose >= idleInterval`. One + // millisecond either side of the boundary, to pin the comparison as + // inclusive rather than strict. + ConsensusParms const p{}; + + EXPECT_EQ(whyCloseLedger(false, 10, 0, 0, 1s, 9999ms, 1s, 10s, p), LedgerCloseReason::KeepOpen); + EXPECT_EQ(whyCloseLedger(false, 10, 0, 0, 1s, 10s, 1s, 10s, p), LedgerCloseReason::Idle); + EXPECT_EQ(whyCloseLedger(false, 10, 0, 0, 1s, 10001ms, 1s, 10s, p), LedgerCloseReason::Idle); +} + TEST(ConsensusTest, check_consensus) { using namespace std::chrono_literals; diff --git a/src/tests/libxrpl/telemetry/ConsensusSpanNames.cpp b/src/tests/libxrpl/telemetry/ConsensusSpanNames.cpp index ea8384038b..d371b67789 100644 --- a/src/tests/libxrpl/telemetry/ConsensusSpanNames.cpp +++ b/src/tests/libxrpl/telemetry/ConsensusSpanNames.cpp @@ -41,6 +41,39 @@ TEST(ConsensusSpanNames, phase_open_end_attribute_keys) EXPECT_EQ(std::string_view(attr::openDurationMs), "open_duration_ms"); EXPECT_EQ(std::string_view(attr::peerPositionsAtClose), "peer_positions_at_close"); EXPECT_EQ(std::string_view(attr::txSetsAcquired), "tx_sets_acquired"); + EXPECT_EQ(std::string_view(attr::closeReason), "close_reason"); + EXPECT_EQ(std::string_view(attr::proposersValidated), "proposers_validated"); +} + +TEST(ConsensusSpanNames, close_reason_values_are_the_close_paths) +{ + // One per branch of whyCloseLedger() that closes the ledger. keep_open is + // never emitted (the attribute is only set on the closing path) but is + // labelled rather than left blank so the mapping is total. + EXPECT_EQ(std::string_view(val::closeKeepOpen), "keep_open"); + EXPECT_EQ(std::string_view(val::closeAnomaly), "anomaly"); + EXPECT_EQ(std::string_view(val::closeOthersClosed), "others_closed"); + EXPECT_EQ(std::string_view(val::closeIdle), "idle"); + EXPECT_EQ(std::string_view(val::closeNormal), "normal"); +} + +TEST(ConsensusSpanNames, close_reason_label_maps_every_enum_state) +{ + // A missed branch would attribute a close to the wrong cause, which is the + // whole point of the attribute, so every enumerator is asserted. + EXPECT_EQ(closeReasonLabel(xrpl::LedgerCloseReason::KeepOpen), "keep_open"); + EXPECT_EQ(closeReasonLabel(xrpl::LedgerCloseReason::Anomaly), "anomaly"); + EXPECT_EQ(closeReasonLabel(xrpl::LedgerCloseReason::OthersClosed), "others_closed"); + EXPECT_EQ(closeReasonLabel(xrpl::LedgerCloseReason::Idle), "idle"); + EXPECT_EQ(closeReasonLabel(xrpl::LedgerCloseReason::Normal), "normal"); +} + +TEST(ConsensusSpanNames, close_reason_label_is_usable_at_compile_time) +{ + static_assert( + closeReasonLabel(xrpl::LedgerCloseReason::Idle) == "idle", + "closeReasonLabel must be constexpr-evaluable"); + SUCCEED(); } TEST(ConsensusSpanNames, establish_attribute_keys) From f4718cee0871774ce9bcb55702fca9c34c692939 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Mon, 24 Aug 2026 20:40:00 +0100 Subject: [PATCH 5/5] fix(telemetry): correct mistimed and ambiguous consensus phase attributes Three defects found in review of the two preceding commits. Drop disputes_count_initial. It claimed to be the dispute count carried in from the positions held at close, but startEstablishTracing() runs a full timer tick after closeLedger(): timerEntry() dispatches `if (phase_ == Open) phaseOpen(); else if (phase_ == Establish) phaseEstablish();`, and phase_ was Open on the closing tick, so the else-if cannot run. With ledgerGRANULARITY at 1s the value absorbed up to a second of dispute growth from peer proposals and arriving tx sets. Making it honest needs either a member captured at close or moving span creation into closeLedger(), so it is removed rather than shipped mislabelled. Record close_time_avalanche_state on recovered rounds. startRoundInternal() reset establishSpan_ inline, discarding the span before the attribute was written, so the value was present only on rounds that reached Accepted -- survivor bias in exactly the rounds worth investigating. It now calls endEstablishTracing(). The comment claiming this avoided "reporting a stale regime" was wrong: closeTimeAvalancheState_ is not reset until 39 lines later, so the value was still that span's terminal regime. Rename avalanche_state to close_time_avalanche_state. DisputedTx carries a second, per-transaction avalanche tracker; the bare name invited reading a close-time-only value as the transaction one, which is the tracker that actually escalates in a stuck round. Also: both label helpers now fall through to "unknown" instead of a plausible-looking regime, matching to_string(ConsensusPhase); and the header now records that the end-of-open attributes are absent on recovered and simulated rounds, and that tx_sets_acquired can skew either way because handleWrongLedger clears currPeerPositions_ but not acquired_. Tests: the minimum-open-time assertion used prevRoundTime=10s, where openTime=1s trips the too-fast branch as well, so deleting the ledgerMinClose check entirely left it green. Replaced with prevRoundTime=2s, which isolates the branch. Added the others-closed boundary, which is strict and was untested in either direction, its integer truncation for odd prevProposers, and its precedence over the no-transactions and minimum-open branches. --- include/xrpl/consensus/Consensus.h | 31 ++++-------- include/xrpl/consensus/ConsensusSpanNames.h | 50 ++++++++++--------- src/tests/libxrpl/consensus/Consensus.cpp | 31 ++++++++++-- .../libxrpl/telemetry/ConsensusSpanNames.cpp | 5 +- 4 files changed, 69 insertions(+), 48 deletions(-) diff --git a/include/xrpl/consensus/Consensus.h b/include/xrpl/consensus/Consensus.h index 5922801e1c..577b154cbe 100644 --- a/include/xrpl/consensus/Consensus.h +++ b/include/xrpl/consensus/Consensus.h @@ -694,8 +694,7 @@ private: /** * Create the establish-phase span if not yet active. - * Called on each phaseEstablish() invocation; no-op while span is live, - * so the entry-state attributes it sets are written exactly once. + * Called on each phaseEstablish() invocation; no-op while span is live. */ void startEstablishTracing(); @@ -708,10 +707,9 @@ private: updateEstablishTracing(); /** - * End the establish span when transitioning to the accepted phase. - * Records the terminal avalanche_state before ending the span. A round - * that instead loses the establish span to a wrongLedger recovery omits - * the attribute rather than reporting a stale regime. + * End the establish span, recording its terminal regime. + * Also called from startRoundInternal() on a wrongLedger recovery, so a + * round that never reaches Accepted still reports the regime it reached. */ void endEstablishTracing(); @@ -799,11 +797,12 @@ Consensus::startRoundInternal( CLOG(clog) << "startRoundInternal transitioned to ConsensusPhase::Open, " "previous ledgerID: " << prevLedgerID << ", seq: " << prevLedger.seq() << ". "; - // Reset establishSpan_ so a wrongLedger recovery mid-establish doesn't - // leak the prior round's span into the new one (startEstablishTracing - // early-returns when establishSpan_ is populated). - establishSpan_.reset(); - establishSpanContext_ = telemetry::SpanContext{}; + // End establishSpan_ so a wrongLedger recovery mid-establish doesn't leak + // the prior round's span into the new one (startEstablishTracing + // early-returns when establishSpan_ is populated). Via + // endEstablishTracing() so the recovered round still records its terminal + // regime; closeTimeAvalancheState_ is not reset until further down. + endEstablishTracing(); // Child of the round span via its captured context: parent phase.open // explicitly under roundSpanContext_. An invalid round context (round span // not yet created) yields a null guard. openSpan_ is a thread-free @@ -2180,14 +2179,6 @@ Consensus::startEstablishTracing() if (*establishSpan_) { establishSpanContext_ = establishSpan_->spanContext(); - // Disputes carried in from the positions held at close. Set once: - // this function early-returns while the span is live. - if (result_) - { - establishSpan_->setAttribute( - telemetry::consensus::span::attr::disputesCountInitial, - static_cast(result_->disputes.size())); - } } } @@ -2217,7 +2208,7 @@ Consensus::endEstablishTracing() if (establishSpan_ && *establishSpan_) { establishSpan_->setAttribute( - telemetry::consensus::span::attr::avalancheState, + telemetry::consensus::span::attr::closeTimeAvalancheState, telemetry::consensus::span::avalancheStateLabel(closeTimeAvalancheState_)); } establishSpan_.reset(); diff --git a/include/xrpl/consensus/ConsensusSpanNames.h b/include/xrpl/consensus/ConsensusSpanNames.h index cb7d7a8c85..d6715f2732 100644 --- a/include/xrpl/consensus/ConsensusSpanNames.h +++ b/include/xrpl/consensus/ConsensusSpanNames.h @@ -22,8 +22,11 @@ * | Created: Consensus::startRoundInternal() * | Ended: Consensus::closeLedger() * | Attrs: start_reason, previous_close_agree, peer_positions_at_open, - * | early_close_triggered (all at start); open_duration_ms, - * | peer_positions_at_close, tx_sets_acquired (all at end) + * | early_close_triggered (at start); open_duration_ms, + * | peer_positions_at_close, tx_sets_acquired, close_reason, + * | proposers_validated (at close; absent if the round is + * | recovered or simulated, neither of which reaches + * | closeLedger()) * | * +-- consensus.proposal.send [main thread] * | Created: Adaptor::propose() @@ -36,9 +39,9 @@ * +-- consensus.establish [main thread, child] * | Created: Consensus::startEstablishTracing() * | Ended: Consensus::phaseEstablish() on accept - * | Attrs: disputes_count_initial (at start); converge_percent, - * | establish_count, proposers, disputes_count (overwritten - * | each iteration); avalanche_state (terminal, at end) + * | Attrs: converge_percent, establish_count, proposers, + * | disputes_count (overwritten each iteration); + * | close_time_avalanche_state (terminal, at end) * | * +-- consensus.update_positions [main thread] * | Created: Consensus::updateOurPositions() @@ -191,10 +194,11 @@ inline constexpr auto earlyCloseTriggered = makeStr("early_close_triggered"); /** * Open-phase end metadata (set on consensus.phase.open before reset). * - * A low `tx_sets_acquired` next to a high peer_positions_at_close points at - * missing tx-set fetches rather than at disagreement. `close_reason` plus - * `proposers_validated` separate "the network moved on without us" from "the - * network was quiet". + * A low `tx_sets_acquired` next to a high peer_positions_at_close suggests + * tx-set fetches did not land; the reverse skew is also possible, because + * handleWrongLedger clears currPeerPositions_ but not acquired_. + * `close_reason` plus `proposers_validated` separate "the network moved on + * without us" from "the network was quiet". */ inline constexpr auto openDurationMs = makeStr("open_duration_ms"); inline constexpr auto peerPositionsAtClose = makeStr("peer_positions_at_close"); @@ -210,15 +214,13 @@ inline constexpr auto txCountOpen = makeStr("tx_count_open"); */ inline constexpr auto proposersFinished = makeStr("proposers_finished"); /** - * Establish-phase start/end metadata. + * Establish-phase end metadata. * - * Both are set once, unlike `disputes_count`, which - * updateEstablishTracing() overwrites every iteration. - * `avalanche_state` is the terminal regime; the derived - * `avalanche_threshold` cannot be inverted back to it. + * The terminal close-time regime, set once. Qualified because DisputedTx + * tracks a SECOND, per-transaction avalanche; this is not that one. The + * derived `avalanche_threshold` cannot be inverted back to it. */ -inline constexpr auto disputesCountInitial = makeStr("disputes_count_initial"); -inline constexpr auto avalancheState = makeStr("avalanche_state"); +inline constexpr auto closeTimeAvalancheState = makeStr("close_time_avalanche_state"); /** * Accept/apply enrichment. */ @@ -331,11 +333,13 @@ inline constexpr auto phaseAccepted = makeStr("accepted"); // start_reason values (how startRoundInternal was entered). inline constexpr auto startInitial = makeStr("initial"); inline constexpr auto startRecovered = makeStr("recovered"); -// avalanche_state values, one per ConsensusParms::AvalancheState enumerator. +// close_time_avalanche_state values, one per AvalancheState enumerator. inline constexpr auto avalancheInit = makeStr("init"); inline constexpr auto avalancheMid = makeStr("mid"); inline constexpr auto avalancheLate = makeStr("late"); inline constexpr auto avalancheStuck = makeStr("stuck"); +// Sentinel for an unmapped enumerator, matching to_string(ConsensusPhase). +inline constexpr auto unknown = makeStr("unknown"); // close_reason values, one per LedgerCloseReason enumerator. keep_open is // never emitted: the attribute is only set on the path that closes. inline constexpr auto closeKeepOpen = makeStr("keep_open"); @@ -354,8 +358,8 @@ inline constexpr auto closeNormal = makeStr("normal"); * @param state The state held by Consensus::closeTimeAvalancheState_. * @return The wire label; one of val::avalanche*. * - * @note No default arm, so a new enumerator is a compiler warning rather than - * a silently reused label. + * @note No default arm, so a new enumerator is a -Wswitch warning; the + * fall-through returns "unknown" rather than a plausible-looking regime. */ [[nodiscard]] constexpr std::string_view avalancheStateLabel(ConsensusParms::AvalancheState const state) @@ -371,7 +375,7 @@ avalancheStateLabel(ConsensusParms::AvalancheState const state) case ConsensusParms::AvalancheState::Stuck: return val::avalancheStuck; } - return val::avalancheInit; + return val::unknown; } /** @@ -380,8 +384,8 @@ avalancheStateLabel(ConsensusParms::AvalancheState const state) * @param reason The value returned by whyCloseLedger(). * @return The wire label; one of val::close*. * - * @note No default arm, so a new enumerator is a compiler warning rather than - * a silently reused label. `keep_open` is mapped but never emitted. + * @note No default arm, so a new enumerator is a -Wswitch warning; the + * fall-through returns "unknown". `keep_open` is mapped but never emitted. */ [[nodiscard]] constexpr std::string_view closeReasonLabel(LedgerCloseReason const reason) @@ -399,7 +403,7 @@ closeReasonLabel(LedgerCloseReason const reason) case LedgerCloseReason::Normal: return val::closeNormal; } - return val::closeKeepOpen; + return val::unknown; } } // namespace xrpl::telemetry::consensus::span diff --git a/src/tests/libxrpl/consensus/Consensus.cpp b/src/tests/libxrpl/consensus/Consensus.cpp index c0298b64a7..d0878252bf 100644 --- a/src/tests/libxrpl/consensus/Consensus.cpp +++ b/src/tests/libxrpl/consensus/Consensus.cpp @@ -257,7 +257,7 @@ TEST(ConsensusTest, why_close_ledger_reports_the_deciding_branch) ConsensusParms const p{}; // Bizarre times forcibly close. These vectors ALSO satisfy the - // others-closed condition (8 > 10/2), so they pin the precedence: the + // others-closed condition (10+10 > 10/2), so they pin the precedence: the // anomaly check runs first. EXPECT_EQ(whyCloseLedger(true, 10, 10, 10, -10s, 10s, 1s, 1s, p), LedgerCloseReason::Anomaly); EXPECT_EQ(whyCloseLedger(true, 10, 10, 10, 100h, 10s, 1s, 1s, p), LedgerCloseReason::Anomaly); @@ -272,8 +272,11 @@ TEST(ConsensusTest, why_close_ledger_reports_the_deciding_branch) EXPECT_EQ(whyCloseLedger(false, 10, 0, 0, 1s, 1s, 1s, 10s, p), LedgerCloseReason::KeepOpen); EXPECT_EQ(whyCloseLedger(false, 10, 0, 0, 1s, 10s, 1s, 10s, p), LedgerCloseReason::Idle); - // Transactions present, but under ledgerMinClose (2s). - EXPECT_EQ(whyCloseLedger(true, 10, 0, 0, 10s, 10s, 1s, 10s, p), LedgerCloseReason::KeepOpen); + // Under ledgerMinClose (2s). prevRoundTime is 2s so prevRoundTime/2 is 1s + // and openTime is NOT under it -- this vector isolates the min-close + // branch, which the 10s variant does not (there openTime < 5s trips the + // too-fast branch as well, so deleting min-close entirely still passes). + EXPECT_EQ(whyCloseLedger(true, 10, 0, 0, 2s, 10s, 1s, 10s, p), LedgerCloseReason::KeepOpen); // Past ledgerMinClose but under prevRoundTime/2 (5s), so still too fast. EXPECT_EQ(whyCloseLedger(true, 10, 0, 0, 10s, 10s, 3s, 10s, p), LedgerCloseReason::KeepOpen); @@ -282,6 +285,28 @@ TEST(ConsensusTest, why_close_ledger_reports_the_deciding_branch) EXPECT_EQ(whyCloseLedger(true, 10, 0, 0, 10s, 10s, 10s, 10s, p), LedgerCloseReason::Normal); } +TEST(ConsensusTest, why_close_ledger_others_closed_boundary_is_exclusive) +{ + using namespace std::chrono_literals; + SCOPED_TRACE("others-closed boundary"); + + // The branch is `(closed + validated) > prevProposers / 2`, strict. With + // prevProposers 10 the threshold is 5, so 5 must NOT close and 6 must. + // Flipping > to >= would otherwise go unnoticed. + ConsensusParms const p{}; + + EXPECT_EQ(whyCloseLedger(true, 10, 3, 2, 10s, 10s, 10s, 10s, p), LedgerCloseReason::Normal); + EXPECT_EQ( + whyCloseLedger(true, 10, 3, 3, 10s, 10s, 10s, 10s, p), LedgerCloseReason::OthersClosed); + + // Integer truncation: 11/2 is 5, so 5 still does not close. + EXPECT_EQ(whyCloseLedger(true, 11, 3, 2, 10s, 10s, 10s, 10s, p), LedgerCloseReason::Normal); + + // Others-closed outranks both the no-transactions and the minimum-open + // branches, which would otherwise return KeepOpen for these inputs. + EXPECT_EQ(whyCloseLedger(false, 10, 3, 5, 1s, 1s, 1s, 10s, p), LedgerCloseReason::OthersClosed); +} + TEST(ConsensusTest, why_close_ledger_idle_boundary_is_inclusive) { using namespace std::chrono_literals; diff --git a/src/tests/libxrpl/telemetry/ConsensusSpanNames.cpp b/src/tests/libxrpl/telemetry/ConsensusSpanNames.cpp index d371b67789..d4cd8ebcb5 100644 --- a/src/tests/libxrpl/telemetry/ConsensusSpanNames.cpp +++ b/src/tests/libxrpl/telemetry/ConsensusSpanNames.cpp @@ -78,8 +78,8 @@ TEST(ConsensusSpanNames, close_reason_label_is_usable_at_compile_time) TEST(ConsensusSpanNames, establish_attribute_keys) { - EXPECT_EQ(std::string_view(attr::disputesCountInitial), "disputes_count_initial"); - EXPECT_EQ(std::string_view(attr::avalancheState), "avalanche_state"); + // Qualified: DisputedTx tracks a second, per-transaction avalanche. + EXPECT_EQ(std::string_view(attr::closeTimeAvalancheState), "close_time_avalanche_state"); } TEST(ConsensusSpanNames, start_reason_values_are_the_two_entry_paths) @@ -97,6 +97,7 @@ TEST(ConsensusSpanNames, avalanche_state_values_match_the_parms_enum) EXPECT_EQ(std::string_view(val::avalancheMid), "mid"); EXPECT_EQ(std::string_view(val::avalancheLate), "late"); EXPECT_EQ(std::string_view(val::avalancheStuck), "stuck"); + EXPECT_EQ(std::string_view(val::unknown), "unknown"); } TEST(ConsensusSpanNames, avalanche_state_label_maps_every_enum_state)