perf(tx-pool): avoid copying tx cost

[hai-rise]

Apparently, copying the 32-byte cost of existing mempool transactions can be a bottleneck when adding new ones. It is in the hot path when there are bots with many transactions such as popular delegated EIP-7702 addresses, AI agents, etc.

This PR makes PoolTransaction::cost return a reference to avoid a copy when we only compare U256s. This is similar to PoolTransaction::hash returning a &TxHash instead of a copied TxHash.

[mattsse]

the cost fn for mocktx changed, we could use a LazyLock here? or change how this is updated

was this profiled with mocktx are ethpooltx?

[hai-rise]

the cost fn for mocktx changed, we could use a LazyLock here? or change how this is updated

I actually started with Box::leak for MockTransaction::cost and thought that was too much voodoo. Now I want to go back as adding cost to MockTransaction was already a bit much, adding update_cost to all price-update functions like inc_price_by to pass tests would be ugly.

--- a/crates/transaction-pool/src/test_utils/mock.rs
+++ b/crates/transaction-pool/src/test_utils/mock.rs
@@ -593,8 +593,11 @@ impl PoolTransaction for MockTransaction {
         *self.get_nonce()
     }

-    fn cost(&self) -> U256 {
-        match self {
+    fn cost(&self) -> &U256 {
+        // This leak is only acceptable for testing. A practical implementation should
+        // store an immutable cost instead of calculating it at query time, which only
+        // makes sense here as we mutate gas data after construction for different tests.
+        Box::leak(Box::new(match self {
             Self::Legacy { gas_price, value, gas_limit, .. } |
             Self::Eip2930 { gas_limit, gas_price, value, .. } => {
                 U256::from(*gas_limit) * U256::from(*gas_price) + *value
@@ -603,7 +606,7 @@ impl PoolTransaction for MockTransaction {
             Self::Eip4844 { max_fee_per_gas, value, gas_limit, .. } => {
                 U256::from(*gas_limit) * U256::from(*max_fee_per_gas) + *value
             }
-        }
+        }))
     }

What do you think?

was this profiled with mocktx are ethpooltx?

Ah, this was ~200 accounts constantly submitting 10M transactions to an L2 devnet. To hit 100k TPS throughput we must first add 100k transactions to the mempool every second!

[mattsse]

What do you think?

I think this can be solved with OnceLock where the init fn is the old cost fn impl

[hai-rise]

I think this can be solved with OnceLock where the init fn is the old cost fn impl

That could work if cost is only ever called once. Otherwise, it has this hidden side effect:

let first = MockTransaction::eip1559().inc_price().inc_limit();
dbg!(first.cost()); // 8
let second = first.inc_price().inc_limit();
dbg!(second.cost()); // 8 instead of 18 as inc_limit cloned the initialized cost!

Personally, I'd avoid these quirky behaviours. If gas numbers can be arbitrarily updated then cost should be as well.

[mattsse]

then the only other solution I see is always recomputing the cost when we change value,fee fields.

[hai-rise]

then the only other solution I see is always recomputing the cost when we change value,fee fields.

That is very error-prone unless we do a huge refactoring on MockTransaction to minimize mutability. I much prefer Box::leak for test code, or we can have a middle ground of making it Cow<'_, U256> instead? MockTransaction can return Owned and EthPooledTransaction can return Borrowed.

[mattsse]

I'm fine with cow

[hai-rise]

I'm fine with cow

I've pushed cow!

[hai-rise]

on second thought, this defeats the purpose entirely because cow is even larger than a u256

I really suggested using Cow for fixed-size stack allocation haha. Let me think of something brighter.

[hai-rise]

@mattsse Okay I've pushed another one. More code than wanted but it avoids Box::leak and has consistent behaviours:

• No setters for cost.
• cost is correct from tx construction and auto-updated per field mutation.