From 9f6e7dd3156d30966da2e0d890d854b2e8d7a0f7 Mon Sep 17 00:00:00 2001 From: Nicholas Dudfield Date: Wed, 24 Jun 2026 13:10:57 +0700 Subject: [PATCH] refactor(consensus): make extended position identity explicit --- src/test/consensus/ExtendedPosition_test.cpp | 45 ++++++++-------- src/test/csf/Proposal.h | 41 -------------- src/xrpld/app/consensus/RCLCxPeerPos.h | 53 +++---------------- src/xrpld/consensus/Consensus.h | 19 ++++--- src/xrpld/consensus/ConsensusExtensionsTick.h | 8 +-- src/xrpld/consensus/ConsensusProposal.h | 13 ++++- src/xrpld/consensus/ConsensusTypes.h | 2 +- 7 files changed, 57 insertions(+), 124 deletions(-) diff --git a/src/test/consensus/ExtendedPosition_test.cpp b/src/test/consensus/ExtendedPosition_test.cpp index 06434c23c..c5bd3f923 100644 --- a/src/test/consensus/ExtendedPosition_test.cpp +++ b/src/test/consensus/ExtendedPosition_test.cpp @@ -572,9 +572,9 @@ class ExtendedPosition_test : public beast::unit_test::suite } void - testEquality() + testTxSetIdentity() { - testcase("Equality is txSetHash only"); + testcase("Tx-set identity is explicit"); auto const txSet = makeHash("txset-eq"); auto const txSet2 = makeHash("txset-eq-2"); @@ -585,40 +585,36 @@ class ExtendedPosition_test : public beast::unit_test::suite ExtendedPosition b{txSet}; b.myCommitment = makeHash("commit2-eq"); - // Same txSetHash, different leaves -> equal - BEAST_EXPECT(a == b); + // Same txSetHash, different leaves -> same consensus tx-set key. + BEAST_EXPECT(positionTxSetID(a) == positionTxSetID(b)); - // Same txSetHash, different commitSetHash -> still equal + // Same txSetHash, different commitSetHash -> same tx-set key // (sub-state quorum handles commitSetHash agreement) b.commitSetHash = makeHash("cs-eq"); - BEAST_EXPECT(a == b); + BEAST_EXPECT(positionTxSetID(a) == positionTxSetID(b)); - // Same txSetHash, different entropySetHash -> still equal + // Same txSetHash, different entropySetHash -> same tx-set key b.entropySetHash = makeHash("es-eq"); - BEAST_EXPECT(a == b); + BEAST_EXPECT(positionTxSetID(a) == positionTxSetID(b)); - // Same txSetHash, different export signature digest -> still equal + // Same txSetHash, different export signature digest -> same tx-set key b.exportSignaturesHash = makeHash("export-sigs-eq"); - BEAST_EXPECT(a == b); + BEAST_EXPECT(positionTxSetID(a) == positionTxSetID(b)); - // Same txSetHash, different participant diagnostics -> still equal + // Same txSetHash, different participant diagnostics -> same tx-set key b.observedParticipantsHash = makeHash("participants-eq"); - BEAST_EXPECT(a == b); + BEAST_EXPECT(positionTxSetID(a) == positionTxSetID(b)); - // Different txSetHash -> not equal + // Different txSetHash -> different tx-set key ExtendedPosition c{txSet2}; - BEAST_EXPECT(a != c); + BEAST_EXPECT(positionTxSetID(a) != positionTxSetID(c)); - BEAST_EXPECT(a == txSet); - BEAST_EXPECT(txSet == a); - BEAST_EXPECT(!(a != txSet)); - BEAST_EXPECT(!(txSet != a)); - BEAST_EXPECT(a != txSet2); - BEAST_EXPECT(txSet2 != a); + BEAST_EXPECT(positionTxSetID(a) == txSet); + BEAST_EXPECT(positionTxSetID(a) != txSet2); a.updateTxSet(txSet2); - BEAST_EXPECT(a == txSet2); - BEAST_EXPECT(a != b); + BEAST_EXPECT(positionTxSetID(a) == txSet2); + BEAST_EXPECT(positionTxSetID(a) != positionTxSetID(b)); } void @@ -686,7 +682,8 @@ class ExtendedPosition_test : public beast::unit_test::suite auto sameTxDifferentSidecar = pos; sameTxDifferentSidecar.entropySetHash = makeHash("entropyset-other"); - BEAST_EXPECT(pos == sameTxDifferentSidecar); + BEAST_EXPECT( + positionTxSetID(pos) == positionTxSetID(sameTxDifferentSidecar)); BEAST_EXPECT(sha512Half(pos) != sha512Half(sameTxDifferentSidecar)); } @@ -699,7 +696,7 @@ public: testSuppressionConsistency(); testPeerPosition(); testMalformedPayload(); - testEquality(); + testTxSetIdentity(); testExportSignatureDigest(); testStringJsonAndHash(); } diff --git a/src/test/csf/Proposal.h b/src/test/csf/Proposal.h index f8e215179..cd137e937 100644 --- a/src/test/csf/Proposal.h +++ b/src/test/csf/Proposal.h @@ -53,54 +53,13 @@ struct RngPosition { } - operator TxSet::ID() const - { - return txSetHash; - } - void updateTxSet(TxSet::ID txSet) { txSetHash = txSet; } - - bool - operator==(RngPosition const& other) const - { - return txSetHash == other.txSetHash; - } - - bool - operator!=(RngPosition const& other) const - { - return !(*this == other); - } - - bool - operator==(TxSet::ID txSet) const - { - return txSetHash == txSet; - } - - bool - operator!=(TxSet::ID txSet) const - { - return txSetHash != txSet; - } }; -inline bool -operator==(TxSet::ID txSet, RngPosition const& pos) -{ - return pos == txSet; -} - -inline bool -operator!=(TxSet::ID txSet, RngPosition const& pos) -{ - return pos != txSet; -} - inline std::string to_string(RngPosition const& pos) { diff --git a/src/xrpld/app/consensus/RCLCxPeerPos.h b/src/xrpld/app/consensus/RCLCxPeerPos.h index 6c4a1e143..185033814 100644 --- a/src/xrpld/app/consensus/RCLCxPeerPos.h +++ b/src/xrpld/app/consensus/RCLCxPeerPos.h @@ -42,11 +42,11 @@ namespace ripple { /** Extended position for consensus with RNG entropy support. Carries the tx-set hash (the core convergence target), RNG set hashes - (agreed via sub-state quorum, not via operator==), and per-validator + (agreed via sub-state quorum), and per-validator leaves (unique to each proposer, piggybacked on proposals). Critical design: - - operator== compares txSetHash ONLY (sub-states handle the rest) + - consensus convergence compares txSetHash explicitly - add() includes ALL fields for signing (prevents stripping attacks) */ struct ExtendedPosition @@ -71,12 +71,6 @@ struct ExtendedPosition { } - // Implicit conversion for legacy compatibility - operator uint256() const - { - return txSetHash; - } - // Helper to update TxSet while preserving sidecar data void updateTxSet(uint256 const& set) @@ -84,7 +78,7 @@ struct ExtendedPosition txSetHash = set; } - // CRITICAL: Only compare txSetHash for consensus convergence. + // CRITICAL: consensus convergence compares txSetHash only. // // Why not commitSetHash / entropySetHash? // Nodes transition through sub-states (ConvergingTx → ConvergingCommit @@ -107,42 +101,11 @@ struct ExtendedPosition // - Leaves (myCommitment, myReveal) are also excluded — they are // per-validator data unique to each proposer. //@@start rng-extended-position-equality - bool - operator==(ExtendedPosition const& other) const - { - return txSetHash == other.txSetHash; - } - - bool - operator!=(ExtendedPosition const& other) const - { - return !(*this == other); - } - - // Comparison with uint256 (compares txSetHash only) - bool - operator==(uint256 const& hash) const - { - return txSetHash == hash; - } - - bool - operator!=(uint256 const& hash) const - { - return txSetHash != hash; - } - - friend bool - operator==(uint256 const& hash, ExtendedPosition const& pos) - { - return pos.txSetHash == hash; - } - - friend bool - operator!=(uint256 const& hash, ExtendedPosition const& pos) - { - return pos.txSetHash != hash; - } + // No operator== and no implicit uint256 conversion on purpose: comparing + // an ExtendedPosition as if it were a whole value is misleading because + // consensus convergence intentionally ignores sidecar hashes and leaves. + // Callers that need tx-set identity compare txSetHash explicitly, or use + // the generic positionTxSetID(position) helper. //@@end rng-extended-position-equality // CRITICAL: Include ALL fields for signing (prevents stripping attacks) diff --git a/src/xrpld/consensus/Consensus.h b/src/xrpld/consensus/Consensus.h index f77e71987..eac4d53a8 100644 --- a/src/xrpld/consensus/Consensus.h +++ b/src/xrpld/consensus/Consensus.h @@ -931,13 +931,14 @@ Consensus::peerProposalInternal( << "/" << newPeerProp.position(); { - auto const ait = acquired_.find(newPeerProp.position()); + auto const txSetID = positionTxSetID(newPeerProp.position()); + auto const ait = acquired_.find(txSetID); if (ait == acquired_.end()) { // acquireTxSet will return the set if it is available, or // spawn a request for it and return nullopt/nullptr. It will call // gotTxSet once it arrives - if (auto set = adaptor_.acquireTxSet(newPeerProp.position())) + if (auto set = adaptor_.acquireTxSet(txSetID)) gotTxSet(now_, *set); else JLOG(j_.debug()) << "Don't have tx set for peer"; @@ -1012,12 +1013,12 @@ Consensus::gotTxSet( // Our position is added to acquired_ as soon as we create it, // so this txSet must differ XRPL_ASSERT( - id != result_->position.position(), + id != positionTxSetID(result_->position.position()), "ripple::Consensus::gotTxSet : updated transaction set"); bool any = false; for (auto const& [nodeId, peerPos] : currPeerPositions_) { - if (peerPos.proposal().position() == id) + if (positionTxSetID(peerPos.proposal().position()) == id) { updateDisputes(nodeId, txSet); any = true; @@ -1730,7 +1731,7 @@ Consensus::closeLedger(std::unique_ptr const& clog) for (auto const& pit : currPeerPositions_) { auto const& pos = pit.second.proposal().position(); - auto const it = acquired_.find(pos); + auto const it = acquired_.find(positionTxSetID(pos)); if (it != acquired_.end()) createDisputes(it->second, clog); } @@ -1948,7 +1949,7 @@ Consensus::updateOurPositions( for (auto const& [nodeId, peerPos] : currPeerPositions_) { Proposal_t const& p = peerPos.proposal(); - if (p.position() == newID) + if (positionTxSetID(p.position()) == newID) updateDisputes(nodeId, result_->txns); } } @@ -1977,7 +1978,8 @@ Consensus::haveConsensus( for (auto const& [nodeId, peerPos] : currPeerPositions_) { Proposal_t const& peerProp = peerPos.proposal(); - if (peerProp.position() == ourPosition) + if (positionTxSetID(peerProp.position()) == + positionTxSetID(ourPosition)) { ++agree; } @@ -2209,7 +2211,8 @@ Consensus::createDisputes( for (auto const& [nodeId, peerPos] : currPeerPositions_) { Proposal_t const& peerProp = peerPos.proposal(); - auto const cit = acquired_.find(peerProp.position()); + auto const cit = + acquired_.find(positionTxSetID(peerProp.position())); if (cit != acquired_.end() && dtx.setVote(nodeId, cit->second.exists(txID))) peerUnchangedCounter_ = 0; diff --git a/src/xrpld/consensus/ConsensusExtensionsTick.h b/src/xrpld/consensus/ConsensusExtensionsTick.h index 26e99ba1c..bfa2a35cd 100644 --- a/src/xrpld/consensus/ConsensusExtensionsTick.h +++ b/src/xrpld/consensus/ConsensusExtensionsTick.h @@ -73,7 +73,7 @@ inspectTxConvergedSidecarPeers( continue; // outside the active view -> not in the counting // universe auto const& pp = peerPos.proposal().position(); - if (!(pp == pos)) + if (positionTxSetID(pp) != positionTxSetID(pos)) continue; // not tx-converged ++state.txConverged; @@ -445,7 +445,7 @@ extensionsTick(Ext& ext, Ctx const& ctx) for (auto const& [nodeId, peerPos] : ctx.peerPositions) { auto const& peerPosition = peerPos.proposal().position(); - if (!(peerPosition == ourPos)) + if (peerPosition.txSetHash != ourPos.txSetHash) continue; ext.fetchRngSetIfNeeded( peerPosition.commitSetHash, Ext::SidecarKind::commit); @@ -483,7 +483,7 @@ extensionsTick(Ext& ext, Ctx const& ctx) for (auto const& [nodeId, peerPos] : ctx.peerPositions) { auto const& peerPosition = peerPos.proposal().position(); - if (!(peerPosition == ourPos)) + if (peerPosition.txSetHash != ourPos.txSetHash) continue; if (note(peerPosition)) return true; @@ -939,7 +939,7 @@ extensionsTick(Ext& ext, Ctx const& ctx) for (auto const& [_, peerPos] : ctx.peerPositions) { auto const& pp = peerPos.proposal().position(); - if (!(pp == pos)) + if (positionTxSetID(pp) != positionTxSetID(pos)) continue; // not tx-converged if (!pp.exportSigSetHash) continue; diff --git a/src/xrpld/consensus/ConsensusProposal.h b/src/xrpld/consensus/ConsensusProposal.h index 18dcf6000..31f497838 100644 --- a/src/xrpld/consensus/ConsensusProposal.h +++ b/src/xrpld/consensus/ConsensusProposal.h @@ -29,6 +29,16 @@ #include namespace ripple { +template +auto const& +positionTxSetID(Position_t const& position) +{ + if constexpr (requires { position.txSetHash; }) + return position.txSetHash; + else + return position; +} + /** Represents a proposed position taken during a round of consensus. During consensus, peers seek agreement on a set of transactions to @@ -275,7 +285,8 @@ operator==( ConsensusProposal const& b) { return a.nodeID() == b.nodeID() && a.proposeSeq() == b.proposeSeq() && - a.prevLedger() == b.prevLedger() && a.position() == b.position() && + a.prevLedger() == b.prevLedger() && + positionTxSetID(a.position()) == positionTxSetID(b.position()) && a.closeTime() == b.closeTime() && a.seenTime() == b.seenTime(); } } // namespace ripple diff --git a/src/xrpld/consensus/ConsensusTypes.h b/src/xrpld/consensus/ConsensusTypes.h index 18a750249..023fa8a4c 100644 --- a/src/xrpld/consensus/ConsensusTypes.h +++ b/src/xrpld/consensus/ConsensusTypes.h @@ -272,7 +272,7 @@ struct ConsensusResult : txns{std::move(s)}, position{std::move(p)} { XRPL_ASSERT( - txns.id() == position.position(), + txns.id() == positionTxSetID(position.position()), "ripple::ConsensusResult : valid inputs"); }