Merge pull request #3838 from marta-lokhova/get_fee_bid_audit

rename getFeeBid -> getInclusionFee, bugfixes

Reviewed-by: dmkozh

src/herder/HerderSCPDriver.cpp

@@ -581,8 +581,8 @@ compareTxSets(TxSetFrameConstPtr l, TxSetFrameConstPtr r, Hash const& lh,
     if (protocolVersionStartsFrom(header.ledgerVersion,
                                   GENERALIZED_TX_SET_PROTOCOL_VERSION))
     {
-        auto lBids = l->getTotalBids();
-        auto rBids = r->getTotalBids();
+        auto lBids = l->getTotalInclusionFees();
+        auto rBids = r->getTotalInclusionFees();
         if (lBids != rBids)
         {
             return lBids < rBids;

src/herder/SurgePricingUtils.cpp

@@ -14,11 +14,13 @@ namespace stellar
 namespace
 {
 
+// Use _inclusion_ fee to order transactions
 int
 feeRate3WayCompare(TransactionFrameBase const& l, TransactionFrameBase const& r)
 {
-    return stellar::feeRate3WayCompare(l.getFeeBid(), l.getNumOperations(),
-                                       r.getFeeBid(), r.getNumOperations());
+    return stellar::feeRate3WayCompare(
+        l.getInclusionFee(), l.getNumOperations(), r.getInclusionFee(),
+        r.getNumOperations());
 }
 
 } // namespace
@@ -84,8 +86,8 @@ bool
 SurgePricingPriorityQueue::TxStackComparator::compareFeeOnly(
     TransactionFrameBase const& tx1, TransactionFrameBase const& tx2) const
 {
-    return compareFeeOnly(tx1.getFeeBid(), tx1.getNumOperations(),
-                          tx2.getFeeBid(), tx2.getNumOperations());
+    return compareFeeOnly(tx1.getInclusionFee(), tx1.getNumOperations(),
+                          tx2.getInclusionFee(), tx2.getNumOperations());
 }
 
 bool
@@ -458,9 +460,10 @@ SurgePricingPriorityQueue::canFitWithEviction(
         // computation is a no-op.
         if (!mComparator.compareFeeOnly(evictTx, tx))
         {
-            auto minFee = computeBetterFee(tx, evictTx.getFeeBid(),
+            auto minFee = computeBetterFee(tx, evictTx.getInclusionFee(),
                                            evictTx.getNumOperations());
-            return std::make_pair(false, minFee);
+            return std::make_pair(
+                false, minFee + (tx.getFullFee() - tx.getInclusionFee()));
         }
         // Ensure that this transaction is not from the same account.
         if (tx.getSourceID() == evictTx.getSourceID())

src/herder/TransactionQueue.cpp

@@ -98,15 +98,16 @@ TransactionQueue::~TransactionQueue()
 }
 
 // returns true, if a transaction can be replaced by another
-// `minFee` is set when returning false, and is the smallest fee
+// `minFee` is set when returning false, and is the smallest _full_ fee
 // that would allow replace by fee to succeed in this situation
+// Note that replace-by-fee logic is done on _inclusion_ fee
 static bool
 canReplaceByFee(TransactionFrameBasePtr tx, TransactionFrameBasePtr oldTx,
                 int64_t& minFee)
 {
-    int64_t newFee = tx->getFeeBid();
+    int64_t newFee = tx->getInclusionFee();
     uint32_t newNumOps = std::max<uint32_t>(1, tx->getNumOperations());
-    int64_t oldFee = oldTx->getFeeBid();
+    int64_t oldFee = oldTx->getInclusionFee();
     uint32_t oldNumOps = std::max<uint32_t>(1, oldTx->getNumOperations());
 
     // newFee / newNumOps >= FEE_MULTIPLIER * oldFee / oldNumOps
@@ -129,7 +130,7 @@ canReplaceByFee(TransactionFrameBasePtr tx, TransactionFrameBasePtr oldTx,
         else
         {
             // Add the potential flat component to the resulting min fee.
-            minFee += tx->getFullFee() - tx->getFeeBid();
+            minFee += tx->getFullFee() - tx->getInclusionFee();
         }
     }
     return res;
@@ -238,7 +239,7 @@ TransactionQueue::canAdd(TransactionFrameBasePtr tx,
         ltx.loadHeader().current().ledgerVersion,
         mApp.getLedgerManager().getSorobanNetworkConfig(ltx), mApp.getConfig());
 #endif
-    int64_t netFee = tx->getFeeBid();
+    int64_t newInclusionFee = tx->getInclusionFee();
     int64_t seqNum = 0;
     TransactionFrameBasePtr oldTx;
 
@@ -316,10 +317,10 @@ TransactionQueue::canAdd(TransactionFrameBasePtr tx,
                     }
 
                     oldTx = txToReplaceIter->mTx;
-                    int64_t oldFee = oldTx->getFeeBid();
+                    int64_t oldInclusionFee = oldTx->getInclusionFee();
                     if (oldTx->getFeeSourceID() == tx->getFeeSourceID())
                     {
-                        netFee -= oldFee;
+                        newInclusionFee -= oldInclusionFee;
                     }
                 }
 
@@ -402,7 +403,8 @@ TransactionQueue::canAdd(TransactionFrameBasePtr tx,
     int64_t totalFees = feeStateIter == mAccountStates.end()
                             ? 0
                             : feeStateIter->second.mTotalFees;
-    if (getAvailableBalance(ltx.loadHeader(), feeSource) - netFee < totalFees)
+    if (getAvailableBalance(ltx.loadHeader(), feeSource) - newInclusionFee <
+        totalFees)
     {
         tx->getResult().result.code(txINSUFFICIENT_BALANCE);
         return TransactionQueue::AddResult::ADD_STATUS_ERROR;

src/herder/TxQueueLimiter.cpp

@@ -18,8 +18,8 @@ computeBetterFee(std::pair<int64, uint32_t> const& evictedBid,
                  TransactionFrameBase const& tx)
 {
     if (evictedBid.second != 0 &&
-        feeRate3WayCompare(evictedBid.first, evictedBid.second, tx.getFeeBid(),
-                           tx.getNumOperations()) >= 0)
+        feeRate3WayCompare(evictedBid.first, evictedBid.second,
+                           tx.getInclusionFee(), tx.getNumOperations()) >= 0)
     {
         return computeBetterFee(tx, evictedBid.first, evictedBid.second);
     }
@@ -157,16 +157,20 @@ TxQueueLimiter::canAddTx(TransactionFrameBasePtr const& newTx,
     // that the new transaction is better (even if it fits otherwise). This
     // guarantees that we don't replace transactions with higher bids with
     // transactions with lower bids and less operations.
-    int64_t minFeeToBeatEvicted = std::max(
+    int64_t minInclusionFeeToBeatEvicted = std::max(
         computeBetterFee(
-            mLaneEvictedFeeBid[mSurgePricingLaneConfig->getLane(*newTx)],
+            mLaneEvictedInclusionFee[mSurgePricingLaneConfig->getLane(*newTx)],
             *newTx),
         computeBetterFee(
-            mLaneEvictedFeeBid[SurgePricingPriorityQueue::GENERIC_LANE],
+            mLaneEvictedInclusionFee[SurgePricingPriorityQueue::GENERIC_LANE],
             *newTx));
-    if (minFeeToBeatEvicted > 0)
+    // minInclusionFeeToBeatEvicted is the minimum _inclusion_ fee to evict txs.
+    // For reporting, return _full_ minimum fee
+    if (minInclusionFeeToBeatEvicted > 0)
     {
-        return std::make_pair(false, minFeeToBeatEvicted);
+        return std::make_pair(
+            false, minInclusionFeeToBeatEvicted +
+                       (newTx->getFullFee() - newTx->getInclusionFee()));
     }
 
     uint32_t txOpsDiscount = 0;
@@ -212,15 +216,15 @@ TxQueueLimiter::evictTransactions(
             // If tx has been evicted due to lane limit, then all the following
             // txs in this lane have to beat it. However, other txs could still
             // fit with a lower fee.
-            mLaneEvictedFeeBid[mSurgePricingLaneConfig->getLane(*tx)] = {
-                tx->getFeeBid(), tx->getNumOperations()};
+            mLaneEvictedInclusionFee[mSurgePricingLaneConfig->getLane(*tx)] = {
+                tx->getInclusionFee(), tx->getNumOperations()};
         }
         else
         {
             // If tx has been evicted before reaching the lane limit, we just
             // add it to generic lane, so that every new tx has to beat it.
-            mLaneEvictedFeeBid[SurgePricingPriorityQueue::GENERIC_LANE] = {
-                tx->getFeeBid(), tx->getNumOperations()};
+            mLaneEvictedInclusionFee[SurgePricingPriorityQueue::GENERIC_LANE] =
+                {tx->getInclusionFee(), tx->getNumOperations()};
         }
 
         evict(tx);
@@ -273,12 +277,12 @@ TxQueueLimiter::resetEvictionState()
 {
     if (mSurgePricingLaneConfig != nullptr)
     {
-        mLaneEvictedFeeBid.assign(
+        mLaneEvictedInclusionFee.assign(
             mSurgePricingLaneConfig->getLaneLimits().size(), {0, 0});
     }
     else
     {
-        releaseAssert(mLaneEvictedFeeBid.empty());
+        releaseAssert(mLaneEvictedInclusionFee.empty());
     }
 }
 }

src/herder/TxQueueLimiter.h

@@ -65,9 +65,9 @@ class TxQueueLimiter
     // When non-nullopt, limit the number dex operations by this value
     std::optional<Resource> mMaxDexOperations;
 
-    // Stores the maximum bid among the transactions evicted from every tx lane.
-    // Bids are stored as ratios (fee_bid / num_ops).
-    std::vector<std::pair<int64, uint32_t>> mLaneEvictedFeeBid;
+    // Stores the maximum inclusion fee among the transactions evicted from
+    // every tx lane. Inclusion fees are stored as ratios (fee_bid / num_ops).
+    std::vector<std::pair<int64, uint32_t>> mLaneEvictedInclusionFee;
 
     // Configuration of SurgePricingPriorityQueue with the per-lane operation
     // limits.

src/herder/TxSetFrame.cpp

@@ -208,8 +208,8 @@ computePerOpFee(TransactionFrameBase const& tx, uint32_t ledgerVersion)
                         ? Rounding::ROUND_DOWN
                         : Rounding::ROUND_UP;
     auto txOps = tx.getNumOperations();
-    return bigDivideOrThrow(tx.getFeeBid(), 1, static_cast<int64_t>(txOps),
-                            rounding);
+    return bigDivideOrThrow(tx.getInclusionFee(), 1,
+                            static_cast<int64_t>(txOps), rounding);
 }
 
 } // namespace
@@ -605,12 +605,15 @@ TxSetFrame::checkValid(Application& app, uint64_t lowerBoundCloseTimeOffset,
                         hexAbbrev(mPreviousLedgerHash), *fee);
                     return false;
                 }
-                if (tx->getFeeBid() < getMinFee(*tx, lcl.header, fee))
+                if (tx->getInclusionFee() <
+                    getMinInclusionFee(*tx, lcl.header, fee))
                 {
-                    CLOG_DEBUG(Herder,
-                               "Got bad txSet: {} has tx with fee bid lower "
-                               "than base fee",
-                               hexAbbrev(mPreviousLedgerHash));
+                    CLOG_DEBUG(
+                        Herder,
+                        "Got bad txSet: {} has tx with fee bid ({}) lower "
+                        "than base fee ({})",
+                        hexAbbrev(mPreviousLedgerHash), tx->getInclusionFee(),
+                        getMinInclusionFee(*tx, lcl.header, fee));
                     return false;
                 }
             }
@@ -954,7 +957,7 @@ TxSetFrame::getTotalFees(LedgerHeader const& lh) const
 }
 
 int64_t
-TxSetFrame::getTotalBids() const
+TxSetFrame::getTotalInclusionFees() const
 {
     ZoneScoped;
     int64_t total{0};
@@ -963,7 +966,7 @@ TxSetFrame::getTotalBids() const
                       total += std::accumulate(
                           phase.begin(), phase.end(), int64_t(0),
                           [&](int64_t t, TransactionFrameBasePtr const& tx) {
-                              return t + tx->getFeeBid();
+                              return t + tx->getInclusionFee();
                           });
                   });
 
@@ -979,7 +982,7 @@ TxSetFrame::summary() const
     }
     if (isGeneralizedTxSet())
     {
-        auto feeStats = [&](auto& feeMap) {
+        auto feeStats = [&](auto const& feeMap) {
             std::map<std::optional<int64_t>, std::pair<int, int>>
                 componentStats;
             for (auto const& [tx, fee] : feeMap)
@@ -1211,8 +1214,9 @@ TxSetFrame::applySurgePricing(Application& app)
             phase = includedTxs;
             if (isGeneralizedTxSet())
             {
-                computeTxFees(phaseType, lclHeader, *surgePricingLaneConfig,
-                              lowestLaneFee, hadTxNotFittingLane);
+                computeTxFees(TxSetFrame::Phase::CLASSIC, lclHeader,
+                              *surgePricingLaneConfig, lowestLaneFee,
+                              hadTxNotFittingLane);
             }
             else
             {

src/herder/TxSetFrame.h

@@ -143,8 +143,9 @@ class TxSetFrame : public NonMovableOrCopyable
     // Returns the sum of all fees that this transaction set would take.
     int64_t getTotalFees(LedgerHeader const& lh) const;
 
-    // Returns the sum of all bids for all transactions in this set.
-    int64_t getTotalBids() const;
+    // Returns the sum of all _inclusion fee_ bids for all transactions in this
+    // set.
+    int64_t getTotalInclusionFees() const;
 
     // Returns whether this transaction set is generalized, i.e. representable
     // by GeneralizedTransactionSet XDR.

src/herder/test/HerderTests.cpp

@@ -266,7 +266,8 @@ makeMultiPayment(stellar::TestAccount& destAccount, stellar::TestAccount& src,
         ops.emplace_back(payment(destAccount, i + paymentBase));
     }
     auto tx = src.tx(ops);
-    setFee(tx, static_cast<uint32_t>(tx->getFeeBid()) * feeMult + extraFee);
+    setFee(tx,
+           static_cast<uint32_t>(tx->getInclusionFee()) * feeMult + extraFee);
     getSignatures(tx).clear();
     tx->addSignature(src);
     return tx;
@@ -1460,7 +1461,7 @@ surgeTest(uint32 protocolVersion, uint32_t nbTxs, uint32_t maxTxSetSize,
         for (uint32_t n = 0; n < nbTxs; n++)
         {
             auto tx = multiPaymentTx(accountB, n + 1, 10000 + 1000 * n);
-            setFee(tx, static_cast<uint32_t>(tx->getFeeBid()) - 1);
+            setFee(tx, static_cast<uint32_t>(tx->getInclusionFee()) - 1);
             getSignatures(tx).clear();
             tx->addSignature(accountB);
             rootTxs.push_back(tx);
@@ -1484,7 +1485,7 @@ surgeTest(uint32 protocolVersion, uint32_t nbTxs, uint32_t maxTxSetSize,
             REQUIRE(rTx->getNumOperations() == n + 1);
             REQUIRE(tx->getNumOperations() == n + 2);
             // use the same fee
-            setFee(tx, static_cast<uint32_t>(rTx->getFeeBid()));
+            setFee(tx, static_cast<uint32_t>(rTx->getInclusionFee()));
             getSignatures(tx).clear();
             tx->addSignature(accountB);
             rootTxs.push_back(tx);
@@ -1506,11 +1507,11 @@ surgeTest(uint32 protocolVersion, uint32_t nbTxs, uint32_t maxTxSetSize,
             auto tx = multiPaymentTx(accountB, n + 1, 10000 + 1000 * n);
             if (n == 2)
             {
-                setFee(tx, static_cast<uint32_t>(tx->getFeeBid()) - 1);
+                setFee(tx, static_cast<uint32_t>(tx->getInclusionFee()) - 1);
             }
             else
             {
-                setFee(tx, static_cast<uint32_t>(tx->getFeeBid()) + 1);
+                setFee(tx, static_cast<uint32_t>(tx->getInclusionFee()) + 1);
             }
             getSignatures(tx).clear();
             tx->addSignature(accountB);
@@ -4545,7 +4546,7 @@ TEST_CASE("do not flood too many transactions", "[herder][transactionqueue]")
                 ops.emplace_back(payment(source, i));
             }
             auto tx = source.tx(ops);
-            auto txFee = static_cast<uint32_t>(tx->getFeeBid());
+            auto txFee = static_cast<uint32_t>(tx->getInclusionFee());
             if (highFee)
             {
                 txFee += 100000;
@@ -4598,7 +4599,7 @@ TEST_CASE("do not flood too many transactions", "[herder][transactionqueue]")
                 REQUIRE(expected == tx->getSeqNum());
             }
             // check if we have the expected fee
-            REQUIRE(tx->getFeeBid() == fees.front());
+            REQUIRE(tx->getInclusionFee() == fees.front());
             fees.pop_front();
             ++numBroadcast;
         };
@@ -4754,7 +4755,7 @@ TEST_CASE("do not flood too many transactions with DEX separation",
                 }
             }
             auto tx = source.tx(ops);
-            auto txFee = tx->getFeeBid();
+            auto txFee = tx->getInclusionFee();
             if (highFee)
             {
                 txFee += 100000;
@@ -4874,7 +4875,7 @@ TEST_CASE("do not flood too many transactions with DEX separation",
                     ->front()
                     .first;
 
-            REQUIRE(tx->getFeeBid() == expectedFee);
+            REQUIRE(tx->getInclusionFee() == expectedFee);
             accountFees[accountToIndex[tx->getSourceID()]].pop_front();
             if (tx->hasDexOperations())
             {