diff --git a/src/test/overlay/tx_reduce_relay_test.cpp b/src/test/overlay/tx_reduce_relay_test.cpp index ebec371244..1eabe7c85c 100644 --- a/src/test/overlay/tx_reduce_relay_test.cpp +++ b/src/test/overlay/tx_reduce_relay_test.cpp @@ -121,6 +121,7 @@ private: void run() override { + run_++; } void send(std::shared_ptr const&) override @@ -137,11 +138,13 @@ private: { queueTx_ = 0; sendTx_ = 0; + run_ = 0; sid_ = 0; } inline static std::size_t sid_ = 0; inline static std::uint16_t queueTx_ = 0; inline static std::uint16_t sendTx_ = 0; + inline static std::uint16_t run_ = 0; }; std::uint16_t lid_{0}; @@ -246,6 +249,10 @@ private: testcase("required protocol feature gate"); jtx::Env env(*this); auto& overlay = dynamic_cast(env.app().overlay()); + std::vector> peers; + PeerTest::init(); + lid_ = 0; + rid_ = 1; boost::beast::http::fields legacy; boost::beast::http::fields capable; @@ -256,17 +263,42 @@ private: BEAST_EXPECT(!overlay.isProtocolFeatureRequired( ProtocolFeature::ConsensusEntropy)); BEAST_EXPECT(!overlay.missingRequiredProtocolFeatureInHandshake( - legacy)); + legacy, make_protocol(2, 2))); + + // Leave a legacy peer in the active map while eviction is posted to its + // strand. Proposal filtering must take effect synchronously with the + // atomic requirement, without waiting for close(). + std::uint16_t disabled = 1; + addPeer(env, peers, disabled); + BEAST_EXPECT(PeerTest::run_ == 1); overlay.requireProtocolFeature(ProtocolFeature::ConsensusEntropy); BEAST_EXPECT(overlay.isProtocolFeatureRequired( ProtocolFeature::ConsensusEntropy)); BEAST_EXPECT( - overlay.missingRequiredProtocolFeatureInHandshake(legacy) == + overlay.missingRequiredProtocolFeatureInHandshake( + legacy, make_protocol(2, 2)) == ProtocolFeature::ConsensusEntropy); BEAST_EXPECT(!overlay.missingRequiredProtocolFeatureInHandshake( - capable)); + capable, make_protocol(2, 2))); + + protocol::TMProposeSet proposal; + proposal.set_proposeseq(1); + overlay.broadcast(proposal); + BEAST_EXPECT(PeerTest::sendTx_ == 0); + + // A capable peer admitted after the cutoff still runs and receives + // proposals; a later legacy peer is inserted only for lifecycle cleanup + // and is failed before run(). + disabled = 0; + addPeer(env, peers, disabled); + BEAST_EXPECT(PeerTest::run_ == 2); + overlay.broadcast(proposal); + BEAST_EXPECT(PeerTest::sendTx_ == 1); + disabled = 1; + addPeer(env, peers, disabled); + BEAST_EXPECT(PeerTest::run_ == 2); // Requiring an already-required feature is intentionally idempotent. overlay.requireProtocolFeature(ProtocolFeature::ConsensusEntropy); @@ -274,6 +306,45 @@ private: ProtocolFeature::ConsensusEntropy)); } + void + testProtocolFeatureGateFromLedger() + { + testcase("active ledger installs protocol feature gate"); + auto config = jtx::envconfig(); + config->features.insert(featureConsensusEntropy); + jtx::Env env(*this, std::move(config)); + auto& overlay = dynamic_cast(env.app().overlay()); + BEAST_EXPECT(overlay.isProtocolFeatureRequired( + ProtocolFeature::ConsensusEntropy)); + } + + void + testGenericProtocolFeatureAdmission() + { + testcase("generic protocol feature admission"); + jtx::Env env(*this); + auto& overlay = dynamic_cast(env.app().overlay()); + boost::beast::http::fields headers; + + overlay.requireProtocolFeature( + ProtocolFeature::ValidatorList2Propagation); + BEAST_EXPECT( + overlay.missingRequiredProtocolFeatureInHandshake( + headers, make_protocol(2, 1)) == + ProtocolFeature::ValidatorList2Propagation); + BEAST_EXPECT(!overlay.missingRequiredProtocolFeatureInHandshake( + headers, make_protocol(2, 2))); + + overlay.requireProtocolFeature(ProtocolFeature::LedgerReplay); + BEAST_EXPECT( + overlay.missingRequiredProtocolFeatureInHandshake( + headers, make_protocol(2, 2)) == + ProtocolFeature::LedgerReplay); + headers.set("X-Protocol-Ctl", "ledgerreplay=1;"); + BEAST_EXPECT(!overlay.missingRequiredProtocolFeatureInHandshake( + headers, make_protocol(2, 2))); + } + void run() override { @@ -281,6 +352,8 @@ private: std::set skip = {0, 1, 2, 3, 4}; testConfig(log); testProtocolFeatureGate(); + testProtocolFeatureGateFromLedger(); + testGenericProtocolFeatureAdmission(); // relay to all peers, no hash queue testRelay("feature disabled", false, 10, 0, 10, 25, 10, 0); // relay to nPeers - skip (10-5=5) diff --git a/src/xrpld/app/consensus/RCLConsensus.cpp b/src/xrpld/app/consensus/RCLConsensus.cpp index 5bb0569fe2..36bf527c1a 100644 --- a/src/xrpld/app/consensus/RCLConsensus.cpp +++ b/src/xrpld/app/consensus/RCLConsensus.cpp @@ -671,6 +671,14 @@ RCLConsensus::Adaptor::doAccept( auto const newLCLHash = built.id(); JLOG(j_.debug()) << "Built ledger #" << built.seq() << ": " << newLCLHash; + // Once the accepted result has built the enable-amendment ledger, the + // protocol cutoff is a fait accompli. Publish it before status notification + // or any remaining accept work; beginConsensus repeats this idempotently to + // cover startup and catch-up from an already-enabled ledger. + if (built.ledger_->rules().enabled(featureConsensusEntropy)) + app_.overlay().requireProtocolFeature( + ProtocolFeature::ConsensusEntropy); + // Tell directly connected peers that we have a new LCL notify(protocol::neACCEPTED_LEDGER, built, haveCorrectLCL); diff --git a/src/xrpld/overlay/Peer.h b/src/xrpld/overlay/Peer.h index 46559b2ca8..aae0f9356f 100644 --- a/src/xrpld/overlay/Peer.h +++ b/src/xrpld/overlay/Peer.h @@ -26,6 +26,7 @@ #include #include +#include #include namespace ripple { @@ -41,6 +42,13 @@ enum class ProtocolFeature { ConsensusEntropy, }; +inline constexpr std::array allProtocolFeatures = { + ProtocolFeature::ValidatorListPropagation, + ProtocolFeature::ValidatorList2Propagation, + ProtocolFeature::LedgerReplay, + ProtocolFeature::ConsensusEntropy, +}; + /** Stable diagnostic name for a negotiated peer-protocol feature. */ constexpr std::string_view protocolFeatureName(ProtocolFeature feature) diff --git a/src/xrpld/overlay/ProtocolFeatureRequirements.md b/src/xrpld/overlay/ProtocolFeatureRequirements.md index f310a1e1b6..236b911f64 100644 --- a/src/xrpld/overlay/ProtocolFeatureRequirements.md +++ b/src/xrpld/overlay/ProtocolFeatureRequirements.md @@ -39,23 +39,26 @@ For the observed rollout, the relevant ledgers are: | 257 | contains `EnableAmendment`; its resulting rules enable CE | install the required-capability gate | | 258 | first consensus round built with CE active | only capable sessions may participate | -`NetworkOPsImp::beginConsensus` is the boundary. Its `prevLedger` is the newly -closed ledger whose rules govern the round about to start. If those rules -enable Consensus Entropy, it calls: +As soon as the accepted consensus result has built ledger 257, `doAccept` +installs the requirement before status notification and the remainder of the +accept work. It does not wait for ledger 257 to become fully validated or for +the next round to start: ```cpp app.overlay().requireProtocolFeature( ProtocolFeature::ConsensusEntropy); ``` -This happens before `RCLConsensus::startRound`. Thus accepting ledger 257 -disconnects incompatible sessions before any ledger-258 proposal is created or -relayed. Local `VOTE`/`VETO` state is not used: a validator that locally vetoed -the amendment must still obey the rules of the ledger the network accepted. +`NetworkOPsImp::beginConsensus` repeats the same idempotent call from its +`prevLedger`, the newly closed ledger whose rules govern the round about to +start. Thus accepting ledger 257 disconnects incompatible sessions before any +ledger-258 proposal is created or relayed. Local `VOTE`/`VETO` state is not +used: a validator that locally vetoed the amendment must still obey the rules +of the ledger the network accepted. -The same path runs for the first consensus round after a restart or catch-up, -so a node starting from an already-enabled ledger installs the requirement -before joining consensus. +The `beginConsensus` path also runs for the first round after a restart or +catch-up, so a node starting from an already-enabled ledger installs the +requirement before joining consensus. ## Session behavior diff --git a/src/xrpld/overlay/detail/ConnectAttempt.cpp b/src/xrpld/overlay/detail/ConnectAttempt.cpp index e8162c33dc..30bb22c75f 100644 --- a/src/xrpld/overlay/detail/ConnectAttempt.cpp +++ b/src/xrpld/overlay/detail/ConnectAttempt.cpp @@ -364,7 +364,8 @@ ConnectAttempt::processResponse() app_); if (auto const missing = - overlay_.missingRequiredProtocolFeatureInHandshake(response_)) + overlay_.missingRequiredProtocolFeatureInHandshake( + response_, *negotiatedProtocol)) return fail( "Handshake missing required protocol feature " + std::string(protocolFeatureName(*missing))); diff --git a/src/xrpld/overlay/detail/OverlayImpl.cpp b/src/xrpld/overlay/detail/OverlayImpl.cpp index 490063dc57..c5b7275c07 100644 --- a/src/xrpld/overlay/detail/OverlayImpl.cpp +++ b/src/xrpld/overlay/detail/OverlayImpl.cpp @@ -273,7 +273,8 @@ OverlayImpl::onHandoff( app_); if (auto const missing = - missingRequiredProtocolFeatureInHandshake(request)) + missingRequiredProtocolFeatureInHandshake( + request, *negotiatedVersion)) throw std::runtime_error( "Handshake missing required protocol feature " + std::string(protocolFeatureName(*missing))); @@ -632,6 +633,9 @@ OverlayImpl::activate(std::shared_ptr const& peer) << "Rejected active peer missing required protocol feature " << protocolFeatureName(*missing) << " from " << peer->getRemoteAddress(); + peer->fail( + "Missing required protocol feature " + + std::string(protocolFeatureName(*missing))); return false; } @@ -1196,9 +1200,10 @@ OverlayImpl::isProtocolFeatureRequired(ProtocolFeature feature) const std::optional OverlayImpl::missingRequiredProtocolFeature(Peer const& peer) const { - if (isProtocolFeatureRequired(ProtocolFeature::ConsensusEntropy) && - !peer.supportsFeature(ProtocolFeature::ConsensusEntropy)) - return ProtocolFeature::ConsensusEntropy; + for (auto const feature : allProtocolFeatures) + if (isProtocolFeatureRequired(feature) && + !peer.supportsFeature(feature)) + return feature; return std::nullopt; } @@ -1206,7 +1211,10 @@ void OverlayImpl::broadcast(protocol::TMProposeSet& m) { auto const sm = std::make_shared(m, protocol::mtPROPOSE_LEDGER); - for_each([&](std::shared_ptr&& p) { p->send(sm); }); + for_each([&](std::shared_ptr&& p) { + if (!missingRequiredProtocolFeature(*p)) + p->send(sm); + }); } std::set @@ -1220,7 +1228,8 @@ OverlayImpl::relay( auto const sm = std::make_shared(m, protocol::mtPROPOSE_LEDGER, validator); for_each([&](std::shared_ptr&& p) { - if (toSkip->find(p->id()) == toSkip->end()) + if (toSkip->find(p->id()) == toSkip->end() && + !missingRequiredProtocolFeature(*p)) p->send(sm); }); return *toSkip; diff --git a/src/xrpld/overlay/detail/OverlayImpl.h b/src/xrpld/overlay/detail/OverlayImpl.h index 513f9a86ac..0b9b9e350d 100644 --- a/src/xrpld/overlay/detail/OverlayImpl.h +++ b/src/xrpld/overlay/detail/OverlayImpl.h @@ -279,11 +279,36 @@ public: /** Return the first mandatory feature missing from handshake headers. */ template std::optional - missingRequiredProtocolFeatureInHandshake(Headers const& headers) const + missingRequiredProtocolFeatureInHandshake( + Headers const& headers, + ProtocolVersion const& protocol) const { - if (isProtocolFeatureRequired(ProtocolFeature::ConsensusEntropy) && - !peerFeatureEnabled(headers, FEATURE_CONSENSUS_ENTROPY, true)) - return ProtocolFeature::ConsensusEntropy; + for (auto const feature : allProtocolFeatures) + { + if (!isProtocolFeatureRequired(feature)) + continue; + + bool supported = false; + switch (feature) + { + case ProtocolFeature::ValidatorListPropagation: + supported = protocol >= make_protocol(2, 1); + break; + case ProtocolFeature::ValidatorList2Propagation: + supported = protocol >= make_protocol(2, 2); + break; + case ProtocolFeature::LedgerReplay: + supported = peerFeatureEnabled( + headers, FEATURE_LEDGER_REPLAY, true); + break; + case ProtocolFeature::ConsensusEntropy: + supported = peerFeatureEnabled( + headers, FEATURE_CONSENSUS_ENTROPY, true); + break; + } + if (!supported) + return feature; + } return std::nullopt; } diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index e7f9cdd5cf..24c044b80c 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -866,7 +866,7 @@ PeerImp::doAccept() } if (!overlay_.activate(shared_from_this())) - return fail("Missing required protocol feature"); + return; // XXX Set timer: connection is in grace period to be useful. // XXX Set timer: connection idle (idle may vary depending on connection