move comment to original

src/test/consensus/UNLReport_test.cpp

@@ -1203,6 +1203,96 @@ class UNLReportVoteNewValidator_test : public beast::unit_test::suite
     }
 };
 
+/*
+ * Regression test: UNLReport proposals must not be gated on the N-UNL local
+ * reliability threshold.
+ *
+ * The local node (validator 0) validates exactly `myVals` of the 256 scored
+ * ledgers, validator 1 validates none (so it is an N-UNL disable candidate and
+ * absent from the report), every other validator validates all of them.
+ *
+ * == myVals <= 128: not active ourselves -> no UNLReport, no UNLModify
+ * == 129 .. 229:    UNLReport proposed, not reliable enough to vote on N-UNL
+ * == >= 230:        UNLReport and one UNLModify (230 used to fall into the
+ *                   "Too many!" branch and fail)
+ */
+class UNLReportVoteLocalReliability_test : public beast::unit_test::suite
+{
+    void
+    testDoVoting()
+    {
+        testcase("Do Voting with local reliability below N-UNL threshold");
+
+        struct Case
+        {
+            std::uint32_t myVals;
+            bool expectReport;
+            bool expectModify;
+        };
+
+        std::array<Case, 6> const cases = {{
+            {128, false, false},
+            {129, true, false},
+            {229, true, false},
+            {230, true, true},
+            {231, true, true},
+            {256, true, true},
+        }};
+
+        for (bool const withVLImport : {true, false})
+        {
+            for (auto const& c : cases)
+            {
+                std::uint32_t const numNodes = 20;
+                URNetworkHistory history = {
+                    *this, {numNodes, 0, false, false, withVLImport, {}}};
+                BEAST_EXPECT(history.goodHistory);
+                if (!history.goodHistory)
+                    continue;
+
+                // scored ledgers are [lastSeq - 256, lastSeq - 1]
+                auto const lastSeq = history.lastLedger()->seq();
+                history.walkHistoryAndAddValidations(
+                    [&](std::shared_ptr<Ledger const> const& l,
+                        std::size_t idx) -> bool {
+                        if (idx == 1)
+                            return false;
+                        if (idx != 0)
+                            return true;
+                        return l->seq() + 256 < lastSeq + c.myVals;
+                    });
+
+                NegativeUNLVote vote(
+                    history.UNLNodeIDs[0],
+                    history.env.journal,
+                    history.env.app());
+                auto txSet = std::make_shared<SHAMap>(
+                    SHAMapType::TRANSACTION, history.env.app().getNodeFamily());
+                vote.doVoting(
+                    history.lastLedger(),
+                    history.UNLKeySet,
+                    history.validations,
+                    txSet);
+
+                std::size_t const expectReport =
+                    c.expectReport ? numNodes - 1 + (withVLImport ? 1 : 0) : 0;
+                std::size_t const expectModify = c.expectModify ? 1 : 0;
+                std::string const ctx = "myVals=" + std::to_string(c.myVals) +
+                    " withVLImport=" + std::to_string(withVLImport);
+
+                BEAST_EXPECTS(countUNLRTx(txSet) == expectReport, ctx);
+                BEAST_EXPECTS(unl::countTx(txSet) == expectModify, ctx);
+            }
+        }
+    }
+
+    void
+    run() override
+    {
+        testDoVoting();
+    }
+};
+
 BEAST_DEFINE_TESTSUITE(UNLReport, ledger, ripple);
 BEAST_DEFINE_TESTSUITE(UNLReportNoAmendment, ledger, ripple);
 BEAST_DEFINE_TESTSUITE(UNLReportFork, consensus, ripple);
@@ -1215,6 +1305,7 @@ BEAST_DEFINE_TESTSUITE_PRIO(
     ripple,
     1);
 BEAST_DEFINE_TESTSUITE(UNLReportVoteNewValidator, consensus, ripple);
+BEAST_DEFINE_TESTSUITE(UNLReportVoteLocalReliability, consensus, ripple);
 
 ///////////////////////////////////////////////////////////////////////
 ///////////////////////////////////////////////////////////////////////

src/xrpld/app/consensus/RCLConsensus.cpp

@@ -348,6 +348,17 @@ RCLConsensus::Adaptor::onClose(
             make_shamapitem(tx.first->getTransactionID(), s.slice()));
     }
 
+    // Keep enough validation history for the next flag ledger's score table
+    // even when we don't vote this round (not proposing / wrong LCL).
+    // Otherwise toKeep_ goes stale, validations older than
+    // validationSET_EXPIRES (10 min, < 256 ledgers) get swept, and we fail
+    // the next flag's score table too.
+    if (prevLedger->isVotingLedger())
+    {
+        auto const seq = prevLedger->info().seq + 1;
+        app_.getValidations().setSeqToKeep(seq - 1, seq + FLAG_LEDGER_INTERVAL);
+    }
+
     // Add pseudo-transactions to the set
     if (app_.config().standalone() || (proposing && !wrongLCL))
     {

src/xrpld/app/misc/NegativeUNLVote.cpp

@@ -58,9 +58,19 @@ NegativeUNLVote::doVoting(
         unlNodeIDs.emplace(nid);
     }
 
-    // Build a reliability score table of validators
-    if (std::optional<hash_map<NodeID, std::uint32_t>> scoreTable =
-            buildScoreTable(prevLedger, unlNodeIDs, validations))
+    // Build a reliability score table of validators. This only fails if
+    // there isn't enough ledger history; it does NOT apply the local
+    // reliability gate (that is N-UNL specific, see below).
+    auto const scoreTable =
+        buildRawScoreTable(prevLedger, unlNodeIDs, validations);
+    if (!scoreTable)
+        return;
+
+    auto const seq = prevLedger->info().seq + 1;
+
+    // N-UNL voting: only vote to disable/re-enable others if we ourselves
+    // have been reliable over the last flag ledger interval.
+    if (localNodeReliableForNUNL(*scoreTable, seq))
     {
         // build next negUnl
         auto negUnlKeys = prevLedger->negativeUNL();
@@ -82,7 +92,6 @@ NegativeUNLVote::doVoting(
             }
         }
 
-        auto const seq = prevLedger->info().seq + 1;
         purgeNewValidators(seq);
 
         // Process the table and find all candidates to disable or to re-enable
@@ -109,14 +118,33 @@ NegativeUNLVote::doVoting(
                 "ripple::NegativeUNLVote::doVoting : found node to enable");
             addTx(seq, nidToKeyMap.at(n), ToReEnable, initialSet);
         }
+    }
 
-        // do reporting when enabled
-        if (prevLedger->rules().enabled(featureXahauGenesis) &&
-            scoreTable->size() > 0)
+    // UNLReport. Previously this sat behind the N-UNL local reliability gate
+    // (>= 90% of our own validations on-chain in the last 256 ledgers). Every
+    // ttUNL_REPORT is its own deterministic txn and is voted on individually
+    // in consensus, so one proposer's skewed view can't change the outcome;
+    // but gating the whole batch per-proposer made the reports all-or-nothing:
+    // whenever fewer than ~half the UNL cleared the 90% bar the entire report
+    // lost the dispute and the flag ledger carried no UNLReport at all.
+    // Use the same bar the report itself uses for "active".
+    if (prevLedger->rules().enabled(featureXahauGenesis) &&
+        !scoreTable->empty())
+    {
+        auto const it = scoreTable->find(myId_);
+        auto const myScore = it == scoreTable->end() ? 0u : it->second;
+        if (myScore > (FLAG_LEDGER_INTERVAL >> 1))
         {
             addReportingTx(seq, *scoreTable, nidToKeyMap, initialSet);
             addImportVLTx(seq, initialSet);
         }
+        else
+        {
+            JLOG(j_.debug())
+                << "R-UNL: ledger " << seq << ". Local node only issued "
+                << myScore << " validations in last " << FLAG_LEDGER_INTERVAL
+                << " ledgers, not proposing UNLReport.";
+        }
     }
 }
 
@@ -277,7 +305,7 @@ NegativeUNLVote::choose(
 }
 
 std::optional<hash_map<NodeID, std::uint32_t>>
-NegativeUNLVote::buildScoreTable(
+NegativeUNLVote::buildRawScoreTable(
     std::shared_ptr<Ledger const> const& prevLedger,
     hash_set<NodeID> const& unl,
     RCLValidations& validations)
@@ -327,6 +355,14 @@ NegativeUNLVote::buildScoreTable(
         }
     }
 
+    return scoreTable;
+}
+
+bool
+NegativeUNLVote::localNodeReliableForNUNL(
+    hash_map<NodeID, std::uint32_t> const& scoreTable,
+    LedgerIndex seq) const
+{
     // Return false if the validation message history or local node's
     // participation in the history is not good.
     auto const myValidationCount = [&]() -> std::uint32_t {
@@ -341,23 +377,33 @@ NegativeUNLVote::buildScoreTable(
                          << " validations in last " << FLAG_LEDGER_INTERVAL
                          << " ledgers."
                          << " The reliability measurement could be wrong.";
-        return {};
+        return false;
     }
-    else if (
-        myValidationCount > negativeUNLMinLocalValsToVote &&
-        myValidationCount <= FLAG_LEDGER_INTERVAL)
-    {
-        return scoreTable;
-    }
-    else
+    if (myValidationCount > FLAG_LEDGER_INTERVAL)
     {
         // cannot happen because validations.getTrustedForLedger does not
         // return multiple validations of the same ledger from a validator.
         JLOG(j_.error()) << "N-UNL: ledger " << seq << ". Local node issued "
                          << myValidationCount << " validations in last "
                          << FLAG_LEDGER_INTERVAL << " ledgers. Too many!";
-        return {};
+        return false;
     }
+    // NB: the old code used `>` here, so exactly 230 fell through to the
+    // "Too many!" branch.
+    return true;
+}
+
+std::optional<hash_map<NodeID, std::uint32_t>>
+NegativeUNLVote::buildScoreTable(
+    std::shared_ptr<Ledger const> const& prevLedger,
+    hash_set<NodeID> const& unl,
+    RCLValidations& validations)
+{
+    auto scoreTable = buildRawScoreTable(prevLedger, unl, validations);
+    if (scoreTable &&
+        !localNodeReliableForNUNL(*scoreTable, prevLedger->info().seq + 1))
+        return {};
+    return scoreTable;
 }
 
 NegativeUNLVote::Candidates const

src/xrpld/app/misc/NegativeUNLVote.h

@@ -192,6 +192,26 @@ private:
     NodeID
     choose(uint256 const& randomPadData, std::vector<NodeID> const& candidates);
 
+    /**
+     * Count trusted full validations per UNL validator over the last
+     * FLAG_LEDGER_INTERVAL ledgers. Fails only on insufficient ledger
+     * history; does not apply the local reliability gate.
+     */
+    std::optional<hash_map<NodeID, std::uint32_t>>
+    buildRawScoreTable(
+        std::shared_ptr<Ledger const> const& prevLedger,
+        hash_set<NodeID> const& unl,
+        RCLValidations& validations);
+
+    /**
+     * True if the local node issued enough validations in the score table
+     * to be trusted to vote on N-UNL changes.
+     */
+    bool
+    localNodeReliableForNUNL(
+        hash_map<NodeID, std::uint32_t> const& scoreTable,
+        LedgerIndex seq) const;
+
     /**
      * Build a reliability measurement score table of validators' validation
      * messages in the last flag ledger period.