-
Notifications
You must be signed in to change notification settings - Fork 54
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
push #32
push #32
Conversation
WalkthroughThe changes primarily focus on refactoring the handling of precompiled contracts in the Ethereum Virtual Machine (EVM). The modifications enhance modularity, encapsulation, and error handling. They also introduce new methods to interfaces and structs, alter function signatures, and update how active precompiles are retrieved. Changes
TipsChat with CodeRabbit Bot (
|
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.
Review Status
Actionable comments generated: 0
Files selected for processing (13)
- core/state/interface.go (1 hunks)
- core/state/statedb.go (1 hunks)
- core/state_processor.go (3 hunks)
- core/state_transition.go (4 hunks)
- core/vm/contracts.go (1 hunks)
- core/vm/evm.go (17 hunks)
- core/vm/interface.go (2 hunks)
- core/vm/precompile_manager.go (2 hunks)
- core/vm/runtime/runtime.go (3 hunks)
- eth/tracers/js/goja.go (1 hunks)
- eth/tracers/native/4byte.go (1 hunks)
- eth/tracers/native/call_flat.go (1 hunks)
- ethapi/api.go (1 hunks)
Files skipped from review due to trivial changes (5)
- core/state/interface.go
- core/state/statedb.go
- core/vm/interface.go
- core/vm/precompile_manager.go
- core/vm/runtime/runtime.go
Additional comments (Suppressed): 27
ethapi/api.go (2)
- 1629-1634: The error handling here is good, but it's important to ensure that the error returned from
StateAndHeaderByNumberOrHash
is meaningful and provides enough context for debugging. If not, consider wrapping this error with additional context.1637:
The comment suggests a future change to use the EVM precompile manager instead of the current approach. This should be tracked in your project management tool to ensure it doesn't get lost. Also, the creation of a new EVM instance just to get the active precompiles might be inefficient if done frequently. Consider refactoring if this is the case.
- 1637-1640: The changes made here are consistent with the summary provided. The
ActivePrecompiles
function now takes an additional parameter and returns a list of active precompiled addresses based on provided rules. TheNewAccessListTracer
function is updated accordingly.core/vm/contracts.go (2)
156-174: The function
ActivePrecompiles
now takes an additional parameterevm *EVM
. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, the function now appends precompiled addresses from different Ethereum network releases instead of returning them directly based on the rules. This change could potentially lead to a larger list of active precompiles than before. Verify if this is the intended behavior.171-173: The function now includes active precompiles from the
PrecompileManager
of the providedEVM
instance. This is a new behavior and might introduce additional precompiles into the returned list. Make sure this aligns with the expected functionality.eth/tracers/js/goja.go (1)
- 251-254: The method of obtaining active precompiles has been changed from
env.PrecompileManager.GetActive(&rules)
tovm.ActivePrecompiles(env, rules)
. Ensure that the new functionvm.ActivePrecompiles
is correctly implemented and returns the expected results. Also, verify that this change does not affect other parts of the codebase that rely on the list of active precompiles.eth/tracers/native/4byte.go (2)
82-85: The method of obtaining active precompiles has been updated from
env.PrecompileManager.GetActive(&rules)
tovm.ActivePrecompiles(env, rules)
. Ensure that the new functionvm.ActivePrecompiles
is implemented correctly and returns the expected results. Also, verify that this change does not affect other parts of the codebase that rely on the list of active precompiles.88-88: No changes have been made to the condition for saving the outer calldata. The code continues to save the outer calldata if its length is 4 or more.
eth/tracers/native/call_flat.go (1)
- 148-151: The method of updating the list of active precompiles has been changed from
env.PrecompileManager.GetActive(&rules)
tovm.ActivePrecompiles(env, rules)
. Ensure that this new function behaves as expected and doesn't introduce any breaking changes.- t.activePrecompiles = env.PrecompileManager.GetActive(&rules) + t.activePrecompiles = vm.ActivePrecompiles(env, rules)core/state_transition.go (4)
262-265: The error handling after
st.state.SubBalance(st.msg.From, mgval)
has been removed. Ensure that theSubBalance
function is guaranteed not to return an error or that the error is handled elsewhere.402-405: The call to
st.state.Prepare()
now usesvm.ActivePrecompiles(st.evm, rules)
instead ofst.evm.PrecompileManager.GetActive(&rules)
. Verify that this change does not affect the functionality and that all dependencies have been updated accordingly.413-417: Error handling after
st.state.SetNonce(msg.From, st.state.GetNonce(sender.Address())+1)
has been removed. IfSetNonce
can potentially fail, this could lead to unhandled errors.436-439: Error handling after
st.state.AddBalance(st.evm.Context.Coinbase, fee)
has been removed. IfAddBalance
can potentially fail, this could lead to unhandled errors.core/state_processor.go (3)
95-98: The function
applyTransaction
has been modified to take the EVM environment as an argument instead of creating it within the function. This change could potentially affect the state of the EVM environment if it's being used elsewhere in the code. Ensure that this change doesn't introduce any side effects.113-116: The function signature of
applyTransaction
has been changed, and the order of arguments has been rearranged. Make sure all calls to this function throughout the codebase have been updated to match the new signature.167-176: The function
ApplyTransaction
now creates a new EVM environment and passes it toapplyTransaction
. This is a significant change from the previous version where the EVM environment was created insideapplyTransaction
. Ensure that this change doesn't introduce any side effects.core/vm/evm.go (12)
19-22: The import of the
errors
package has been removed. Ensure that this does not affect error handling in other parts of the code.52-58: The logic for retrieving precompiled contracts has been modified. Now, if a precompiled contract is not found in the initial map, it tries to get it from the
PrecompileManager
. This change could potentially impact performance and behavior of the EVM.- p, ok := precompiles[addr] - return p, ok + p, ok := precompiles[addr] + if p == nil || !ok { + p, ok = evm.PrecompileManager.Get(addr, &evm.chainRules) + } + return p, ok
- 139-146: The initialization of
PrecompileManager
in theNewEVM()
function has been changed. It now attempts to get thePrecompileManager
from theStateDB
before creating a new one. This change could have implications on how precompiled contracts are managed in the EVM.- evm.PrecompileManager = NewPrecompileManager(&evm.chainRules) + if pm := statedb.GetPrecompileManager(); pm != nil { + evm.PrecompileManager = pm.(PrecompileManager) + } else { + evm.PrecompileManager = NewPrecompileManager() + }
174-179: A new method
SetBlockContext()
has been added to update the block context of the EVM. This could potentially impact the state of the EVM during execution.196-199: The way precompiled contracts are checked has been changed. The
precompile()
function is now used instead of theHas
method ofPrecompileManager
. This could potentially impact performance and behavior of the EVM.- isPrecompile := evm.PrecompileManager.Has(addr) + p, isPrecompile := evm.precompile(addr)
- 233-235: The
Run
method ofPrecompileManager
now uses the precompiled contract returned by theprecompile()
function. This change could potentially impact the execution of precompiled contracts in the EVM.- ret, gas, err = evm.PrecompileManager.Run( - evm, evm.PrecompileManager.Get(addr), input, caller.Address(), value, gas, false, - ) + ret, gas, err = evm.PrecompileManager.Run(evm, p, input, caller.Address(), value, gas, false)
- 256-258: The error handling logic has been simplified. The check for
ErrExecutionReverted
is now a direct comparison instead of usingerrors.Is()
. Ensure that this does not affect error handling in other parts of the code.- if errors.Is(err, ErrExecutionReverted) { + if err != ErrExecutionReverted {
293-295: Similar changes as above have been made to the
DelegateCall()
function. Ensure that these changes do not affect the execution of delegate calls in the EVM.338-340: Similar changes as above have been made to the
StaticCall()
function. Ensure that these changes do not affect the execution of static calls in the EVM.387-394: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [383-391]
The error checking after
StateDB.AddBalance()
has been removed. Ensure that this does not affect error handling in other parts of the code.
455-459: The error checking after
StateDB.SetNonce()
has been removed. Ensure that this does not affect error handling in other parts of the code.536-538: The
GetContext()
andGetStateDB()
methods have been moved to the end of the file. This is a minor change and should not affect the functionality of the EVM.
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.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- core/vm/evm.go (17 hunks)
Additional comments (Suppressed): 14
core/vm/evm.go (14)
53-59: The new hunk introduces a fallback mechanism to fetch precompiled contracts from the
PrecompileManager
if they are not found in the predefinedprecompiles
map. This change seems to be part of a larger refactoring effort to manage precompiled contracts more dynamically. Ensure that thePrecompileManager.Get()
method is implemented correctly and returns the expected results.140-147: The initialization of
evm.PrecompileManager
has been changed. Previously, it was always initialized with a new instance ofNewPrecompileManager()
. Now, it first tries to get thePrecompileManager
from theStateDB
, and if it's not available, it falls back to creating a new instance. This change allows for more flexibility in managing precompiled contracts, but ensure that theStateDB.GetPrecompileManager()
method is implemented correctly and that theStateDB
is properly initialized with aPrecompileManager
when necessary.176-180: The
SetBlockContext
function now updates thechainRules
field of theEVM
based on the providedblockCtx
. This change aligns with the Ethereum network upgrade mechanism where different rules apply at different block numbers. Make sure that thechainConfig.Rules()
method is implemented correctly and that theblockCtx
is always updated before calling any EVM functions that depend on thechainRules
.197-200: The way precompiled contracts are checked has been modified. Instead of directly checking if the address exists in the
PrecompileManager
, the new code uses theprecompile()
function which checks both the predefinedprecompiles
map and thePrecompileManager
. This change is consistent with the changes made in lines 53-59.234-236: The
PrecompileManager.Run()
function is now called with the precompiled contract returned by theprecompile()
function instead of getting it again from thePrecompileManager
. This change reduces unnecessary calls toPrecompileManager.Get()
.257-259: The condition for setting
gas
to 0 when an error occurs has been inverted. Previously,gas
was set to 0 only if the error wasErrExecutionReverted
. Now,gas
is set to 0 for all errors exceptErrExecutionReverted
. Ensure that this change does not affect the gas calculation logic in other parts of the code.294-296: Similar to lines 197-200, the way precompiled contracts are checked has been modified to use the
precompile()
function.309-312: Similar to lines 257-259, the condition for setting
gas
to 0 when an error occurs has been inverted.341-343: Similar to lines 197-200 and 294-296, the way precompiled contracts are checked has been modified to use the
precompile()
function.352-356: Similar to lines 257-259 and 309-312, the condition for setting
gas
to 0 when an error occurs has been inverted.441-446: The error check after
evm.StateDB.SetNonce()
has been removed. As per the instructions, we assume that arguments to internal function calls are pre-sanitized and do not require additional error handling. However, ensure that theSetNonce()
method is implemented correctly and that its callers always provide valid inputs.456-461: The error check after
evm.StateDB.SetNonce()
andevm.Context.Transfer()
has been removed. Similar to the comment for lines 441-446, ensure that these methods are implemented correctly and that their callers always provide valid inputs.502-508: The condition for setting
gas
to 0 when an error occurs has been inverted. This is consistent with the changes made in lines 257-259, 309-312, and 352-356.537-539: The
GetContext()
andGetStateDB()
functions have been added to theEVM
struct. These functions provide a way to access theContext
andStateDB
fields of theEVM
, which can be useful for testing or debugging. Ensure that these functions are used appropriately and do not expose sensitive information.
Summary by CodeRabbit
Refactor:
core/state
,core/vm
,eth/tracers
, andethapi
packages.GetPrecompileManager()
to theStateDBI
interface andStateDB
struct.StateProcessor
package for better argument organization.state_transition.go
.evm.go
for retrieving context and state DB.Bug Fix:
buyGas()
function where it was returning incorrect value.These changes aim to improve code maintainability and robustness, with no direct impact on end-user functionality.