Refactor: Split transaction pre verification to separate function#573
Conversation
thedevbirb
force-pushed
the
refactor_split_tx_preverification
branch
from
f283376 to
2d68733
Compare
rakita
left a comment
rakita
left a comment
There was a problem hiding this comment.
Left comments.
And I apologise for a long wait I wanted to think about some additional things to make this more flexible but it missed me, we can push this over the line.
bins/revme/src/statetest/runner.rs
Outdated
| } else { | ||
| evm.transact_commit() | ||
| evm.preverify_transaction() | ||
| .and_then(|_| evm.transact_commit()) |
There was a problem hiding this comment.
Thanks! Now I moved it to the transact function which does preverify_transaction and then transact_preverified, according to the other comment you made
crates/revm/src/evm_impl.rs
Outdated
| Ok(()) | ||
| } | ||
|
|
||
| fn transact(&mut self) -> EVMResult<DB::Error> { |
There was a problem hiding this comment.
It will be a footgun if we silently remove some checks.
We should probably add an additional function transact_preverified to make it explicit.
There was a problem hiding this comment.
You're right! Sorry for the dumb take. Now I added the transact_preverified function in the Transact trait
thedevbirb
force-pushed
the
refactor_split_tx_preverification
branch
from
2d68733 to
9e807d0
Compare
Hi @rakita! I made some small changes according to the comments. Thanks for the feedback. I hope it is ok now. |
| fn transact(&mut self) -> EVMResult<DB::Error> { | ||
| self.env().validate_block_env::<GSPEC, DB::Error>()?; | ||
| self.env().validate_tx::<GSPEC>()?; | ||
| fn preverify_transaction(&mut self) -> Result<(), EVMError<DB::Error>> { |
There was a problem hiding this comment.
Interesting - I wonder how much time we spend doing these checks, and whether we could potentially run all these checks in parallel at the beginning of each block in Reth?
I have seen similar separations to be valuable in the context of account abstraction where you'd want to verify e.g. all signatures in parallel (we do that alrd) and then execute.
There was a problem hiding this comment.
It is negligible, check these measurments #499 (comment)
It is 250ms for 100k blocks that took 56s (on sepolia). Although tx num varies it is under 0.5% of overall the time spent.
…uealloy#573) * feat(transaction): preverify_transaction function in Transact trait * chore(transaction): implemented tx preverification and removed checks from transaction functions * chore: documentation * feat(transaction): transact_preverified function in Transact trait; updated docs
…uealloy#573) * feat(transaction): preverify_transaction function in Transact trait * chore(transaction): implemented tx preverification and removed checks from transaction functions * chore: documentation * feat(transaction): transact_preverified function in Transact trait; updated docs
3 participants
Hi! I tried to give a shot to #432 in this PR. I hope to have understood and addressed the problem indicated by the issue.