From e55f48caf8c212bbec37432e3d49bef0b12e9077 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Mon, 14 Sep 2026 23:31:27 +0100 Subject: [PATCH] test(server): cover every resolveNodeIdentity() decision branch Lift the seed parsing and the stored-vs-mint choice into two libxrpl helpers, parseNodeIdentitySeed() and selectNodeIdentity(), so xrpl_tests can drive each branch without an xrpld Config. resolveNodeIdentity() now marshals Config and the cmdline into them; behaviour is unchanged. Also pin that storeNodeIdentity() appends (row count, not SQLite row order), fix the test header that described getNodeIdentity()'s property as the store's, and route NullTelemetry::getMeter() through noopMeter(). Co-Authored-By: Claude Fable 5.1 --- include/xrpl/server/Wallet.h | 41 ++++ src/libxrpl/server/Wallet.cpp | 53 +++++ src/libxrpl/telemetry/NullTelemetry.cpp | 9 +- src/tests/libxrpl/server/NodeIdentity.cpp | 257 +++++++++++++++++++--- src/xrpld/app/main/NodeIdentity.cpp | 82 ++----- 5 files changed, 346 insertions(+), 96 deletions(-) diff --git a/include/xrpl/server/Wallet.h b/include/xrpl/server/Wallet.h index e549a1305e..8a1a76c55c 100644 --- a/include/xrpl/server/Wallet.h +++ b/include/xrpl/server/Wallet.h @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -83,6 +84,46 @@ saveManifests( void addValidatorManifest(soci::session& session, std::string const& serialized); +/** + * The seed a configured [node_seed] or --nodeid names. + * + * The command-line value wins when both are set. A cmdline value is parsed + * with parseGenericSeed(rfc1751=false); a config value is parsed as Base58 + * only. Empty inputs count as supplied-but-invalid and throw. + * + * @param cmdlineSeed The --nodeid value, or std::nullopt when not passed. + * @param configSeed The first line of [node_seed], or std::nullopt when the + * section is absent. An empty section is passed as "". + * @return The parsed seed, or std::nullopt when neither is set. + * @throws std::runtime_error if the value present is malformed. + */ +[[nodiscard]] std::optional +parseNodeIdentitySeed( + std::optional const& cmdlineSeed, + std::optional const& configSeed); + +/** + * Pick this node's keypair from a pre-parsed seed and a stored-key reader. + * + * A configured seed wins outright and the reader is not consulted. When + * newNodeId is set, mint a fresh pair and skip the reader too. Otherwise + * consult the reader; return its pair if it has one, else mint. + * + * Lifting the decision out of the Application layer lets libxrpl-level + * tests drive every branch without an xrpld Config. + * + * @param configuredSeed Seed from parseNodeIdentitySeed(), or std::nullopt. + * @param newNodeId True when --newnodeid was passed. + * @param readStored Callable that returns the wallet's stored pair, or + * std::nullopt when nothing is stored. + * @return This node's keypair. + */ +[[nodiscard]] std::pair +selectNodeIdentity( + std::optional const& configuredSeed, + bool newNodeId, + std::function>()> const& readStored); + /** * Delete any saved public/private key associated with this node. */ diff --git a/src/libxrpl/server/Wallet.cpp b/src/libxrpl/server/Wallet.cpp index ec9f1fd3b5..d1b36fb143 100644 --- a/src/libxrpl/server/Wallet.cpp +++ b/src/libxrpl/server/Wallet.cpp @@ -3,13 +3,16 @@ #include #include #include +#include #include #include #include +#include #include #include #include #include +#include #include #include #include @@ -33,6 +36,7 @@ #include #include #include +#include #include #include #include @@ -142,6 +146,55 @@ addValidatorManifest(soci::session& session, std::string const& serialized) tr.commit(); } +std::optional +parseNodeIdentitySeed( + std::optional const& cmdlineSeed, + std::optional const& configSeed) +{ + if (cmdlineSeed) + { + auto seed = parseGenericSeed(*cmdlineSeed, false); + if (!seed) + Throw("Invalid 'nodeid' in command line"); + return seed; + } + + if (configSeed) + { + auto seed = parseBase58(*configSeed); + if (!seed) + { + Throw( + std::string("Invalid [") + Sections::kNodeSeed + "] in configuration file"); + } + return seed; + } + + return std::nullopt; +} + +std::pair +selectNodeIdentity( + std::optional const& configuredSeed, + bool newNodeId, + std::function>()> const& readStored) +{ + if (configuredSeed) + { + auto const sk = generateSecretKey(KeyType::Secp256k1, *configuredSeed); + return {derivePublicKey(KeyType::Secp256k1, sk), sk}; + } + + // --newnodeid discards whatever is stored, so mint now. + if (!newNodeId) + { + if (auto stored = readStored()) + return *stored; + } + + return randomKeyPair(KeyType::Secp256k1); +} + void clearNodeIdentity(soci::session& session) { diff --git a/src/libxrpl/telemetry/NullTelemetry.cpp b/src/libxrpl/telemetry/NullTelemetry.cpp index a8dd40f322..e6bac72d65 100644 --- a/src/libxrpl/telemetry/NullTelemetry.cpp +++ b/src/libxrpl/telemetry/NullTelemetry.cpp @@ -25,7 +25,6 @@ #ifdef XRPL_ENABLE_TELEMETRY #include #include -#include #include #include #include @@ -141,11 +140,11 @@ public: } opentelemetry::nostd::shared_ptr - getMeter(std::string_view) override + getMeter(std::string_view name) override { - static auto noopMeter = opentelemetry::nostd::shared_ptr( - new opentelemetry::metrics::NoopMeter()); - return noopMeter; + // Route through the shared helper so the meter identity (name + + // kMeterVersion) matches every other noop path in the process. + return noopMeter(name); } #endif }; diff --git a/src/tests/libxrpl/server/NodeIdentity.cpp b/src/tests/libxrpl/server/NodeIdentity.cpp index 22dc9d2948..086b63bbcf 100644 --- a/src/tests/libxrpl/server/NodeIdentity.cpp +++ b/src/tests/libxrpl/server/NodeIdentity.cpp @@ -1,32 +1,46 @@ /** * @file NodeIdentity.cpp - * GTest unit tests for the wallet database's node-identity storage. + * GTest unit tests for the wallet's node-identity helpers and the pure + * decision logic behind resolveNodeIdentity(). * - * 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. + * Two groups of functions share the NodeIdentity table: + * - readNodeIdentity() / storeNodeIdentity() / clearNodeIdentity() are the + * wallet-layer primitives xrpld composes at startup. + * - getNodeIdentity(session&) is the read-or-mint helper. It persists a + * freshly minted pair, so a second call after a mint returns the same + * key: that is the property that keeps a node's identity stable across + * restarts. * - * Each test gets its own database file in a temporary directory, so nothing - * here depends on order or on the developer's data directory. + * parseNodeIdentitySeed() and selectNodeIdentity() are the libxrpl-level + * decision helpers that xrpld's resolveNodeIdentity() marshals its inputs + * into. Every outcome branch of resolveNodeIdentity() reduces to one of + * these two, so testing them here covers the decision tree without an + * xrpld Config. + * + * Each database-backed test gets its own 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 #include +#include #include +#include +#include #include +#include #include using namespace xrpl; @@ -82,7 +96,7 @@ 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"); + TempWalletDb const wallet("store-then-read"); auto const minted = randomKeyPair(KeyType::Secp256k1); { @@ -98,25 +112,32 @@ TEST(WalletNodeIdentity, store_then_read_returns_the_same_pair) EXPECT_EQ(stored->second, minted.second); } -TEST(WalletNodeIdentity, store_does_not_replace_an_existing_identity) +TEST(WalletNodeIdentity, store_appends_rather_than_replacing) { - // 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) + // Catches storeNodeIdentity being changed into an UPSERT: the wallet + // helper deliberately inserts without clearing, and the caller + // (getNodeIdentity(session&) or resolveNodeIdentity+setup) is what makes + // sure the table is empty first. Two stores must leave two rows. + TempWalletDb const wallet("store-appends"); + auto const first = randomKeyPair(KeyType::Secp256k1); + auto const second = randomKeyPair(KeyType::Secp256k1); + ASSERT_NE(first.first, second.first) << "the two pairs must differ for this test to mean anything"; - storeNodeIdentity(*db, other); + auto db = (*wallet).checkoutDb(); + storeNodeIdentity(*db, first); + storeNodeIdentity(*db, second); + int rowCount = 0; + *db << "SELECT COUNT(*) FROM NodeIdentity;", soci::into(rowCount); + EXPECT_EQ(rowCount, 2) << "storeNodeIdentity must not clear the table"; + + // The read has no ORDER BY, so pin only that ONE of the two stored keys + // comes back -- not which one. auto const stored = readNodeIdentity(*db); ASSERT_TRUE(stored.has_value()); - EXPECT_EQ(stored->first, first.first); - EXPECT_EQ(getNodeIdentity(*db).first, first.first); + EXPECT_TRUE(stored->first == first.first || stored->first == second.first) + << "readNodeIdentity must return one of the two stored pairs"; } TEST(WalletNodeIdentity, clear_then_store_installs_the_new_pair) @@ -124,7 +145,7 @@ 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"); + TempWalletDb const wallet("clear-then-store"); auto db = (*wallet).checkoutDb(); auto const first = getNodeIdentity(*db); @@ -146,7 +167,7 @@ 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"); + TempWalletDb const wallet("mint-and-persist"); auto db = (*wallet).checkoutDb(); auto const minted = getNodeIdentity(*db); @@ -156,3 +177,185 @@ TEST(WalletNodeIdentity, get_mints_and_persists_when_the_table_is_empty) EXPECT_EQ(stored->first, minted.first); EXPECT_EQ(getNodeIdentity(*db).first, minted.first); } + +// ----------------------------------------------------------------------------- +// parseNodeIdentitySeed() +// +// The seed-parsing half of resolveNodeIdentity(). Each test drives one branch +// of the review's outcome list: cmdline valid, cmdline malformed, config valid, +// config malformed, neither, and cmdline-wins-over-config. +// ----------------------------------------------------------------------------- + +namespace { + +// A base58 seed known to parse (from the "masterpassphrase" node in +// src/test/protocol/Seed_test.cpp). +constexpr auto kValidSeed = "snoPBrXtMeMyMHUVTgbuqAfg1SUTb"; + +// Public key derived from kValidSeed (secp256k1), same source. +constexpr auto kValidSeedPublic = "n94a1u4jAz288pZLtw6yFWVbi89YamiC6JBXPVUj5zmExe5fTVg9"; + +// A second base58 seed to prove cmdline-wins-over-config. +constexpr auto kOtherSeed = "snMKnVku798EnBwUfxeSD8953sLYA"; + +} // namespace + +TEST(ParseNodeIdentitySeed, both_absent_returns_nullopt) +{ + // Catches replacing the fallthrough with a throw, or making it mint a + // random seed. resolveNodeIdentity() then falls through to the wallet. + EXPECT_FALSE(parseNodeIdentitySeed(std::nullopt, std::nullopt).has_value()); +} + +TEST(ParseNodeIdentitySeed, valid_cmdline_returns_that_seed) +{ + // Catches swapping parseGenericSeed to always return nullopt, or reading + // configSeed instead of cmdlineSeed. Pin the concrete public key derived + // from the seed so the returned optional cannot silently be a different + // valid seed. + auto const seed = parseNodeIdentitySeed(std::string{kValidSeed}, std::nullopt); + ASSERT_TRUE(seed.has_value()); + + auto const sk = generateSecretKey(KeyType::Secp256k1, *seed); + auto const pk = derivePublicKey(KeyType::Secp256k1, sk); + EXPECT_EQ(toBase58(TokenType::NodePublic, pk), std::string{kValidSeedPublic}); +} + +TEST(ParseNodeIdentitySeed, valid_config_returns_that_seed) +{ + // Catches ignoring the config branch. Same public-key pin as above so the + // result cannot silently drift to another seed. + auto const seed = parseNodeIdentitySeed(std::nullopt, std::string{kValidSeed}); + ASSERT_TRUE(seed.has_value()); + + auto const sk = generateSecretKey(KeyType::Secp256k1, *seed); + auto const pk = derivePublicKey(KeyType::Secp256k1, sk); + EXPECT_EQ(toBase58(TokenType::NodePublic, pk), std::string{kValidSeedPublic}); +} + +TEST(ParseNodeIdentitySeed, malformed_cmdline_throws) +{ + // Catches removing the throw. An empty string flunks parseGenericSeed( + // rfc1751=false) because the first check inside is str.empty(). A base58 + // public key would too, but empty is the shorter probe. + EXPECT_THROW( + static_cast(parseNodeIdentitySeed(std::string{}, std::nullopt)), std::runtime_error); +} + +TEST(ParseNodeIdentitySeed, malformed_config_throws) +{ + // Catches removing the throw. "garbage" is neither valid base58 nor a + // valid seed encoding, so parseBase58 returns nullopt and the + // config branch throws. + EXPECT_THROW( + static_cast(parseNodeIdentitySeed(std::nullopt, std::string{"garbage"})), + std::runtime_error); +} + +TEST(ParseNodeIdentitySeed, cmdline_wins_over_config) +{ + // Catches swapping the two if-branches. Passing DIFFERENT valid seeds on + // each input and asserting the returned seed derives to kValidSeed's + // public key proves which one won. + auto const seed = parseNodeIdentitySeed(std::string{kValidSeed}, std::string{kOtherSeed}); + ASSERT_TRUE(seed.has_value()); + + auto const sk = generateSecretKey(KeyType::Secp256k1, *seed); + auto const pk = derivePublicKey(KeyType::Secp256k1, sk); + EXPECT_EQ(toBase58(TokenType::NodePublic, pk), std::string{kValidSeedPublic}); +} + +// ----------------------------------------------------------------------------- +// selectNodeIdentity() +// +// The decision half of resolveNodeIdentity(). Every remaining branch from the +// review's outcome list reduces to one of these four cases, because +// storedIdentity() catches its own exceptions and returns std::nullopt for +// every failure mode (standalone non-Load, wallet absent, invalid row, read +// throws) -- see storedIdentity() in NodeIdentity.cpp. +// ----------------------------------------------------------------------------- + +namespace { + +// Reader that records whether it was called. resolveNodeIdentity()'s reader +// is a filesystem-touching lambda, so pinning "was it consulted" catches the +// mutations that flip which cases open the wallet. +struct TrackingReader +{ + bool called{false}; + std::optional> value; + + std::function>()> + fn() + { + return [this] { + called = true; + return value; + }; + } +}; + +} // namespace + +TEST(SelectNodeIdentity, seed_wins_and_reader_not_consulted) +{ + // Catches removing the seed branch (or checking newNodeId first). The + // returned pair must be keysFromSeed(kValidSeed); if the seed branch is + // gone the reader gets called and its recorded pair or a fresh mint + // comes back instead. + TrackingReader reader; + reader.value = randomKeyPair(KeyType::Secp256k1); + + auto const seed = parseBase58(std::string{kValidSeed}); + ASSERT_TRUE(seed.has_value()); + + auto const result = selectNodeIdentity(seed, /*newNodeId=*/false, reader.fn()); + + EXPECT_FALSE(reader.called) << "the reader must not run when a seed is configured"; + EXPECT_EQ(toBase58(TokenType::NodePublic, result.first), std::string{kValidSeedPublic}); +} + +TEST(SelectNodeIdentity, newnodeid_mints_fresh_and_skips_reader) +{ + // Catches removing the `!newNodeId` guard. With a stored pair available, + // the mint path must still run and the stored pair must not come back. + TrackingReader reader; + reader.value = randomKeyPair(KeyType::Secp256k1); + auto const storedPair = *reader.value; + + auto const result = selectNodeIdentity(std::nullopt, /*newNodeId=*/true, reader.fn()); + + EXPECT_FALSE(reader.called) << "--newnodeid must not open the wallet"; + EXPECT_NE(result.first, storedPair.first) << "the stored pair must be discarded"; +} + +TEST(SelectNodeIdentity, returns_stored_when_reader_has_one) +{ + // Catches replacing the stored-return with a mint. Also catches the + // reader being called but its result discarded. + TrackingReader reader; + reader.value = randomKeyPair(KeyType::Secp256k1); + auto const storedPair = *reader.value; + + auto const result = selectNodeIdentity(std::nullopt, /*newNodeId=*/false, reader.fn()); + + EXPECT_TRUE(reader.called); + EXPECT_EQ(result.first, storedPair.first); + EXPECT_EQ(result.second, storedPair.second); +} + +TEST(SelectNodeIdentity, mints_when_nothing_stored) +{ + // Catches removing the mint fallback. With the reader returning + // nullopt, selectNodeIdentity must still produce a keypair, and it must + // differ from anything it could have accidentally reused. + TrackingReader reader; // value stays std::nullopt. + + auto const result = selectNodeIdentity(std::nullopt, /*newNodeId=*/false, reader.fn()); + + EXPECT_TRUE(reader.called); + // The mint path is randomKeyPair(), so the two calls must yield distinct + // keys. Same probe the wallet-side tests use. + auto const another = randomKeyPair(KeyType::Secp256k1); + EXPECT_NE(result.first, another.first); +} diff --git a/src/xrpld/app/main/NodeIdentity.cpp b/src/xrpld/app/main/NodeIdentity.cpp index bb62c99cf4..76a9fe94dd 100644 --- a/src/xrpld/app/main/NodeIdentity.cpp +++ b/src/xrpld/app/main/NodeIdentity.cpp @@ -4,15 +4,11 @@ #include #include -#include #include #include #include -#include #include #include -#include -#include #include #include #include @@ -23,7 +19,6 @@ #include #include #include -#include #include #include #include @@ -32,53 +27,6 @@ namespace xrpl { 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")) - { - auto seed = parseGenericSeed(cmdline["nodeid"].as(), false); - if (!seed) - Throw("Invalid 'nodeid' in command line"); - return seed; - } - - 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; - } - - return std::nullopt; -} - -/** - * The keypair a seed defines. - * - * @param seed The configured seed. - * @return The derived secp256k1 keypair. - */ -std::pair -keysFromSeed(Seed const& seed) -{ - auto const secretKey = generateSecretKey(KeyType::Secp256k1, seed); - return {derivePublicKey(KeyType::Secp256k1, secretKey), secretKey}; -} - /** * The stored identity, read without creating or modifying anything. * @@ -138,22 +86,28 @@ resolveNodeIdentity( 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); + // Marshal Config and the cmdline into the libxrpl-level primitives the + // decision helpers take. Keeping the decision in libxrpl lets its tests + // cover every branch without an xrpld Config. + std::optional cmdlineSeed; + if (cmdline.contains("nodeid")) + cmdlineSeed = cmdline["nodeid"].as(); - // --newnodeid discards whatever is stored, so mint now; getNodeIdentity() - // clears the old row and stores this pair. - if (!cmdline.contains("newnodeid")) + std::optional configSeedLine; + if (config.exists(Sections::kNodeSeed)) { - if (auto const stored = storedIdentity(config, journal)) - return *stored; + auto const& lines = config.section(Sections::kNodeSeed).lines(); + // Present-but-empty stays as an empty string, so parseNodeIdentitySeed + // throws the same "invalid [node_seed]" error the old code did. + configSeedLine = lines.empty() ? std::string{} : lines.front(); } - // 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); + auto const seed = parseNodeIdentitySeed(cmdlineSeed, configSeedLine); + bool const newNodeId = cmdline.contains("newnodeid"); + + // storedIdentity() catches its own exceptions and returns std::nullopt on + // any read failure, so a wallet that will not open collapses into "mint". + return selectNodeIdentity(seed, newNodeId, [&] { return storedIdentity(config, journal); }); } std::pair