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/docker/telemetry/TESTING.md b/docker/telemetry/TESTING.md index 9705934aaa..128687622c 100644 --- a/docker/telemetry/TESTING.md +++ b/docker/telemetry/TESTING.md @@ -275,8 +275,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": "info" } 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/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/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/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 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