diff --git a/src/test/app/Manifest_test.cpp b/src/test/app/Manifest_test.cpp index b5fca61816..ee823a2111 100644 --- a/src/test/app/Manifest_test.cpp +++ b/src/test/app/Manifest_test.cpp @@ -433,11 +433,7 @@ public: *db << sql, soci::into(n); return n; }; - auto save = [&]() { - cache.save(wallet, "ValidatorManifests", [](PublicKey const&) { - return false; - }); - }; + auto save = [&]() { cache.saveListed(); }; bool failed = false; try { diff --git a/src/test/app/SetManifest_test.cpp b/src/test/app/SetManifest_test.cpp index 9fb2f5dbca..d8140394d8 100644 --- a/src/test/app/SetManifest_test.cpp +++ b/src/test/app/SetManifest_test.cpp @@ -21,11 +21,13 @@ #include #include #include +#include #include #include #include #include #include +#include #include #include #include @@ -427,6 +429,119 @@ struct SetManifest_test : public beast::unit_test::suite BEAST_EXPECT(cache.revoked(master.pk())); } + void + testListedAfterLedgerRevocation( + FeatureBitset features, + bool scheduled = false) + { + testcase( + scheduled + ? "scheduled listing checks revocation before publishing trust" + : "newly listed warm identity honours the ledger revocation"); + using namespace jtx; + Env env{*this, makeConfig(), features}; + auto const master = Account("late-listed-master", KeyType::ed25519); + auto const signing = Account("late-listed-signing", KeyType::secp256k1); + auto const control = Account("late-list-control", KeyType::ed25519); + auto const controlSigning = + Account("late-list-control-signing", KeyType::secp256k1); + auto& cache = env.app().validatorManifests(); + auto& lists = env.app().validators(); + BEAST_EXPECT( + cache.applyManifest(*deserializeManifest(makeManifest( + control, controlSigning, 1))) == ManifestDisposition::accepted); + BEAST_EXPECT(lists.load( + {}, {toBase58(TokenType::NodePublic, control.pk())}, {})); + env.fund(XRP(1000), master); + env.close(); + BEAST_EXPECT( + engineResult(submit(env, makeManifest(master, signing, 1))) == + "tesSUCCESS"); + env.close(); + + BEAST_EXPECT(!lists.listed(master.pk())); + BEAST_EXPECT( + cache.applyLedgerSigningKey(*env.closed(), signing.pk()) == + master.pk()); + BEAST_EXPECT(cache.getSequence(master.pk()) == 1); + + BEAST_EXPECT( + engineResult(submit( + env, + makeManifest( + master, + signing, + std::numeric_limits::max()))) == + "tesSUCCESS"); + env.close(); + BEAST_EXPECT( + env.le(keylet::manifest(master.pk()))->getFieldU32(sfSequence) == + std::numeric_limits::max()); + BEAST_EXPECT(!cache.revoked(master.pk())); + + auto const effective = + env.timeKeeper().now() + std::chrono::seconds{120}; + if (scheduled) + { + auto const publisher = + Account("late-list-publisher", KeyType::ed25519); + auto const pubSigning = + Account("late-list-pub-signing", KeyType::secp256k1); + BEAST_EXPECT(lists.load({}, {}, {strHex(publisher.pk())})); + auto list = [&](bool future) { + Json::Value body(Json::objectValue); + body["sequence"] = future ? 2 : 1; + body["expiration"] = static_cast( + (effective + std::chrono::seconds{3600}) + .time_since_epoch() + .count()); + if (future) + body["effective"] = static_cast( + effective.time_since_epoch().count()); + body["validators"] = Json::Value(Json::arrayValue); + auto add = [&](Account const& m, Account const& s) { + Json::Value entry(Json::objectValue); + entry["validation_public_key"] = strHex(m.pk()); + entry["manifest"] = base64_encode(makeManifest(m, s, 1)); + body["validators"].append(entry); + }; + add(control, controlSigning); + if (future) + add(master, signing); + auto const raw = to_string(body); + return ValidatorBlobInfo{ + base64_encode(raw), + strHex( + sign(pubSigning.pk(), pubSigning.sk(), makeSlice(raw))), + {}}; + }; + auto const result = lists.applyLists( + base64_encode(makeManifest(publisher, pubSigning, 1)), + 2, + {list(false), list(true)}, + "test"); + BEAST_EXPECT(result.bestDisposition() == ListDisposition::accepted); + BEAST_EXPECT(lists.listed(control.pk())); + BEAST_EXPECT(!lists.listed(master.pk())); + } + else + BEAST_EXPECT(lists.load( + {}, {toBase58(TokenType::NodePublic, master.pk())}, {})); + // Run the actual beginConsensus path, not a test-side cache refresh. + if (scheduled) + BEAST_EXPECT(env.close(effective + std::chrono::seconds{1})); + else + BEAST_EXPECT(env.close()); + BEAST_EXPECT(lists.listed(master.pk())); + BEAST_EXPECT(cache.revoked(master.pk())); + BEAST_EXPECT(!lists.trusted(master.pk())); + BEAST_EXPECT(!lists.getQuorumKeys().second.contains(signing.pk())); + BEAST_EXPECT(lists.trusted(control.pk())); + BEAST_EXPECT(env.close()); + BEAST_EXPECT(cache.revoked(master.pk())); + BEAST_EXPECT(!lists.trusted(master.pk())); + } + void testSigningKeyRetrieval(FeatureBitset features) { @@ -843,6 +958,8 @@ public: testUpdate(sa); testRevocation(sa); testRetrieval(sa); + testListedAfterLedgerRevocation(sa); + testListedAfterLedgerRevocation(sa, true); testSigningKeyRetrieval(sa); testMalformed(sa); testEnvelopeRejections(sa); diff --git a/src/test/app/ValidatorList_test.cpp b/src/test/app/ValidatorList_test.cpp index 80bf43330b..6c3ca2a76f 100644 --- a/src/test/app/ValidatorList_test.cpp +++ b/src/test/app/ValidatorList_test.cpp @@ -287,9 +287,7 @@ private: cold.loadListed(wallet); BEAST_EXPECT(!cold.getRawManifest(master.first)); BEAST_EXPECT(!cold.getRawManifest(unrelated.masterPublic)); - cold.save(wallet, "ValidatorManifests", [](PublicKey const&) { - return false; - }); + cold.saveListed(); } BEAST_EXPECT(rows() == saved); @@ -325,9 +323,7 @@ private: manifests.applyGossipManifest(*deserializeManifest(base64_decode( validator.manifest))) == ManifestDisposition::stale); BEAST_EXPECT(!manifests.getRawManifest(unrelated.masterPublic)); - manifests.save(wallet, "ValidatorManifests", [&](PublicKey const& key) { - return lists.listed(key); - }); + manifests.saveListed(); BEAST_EXPECT( rows() == saved - 1); // One row replaces the old version and revocation. @@ -336,10 +332,7 @@ private: BEAST_EXPECT(manifests.loadConfig(local.manifest, {})); for (int i = 0; i < 2; ++i) { - manifests.save( - wallet, "ValidatorManifests", [&](PublicKey const& key) { - return lists.listed(key); - }); + manifests.saveListed(); BEAST_EXPECT(rows() == saved); } @@ -447,9 +440,7 @@ private: *db, "ValidatorManifests", {master.first}, env.journal); BEAST_EXPECT(saved.at(master.first).serialized == revokedBytes); } - cache.save(wallet, "ValidatorManifests", [](PublicKey const&) { - return false; - }); + cache.saveListed(); } ManifestCache restarted{env.journal, 0}; ManifestCache publishers; diff --git a/src/xrpld/app/main/Application.cpp b/src/xrpld/app/main/Application.cpp index 8541d55c61..dac76c5860 100644 --- a/src/xrpld/app/main/Application.cpp +++ b/src/xrpld/app/main/Application.cpp @@ -1643,10 +1643,7 @@ ApplicationImp::run() validatorSites_->stop(); // TODO Store manifests in manifests.sqlite instead of wallet.db - validatorManifests_->save( - getWalletDB(), "ValidatorManifests", [this](PublicKey const& pubKey) { - return validators().listed(pubKey); - }); + validatorManifests_->saveListed(); // List updates restore wallet history while holding the list lock. Never // call back into that lock from a save that already holds the wallet. diff --git a/src/xrpld/app/misc/Manifest.h b/src/xrpld/app/misc/Manifest.h index 038609f939..7af2fbfb02 100644 --- a/src/xrpld/app/misc/Manifest.h +++ b/src/xrpld/app/misc/Manifest.h @@ -570,11 +570,10 @@ public: void loadListed(DatabaseCon& dbCon); - /** Save cached manifests to database. + /** Replace a table with selected cached manifests and all revocations. - With a wallet attached, save listed/configured and pending history, - preserve unrelated wallet rows, and ignore isTrusted. Otherwise use - isTrusted and retain revocations, as for the publisher cache. + This is the legacy/publisher save policy, independent of wallet + attachment. Use saveListed() for selective validator history. @param dbCon Database containing dbTable @@ -590,6 +589,15 @@ public: std::string const& dbTable, std::function const& isTrusted); + /** Save listed/configured and pending history to the attached wallet. + + Preserve unrelated rows. Requires loadListed() to have attached the + wallet; no caller-supplied trust predicate or ValidatorList lock is + used. + */ + void + saveListed(); + /** Invokes the callback once for every populated manifest. @note Do not call ManifestCache member functions from within the diff --git a/src/xrpld/app/misc/NetworkOPs.cpp b/src/xrpld/app/misc/NetworkOPs.cpp index 613c34e029..02125623ec 100644 --- a/src/xrpld/app/misc/NetworkOPs.cpp +++ b/src/xrpld/app/misc/NetworkOPs.cpp @@ -2071,7 +2071,11 @@ NetworkOPsImp::beginConsensus( closingInfo.parentCloseTime, *this, app_.overlay(), - app_.getHashRouter()); + app_.getHashRouter(), + [this, &prevLedger](hash_set const& candidates) { + if (prevLedger->rules().enabled(featureOnChainManifests)) + app_.validatorManifests().applyLedger(*prevLedger, candidates); + }); if (!changes.added.empty() || !changes.removed.empty()) { diff --git a/src/xrpld/app/misc/ValidatorList.h b/src/xrpld/app/misc/ValidatorList.h index 3d40fd271f..659183126f 100644 --- a/src/xrpld/app/misc/ValidatorList.h +++ b/src/xrpld/app/misc/ValidatorList.h @@ -31,6 +31,7 @@ #include #include #include +#include #include #include #include @@ -474,6 +475,10 @@ public: @param seenValidators Set of NodeIDs of validators that have signed recently received validations + @param reconcileCandidates Optional synchronous reconciliation before + new trust is published, after pending lists rotate. Called under the + list lock; must not re-enter ValidatorList. + @return TrustedKeyChanges instance with newly trusted or untrusted node identities. @@ -487,7 +492,9 @@ public: NetClock::time_point closeTime, NetworkOPs& ops, Overlay& overlay, - HashRouter& hashRouter); + HashRouter& hashRouter, + std::function const&)> const& + reconcileCandidates = {}); /** Get quorum value for current trusted key set diff --git a/src/xrpld/app/misc/detail/Manifest.cpp b/src/xrpld/app/misc/detail/Manifest.cpp index 49f49c7fa9..e1873e1510 100644 --- a/src/xrpld/app/misc/detail/Manifest.cpp +++ b/src/xrpld/app/misc/detail/Manifest.cpp @@ -432,13 +432,12 @@ ManifestCache::pin(hash_set keys) try { auto db = wallet->checkoutDb(); - saveManifests( + compactManifests( *db, "ValidatorManifests", [](PublicKey const&) { return true; }, departing, - j_, - true); + j_); } catch (soci::soci_error const& e) { @@ -886,20 +885,28 @@ ManifestCache::save( std::shared_lock lock{mutex_}; auto db = dbCon.checkoutDb(); - saveManifests( + saveManifests(*db, dbTable, isTrusted, map_, j_); +} + +void +ManifestCache::saveListed() +{ + std::shared_lock lock{mutex_}; + if (!wallet_) + Throw("Validator manifest wallet is not attached"); + auto db = wallet_->checkoutDb(); + + compactManifests( *db, - dbTable, - [this, &isTrusted](PublicKey const& key) { + "ValidatorManifests", + [this](PublicKey const& key) { // Membership is already mirrored here. Do not take ValidatorList's // lock while holding the cache lock (pin() takes them in reverse). - if (!wallet_) - return isTrusted(key); return pinned_.contains(key) || configured_.contains(key) || pendingSave_.contains(key); }, map_, - j_, - wallet_ != nullptr); + j_); } // Clean up macros to avoid namespace pollution diff --git a/src/xrpld/app/misc/detail/ValidatorList.cpp b/src/xrpld/app/misc/detail/ValidatorList.cpp index 75ac486a0c..a2fc20bd7f 100644 --- a/src/xrpld/app/misc/detail/ValidatorList.cpp +++ b/src/xrpld/app/misc/detail/ValidatorList.cpp @@ -1938,7 +1938,8 @@ ValidatorList::updateTrusted( NetClock::time_point closeTime, NetworkOPs& ops, Overlay& overlay, - HashRouter& hashRouter) + HashRouter& hashRouter, + std::function const&)> const& reconcileCandidates) { using namespace std::chrono_literals; if (timeKeeper_.now() > closeTime + 30s) @@ -2030,6 +2031,20 @@ ValidatorList::updateTrusted( if (good) ops.clearUNLBlocked(); + // The per-round ledger pass covered the previously trusted set. A key can + // become eligible between rounds or in the pending-list rotation above; + // reconcile it now so a warm stale binding is never trusted for one round. + if (reconcileCandidates) + { + hash_set candidates; + for (auto const& [key, count] : keyListings_) + if (count >= listThreshold_ && !trustedMasterKeys_.contains(key) && + !validatorManifests_.revoked(key)) + candidates.insert(key); + if (!candidates.empty()) + reconcileCandidates(candidates); + } + TrustChanges trustChanges; auto it = trustedMasterKeys_.cbegin(); diff --git a/src/xrpld/app/rdb/Wallet.h b/src/xrpld/app/rdb/Wallet.h index be2b3601cf..5eef73e406 100644 --- a/src/xrpld/app/rdb/Wallet.h +++ b/src/xrpld/app/rdb/Wallet.h @@ -84,8 +84,6 @@ getManifestsForKeys( * @param isTrusted Callback that returns true if the key is trusted. * @param map Maps public keys to manifests. * @param j Journal. - * @param preserveUnloaded Keep unloaded history and save only local keys, - * without duplicating versions already saved (validator cache). */ void saveManifests( @@ -93,8 +91,21 @@ saveManifests( std::string const& dbTable, std::function const& isTrusted, hash_map const& map, - beast::Journal j, - bool preserveUnloaded = false); + beast::Journal j); + +/** Save selected identities without deleting unrelated wallet history. + + Keep the highest valid cache/disk version per selected master and compact + its old rows in one transaction. shouldRetain selects from map; unlike the + legacy saveManifests(), this never rewrites the whole table. +*/ +void +compactManifests( + soci::session& session, + std::string const& dbTable, + std::function const& shouldRetain, + hash_map const& map, + beast::Journal j); /** * @brief addValidatorManifest Saves the manifest of a validator to the diff --git a/src/xrpld/app/rdb/detail/Wallet.cpp b/src/xrpld/app/rdb/detail/Wallet.cpp index 52080131e9..79bd3ecd49 100644 --- a/src/xrpld/app/rdb/detail/Wallet.cpp +++ b/src/xrpld/app/rdb/detail/Wallet.cpp @@ -135,56 +135,59 @@ saveManifest( soci::use(rawData); } +void +compactManifests( + soci::session& session, + std::string const& dbTable, + std::function const& shouldRetain, + hash_map const& map, + beast::Journal j) +{ + soci::transaction tr(session); + hash_map retained; + for (auto const& [key, manifest] : map) + if (shouldRetain(key)) + retained.emplace(key, manifest.clone()); + + if (!retained.empty()) + { + // Stage row IDs in SQLite, not an unbounded C++ vector. Keep the + // source table untouched while its read cursor is active. This + // table is created and dropped in one transaction, with no schema + // migration or persistent staging state. + auto const obsolete = dbTable + "_Compacting"; + session << "CREATE TABLE " + obsolete + " (RowID INTEGER PRIMARY KEY);"; + readManifests( + session, + dbTable, + [&](Manifest m, std::int64_t rowid) { + auto const it = retained.find(m.masterKey); + if (it == retained.end()) + return; + if (m.sequence > it->second.sequence && m.verify()) + it->second = std::move(m); + session << "INSERT INTO " + obsolete + " (RowID) VALUES (:id);", + soci::use(rowid); + }, + j); + session << "DELETE FROM " + dbTable + + " WHERE rowid IN (SELECT RowID FROM " + obsolete + ");"; + for (auto const& [key, manifest] : retained) + saveManifest(session, dbTable, manifest.serialized); + session << "DROP TABLE " + obsolete + ";"; + } + tr.commit(); +} + void saveManifests( soci::session& session, std::string const& dbTable, std::function const& isTrusted, hash_map const& map, - beast::Journal j, - bool preserveUnloaded) + beast::Journal j) { soci::transaction tr(session); - if (preserveUnloaded) - { - hash_map retained; - for (auto const& [key, manifest] : map) - if (isTrusted(key)) - retained.emplace(key, manifest.clone()); - - if (!retained.empty()) - { - // Stage row IDs in SQLite, not an unbounded C++ vector. Keep the - // source table untouched while its read cursor is active. This - // table is created and dropped in one transaction, with no schema - // migration or persistent staging state. - auto const obsolete = dbTable + "_Compacting"; - session << "CREATE TABLE " + obsolete + - " (RowID INTEGER PRIMARY KEY);"; - readManifests( - session, - dbTable, - [&](Manifest m, std::int64_t rowid) { - auto const it = retained.find(m.masterKey); - if (it == retained.end()) - return; - if (m.sequence > it->second.sequence && m.verify()) - it->second = std::move(m); - session - << "INSERT INTO " + obsolete + " (RowID) VALUES (:id);", - soci::use(rowid); - }, - j); - session << "DELETE FROM " + dbTable + - " WHERE rowid IN (SELECT RowID FROM " + obsolete + ");"; - for (auto const& [key, manifest] : retained) - saveManifest(session, dbTable, manifest.serialized); - session << "DROP TABLE " + obsolete + ";"; - } - tr.commit(); - return; - } - session << "DELETE FROM " << dbTable; for (auto const& v : map) {