Optimise coin-age priority by splitting up steps to calculate it

    double priority = GetPriority(tx, view, height, ICIV);
becomes
    const double coin_age = GetCoinAge(tx, view, height, ICIV);
    const auto mod_vsize = CalculateModifiedSize(tx);
    const double priority = ComputePriority2(coin_age, mod_vsize);

Author: luke-jr

src/bench/mempool_stress.cpp

@@ -16,13 +16,13 @@
 static void AddTx(const CTransactionRef& tx, CTxMemPool& pool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, pool.cs)
 {
     int64_t nTime = 0;
-    const double coinage_priority = 10.0;
+    constexpr double coin_age{10.0};
     unsigned int nHeight = 1;
     uint64_t sequence = 0;
     bool spendsCoinbase = false;
     unsigned int sigOpCost = 4;
     LockPoints lp;
-    pool.addUnchecked(CTxMemPoolEntry(tx, 1000, nTime, coinage_priority, nHeight, sequence, tx->GetValueOut(), spendsCoinbase, sigOpCost, lp));
+    pool.addUnchecked(CTxMemPoolEntry(tx, 1000, nTime, nHeight, sequence, /*entry_tx_inputs_coin_age=*/coin_age, tx->GetValueOut(), spendsCoinbase, sigOpCost, lp));
 }
 
 struct Available {

src/bench/rpc_mempool.cpp

@@ -17,7 +17,7 @@
 static void AddTx(const CTransactionRef& tx, const CAmount& fee, CTxMemPool& pool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, pool.cs)
 {
     LockPoints lp;
-    pool.addUnchecked(CTxMemPoolEntry(tx, fee, /*time=*/0, /*entry_priority=*/0, /*entry_height=*/0, /*entry_sequence=*/0, /*in_chain_input_value=*/0, /*spends_coinbase=*/false, /*sigops_cost=*/4, lp));
+    pool.addUnchecked(CTxMemPoolEntry(tx, fee, /*time=*/0, /*entry_height=*/0, /*entry_sequence=*/0, /*entry_tx_inputs_coin_age=*/0.0, /*in_chain_input_value=*/0, /*spends_coinbase=*/false, /*sigops_cost=*/4, lp));
 }
 
 static void RpcMempool(benchmark::Bench& bench)

src/kernel/mempool_entry.h

@@ -86,14 +86,14 @@ class CTxMemPoolEntry
     const size_t nUsageSize;        //!< ... and total memory usage
     const int64_t nTime;            //!< Local time when entering the mempool
     const uint64_t entry_sequence;  //!< Sequence number used to determine whether this transaction is too recent for relay
+    const int64_t sigOpCost;        //!< Total sigop cost
+    const size_t nModSize;          //!< Cached modified size for priority
     const double entryPriority;     //!< Priority when entering the mempool
     const unsigned int entryHeight; //!< Chain height when entering the mempool
     double cachedPriority;          //!< Last calculated priority
     unsigned int cachedHeight;      //!< Height at which priority was last calculated
     CAmount inChainInputValue;      //!< Sum of all txin values that are already in blockchain
     const bool spendsCoinbase;      //!< keep track of transactions that spend a coinbase
-    const int64_t sigOpCost;        //!< Total sigop cost
-    const size_t nModSize;          //!< Cached modified size for priority
     CAmount m_modified_fee;         //!< Used for determining the priority of the transaction for mining in a block
     mutable LockPoints lockPoints;  //!< Track the height and time at which tx was final
 
@@ -114,7 +114,8 @@ class CTxMemPoolEntry
 
 public:
     CTxMemPoolEntry(const CTransactionRef& tx, CAmount fee,
-                    int64_t time, double entry_priority, unsigned int entry_height, uint64_t entry_sequence,
+                    int64_t time, unsigned int entry_height, uint64_t entry_sequence,
+                    double entry_tx_inputs_coin_age,
                     CAmount in_chain_input_value,
                     bool spends_coinbase,
                     int64_t sigops_cost, LockPoints lp)
@@ -124,15 +125,15 @@ class CTxMemPoolEntry
           nUsageSize{RecursiveDynamicUsage(tx)},
           nTime{time},
           entry_sequence{entry_sequence},
-          entryPriority{entry_priority},
+          sigOpCost{sigops_cost},
+          nModSize{CalculateModifiedSize(*tx, GetTxSize())},
+          entryPriority{ComputePriority2(entry_tx_inputs_coin_age, nModSize)},
           entryHeight{entry_height},
-          cachedPriority{entry_priority},
+          cachedPriority{entryPriority},
           // Since entries arrive *after* the tip's height, their entry priority is for the height+1
           cachedHeight{entry_height + 1},
           inChainInputValue{in_chain_input_value},
           spendsCoinbase{spends_coinbase},
-          sigOpCost{sigops_cost},
-          nModSize{CalculateModifiedSize(*tx, GetTxSize())},
           m_modified_fee{nFee},
           lockPoints{lp},
           nSizeWithDescendants{GetTxSize()},

src/policy/coin_age_priority.cpp

@@ -11,6 +11,7 @@
 #include <policy/policy.h>
 #include <primitives/transaction.h>
 #include <txmempool.h>
+#include <util/check.h>
 #include <validation.h>
 
 using node::BlockAssembler;
@@ -22,8 +23,7 @@ unsigned int CalculateModifiedSize(const CTransaction& tx, unsigned int nTxSize)
     // is enough to cover a compressed pubkey p2sh redemption) for priority.
     // Providing any more cleanup incentive than making additional inputs free would
     // risk encouraging people to create junk outputs to redeem later.
-    if (nTxSize == 0)
-        nTxSize = (GetTransactionWeight(tx) + WITNESS_SCALE_FACTOR - 1) / WITNESS_SCALE_FACTOR;
+    Assert(nTxSize > 0);
     for (std::vector<CTxIn>::const_iterator it(tx.vin.begin()); it != tx.vin.end(); ++it)
     {
         unsigned int offset = 41U + std::min(110U, (unsigned int)it->scriptSig.size());
@@ -33,15 +33,14 @@ unsigned int CalculateModifiedSize(const CTransaction& tx, unsigned int nTxSize)
     return nTxSize;
 }
 
-double ComputePriority(const CTransaction& tx, double dPriorityInputs, unsigned int nTxSize)
+double ComputePriority2(double inputs_coin_age, unsigned int mod_vsize)
 {
-    nTxSize = CalculateModifiedSize(tx, nTxSize);
-    if (nTxSize == 0) return 0.0;
+    if (mod_vsize == 0) return 0.0;
 
-    return dPriorityInputs / nTxSize;
+    return inputs_coin_age / mod_vsize;
 }
 
-double GetPriority(const CTransaction &tx, const CCoinsViewCache& view, int nHeight, CAmount &inChainInputValue)
+double GetCoinAge(const CTransaction &tx, const CCoinsViewCache& view, int nHeight, CAmount &inChainInputValue)
 {
     inChainInputValue = 0;
     if (tx.IsCoinBase())
@@ -58,7 +57,7 @@ double GetPriority(const CTransaction &tx, const CCoinsViewCache& view, int nHei
             inChainInputValue += coin.out.nValue;
         }
     }
-    return ComputePriority(tx, dResult);
+    return dResult;
 }
 
 void CTxMemPoolEntry::UpdateCachedPriority(unsigned int currentHeight, CAmount valueInCurrentBlock)

src/policy/coin_age_priority.h

@@ -10,17 +10,19 @@
 class CCoinsViewCache;
 class CTransaction;
 
-// Compute modified tx size for priority calculation (optionally given tx size)
-unsigned int CalculateModifiedSize(const CTransaction& tx, unsigned int nTxSize=0);
+// Compute modified tx vsize for priority calculation
+unsigned int CalculateModifiedSize(const CTransaction& tx, unsigned int nTxSize);
 
-// Compute priority, given sum coin-age of inputs and (optionally) tx size
-double ComputePriority(const CTransaction& tx, double dPriorityInputs, unsigned int nTxSize=0);
+// Compute priority, given sum coin-age of inputs and modified tx vsize
+// CAUTION: Original ComputePriority accepted UNMODIFIED tx vsize and did the modification internally
+double ComputePriority2(double inputs_coin_age, unsigned int mod_vsize);
 
 /**
- * Return priority of tx at height nHeight. Also calculate the sum of the values of the inputs
+ * Return sum coin-age of tx inputs at height nHeight. Also calculate the sum of the values of the inputs
  * that are already in the chain.  These are the inputs that will age and increase priority as
  * new blocks are added to the chain.
+ * CAUTION: Original GetPriority also called ComputePriority and returned the final coin-age priority
  */
-double GetPriority(const CTransaction &tx, const CCoinsViewCache& view, int nHeight, CAmount &inChainInputValue);
+double GetCoinAge(const CTransaction &tx, const CCoinsViewCache& view, int nHeight, CAmount &inChainInputValue);
 
 #endif // BITCOIN_POLICY_COIN_AGE_PRIORITY_H

src/test/fuzz/util/mempool.cpp

@@ -35,9 +35,9 @@ CTxMemPoolEntry ConsumeTxMemPoolEntry(FuzzedDataProvider& fuzzed_data_provider,
     assert(MoneyRange(fee));
     const int64_t time = fuzzed_data_provider.ConsumeIntegral<int64_t>();
     const uint64_t entry_sequence{fuzzed_data_provider.ConsumeIntegral<uint64_t>()};
-    const double coinage_priority = fuzzed_data_provider.ConsumeFloatingPoint<double>();
+    const double coin_age = fuzzed_data_provider.ConsumeFloatingPoint<double>();
     const unsigned int entry_height = fuzzed_data_provider.ConsumeIntegralInRange<unsigned int>(0, std::numeric_limits<unsigned int>::max() - 1);
     const bool spends_coinbase = fuzzed_data_provider.ConsumeBool();
     const unsigned int sig_op_cost = fuzzed_data_provider.ConsumeIntegralInRange<unsigned int>(0, MAX_BLOCK_SIGOPS_COST);
-    return CTxMemPoolEntry{MakeTransactionRef(tx), fee, time, coinage_priority, entry_height, entry_sequence, tx.GetValueOut(), spends_coinbase, sig_op_cost, {}};
+    return CTxMemPoolEntry{MakeTransactionRef(tx), fee, time, entry_height, entry_sequence, /*entry_tx_inputs_coin_age=*/coin_age, tx.GetValueOut(), spends_coinbase, sig_op_cost, {}};
 }

src/test/util/setup_common.cpp

@@ -561,9 +561,10 @@ std::vector<CTransactionRef> TestChain100Setup::PopulateMempool(FastRandomContex
             LOCK2(cs_main, m_node.mempool->cs);
             LockPoints lp;
             CAmount in_chain_input_value;
-            double dPriority = GetPriority(*ptx, active_chainstate.CoinsTip(), height + 1, in_chain_input_value);
+            const double coin_age = GetCoinAge(*ptx, active_chainstate.CoinsTip(), height + 1, in_chain_input_value);
             m_node.mempool->addUnchecked(CTxMemPoolEntry(ptx, /*fee=*/(total_in - num_outputs * amount_per_output),
-                                                         /*time=*/0, /*entry_priority=*/ dPriority, /*entry_height=*/ height, /*entry_sequence=*/0,
+                                                         /*time=*/0, /*entry_height=*/ height, /*entry_sequence=*/0,
+                                                         /*entry_tx_inputs_coin_age=*/coin_age,
                                                          in_chain_input_value,
                                                          /*spends_coinbase=*/false, /*sigops_cost=*/4, lp));
         }
@@ -594,7 +595,8 @@ void TestChain100Setup::MockMempoolMinFee(const CFeeRate& target_feerate)
     const auto tx_fee = target_feerate.GetFee(GetVirtualTransactionSize(*tx)) -
         m_node.mempool->m_opts.incremental_relay_feerate.GetFee(GetVirtualTransactionSize(*tx));
     m_node.mempool->addUnchecked(CTxMemPoolEntry(tx, /*fee=*/tx_fee,
-                                                 /*time=*/0, /*entry_priority=*/0, /*entry_height=*/1, /*entry_sequence=*/0,
+                                                 /*time=*/0, /*entry_height=*/1, /*entry_sequence=*/0,
+                                                 /*entry_tx_inputs_coin_age=*/0.0,
                                                  /*in_chain_input_value=*/0,
                                                  /*spends_coinbase=*/true, /*sigops_cost=*/1, lp));
     m_node.mempool->TrimToSize(0);

src/test/util/txmempool.cpp

@@ -37,9 +37,9 @@ CTxMemPoolEntry TestMemPoolEntryHelper::FromTx(const CMutableTransaction& tx) co
 
 CTxMemPoolEntry TestMemPoolEntryHelper::FromTx(const CTransactionRef& tx) const
 {
-    const double dPriority = 0;
+    constexpr double coin_age{0};
     const CAmount inChainValue = 0;
-    return CTxMemPoolEntry{tx, nFee, TicksSinceEpoch<std::chrono::seconds>(time), dPriority, nHeight, m_sequence, inChainValue, spendsCoinbase, sigOpCost, lp};
+    return CTxMemPoolEntry{tx, nFee, TicksSinceEpoch<std::chrono::seconds>(time), nHeight, m_sequence, /*entry_tx_inputs_coin_age=*/coin_age, inChainValue, spendsCoinbase, sigOpCost, lp};
 }
 
 std::optional<std::string> CheckPackageMempoolAcceptResult(const Package& txns,

src/txmempool.cpp

@@ -677,7 +677,9 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei
     for (const auto& it : GetSortedDepthAndScore()) {
         checkTotal += it->GetTxSize();
         CAmount dummyValue;
-        double freshPriority = GetPriority(it->GetTx(), active_coins_tip, spendheight, dummyValue);
+        const double fresh_coin_age = GetCoinAge(it->GetTx(), active_coins_tip, spendheight, dummyValue);
+        const auto fresh_mod_vsize = CalculateModifiedSize(it->GetTx(), it->GetTxSize());
+        const double freshPriority = ComputePriority2(fresh_coin_age, fresh_mod_vsize);
         double cachePriority = it->GetPriority(spendheight);
         double priDiff = cachePriority > freshPriority ? cachePriority - freshPriority : freshPriority - cachePriority;
         // Verify that the difference between the on the fly calculation and a fresh calculation

src/validation.cpp

@@ -938,7 +938,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
 
     CAmount inChainInputValue;
     // Since entries arrive *after* the tip's height, their priority is for the height+1
-    double dPriority = GetPriority(tx, m_view, m_active_chainstate.m_chain.Height() + 1, inChainInputValue);
+    const double coin_age = GetCoinAge(tx, m_view, m_active_chainstate.m_chain.Height() + 1, inChainInputValue);
 
     // Keep track of transactions that spend a coinbase, which we re-scan
     // during reorgs to ensure COINBASE_MATURITY is still met.
@@ -954,7 +954,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
     // Set entry_sequence to 0 when rejectmsg_zero_mempool_entry_seq is used; this allows txs from a block
     // reorg to be marked earlier than any child txs that were already in the mempool.
     const uint64_t entry_sequence = args.m_ignore_rejects.count(rejectmsg_zero_mempool_entry_seq) ? 0 : m_pool.GetSequence();
-    entry.reset(new CTxMemPoolEntry(ptx, ws.m_base_fees, nAcceptTime, dPriority, m_active_chainstate.m_chain.Height(), entry_sequence,
+    entry.reset(new CTxMemPoolEntry(ptx, ws.m_base_fees, nAcceptTime, m_active_chainstate.m_chain.Height(), entry_sequence,
+                                    /*entry_tx_inputs_coin_age=*/coin_age,
                                     inChainInputValue,
                                     fSpendsCoinbase, nSigOpsCost, lock_points.value()));
     ws.m_vsize = entry->GetTxSize();