From 8697c5d8215903862fc85757ea8aa0d201616749 Mon Sep 17 00:00:00 2001 From: Nicholas Dudfield Date: Thu, 9 Apr 2026 15:34:13 +0700 Subject: [PATCH] refactor(export): explicit verified/unverified sig API in collector MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the ambiguous addSignature/hasSignature API with clearly named methods that make verification state explicit: addVerifiedSignature() — sig passed buildMultiSigningData + verify() addUnverifiedSignature() — trusted source but no multisign check yet addStandaloneSignature() — pubkey-only for standalone/test mode hasVerifiedSignature() — only returns true for verified sigs Unverified sigs (relay ordering fallback) are no longer treated as verified by the cache. When the same sig is encountered again via a path that CAN verify (e.g. SHAMap merge after the tx arrives), the verification runs and upgrades it to verified. addUnverifiedSignature() won't overwrite a verified sig, preventing downgrade. SigEntry tracks verified validators in a separate set. --- .../app/consensus/ConsensusExtensions.cpp | 27 ++--- src/xrpld/app/misc/ExportSigCollector.h | 100 +++++++++++------- 2 files changed, 76 insertions(+), 51 deletions(-) diff --git a/src/xrpld/app/consensus/ConsensusExtensions.cpp b/src/xrpld/app/consensus/ConsensusExtensions.cpp index e137f67a6..317c2ba10 100644 --- a/src/xrpld/app/consensus/ConsensusExtensions.cpp +++ b/src/xrpld/app/consensus/ConsensusExtensions.cpp @@ -676,7 +676,7 @@ ConsensusExtensions::onAcquiredSidecarSet(std::shared_ptr const& map) // Skip if we already have a verified sig for this // validator (e.g. from the proposal ingestion path). - if (exportSigCollector_.hasSignature(txHash, valPK)) + if (exportSigCollector_.hasVerifiedSignature(txHash, valPK)) return; auto const sigSlice = data.substr(65); @@ -727,7 +727,8 @@ ConsensusExtensions::onAcquiredSidecarSet(std::shared_ptr const& map) } Buffer sigBuf(sigSlice.data(), sigSlice.size()); - exportSigCollector_.addSignature(txHash, valPK, sigBuf); + exportSigCollector_.addVerifiedSignature( + txHash, valPK, sigBuf); ++merged; }); JLOG(j_.info()) << "Export: merged " << merged @@ -1645,7 +1646,7 @@ ConsensusExtensions::onTrustedPeerMessage( } // Skip if we already have a verified sig for this validator. - if (exportSigCollector_.hasSignature(txHash, senderPK)) + if (exportSigCollector_.hasVerifiedSignature(txHash, senderPK)) continue; auto const fullSlice = makeSlice(blob); @@ -1653,20 +1654,20 @@ ConsensusExtensions::onTrustedPeerMessage( // Verify the multisign signature against the inner tx when // possible. If the ttEXPORT isn't in our open ledger yet - // (relay ordering), store the sig anyway — the proposal-level + // (relay ordering), store as unverified — the proposal-level // authentication (checkSign + sender binding) provides - // sufficient trust, and the collector's stale cleanup (256 - // ledgers) bounds retention. The multisign sig will be - // verified on the destination chain regardless. + // sufficient trust. Unverified sigs can be upgraded to + // verified if encountered again through a path that CAN + // verify (e.g. SHAMap merge after the tx arrives). auto const txIt = exportTxns.find(txHash); if (txIt == exportTxns.end() || !txIt->second->isFieldPresent(sfExportedTxn)) { - JLOG(j_.debug()) << "Export: storing sig for tx " << txHash - << " without multisign verification" - << " (not in open ledger yet)"; + JLOG(j_.debug()) << "Export: storing unverified sig for tx " + << txHash << " (not in open ledger yet)"; Buffer sigBuf(sigSlice.data(), sigSlice.size()); - exportSigCollector_.addSignature(txHash, senderPK, sigBuf); + exportSigCollector_.addUnverifiedSignature( + txHash, senderPK, sigBuf); continue; } @@ -1697,7 +1698,7 @@ ConsensusExtensions::onTrustedPeerMessage( } Buffer sigBuf(sigSlice.data(), sigSlice.size()); - exportSigCollector_.addSignature(txHash, senderPK, sigBuf); + exportSigCollector_.addVerifiedSignature(txHash, senderPK, sigBuf); } } //@@end peer-harvest-export-sigs @@ -1846,7 +1847,7 @@ ConsensusExtensions::decorateMessage( prop.add_exportsignatures(s.peekData().data(), s.peekData().size()); //@@end export-attach-wire-sigs - exportSigCollector_.addSignature(txHash, valPK, sigBuf); + exportSigCollector_.addVerifiedSignature(txHash, valPK, sigBuf); JLOG(j_.debug()) << "Export: attached sig for " << txHash << " to proposal (sigLen=" << sigBuf.size() << ")"; diff --git a/src/xrpld/app/misc/ExportSigCollector.h b/src/xrpld/app/misc/ExportSigCollector.h index c252f9784..707f96c69 100644 --- a/src/xrpld/app/misc/ExportSigCollector.h +++ b/src/xrpld/app/misc/ExportSigCollector.h @@ -12,9 +12,23 @@ namespace ripple { /// Export signature collector for the retriable export approach. -/// Tracks which validators have "signed" each pending export, and -/// which exports we've already attached our own sig to (so we don't -/// redundantly re-send on every proposal). +/// +/// Stores multisign signatures from validators for pending ttEXPORT +/// transactions. Signatures arrive via two paths: +/// +/// 1. Proposal ingestion (onTrustedPeerMessage) — post-checkSign, +/// sender-bound, and (when possible) multisign-verified. +/// 2. SHAMap merge (onAcquiredSidecarSet) — trusted + verified. +/// +/// Signatures are either **verified** (cryptographically checked against +/// buildMultiSigningData) or **unverified** (stored on proposal-level +/// trust alone, e.g. when the ttEXPORT tx isn't in the open ledger yet +/// due to relay ordering). +/// +/// The distinction matters for caching: `hasVerifiedSignature()` returns +/// true only for verified sigs, so unverified sigs get re-checked when +/// encountered again through a path that CAN verify. +/// //@@start export-sig-collector-mutex /// Thread-safe. class ExportSigCollector @@ -26,8 +40,11 @@ class ExportSigCollector { std::set validators; /// Actual multisign signatures keyed by validator pubkey. - /// Empty buffers mean pubkey-only (quorum counting without real sigs). + /// Empty buffers mean pubkey-only (standalone/test quorum counting). std::map signatures; + /// Validators whose signatures have been cryptographically verified + /// via buildMultiSigningData + verify(). + std::set verified; std::uint32_t firstSeenSeq{0}; }; @@ -37,51 +54,64 @@ class ExportSigCollector static constexpr std::uint32_t maxStaleLedgers = 256; public: - /// Add a pubkey-only entry (no real signature). Used in standalone/tests - /// where quorum counting is sufficient. + /// Store a signature that has been cryptographically verified + /// against buildMultiSigningData + verify(). void - addSignature( + addVerifiedSignature( uint256 const& txnHash, PublicKey const& validator, - std::uint32_t currentSeq = 0) + Buffer const& signature) + { + std::lock_guard lock(mutex_); + auto& entry = sigs_[txnHash]; + entry.validators.insert(validator); + entry.signatures[validator] = signature; + entry.verified.insert(validator); + } + + /// Store a signature from a trusted source (checkSign + sender + /// binding passed) but without multisign content verification. + /// Used when the ttEXPORT tx isn't in the open ledger yet due + /// to relay ordering. Will be upgraded to verified if the same + /// sig is encountered again through a path that CAN verify. + void + addUnverifiedSignature( + uint256 const& txnHash, + PublicKey const& validator, + Buffer const& signature) + { + std::lock_guard lock(mutex_); + auto& entry = sigs_[txnHash]; + entry.validators.insert(validator); + // Don't overwrite a verified sig with an unverified one. + if (entry.verified.find(validator) == entry.verified.end()) + entry.signatures[validator] = signature; + } + + /// Store a pubkey-only entry (no real signature). Used in + /// standalone mode where quorum counting is sufficient. + void + addStandaloneSignature(uint256 const& txnHash, PublicKey const& validator) { std::lock_guard lock(mutex_); auto& entry = sigs_[txnHash]; entry.validators.insert(validator); if (entry.signatures.find(validator) == entry.signatures.end()) entry.signatures[validator] = Buffer{}; - if (entry.firstSeenSeq == 0 && currentSeq > 0) - entry.firstSeenSeq = currentSeq; } - /// Add a pubkey + real multisign signature entry. - void - addSignature( - uint256 const& txnHash, - PublicKey const& validator, - Buffer const& signature, - std::uint32_t currentSeq = 0) - { - std::lock_guard lock(mutex_); - auto& entry = sigs_[txnHash]; - entry.validators.insert(validator); - entry.signatures[validator] = signature; - if (entry.firstSeenSeq == 0 && currentSeq > 0) - entry.firstSeenSeq = currentSeq; - } - - /// Check if a verified signature already exists for this validator. - /// Used to skip redundant verify() calls when the same sig arrives - /// via multiple paths (proposal + SHAMap merge). + /// Check if a cryptographically verified signature exists. + /// Used to skip redundant verify() calls when the same sig + /// arrives via multiple paths (proposal + SHAMap merge). bool - hasSignature(uint256 const& txnHash, PublicKey const& validator) const + hasVerifiedSignature(uint256 const& txnHash, PublicKey const& validator) + const { std::lock_guard lock(mutex_); auto it = sigs_.find(txnHash); if (it == sigs_.end()) return false; - auto sit = it->second.signatures.find(validator); - return sit != it->second.signatures.end() && sit->second.size() > 0; + return it->second.verified.find(validator) != it->second.verified.end(); } std::size_t @@ -94,12 +124,6 @@ public: return it->second.validators.size(); } - bool - hasQuorum(uint256 const& txnHash, std::size_t threshold) const - { - return signatureCount(txnHash) >= threshold; - } - void clear(uint256 const& txnHash) {