Cleanup

Author: dmkozh

src/herder/TxSetFrame.cpp

@@ -419,7 +419,7 @@ addWireTxsToList(Hash const& networkID,
     for (auto const& env : xdrTxs)
     {
         auto tx = TransactionFrameBase::makeTransactionFromWire(networkID, env);
-        if (!tx->XDRProvidesValidFee())
+        if (!tx->XDRProvidesValidFee() || tx->getInclusionFee() <= 0)
         {
             return false;
         }
@@ -1504,7 +1504,8 @@ TxSetPhaseFrame::makeFromWire(TxSetPhase phase, Hash const& networkID,
                 {
                     auto tx = TransactionFrameBase::makeTransactionFromWire(
                         networkID, env);
-                    if (!tx->XDRProvidesValidFee())
+                    if (!tx->XDRProvidesValidFee() ||
+                        tx->getInclusionFee() <= 0)
                     {
                         CLOG_DEBUG(Herder, "Got bad generalized txSet: "
                                            "transaction has invalid XDR");
@@ -1566,7 +1567,7 @@ TxSetPhaseFrame::makeFromWireLegacy(
         CLOG_DEBUG(
             Herder,
             "Got bad legacy txSet: transactions are not ordered correctly "
-            "or contain invalid phase transactions");
+            "or contain invalid transactions");
         return std::nullopt;
     }
     auto inclusionFeeMapPtr = std::make_shared<InclusionFeeMap>();

src/herder/test/HerderTests.cpp

@@ -478,6 +478,62 @@ testTxSet(uint32 protocolVersion)
                         TxSetValidationResult::TX_VALIDATION_FAILED);
             }
         }
+        SECTION("negative inclusion fee tx")
+        {
+            auto sorobanTx =
+                createUploadWasmTx(*app, *root, 100, 1000, SorobanResources{});
+            auto negFeeSorobanTxEnvelope = sorobanTx->getEnvelope();
+            negFeeSorobanTxEnvelope.v1().tx.fee = 1;
+            auto negFeeBump = feeBump(*app, *root, sorobanTx, 1,
+                                      /* useInclusionAsFullFee */ true);
+            auto lclHeader =
+                app->getLedgerManager().getLastClosedLedgerHeader();
+            SECTION("legacy tx set")
+            {
+                // This is a regression test - legacy tx sets are not allowed in
+                // new protocols, but Core still accepts them and it does some
+                // tx-related validation before reaching the
+                // `GENERALIZED_TXSET_MISMATCH` check.
+                TransactionSet txSet;
+                txSet.previousLedgerHash = lclHeader.hash;
+                txSet.txs.push_back(negFeeSorobanTxEnvelope);
+                auto applicableTxSet =
+                    TxSetXDRFrame::makeFromWire(txSet)->prepareForApply(
+                        *app, lclHeader.header);
+                REQUIRE(applicableTxSet == nullptr);
+
+                txSet.txs[0] = negFeeBump->getEnvelope();
+                auto applicableTxSet2 =
+                    TxSetXDRFrame::makeFromWire(txSet)->prepareForApply(
+                        *app, lclHeader.header);
+                REQUIRE(applicableTxSet2 == nullptr);
+            }
+            SECTION("generalized tx set")
+            {
+                auto txSet =
+                    testtxset::makeNonValidatedGeneralizedTxSet(
+                        {{},
+                         {std::make_pair(
+                             std::nullopt,
+                             std::vector<TransactionFrameBasePtr>{
+                                 TransactionFrameBase::makeTransactionFromWire(
+                                     app->getNetworkID(),
+                                     negFeeSorobanTxEnvelope)})}},
+                        *app, lclHeader.hash)
+                        .second;
+                REQUIRE(txSet == nullptr);
+
+                auto txSet2 =
+                    testtxset::makeNonValidatedGeneralizedTxSet(
+                        {{},
+                         {std::make_pair(std::nullopt,
+                                         std::vector<TransactionFrameBasePtr>{
+                                             negFeeBump})}},
+                        *app, lclHeader.hash)
+                        .second;
+                REQUIRE(txSet2 == nullptr);
+            }
+        }
     }
 }
 

src/herder/test/TxSetTests.cpp

@@ -83,7 +83,7 @@ TEST_CASE("generalized tx set XDR validation", "[txset]")
             txEnv.v1().tx.operations.emplace_back();
             // The fee is actually not relevant for XDR validation, we just use
             // it to have different tx envelopes.
-            txEnv.v1().tx.fee = 100 + txId;
+            txEnv.v1().tx.fee = 10000 + txId;
             ++txId;
             txEnv.v1().tx.operations.back().body.type(
                 isSoroban ? OperationType::INVOKE_HOST_FUNCTION

src/overlay/test/OverlayTests.cpp

@@ -3371,7 +3371,7 @@ TEST_CASE("populateSignatureCache tests", "[overlay]")
         REQUIRE(finalMisses == 0);
     }
 
-    SECTION("Ed25519 signed payload signer skipped in overlay validation")
+    SECTION("Ed25519 signed payload signer cached during overlay validation")
     {
         // Add an ed25519 signed payload signer to testAccount and remove the
         // master key, so the account can only be authorized via the payload
@@ -3404,18 +3404,19 @@ TEST_CASE("populateSignatureCache tests", "[overlay]")
 
         resetCache();
 
-        // populateSignatureCache runs with isOverlayValidation=true, which
-        // skips ed25519 signed payload signers. So no cache entries should
-        // be produced.
         invokePopulateSignatureCache(payTx);
 
         uint64_t hits, misses;
         PubKeyUtils::flushVerifySigCacheCounts(hits, misses);
-        REQUIRE(hits == 0);
-        REQUIRE(misses == 0);
+        REQUIRE(misses == 1);
+        // `populateSignatureCache` verifies the payload signature at the
+        // transaction level (cache miss, added to cache) and again at the
+        // operation level (cache hit from the first check within the same
+        // call).
+        REQUIRE(hits == 1);
 
-        // Now call checkValid (normal path), which DOES check payload
-        // signers. Since the cache was not populated, we should see a miss.
+        // checkValid now sees the cache already populated: both tx-level
+        // and op-level signed-payload lookups are pure cache hits.
         LedgerTxn ltx(app->getLedgerTxnRoot());
         auto ls = LedgerSnapshot(ltx);
         auto diagnostics = DiagnosticEventManager::createDisabled();
@@ -3424,16 +3425,8 @@ TEST_CASE("populateSignatureCache tests", "[overlay]")
         REQUIRE(result->isSuccess());
 
         PubKeyUtils::flushVerifySigCacheCounts(hits, misses);
-        // The payload signer was not cached by populateSignatureCache, so
-        // checkValid must produce a cache miss for the payload signature.
-        // This contrasts with the "Normal transaction" test where
-        // populateSignatureCache caches the ed25519 signature and
-        // checkValid sees only cache hits (misses == 0).
-        REQUIRE(misses == 1);
-        // checkValid verifies the payload signature at the transaction
-        // level (cache miss, added to cache) and again at the operation
-        // level (cache hit from the first check within the same call).
-        REQUIRE(hits == 1);
+        REQUIRE(misses == 0);
+        REQUIRE(hits == 2);
     }
 
     SECTION("Signature cache invalidation after signer removal")

src/overlay/test/SurveyManagerTests.cpp

@@ -811,7 +811,7 @@ TEST_CASE("Time sliced dynamic topology survey", "[overlay][survey][topology]")
         [&simulation, nLedgers]() {
             return simulation->haveAllExternalized(nLedgers + 1, 1);
         },
-        10 * nLedgers * simulation->getExpectedLedgerCloseTime(), false);
+        20 * nLedgers * simulation->getExpectedLedgerCloseTime(), false);
 
     REQUIRE(simulation->haveAllExternalized(nLedgers + 1, 1));
 

src/transactions/FeeBumpTransactionFrame.cpp

@@ -240,8 +240,7 @@ FeeBumpTransactionFrame::checkSignature(SignatureChecker& signatureChecker,
     }
     signers.insert(signers.end(), acc.signers.begin(), acc.signers.end());
 
-    return signatureChecker.checkSignature(
-        signers, neededWeight, !signatureChecker.isOverlayValidation());
+    return signatureChecker.checkSignature(signers, neededWeight);
 }
 
 bool

src/transactions/SignatureChecker.cpp

@@ -39,10 +39,16 @@ SignatureChecker::isOverlayValidation() const
     return mIsOverlayValidation;
 }
 
+bool
+SignatureChecker::isOverVerificationBudget() const
+{
+    return mIsOverlayValidation &&
+           mTxEd25519Verifications >= OVERLAY_TX_ED25519_VERIFY_BUDGET;
+}
+
 bool
 SignatureChecker::checkSignature(std::vector<Signer> const& signersV,
-                                 int neededWeight,
-                                 bool checkEd25519SignedPayload)
+                                 int neededWeight)
 {
 #ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
     return true;
@@ -127,6 +133,10 @@ SignatureChecker::checkSignature(std::vector<Signer> const& signersV,
     verified =
         verifyAll(signers[SIGNER_KEY_TYPE_ED25519],
                   [&](DecoratedSignature const& sig, Signer const& signerKey) {
+                      if (isOverVerificationBudget())
+                      {
+                          return false;
+                      }
                       auto [valid, cacheLookupRes] = SignatureUtils::verify(
                           sig, signerKey.key, mContentsHash);
                       updateTxSigCacheMetrics(cacheLookupRes);
@@ -136,26 +146,26 @@ SignatureChecker::checkSignature(std::vector<Signer> const& signersV,
     {
         return true;
     }
-
-    if (checkEd25519SignedPayload)
+    if (isOverVerificationBudget())
     {
-        verified = verifyAll(
-            signers[SIGNER_KEY_TYPE_ED25519_SIGNED_PAYLOAD],
-            [&](DecoratedSignature const& sig, Signer const& signerKey) {
-                auto [valid, cacheLookupRes] =
-                    SignatureUtils::verifyEd25519SignedPayload(sig,
-                                                               signerKey.key);
-                updateTxSigCacheMetrics(cacheLookupRes);
-                return valid;
-            });
-        if (verified)
-        {
-            return true;
-        }
+        return false;
     }
-    else
+
+    verified = verifyAll(
+        signers[SIGNER_KEY_TYPE_ED25519_SIGNED_PAYLOAD],
+        [&](DecoratedSignature const& sig, Signer const& signerKey) {
+            if (isOverVerificationBudget())
+            {
+                return false;
+            }
+            auto [valid, cacheLookupRes] =
+                SignatureUtils::verifyEd25519SignedPayload(sig, signerKey.key);
+            updateTxSigCacheMetrics(cacheLookupRes);
+            return valid;
+        });
+    if (verified)
     {
-        releaseAssert(mIsOverlayValidation);
+        return true;
     }
 
     return false;
@@ -204,6 +214,11 @@ void
 SignatureChecker::updateTxSigCacheMetrics(
     PubKeyUtils::VerifySigCacheLookupResult cacheLookupRes)
 {
+    if (cacheLookupRes != PubKeyUtils::VerifySigCacheLookupResult::NO_LOOKUP)
+    {
+        ++mTxEd25519Verifications;
+    }
+
     if (!mTrackCacheMetrics)
     {
         return;

src/transactions/SignatureChecker.h

@@ -9,7 +9,6 @@
 #include "xdr/Stellar-transaction.h"
 #include "xdr/Stellar-types.h"
 
-#include <map>
 #include <set>
 #include <stdint.h>
 #include <vector>
@@ -26,8 +25,7 @@ class SignatureChecker
         bool isOverlayValidation = false);
 #ifdef BUILD_TESTS
     virtual bool checkSignature(std::vector<Signer> const& signersV,
-                                int32_t neededWeight,
-                                bool checkEd25519SignedPayload = true);
+                                int32_t neededWeight);
     virtual bool checkAllSignaturesUsed() const;
     virtual ~SignatureChecker() = default;
 
@@ -37,8 +35,7 @@ class SignatureChecker
     virtual void disableCacheMetricsTracking();
 #else
     bool checkSignature(std::vector<Signer> const& signersV,
-                        int32_t neededWeight,
-                        bool checkEd25519SignedPayload = true);
+                        int32_t neededWeight);
     bool checkAllSignaturesUsed() const;
 
     // Do not record signature cache metrics for this instance. This should be
@@ -47,6 +44,7 @@ class SignatureChecker
 #endif // BUILD_TESTS
 
     bool isOverlayValidation() const;
+    static constexpr uint32_t OVERLAY_TX_ED25519_VERIFY_BUDGET = 1000;
 
     // Reset and return the counts of signature checks performed as part of
     // transaction `checkValid` or apply flow. The first element of the pair is
@@ -62,6 +60,9 @@ class SignatureChecker
     bool mIsOverlayValidation{false};
 
     std::vector<bool> mUsedSignatures;
+    uint32_t mTxEd25519Verifications{0};
+
+    bool isOverVerificationBudget() const;
 
     // Static fields for tracking signature verification cache performance
     // during the `checkValid` or apply flow
@@ -90,7 +91,7 @@ class AlwaysValidSignatureChecker : public SignatureChecker
     }
 
     bool
-    checkSignature(std::vector<Signer> const&, int32_t, bool) override
+    checkSignature(std::vector<Signer> const&, int32_t) override
     {
         return true;
     }

src/transactions/TransactionFrame.cpp

@@ -511,8 +511,7 @@ TransactionFrame::checkSignature(SignatureChecker& signatureChecker,
     }
     signers.insert(signers.end(), acc.signers.begin(), acc.signers.end());
 
-    return signatureChecker.checkSignature(
-        signers, neededWeight, !signatureChecker.isOverlayValidation());
+    return signatureChecker.checkSignature(signers, neededWeight);
 }
 
 bool
@@ -547,7 +546,7 @@ TransactionFrame::checkExtraSigners(SignatureChecker& signatureChecker) const
         // we assign a weight of 1 to each key, and set the neededWeight to the
         // number of extraSigners
         return signatureChecker.checkSignature(
-            signers, static_cast<int32_t>(signers.size()), true);
+            signers, static_cast<int32_t>(signers.size()));
     }
     return true;
 }

src/transactions/test/TxEnvelopeTests.cpp

@@ -2478,7 +2478,7 @@ TEST_CASE_VERSIONS("txenvelope", "[tx][envelope]")
     }
 }
 
-TEST_CASE_VERSIONS("overlay validation skips ed25519 signed payload signers",
+TEST_CASE_VERSIONS("overlay validation handles ed25519 signed payload signers",
                    "[tx][envelope][overlay]")
 {
     Config cfg = getTestConfig(0, Config::TESTDB_IN_MEMORY);
@@ -2489,9 +2489,8 @@ TEST_CASE_VERSIONS("overlay validation skips ed25519 signed payload signers",
 
     int64_t const paymentAmount = app->getLedgerManager().getLastReserve() * 10;
 
-    // This test validates that checkValidForOverlay skips
-    // SIGNER_KEY_TYPE_ED25519_SIGNED_PAYLOAD verification, while checkValid
-    // (used during txset validation and apply) does not.
+    // This test validates that checkValidForOverlay verifies
+    // SIGNER_KEY_TYPE_ED25519_SIGNED_PAYLOAD signers normally
     for_versions_from(19, *app, [&] {
         auto a1 = root->create("a1", paymentAmount);
 
@@ -2537,18 +2536,14 @@ TEST_CASE_VERSIONS("overlay validation skips ed25519 signed payload signers",
             REQUIRE(tx->getResultCode() == txSUCCESS);
         }
 
-        SECTION("checkValidForOverlay rejects ed25519 signed payload signer")
+        SECTION("checkValidForOverlay accepts ed25519 signed payload signer")
         {
-            // Overlay validation skips payload signer checking, so a tx
-            // authorized only via payload signer should fail overlay
-            // validation
             LedgerTxn ltx(app->getLedgerTxnRoot());
             auto ls = LedgerSnapshot(ltx);
             auto diagnostics = DiagnosticEventManager::createDisabled();
             auto result = tx->checkValidForOverlay(app->getAppConnector(), ls,
                                                    0, 0, 0, diagnostics);
-            REQUIRE(!result->isSuccess());
-            REQUIRE(result->getResultCode() == txBAD_AUTH);
+            REQUIRE(result->isSuccess());
         }
 
         SECTION("fee bump with ed25519 signed payload signer on inner tx")
@@ -2567,16 +2562,14 @@ TEST_CASE_VERSIONS("overlay validation skips ed25519 signed payload signers",
                 REQUIRE(result->isSuccess());
             }
 
-            SECTION("checkValidForOverlay rejects fee bump")
+            SECTION("checkValidForOverlay accepts fee bump")
             {
                 LedgerTxn ltx(app->getLedgerTxnRoot());
                 auto ls = LedgerSnapshot(ltx);
                 auto diagnostics = DiagnosticEventManager::createDisabled();
                 auto result = feeBumpTx->checkValidForOverlay(
                     app->getAppConnector(), ls, 0, 0, 0, diagnostics);
-                REQUIRE(!result->isSuccess());
-                // Fee bump wraps the inner failure
-                REQUIRE(result->getResultCode() == txFEE_BUMP_INNER_FAILED);
+                REQUIRE(result->isSuccess());
             }
         }
     });