CAP-83: Harden previousLedgerHash check in combineCandidates (#5380)

This change moves a correctness check for a transaction set's
`previousLedgerHash` up before calling `prepareForApply` on that
transaction set.

Author: marta-lokhova

src/herder/HerderSCPDriver.cpp

@@ -1066,27 +1066,31 @@ HerderSCPDriver::combineCandidates(uint64_t slotIndex,
             }
             // else: EmptyTxSet -> cTxSet stays null, handled by existing
 
-            // Only valid applicable tx sets should be combined.
-            auto cApplicableTxSet =
-                cTxSet ? cTxSet->prepareForApply(mApp, lcl.header) : nullptr;
-            if (!cTxSet || cTxSet->previousLedgerHash() == lcl.hash)
+            // Prefer applicable tx sets. `compareTxSets` down-ranks
+            // non-applicable ones. Without CAP-0083 every candidate here is
+            // applicable (a mismatched one can't be ratified), so the
+            // non-applicable case doesn't arise. Under CAP-0083 a
+            // non-applicable candidate may be selected and is replaced with an
+            // empty-tx-set value during balloting.
+            ApplicableTxSetFrameConstPtr cApplicableTxSet = nullptr;
+            if (cTxSet && cTxSet->previousLedgerHash() == lcl.hash)
             {
+                cApplicableTxSet = cTxSet->prepareForApply(mApp, lcl.header);
+            }
 
-                if (highest == candidateValues.cend() ||
-                    compareTxSets(
-                        highestApplicableTxSet, cApplicableTxSet,
-                        highest->txSetHash, sv.txSetHash,
-                        highestTxSet
-                            ? std::make_optional(highestTxSet->encodedSize())
-                            : std::nullopt,
-                        cTxSet ? std::make_optional(cTxSet->encodedSize())
-                               : std::nullopt,
-                        lcl.header, candidatesHash))
-                {
-                    highest = it;
-                    highestTxSet = cTxSet;
-                    highestApplicableTxSet = std::move(cApplicableTxSet);
-                }
+            if (highest == candidateValues.cend() ||
+                compareTxSets(highestApplicableTxSet, cApplicableTxSet,
+                              highest->txSetHash, sv.txSetHash,
+                              highestTxSet ? std::make_optional(
+                                                 highestTxSet->encodedSize())
+                                           : std::nullopt,
+                              cTxSet ? std::make_optional(cTxSet->encodedSize())
+                                     : std::nullopt,
+                              lcl.header, candidatesHash))
+            {
+                highest = it;
+                highestTxSet = cTxSet;
+                highestApplicableTxSet = std::move(cApplicableTxSet);
             }
         }
         if (highest == candidateValues.cend())

src/herder/test/HerderTests.cpp

@@ -3362,6 +3362,64 @@ TEST_CASE("SCP Driver", "[herder][acceptance]")
     }
 }
 
+// Test combineCandidates handling of candidates where
+// previousLedgerHash != LCL.hash
+TEST_CASE("combineCandidates with mismatched previousLedgerHash candidate",
+          "[herder][bug]")
+{
+    Config cfg(getTestConfig());
+
+    VirtualClock clock;
+    auto app = createTestApplication(clock, cfg);
+    auto& herder = dynamic_cast<HerderImpl&>(app->getHerder());
+    auto& pe = herder.getPendingEnvelopes();
+    auto& driver = herder.getHerderSCPDriver();
+
+    auto const& lcl = app->getLedgerManager().getLastClosedLedgerHeader();
+    uint32_t const ver = lcl.header.ledgerVersion;
+    uint64_t const closeTime = lcl.header.scpValue.closeTime + 1;
+    uint64_t const slotIndex = lcl.header.ledgerSeq + 1;
+
+    // Two structurally-valid empty tx sets that differ only in
+    // previousLedgerHash.
+    auto goodTxSet = TxSetXDRFrame::makeEmpty(lcl.hash, ver); // matches LCL
+    auto badTxSet = TxSetXDRFrame::makeEmpty(sha256("not the LCL hash"),
+                                             ver); // mismatched
+
+    ValueWrapperPtrSet candidates;
+    // Register the tx set so combineCandidates' getTxSet() returns it, then add
+    // a candidate value referencing it.
+    auto addCandidate = [&](TxSetXDRFrameConstPtr const& txSet) {
+        pe.addTxSet(txSet->getContentsHash(), slotIndex, txSet);
+        StellarValue sv =
+            herder.makeStellarValue(txSet->getContentsHash(), closeTime,
+                                    emptyUpgradeSteps, cfg.NODE_SEED);
+        candidates.emplace(driver.wrapValue(xdr::xdr_to_opaque(sv)));
+    };
+    auto combinedTxSetHash = [&]() {
+        ValueWrapperPtr result =
+            driver.combineCandidates(slotIndex, candidates);
+        StellarValue sv;
+        xdr::xdr_from_opaque(result->getValue(), sv);
+        return sv.txSetHash;
+    };
+
+    SECTION("prefer applicable candidate over mismatched candidate")
+    {
+        addCandidate(goodTxSet);
+        addCandidate(badTxSet);
+        REQUIRE(combinedTxSetHash() == goodTxSet->getContentsHash());
+    }
+
+    SECTION("all candidates have mismatched previousLedgerHash")
+    {
+        // If the *only* option is a candidate with a mismatched
+        // previousLedgerHash, choose it.
+        addCandidate(badTxSet);
+        REQUIRE(combinedTxSetHash() == badTxSet->getContentsHash());
+    }
+}
+
 TEST_CASE("SCP State", "[herder]")
 {
     SecretKey nodeKeys[3];