refactor: Execution Stage owns Executor#1568
Conversation
bin/reth/src/chain/import.rs
Outdated
There was a problem hiding this comment.
This DefaultDB thing is the only thing I don't like. It's necessary because when you instantiate the stage it still doesn't know the type of the database we'll use. It'd be nice if we can avoid it.
There was a problem hiding this comment.
It should be possible to avoid this if we get a SubState type that expires part of the cache (in a correct way) since we would be able to instantiate that once as well. For now this has to do.
There was a problem hiding this comment.
this looks like a redundant call to make on each iteration. shouldn't this be above along with state provider initialization?
There was a problem hiding this comment.
I think the primary issue rn is that SubState is just a type alias for some wrapper around a StateProvider and revm's CacheDB, so there is no expiry/rotation of the cache. If you executed from block 0 to 10 million you would end up having the entire state in memory, so for now, we have to do this I guess
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #1568 +/- ##
==========================================
- Coverage 74.91% 74.84% -0.08%
==========================================
Files 373 373
Lines 43939 43946 +7
==========================================
- Hits 32917 32891 -26
- Misses 11022 11055 +33
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
onbjerg
left a comment
onbjerg
left a comment
There was a problem hiding this comment.
Feels like error is unrelated?
onbjerg
added
C-debt
gakonst
left a comment
gakonst
left a comment
There was a problem hiding this comment.
Nope sorry good to go.
before, we were only storing a chainspec and not an executor, which resulted in a weird API with functions with a lot of arguments..this refactors the stage to be able to be constructed from an executor or a chainspec. this in turn will allow us to instantiate a stage with an executor that e.g. is loaded with an inspector and have it be used across all transactions
onbjerg
force-pushed
the
refactor/stage-executor
branch
from
84bee3a to
ad8a495
Compare
|
thx for getting over the line bjerg - my bad for dropping the ball here |
4 participants
Before, we had a weird pattern IMO where we'd call a
execut_and_verify_with_receiptsfunction with 5 arguments, and instantiating a new executor each time. That would make it really ugly if we wanted to integrate with the feature from PR #1567 as it'd require that we pass around the configuration for the inspectors. Instead, in this PR we refactor it so that the ExecutionStage now owns an Executor. It makes more sense this way, as we want the stage to basically be a coordinator around the 1-2 functions of the executor that are used across blocks (vs tracking more data than that).