-
Notifications
You must be signed in to change notification settings - Fork 112
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add support for async transaction execution #1439
Conversation
@KolbyML, I didn't do any end-to-end testing to make sure that trin-execution still works. Any recommended way to do it quickly (quickly in the sense that it doesn't take days to run it)? |
if you run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks mostly good, one concern with how mix_hash
is used because the way the code is written, it wouldn't match the code before this PR in that, it would set BlockEnv::prevrandao
to Some() for pre-merge blocks, where before this PR we had a specifically set it to None
for pre-merge blocks.
Maybe this might be fine, and Revm is smart enough to ignore this, but I think we should re-add the if merge {} check for BlockEnv::prevrandao
, for sanity sake.
trin-execution/src/evm/mod.rs
Outdated
gas_limit: header.gas_limit, | ||
basefee: header.base_fee_per_gas.unwrap_or_default(), | ||
difficulty: header.difficulty, | ||
prevrandao: header.mix_hash, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prevrandao: header.mix_hash,
I believe we should still be checking if we are past the Merge for prevrandao
.
mix_hash won't be none in pre-merge blocks, it was used in PoW mining. Maybe it will be fine? but I would rather not take chances
For difficulty I think what you did is fine as the processed header should always have the right difficulty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know that header.mix_hash
won't be none pre-merge. I fixed it.
I think one of the best ways would be to tap into existing EL testing infrastructure, but that would require us to build a lot of EL infrastructure out such as the JSON-RPC, and maybe command line arguments, I haven't fully looked into the process of this. For now with our limit scope being state bridging (for the time being), just running Trin execution might be the best solution for the time being, like I gave an example for above |
I will let it run over night and merge in the morning if everything looks ok. |
It managed to process 3'981'311 blocks, but then it failed because it panicked while deserializing era1 file. Then I tried to resume execution (from the latest saved checkpoint - block 3'700'003), but that one failed. This was also not caused by this PR. I investigated it and figure out the problem (see #1440). I managed to fix the issue locally (in a bit hacky way), and I was able to continue execution. Execution is still running, it passed 4'400'000 as I'm writing this. I will consider this a successful test, at least in regard to this PR, and I will merge it now. |
What was wrong?
The
trin-execution
doesn't support async block/transaction execution. This is needed if we want to use state network to execute transactions (e.g. eth_call, eth_estimateGas).How was it fixed?
Created
AsyncDatabase
trait and added support for using it in order to execute transactions.Some refactoring of the
trin-execution
crate:evm
moduleTo-Do