Fix: Use lock properly in filter_order_by_nonces to do it concurrently

[0xqd]

📝 Summary

The current use of Mutex on nonce_cache in filter_order_by_nonces is not correct. It causes the function to run sequentially because every other task will wait for prev one ( both read and write ) complete in order to do its work. I change to use RwLock instead. Before the change it takes 18m to fetch a block with 2k4 orders, and now it takes 18s with 400 concurrent request.

• And lint the file

💡 Motivation and Context

---

✅ I have completed the following steps:

• [x] Run make lint
• [x] Run make test
• [ ] Added tests (if applicable)

[github-actions[bot]]

Benchmark results for 465eadb

Report: https://flashbots-rbuilder-ci-stats.s3.us-east-2.amazonaws.com/benchmark/465eadb-db634fc/report/index.html

Date (UTC)	2024-07-13T16:06:03+00:00
Commit	465eadbd19a1cb2d62f8fe829e819e7db6e1a902
Base SHA	db634fcb90a6b2fc484e19f02a59a5b74ffeefa2

Significant changes

None

[0xqd]

@bertmiller spot on the analysis that there is an overwrite on the nonce value for the same address X. However, the part of getting the nonce, is a nonce of an address from parent block which should be the same regardless of the order of onchain network request to get the data.

[ZanCorDX]

Thanks!!!!! I believe the change from Mutex to RWlock is unimportant, but the great catch is not keeping the lock while calling get_transaction_count. Can you remove the is_none and the 2 unwraps in favor of an if let?

[0xqd]

Thanks!!!!! I believe the change from Mutex to RWlock is unimportant, but the great catch is not keeping the lock while calling get_transaction_count. Can you remove the is_none and the 2 unwraps in favor of an if let?

Thanks for your feedback. At the is_none() checking, we don't intend to read the value, only write if it's none and for the 2 unwrap(), the value surely is available, so I don't think we need to use if-let here, I believe.

[ZanCorDX]

Thanks!!!!! I believe the change from Mutex to RWlock is unimportant, but the great catch is not keeping the lock while calling get_transaction_count. Can you remove the is_none and the 2 unwraps in favor of an if let?

Thanks for your feedback. At the is_none() checking, we don't intend to read the value, only write if it's none and for the 2 unwrap(), the value surely is available, so I don't think we need to use if-let here, I believe.

It's just to make the code more robust, in most cases rust allows us to avoid wrap() which might fail if something changes in the future. It would be like this:

                     let res_onchain_nonce = if let Some(res_onchain_nonce) = res_onchain_nonce {
                            res_onchain_nonce
                        } else {
                            let address = nonce.address;
                            let onchain_nonce = self
                                .eth_provider
                                .get_transaction_count(address)
                                .block_id(BlockId::Number(parent_block.into()))
                                .await
                                .wrap_err("Failed to fetch onchain tx count")?;

                            if let Ok(mut nonce_cache) = nonce_cache.write() {
                                nonce_cache.entry(address).or_insert(onchain_nonce);
                            }
                            onchain_nonce
                        };
                        if res_onchain_nonce > nonce.nonce && !nonce.optional {

This way the code enforces the correctness at compile time and not at runtime just by the logic of it.

[0xqd]

it makes sense.