-
Notifications
You must be signed in to change notification settings - Fork 862
Add new giga executor to sei with config #2668
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
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
## Describe your changes and provide context - wraps giga with occ - leaves sequential giga in place ## Testing performed to validate your change - more benchmarking via benchmark.sh
850361f to
a835d23
Compare
## Describe your changes and provide context - associate evm senders similar to antehandler (until we can retire association) ## Testing performed to validate your change - new unit tests - benchmarks
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.
Reminder: Interpreter is the one currently doing depth management - check if it is handling depth at the right moments in regards to compatibility with evmc/evmone.
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.
Reminder: In case of delegate calls we might not be propagating sender and recipient correctly.
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.
Reminder: In case of delegate calls we might not be propagating sender and recipient correctly.
| defer stateDB.Cleanup() | ||
|
|
||
| // Get EVM message from the transaction using recovered sender | ||
| evmMsg := app.EvmKeeper.GetEVMMessage(ctx, ethTx, sender) |
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.
https://github.com/sei-protocol/go-ethereum/blob/ad46da780dee52820de6ccca91e7d797c00f0c10/core/state_transition.go#L170 we should be using this function instead - we can pass our implementation of signer which already has the senders computed.
Not a blocker for merger of this PR, we should probably have a layer above VM e.g. ExecuteTransaction where we pass an Ethereum transaction and it handles the conversion to message and invocation of vm.ApplyMessage.
pdrobnjak
left a comment
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've wired the calls between interpreter, evmc/evmone, and geth.
Looks ready to be merged!
Did not review the following files:
app/benchmark.goapp/benchmark_test.gogiga/tests/giga_test.goscripts/benchmark.shutils/helpers/address_test.go
Describe your changes and provide context
Testing performed to validate your change