From 156a8cbb8510dee1560943b3809fcbf668faa0bc Mon Sep 17 00:00:00 2001 From: Nicholas Dudfield Date: Mon, 15 Jun 2026 17:14:30 +0700 Subject: [PATCH] refactor(rng): address review minors (dead-API rename + explicit-final dedup) Two small follow-ups from the Codex review of the tier-2 implementation, both on the consensus-extension internals: - Rename shouldZeroEntropy() -> belowValidatorQuorum(). The selector replaced its callers, leaving it production-dead and MISNAMED: post-tier-2, "below the 80% validator quorum" is no longer "zero entropy" (a participant_aligned set is sub-quorum but non-zero). The new name + a doc comment make it a tier-3 eligibility predicate only and warn against gating injection on it (selectEntropy() owns the tiering). Behavior unchanged. - buildExplicitFinalProposalTxSet now dedups the entropy pseudo-tx by VALUE, not type. Its comment claimed it "mirrors onPreBuild", but onPreBuild went value-based: it verifies a present pseudo-tx is the EXACT txID it would have produced and logs a determinism-violation on mismatch. Explicit-final now does the same (skip-duplicate-verified on match, error log on mismatch), returning the base unchanged either way. Default-off experimental path, so low blast radius, but the two paths now agree. --- .../consensus/ConsensusExtensions_test.cpp | 6 +-- .../app/consensus/ConsensusExtensions.cpp | 39 ++++++++++++------- src/xrpld/app/consensus/ConsensusExtensions.h | 6 ++- 3 files changed, 34 insertions(+), 17 deletions(-) diff --git a/src/test/consensus/ConsensusExtensions_test.cpp b/src/test/consensus/ConsensusExtensions_test.cpp index c2a595cdf..d6f8f9f16 100644 --- a/src/test/consensus/ConsensusExtensions_test.cpp +++ b/src/test/consensus/ConsensusExtensions_test.cpp @@ -1161,7 +1161,7 @@ class ConsensusExtensions_test : public beast::unit_test::suite auto const ledger = env.app().getLedgerMaster().getClosedLedger(); ce.onRoundStart(RCLCxLedger{ledger}, {}); ce.setRngEnabledThisRound(true); - BEAST_EXPECT(ce.shouldZeroEntropy()); + BEAST_EXPECT(ce.belowValidatorQuorum()); CanonicalTXSet retriableTxs{makeHash("rng-zero-fallback-salt")}; auto const seq = ledger->seq() + 1; @@ -1239,7 +1239,7 @@ class ConsensusExtensions_test : public beast::unit_test::suite auto const entropySetHash = ce.buildEntropySet(seq); BEAST_EXPECT(ce.isSidecarSet(entropySetHash)); - BEAST_EXPECT(!ce.shouldZeroEntropy()); + BEAST_EXPECT(!ce.belowValidatorQuorum()); CanonicalTXSet retriableTxs{makeHash("entropy-set-prebuild-salt")}; ce.onPreBuild(retriableTxs, seq, txSetHash); @@ -2809,7 +2809,7 @@ class ConsensusExtensions_test : public beast::unit_test::suite BEAST_EXPECT(ce.exportSigConvergenceFailed()); ce.setEntropyFailed(); - BEAST_EXPECT(ce.shouldZeroEntropy()); + BEAST_EXPECT(ce.belowValidatorQuorum()); ce.generateEntropySecret(); BEAST_EXPECT(!ce.hasAnyReveals()); diff --git a/src/xrpld/app/consensus/ConsensusExtensions.cpp b/src/xrpld/app/consensus/ConsensusExtensions.cpp index 6c2bc6904..48cf90a38 100644 --- a/src/xrpld/app/consensus/ConsensusExtensions.cpp +++ b/src/xrpld/app/consensus/ConsensusExtensions.cpp @@ -413,7 +413,7 @@ ConsensusExtensions::hasAnyReveals() const } bool -ConsensusExtensions::shouldZeroEntropy() const +ConsensusExtensions::belowValidatorQuorum() const { if (entropyFailed_ || !entropySetMap_) return true; @@ -452,9 +452,9 @@ ConsensusExtensions::selectEntropy( // No agreed entropy set (round failed, or none was built) → fallback. We do // NOT fall back merely for being below the 80% quorum the way - // shouldZeroEntropy() does: a sub-quorum-but-aligned set may still qualify - // for participant_aligned (tier 2). The tier ladder below decides from the - // agreed participant count. + // belowValidatorQuorum() does: a sub-quorum-but-aligned set may still + // qualify for participant_aligned (tier 2). The tier ladder below decides + // from the agreed participant count. if (entropyFailed_ || !entropySetMap_) return fallback(); @@ -591,29 +591,42 @@ ConsensusExtensions::buildExplicitFinalProposalTxSet( }); auto const txID = tx.getTransactionID(); - // Dedup by type (mirrors onPreBuild): there must never be two entropy - // pseudo-txs, and a fallback digest derived from a different base set - // hash would not match by exact txID. - bool alreadyPresent = false; + // Value-based dedup (mirrors onPreBuild): there must never be two entropy + // pseudo-txs. If one is already in the base set it must be the EXACT + // pseudo-tx we would have produced (injection is deterministic, so the same + // agreed inputs yield an identical txID); a present-but-different one is a + // determinism violation to surface, not silently accept. Either way return + // the base unchanged — explicit-final is best-effort and must not rewrite + // an already-committed set. + std::optional presentID; txns.map_->visitLeaves( [&](boost::intrusive_ptr const& item) { - if (alreadyPresent) + if (presentID) return; try { SerialIter sit(item->slice()); STTx const parsed{sit}; if (parsed.getTxnType() == ttCONSENSUS_ENTROPY) - alreadyPresent = true; + presentID = parsed.getTransactionID(); } catch (...) { } }); - if (alreadyPresent) + if (presentID) { - JLOG(j_.debug()) << "RNGFINAL: entropy pseudo-tx already in base txSet" - << " txHash=" << txID << " baseTxSet=" << txns.id(); + if (*presentID == txID) + JLOG(j_.debug()) + << "RNGFINAL: entropy pseudo-tx already in base txSet" + << " txHash=" << txID << " baseTxSet=" << txns.id() + << " action=skip-duplicate-verified"; + else + JLOG(j_.error()) + << "RNGFINAL: entropy pseudo-tx MISMATCH in base txSet" + << " reason=determinism-violation action=keep-base" + << " ourTxHash=" << txID << " presentTxHash=" << *presentID + << " baseTxSet=" << txns.id(); return txns; } diff --git a/src/xrpld/app/consensus/ConsensusExtensions.h b/src/xrpld/app/consensus/ConsensusExtensions.h index fd2258567..5a37bf25f 100644 --- a/src/xrpld/app/consensus/ConsensusExtensions.h +++ b/src/xrpld/app/consensus/ConsensusExtensions.h @@ -189,8 +189,12 @@ public: bool hasAnyReveals() const; + /// True when the agreed entropy set would NOT reach validator_quorum + /// (tier 3): absent/failed, or fewer leaves than quorumThreshold(). A + /// tier-3 eligibility predicate only — it does NOT account for the tier-2 + /// floor, so never gate injection on it (selectEntropy() does the tiering). bool - shouldZeroEntropy() const; + belowValidatorQuorum() const; /// Result of the shared deterministic entropy selector: the digest to /// inject plus its tier/count labels. Both injection paths derive these