update trace_filter, trace block only to highest matching index by smatthewenglish · Pull Request #4974 · paradigmxyz/reth

smatthewenglish · 2023-10-10T19:45:49Z

This PR aims to address the following TODO:

// TODO: this could be optimized to only trace the block until the highest matching index in
// that block

This is my first contribution to reth, I've read through the CONTRIBUTING.md doc and executed all the specified checks. If there's anything I've overlooked or missed please let me know and I'll be happy to address it.

mattsse

that's a great start.

I have some nits,
the biggest improvement would still be to only trace the block until the highest index, for that we'd need a new function that is similar to:
https://github.com/paradigmxyz/reth/blob/main/crates/rpc/rpc/src/trace.rs#L363-L369
pub only traces the block until a given transaction,
we can unify some code by moving this loop to a standalone function so it can be reused:

https://github.com/paradigmxyz/reth/blob/main/crates/rpc/rpc/src/trace.rs#L408-L408

mattsse · 2023-10-11T10:48:23Z

crates/rpc/rpc/src/trace.rs

+                    if idx > highest_matching_index {
+                        highest_matching_index = idx;
+                    }


this check is redundant because all tx_idx are only increasing

mattsse · 2023-10-11T10:50:08Z

crates/rpc/rpc/src/trace.rs

+                        if idx > highest_idx {
+                            return Ok(None);
+                        }


this is a smol improvement, however this still traces the entire block,
https://github.com/paradigmxyz/reth/blob/main/crates/rpc/rpc/src/trace.rs#L353-L362

mattsse · 2023-10-11T10:51:09Z

crates/rpc/rpc/src/trace.rs

        let mut block_traces = Vec::with_capacity(target_blocks.len());
-        for (num, indices) in target_blocks {
+        for (num, indices, highest_idx) in target_blocks {
            let traces = self.trace_block_with(


there's something else we need to do here:
skipping blocks with empty indices entirely

smatthewenglish · 2023-10-12T01:27:11Z

i wanted to localize this code in one place, instead of defining it twice, once for each function/

F: for<'a> Fn(
        TransactionInfo,
        TracingInspector,
        ExecutionResult,
        &'a revm_primitives::State,
        &'a CacheDB<StateProviderDatabase<StateProviderBox<'a>>>,
    ) -> EthResult<R>
    + Send
    + 'static,

so i defined it like this at the top of the file

trait TransactionCallback<R> 
where
    Self: for<'a> Fn(
        TransactionInfo,
        TracingInspector,
        ExecutionResult,
        &'a State,
        &'a CacheDB<StateProviderDatabase<StateProviderBox<'a>>>,
    ) -> EthResult<R>
    + Send
    + 'static,
{
}

impl<F, R> TransactionCallback<R> for F
where
    F: for<'a> Fn(
        TransactionInfo,
        TracingInspector,
        ExecutionResult,
        &'a State,
        &'a CacheDB<StateProviderDatabase<StateProviderBox<'a>>>,
    ) -> EthResult<R>
    + Send
    + 'static,
{
}

but i think that's still pretty redundant. I'm not that familiar with rust, it's why I wanna become a contributor to this project, to learn rust. is there a more idiomatic way to do that^?

mattsse

nice, almost there

mattsse · 2023-10-12T10:32:54Z

crates/rpc/rpc/src/trace.rs

+                    // Check if current index exceeds the highest_index and break if it does
+                    if let Some(highest) = highest_index {
+                        if idx as u64 > highest {
+                            break;
+                        }
+                    }


I'd like to move this to transactions.into_iter().take() instead

mattsse · 2023-10-12T10:35:24Z

crates/rpc/rpc/src/trace.rs

                        .into_localized_transaction_traces(tx_info);
                    Ok(Some(traces))
                },
+                Some(highest_idx),


this argument should be moved and should be the second argument, so that the closure is always the last argument

codecov · 2023-10-12T13:59:10Z

Codecov Report

Merging #4974 (d50470b) into main (67773ad) will increase coverage by 0.16%.
Report is 22 commits behind head on main.
The diff coverage is 67.64%.

Files	Coverage Δ
crates/rpc/rpc/src/trace.rs	`43.67% <67.64%> (+1.77%)`	⬆️

... and 65 files with indirect coverage changes

Flag	Coverage Δ
integration-tests	`15.87% <67.64%> (+0.60%)`	⬆️
unit-tests	`62.43% <0.00%> (-0.11%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
reth binary	`30.61% <ø> (-0.15%)`	⬇️
blockchain tree	`80.81% <ø> (+0.18%)`	⬆️
pipeline	`88.37% <ø> (-0.09%)`	⬇️
storage (db)	`74.40% <ø> (-0.01%)`	⬇️
trie	`94.44% <ø> (-0.01%)`	⬇️
txpool	`48.66% <ø> (+0.13%)`	⬆️
networking	`76.44% <ø> (+0.35%)`	⬆️
rpc	`57.99% <67.64%> (-0.06%)`	⬇️
consensus	`63.01% <ø> (ø)`
revm	`27.89% <ø> (+0.10%)`	⬆️
payload builder	`7.96% <ø> (ø)`
primitives	`86.31% <ø> (+0.88%)`	⬆️

mattsse · 2023-10-12T14:06:22Z

crates/rpc/rpc/src/trace.rs

                let mut db = CacheDB::new(StateProviderDatabase::new(state));

-                let mut transactions = transactions.into_iter().enumerate().peekable();
+                let max_transactions = highest_index.map_or(transactions.len(), |highest| highest as usize + 1);


this looks wrong, we should not need +1

mattsse · 2023-10-12T14:06:45Z

crates/rpc/rpc/src/trace.rs

-                    // need to apply the state changes of this transaction before executing the
-                    // next transaction
-                    if transactions.peek().is_some() {
-                        // need to apply the state changes of this transaction before executing
-                        // the next transaction
-                        db.commit(state)
-                    }


we still need this

mattsse · 2023-10-12T15:56:25Z

crates/rpc/rpc/src/trace.rs

+trait TransactionCallback<R>
+where
+    Self: for<'a> Fn(
+            TransactionInfo,
+            TracingInspector,
+            ExecutionResult,
+            &'a State,
+            &'a CacheDB<StateProviderDatabase<StateProviderBox<'a>>>,
+        ) -> EthResult<R>
+        + Send
+        + 'static,
+{
+}
+
+impl<F, R> TransactionCallback<R> for F where
+    F: for<'a> Fn(
+            TransactionInfo,
+            TracingInspector,
+            ExecutionResult,
+            &'a State,
+            &'a CacheDB<StateProviderDatabase<StateProviderBox<'a>>>,
+        ) -> EthResult<R>
+        + Send
+        + 'static
+{
+}


I don't really like this because this is just a hack to make signatures nicer and not really needed

mattsse

nice, lgtm

Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>

only trace the block until the highest matching index in that block

833787a

smatthewenglish requested review from Rjected and mattsse as code owners October 10, 2023 19:45

mattsse added C-perf A change motivated by improving speed, memory usage or disk footprint A-rpc Related to the RPC implementation labels Oct 11, 2023

mattsse requested changes Oct 11, 2023

View reviewed changes

smatthewenglish added 9 commits October 11, 2023 16:41

wip

9af97e0

update

3b7c2fb

update

b7fbe2d

update

c7c2c19

update

6a7d680

update

8404056

update

c5fcc5b

update

15b20b9

update

f6eab16

mattsse requested changes Oct 12, 2023

View reviewed changes

smatthewenglish added 2 commits October 12, 2023 09:46

update

2f96b93

update

472cb14

update

5081797

mattsse reviewed Oct 12, 2023

View reviewed changes

remove fmt

ee82515

mattsse reviewed Oct 12, 2023

View reviewed changes

smatthewenglish added 3 commits October 12, 2023 10:08

update

5c05aa2

update

d2c94d2

fix

0e85a9c

mattsse requested changes Oct 12, 2023

View reviewed changes

smatthewenglish added 2 commits October 12, 2023 12:20

fmt

17e88aa

update

d43b264

smatthewenglish and others added 3 commits October 12, 2023 12:23

update

caf96fc

update

5dd5712

docs: some docs

d50470b

mattsse approved these changes Oct 12, 2023

View reviewed changes

mattsse enabled auto-merge October 12, 2023 18:33

mattsse added this pull request to the merge queue Oct 12, 2023

mattsse mentioned this pull request Oct 12, 2023

Move trace_block_with and trace_block_until to EthTransactions trait #5002

Closed

Merged via the queue into paradigmxyz:main with commit d3794f2 Oct 12, 2023

mattsse added a commit that referenced this pull request Nov 8, 2023

update trace_filter, trace block only to highest matching index (#4974)

55b0cf0

Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>

Conversation

smatthewenglish commented Oct 10, 2023

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smatthewenglish commented Oct 12, 2023

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Oct 12, 2023 •

edited

Loading