Check envelope types in test + remove outer db txn

Author: bboston7

src/herder/HerderImpl.cpp

@@ -308,11 +308,6 @@ HerderImpl::processExternalized(uint64 slotIndex, StellarValue const& value,
     if (mApp.getConfig().MODE_STORES_HISTORY_MISC)
     {
         ZoneNamedN(updateSCPHistoryZone, "update SCP history", true);
-
-        // NOTE: Consolidating the two saveSCPHistory calls into one transaction
-        // provides modest performance increases for sqlite and insignificant
-        // performance increases for postgres.
-        soci::transaction txScope(mApp.getDatabase().getSession());
         if (slotIndex != 0)
         {
             // Save any new SCP messages received about the previous ledger.
@@ -328,7 +323,6 @@ HerderImpl::processExternalized(uint64 slotIndex, StellarValue const& value,
             static_cast<uint32>(slotIndex),
             getSCP().getExternalizingState(slotIndex),
             mPendingEnvelopes.getCurrentlyTrackedQuorum());
-        txScope.commit();
     }
 
     // reflect upgrades with the ones included in this SCP round

src/herder/test/HerderTests.cpp

@@ -5848,27 +5848,46 @@ TEST_CASE("SCP message capture from previous ledger", "[herder]")
         },
         2 * Herder::EXP_LEDGER_TIMESPAN_SECONDS, false);
 
-    // Return the number of entries in a node's scphistory table for the given
-    // ledger number
-    auto getNumSCPHistoryEntries = [&](Application::pointer node,
-                                       uint32_t ledgerNum) {
-        auto& db = node->getDatabase();
-        auto prep = db.getPreparedStatement(
-            "SELECT COUNT(*) FROM scphistory WHERE ledgerseq = :l");
-        auto& st = prep.statement();
-        st.exchange(soci::use(ledgerNum));
-        uint32_t count;
-        st.exchange(soci::into(count));
-        st.define_and_bind();
-        st.execute(true);
-        return count;
-    };
+    // Check that a node's scphistory table for a given ledger has the correct
+    // number of entries of each type in `expectedTypes`
+    auto checkSCPHistoryEntries =
+        [&](Application::pointer node, uint32_t ledgerNum,
+            UnorderedMap<SCPStatementType, size_t> const& expectedTypes) {
+            // Prepare query
+            auto& db = node->getDatabase();
+            auto prep = db.getPreparedStatement(
+                "SELECT envelope FROM scphistory WHERE ledgerseq = :l");
+            auto& st = prep.statement();
+            st.exchange(soci::use(ledgerNum));
+            std::string envStr;
+            st.exchange(soci::into(envStr));
+            st.define_and_bind();
+            st.execute(false);
+
+            // Count the number of entries of each type
+            UnorderedMap<SCPStatementType, size_t> actualTypes;
+            while (st.fetch())
+            {
+                Value v;
+                decoder::decode_b64(envStr, v);
+                SCPEnvelope env;
+                xdr::xdr_from_opaque(v, env);
+                ++actualTypes[env.statement.pledges.type()];
+            }
+
+            REQUIRE(actualTypes == expectedTypes);
+        };
+
+    // A has 1 CONFIRM and 1 EXTERNALIZE in its scphistory table for ledger 2.
+    checkSCPHistoryEntries(A, 2,
+                           {{SCPStatementType::SCP_ST_CONFIRM, 1},
+                            {SCPStatementType::SCP_ST_EXTERNALIZE, 1}});
+
+    // B has 2 EXTERNALIZEs in its scphistory table for ledger 2.
+    checkSCPHistoryEntries(B, 2, {{SCPStatementType::SCP_ST_EXTERNALIZE, 2}});
 
-    // A and B should have 2 entries in their scphistory table for ledger 2. C
-    // should have 0.
-    REQUIRE(getNumSCPHistoryEntries(A, 2) == 2);
-    REQUIRE(getNumSCPHistoryEntries(B, 2) == 2);
-    REQUIRE(getNumSCPHistoryEntries(C, 2) == 0);
+    // C has no entries in its scphistory table for ledger 2.
+    checkSCPHistoryEntries(C, 2, {});
 
     // Get messages from A and B
     HerderImpl& herderA = dynamic_cast<HerderImpl&>(A->getHerder());
@@ -5910,9 +5929,12 @@ TEST_CASE("SCP message capture from previous ledger", "[herder]")
         },
         2 * Herder::EXP_LEDGER_TIMESPAN_SECONDS, false);
 
-    // A and B should now have 3 entries in their scphistory table for ledger 2.
-    REQUIRE(getNumSCPHistoryEntries(A, 2) == 3);
-    REQUIRE(getNumSCPHistoryEntries(B, 2) == 3);
+    // A and B should now each have 3 EXTERNALIZEs in their scphistory table for
+    // ledger 2. A's CONFIRM entry has been replaced with an EXTERNALIZE.
+    UnorderedMap<SCPStatementType, size_t> const expectedTypes = {
+        {SCPStatementType::SCP_ST_EXTERNALIZE, 3}};
+    checkSCPHistoryEntries(A, 2, expectedTypes);
+    checkSCPHistoryEntries(B, 2, expectedTypes);
 
     // Connect C to B and crank C to catch up with A and B
     simulation->addConnection(validatorCKey.getPublicKey(),
@@ -5921,8 +5943,8 @@ TEST_CASE("SCP message capture from previous ledger", "[herder]")
         [&]() { return C->getLedgerManager().getLastClosedLedgerNum() == 3; },
         2 * Herder::EXP_LEDGER_TIMESPAN_SECONDS, false);
 
-    // C should have 3 entries in its scphistory table for ledger 2. This check
-    // ensures that C does not double count messages from ledger 2 when closing
-    // ledger 3.
-    REQUIRE(getNumSCPHistoryEntries(C, 2) == 3);
+    // C should have 3 EXTERNALIZEs in its scphistory table for ledger 2. This
+    // check ensures that C does not double count messages from ledger 2 when
+    // closing ledger 3.
+    checkSCPHistoryEntries(C, 2, expectedTypes);
 }