From 859aafa3d6fd6279455b5dfeb00a964d9ca88f83 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 10 Sep 2026 15:39:58 +0100 Subject: [PATCH 1/3] fix(insight): hold collector hooks weakly, and cover the lifetime callHooks() copied the hook list into a vector of raw pointers, released mutex_, then dereferenced them. It has to release the lock: a handler may drop the last reference to a hook, and ~OTelHookImpl re-acquires mutex_, so invoking handlers under the non-recursive lock would deadlock. That left a window in which an entry could be freed before it was used. The window is not reachable today. Every hook belongs to a long-lived ApplicationImp member, and onCollectionStopping() runs before those members are destroyed, both from stop() and from the destructor body. That call disarms each gauge via RemoveCallback, which blocks until an in-flight callback finishes, because the SDK holds its registry mutex across the callback. Safety therefore rests on four separate facts, none of them enforced by a test, one of them internal to a vendored library. Store weak references instead, so the code is correct by construction: locking an entry keeps that hook alive for exactly its own handler call, and a hook destroyed since the snapshot locks to null and is skipped. Registration moves from the OTelHookImpl constructor to makeHook(), because no weak_ptr to the object exists until the owning shared_ptr does, and the destructor now prunes by expiry rather than by address. Add three GTests over the real collector. A test-local MetricReader drives one synchronous collection pass, since the SDK ships only a threaded periodic reader. They assert a live hook runs, a destroyed hook is skipped, and destroying one hook leaves its siblings registered -- the last pairing both directions so neither can pass vacuously. Not yet run: verifying them needs a telemetry-enabled build of xrpl_tests. Compile, clang-tidy and the pre-commit gates are clean. --- src/libxrpl/beast/insight/OTelCollector.cpp | 63 ++++-- .../beast/insight/OTelCollectorHooks.cpp | 208 ++++++++++++++++++ 2 files changed, 253 insertions(+), 18 deletions(-) create mode 100644 src/tests/libxrpl/beast/insight/OTelCollectorHooks.cpp diff --git a/src/libxrpl/beast/insight/OTelCollector.cpp b/src/libxrpl/beast/insight/OTelCollector.cpp index b957bd5f79..aba5e59835 100644 --- a/src/libxrpl/beast/insight/OTelCollector.cpp +++ b/src/libxrpl/beast/insight/OTelCollector.cpp @@ -511,17 +511,25 @@ public: /** * @brief Register a hook for periodic invocation. - * @param hook Pointer to the hook to register. + * + * Takes the owning shared_ptr so the list can store a weak reference. + * Called from makeHook() rather than the hook's constructor, because a + * weak_ptr cannot be formed until the shared_ptr owns the object. + * + * @param hook Owning pointer to the hook to register. */ void - addHook(OTelHookImpl* hook); + addHook(std::shared_ptr const& hook); /** - * @brief Unregister a hook. - * @param hook Pointer to the hook to unregister. + * @brief Drop entries for hooks that have been destroyed. + * + * Called from ~OTelHookImpl. The dying hook's weak_ptr has already + * expired by then, so the entry is identified by expiry rather than by + * address. */ void - removeHook(OTelHookImpl* hook); + removeExpiredHooks(); /** * @brief Invoke all registered hooks. @@ -597,8 +605,16 @@ private: /** * Registered hooks called during observable callbacks. + * + * Weak, not owning, and not raw. callHooks() must invoke handlers with + * mutex_ released, because a handler may drop the last reference to a + * hook and ~OTelHookImpl re-acquires mutex_. A raw pointer copied out of + * this list could therefore be dangling by the time it is dereferenced. + * Locking a weak_ptr instead keeps the hook alive for exactly the + * duration of its own handler call, and an already-destroyed hook is + * skipped rather than followed. */ - std::vector hooks_; + std::vector> hooks_; /** * Registered gauges read during observable callbacks. @@ -634,12 +650,14 @@ private: OTelHookImpl::OTelHookImpl(HandlerType handler, std::shared_ptr impl) : impl_(std::move(impl)), handler_(std::move(handler)) { - impl_->addHook(this); + // Registration happens in OTelCollectorImp::makeHook(), not here: the + // list holds weak references, and no weak_ptr to this object exists + // until the owning shared_ptr does. } OTelHookImpl::~OTelHookImpl() { - impl_->removeHook(this); + impl_->removeExpiredHooks(); } void @@ -849,7 +867,9 @@ OTelCollectorImp::~OTelCollectorImp() Hook OTelCollectorImp::makeHook(HookImpl::HandlerType const& handler) { - return Hook(std::make_shared(handler, shared_from_this())); + auto hook = std::make_shared(handler, shared_from_this()); + addHook(hook); + return Hook(hook); } Counter @@ -883,17 +903,17 @@ OTelCollectorImp::makeMeter(std::string const& name) } void -OTelCollectorImp::addHook(OTelHookImpl* hook) +OTelCollectorImp::addHook(std::shared_ptr const& hook) { std::scoped_lock const lock(mutex_); - hooks_.push_back(hook); + hooks_.emplace_back(hook); } void -OTelCollectorImp::removeHook(OTelHookImpl* hook) +OTelCollectorImp::removeExpiredHooks() { std::scoped_lock const lock(mutex_); - std::erase(hooks_, hook); + std::erase_if(hooks_, [](std::weak_ptr const& hook) { return hook.expired(); }); } void @@ -910,15 +930,22 @@ OTelCollectorImp::callHooks() // Copy the hook list under the lock, then invoke handlers outside it. // A handler may drop the last reference to an OTelHookImpl, whose - // destructor calls removeHook() and re-acquires mutex_; invoking - // handlers while holding the (non-recursive) lock would deadlock. - std::vector hooks; + // destructor re-acquires mutex_; invoking handlers while holding the + // (non-recursive) lock would deadlock. + std::vector> hooks; { std::scoped_lock const lock(mutex_); hooks = hooks_; } - for (auto* hook : hooks) - hook->callHandler(); + + // Locking each entry keeps that hook alive across its own handler call, + // so releasing mutex_ above cannot leave a dangling reference. A hook + // destroyed since the snapshot was taken locks to null and is skipped. + for (auto const& weakHook : hooks) + { + if (auto const hook = weakHook.lock()) + hook->callHandler(); + } } void diff --git a/src/tests/libxrpl/beast/insight/OTelCollectorHooks.cpp b/src/tests/libxrpl/beast/insight/OTelCollectorHooks.cpp new file mode 100644 index 0000000000..18f35e2f8a --- /dev/null +++ b/src/tests/libxrpl/beast/insight/OTelCollectorHooks.cpp @@ -0,0 +1,208 @@ +#ifdef XRPL_ENABLE_TELEMETRY + +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include + +namespace beast::insight { + +namespace metrics_api = opentelemetry::metrics; +namespace metrics_sdk = opentelemetry::sdk::metrics; + +/** + * A MetricReader that collects only when the test asks it to. + * + * The SDK ships only PeriodicExportingMetricReader, whose background thread + * would make these tests depend on timing. MetricReader::Collect() is public + * and synchronous, so a minimal subclass lets a test drive one collection pass + * on the calling thread. That pass is what invokes an observable gauge's + * callback, which is the only path that reaches the collector's hooks. + * + * @code + * auto reader = std::make_shared(); + * provider->AddMetricReader(reader); + * reader->collectOnce(); // runs every registered observable callback + * @endcode + */ +class ManualMetricReader : public metrics_sdk::MetricReader +{ +public: + /** + * @brief Run exactly one collection pass, discarding the metric data. + * + * The tests assert on hook side effects, not on exported points, so the + * callback returns true without inspecting what it was handed. + */ + void + collectOnce() + { + Collect([](metrics_sdk::ResourceMetrics&) { return true; }); + } + + metrics_sdk::AggregationTemporality + GetAggregationTemporality(metrics_sdk::InstrumentType) const noexcept override + { + return metrics_sdk::AggregationTemporality::kCumulative; + } + + bool + OnForceFlush(std::chrono::microseconds) noexcept override + { + return true; + } + + bool + OnShutDown(std::chrono::microseconds) noexcept override + { + return true; + } +}; + +/** + * Installs a real SDK MeterProvider so observable gauges actually fire. + * + * OTelCollector takes its Meter from the global provider. Under the default + * noop provider an observable gauge's callback is never invoked, so a hook + * test would pass whatever the collector did. The fixture swaps in an SDK + * provider with a ManualMetricReader and restores the previous global provider + * afterwards, so it leaks no state into other telemetry tests in this binary. + */ +class OTelCollectorHooks : public ::testing::Test +{ +protected: + void + SetUp() override + { + previous_ = metrics_api::Provider::GetMeterProvider(); + reader_ = std::make_shared(); + auto provider = metrics_sdk::MeterProviderFactory::Create(); + provider->AddMetricReader(reader_); + provider_ = std::shared_ptr(std::move(provider)); + metrics_api::Provider::SetMeterProvider( + opentelemetry::nostd::shared_ptr(provider_)); + } + + void + TearDown() override + { + metrics_api::Provider::SetMeterProvider(previous_); + provider_.reset(); + reader_.reset(); + } + + /** + * @brief Build a collector, plus the armed gauge that drives its hooks. + * + * A collection pass only reaches the hooks through an observable gauge's + * callback, and a gauge is armed by onCollectionReady(), so every test + * needs both. The gauge is returned because dropping it would unregister + * the callback. + * + * Each test builds its own collector: the hook debounce is keyed to the + * time of the last invocation, which starts unset, so the first collection + * on a fresh collector always runs the hooks. + */ + static std::pair + makeArmedCollector() + { + auto collector = OTelCollector::New( + "http://127.0.0.1:4318/v1/metrics", + "", + "test-instance", + "xrpld", + "test", + Journal(Journal::getNullSink())); + auto gauge = collector->makeGauge("hook_test_gauge"); + collector->onCollectionReady(); + return {std::move(collector), std::move(gauge)}; + } + + opentelemetry::nostd::shared_ptr previous_; + std::shared_ptr reader_; + std::shared_ptr provider_; +}; + +// --------------------------------------------------------------------------- +// 1. A hook that is still alive runs on a collection pass. +// This is the registration path: makeHook() puts the hook on the +// collector's list, and an observable gauge callback invokes it. Without +// this, a hook that is never registered is indistinguishable from one that +// is registered and skipped. +// --------------------------------------------------------------------------- +TEST_F(OTelCollectorHooks, live_hook_runs_once_per_collection) +{ + auto [collector, gauge] = makeArmedCollector(); + + std::size_t calls = 0; + auto const hook = collector->makeHook([&calls] { ++calls; }); + + reader_->collectOnce(); + + EXPECT_EQ(calls, 1u); +} + +// --------------------------------------------------------------------------- +// 2. A hook destroyed before the collection pass is skipped, not called. +// The collector holds weak references, so the destroyed hook's entry locks +// to null. Asserting zero (not "did not crash") is what makes this a real +// check: a stale entry that was still followed would run the handler and +// increment the counter through freed memory. +// --------------------------------------------------------------------------- +TEST_F(OTelCollectorHooks, destroyed_hook_is_skipped) +{ + auto [collector, gauge] = makeArmedCollector(); + + std::size_t calls = 0; + { + auto const hook = collector->makeHook([&calls] { ++calls; }); + } + + reader_->collectOnce(); + + EXPECT_EQ(calls, 0u); +} + +// --------------------------------------------------------------------------- +// 3. Destroying one hook leaves its siblings registered. +// Guards the pruning step: removeExpiredHooks() erases by expiry rather +// than by address, so an over-broad predicate would drop live hooks too and +// silently stop their metrics updating. +// --------------------------------------------------------------------------- +TEST_F(OTelCollectorHooks, destroying_one_hook_keeps_the_others) +{ + auto [collector, gauge] = makeArmedCollector(); + + std::size_t kept = 0; + std::size_t dropped = 0; + auto const keptHook = collector->makeHook([&kept] { ++kept; }); + { + auto const droppedHook = collector->makeHook([&dropped] { ++dropped; }); + } + + reader_->collectOnce(); + + EXPECT_EQ(kept, 1u); + EXPECT_EQ(dropped, 0u); +} + +} // namespace beast::insight + +#endif // XRPL_ENABLE_TELEMETRY From e1ef6ba18372230d00ccd9ce68c5814ef3621cd7 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Fri, 11 Sep 2026 11:30:17 +0100 Subject: [PATCH 2/3] docs(telemetry): drop the inert insight endpoint from the test config template On the OTel path only [insight] server is load-bearing. CollectorManager reads endpoint and hands it to OTelCollector, which logs it at startup and routes nothing with it; the real export endpoint is [telemetry] metrics_endpoint, which the template already sets. service_instance_id and service_name in that section are read and discarded. Leaving the line invited an operator to reconcile a mismatch that has no effect. integration-test.sh already emits only server=otel with the same explanation, so the two now agree. --- docker/telemetry/TESTING.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docker/telemetry/TESTING.md b/docker/telemetry/TESTING.md index a18baaadc7..d18114d80c 100644 --- a/docker/telemetry/TESTING.md +++ b/docker/telemetry/TESTING.md @@ -266,8 +266,10 @@ trace_peer=1 trace_ledger=1 [insight] +# server=otel is the only load-bearing key here -- it selects OTelCollector. +# The export endpoint comes from [telemetry] metrics_endpoint, and [insight]'s +# own service_instance_id/service_name keys are ignored. server=otel -endpoint=http://localhost:4318/v1/metrics [rpc_startup] { "command": "log_level", "severity": "warning" } From bec9e1c8a9ab4a8a4a74a603dfb0684fe40f9959 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Mon, 14 Sep 2026 20:34:31 +0100 Subject: [PATCH 3/3] fix(telemetry): resolve the node identity before the Application is built resolveNodePublicKey() returned std::nullopt in three real cases: a first boot with no wallet database, a standalone run (its wallet is a private temporary database), and --newnodeid. Telemetry's resources are built during ApplicationImp's member-init list and are immutable, so on those runs the node reported an empty service.instance.id and no xrpl.node.id for the whole run, while setup() minted a key moments later and patched only the tracer. Replace it with resolveNodeIdentity(), which always returns a keypair: derived from a configured seed, else read from an existing wallet database, else minted. Main.cpp passes that pair to makeApplication(), ApplicationImp stores it in nodeIdentity_ -- now declared before telemetry_ and no longer an optional, because it is always set -- and builds the telemetry resource from it. setup() calls getNodeIdentity(), which now persists rather than mints: it stores the resolved pair when the wallet holds no identity, adopts the stored one when it does, and clears first for --newnodeid. The write stays in setup() because that is where the database exists; a standalone run has no persistent wallet to write to, which is why the pair has to be decided before construction rather than read back afterwards. Wallet gains storeNodeIdentity() for that write, and getNodeIdentity(session) now uses it instead of repeating the insert. The three-argument makeApplication() mints a keypair, so jtx::Env and any other test Application behave as a standalone run always did. Also fold the three hand-rolled "meter from a NoopMeterProvider" copies into telemetry::noopMeter(): the base-pointer call and the kMeterVersion argument are both easy to get wrong alone, and the meter identity has to match the one the histogram views select on. The new gtest covers the wallet half: store-then-read, store not replacing an existing identity, clear-then-store, and that the mint path persists. It adds the tests.libxrpl > xrpl.rdb levelization edge, regenerated here. --- .../scripts/levelization/results/ordering.txt | 1 + .../05-configuration-reference.md | 16 +- include/xrpl/server/Wallet.h | 16 ++ include/xrpl/telemetry/Telemetry.h | 19 ++ src/libxrpl/server/Wallet.cpp | 22 ++- src/libxrpl/telemetry/Telemetry.cpp | 17 +- src/tests/libxrpl/server/NodeIdentity.cpp | 158 ++++++++++++++++ .../libxrpl/telemetry/SpanGuardScope.cpp | 5 +- src/xrpld/app/main/Application.cpp | 43 +++-- src/xrpld/app/main/Application.h | 13 +- src/xrpld/app/main/Main.cpp | 23 +-- src/xrpld/app/main/NodeIdentity.cpp | 172 +++++++++++------- src/xrpld/app/main/NodeIdentity.h | 51 ++++-- 13 files changed, 412 insertions(+), 144 deletions(-) create mode 100644 src/tests/libxrpl/server/NodeIdentity.cpp diff --git a/.github/scripts/levelization/results/ordering.txt b/.github/scripts/levelization/results/ordering.txt index 51db8661c8..49eb71d8e0 100644 --- a/.github/scripts/levelization/results/ordering.txt +++ b/.github/scripts/levelization/results/ordering.txt @@ -196,6 +196,7 @@ tests.libxrpl > xrpl.nodestore tests.libxrpl > xrpl.peerfinder tests.libxrpl > xrpl.protocol tests.libxrpl > xrpl.protocol_autogen +tests.libxrpl > xrpl.rdb tests.libxrpl > xrpl.resource tests.libxrpl > xrpl.server tests.libxrpl > xrpl.shamap diff --git a/OpenTelemetryPlan/05-configuration-reference.md b/OpenTelemetryPlan/05-configuration-reference.md index 057ab34a3e..ddd05cfdf0 100644 --- a/OpenTelemetryPlan/05-configuration-reference.md +++ b/OpenTelemetryPlan/05-configuration-reference.md @@ -62,13 +62,17 @@ The parser `makeTelemetrySetup()` in `src/libxrpl/telemetry/TelemetryConfig.cpp` ### 5.3.1 ApplicationImp Changes -> **Deferred identity**: The node public key (`nodeIdentity_`) is not -> available during `ApplicationImp`'s member initializer list — it is -> resolved later in `setup()`. The `Telemetry` object is therefore -> constructed with an empty `serviceInstanceId` and patched via -> `setServiceInstanceId()` once `setup()` has called `getNodeIdentity()`. +> **Identity before construction**: telemetry stamps the node public key into +> resources that are immutable once built, and it builds them during +> `ApplicationImp`'s member initializer list. So `Main.cpp` calls +> `resolveNodeIdentity()` first, from the config and command line alone, and +> passes the keypair to `makeApplication()`. It never comes back empty: a +> configured `[node_seed]` decides it, else the wallet database supplies it if +> one already exists, else it is minted. `ApplicationImp::setup()` then calls +> `getNodeIdentity()`, which stores that keypair when the wallet holds none and +> otherwise adopts what the wallet holds. -`ApplicationImp` (in `src/xrpld/app/main/Application.cpp`) owns a `std::unique_ptr telemetry_`. It is built in the member initializer list via `makeTelemetry(makeTelemetrySetup(...))` with an empty `serviceInstanceId`, then patched in `setup()` by calling `setServiceInstanceId()` with the Base58 node public key (unless the user supplied a custom `service_instance_id`). `start()` and `run()` forward to `telemetry_->start()` / `telemetry_->stop()`, and `getTelemetry()` returns the owned instance. +`ApplicationImp` (in `src/xrpld/app/main/Application.cpp`) owns a `std::pair nodeIdentity_`, declared before `std::unique_ptr telemetry_` so the resource can be built from it. `telemetry_` is built in the member initializer list via `makeTelemetry(makeTelemetrySetup(...))` with that key as `serviceInstanceId` (unless the user supplied a custom `service_instance_id`). `setup()` still calls `setServiceInstanceId()`, which now matters only where the stored key differs from the resolved one, and reaches the tracer resource alone. `start()` and `run()` forward to `telemetry_->start()` / `telemetry_->stop()`, and `getTelemetry()` returns the owned instance. ### 5.3.2 ServiceRegistry Interface Addition diff --git a/include/xrpl/server/Wallet.h b/include/xrpl/server/Wallet.h index af6c92b83d..e549a1305e 100644 --- a/include/xrpl/server/Wallet.h +++ b/include/xrpl/server/Wallet.h @@ -102,6 +102,22 @@ clearNodeIdentity(soci::session& session); std::optional> readNodeIdentity(soci::session& session); +/** + * Persist a keypair as this node's identity. + * + * Write-only counterpart of readNodeIdentity(). The caller must have found the + * table empty: this inserts a row without clearing, so storing twice leaves two + * and readNodeIdentity() then returns whichever the query yields first. + * + * Exists because xrpld resolves its identity before the Application, and so + * before any database, is built; setup() persists that keypair here. + * + * @param session Session with the database. + * @param keys The keypair to store. + */ +void +storeNodeIdentity(soci::session& session, std::pair const& keys); + /** * Returns a stable public and private key for this node. * diff --git a/include/xrpl/telemetry/Telemetry.h b/include/xrpl/telemetry/Telemetry.h index a0f56387b0..335b2ae7b8 100644 --- a/include/xrpl/telemetry/Telemetry.h +++ b/include/xrpl/telemetry/Telemetry.h @@ -133,6 +133,25 @@ inline constexpr std::string_view kMeterName{"xrpld"}; * OTel instrumentation scope version reported for the meter. */ inline constexpr std::string_view kMeterVersion{"1.0.0"}; + +/** + * A meter whose instruments record nothing. + * + * For every path that must hand out a usable meter without a pipeline behind + * it: telemetry disabled, or an exporter that failed to build. Callers then + * need no null check, because an instrument always comes back. + * + * Two details are easy to get wrong alone, which is why this is shared: the + * provider must be reached through a base `MeterProvider` pointer, because + * `NoopMeterProvider`'s override hides the base class's defaulted overload; + * and the version must be @ref kMeterVersion, or the meter identity differs + * from the one the histogram views select on. + * + * @param name Instrumentation scope name to report. + * @return An inert meter. Never empty. + */ +[[nodiscard]] opentelemetry::nostd::shared_ptr +noopMeter(std::string_view name = kMeterName); #endif /** diff --git a/src/libxrpl/server/Wallet.cpp b/src/libxrpl/server/Wallet.cpp index 92317d40f6..ec9f1fd3b5 100644 --- a/src/libxrpl/server/Wallet.cpp +++ b/src/libxrpl/server/Wallet.cpp @@ -171,6 +171,16 @@ readNodeIdentity(soci::session& session) return std::nullopt; } +void +storeNodeIdentity(soci::session& session, std::pair const& keys) +{ + session << std::format( + "INSERT INTO NodeIdentity (PublicKey,PrivateKey) " + "VALUES ('{}','{}');", + toBase58(TokenType::NodePublic, keys.first), + toBase58(TokenType::NodePrivate, keys.second)); +} + std::pair getNodeIdentity(soci::session& session) { @@ -178,15 +188,9 @@ getNodeIdentity(soci::session& session) return *stored; // If a valid identity wasn't found, we randomly generate a new one: - auto [newpublicKey, newsecretKey] = randomKeyPair(KeyType::Secp256k1); - - session << std::format( - "INSERT INTO NodeIdentity (PublicKey,PrivateKey) " - "VALUES ('{}','{}');", - toBase58(TokenType::NodePublic, newpublicKey), - toBase58(TokenType::NodePrivate, newsecretKey)); - - return {newpublicKey, newsecretKey}; + auto const keys = randomKeyPair(KeyType::Secp256k1); + storeNodeIdentity(session, keys); + return keys; } std::unordered_set, KeyEqual> diff --git a/src/libxrpl/telemetry/Telemetry.cpp b/src/libxrpl/telemetry/Telemetry.cpp index ad9bc1479e..6ff5993f93 100644 --- a/src/libxrpl/telemetry/Telemetry.cpp +++ b/src/libxrpl/telemetry/Telemetry.cpp @@ -261,11 +261,8 @@ public: [[nodiscard]] opentelemetry::nostd::shared_ptr getMeter(std::string_view name) override { - // Serve a meter from a process-wide noop provider, mirroring the - // noop tracer above. Instruments created from it are inert. - static auto noopProvider = opentelemetry::nostd::shared_ptr( - new metrics_api::NoopMeterProvider()); - return noopProvider->GetMeter(std::string(name), std::string(kMeterVersion)); + // Mirrors the noop tracer above: instruments created from it are inert. + return noopMeter(name); } [[nodiscard]] opentelemetry::nostd::shared_ptr @@ -703,6 +700,16 @@ public: } // namespace +opentelemetry::nostd::shared_ptr +noopMeter(std::string_view name) +{ + // One provider for the process: it holds a single inert meter, so nothing + // is gained by building another. + static auto const provider = opentelemetry::nostd::shared_ptr( + new metrics_api::NoopMeterProvider()); + return provider->GetMeter(std::string(name), std::string(kMeterVersion)); +} + opentelemetry::exporter::otlp::OtlpHttpExporterOptions makeTraceExporterOptions(Telemetry::Setup const& setup) { diff --git a/src/tests/libxrpl/server/NodeIdentity.cpp b/src/tests/libxrpl/server/NodeIdentity.cpp new file mode 100644 index 0000000000..22dc9d2948 --- /dev/null +++ b/src/tests/libxrpl/server/NodeIdentity.cpp @@ -0,0 +1,158 @@ +/** + * @file NodeIdentity.cpp + * GTest unit tests for the wallet database's node-identity storage. + * + * Three functions share one table, `NodeIdentity`, and the split between them + * is what the telemetry startup order depends on: `readNodeIdentity()` only + * reads, `storeNodeIdentity()` only writes, and `getNodeIdentity()` reads then + * writes a fresh key when the table is empty. `xrpld` resolves its identity + * before the Application exists and persists it later, so the store step has + * to be callable on its own and has to be idempotent-by-read: a second run + * must return the first run's key, not a new one. + * + * Each test gets its own database file in a temporary directory, so nothing + * here depends on order or on the developer's data directory. + */ + +#include +#include +#include +#include +#include +#include +#include + +#include + +#include +#include +#include +#include + +using namespace xrpl; + +namespace { + +/** + * A wallet database in its own temporary directory, removed on destruction. + * + * `makeTestWalletDB()` creates the schema, so every fixture starts with an + * empty `NodeIdentity` table. + */ +class TempWalletDb +{ +public: + explicit TempWalletDb(std::string const& name) + : dir_(std::filesystem::temp_directory_path() / ("xrpl-node-identity-" + name)) + { + std::filesystem::remove_all(dir_); + std::filesystem::create_directories(dir_); + + DatabaseCon::Setup setup; + setup.dataDir = dir_; + db_ = makeTestWalletDB(setup, "wallet.db", beast::Journal{beast::Journal::getNullSink()}); + } + + ~TempWalletDb() + { + db_.reset(); + std::error_code ec; + std::filesystem::remove_all(dir_, ec); + } + + TempWalletDb(TempWalletDb const&) = delete; + TempWalletDb& + operator=(TempWalletDb const&) = delete; + + [[nodiscard]] DatabaseCon& + operator*() const noexcept + { + return *db_; + } + +private: + std::filesystem::path dir_; + std::unique_ptr db_; +}; + +} // namespace + +TEST(WalletNodeIdentity, store_then_read_returns_the_same_pair) +{ + // The store step exists so a key minted before the Application is built + // can be persisted afterwards. Reading it back must give the same pair, or + // the two halves of one run report two identities. + TempWalletDb wallet("store-then-read"); + auto const minted = randomKeyPair(KeyType::Secp256k1); + + { + auto db = (*wallet).checkoutDb(); + ASSERT_FALSE(readNodeIdentity(*db).has_value()) << "a fresh wallet must hold no identity"; + storeNodeIdentity(*db, minted); + } + + auto db = (*wallet).checkoutDb(); + auto const stored = readNodeIdentity(*db); + ASSERT_TRUE(stored.has_value()); + EXPECT_EQ(stored->first, minted.first); + EXPECT_EQ(stored->second, minted.second); +} + +TEST(WalletNodeIdentity, store_does_not_replace_an_existing_identity) +{ + // getNodeIdentity() is the read-or-mint path and must keep the first key, + // so a restart does not change the node's identity on the network. The + // stored pair wins over anything a later caller offers. + TempWalletDb wallet("no-replace"); + auto db = (*wallet).checkoutDb(); + + auto const first = getNodeIdentity(*db); + auto const other = randomKeyPair(KeyType::Secp256k1); + ASSERT_NE(first.first, other.first) + << "the two pairs must differ for this test to mean anything"; + + storeNodeIdentity(*db, other); + + auto const stored = readNodeIdentity(*db); + ASSERT_TRUE(stored.has_value()); + EXPECT_EQ(stored->first, first.first); + EXPECT_EQ(getNodeIdentity(*db).first, first.first); +} + +TEST(WalletNodeIdentity, clear_then_store_installs_the_new_pair) +{ + // --newnodeid clears the row and then persists the freshly minted pair. + // Both steps are needed: clearing alone would leave the node with no + // stored identity at all. + TempWalletDb wallet("clear-then-store"); + auto db = (*wallet).checkoutDb(); + + auto const first = getNodeIdentity(*db); + auto const replacement = randomKeyPair(KeyType::Secp256k1); + ASSERT_NE(first.first, replacement.first); + + clearNodeIdentity(*db); + EXPECT_FALSE(readNodeIdentity(*db).has_value()) << "clear must leave the table empty"; + + storeNodeIdentity(*db, replacement); + auto const stored = readNodeIdentity(*db); + ASSERT_TRUE(stored.has_value()); + EXPECT_EQ(stored->first, replacement.first); + EXPECT_EQ(stored->second, replacement.second); +} + +TEST(WalletNodeIdentity, get_mints_and_persists_when_the_table_is_empty) +{ + // The mint path must persist, not just return: a second call has to give + // the same key. This is the property --newnodeid relies on to be + // meaningful, and the one a caller that only reads would break. + TempWalletDb wallet("mint-and-persist"); + auto db = (*wallet).checkoutDb(); + + auto const minted = getNodeIdentity(*db); + + auto const stored = readNodeIdentity(*db); + ASSERT_TRUE(stored.has_value()) << "getNodeIdentity() must persist what it mints"; + EXPECT_EQ(stored->first, minted.first); + EXPECT_EQ(getNodeIdentity(*db).first, minted.first); +} diff --git a/src/tests/libxrpl/telemetry/SpanGuardScope.cpp b/src/tests/libxrpl/telemetry/SpanGuardScope.cpp index 78a5a04983..b46bbcc90f 100644 --- a/src/tests/libxrpl/telemetry/SpanGuardScope.cpp +++ b/src/tests/libxrpl/telemetry/SpanGuardScope.cpp @@ -193,10 +193,7 @@ public: opentelemetry::nostd::shared_ptr getMeter(std::string_view name) override { - static auto noopProvider = - opentelemetry::nostd::shared_ptr( - new opentelemetry::metrics::NoopMeterProvider()); - return noopProvider->GetMeter(std::string(name), std::string(kMeterVersion)); + return noopMeter(name); } opentelemetry::nostd::shared_ptr diff --git a/src/xrpld/app/main/Application.cpp b/src/xrpld/app/main/Application.cpp index 412a4b4cd5..13afb18c1d 100644 --- a/src/xrpld/app/main/Application.cpp +++ b/src/xrpld/app/main/Application.cpp @@ -80,9 +80,11 @@ #include #include #include // IWYU pragma: keep +#include #include #include #include +#include #include #include // IWYU pragma: keep #include @@ -220,6 +222,13 @@ public: beast::Journal journal_; std::unique_ptr perfLog_; + /** + * This node's keypair, resolved before construction by + * resolveNodeIdentity() and persisted by setup(). Declared before + * telemetry_ because that builds resource attributes from it, and they are + * immutable once built. + */ + std::pair nodeIdentity_; std::unique_ptr telemetry_; Application::MutexType masterMutex_; @@ -236,7 +245,6 @@ public: NodeCache tempNodeCache_; CachedSLEs cachedSLEs_; std::unique_ptr networkIDService_; - std::optional> nodeIdentity_; ValidatorKeys const validatorKeys_; std::unique_ptr resourceManager_; @@ -317,7 +325,7 @@ public: std::unique_ptr config, std::unique_ptr logs, std::unique_ptr timeKeeper, - std::optional const& nodePublicKey) + std::pair const& resolvedIdentity) : BasicApp(numberOfThreads(*config)) , config_(std::move(config)) , logs_(std::move(logs)) @@ -331,15 +339,16 @@ public: *this, logs_->journal("PerfLog"), [this] { signalStop("PerfLog"); })) + , nodeIdentity_(resolvedIdentity) // Telemetry publishes the MeterProvider on construction, so it must // precede collectorManager_ below and every subsystem that creates an // instrument. Its resource is immutable, so the instance id has to be - // supplied now; empty means this run reports none. + // supplied now, from the identity resolved above. , telemetry_( telemetry::makeTelemetry( telemetry::makeTelemetrySetup( config_->section("telemetry"), - nodePublicKey.value_or(""), + toBase58(TokenType::NodePublic, nodeIdentity_.first), build_info::getVersionString(), config_->networkId), logs_->journal("Telemetry"))) @@ -619,10 +628,7 @@ public: std::pair const& nodeIdentity() override { - if (nodeIdentity_) - return *nodeIdentity_; - - logicError("Accessing Application::nodeIdentity() before it is initialized."); + return nodeIdentity_; } std::optional @@ -1306,12 +1312,15 @@ ApplicationImp::setup(boost::program_options::variables_map const& cmdline) return false; } - nodeIdentity_ = getNodeIdentity(*this, cmdline); + // Persist the identity resolved before construction, or adopt the one the + // wallet already holds. Telemetry is already reporting the resolved key. + nodeIdentity_ = getNodeIdentity(*this, cmdline, nodeIdentity_); // The metrics resource was fixed at construction, but the tracer resource is - // built by start() below, so a key minted just now can still reach spans. + // built by start() below, so the stored key still reaches spans if it + // differs from the resolved one. if (!config_->section("telemetry").exists("service_instance_id")) - telemetry_->setServiceInstanceId(toBase58(TokenType::NodePublic, nodeIdentity_->first)); + telemetry_->setServiceInstanceId(toBase58(TokenType::NodePublic, nodeIdentity_.first)); // Start telemetry here, not in start(). Spans are emitted during the rest // of setup() — the first consensus round in beginConsensus() below — and @@ -2298,7 +2307,13 @@ makeApplication( std::unique_ptr logs, std::unique_ptr timeKeeper) { - return makeApplication(std::move(config), std::move(logs), std::move(timeKeeper), std::nullopt); + // No identity supplied, so mint one. setup() stores it if the wallet holds + // none, which is what a standalone run and a test Application do anyway. + return makeApplication( + std::move(config), + std::move(logs), + std::move(timeKeeper), + randomKeyPair(KeyType::Secp256k1)); } std::unique_ptr @@ -2306,10 +2321,10 @@ makeApplication( std::unique_ptr config, std::unique_ptr logs, std::unique_ptr timeKeeper, - std::optional const& nodePublicKey) + std::pair const& nodeIdentity) { return std::make_unique( - std::move(config), std::move(logs), std::move(timeKeeper), nodePublicKey); + std::move(config), std::move(logs), std::move(timeKeeper), nodeIdentity); } void diff --git a/src/xrpld/app/main/Application.h b/src/xrpld/app/main/Application.h index 1d7125cd64..8791a6ec8f 100644 --- a/src/xrpld/app/main/Application.h +++ b/src/xrpld/app/main/Application.h @@ -175,18 +175,21 @@ makeApplication( std::unique_ptr timeKeeper); /** - * Construct the application with a known node public key. + * Construct the application with a known node identity. * * Telemetry builds its resource attributes during construction and they are - * immutable, so the base58 node public key must be supplied here. Pass - * std::nullopt when it is unknown; that run reports no instance id. See - * resolveNodePublicKey(). + * immutable, so the node keypair must be supplied here. See + * resolveNodeIdentity(), which decides it from the config and command line + * alone; setup() then persists it. + * + * The three-argument overload above mints a keypair, which is what a test + * Application and a standalone run get anyway. */ std::unique_ptr makeApplication( std::unique_ptr config, std::unique_ptr logs, std::unique_ptr timeKeeper, - std::optional const& nodePublicKey); + std::pair const& resolvedIdentity); } // namespace xrpl diff --git a/src/xrpld/app/main/Main.cpp b/src/xrpld/app/main/Main.cpp index fcae528737..d72fbf5c77 100644 --- a/src/xrpld/app/main/Main.cpp +++ b/src/xrpld/app/main/Main.cpp @@ -807,13 +807,14 @@ run(int argc, char** argv) if (vm.contains("debug")) setDebugLogSink(logs->makeSink("Debug", beast::Severity::Trace)); - // Telemetry needs the node public key at construction, so read it here - // where a config error can still be reported and the process can exit - // cleanly. getNodeIdentity() in setup() stays authoritative. - std::optional nodePublicKey; + // Telemetry stamps the node public key into resources it builds during + // construction, so the identity is decided here, where a malformed + // [node_seed] can still be reported and the process can exit cleanly. + // setup() persists it; see getNodeIdentity(). + std::optional> nodeIdentity; try { - nodePublicKey = resolveNodePublicKey(*config, vm, logs->journal("Application")); + nodeIdentity = resolveNodeIdentity(*config, vm, logs->journal("Application")); } catch (std::exception const& e) { @@ -821,14 +822,6 @@ run(int argc, char** argv) return -1; } - if (!nodePublicKey) - { - JLOG(logs->journal("Application").warn()) - << "Telemetry: no node identity available yet, so this run reports an empty " - "service.instance.id. Set [telemetry] service_instance_id, or restart once " - "the node key exists."; - } - // Application construction runs member initializers that validate // config (for example the [telemetry] section) and can throw. A throw // from a member-initializer list cannot be recovered inside the @@ -840,14 +833,14 @@ run(int argc, char** argv) // // Only the construction is covered. The [telemetry] section is parsed // near the top of the member list, before the job queue and node store - // are built, so unwinding that throw destroys very little. setup() is + // are built, so unwinding that throw destroys little. setup() is // left outside deliberately: it starts subsystems whose shutdown order // is delicate, and only the normal stop sequence gets that order right. std::unique_ptr app; try { app = makeApplication( - std::move(config), std::move(logs), std::make_unique(), nodePublicKey); + std::move(config), std::move(logs), std::make_unique(), *nodeIdentity); } catch (std::exception const& e) { diff --git a/src/xrpld/app/main/NodeIdentity.cpp b/src/xrpld/app/main/NodeIdentity.cpp index 3860bbe3c6..bb62c99cf4 100644 --- a/src/xrpld/app/main/NodeIdentity.cpp +++ b/src/xrpld/app/main/NodeIdentity.cpp @@ -30,104 +30,88 @@ namespace xrpl { -std::pair -getNodeIdentity(Application& app, boost::program_options::variables_map const& cmdline) -{ - std::optional seed; +namespace { +/** + * The seed a configured `[node_seed]` or `--nodeid` names. + * + * @param config The server configuration. + * @param cmdline The command line parameters passed into the application. + * @return The seed, or std::nullopt when neither is configured. + * @throws std::runtime_error if the configured value is malformed. + */ +std::optional +configuredSeed(Config const& config, boost::program_options::variables_map const& cmdline) +{ if (cmdline.contains("nodeid")) { - seed = parseGenericSeed(cmdline["nodeid"].as(), false); - + auto seed = parseGenericSeed(cmdline["nodeid"].as(), false); if (!seed) Throw("Invalid 'nodeid' in command line"); + return seed; } - else if (app.config().exists(Sections::kNodeSeed)) - { - seed = parseBase58(app.config().section(Sections::kNodeSeed).lines().front()); + if (config.exists(Sections::kNodeSeed)) + { + auto const& lines = config.section(Sections::kNodeSeed).lines(); + auto seed = lines.empty() ? std::nullopt : parseBase58(lines.front()); if (!seed) { Throw( std::string("Invalid [") + Sections::kNodeSeed + "] in configuration file"); } + return seed; } - if (seed) - { - auto secretKey = generateSecretKey(KeyType::Secp256k1, *seed); - auto publicKey = derivePublicKey(KeyType::Secp256k1, secretKey); - - return {publicKey, secretKey}; - } - - auto db = app.getWalletDB().checkoutDb(); - - if (cmdline.contains("newnodeid")) - clearNodeIdentity(*db); - - return getNodeIdentity(*db); + return std::nullopt; } -std::optional -resolveNodePublicKey( - Config const& config, - boost::program_options::variables_map const& cmdline, - beast::Journal journal) +/** + * The keypair a seed defines. + * + * @param seed The configured seed. + * @return The derived secp256k1 keypair. + */ +std::pair +keysFromSeed(Seed const& seed) { - std::optional seed; - bool seedConfigured = false; - - if (cmdline.contains("nodeid")) - { - seedConfigured = true; - seed = parseGenericSeed(cmdline["nodeid"].as(), false); - } - else if (config.exists(Sections::kNodeSeed)) - { - seedConfigured = true; - if (auto const& lines = config.section(Sections::kNodeSeed).lines(); !lines.empty()) - seed = parseBase58(lines.front()); - } - - // A configured seed decides the identity outright. A malformed or missing - // one is reported by getNodeIdentity(), which runs later. - if (seedConfigured) - { - if (!seed) - return std::nullopt; - - auto const secretKey = generateSecretKey(KeyType::Secp256k1, *seed); - return toBase58(TokenType::NodePublic, derivePublicKey(KeyType::Secp256k1, secretKey)); - } - - // --newnodeid discards whatever is stored. - if (cmdline.contains("newnodeid")) - return std::nullopt; + auto const secretKey = generateSecretKey(KeyType::Secp256k1, seed); + return {derivePublicKey(KeyType::Secp256k1, secretKey), secretKey}; +} +/** + * The stored identity, read without creating or modifying anything. + * + * Runs before the Application, so it opens the wallet itself rather than going + * through getWalletDB(). Three things keep that safe: the file must already + * exist, the init SQL is empty so the schema is never created, and the global + * pragmas are off because they include journal_mode, which rewrites the + * database header. The connection closes before this returns. + * + * @param config The server configuration. + * @param journal Journal for reporting an unreadable database. + * @return The stored keypair, or std::nullopt when there is none to read. + */ +std::optional> +storedIdentity(Config const& config, beast::Journal journal) +{ try { auto setup = setupDatabaseCon(config, journal); - // Standalone uses a temporary database, so nothing is persisted and this - // run will mint a fresh key. + // Standalone gets a private temporary database, so there is nothing + // persisted to read and nothing setup() could read back either. if (setup.standAlone && setup.startUp != StartUpType::Load && setup.startUp != StartUpType::LoadFile && setup.startUp != StartUpType::Replay) { return std::nullopt; } - // The global pragmas include journal_mode, which rewrites the database - // header. The wallet is opened without them everywhere else. setup.useGlobalPragma = false; - // Only read an existing file: SQLite would otherwise create one. if (std::error_code ec; !std::filesystem::exists(setup.dataDir / kWalletDbName, ec)) - { return std::nullopt; - } - // Empty init SQL: open the existing schema, never create it. DatabaseCon walletDb{ setup, kWalletDbName, @@ -136,8 +120,7 @@ resolveNodePublicKey( journal}; auto db = walletDb.checkoutDb(); - if (auto const stored = readNodeIdentity(*db)) - return toBase58(TokenType::NodePublic, stored->first); + return readNodeIdentity(*db); } catch (std::exception const& e) { @@ -147,4 +130,59 @@ resolveNodePublicKey( return std::nullopt; } +} // namespace + +std::pair +resolveNodeIdentity( + Config const& config, + boost::program_options::variables_map const& cmdline, + beast::Journal journal) +{ + // A configured seed decides the identity outright, and nothing is stored. + if (auto const seed = configuredSeed(config, cmdline)) + return keysFromSeed(*seed); + + // --newnodeid discards whatever is stored, so mint now; getNodeIdentity() + // clears the old row and stores this pair. + if (!cmdline.contains("newnodeid")) + { + if (auto const stored = storedIdentity(config, journal)) + return *stored; + } + + // Nothing to read: a first boot, or a standalone run's temporary database. + // Mint here so telemetry has an identity from construction; setup() + // persists this pair if there is a database to hold it. + return randomKeyPair(KeyType::Secp256k1); +} + +std::pair +getNodeIdentity( + Application& app, + boost::program_options::variables_map const& cmdline, + std::pair const& resolved) +{ + // A configured seed reaches neither the reader nor the writer. + if (cmdline.contains("nodeid") || app.config().exists(Sections::kNodeSeed)) + return resolved; + + auto db = app.getWalletDB().checkoutDb(); + + if (cmdline.contains("newnodeid")) + clearNodeIdentity(*db); + + // What is stored wins, so a restart keeps the node's identity even if + // another process wrote one between construction and here. Telemetry's + // resources are already built from `resolved`, so on that one run the two + // would disagree; it needs a restart to line up, as the configuration + // reference records. + if (auto const stored = readNodeIdentity(*db)) + return *stored; + + // Nothing stored, or --newnodeid just cleared it. Persist the pair + // telemetry is already reporting, so both agree from now on. + storeNodeIdentity(*db, resolved); + return resolved; +} + } // namespace xrpl diff --git a/src/xrpld/app/main/NodeIdentity.h b/src/xrpld/app/main/NodeIdentity.h index 7309f6007a..837d07184b 100644 --- a/src/xrpld/app/main/NodeIdentity.h +++ b/src/xrpld/app/main/NodeIdentity.h @@ -16,34 +16,47 @@ namespace xrpl { /** - * The cryptographic credentials identifying this server instance. + * This server's identity, resolved before the Application exists. * - * @param app The application object - * @param cmdline The command line parameters passed into the application. - */ -std::pair -getNodeIdentity(Application& app, boost::program_options::variables_map const& cmdline); - -/** - * This server's public key, read without creating or modifying anything. + * Telemetry stamps the node public key into resource attributes that are + * immutable once built, and those resources are built in ApplicationImp's + * member-init list. So the identity has to be decided before construction, + * from the config and the command line alone. * - * For callers that need the identity before the Application exists, such as - * telemetry building its resource attributes in the member-init list. Derives - * from a configured seed when there is one, otherwise reads the wallet database - * only if it already exists. - * - * getNodeIdentity() remains authoritative and mints a key when none exists. + * Always returns a keypair. It derives one from a configured seed, else reads + * the wallet database if it already exists, else mints one. Nothing is created + * or written here: getNodeIdentity() persists the result once setup() has + * opened the database. * * @param config The server configuration. * @param cmdline The command line parameters passed into the application. * @param journal Journal for reporting an unreadable database. - * @return The base58-encoded node public key, or std::nullopt if none can be - * read. + * @return This node's keypair. + * @throws std::runtime_error if a configured seed is malformed. */ -std::optional -resolveNodePublicKey( +std::pair +resolveNodeIdentity( Config const& config, boost::program_options::variables_map const& cmdline, beast::Journal journal); +/** + * The cryptographic credentials identifying this server instance, persisted. + * + * Called from setup(), once the wallet database is open. Stores @p resolved + * when the database holds no identity, and returns whatever the database holds + * when it does. + * + * @param app The application object + * @param cmdline The command line parameters passed into the application. + * @param resolved The keypair resolveNodeIdentity() decided before + * construction, which telemetry is already reporting. + * @return This node's keypair. + */ +std::pair +getNodeIdentity( + Application& app, + boost::program_options::variables_map const& cmdline, + std::pair const& resolved); + } // namespace xrpl