Repository: zcash/zcash (language: rust)

Issue:
Change ZIP 401 mempool limiting to use conventional fee and new constants
1. Apply `low_fee_penalty` to transactions paying below the ZIP 317 fee [as already specified in ZIP 401](https://github.com/zcash/zips/commit/bb04751a5bb3dfa3f3e885f5d1f980040f1347df#diff-e8ce48f91a8494325daeea2a8bfb2f5a4f1d6d261d0c3a985da463b03a24e14c).
2. Change the `min_tx_cost` and `low_fee_penalty` constants per zcash/zips#565 .


Discussion / hints:

Fix patch:
diff --git a/src/mempool_limit.cpp b/src/mempool_limit.cpp
index 743a10e7b32..89f6f28b4ff 100644
--- a/src/mempool_limit.cpp
+++ b/src/mempool_limit.cpp
@@ -44,7 +44,7 @@ std::pair<int64_t, int64_t> MempoolCostAndEvictionWeight(const CTransaction& tx,
     size_t memUsage = RecursiveDynamicUsage(tx);
     int64_t cost = std::max((int64_t) memUsage, (int64_t) MIN_TX_COST);
     int64_t evictionWeight = cost;
-    if (fee < DEFAULT_FEE) {
+    if (fee < tx.GetConventionalFee()) {
         evictionWeight += LOW_FEE_PENALTY;
     }
     return std::make_pair(cost, evictionWeight);
diff --git a/src/mempool_limit.h b/src/mempool_limit.h
index b9cc314dfdf..b9a7ffee5e3 100644
--- a/src/mempool_limit.h
+++ b/src/mempool_limit.h
@@ -22,8 +22,8 @@ const size_t DEFAULT_MEMPOOL_TOTAL_COST_LIMIT = 80000000;
 const int64_t DEFAULT_MEMPOOL_EVICTION_MEMORY_MINUTES = 60;
 
 const size_t EVICTION_MEMORY_ENTRIES = 40000;
-const int64_t MIN_TX_COST = 4000;
-const int64_t LOW_FEE_PENALTY = 16000;
+const int64_t MIN_TX_COST = 10000;
+const int64_t LOW_FEE_PENALTY = 40000;
 
 
 // This class keeps track of transactions which have been recently evicted from the mempool


Test patch:
diff --git a/qa/rpc-tests/mempool_limit.py b/qa/rpc-tests/mempool_limit.py
index 466444006cd..874f238b5e0 100755
--- a/qa/rpc-tests/mempool_limit.py
+++ b/qa/rpc-tests/mempool_limit.py
@@ -10,8 +10,7 @@
     get_coinbase_address,
     fail,
     start_nodes,
-    wait_and_assert_operationid_status,
-    DEFAULT_FEE
+    wait_and_assert_operationid_status
 )
 from test_framework.zip317 import conventional_fee
 
@@ -28,11 +27,11 @@
 class MempoolLimit(BitcoinTestFramework):
     def setup_nodes(self):
         extra_args = [
-            BASE_ARGS + ['-mempooltxcostlimit=8000'], # 2 transactions at min cost
-            BASE_ARGS + ['-mempooltxcostlimit=8000'], # 2 transactions at min cost
-            BASE_ARGS + ['-mempooltxcostlimit=8000'], # 2 transactions at min cost
+            BASE_ARGS + ['-mempooltxcostlimit=20000'], # 2 transactions at min cost
+            BASE_ARGS + ['-mempooltxcostlimit=20000'], # 2 transactions at min cost
+            BASE_ARGS + ['-mempooltxcostlimit=20000'], # 2 transactions at min cost
             # Let node 3 hold one more transaction
-            BASE_ARGS + ['-mempooltxcostlimit=12000'], # 3 transactions at min cost
+            BASE_ARGS + ['-mempooltxcostlimit=30000'], # 3 transactions at min cost
         ]
         return start_nodes(self.num_nodes, self.options.tmpdir, extra_args)
 
@@ -68,17 +67,18 @@ def run_test(self):
         zaddr1 = self.nodes[0].z_getnewaddress('sapling')
         zaddr2 = self.nodes[0].z_getnewaddress('sapling')
         zaddr3 = self.nodes[0].z_getnewaddress('sapling')
+        fee = conventional_fee(2)
 
         print("Filling mempool...")
         opid1 = self.nodes[1].z_sendmany(
             get_coinbase_address(self.nodes[1]),
-            [{"address": zaddr1, "amount": Decimal('10.0') - DEFAULT_FEE}],
-            1, DEFAULT_FEE, 'AllowRevealedSenders')
+            [{"address": zaddr1, "amount": Decimal('10.0') - fee}],
+            1, fee, 'AllowRevealedSenders')
         wait_and_assert_operationid_status(self.nodes[1], opid1)
         opid2 = self.nodes[2].z_sendmany(
             get_coinbase_address(self.nodes[2]),
-            [{"address": zaddr2, "amount": Decimal('10.0') - DEFAULT_FEE}],
-            1, DEFAULT_FEE, 'AllowRevealedSenders')
+            [{"address": zaddr2, "amount": Decimal('10.0') - fee}],
+            1, fee, 'AllowRevealedSenders')
         wait_and_assert_operationid_status(self.nodes[2], opid2)
         self.sync_all()
 
@@ -87,8 +87,8 @@ def run_test(self):
         print("Adding one more transaction...")
         opid3 = self.nodes[3].z_sendmany(
             get_coinbase_address(self.nodes[3]),
-            [{"address": zaddr3, "amount": Decimal('10.0') - DEFAULT_FEE}],
-            1, DEFAULT_FEE, 'AllowRevealedSenders')
+            [{"address": zaddr3, "amount": Decimal('10.0') - fee}],
+            1, fee, 'AllowRevealedSenders')
         wait_and_assert_operationid_status(self.nodes[3], opid3)
         # The mempools are no longer guaranteed to be in a consistent state, so we cannot sync
         sleep(5)
@@ -106,9 +106,9 @@ def run_test(self):
         print("Checking mempool size reset after block mined...")
         self.check_mempool_sizes(0)
         zaddr4 = self.nodes[0].z_getnewaddress('sapling')
-        opid4 = self.nodes[0].z_sendmany(zaddr1, [{"address": zaddr4, "amount": Decimal('10.0') - 2*conventional_fee(2)}], 1)
+        opid4 = self.nodes[0].z_sendmany(zaddr1, [{"address": zaddr4, "amount": Decimal('10.0') - 2*fee}], 1)
         wait_and_assert_operationid_status(self.nodes[0], opid4)
-        opid5 = self.nodes[0].z_sendmany(zaddr2, [{"address": zaddr4, "amount": Decimal('10.0') - 2*conventional_fee(2)}], 1)
+        opid5 = self.nodes[0].z_sendmany(zaddr2, [{"address": zaddr4, "amount": Decimal('10.0') - 2*fee}], 1)
         wait_and_assert_operationid_status(self.nodes[0], opid5)
         self.sync_all()
 
diff --git a/src/gtest/test_mempoollimit.cpp b/src/gtest/test_mempoollimit.cpp
index 3b47497e077..fb08bb1156a 100644
--- a/src/gtest/test_mempoollimit.cpp
+++ b/src/gtest/test_mempoollimit.cpp
@@ -11,6 +11,8 @@
 #include "util/time.h"
 #include "util/test.h"
 #include "transaction_builder.h"
+#include "zip317.h"
+#include "core_memusage.h"
 
 
 const uint256 TX_ID1 = ArithToUint256(1);
@@ -89,18 +91,18 @@ TEST(MempoolLimitTests, MempoolLimitTxSetCheckSizeAfterDropping)
         MempoolLimitTxSet limitSet(MIN_TX_COST * 2);
         EXPECT_EQ(0, limitSet.getTotalWeight());
         limitSet.add(TX_ID1, MIN_TX_COST, MIN_TX_COST);
-        EXPECT_EQ(4000, limitSet.getTotalWeight());
+        EXPECT_EQ(10000, limitSet.getTotalWeight());
         limitSet.add(TX_ID2, MIN_TX_COST, MIN_TX_COST);
-        EXPECT_EQ(8000, limitSet.getTotalWeight());
+        EXPECT_EQ(20000, limitSet.getTotalWeight());
         EXPECT_FALSE(limitSet.maybeDropRandom().has_value());
         limitSet.add(TX_ID3, MIN_TX_COST, MIN_TX_COST + LOW_FEE_PENALTY);
-        EXPECT_EQ(12000 + LOW_FEE_PENALTY, limitSet.getTotalWeight());
+        EXPECT_EQ(30000 + LOW_FEE_PENALTY, limitSet.getTotalWeight());
         std::optional<uint256> drop = limitSet.maybeDropRandom();
         ASSERT_TRUE(drop.has_value());
         uint256 txid = drop.value();
         testedDropping.insert(txid);
         // Do not continue to test if a particular trial fails
-        ASSERT_EQ(txid == TX_ID3 ? 8000 : 8000 + LOW_FEE_PENALTY, limitSet.getTotalWeight());
+        ASSERT_EQ(txid == TX_ID3 ? 20000 : 20000 + LOW_FEE_PENALTY, limitSet.getTotalWeight());
     }
 }
 
@@ -113,7 +115,8 @@ TEST(MempoolLimitTests, MempoolCostAndEvictionWeight)
     auto consensusParams = RegtestActivateSapling();
 
     auto sk = libzcash::SaplingSpendingKey::random();
-    auto testNote = GetTestSaplingNote(sk.default_address(), 50000);
+    CAmount funds = 66000;
+    auto testNote = GetTestSaplingNote(sk.default_address(), funds);
 
     // Default fee
     {
@@ -121,7 +124,7 @@ TEST(MempoolLimitTests, MempoolCostAndEvictionWeight)
         builder.AddSaplingSpend(sk.expanded_spending_key(), testNote.note, testNote.tree.root(), testNote.tree.witness());
         builder.AddSaplingOutput(sk.full_viewing_key().ovk, sk.default_address(), 25000, {});
 
-        auto [cost, evictionWeight] = MempoolCostAndEvictionWeight(builder.Build().GetTxOrThrow(), DEFAULT_FEE);
+        auto [cost, evictionWeight] = MempoolCostAndEvictionWeight(builder.Build().GetTxOrThrow(), MINIMUM_FEE);
         EXPECT_EQ(MIN_TX_COST, cost);
         EXPECT_EQ(MIN_TX_COST, evictionWeight);
     }
@@ -131,10 +134,10 @@ TEST(MempoolLimitTests, MempoolCostAndEvictionWeight)
         auto builder = TransactionBuilder(consensusParams, 1, std::nullopt);
         builder.AddSaplingSpend(sk.expanded_spending_key(), testNote.note, testNote.tree.root(), testNote.tree.witness());
         builder.AddSaplingOutput(sk.full_viewing_key().ovk, sk.default_address(), 25000, {});
-        static_assert(DEFAULT_FEE == 1000);
-        builder.SetFee(DEFAULT_FEE-1);
+        static_assert(MINIMUM_FEE == 10000);
+        builder.SetFee(MINIMUM_FEE-1);
 
-        auto [cost, evictionWeight] = MempoolCostAndEvictionWeight(builder.Build().GetTxOrThrow(), DEFAULT_FEE-1);
+        auto [cost, evictionWeight] = MempoolCostAndEvictionWeight(builder.Build().GetTxOrThrow(), MINIMUM_FEE-1);
         EXPECT_EQ(MIN_TX_COST, cost);
         EXPECT_EQ(MIN_TX_COST + LOW_FEE_PENALTY, evictionWeight);
     }
@@ -143,18 +146,34 @@ TEST(MempoolLimitTests, MempoolCostAndEvictionWeight)
     {
         auto builder = TransactionBuilder(consensusParams, 1, std::nullopt);
         builder.AddSaplingSpend(sk.expanded_spending_key(), testNote.note, testNote.tree.root(), testNote.tree.witness());
-        builder.AddSaplingOutput(sk.full_viewing_key().ovk, sk.default_address(), 5000, {});
-        builder.AddSaplingOutput(sk.full_viewing_key().ovk, sk.default_address(), 5000, {});
-        builder.AddSaplingOutput(sk.full_viewing_key().ovk, sk.default_address(), 5000, {});
-        builder.AddSaplingOutput(sk.full_viewing_key().ovk, sk.default_address(), 5000, {});
+        for (int i = 0; i < 10; i++) {
+            builder.AddSaplingOutput(sk.full_viewing_key().ovk, sk.default_address(), 1000, {});
+        }
 
         auto result = builder.Build();
         if (result.IsError()) {
             std::cerr << result.GetError() << std::endl;
         }
-        auto [cost, evictionWeight] = MempoolCostAndEvictionWeight(result.GetTxOrThrow(), DEFAULT_FEE);
-        EXPECT_EQ(5168, cost);
-        EXPECT_EQ(5168, evictionWeight);
+        // max(1 input, 10 outputs + 1 change output) => 11 logical actions.
+        CAmount zip317_fee = CalculateConventionalFee(11);
+        ASSERT_GT(funds, 1000*10 + zip317_fee);
+        const CTransaction tx {result.GetTxOrThrow()};
+        EXPECT_EQ(11, tx.GetLogicalActionCount());
+
+        // For the test to be valid, we want the memory usage of this transaction to be more than MIN_TX_COST.
+        // Avoid hard-coding the usage because it might be platform-dependent.
+        ASSERT_GT(GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION), MIN_TX_COST);
+        size_t tx_usage = RecursiveDynamicUsage(tx);
+        EXPECT_GT(tx_usage, MIN_TX_COST);
+
+        auto [cost, evictionWeight] = MempoolCostAndEvictionWeight(tx, zip317_fee);
+        EXPECT_EQ(tx_usage, cost);
+        EXPECT_EQ(tx_usage, evictionWeight);
+
+        // If we pay less than the conventional fee for 11 actions, we should incur a low fee penalty.
+        auto [cost2, evictionWeight2] = MempoolCostAndEvictionWeight(tx, zip317_fee-1);
+        EXPECT_EQ(tx_usage, cost2);
+        EXPECT_EQ(tx_usage + LOW_FEE_PENALTY, evictionWeight2);
     }
 
     RegtestDeactivateSapling();