feat: implemented a configurable recovered-transaction LRU cache in the network transactions manager.

[MatusKysel]

Key changes:

• Added DEFAULT_RECOVERED_TX_CACHE_SIZE and recovered_tx_cache_size knob to TransactionsManagerConfig (crates/net/network/src/transactions/mod.rs, crates/net/network/src/transactions/config.rs), and wired the cache into import_transactions, with a zero size disabling caching.
• Stored recovered transactions in an LRU (recovered_txs) to avoid repeated signature recovery; cache size is set from config and respected during recovery.
• Exposed the default cache size through node args config (crates/node/core/src/args/network.rs).
• Added a regression test ensuring duplicate pooled transactions only populate the recovered cache once (test_recovered_tx_cache_stores_recovered_once in crates/net/network/src/transactions/mod.rs).

[Copilot]

Pull request overview

This PR implements a configurable LRU cache for recovered transactions in the network transactions manager to avoid repeated expensive signature recovery operations for duplicate transactions. The cache is integrated into the transaction import path with a default size of 32,768 entries and can be disabled by setting the size to zero.

Key changes:

• Added DEFAULT_RECOVERED_TX_CACHE_SIZE constant (32,768) and recovered_tx_cache_size configuration field to TransactionsManagerConfig
• Modified import_transactions to check the cache before performing signature recovery, storing newly recovered transactions for subsequent lookups
• Exposed the cache configuration through node arguments with the default value

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File	Description
crates/node/core/src/args/network.rs	Exposes DEFAULT_RECOVERED_TX_CACHE_SIZE and sets it in the network configuration builder
crates/net/network/src/transactions/mod.rs	Adds the recovered transaction cache field, implements caching logic in import_transactions, and includes a regression test
crates/net/network/src/transactions/config.rs	Adds recovered_tx_cache_size field to TransactionsManagerConfig with serde support

Critical Issues Found:

1. The cache initialization uses .max(1) which prevents zero from disabling the cache as intended
2. The serde default attribute syntax is incorrect and points to a constant instead of a function

Moderate Issues Found:

1. Test coverage doesn't verify that signature recovery only happens once
2. Duplicated error handling code between cache-enabled and cache-disabled paths
3. Performance could be improved by using Arc<Recovered<...>> to avoid cloning transaction data

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The error handling code for signature recovery failure is duplicated between the cache-disabled path (lines 1385-1396) and the cache-enabled path (lines 1404-1419). Consider extracting the recovery and error handling logic into a helper method or closure to reduce duplication and improve maintainability.

Example refactoring:

let recover_tx = |tx: N::PooledTransaction| {
    #[cfg(test)]
    RECOVER_INVOCATIONS.fetch_add(1, Ordering::Relaxed);
    match tx.try_into_recovered() {
        Ok(tx) => Some(Arc::new(tx)),
        Err(badtx) => {
            trace!(target: "net::tx",
                peer_id=format!("{peer_id:#}"),
                hash=%badtx.tx_hash(),
                client_version=%peer.client_version,
                "failed ecrecovery for transaction"
            );
            has_bad_transactions = true;
            None
        }
    }
};

let tx = if self.config.recovered_tx_cache_size == 0 {
    recover_tx(tx)?
} else {
    match self.recovered_txs.get(&tx_hash) {
        Some(cached) => cached.clone(),
        None => {
            let tx = recover_tx(tx)?;
            _ = self.recovered_txs.insert(tx_hash, tx.clone());
            tx
        }
    }
};
// Then use `?` to skip on None or convert to continue in the loop

[Copilot]

The test manually sets tx_manager.recovered_txs = LruMap::new(1) but doesn't update tx_manager.config.recovered_tx_cache_size to match. While this doesn't break the test (since the config value is only checked for == 0), it creates an inconsistency between the actual cache size and the config value. Consider also setting tx_manager.config.recovered_tx_cache_size = 1; for better test clarity and to ensure the cache size check at line 1382 is consistent with the actual cache.

Suggested change

	tx_manager.recovered_txs = LruMap::new(1);
	tx_manager.recovered_txs = LruMap::new(1);
	tx_manager.config.recovered_tx_cache_size = 1;