Fix more sub-second ledger edge cases

Author: SirTyson

src/herder/LedgerCloseData.cpp

@@ -81,7 +81,7 @@ stellarValueToString(Config const& c, StellarValue const& sv)
     }
     res << " txH: " << hexAbbrev(sv.txSetHash) << ", ct: " << sv.closeTime;
 #ifdef MS_CLOSE_TIME
-    if (getCloseTimeMs(sv) != 0 || isMsCloseTimeStellarValue(sv))
+    if (isMsCloseTimeStellarValue(sv))
     {
         res << ", ctMs: " << getCloseTimeMs(sv);
     }

src/overlay/Peer.cpp

@@ -1422,8 +1422,10 @@ bool
 Peer::process(QueryInfo& queryInfo, std::optional<uint32_t> maxQueriesPerWindow)
 {
     auto const& cfg = mAppConnector.getConfig();
+    // Round up so sub-second close times can't produce a zero-length window
+    // (which would zero out QUERIES_PER_WINDOW and reject every query).
     std::chrono::seconds const QUERY_WINDOW =
-        std::chrono::duration_cast<std::chrono::seconds>(
+        std::chrono::ceil<std::chrono::seconds>(
             mAppConnector.getLedgerManager().getExpectedLedgerCloseTime() *
             cfg.MAX_SLOTS_TO_REMEMBER);
     uint32_t const QUERIES_PER_WINDOW = maxQueriesPerWindow.value_or(

src/overlay/Peer.h

@@ -508,6 +508,13 @@ class Peer : public std::enable_shared_from_this<Peer>,
         releaseAssert(threadIsMain());
         return mSCPStateQueryInfo.mNumQueries;
     }
+
+    // Testing only function to expose the query rate-limiting check
+    bool
+    processQueryForTesting(QueryInfo& queryInfo)
+    {
+        return process(queryInfo);
+    }
 #endif
 
     // Public thread-safe methods that access Peer's state

src/overlay/test/OverlayTests.cpp

@@ -2097,6 +2097,52 @@ TEST_CASE("GET_SCP_STATE rate limiting", "[overlay]")
     testutil::shutdownWorkScheduler(*app1);
 }
 
+TEST_CASE("query rate limit window under sub-second close times", "[overlay]")
+{
+    VirtualClock clock;
+    Config cfg1 = getTestConfig(0);
+    Config cfg2 = getTestConfig(1);
+
+    // Make the raw query window (close time * slots to remember) shorter than
+    // one second. It must round up to a one-second window; truncating instead
+    // would produce a zero-length window with a zero query allowance,
+    // rejecting every query.
+    for (auto* cfg : {&cfg1, &cfg2})
+    {
+        cfg->ARTIFICIALLY_SET_CLOSE_TIME_FOR_TESTING = 100;
+        cfg->MAX_SLOTS_TO_REMEMBER = 4;
+    }
+
+    auto app1 = createTestApplication(clock, cfg1);
+    auto app2 = createTestApplication(clock, cfg2);
+
+    LoopbackPeerConnection conn(*app1, *app2);
+    testutil::crankSome(clock);
+    auto peer = conn.getAcceptor();
+    REQUIRE(peer->isAuthenticatedForTesting());
+
+    // The rounded-up 1s window admits QUERY_RESPONSE_MULTIPLIER (see Peer.cpp)
+    // queries per window
+    uint32_t constexpr EXPECTED_QUERIES_PER_WINDOW = 5;
+
+    Peer::QueryInfo queryInfo;
+    for (uint32_t i = 0; i < EXPECTED_QUERIES_PER_WINDOW; i++)
+    {
+        REQUIRE(peer->processQueryForTesting(queryInfo));
+        queryInfo.mNumQueries++;
+    }
+    // The next query in the same window exceeds the allowance
+    REQUIRE(!peer->processQueryForTesting(queryInfo));
+
+    // Advancing past the rounded-up window resets the allowance
+    testutil::crankFor(clock, std::chrono::seconds(2));
+    REQUIRE(peer->processQueryForTesting(queryInfo));
+    REQUIRE(queryInfo.mNumQueries == 0);
+
+    testutil::shutdownWorkScheduler(*app2);
+    testutil::shutdownWorkScheduler(*app1);
+}
+
 TEST_CASE("reject peers with the same nodeid", "[overlay][connections]")
 {
     VirtualClock clock;

src/simulation/LoadGenerator.cpp

@@ -502,12 +502,15 @@ LoadGenerator::scheduleLoadGeneration(GeneratedLoadConfig cfg)
     }
 
     // During load submission, we must have enough unique source accounts (with
-    // a buffer) to accommodate the desired tx rate.
-    auto closeTimeSeconds = std::chrono::duration_cast<std::chrono::seconds>(
-        mApp.getLedgerManager().getExpectedLedgerCloseTime());
-    if (cfg.nTxs > cfg.nAccounts && (cfg.txRate * closeTimeSeconds.count()) *
-                                            MIN_UNIQUE_ACCOUNT_MULTIPLIER >
-                                        cfg.nAccounts)
+    // a buffer) to accommodate the desired tx rate. txRate is per second, so
+    // (txRate * closeTime) is the per-ledger tx count; round it up so
+    // sub-second close times don't truncate it to 0 and disable the check.
+    auto const closeTime = mApp.getLedgerManager().getExpectedLedgerCloseTime();
+    auto const txsPerLedger = std::chrono::ceil<std::chrono::seconds>(
+                                  closeTime * static_cast<int64_t>(cfg.txRate))
+                                  .count();
+    if (cfg.nTxs > cfg.nAccounts &&
+        txsPerLedger * MIN_UNIQUE_ACCOUNT_MULTIPLIER > cfg.nAccounts)
     {
         errorMsg = fmt::format(
             "Tx rate is too high, there are not enough unique accounts. Make "

src/transactions/TransactionUtils.cpp

@@ -1387,10 +1387,12 @@ getUpperBoundCloseTimeOffset(Application& app, uint64_t lastCloseTime)
     uint64_t closeTimeDrift =
         currentTime <= lastCloseTime ? 0 : currentTime - lastCloseTime;
 
-    return std::chrono::duration_cast<std::chrono::seconds>(
-               app.getLedgerManager().getExpectedLedgerCloseTime())
-                   .count() *
-               EXPECTED_CLOSE_TIME_MULT +
+    // Round up so sub-second expected close times still leave at least a one
+    // second buffer (time bounds are whole-second quantities).
+    return std::chrono::ceil<std::chrono::seconds>(
+               app.getLedgerManager().getExpectedLedgerCloseTime() *
+               EXPECTED_CLOSE_TIME_MULT)
+               .count() +
            closeTimeDrift;
 }
 

src/transactions/test/TxEnvelopeTests.cpp

@@ -3241,3 +3241,79 @@ TEST_CASE("transaction time bounds under sub-second ledgers", "[tx][envelope]")
     }
 }
 #endif // MS_CLOSE_TIME
+
+TEST_CASE("getUpperBoundCloseTimeOffset under sub-second ledgers",
+          "[tx][envelope]")
+{
+    // The offset is consumed at transaction queue admission: a tx whose
+    // maxTime falls inside [lclCloseTime, lclCloseTime + offset) is rejected
+    // as txTOO_LATE by Herder::recvTransaction, since it would likely be
+    // expired by the time it lands in a ledger. Exercise that path end to end
+    // and assert exactly where the admission boundary sits.
+    auto testWithCloseTimeMs = [](uint32_t closeTimeMs,
+                                  uint64_t expectedBuffer) {
+        VirtualClock clock;
+        auto cfg = getTestConfig();
+        cfg.ARTIFICIALLY_SET_CLOSE_TIME_FOR_TESTING = closeTimeMs;
+        auto app = createTestApplication(clock, cfg);
+        auto& lm = app->getLedgerManager();
+        auto root = app->getRoot();
+
+        // Close a ledger at a known whole second T, then align the clock with
+        // it so the offset's drift term starts at zero
+        TimePoint const T =
+            VirtualClock::to_time_t(app->getClock().system_now()) + 1000;
+        closeLedgerOn(*app, lm.getLastClosedLedgerNum() + 1, T);
+
+        // The queue admits one pending tx per source account, so give every
+        // submission its own account
+        int accountIndex = 0;
+        auto submitWithMaxTime = [&](TimePoint maxTime) {
+            auto acc =
+                root->create("sub-second-" + std::to_string(accountIndex++),
+                             lm.getLastMinBalance(0) + 10000);
+            auto tx = acc.tx({payment(*root, 1)});
+            setMaxTime(tx, maxTime);
+            getSignatures(tx).clear();
+            tx->addSignature(acc.getSecretKey());
+            return app->getHerder().recvTransaction(tx, true);
+        };
+        auto expectTooLate = [&](TimePoint maxTime) {
+            auto r = submitWithMaxTime(maxTime);
+            REQUIRE(r.code ==
+                    TransactionQueue::AddResultCode::ADD_STATUS_ERROR);
+            REQUIRE(r.txResult->getResultCode() == txTOO_LATE);
+        };
+        auto expectAdmitted = [&](TimePoint maxTime) {
+            REQUIRE(submitWithMaxTime(maxTime).code ==
+                    TransactionQueue::AddResultCode::ADD_STATUS_PENDING);
+        };
+
+        clock.setCurrentVirtualTime(VirtualClock::from_time_t(T));
+
+        // With no drift, the first admissible maxTime is T + buffer
+        expectTooLate(T + expectedBuffer - 1);
+        expectAdmitted(T + expectedBuffer);
+
+        // Wall-clock time elapsed since the last close is added on top
+        clock.setCurrentVirtualTime(VirtualClock::from_time_t(T + 7));
+        expectTooLate(T + expectedBuffer + 7 - 1);
+        expectAdmitted(T + expectedBuffer + 7);
+    };
+
+    SECTION("whole-second close times keep the legacy 2x buffer")
+    {
+        testWithCloseTimeMs(5000, 10);
+    }
+    SECTION("sub-second close times round the buffer up to a whole second")
+    {
+        // 2 * 400ms rounds up to 1s rather than truncating to 0, which would
+        // erase the admission-time expiry buffer entirely
+        testWithCloseTimeMs(400, 1);
+    }
+    SECTION("fractional-second close times round up, not down")
+    {
+        // 2 * 1300ms = 2600ms rounds up to 3s
+        testWithCloseTimeMs(1300, 3);
+    }
+}

test-tx-meta-baseline-current/TxEnvelopeTests.json

@@ -35,6 +35,9 @@
 		27,
 		28
 	],
+	"getUpperBoundCloseTimeOffset under sub-second ledgers|fractional-second close times round up, not down" : [ "EbIPrt4owbA=", "kAwIMT7FxME=", "+8eXWHGepSo=", "Pk+9/t2Sw1Q=" ],
+	"getUpperBoundCloseTimeOffset under sub-second ledgers|sub-second close times round the buffer up to a whole second" : [ "EbIPrt4owbA=", "kAwIMT7FxME=", "+8eXWHGepSo=", "Pk+9/t2Sw1Q=" ],
+	"getUpperBoundCloseTimeOffset under sub-second ledgers|whole-second close times keep the legacy 2x buffer" : [ "EbIPrt4owbA=", "kAwIMT7FxME=", "+8eXWHGepSo=", "Pk+9/t2Sw1Q=" ],
 	"overlay validation handles ed25519 signed payload signers|protocol version 19" : 
 	[
 		"bXrfFM/EOrA=",

test-tx-meta-baseline-next/TxEnvelopeTests.json

@@ -36,6 +36,9 @@
 		28,
 		29
 	],
+	"getUpperBoundCloseTimeOffset under sub-second ledgers|fractional-second close times round up, not down" : [ "EbIPrt4owbA=", "kAwIMT7FxME=", "+8eXWHGepSo=", "Pk+9/t2Sw1Q=" ],
+	"getUpperBoundCloseTimeOffset under sub-second ledgers|sub-second close times round the buffer up to a whole second" : [ "EbIPrt4owbA=", "kAwIMT7FxME=", "+8eXWHGepSo=", "Pk+9/t2Sw1Q=" ],
+	"getUpperBoundCloseTimeOffset under sub-second ledgers|whole-second close times keep the legacy 2x buffer" : [ "EbIPrt4owbA=", "kAwIMT7FxME=", "+8eXWHGepSo=", "Pk+9/t2Sw1Q=" ],
 	"overlay validation handles ed25519 signed payload signers|protocol version 19" : 
 	[
 		"bXrfFM/EOrA=",