-
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
[DO NOT MERGE] Stateful v1.13.1 #31
Conversation
|
||
// TODO: FIGURE OUT WHY THIS HACK IS NEEDED THANKS | ||
// bump the estimate gas by 20% for stateful precompiles | ||
hi += uint64(float64(hi) * 0.2) |
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 am gonna fucking rope
WalkthroughThis pull request introduces significant refactoring and updates to the Ethereum codebase. The changes include a shift from 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: 11
Commits
Files that changed from the base of the PR and between 90d5bd8 and c8dcceccf25ed1e988de2345b489e45e38adcc06.Files selected for processing (57)
- accounts/abi/bind/backends/simulated.go (4 hunks)
- accounts/abi/bind/base.go (3 hunks)
- cmd/clef/main.go (1 hunks)
- cmd/evm/internal/t8ntool/execution.go (2 hunks)
- cmd/evm/runner.go (1 hunks)
- cmd/evm/staterunner.go (1 hunks)
- cmd/geth/config.go (1 hunks)
- cmd/geth/main.go (1 hunks)
- cmd/utils/flags.go (2 hunks)
- consensus/beacon/consensus.go (2 hunks)
- consensus/clique/clique.go (1 hunks)
- consensus/consensus.go (1 hunks)
- consensus/ethash/consensus.go (2 hunks)
- consensus/misc/dao.go (1 hunks)
- core/block_validator.go (1 hunks)
- core/blockchain.go (5 hunks)
- core/blockchain_reader.go (1 hunks)
- core/chain_makers.go (3 hunks)
- core/genesis.go (2 hunks)
- core/state/access_list.go (6 hunks)
- core/state/interface.go (1 hunks)
- core/state/journal.go (1 hunks)
- core/state/state_object.go (17 hunks)
- core/state/state_test.go (1 hunks)
- core/state/statedb.go (14 hunks)
- core/state/statedb_test.go (5 hunks)
- core/state_prefetcher.go (2 hunks)
- core/state_processor.go (4 hunks)
- core/state_transition.go (4 hunks)
- core/txpool/blobpool/blobpool.go (1 hunks)
- core/txpool/blobpool/blobpool_test.go (1 hunks)
- core/txpool/blobpool/interface.go (1 hunks)
- core/txpool/legacypool/legacypool.go (3 hunks)
- core/txpool/legacypool/legacypool_test.go (3 hunks)
- core/txpool/legacypool/noncer.go (1 hunks)
- core/txpool/validation.go (1 hunks)
- core/types.go (1 hunks)
- core/types/transaction_signing.go (7 hunks)
- core/vm/contracts.go (25 hunks)
- core/vm/contracts_test.go (2 hunks)
- core/vm/eips.go (1 hunks)
- core/vm/evm.go (16 hunks)
- core/vm/instructions_test.go (1 hunks)
- core/vm/interface.go (2 hunks)
- core/vm/precompile_manager.go (1 hunks)
- core/vm/runtime/runtime.go (5 hunks)
- eth/api_backend.go (5 hunks)
- eth/api_debug.go (3 hunks)
- eth/api_debug_test.go (1 hunks)
- eth/backend.go (2 hunks)
- eth/filters/api.go (1 hunks)
- eth/filters/filter_system.go (1 hunks)
- eth/filters/filter_system_test.go (1 hunks)
- eth/protocols/snap/sync.go (1 hunks)
- eth/state_accessor.go (4 hunks)
- eth/tracers/api.go (7 hunks)
- eth/tracers/api_test.go (3 hunks)
Files not processed due to max files limit (39)
- eth/tracers/js/goja.go
- eth/tracers/logger/access_list_tracer.go
- eth/tracers/native/4byte.go
- eth/tracers/native/call_flat.go
- ethapi/addrlock.go
- ethapi/api.go
- ethapi/api_test.go
- ethapi/backend.go
- ethapi/dbapi.go
- ethapi/transaction_args.go
- ethapi/transaction_args_test.go
- ethclient/signer.go
- graphql/graphql.go
- graphql/service.go
- les/api_backend.go
- les/client.go
- les/odr_test.go
- les/state_accessor.go
- light/odr_test.go
- light/trie.go
- light/txpool.go
- miner/miner.go
- miner/miner_test.go
- miner/ordering.go
- miner/ordering_test.go
- miner/worker.go
- node/node.go
- params/protocol_params.go
- signer/core/api.go
- signer/core/api_test.go
- signer/core/auditlog.go
- signer/core/cliui.go
- signer/core/stdioui.go
- signer/rules/rules.go
- signer/rules/rules_test.go
- tests/block_test_util.go
- tests/fuzzers/bls12381/precompile_fuzzer.go
- tests/state_test.go
- tests/state_test_util.go
Files skipped from review due to trivial changes (14)
- accounts/abi/bind/base.go
- cmd/clef/main.go
- core/state/access_list.go
- core/state/state_object.go
- core/state/statedb.go
- core/txpool/blobpool/blobpool_test.go
- core/txpool/legacypool/noncer.go
- core/txpool/validation.go
- core/vm/eips.go
- core/vm/instructions_test.go
- eth/api_backend.go
- eth/api_debug_test.go
- eth/filters/api.go
- eth/protocols/snap/sync.go
Additional comments (Suppressed): 120
cmd/geth/config.go (1)
- 35-41: The import path for the
ethapi
package has been changed from"github.com/ethereum/go-ethereum/internal/ethapi"
to"github.com/ethereum/go-ethereum/ethapi"
. Please ensure that this new path is correct and accessible, and that it does not break any dependencies.cmd/evm/runner.go (1)
- 123-127: The type of
statedb
has been changed from a concrete type (*state.StateDB
) to an interface (state.StateDBI
). This change enhances the modularity and flexibility of the code, allowing any type that implements theStateDBI
interface to be used as the state database. However, ensure that all usages ofstatedb
in this file are compatible with the new interface.cmd/evm/staterunner.go (1)
- 106-112: The function signature of the callback in
test.Run
has been updated to use thestate.StateDBI
interface instead of the concrete type*state.StateDB
. This change enhances modularity and allows for different implementations of the state database. Ensure that all implementations ofstate.StateDBI
have a methodIntermediateRoot
that matches the one previously available in*state.StateDB
.consensus/misc/dao.go (1)
- 75-78: The function signature has been changed from using a concrete type (
*state.StateDB
) to an interface (state.StateDBI
). This change enhances the modularity and flexibility of the code, allowing any type that implements theStateDBI
interface to be passed into the function. However, ensure that all calls to this function throughout the codebase have been updated to match the new signature.consensus/consensus.go (1)
- 88-100: The function signatures for
Finalize
andFinalizeAndAssemble
have been updated to use thestate.StateDBI
interface instead of the concrete type*state.StateDB
. This change enhances modularity by allowing any type that implements theStateDBI
interface to be passed into these functions. However, ensure that all implementations of these methods throughout the codebase have been updated to match the new signature.consensus/ethash/consensus.go (3)
490-493: The function signature for
Finalize
has been updated to use the interfacestate.StateDBI
instead of the concrete type*state.StateDB
. This change enhances modularity and flexibility, allowing any type that implements theStateDBI
interface to be passed into the function. Ensure all calls to this function throughout the codebase have been updated to match the new signature.497-500: Similar to the
Finalize
function, theFinalizeAndAssemble
function now also uses thestate.StateDBI
interface. The error handling for unsupported withdrawals remains unchanged.547-553: The
accumulateRewards
function now uses thestate.StateDBI
interface as well. This change is consistent with the updates made to theFinalize
andFinalizeAndAssemble
functions.core/block_validator.go (1)
- 124-124: The function signature of
ValidateState
has been changed. The type of the second parameterstatedb
has been updated from a pointer tostate.StateDB
to an interfacestate.StateDBI
. This change enhances modularity and allows for different implementations of the state database to be used.- func (v *BlockValidator) ValidateState(block *types.Block, statedb *state.StateDB, receipts types.Receipts, usedGas uint64) error { + func (v *BlockValidator) ValidateState(block *types.Block, statedb state.StateDBI, receipts types.Receipts, usedGas uint64) error {Please ensure that all calls to this function throughout the codebase have been updated to match the new signature.
consensus/clique/clique.go (2)
570-574: The
Finalize
function has been updated to use the interfacestate.StateDBI
instead of the concrete type*state.StateDB
. This change enhances modularity and flexibility in the code. However, ensure that all calls to this function throughout the codebase have been updated to match the new signature.578-581: Similar to the
Finalize
function, theFinalizeAndAssemble
function now uses thestate.StateDBI
interface. The error handling for unsupported withdrawals remains unchanged. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.consensus/beacon/consensus.go (2)
347-355: The
Finalize
function now uses thestate.StateDBI
interface instead of the concrete type*state.StateDB
. This change enhances modularity and flexibility, allowing any type that implements theStateDBI
interface to be passed as an argument. Ensure all calls to this function have been updated accordingly.366-374: Similar to the
Finalize
function, theFinalizeAndAssemble
function now also uses thestate.StateDBI
interface. This change should improve code maintainability and testability by decoupling the function from a specific implementation of state management. Make sure all calls to this function are updated to match the new signature.core/blockchain.go (5)
1404-1407: The function signature of
writeBlockWithState
has been changed to use the interfacestate.StateDBI
instead of the concrete type*state.StateDB
. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.1492-1495: The function signature of
WriteBlockAndSetHead
has been changed to use the interfacestate.StateDBI
instead of the concrete type*state.StateDB
. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.1503-1506: The function signature of
writeBlockAndSetHead
has been changed to use the interfacestate.StateDBI
instead of the concrete type*state.StateDB
. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.1729-1732: The variable
activeState
now uses the interfacestate.StateDBI
instead of the concrete type*state.StateDB
. Ensure that all usages of this variable are compatible with the new interface.1813-1816: In the goroutine, the variable
throwaway
now uses the interfacestate.StateDBI
instead of the concrete type*state.StateDB
. Ensure that all usages of this variable in the goroutine are compatible with the new interface.core/chain_makers.go (3)
41-44: The type of
statedb
has been changed from a concrete type (*state.StateDB
) to an interface (state.StateDBI
). This change enhances the modularity and flexibility of the code, allowing any type that implements theStateDBI
interface to be used as the state database. Ensure all usages ofstatedb
are updated accordingly.292-294: The function
genblock
now acceptsstate.StateDBI
instead of*state.StateDB
. This is in line with the changes made to theBlockGen
struct. Make sure that all calls to this function have been updated to pass an object of a type that implementsStateDBI
.374-374: The function
makeHeader
now acceptsstate.StateDBI
instead of*state.StateDB
. This is consistent with the other changes made in this PR. Ensure that all calls to this function have been updated to pass an object of a type that implementsStateDBI
.cmd/evm/internal/t8ntool/execution.go (1)
- 117-123: The function signature for
Apply
has been updated to return an interface (state.StateDBI
) instead of a concrete type (*state.StateDB
). This change enhances the flexibility and modularity of the code, allowing any type that implements theStateDBI
interface to be returned. Ensure all calls to this function throughout the codebase have been updated to handle the new return type.core/state/journal.go (1)
- 93-97: The type of
prev
has been changed from*stateObject
to*StateObject
. Ensure that all usages of this field are updated accordingly and that the new type provides all necessary methods and fields used in the code. Also, verify if the change doesn't break any existing functionality or assumptions about theprev
field.cmd/utils/flags.go (2)
54-64: The import path for
ethapi
has been changed from"github.com/ethereum/go-ethereum/internal/ethapi"
to"github.com/ethereum/go-ethereum/ethapi"
. Ensure that this new path is correct and accessible, and that all references toethapi
in the codebase have been updated accordingly.1954-1956: The type of
stack
parameter inRegisterFilterAPI
function has been changed from*node.Node
toNetworkingStack
interface. This change enhances modularity by allowing any type that implements theNetworkingStack
interface to be passed as an argument. However, ensure that all types that are passed asstack
implement theRegisterAPIs
method required by theNetworkingStack
interface.core/state/state_test.go (1)
- 249-252: The function
compareStateObjects
has been updated to use the exported typeStateObject
instead of the unexported typestateObject
. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that theAddress()
method is available and behaves as expected on theStateObject
type.core/txpool/blobpool/interface.go (1)
- 40-44: The return type of the
StateAt
method has been changed from a concrete type (*state.StateDB
) to an interface (state.StateDBI
). This change enhances modularity and allows for easier testing, as mock implementations can be used. However, ensure that all calls to this function throughout the codebase have been updated to handle the new return type correctly.core/state_transition.go (4)
262-265: The function now returns the error from
st.state.Error()
instead ofnil
. Ensure that this change does not affect the callers of this function. They should be able to handle the returned error appropriately.402-405: The call to
vm.ActivePrecompiles(rules)
has been replaced withst.evm.PrecompileManager.GetActive(&rules)
. This change seems to be part of the transition from concrete types to interfaces mentioned in the PR summary. Make sure that the new method provides the same functionality as the old one and that it is compatible with the rest of the codebase.413-418: Error handling has been added after the
st.state.SetNonce
operation. This is a good practice as it ensures that any errors occurring during state modification are caught and handled immediately.439-444: Similar to the previous comment, error handling has been added after the
st.state.AddBalance
operation. This is a good practice as it ensures that any errors occurring during state modification are caught and handled immediately.cmd/geth/main.go (1)
- 35-35: The import path for the
ethapi
package has been changed from"github.com/ethereum/go-ethereum/internal/ethapi"
to"github.com/ethereum/go-ethereum/ethapi"
. Please ensure that this new path is correct and accessible, and that all references to theethapi
package in the codebase have been updated accordingly.core/txpool/blobpool/blobpool.go (1)
- 302-302: The type of
state
has been changed from a concrete type (*state.StateDB
) to an interface (state.StateDBI
). This change enhances the modularity and flexibility of the code, allowing any type that implements theStateDBI
interface to be used as the state. However, ensure that all usages ofstate
in the codebase are compatible with this change.core/genesis.go (2)
138-140: The addition of error checking after setting the account code and nonce is a good practice. It ensures that any issues with these operations are caught immediately, preventing potential inconsistencies in the state database.
162-164: Similar to the previous comment, the addition of error checking after setting the account code and nonce is a good practice. This will help catch any issues early on and prevent potential inconsistencies in the state database.
core/state/statedb_test.go (5)
176-182: The
Copy()
function now returns an interface (StateDBI
) instead of a concrete type (*StateDB
). This change enhances modularity and flexibility in the code. However, it requires type assertion to convert the interface back to the concrete type. Ensure that all calls to this function throughout the codebase have been updated to match the new usage.597-600: Similar to the previous comment, the
Copy()
function now returns an interface (StateDBI
) instead of a concrete type (*StateDB
). The returned interface is being converted back to the concrete type using type assertion. Ensure that all calls to this function throughout the codebase have been updated to match the new usage.684-687: Again, the
Copy()
function now returns an interface (StateDBI
) instead of a concrete type (*StateDB
). The returned interface is being converted back to the concrete type using type assertion. Ensure that all calls to this function throughout the codebase have been updated to match the new usage.869-872: The
New()
function now takes acommon.Hash{}
as its first argument instead oftypes.EmptyRootHash
. Also, thenewAccessList()
function has been replaced withNewAccessList()
. Ensure that these changes are reflected across the entire codebase.1021-1024: As with previous comments, the
Copy()
function now returns an interface (StateDBI
) instead of a concrete type (*StateDB
). The returned interface is being converted back to the concrete type using type assertion. Ensure that all calls to this function throughout the codebase have been updated to match the new usage.core/state/interface.go (2)
- 29-106: The introduction of the
StateDBI
interface is a significant change that enhances modularity and flexibility in the code. It abstracts the state database operations, allowing for different implementations that adhere to this interface. This is a good practice as it decouples the code and makes it easier to test and maintain.However, there are some methods in the interface that lack return types. For instance,
CreateAccount(common.Address)
,SetNonce(common.Address, uint64)
,SetCode(common.Address, []byte)
,AddRefund(uint64)
,SubRefund(uint64)
,SetState(common.Address, common.Hash, common.Hash)
,SetTransientState(addr common.Address, key, value common.Hash)
,SelfDestruct(common.Address)
,Selfdestruct6780(common.Address)
,AddAddressToAccessList(addr common.Address)
,AddSlotToAccessList(addr common.Address, slot common.Hash)
,Prepare(rules params.Rules, sender, coinbase common.Address, dest *common.Address, precompiles []common.Address, txAccesses types.AccessList)
,RevertToSnapshot(int)
,AddLog(*types.Log)
,AddPreimage(common.Hash, []byte)
,SetBalance(addr common.Address, amount *big.Int)
,SetStorage(addr common.Address, storage map[common.Hash]common.Hash)
,Finalise(deleteEmptyObjects bool)
,SetTxContext(thash common.Hash, ti int)
,StopPrefetcher()
,StartPrefetcher(namespace string)
do not specify what they return.If these methods do not need to return anything, it would be clearer to explicitly declare them as returning nothing by using the
void
keyword. If they are supposed to return something, please add the appropriate return type.Here's an example of how you might modify one of these methods:
- SetNonce(common.Address, uint64) + SetNonce(common.Address, uint64) void
- 58-58: The method
Selfdestruct6780(common.Address)
seems to be a duplicate ofSelfDestruct(common.Address)
. If they perform the same operation, consider removing one to avoid redundancy. If they are different, consider renaming one or both methods to more accurately reflect their functionality and reduce confusion.core/blockchain_reader.go (3)
323-325: The return type of the
State()
function has been changed from*state.StateDB
tostate.StateDBI
. This change is in line with the PR's goal of transitioning from concrete types to interfaces for state management. Ensure that all calls to this function throughout the codebase have been updated to handle the new interface return type.328-330: The return type of the
StateAt()
function has been changed from*state.StateDB
tostate.StateDBI
. This change is in line with the PR's goal of transitioning from concrete types to interfaces for state management. Ensure that all calls to this function throughout the codebase have been updated to handle the new interface return type.332-335: A new function
StateAtBlockNumber()
has been added which returns a mutable state based on a particular block number. This seems like a useful addition, but ensure that error handling is done properly when callingGetHeaderByNumber(number)
, as it can returnnil
if the header does not exist.core/txpool/legacypool/legacypool_test.go (3)
63-68: The change from
*state.StateDB
tostate.StateDBI
in thetestBlockChain
struct is a good move towards better abstraction and modularity. This allows for easier testing and potential future changes in the implementation of the state database without affecting this part of the code.91-97: The function signatures for
StateAt
andStateAtBlockNumber
have been updated to returnstate.StateDBI
instead of*state.StateDB
. Ensure that all calls to these functions throughout the codebase have been updated to handle the new return type.253-256: The function signature for
State
has been updated to returnstate.StateDBI
instead of*state.StateDB
. Ensure that all calls to this function throughout the codebase have been updated to handle the new return type.accounts/abi/bind/backends/simulated.go (4)
68-68: The type of
pendingState
has been changed from*state.StateDB
tostate.StateDBI
. This change is part of the larger effort to transition from concrete types to interfaces for state management, as mentioned in the PR summary. This should enhance modularity and flexibility in the code.182-185: The return type of
stateByBlockNumber
function has been updated from*state.StateDB
tostate.StateDBI
. Ensure that all calls to this function throughout the codebase have been updated to handle the new return type.587-590: The condition for rejecting a transaction has been slightly modified. Previously, it was rejected if
hi == cap
, but now it's rejected ifhi >= cap
. This seems like a minor change, but please verify that this doesn't unintentionally alter any logic or behavior in the system.608-611: The
callContract
function now acceptsstate.StateDBI
instead of*state.StateDB
as an argument. This aligns with the overall goal of transitioning from concrete types to interfaces for state management. Make sure all calls to this function are updated accordingly.core/txpool/legacypool/legacypool.go (2)
119-126: The interface has been updated to include a new method
StateAtBlockNumber(number uint64) (state.StateDBI, error)
. Ensure that all implementations of this interface have been updated accordingly.216-222: The type of
currentState
has been changed from*state.StateDB
tostate.StateDBI
. This change enhances modularity by allowing any type that implements theStateDBI
interface to be used as the current state. However, ensure that all usages ofcurrentState
throughout the codebase are compatible with this change.core/types.go (3)
36-36: The function signature of
ValidateState
has been changed. The type of the second parameterstate
has been changed from a pointer to a concrete type (*state.StateDB
) to an interface (state.StateDBI
). This change enhances modularity and allows for different implementations of the state database. However, ensure that all calls to this function throughout the codebase have been updated to match the new signature.44-44: The function signature of
Prefetch
has been changed. The type of the second parameterstatedb
has been changed from a pointer to a concrete type (*state.StateDB
) to an interface (state.StateDBI
). This change enhances modularity and allows for different implementations of the state database. However, ensure that all calls to this function throughout the codebase have been updated to match the new signature.52-52: The function signature of
Process
has been changed. The type of the second parameterstatedb
has been changed from a pointer to a concrete type (*state.StateDB
) to an interface (state.StateDBI
). This change enhances modularity and allows for different implementations of the state database. However, ensure that all calls to this function throughout the codebase have been updated to match the new signature.core/state_prefetcher.go (2)
47-53: The function signature of
Prefetch
has been updated to use the interfacestate.StateDBI
instead of the concrete type*state.StateDB
. This change enhances modularity and allows for different implementations of the state database to be used. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.85-91: Similar to the previous comment, the function signature of
precacheTransaction
has been updated to use the interfacestate.StateDBI
instead of the concrete type*state.StateDB
. This change also enhances modularity and allows for different implementations of the state database to be used. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.core/vm/contracts_test.go (3)
19-27: The import of the
context
andmath/big
packages is new in this hunk. Ensure that these packages are used appropriately in the code.98-106: The function
RunPrecompiledContract
has been introduced to execute a precompiled contract with given input and gas. It checks if the supplied gas is sufficient, deducts the required gas cost, and then runs the contract. This function improves modularity by separating the execution logic from the testing logic.104-104: The
Run
method now takes additional parameters: a context, a state interface, an address, and a value. Make sure that all calls to this method have been updated accordingly throughout the codebase.- output, err := p.Run(input) + output, err := p.Run(context.Background(), nil, input, common.Address{}, new(big.Int))eth/backend.go (2)
44-52: The import path for the
ethapi
package has been changed from"github.com/ethereum/go-ethereum/internal/ethapi"
to"github.com/ethereum/go-ethereum/ethapi"
. Ensure that this new path is correct and accessible, and that it does not break any dependencies.299-305: The function call
ethapi.GetAPIs(s.APIBackend, s.BlockChain())
now includes an additional arguments.BlockChain()
. Make sure all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that theBlockChain()
method always returns a valid object when this function is called.core/vm/runtime/runtime.go (4)
50-52: The type of
State
in theConfig
struct has been changed from a concrete type (*state.StateDB
) to an interface (state.StateDBI
). This change enhances modularity and allows for different implementations of the state database. However, ensure that all implementations ofstate.StateDBI
provide the necessary methods used in this file.123-123: The method
vmenv.PrecompileManager.GetActive(&rules)
is now being used instead ofvm.ActivePrecompiles(rules)
. Ensure thatPrecompileManager.GetActive()
provides the same functionality asvm.ActivePrecompiles()
.156-156: Similar to the previous comment,
vmenv.PrecompileManager.GetActive(&rules)
is now being used instead ofvm.ActivePrecompiles(rules)
. Verify thatPrecompileManager.GetActive()
provides the same functionality asvm.ActivePrecompiles()
.184-184: Again,
vmenv.PrecompileManager.GetActive(&rules)
is now being used instead ofvm.ActivePrecompiles(rules)
. As mentioned before, verify thatPrecompileManager.GetActive()
provides the same functionality asvm.ActivePrecompiles()
.eth/tracers/api_test.go (3)
42-42: The import path for the
ethapi
package has been changed from"github.com/ethereum/go-ethereum/internal/ethapi"
to"github.com/ethereum/go-ethereum/ethapi"
. Please ensure that this new path is correct and accessible, and that it does not break any dependencies.142-145: The function signature of
StateAtBlock
has been modified. The type ofbase
parameter has been changed from*state.StateDB
tostate.StateDBI
, and the return type has also been updated from*state.StateDB
tostate.StateDBI
. This change enhances modularity by using interfaces instead of concrete types. However, please verify that all calls to this function throughout the codebase have been updated to match the new signature.158-161: Similar to the previous comment, the function signature of
StateAtTransaction
has been modified. The return type has been updated from*state.StateDB
tostate.StateDBI
. Please verify that all calls to this function throughout the codebase have been updated to match the new signature.core/state_processor.go (8)
61-64: The function signature of
Process
has been updated to use the interfacestate.StateDBI
instead of the concrete type*state.StateDB
. This change enhances modularity and allows for different implementations of the state database. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.89-93: The order of arguments in the
applyTransaction
function call has been changed. Thevmenv
andmsg
parameters are now passed first, followed by the other parameters. Make sure that all calls toapplyTransaction
in your codebase reflect this change.108-114: The function signature of
applyTransaction
has been updated similar toProcess
, using the interfacestate.StateDBI
instead of the concrete type*state.StateDB
. Also, the order of arguments has been changed withevm
andmsg
now being the first two parameters. Ensure that all calls to this function throughout the codebase have been updated to match the new signature and argument order.161-207: A new function
applyTransactionWithResult
has been introduced which is similar toapplyTransaction
but also returns anExecutionResult
. This function can be useful when you want to process a transaction and also get detailed execution results. Make sure to handle the additional return value properly when using this function.213-225: The function
ApplyTransaction
has been updated to use the interfacestate.StateDBI
instead of the concrete type*state.StateDB
. This change enhances modularity and allows for different implementations of the state database. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.227-239: A new function
ApplyTransactionWithResult
has been introduced which is similar toApplyTransaction
but also returns anExecutionResult
. This function can be useful when you want to process a transaction and also get detailed execution results. Make sure to handle the additional return value properly when using this function.241-250: Two new functions
ApplyTransactionWithEVM
andApplyTransactionWithEVMWithResult
have been introduced. These functions allow you to apply a transaction using a pre-existing EVM environment, which can be useful in scenarios where you already have an EVM instance and want to reuse it. Ensure that these functions are used correctly in your codebase.266-269: The function
ProcessBeaconBlockRoot
has been updated to use the interfacestate.StateDBI
instead of the concrete type*state.StateDB
. This change enhances modularity and allows for different implementations of the state database. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.eth/api_debug.go (3)
28-34: The import path for
ethapi
has been changed from"github.com/ethereum/go-ethereum/internal/ethapi"
to"github.com/ethereum/go-ethereum/ethapi"
. Ensure that the new path is correct and accessible, and that it doesn't break any dependencies.136-140: The type of
stateDb
has been changed from a pointer to aStateDB
struct (*state.StateDB
) to aStateDBI
interface. This change enhances modularity by allowing any type implementing theStateDBI
interface to be used here. However, ensure that all types that will be assigned tostateDb
do implement this interface.229-235: Similar to the previous comment, the
statedb
parameter in thestorageRangeAt
function has been changed from a pointer to aStateDB
struct (*state.StateDB
) to aStateDBI
interface. Again, this enhances modularity but requires verification that all types passed asstatedb
implement theStateDBI
interface.core/vm/contracts.go (3)
42-48: The
PrecompiledContract
interface has been updated to include two new methods:RegistryKey()
andRun()
. TheRun()
method now takes additional parameters. Ensure that all implementations of this interface throughout the codebase have been updated accordingly.156-159: The function signature for
ActivePrecompiles
has changed from accepting a value of typeparams.Rules
to a pointer to aparams.Rules
object. Make sure that all calls to this function are updated to pass a pointer instead of a value.741-748: The
Run
method in various structs (ecrecover
,sha256hash
,ripemd160hash
,dataCopy
,bigModExp
,bn256AddIstanbul
,bn256AddByzantium
,bn256ScalarMulIstanbul
,bn256ScalarMulByzantium
,bn256PairingIstanbul
,bn256PairingByzantium
,blake2F
,bls12381G1Add
,bls12381G1Mul
) now includes additional parameters:ctx context.Context
,evm PrecompileEVM
,caller common.Address
,value *big.Int
. Ensure that all calls to theseRun
methods are updated to pass the correct arguments.core/vm/interface.go (2)
97-99: The
CallContext
interface remains unchanged and continues to provide a basic interface for EVM calling conventions. TheCreate
method allows for the creation of new contracts.101-128: Two new interfaces,
PrecompileManager
andPrecompileEVM
, have been introduced.PrecompileManager
provides functionality related to managing and executing precompiled contracts in the EVM.PrecompileEVM
serves as an interface through which stateful precompiles can call back into the EVM. Ensure that these new interfaces are implemented correctly throughout the codebase.eth/filters/filter_system.go (1)
- 545-551: The
Unsubscribe()
method fores.txsSub
has been commented out. This could potentially lead to memory leaks as the subscription may not be properly cleaned up when the event loop ends. If this is intentional, please ensure that there are no adverse effects due to this change.- // es.txsSub.Unsubscribe() + es.txsSub.Unsubscribe()core/types/transaction_signing.go (3)
162-176: The
Signer
interface has been extended with three new methods:PubKey
,Signature
, andSignatureValues
. This change enhances the functionality of the signer, allowing it to retrieve the public key and raw signature of a transaction, in addition to the existing capabilities. Ensure that all implementations of this interface have been updated accordingly.632-646: The
FrontierSigner
struct now implements thePubKey
andSignature
methods. The logic seems correct, checking the transaction type and returning an error if it's not a LegacyTxType.681-712: New utility functions
recoverSig
,recoverPub
, andrecoverPlain
have been added to handle signature recovery. These functions seem to be correctly implemented, validating the signature values and using the appropriate recovery method based on whether the transaction is homestead or not.core/vm/evm.go (17)
19-23: The import of the
errors
package is new in this hunk. Ensure that it's used appropriately in the codebase.102-109: The addition of
PrecompileManager PrecompileManager
to the EVM struct introduces a new way to manage precompiled contracts, which should improve modularity and flexibility.137-142: The instantiation of
PrecompileManager
withinNewEVM()
function has been updated. TheNewPrecompileManager()
function is now being called with&evm.chainRules
as an argument. Make sure that theNewPrecompileManager()
function handles this argument correctly.144-161: A new function
NewEVMWithPrecompiles()
has been introduced. This function allows for the creation of an EVM instance with a specificPrecompileManager
. This could be useful for testing or other scenarios where a customPrecompileManager
is needed.186-192: New getter methods
GetStateDB()
andGetContext()
have been added. These methods provide a way to access the state database and block context of the EVM, respectively. This can enhance encapsulation and data safety.208-211: The check for precompiled contracts has been updated to use the new
PrecompileManager
. This change should make the code more modular and easier to maintain.245-249: The execution of precompiled contracts has been updated to use the new
PrecompileManager
. This change should make the code more modular and easier to maintain.268-273: Error handling has been improved by using the
errors.Is()
function to check if the error isErrExecutionReverted
. This is a more reliable way to check for specific errors compared to direct comparison.309-314: The check and execution of precompiled contracts in the
DelegateCall()
function have been updated to use the newPrecompileManager
. This change should make the code more modular and easier to maintain.322-328: Error handling has been improved by using the
errors.Is()
function to check if the error isErrExecutionReverted
. This is a more reliable way to check for specific errors compared to direct comparison.356-362: The check and execution of precompiled contracts in the
StaticCall()
function have been updated to use the newPrecompileManager
. This change should make the code more modular and easier to maintain.369-375: Error handling has been improved by using the
errors.Is()
function to check if the error isErrExecutionReverted
. This is a more reliable way to check for specific errors compared to direct comparison.400-402: Added error checking after
AddBalance
operation. This is a good practice as it ensures that any errors that occur during state modification are caught and handled appropriately.411-416: The check and execution of precompiled contracts in the
Create()
function have been updated to use the newPrecompileManager
. This change should make the code more modular and easier to maintain.430-436: Error handling has been improved by using the
errors.Is()
function to check if the error isErrExecutionReverted
. This is a more reliable way to check for specific errors compared to direct comparison.467-469: Added error checking after
SetNonce
operation. This is a good practice as it ensures that any errors that occur during state modification are caught and handled appropriately.484-487: Added error checking after
SetNonce
operation. This is a good practice as it ensures that any errors that occur during state modification are caught and handled appropriately.eth/filters/filter_system_test.go (1)
- 38-38: The import path for the
ethapi
package has been changed from"github.com/ethereum/go-ethereum/internal/ethapi"
to"github.com/ethereum/go-ethereum/ethapi"
. Ensure that this new path is correct and accessible, and that it does not break any dependencies.eth/state_accessor.go (4)
40-43: The function signature for
hashState
has been updated to use thestate.StateDBI
interface instead of the concrete type*state.StateDB
. This change enhances modularity and allows for different implementations of the state database. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.177-180: Similar to the previous comment, the function signature for
pathState
has also been updated to use thestate.StateDBI
interface. Again, ensure that all calls to this function throughout the codebase have been updated to match the new signature.211-216: The function signature for
stateAtBlock
has been updated to use thestate.StateDBI
interface. This change is consistent with the updates made to thehashState
andpathState
functions. As before, ensure that all calls to this function throughout the codebase have been updated to match the new signature.240-261: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [219-261]
The function
stateAtTransaction
now uses thestate.StateDBI
interface in its return type. This is consistent with the changes made to other functions in this file. Also, note the change on line 248 where theGetEVM
method is now used instead of directly creating a new EVM instance. This could potentially affect the behavior of the function if theGetEVM
method has different logic than the previously used constructor. Please verify that this change does not introduce any unintended side effects.eth/tracers/api.go (7)
36-43: The import path for
ethapi
has been changed from"github.com/ethereum/go-ethereum/internal/ethapi"
to"github.com/ethereum/go-ethereum/ethapi"
. Ensure that the new path is correct and accessible, and that it doesn't break any dependencies.85-90: The function signatures for
StateAtBlock
andStateAtTransaction
have been updated. The type ofbase
inStateAtBlock
and the return type ofstate.StateDB
in both functions have been changed from*state.StateDB
tostate.StateDBI
. Make sure all calls to these functions throughout the codebase have been updated to match the new signatures.183-189: The type of
statedb
inblockTraceTask
struct has been changed from*state.StateDB
tostate.StateDBI
. This change should improve modularity by allowing different implementations of theStateDBI
interface to be used.200-205: The type of
statedb
intxTraceTask
struct has been changed from*state.StateDB
tostate.StateDBI
. This change should improve modularity by allowing different implementations of theStateDBI
interface to be used.308-313: The type of
statedb
variable has been changed from*state.StateDB
tostate.StateDBI
. This change should improve modularity by allowing different implementations of theStateDBI
interface to be used.626-632: The type of
statedb
parameter intraceBlockParallel
function has been changed from*state.StateDB
tostate.StateDBI
. Make sure all calls to this function throughout the codebase have been updated to match the new signature.922-928: The type of
statedb
parameter intraceTx
function has been changed from*state.StateDB
tostate.StateDBI
. Make sure all calls to this function throughout the codebase have been updated to match the new signature.
func MakePreState(db ethdb.Database, accounts core.GenesisAlloc) state.StateDBI { | ||
sdb := state.NewDatabaseWithConfig(db, &trie.Config{Preimages: true}) | ||
statedb, _ := state.New(types.EmptyRootHash, sdb, nil) | ||
for addr, a := range accounts { |
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.
Similar to the Apply
function, the MakePreState
function now returns an interface (state.StateDBI
) instead of a concrete type (*state.StateDB
). This is a good practice as it allows for better abstraction and flexibility. However, note that the error from the state.New
function is being ignored. It's generally a good idea to handle errors rather than ignoring them, even if you expect them to never occur. Ignoring errors can lead to difficult-to-diagnose bugs.
- statedb, _ := state.New(types.EmptyRootHash, sdb, nil)
+ statedb, err := state.New(types.EmptyRootHash, sdb, nil)
+ if err != nil {
+ // Handle or return the error appropriately
+ }
core/txpool/legacypool/legacypool.go
Outdated
} | ||
statedb, err := pool.chain.StateAt(newHead.Root) | ||
if err != nil { | ||
log.Error("Failed to reset txpool state", "err", err) | ||
return | ||
statedb, err = pool.chain.StateAtBlockNumber(newHead.Number.Uint64()) | ||
if err != nil { | ||
log.Warn("Failed to get new state", "err", err) | ||
} | ||
} | ||
|
||
pool.currentHead.Store(newHead) | ||
pool.currentState = statedb | ||
pool.pendingNonces = newNoncer(statedb) | ||
if pool.currentState != nil { | ||
pool.pendingNonces = newNoncer(statedb) | ||
} else { | ||
pool.pendingNonces = &noncer{} | ||
} |
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.
The error handling logic has been improved here. Previously, if there was an error in getting the state at newHead.Root
, it would log an error and return. Now, it tries to get the state at the block number of newHead
before logging a warning. This is a good improvement as it adds a fallback mechanism when the first attempt to get the state fails. However, note that even if both attempts fail, the function does not return and continues execution. Make sure this is the intended behavior and that it won't lead to any runtime errors or unexpected behavior later on.
statedb, err := pool.chain.StateAt(newHead.Root)
if err != nil {
+ statedb, err = pool.chain.StateAtBlockNumber(newHead.Number.Uint64())
if err != nil {
log.Warn("Failed to get new state", "err", err)
}
}
core/vm/precompile_manager.go
Outdated
if pm.precompiles == nil { | ||
pm.precompiles = activePrecompiles(pm.rules) | ||
} |
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.
The precompiles
map is being initialized inside the Has
and Get
methods. This can lead to data races if these methods are called concurrently from different goroutines. Consider using a mutex or initializing the precompiles
map during the creation of the precompileManager
to avoid this issue.
gasCost := p.RequiredGas(input) | ||
if gasCost > suppliedGas { | ||
return nil, 0, ErrOutOfGas | ||
} |
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.
The error handling here is correct but it might be more informative to return a custom error message that includes the amount of gas that was required and the amount that was supplied. This would make debugging easier if an ErrOutOfGas
error occurs.
suppliedGas -= gasCost | ||
output, err := p.Run(context.Background(), evm, input, caller, value) | ||
|
||
return output, suppliedGas, err |
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.
The context passed to the Run
method of the precompiled contract is always a background context. If there's a need to cancel the execution of the precompiled contract based on some conditions or timeouts, it would be better to accept a context as an argument to the Run
method of the precompileManager
.
core/vm/interface.go
Outdated
GetCodeHash(common.Address) common.Hash | ||
GetCode(common.Address) []byte | ||
SetCode(common.Address, []byte) | ||
GetCodeSize(common.Address) int | ||
|
||
AddRefund(uint64) | ||
SubRefund(uint64) | ||
GetRefund() uint64 | ||
|
||
GetCommittedState(common.Address, common.Hash) common.Hash | ||
GetState(common.Address, common.Hash) common.Hash | ||
SetState(common.Address, common.Hash, common.Hash) | ||
|
||
GetTransientState(addr common.Address, key common.Hash) common.Hash | ||
SetTransientState(addr common.Address, key, value common.Hash) | ||
|
||
SelfDestruct(common.Address) | ||
HasSelfDestructed(common.Address) bool | ||
|
||
Selfdestruct6780(common.Address) | ||
|
||
// Exist reports whether the given account exists in state. | ||
// Notably this should also return true for self-destructed accounts. | ||
Exist(common.Address) bool | ||
// Empty returns whether the given account is empty. Empty | ||
// is defined according to EIP161 (balance = nonce = code = 0). | ||
Empty(common.Address) bool | ||
|
||
AddressInAccessList(addr common.Address) bool | ||
SlotInAccessList(addr common.Address, slot common.Hash) (addressOk bool, slotOk bool) | ||
// AddAddressToAccessList adds the given address to the access list. This operation is safe to perform | ||
// even if the feature/fork is not active yet | ||
AddAddressToAccessList(addr common.Address) | ||
// AddSlotToAccessList adds the given (address,slot) to the access list. This operation is safe to perform | ||
// even if the feature/fork is not active yet | ||
AddSlotToAccessList(addr common.Address, slot common.Hash) | ||
Prepare(rules params.Rules, sender, coinbase common.Address, dest *common.Address, precompiles []common.Address, txAccesses types.AccessList) | ||
|
||
RevertToSnapshot(int) | ||
Snapshot() int | ||
|
||
AddLog(*types.Log) | ||
AddPreimage(common.Hash, []byte) | ||
state.StateDBI | ||
// TODO: streamline this interface | ||
// CreateAccount(common.Address) | ||
|
||
// SubBalance(common.Address, *big.Int) | ||
// AddBalance(common.Address, *big.Int) | ||
// GetBalance(common.Address) *big.Int | ||
|
||
// GetNonce(common.Address) uint64 | ||
// SetNonce(common.Address, uint64) | ||
|
||
// GetCodeHash(common.Address) common.Hash | ||
// GetCode(common.Address) []byte | ||
// SetCode(common.Address, []byte) | ||
// GetCodeSize(common.Address) int | ||
|
||
// AddRefund(uint64) | ||
// SubRefund(uint64) | ||
// GetRefund() uint64 | ||
|
||
// GetCommittedState(common.Address, common.Hash) common.Hash | ||
// GetState(common.Address, common.Hash) common.Hash | ||
// SetState(common.Address, common.Hash, common.Hash) | ||
|
||
// GetTransientState(addr common.Address, key common.Hash) common.Hash | ||
// SetTransientState(addr common.Address, key, value common.Hash) | ||
|
||
// SelfDestruct(common.Address) | ||
// HasSelfDestructed(common.Address) bool | ||
|
||
// Selfdestruct6780(common.Address) | ||
|
||
// // Exist reports whether the given account exists in state. | ||
// // Notably this should also return true for self-destructed accounts. | ||
// Exist(common.Address) bool | ||
// // Empty returns whether the given account is empty. Empty | ||
// // is defined according to EIP161 (balance = nonce = code = 0). | ||
// Empty(common.Address) bool | ||
|
||
// AddressInAccessList(addr common.Address) bool | ||
// SlotInAccessList(addr common.Address, slot common.Hash) (addressOk bool, slotOk bool) | ||
// // AddAddressToAccessList adds the given address to the access list. This operation is safe to perform | ||
// // even if the feature/fork is not active yet | ||
// AddAddressToAccessList(addr common.Address) | ||
// // AddSlotToAccessList adds the given (address,slot) to the access list. This operation is safe to perform | ||
// // even if the feature/fork is not active yet | ||
// AddSlotToAccessList(addr common.Address, slot common.Hash) | ||
// Prepare(rules params.Rules, sender, coinbase common.Address, dest *common.Address, precompiles []common.Address, txAccesses types.AccessList) | ||
|
||
// RevertToSnapshot(int) | ||
// Snapshot() int | ||
|
||
// AddLog(*types.Log) | ||
// AddPreimage(common.Hash, []byte) | ||
|
||
// Error() error | ||
} |
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.
The StateDB
interface has been updated to extend from state.StateDBI
. This change is part of the transition from concrete types to interfaces for state management. However, there are several methods commented out in this interface. If these methods are no longer needed, consider removing them entirely to avoid confusion. If they are planned to be used in the future, consider adding more descriptive comments explaining their intended use or why they are currently commented out.
core/types/transaction_signing.go
Outdated
func (s londonSigner) PubKey(tx *Transaction) ([]byte, error) { | ||
if tx.Type() != DynamicFeeTxType { | ||
return s.eip2930Signer.PubKey(tx) | ||
} | ||
V, R, S := tx.RawSignatureValues() | ||
// DynamicFee txs are defined to use 0 and 1 as their recovery | ||
// id, add 27 to become equivalent to unprotected Homestead signatures. | ||
V = new(big.Int).Add(V, big.NewInt(27)) | ||
if tx.ChainId().Cmp(s.chainId) != 0 { | ||
return nil, fmt.Errorf("%w: have %d want %d", ErrInvalidChainId, tx.ChainId(), s.chainId) | ||
} | ||
return recoverPub(s.Hash(tx), R, S, V, true) | ||
} |
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.
The londonSigner
struct now implements the PubKey
method. The logic seems correct as it checks the transaction type and adjusts the recovery id for DynamicFee transactions. However, there's a potential issue on line 295 where s.eip2930Signer.PubKey(tx)
is called instead of s.eip2930Signer.Signature(tx)
.
- return s.eip2930Signer.PubKey(tx)
+ return s.eip2930Signer.Signature(tx)
core/types/transaction_signing.go
Outdated
func (s eip2930Signer) PubKey(tx *Transaction) ([]byte, error) { | ||
V, R, S := tx.RawSignatureValues() | ||
switch tx.Type() { | ||
case LegacyTxType: | ||
if !tx.Protected() { | ||
return HomesteadSigner{}.PubKey(tx) | ||
} | ||
V = new(big.Int).Sub(V, s.chainIdMul) | ||
V.Sub(V, big8) | ||
case AccessListTxType: | ||
// AL txs are defined to use 0 and 1 as their recovery | ||
// id, add 27 to become equivalent to unprotected Homestead signatures. | ||
V = new(big.Int).Add(V, big.NewInt(27)) | ||
default: | ||
return nil, ErrTxTypeNotSupported | ||
} | ||
if tx.ChainId().Cmp(s.chainId) != 0 { | ||
return nil, fmt.Errorf("%w: have %d want %d", ErrInvalidChainId, tx.ChainId(), s.chainId) | ||
} | ||
return recoverPub(s.Hash(tx), R, S, V, true) | ||
} | ||
|
||
func (s eip2930Signer) Signature(tx *Transaction) ([]byte, error) { | ||
V, R, S := tx.RawSignatureValues() | ||
switch tx.Type() { | ||
case LegacyTxType: | ||
if !tx.Protected() { | ||
return HomesteadSigner{}.PubKey(tx) | ||
} | ||
V = new(big.Int).Sub(V, s.chainIdMul) | ||
V.Sub(V, big8) | ||
case AccessListTxType: | ||
// AL txs are defined to use 0 and 1 as their recovery | ||
// id, add 27 to become equivalent to unprotected Homestead signatures. | ||
V = new(big.Int).Add(V, big.NewInt(27)) | ||
default: | ||
return nil, ErrTxTypeNotSupported | ||
} | ||
if tx.ChainId().Cmp(s.chainId) != 0 { | ||
return nil, fmt.Errorf("%w: have %d want %d", ErrInvalidChainId, tx.ChainId(), s.chainId) | ||
} | ||
return recoverSig(s.Hash(tx), R, S, V, true) | ||
} |
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.
The eip2930Signer
struct now implements the PubKey
and Signature
methods. The logic appears to be correct, handling different transaction types appropriately and adjusting the recovery id for AccessList transactions. However, similar to the previous comment, there's a potential issue on line 410 where HomesteadSigner{}.PubKey(tx)
is called instead of HomesteadSigner{}.Signature(tx)
.
- return HomesteadSigner{}.PubKey(tx)
+ return HomesteadSigner{}.Signature(tx)
core/types/transaction_signing.go
Outdated
func (s EIP155Signer) PubKey(tx *Transaction) ([]byte, error) { | ||
if tx.Type() != LegacyTxType { | ||
return nil, ErrTxTypeNotSupported | ||
} | ||
if !tx.Protected() { | ||
return HomesteadSigner{}.PubKey(tx) | ||
} | ||
if tx.ChainId().Cmp(s.chainId) != 0 { | ||
return nil, fmt.Errorf("%w: have %d want %d", ErrInvalidChainId, tx.ChainId(), s.chainId) | ||
} | ||
V, R, S := tx.RawSignatureValues() | ||
V = new(big.Int).Sub(V, s.chainIdMul) | ||
V.Sub(V, big8) | ||
return recoverPub(s.Hash(tx), R, S, V, true) | ||
} | ||
|
||
func (s EIP155Signer) Signature(tx *Transaction) ([]byte, error) { | ||
if tx.Type() != LegacyTxType { | ||
return nil, ErrTxTypeNotSupported | ||
} | ||
if !tx.Protected() { | ||
return HomesteadSigner{}.Signature(tx) | ||
} | ||
if tx.ChainId().Cmp(s.chainId) != 0 { | ||
return nil, fmt.Errorf("%w: have %d want %d", ErrInvalidChainId, tx.ChainId(), s.chainId) | ||
} | ||
V, R, S := tx.RawSignatureValues() | ||
V = new(big.Int).Sub(V, s.chainIdMul) | ||
V.Sub(V, big8) | ||
return recoverSig(s.Hash(tx), R, S, V, true) | ||
} |
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.
The EIP155Signer
struct now implements the PubKey
and Signature
methods. The logic seems correct, checking if the transaction is protected and adjusting the recovery id accordingly. However, there's a potential issue on line 537 where HomesteadSigner{}.PubKey(tx)
is called instead of HomesteadSigner{}.Signature(tx)
.
- return HomesteadSigner{}.PubKey(tx)
+ return HomesteadSigner{}.Signature(tx)
c8dccec
to
89db795
Compare
Rate Limit ExceededYou have exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 59 minutes and 59 seconds before requesting another review. To re-trigger a review, convert this PR to a draft and then mark it as ready for review again. Alternatively, you can push a new commit to this PR. |
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: 12
Files selected for processing (57)
- accounts/abi/bind/backends/simulated.go (4 hunks)
- accounts/abi/bind/base.go (3 hunks)
- cmd/clef/main.go (1 hunks)
- cmd/evm/internal/t8ntool/execution.go (2 hunks)
- cmd/evm/runner.go (1 hunks)
- cmd/evm/staterunner.go (1 hunks)
- cmd/geth/config.go (1 hunks)
- cmd/geth/main.go (1 hunks)
- cmd/utils/flags.go (2 hunks)
- consensus/beacon/consensus.go (2 hunks)
- consensus/clique/clique.go (1 hunks)
- consensus/consensus.go (1 hunks)
- consensus/ethash/consensus.go (2 hunks)
- consensus/misc/dao.go (1 hunks)
- core/block_validator.go (1 hunks)
- core/blockchain.go (5 hunks)
- core/blockchain_reader.go (1 hunks)
- core/chain_makers.go (3 hunks)
- core/genesis.go (2 hunks)
- core/state/access_list.go (6 hunks)
- core/state/interface.go (1 hunks)
- core/state/journal.go (1 hunks)
- core/state/state_object.go (17 hunks)
- core/state/state_test.go (1 hunks)
- core/state/statedb.go (14 hunks)
- core/state/statedb_test.go (5 hunks)
- core/state_prefetcher.go (2 hunks)
- core/state_processor.go (4 hunks)
- core/state_transition.go (4 hunks)
- core/txpool/blobpool/blobpool.go (1 hunks)
- core/txpool/blobpool/blobpool_test.go (1 hunks)
- core/txpool/blobpool/interface.go (1 hunks)
- core/txpool/legacypool/legacypool.go (3 hunks)
- core/txpool/legacypool/legacypool_test.go (3 hunks)
- core/txpool/legacypool/noncer.go (1 hunks)
- core/txpool/validation.go (1 hunks)
- core/types.go (1 hunks)
- core/types/transaction_signing.go (7 hunks)
- core/vm/contracts.go (25 hunks)
- core/vm/contracts_test.go (2 hunks)
- core/vm/eips.go (1 hunks)
- core/vm/evm.go (16 hunks)
- core/vm/instructions_test.go (1 hunks)
- core/vm/interface.go (2 hunks)
- core/vm/precompile_manager.go (1 hunks)
- core/vm/runtime/runtime.go (5 hunks)
- eth/api_backend.go (5 hunks)
- eth/api_debug.go (3 hunks)
- eth/api_debug_test.go (1 hunks)
- eth/backend.go (2 hunks)
- eth/filters/api.go (1 hunks)
- eth/filters/filter_system.go (1 hunks)
- eth/filters/filter_system_test.go (1 hunks)
- eth/protocols/snap/sync.go (1 hunks)
- eth/state_accessor.go (4 hunks)
- eth/tracers/api.go (7 hunks)
- eth/tracers/api_test.go (3 hunks)
Files not processed due to max files limit (40)
- eth/tracers/js/goja.go
- eth/tracers/logger/access_list_tracer.go
- eth/tracers/native/4byte.go
- eth/tracers/native/call_flat.go
- ethapi/addrlock.go
- ethapi/api.go
- ethapi/api_test.go
- ethapi/backend.go
- ethapi/dbapi.go
- ethapi/transaction_args.go
- ethapi/transaction_args_test.go
- ethclient/signer.go
- graphql/graphql.go
- graphql/service.go
- les/api_backend.go
- les/client.go
- les/odr_test.go
- les/state_accessor.go
- light/odr_test.go
- light/trie.go
- light/txpool.go
- miner/miner.go
- miner/miner_test.go
- miner/ordering.go
- miner/ordering_test.go
- miner/worker.go
- miner/worker_test.go
- node/node.go
- params/protocol_params.go
- signer/core/api.go
- signer/core/api_test.go
- signer/core/auditlog.go
- signer/core/cliui.go
- signer/core/stdioui.go
- signer/rules/rules.go
- signer/rules/rules_test.go
- tests/block_test_util.go
- tests/fuzzers/bls12381/precompile_fuzzer.go
- tests/state_test.go
- tests/state_test_util.go
Files skipped from review due to trivial changes (16)
- accounts/abi/bind/base.go
- cmd/clef/main.go
- core/state/access_list.go
- core/state/statedb.go
- core/state_prefetcher.go
- core/txpool/blobpool/blobpool_test.go
- core/txpool/legacypool/noncer.go
- core/txpool/validation.go
- core/types/transaction_signing.go
- core/vm/eips.go
- core/vm/instructions_test.go
- core/vm/interface.go
- eth/api_backend.go
- eth/api_debug_test.go
- eth/filters/api.go
- eth/protocols/snap/sync.go
Additional comments (Suppressed): 119
cmd/evm/runner.go (1)
- 123-127: The type of
statedb
has been changed from*state.StateDB
tostate.StateDBI
. This change suggests a shift towards an interface-based approach for state management. Ensure that all usages ofstatedb
in the codebase are compatible with this new type.cmd/evm/staterunner.go (1)
- 106-112: The function signature of the callback in
test.Run
has been updated to usestate.StateDBI
instead of*state.StateDB
. Ensure that this change is propagated throughout the codebase and that all implementations ofStateDBI
have a methodIntermediateRoot
with the same behavior as the one in*state.StateDB
.core/block_validator.go (1)
- 124-127: The function signature of
ValidateState
has been changed to acceptstate.StateDBI
instead of*state.StateDB
. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that the interfacestate.StateDBI
provides all the necessary methods that were previously available in*state.StateDB
.cmd/geth/main.go (1)
- 32-40: The import path for the
ethapi
package has been updated fromgithub.com/ethereum/go-ethereum/internal/ethapi
togithub.com/ethereum/go-ethereum/ethapi
. Ensure that this new path is correct and accessible, and that it doesn't break any dependencies.consensus/misc/dao.go (1)
- 72-78: The function signature for
ApplyDAOHardFork
has been updated to accept an interfacestate.StateDBI
instead of a pointer tostate.StateDB
. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that the implementation ofStateDBI
correctly handles theExist
andCreateAccount
methods.cmd/utils/flags.go (2)
54-64: The import path for
ethapi
has been updated fromgithub.com/ethereum/go-ethereum/internal/ethapi
togithub.com/ethereum/go-ethereum/ethapi
. Ensure that this new path is correct and accessible, and that it doesn't break any dependencies.1954-1962: The function signature of
RegisterFilterAPI
has been changed. The first parameter type has been altered from*node.Node
toNetworkingStack
, which is an interface. This change implies a shift towards a more abstracted approach, allowing any type that implements theNetworkingStack
interface to be passed as an argument. Make sure all calls to this function throughout the codebase have been updated to match the new signature.consensus/consensus.go (2)
91-92: The
Finalize
method signature has been updated to acceptstate.StateDBI
instead of*state.StateDB
. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.99-100: The
FinalizeAndAssemble
method signature has been updated to acceptstate.StateDBI
instead of*state.StateDB
. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.core/blockchain.go (5)
1404-1407: The function signature of
writeBlockWithState
has been changed to acceptstate.StateDBI
instead of*state.StateDB
. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.1492-1495: The function signature of
WriteBlockAndSetHead
has been changed to acceptstate.StateDBI
instead of*state.StateDB
. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.1503-1506: The function signature of
writeBlockAndSetHead
has been changed to acceptstate.StateDBI
instead of*state.StateDB
. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.1729-1732: The type of
activeState
has been changed from*state.StateDB
tostate.StateDBI
. Ensure that all operations onactiveState
are compatible with the new interface-based approach.1813-1816: The anonymous goroutine now accepts
throwaway state.StateDBI
instead ofthrowaway *state.StateDB
. Ensure that thebc.prefetcher.Prefetch
method is compatible with the new interface-based approach.cmd/evm/internal/t8ntool/execution.go (2)
118-123: The function signature of
Apply
has been changed to returnstate.StateDBI
instead of*state.StateDB
. Ensure that all calls to this function throughout the codebase have been updated to handle the new return type. Also, verify if the change aligns with the overall design and doesn't introduce any unintended side effects.338-341: The function
MakePreState
now returnsstate.StateDBI
instead of*state.StateDB
. This change should be reflected wherever this function is called. Additionally, the error returned bystate.New
is being ignored. It's a good practice to handle errors rather than ignoring them. Consider adding error handling here.- statedb, _ := state.New(types.EmptyRootHash, sdb, nil) + statedb, err := state.New(types.EmptyRootHash, sdb, nil) + if err != nil { + return nil, err + }consensus/clique/clique.go (2)
570-574: The
Finalize
function has been updated to use the newstate.StateDBI
interface instead of the*state.StateDB
type. The function now also sets theRoot
andUncleHash
fields of the header, which were not set in the previous version. Ensure that these changes do not introduce any unexpected behavior.578-581: The
FinalizeAndAssemble
function has also been updated to use thestate.StateDBI
interface. The function's behavior remains the same, returning an error if there are any withdrawals since Clique does not support them. This change is consistent with the shift towards an interface-based approach for state management.consensus/ethash/consensus.go (3)
490-493: The function signature of
Finalize
has been updated to use the interfacestate.StateDBI
instead of the pointer type*state.StateDB
. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that the implementation ofstate.StateDBI
is correctly handled in theaccumulateRewards
function.497-500: Similar to the
Finalize
function, theFinalizeAndAssemble
function now usesstate.StateDBI
instead of*state.StateDB
. Again, ensure that all calls to this function throughout the codebase have been updated to match the new signature. The error handling for unsupported withdrawals remains unchanged.547-553: The
accumulateRewards
function now acceptsstate.StateDBI
instead of*state.StateDB
. As before, ensure that all calls to this function throughout the codebase have been updated to match the new signature. The logic within the function appears to be unchanged based on the provided hunk.consensus/beacon/consensus.go (2)
350-355: The function signature of
Finalize()
has been updated to acceptstate.StateDBI
instead of*state.StateDB
. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that theFinalize()
method in theethone
engine is compatible with the newstate.StateDBI
type.369-374: The function signature of
FinalizeAndAssemble()
has been updated to acceptstate.StateDBI
instead of*state.StateDB
. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that theFinalizeAndAssemble()
method in theethone
engine is compatible with the newstate.StateDBI
type.core/genesis.go (2)
135-140: The addition of error handling after setting the code and nonce for an account in the state database is a good practice. It ensures that any errors that occur during these operations are caught and handled immediately, preventing potential inconsistencies in the state database.
159-164: Similar to the previous comment, the addition of error handling after setting the code and nonce for an account in the state database is a good practice. This change improves the robustness of the code by ensuring that errors are not silently ignored.
cmd/geth/config.go (1)
- 35-41: The import path for the
ethapi
package has been updated fromgithub.com/ethereum/go-ethereum/internal/ethapi
togithub.com/ethereum/go-ethereum/ethapi
. Ensure that this new path is correct and accessible, and that it doesn't break any dependencies.core/state/interface.go (1)
- 29-106: The introduction of the
StateDBI
interface is a significant change that affects the state management in EVM. This interface-based approach provides flexibility and makes it easier to implement different versions of state databases. However, ensure that all implementations of this interface correctly implement all methods. Also, verify that all parts of the codebase that previously used*state.StateDB
have been updated to useStateDBI
.- *state.StateDB + state.StateDBIcore/chain_makers.go (3)
41-44: The type of
statedb
has been changed from a pointer tostate.StateDB
to an interfacestate.StateDBI
. This change suggests a shift towards an interface-based approach for state management. Ensure that all methods previously called onstatedb
are defined in theStateDBI
interface and that the actual object implementing this interface implements these methods.292-294: The function
genblock
now acceptsstate.StateDBI
instead of*state.StateDB
as an argument. This is consistent with the change made in theBlockGen
struct. Make sure that the passed object correctly implements theStateDBI
interface.374-377: The function
makeHeader
now acceptsstate.StateDBI
instead of*state.StateDB
as an argument. This is consistent with the changes made elsewhere in the code. Ensure that the passed object correctly implements theStateDBI
interface.accounts/abi/bind/backends/simulated.go (4)
66-70: The type of
pendingState
has been changed from*state.StateDB
tostate.StateDBI
. This change is part of a larger refactoring effort to use interfaces for state management. Ensure that all usages ofpendingState
throughout the codebase have been updated to work with the new interface type.182-185: The return type of
stateByBlockNumber
has been changed from*state.StateDB
tostate.StateDBI
. This change is part of a larger refactoring effort to use interfaces for state management. Ensure that all calls to this function throughout the codebase have been updated to handle the new return type.587-590: The condition in line 587 has been changed from
hi == cap
tohi >= cap
. This change seems to be intended to catch cases wherehi
somehow exceedscap
, not just when it equalscap
. Please verify that this change does not introduce any unexpected behavior or off-by-one errors.608-611: The parameter type of
stateDB
incallContract
has been changed from*state.StateDB
tostate.StateDBI
. This change is part of a larger refactoring effort to use interfaces for state management. Ensure that all calls to this function throughout the codebase have been updated to pass an instance ofstate.StateDBI
.core/state/journal.go (1)
- 91-97: The
resetObjectChange
struct has been updated to use the exportedStateObject
type instead of the previously unexportedstateObject
. Ensure that all instances of this struct have been updated accordingly throughout the codebase. Also, verify that the change fromstateObject
toStateObject
does not introduce any unintended side effects due to differences in behavior between these two types.core/txpool/legacypool/legacypool_test.go (3)
63-68: The type of
statedb
has been changed from a pointer tostate.StateDB
to the interfacestate.StateDBI
. This change is in line with the PR's goal of shifting towards an interface-based approach for state management. Ensure that all methods and functions that interact withstatedb
are compatible with this change.91-97: The return type of
StateAt()
andStateAtBlockNumber()
methods have been updated to returnstate.StateDBI
instead of*state.StateDB
. This aligns with the changes made to thestatedb
field in thetestBlockChain
struct. Make sure that all calls to these methods are updated accordingly.253-256: The return type of the
State()
method has been updated tostate.StateDBI
from*state.StateDB
, consistent with the changes made elsewhere in the codebase. Verify that all calls to this method are updated to handle the new return type.core/types.go (3)
36-36: The function signature of
ValidateState
has been changed to acceptstate.StateDBI
instead of*state.StateDB
. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.44-44: The function signature of
Prefetch
has been changed to acceptstate.StateDBI
instead of*state.StateDB
. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.52-52: The function signature of
Process
has been changed to acceptstate.StateDBI
instead of*state.StateDB
. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.core/state/statedb_test.go (5)
176-182: The
Copy()
method now returns an interface (StateDBI
) instead of a concrete type (*StateDB
). This change is reflected by the type assertion to*StateDB
in the new hunk. Ensure that all implementations of theStateDBI
interface have aCopy()
method that returns a copy of the correct type.597-600: Similar to the previous comment, ensure that all implementations of the
StateDBI
interface have aCopy()
method that returns a copy of the correct type.684-687: Again, ensure that all implementations of the
StateDBI
interface have aCopy()
method that returns a copy of the correct type.869-872: The
NewAccessList()
function has replaced thenewAccessList()
. Make sure that this function is correctly implemented and exported as it's now part of the package's public API.1021-1024: Ensure that all implementations of the
StateDBI
interface have aCopy()
method that returns a copy of the correct type.core/state_transition.go (1)
- 402-405: The call to
st.state.Prepare()
has been updated to usest.evm.PrecompileManager.GetActive(&rules)
instead ofvm.ActivePrecompiles(rules)
. Ensure that this change does not affect the functionality of the code, as theGetActive
method might have a different implementation thanActivePrecompiles
.core/vm/runtime/runtime.go (3)
50-52: The type of
State
in theConfig
struct has been changed from*state.StateDB
tostate.StateDBI
. This change suggests a shift towards an interface-based approach for state management. Ensure that all uses of this field throughout the codebase have been updated to work with the new interface type.184-184: The method
vmenv.PrecompileManager.GetActive(&rules)
is used instead ofvm.ActivePrecompiles(rules)
. This change indicates a new way of getting active precompiled contracts. Make sure that thePrecompileManager
correctly implements the retrieval of active precompiled contracts and that it's properly tested.105-108: The function signature of
Execute()
has been updated to returnstate.StateDBI
instead of*state.StateDB
. Ensure that all calls to this function throughout the codebase have been updated to handle the new return type.core/txpool/blobpool/blobpool.go (1)
- 302-302: The type of
state
has been changed from a pointer tostate.StateDB
to an interfacestate.StateDBI
. This change suggests a shift towards an interface-based approach for state management. Ensure that all methods and functions that interact withstate
are compatible with this new interface.eth/api_debug.go (3)
28-34: The import path for the
ethapi
package has been updated fromgithub.com/ethereum/go-ethereum/internal/ethapi
togithub.com/ethereum/go-ethereum/ethapi
. Ensure that this new path is correct and accessible.136-140: The type of
stateDb
has been changed from*state.StateDB
tostate.StateDBI
, indicating a shift towards an interface-based approach. Make sure all methods called onstateDb
are defined in theStateDBI
interface.232-235: Similar to the previous comment, the type of
statedb
in thestorageRangeAt
function has been changed from*state.StateDB
tostate.StateDBI
. Again, ensure that all methods called onstatedb
are defined in theStateDBI
interface.core/txpool/blobpool/interface.go (1)
- 42-43: The function signature for
StateAt
has been changed to returnstate.StateDBI
instead of*state.StateDB
. Ensure that all calls to this function throughout the codebase have been updated to handle the new return type. Also, verify that the implementation of this method is correctly returning an instance of a type that satisfies thestate.StateDBI
interface.core/txpool/legacypool/legacypool.go (2)
119-126: The function signatures for
StateAt
andStateAtBlockNumber
have been updated to returnstate.StateDBI
instead of*state.StateDB
. Ensure that all calls to these functions throughout the codebase have been updated to handle the new return type. Also, verify that the implementation of these methods correctly returns an instance of a type that satisfies thestate.StateDBI
interface.218-219: The type of
currentState
has been changed from*state.StateDB
tostate.StateDBI
. This change aligns with the shift towards an interface-based approach for state management. Make sure that all usages ofcurrentState
in the code are compatible with this change.core/state/state_test.go (1)
- 249-252: The function
compareStateObjects
has been updated to accept arguments of type*StateObject
instead of*stateObject
. Ensure that all calls to this function throughout the codebase have been updated to match the new argument types. Also, verify that the change fromstateObject
toStateObject
does not expose any internal state or methods that should remain unexported.eth/filters/filter_system.go (1)
- 545-551: The call to
es.txsSub.Unsubscribe()
has been commented out. This could potentially lead to memory leaks as the subscription may not be properly cleaned up when the event loop ends. If this is intentional, please verify and provide a comment explaining why this line is commented out.- // es.txsSub.Unsubscribe() + es.txsSub.Unsubscribe()core/vm/evm.go (15)
19-23: The import of the
errors
package is new in this hunk. Ensure that it's used appropriately in the code.102-109: The
PrecompileManager
field has been added to theEVM
struct. This change suggests a shift towards an interface-based approach for managing and executing precompiled contracts in the EVM.137-142: The instantiation of
PrecompileManager
usingNewPrecompileManager()
withinNewEVM()
is a new addition. Make sure thatNewPrecompileManager()
is implemented correctly and that it properly initializes thePrecompileManager
.144-161: A new function
NewEVMWithPrecompiles()
has been introduced, which returns a new EVM instance with a precompile manager. This function seems to be a variant ofNewEVM()
, allowing the caller to provide a customPrecompileManager
. Ensure that all calls to this function throughout the codebase are correct and that the providedPrecompileManager
is valid.186-192: New getter methods
GetStateDB()
andGetContext()
have been added to theEVM
struct. These methods improve encapsulation by providing controlled access to theStateDB
andContext
fields.208-209: The method of checking if a contract is precompiled has changed from
evm.precompile(addr)
toevm.PrecompileManager.Has(addr)
. Ensure that theHas()
method is implemented correctly and that it accurately determines whether a contract is precompiled.245-248: The execution of precompiled contracts has been updated to use the
Run()
method of thePrecompileManager
. Ensure that this method is implemented correctly and that it properly executes precompiled contracts.269-272: The error handling has been updated to use the
errors.Is()
function for checking if the error isErrExecutionReverted
. This change improves the robustness of the error handling by allowing for wrapped errors.309-313: Similar to the changes in lines 245-248, the execution of precompiled contracts in the
CallCode()
method has been updated to use theRun()
method of thePrecompileManager
.323-327: The error handling in the
CallCode()
method has been updated to use theerrors.Is()
function, similar to the changes in lines 269-272.356-361: The execution of precompiled contracts in the
DelegateCall()
method has been updated to use theRun()
method of thePrecompileManager
, similar to the changes in lines 245-248 and 309-313.370-375: The error handling in the
DelegateCall()
method has been updated to use theerrors.Is()
function, similar to the changes in lines 269-272 and 323-327.411-415: The execution of precompiled contracts in the
StaticCall()
method has been updated to use theRun()
method of thePrecompileManager
, similar to the previous changes.430-435: The error handling in the
StaticCall()
method has been updated to use theerrors.Is()
function, similar to the previous changes.485-487: Error handling has been added after the
StateDB.AddBalance()
andStateDB.SetNonce()
calls. This is a good practice as it ensures that any errors that occur during these operations are caught and handled appropriately.eth/state_accessor.go (4)
40-43: The function signature for
hashState()
has been updated to use thestate.StateDBI
interface instead of the concrete type*state.StateDB
. This change promotes better abstraction and decoupling, allowing different implementations of theStateDBI
interface to be used interchangeably. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.177-180: Similar to the
hashState()
function, thepathState()
function's signature has also been updated to use thestate.StateDBI
interface. This change is consistent with the shift towards an interface-based approach for state management. Verify that all calls to this function are updated accordingly.211-216: The
stateAtBlock()
function now accepts and returnsstate.StateDBI
instead of*state.StateDB
, aligning with the changes made in thehashState()
andpathState()
functions. This change enhances modularity and flexibility in the code by allowing differentStateDBI
implementations to be used. Make sure all calls to this function are updated to reflect these changes.240-261: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [219-261]
The
stateAtTransaction()
function has been updated to returnstate.StateDBI
instead of*state.StateDB
. This change is in line with the rest of the updates in this file, promoting an interface-based approach for state management. Additionally, the creation of the EVM environment (line 248) now useseth.APIBackend.GetEVM()
, which might provide more flexibility and control over the EVM configuration. Ensure that all calls to this function are updated to match the new signature and that the new method for creating the EVM environment does not introduce any unexpected behavior.core/vm/contracts_test.go (2)
19-27: The import of the
context
andmath/big
packages is new in this hunk. Ensure that these imports are necessary for the code changes in this file.98-106: The function
RunPrecompiledContract
is newly introduced. It seems to execute a precompiled contract, checking if there's enough gas supplied before running the contract. If not, it returns anErrOutOfGas
error. This function appears to be a utility function for testing purposes. Please ensure that the context being passed to thep.Run
method on line 104 is appropriate for your use case.eth/tracers/api.go (7)
36-43: The import path for
ethapi
has been updated fromgithub.com/ethereum/go-ethereum/internal/ethapi
togithub.com/ethereum/go-ethereum/ethapi
. Ensure that this new path is correct and accessible.85-90: The function signatures for
StateAtBlock
andStateAtTransaction
have been updated to use thestate.StateDBI
interface instead of the*state.StateDB
pointer. Make sure all calls to these functions throughout the codebase have been updated to match the new signatures.183-189: The
statedb
field in theblockTraceTask
struct has been updated to use thestate.StateDBI
interface instead of the*state.StateDB
pointer. This change should not affect the functionality of the code, but it's a good practice to ensure that all usages of this struct are still valid with this change.200-205: The
statedb
field in thetxTraceTask
struct has been updated to use thestate.StateDBI
interface instead of the*state.StateDB
pointer. This change should not affect the functionality of the code, but it's a good practice to ensure that all usages of this struct are still valid with this change.308-314: The
statedb
variable has been updated to use thestate.StateDBI
interface instead of the*state.StateDB
pointer. This change should not affect the functionality of the code, but it's a good practice to ensure that all usages of this variable are still valid with this change.626-632: The
statedb
parameter in thetraceBlockParallel
function has been updated to use thestate.StateDBI
interface instead of the*state.StateDB
pointer. Make sure all calls to this function throughout the codebase have been updated to match the new signature.922-928: The
statedb
parameter in thetraceTx
function has been updated to use thestate.StateDBI
interface instead of the*state.StateDB
pointer. Make sure all calls to this function throughout the codebase have been updated to match the new signature.eth/backend.go (2)
44-52: The import path for the
ethapi
package has been updated fromgithub.com/ethereum/go-ethereum/internal/ethapi
togithub.com/ethereum/go-ethereum/ethapi
. Ensure that this new path is correct and accessible.299-305: The function call
ethapi.GetAPIs(s.APIBackend, s.BlockChain())
now includes an additional arguments.BlockChain()
. Make sure all calls to this function have been updated accordingly throughout the codebase.core/blockchain_reader.go (2)
323-325: The function
State()
now returns an interfacestate.StateDBI
instead of a pointer tostate.StateDB
. This change is part of the shift towards an interface-based approach for state management. Ensure that all calls to this function throughout the codebase have been updated to handle the new return type.328-330: Similar to the
State()
function, theStateAt()
function now returns an interfacestate.StateDBI
instead of a pointer tostate.StateDB
. Make sure all calls to this function are updated accordingly.core/state_processor.go (6)
61-64: The function signature of
Process
has been updated to acceptstate.StateDBI
instead of*state.StateDB
. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.90-93: The order of arguments in the
applyTransaction
function call has been changed. Thevmenv
andmsg
parameters are now passed first, followed by the rest of the parameters. This change should not cause any issues as long as it is consistent across all calls toapplyTransaction
.108-114: The function signature of
applyTransaction
has been updated to acceptstate.StateDBI
instead of*state.StateDB
, and the order of arguments has been changed. Ensure that all calls to this function throughout the codebase have been updated to match the new signature and argument order.161-207: A new function
applyTransactionWithResult
has been introduced. It appears to be a variant ofapplyTransaction
that also returns anExecutionResult
. This function seems to follow good coding practices, with proper error handling and usage of existing functions likeApplyMessage
andcrypto.CreateAddress
. However, ensure that this function is used correctly in the codebase, especially considering its side effects (like state changes).213-250: The function signatures of
ApplyTransaction
,ApplyTransactionWithResult
,ApplyTransactionWithEVM
, andApplyTransactionWithEVMWithResult
have been updated to acceptstate.StateDBI
instead of*state.StateDB
. Ensure that all calls to these functions throughout the codebase have been updated to match the new signatures.266-269: The function signature of
ProcessBeaconBlockRoot
has been updated to acceptstate.StateDBI
instead of*state.StateDB
. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.eth/filters/filter_system_test.go (1)
- 35-43: The import path for the
ethapi
package has been updated fromgithub.com/ethereum/go-ethereum/internal/ethapi
togithub.com/ethereum/go-ethereum/ethapi
. Ensure that this new path is correct and accessible, and that it does not break any dependencies.core/vm/contracts.go (3)
42-48: The
PrecompiledContract
interface has been updated with two new methods:RegistryKey()
and an updatedRun()
method. Ensure that all implementations of this interface throughout the codebase have been updated to include these new methods.156-159: The function signature for
ActivePrecompiles
has changed from accepting a value of typeparams.Rules
to a pointer to aparams.Rules
. Make sure all calls to this function are updated accordingly.741-750: The
Run
method in various contracts now includes additional parameters:ctx context.Context
,evm PrecompileEVM
,caller common.Address
, andvalue *big.Int
. Ensure that all calls to this method are updated accordingly.eth/tracers/api_test.go (3)
42-42: The import path for the
ethapi
package has been updated fromgithub.com/ethereum/go-ethereum/internal/ethapi
togithub.com/ethereum/go-ethereum/ethapi
. Please ensure that this new path is correct and accessible, and that it does not break any dependencies.142-145: The function signature of
StateAtBlock
has been changed. The type of thebase
parameter and the return type have been updated from*state.StateDB
tostate.StateDBI
. This change suggests a shift towards an interface-based approach for state management. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.158-161: The function signature of
StateAtTransaction
has been changed. The return type has been updated from*state.StateDB
tostate.StateDBI
. This change suggests a shift towards an interface-based approach for state management. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.core/state/state_object.go (15)
57-66: The
stateObject
struct has been renamed toStateObject
, making it an exported type. This change will affect all the places in the codebase where this struct is used. Ensure that all references to this struct have been updated accordingly.100-108: The
newObject
function now returns a*StateObject
instead of a*stateObject
. This change is consistent with the renaming of the struct, but make sure that all calls to this function have been updated to handle the new return type.118-120: The
EncodeRLP
method now operates on a*StateObject
instead of a*stateObject
. This change is consistent with the renaming of the struct, but ensure that all calls to this method have been updated accordingly.140-143: The
getTrie
method now operates on a*StateObject
instead of a*stateObject
. This change is consistent with the renaming of the struct, but ensure that all calls to this method have been updated accordingly.159-162: The
GetState
method now operates on a*StateObject
instead of a*stateObject
. This change is consistent with the renaming of the struct, but ensure that all calls to this method have been updated accordingly.170-173: The
GetCommittedState
method now operates on a*StateObject
instead of a*stateObject
. This change is consistent with the renaming of the struct, but ensure that all calls to this method have been updated accordingly.230-233: The
SetState
method now operates on a*StateObject
instead of a*stateObject
. This change is consistent with the renaming of the struct, but ensure that all calls to this method have been updated accordingly.273-276: The
updateTrie
method now operates on a*StateObject
instead of a*stateObject
. This change is consistent with the renaming of the struct, but ensure that all calls to this method have been updated accordingly.361-364: The
updateRoot
method now operates on a*StateObject
instead of a*stateObject
. This change is consistent with the renaming of the struct, but ensure that all calls to this method have been updated accordingly.378-381: The
commit
method now operates on a*StateObject
instead of a*stateObject
. This change is consistent with the renaming of the struct, but ensure that all calls to this method have been updated accordingly.404-407: The
AddBalance
method now operates on a*StateObject
instead of a*stateObject
. This change is consistent with the renaming of the struct, but ensure that all calls to this method have been updated accordingly.418-422: The
SubBalance
method now operates on a*StateObject
instead of a*stateObject
. This change is consistent with the renaming of the struct, but ensure that all calls to this method have been updated accordingly.500-503: The
SetCode
method now operates on a*StateObject
instead of a*stateObject
. This change is consistent with the renaming of the struct, but ensure that all calls to this method have been updated accordingly.516-521: The
SetNonce
method now operates on a*StateObject
instead of a*stateObject
. This change is consistent with the renaming of the struct, but ensure that all calls to this method have been updated accordingly.540-542: All these methods now operate on a
*StateObject
instead of a*stateObject
. These changes are consistent with the renaming of the struct, but ensure that all calls to these methods have been updated accordingly.
SelfDestruct(common.Address) | ||
HasSelfDestructed(common.Address) bool | ||
|
||
Selfdestruct6780(common.Address) |
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.
The method Selfdestruct6780(common.Address)
lacks a comment explaining its purpose. It's important to document what this function does, especially given its name doesn't provide much context.
TxIndex() int | ||
Preimages() map[common.Hash][]byte | ||
|
||
GetOrNewStateObject(addr common.Address) *StateObject |
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.
The method GetOrNewStateObject(addr common.Address) *StateObject
returns a pointer to a StateObject
. If multiple goroutines call this method and manipulate the returned StateObject
, it could lead to data races. Consider returning a copy of the StateObject
or using synchronization primitives to prevent concurrent access.
core/state_transition.go
Outdated
|
||
st.initialGas = st.msg.GasLimit | ||
st.state.SubBalance(st.msg.From, mgval) | ||
return nil | ||
return st.state.Error() |
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.
The function st.state.SubBalance(st.msg.From, mgval)
is called and then immediately after, the error from the state is returned. This change might cause unexpected behavior if SubBalance
method doesn't set an error in the state when it fails. It would be better to modify the SubBalance
method to return an error directly, which can be checked here.
- st.state.SubBalance(st.msg.From, mgval)
- return st.state.Error()
+ if err := st.state.SubBalance(st.msg.From, mgval); err != nil {
+ return err
+ }
+ return nil
core/state_transition.go
Outdated
} else { | ||
// Increment the nonce for the next transaction | ||
st.state.SetNonce(msg.From, st.state.GetNonce(sender.Address())+1) | ||
if err := st.state.Error(); err != nil { | ||
return nil, err | ||
} |
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.
Similar to the previous comment, the error checking after st.state.SetNonce(msg.From, st.state.GetNonce(sender.Address())+1)
could lead to unexpected behavior if SetNonce
doesn't set an error in the state when it fails. Consider modifying SetNonce
to return an error directly.
- st.state.SetNonce(msg.From, st.state.GetNonce(sender.Address())+1)
- if err := st.state.Error(); err != nil {
- return nil, err
- }
+ if err := st.state.SetNonce(msg.From, st.state.GetNonce(sender.Address())+1); err != nil {
+ return nil, err
+ }
core/state_transition.go
Outdated
fee := new(big.Int).SetUint64(st.gasUsed()) | ||
fee.Mul(fee, effectiveTip) | ||
st.state.AddBalance(st.evm.Context.Coinbase, fee) | ||
if err := st.state.Error(); err != nil { | ||
return nil, err | ||
} |
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.
Again, the error checking after st.state.AddBalance(st.evm.Context.Coinbase, fee)
could lead to unexpected behavior if AddBalance
doesn't set an error in the state when it fails. Consider modifying AddBalance
to return an error directly.
- st.state.AddBalance(st.evm.Context.Coinbase, fee)
- if err := st.state.Error(); err != nil {
- return nil, err
- }
+ if err := st.state.AddBalance(st.evm.Context.Coinbase, fee); err != nil {
- return nil, err
+ }
core/vm/precompile_manager.go
Outdated
// NewPrecompileManager returns a new PrecompileManager for the current chain rules. | ||
func NewPrecompileManager(rules *params.Rules) PrecompileManager { | ||
return &precompileManager{ | ||
rules: rules, | ||
} | ||
} |
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.
The NewPrecompileManager
function initializes a new precompileManager
with the provided rules but does not initialize the precompiles
map. This could lead to potential null pointer exceptions if the Has
or Get
methods are called before the precompiles
map is initialized. Consider initializing the precompiles
map in the NewPrecompileManager
function.
func NewPrecompileManager(rules *params.Rules) PrecompileManager {
return &precompileManager{
rules: rules,
+ precompiles: activePrecompiles(rules),
}
}
core/vm/precompile_manager.go
Outdated
// Has returns whether a precompiled contract is deployed at the given address. | ||
func (pm *precompileManager) Has(addr common.Address) bool { | ||
if pm.precompiles == nil { | ||
pm.precompiles = activePrecompiles(pm.rules) | ||
} | ||
_, found := pm.precompiles[addr] | ||
return found | ||
} |
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.
The Has
method checks if a precompiled contract exists for a given address. However, it also initializes the precompiles
map if it's not already initialized. This side effect might be unexpected for a function named Has
. Consider separating the initialization of the precompiles
map from this method to improve clarity and maintainability.
core/vm/precompile_manager.go
Outdated
// Get returns the precompiled contract deployed at the given address. | ||
func (pm *precompileManager) Get(addr common.Address) PrecompiledContract { | ||
if pm.precompiles == nil { | ||
pm.precompiles = activePrecompiles(pm.rules) | ||
} | ||
return pm.precompiles[addr] | ||
} |
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.
Similar to the Has
method, the Get
method also initializes the precompiles
map if it's not already initialized. This side effect might be unexpected for a function named Get
. Consider separating the initialization of the precompiles
map from this method to improve clarity and maintainability.
core/vm/precompile_manager.go
Outdated
// GetActive sets the chain rules on the precompile manager and returns the list of active | ||
// precompile addresses. | ||
func (pm *precompileManager) GetActive(rules *params.Rules) []common.Address { | ||
pm.rules = rules | ||
return ActivePrecompiles(pm.rules) | ||
} |
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.
The GetActive
method sets the chain rules on the precompile manager and returns the list of active precompile addresses. However, it modifies the state of the precompile manager by changing its rules
field. This side effect might be unexpected for a function named GetActive
. Consider renaming this method to reflect that it changes the state of the precompile manager, or refactor the code to avoid changing the state.
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: 2
Files selected for processing (5)
- eth/backend.go (3 hunks)
- miner/miner.go (3 hunks)
- miner/miner_test.go (3 hunks)
- miner/worker.go (11 hunks)
- miner/worker_test.go (1 hunks)
Additional comments (Suppressed): 22
miner/worker_test.go (1)
- 147-148: Ensure that all calls to
BlockChain()
andTxPool()
throughout the codebase have been updated to match the new return types. Also, verify if any changes are required for the newly introducedMinerChain()
method.eth/backend.go (2)
44-52: The import path for
ethapi
has been changed fromgithub.com/ethereum/go-ethereum/internal/ethapi
togithub.com/ethereum/go-ethereum/ethapi
. Ensure that the new path is correct and all functionalities are working as expected.299-305: The function signature of
ethapi.GetAPIs()
has been modified to include an additional parameters.BlockChain()
. Make sure that this change doesn't break any existing calls to this function and the added parameter is handled correctly in the function definition.miner/miner.go (3)
40-45: The
Backend
interface has been updated to include a new methodMinerChain()
. Ensure that all implementations of this interface have been updated accordingly.47-58: The
BlockChain
interface has been significantly expanded with new methods. This change enhances the modularity and flexibility of the codebase, but it's important to ensure that all implementations of this interface are updated to include these new methods. Also, the return type ofStateAt()
andStateAtBlockNumber()
methods has been changed from*state.StateDB
tostate.StateDBI
, which indicates a shift from a concrete implementation to an interface-based approach. This change should improve the testability and maintainability of the code.221-223: The return type of the
Pending()
method has been changed from(*types.Block, *state.StateDB)
to(*types.Block, state.StateDBI)
. This change aligns with the shift towards an interface-based approach for managing state databases. Make sure to update all calls to this method throughout the codebase to handle the new return type.miner/miner_test.go (5)
42-45: The type of
bc
inmockBackend
struct has been changed from a pointer tocore.BlockChain
toBlockChain
. This change indicates that the code is moving towards an interface-based approach, which can improve modularity and testability. However, ensure that all methods that were available on*core.BlockChain
are also available on theBlockChain
interface.54-60: The method
MinerChain()
has been added to themockBackend
struct. This method returns the blockchain associated with the miner. Ensure that this method is used appropriately throughout the codebase.66-68: The function signature of
StateAtBlock
has been updated to returnstate.StateDBI
instead of*state.StateDB
. This aligns with the shift towards using interfaces. However, make sure that all calls to this function have been updated accordingly.71-75: The type of
statedb
intestBlockChain
struct has been changed from a pointer tostate.StateDB
tostate.StateDBI
. This change indicates that the code is moving towards an interface-based approach, which can improve modularity and testability. However, ensure that all methods that were available on*state.StateDB
are also available on thestate.StateDBI
interface.92-98: The function signatures of
StateAt
andStateAtBlockNumber
have been updated to returnstate.StateDBI
instead of*state.StateDB
. This aligns with the shift towards using interfaces. However, make sure that all calls to these functions have been updated accordingly.miner/worker.go (11)
86-89: The
state
field in theenvironment
struct has been changed from a pointer to an interface. This change enhances modularity and allows for different implementations of the state database. Ensure that all usages of this field are updated accordingly.134-137: Similar to the previous comment, the
state
field in thetask
struct has been changed from a pointer to an interface. Make sure all usages of this field are updated to match the new type.179-182: The
chain
field in theworker
struct has been changed from a pointer to an interface. This change improves flexibility by allowing different blockchain implementations. Verify that all usages of this field have been updated to match the new type.215-218: The
snapshotState
field in theworker
struct has been changed from a pointer to an interface. This change enhances modularity and allows for different implementations of the state snapshot. Ensure that all usages of this field are updated accordingly.249-252: The method used to get the blockchain from the Ethereum backend has been changed from
eth.BlockChain()
toeth.MinerChain()
. Verify that this new method returns the expected results and that it doesn't introduce any breaking changes.269-272: The method used to subscribe to chain head events has been changed from
eth.BlockChain().SubscribeChainHeadEvent(worker.chainHeadCh)
toeth.MinerChain().SubscribeChainHeadEvent(worker.chainHeadCh)
. Confirm that this new method works as expected and that it doesn't introduce any breaking changes.340-343: The return type of the
pending()
method has been changed from a pointer to an interface. This change enhances modularity and allows for different implementations of the pending state. Ensure that all usages of this method are updated accordingly.552-555: The
newTransactionsByPriceAndNonce
function has been renamed toNewTransactionsByPriceAndNonce
. This change makes the function exported, allowing it to be used outside of its package. Verify that this change is intentional and doesn't introduce any security issues.708-715: Error handling has been added when getting the state at a specific block root. If an error occurs, the code now tries to get the state at a specific block number instead. This change improves robustness by providing a fallback mechanism. However, ensure that the use of
w.chain.StateAtBlockNumber(parent.Number.Uint64())
is appropriate in this context.797-800: The
commitTransactions
method's second parameter type has been changed from a pointer to an interface. This change enhances modularity and allows for different implementations of transactions sorted by price and nonce. Ensure that all calls to this method have been updated accordingly.993-1002: The
newTransactionsByPriceAndNonce
function has been renamed toNewTransactionsByPriceAndNonce
. This change makes the function exported, allowing it to be used outside of its package. Verify that this change is intentional and doesn't introduce any security issues.
func (b *testWorkerBackend) BlockChain() BlockChain { return b.chain } | ||
func (b *testWorkerBackend) MinerChain() BlockChain { return b.chain } |
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.
The function MinerChain()
is newly introduced and it seems to return the same value as BlockChain()
. If these two functions are intended to serve different purposes, consider adding comments to clarify their roles. If they are meant to be identical, consider removing one to avoid redundancy.
- func (b *testWorkerBackend) BlockChain() BlockChain { return b.chain }
- func (b *testWorkerBackend) MinerChain() BlockChain { return b.chain }
+ // BlockChain returns the blockchain associated with the test worker backend.
+ func (b *testWorkerBackend) BlockChain() BlockChain { return b.chain }
@@ -468,6 +467,7 @@ func (s *Ethereum) Miner() *miner.Miner { return s.miner } | |||
|
|||
func (s *Ethereum) AccountManager() *accounts.Manager { return s.accountManager } | |||
func (s *Ethereum) BlockChain() *core.BlockChain { return s.blockchain } | |||
func (s *Ethereum) MinerChain() miner.BlockChain { return s.blockchain } |
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.
A new method MinerChain()
has been introduced. This seems to return the blockchain object just like the BlockChain()
method. If these two methods serve the same purpose, consider removing one to avoid redundancy. If they serve different purposes, ensure that the differences are well-documented.
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/state_processor.go (5 hunks)
Additional comments (Suppressed): 7
core/state_processor.go (7)
- 35-54: The
StateProcessor
struct has been updated to use an interfaceSPBlockchain
instead of a concrete type*BlockChain
. This change enhances modularity and flexibility, allowing any type that implements theSPBlockchain
interface to be used with theStateProcessor
. Ensure all implementations ofSPBlockchain
adhere to its defined methods.- bc *BlockChain // Canonical block chain + bc SPBlockchain // Canonical block chain
- 66-69: The
Process
method now acceptsstate.StateDBI
as an argument instead of*state.StateDB
. This is another shift from a concrete implementation to an interface-based approach. Make sure all calls to this function are updated accordingly.- func (p *StateProcessor) Process(block *types.Block, statedb *state.StateDB, cfg vm.Config) (types.Receipts, []*types.Log, uint64, error) { + func (p *StateProcessor) Process(block *types.Block, statedb state.StateDBI, cfg vm.Config) (types.Receipts, []*types.Log, uint64, error) {
- 95-98: The order of arguments in the
applyTransaction
function call has changed. Thevmenv
andmsg
parameters have switched places. Ensure that this change does not affect the functionality of the code.- receipt, err := applyTransaction(msg, p.config, gp, statedb, blockNumber, blockHash, tx, usedGas, vmenv) + receipt, err := applyTransaction(vmenv, msg, p.config, gp, statedb, blockNumber, blockHash, tx, usedGas)
- 113-119: Similar to the previous comment, the order of arguments in the
applyTransaction
function definition has changed. Thevmenv
andmsg
parameters have switched places. This change should be consistent across all calls to this function.- func applyTransaction(msg *Message, config *params.ChainConfig, gp *GasPool, statedb *state.StateDB, blockNumber *big.Int, blockHash common.Hash, tx *types.Transaction, usedGas *uint64, evm *vm.EVM) (*types.Receipt, error) { + func applyTransaction(evm *vm.EVM, msg *Message, config *params.ChainConfig, gp *GasPool, statedb state.StateDBI, blockNumber *big.Int, blockHash common.Hash, tx *types.Transaction, usedGas *uint64) (*types.Receipt, error) {
166-212: A new function
applyTransactionWithResult
has been introduced. It seems to perform similar operations asapplyTransaction
, but it also returns anExecutionResult
. Ensure that this function is used correctly where the execution result is needed.214-255: The public functions
ApplyTransaction
,ApplyTransactionWithResult
,ApplyTransactionWithEVM
, andApplyTransactionWithEVMWithResult
have been updated to use thestate.StateDBI
interface instead of the*state.StateDB
concrete type. Make sure all calls to these functions are updated accordingly.- statedb *state.StateDB + statedb state.StateDBI
- 271-274: The
ProcessBeaconBlockRoot
function now acceptsstate.StateDBI
as an argument instead of*state.StateDB
. This is another shift from a concrete implementation to an interface-based approach. Make sure all calls to this function are updated accordingly.- func ProcessBeaconBlockRoot(beaconRoot common.Hash, vmenv *vm.EVM, statedb *state.StateDB) { + func ProcessBeaconBlockRoot(beaconRoot common.Hash, vmenv *vm.EVM, statedb state.StateDBI) {
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: 8
Files selected for processing (13)
- core/state/interface.go (1 hunks)
- core/state/statedb.go (15 hunks)
- core/state_processor.go (5 hunks)
- core/state_transition.go (1 hunks)
- core/vm/contracts.go (24 hunks)
- core/vm/evm.go (14 hunks)
- core/vm/interface.go (2 hunks)
- core/vm/precompile_manager.go (1 hunks)
- core/vm/runtime/runtime.go (5 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 (8 hunks)
Additional comments (Suppressed): 53
core/state/interface.go (1)
- 29-107: The interface
StateDBI
has been introduced to abstract the state database. This is a good practice as it enhances modularity and makes testing easier. However, there are some points that need attention:
The method
GetPrecompileManager()
returnsany
. It's better to return a specific interface or type for clarity and type safety.The method
Selfdestruct6780(common.Address)
lacks a comment explaining its purpose. It's important to document public methods in an interface.The method
DumpToCollector(c DumpCollector, conf *DumpConfig) (nextKey []byte)
uses a custom typeDumpCollector
which is not defined in this file. Ensure that this type is properly defined and imported.The method
GetOrNewStateObject(addr common.Address) *StateObject
returns a pointer toStateObject
. IfStateObject
is a struct, consider returning an interface instead to keep the abstraction consistent.Please verify these points and make necessary changes.
core/state_transition.go (1)
- 402-405: The
Prepare
method now includes an additional argumentvm.ActivePrecompiles(st.evm, rules)
. Ensure that this change doesn't break the existing functionality and is compatible with the rest of the codebase. Also, verify if the new argument correctly represents the active precompiled contracts for the given EVM and rules.eth/tracers/native/4byte.go (2)
82-85: The
CaptureStart
method now includes an additional call tovm.ActivePrecompiles(env, rules)
with the EVM environment and chain rules as parameters. This change suggests that the list of active precompiled contracts is now dynamically updated based on the current block context. Ensure that this dynamic update does not introduce any unexpected behavior or performance issues.85-85: The assignment of
t.activePrecompiles
has been modified. Previously, it was assigned the result ofvm.ActivePrecompiles(rules)
, but now it's assigned the result ofvm.ActivePrecompiles(env, rules)
. This change implies that the functionvm.ActivePrecompiles()
might have been updated to accept an additional parameter. Verify that all calls to this function throughout the codebase have been updated to match the new signature.core/vm/precompile_manager.go (2)
36-39: The
Get
method currently returnsnil, false
for all cases. This might be a placeholder implementation. Ensure that the actual logic is implemented before merging.41-45: Similar to the
Get
method, theGetActive
method also returnsnil
for all cases. This might be a placeholder implementation. Ensure that the actual logic is implemented before merging.eth/tracers/js/goja.go (1)
- 251-254: The
vm.ActivePrecompiles
function now takes two arguments instead of one. Ensure that this change is reflected across all calls to this function in the codebase. Also, verify that the new argumentenv
is always available and correctly initialized where this function is called.- t.activePrecompiles = vm.ActivePrecompiles(rules) + t.activePrecompiles = vm.ActivePrecompiles(env, rules)core/vm/interface.go (3)
28-31: The
StateDB
interface has been simplified to embed thestate.StateDBI
interface. Ensure that thestate.StateDBI
interface includes all necessary methods previously defined in theStateDB
interface.47-59: A new interface
PrecompileManager
is introduced for managing precompiled contracts. This seems to be a good abstraction, but ensure that all methods are implemented wherever this interface is used.61-70: The
PrecompileEVM
interface is introduced to provide stateful precompiles with a way to call back into the EVM. Make sure that all methods are correctly implemented and used.core/vm/contracts.go (1)
- 19-23: The import of
context
is new. Ensure that it's used appropriately in the codebase.ethapi/api.go (7)
- 732-748: The code for creating the accountProof has been commented out and replaced with a TODO comment. This might affect the functionality of the method if proofs are required elsewhere in the codebase. Please verify this change.
- // tr, err := trie.NewStateTrie(trie.StateTrieID(header.Root), state.Database().TrieDB()) - // if err != nil { - // return nil, err - // } - var accountProof proofList = make(proofList, 0) - // if err := tr.Prove(crypto.Keccak256(address.Bytes()), &accountProof); err != nil { - // return nil, err - // }
- 957-970: The
Apply
function now checks for errors after setting the nonce and code. This is a good practice as it ensures that any issues are caught immediately.+ if err := state.Error(); err != nil { + return err + }
- 974-979: Similar to the previous comment, error checking has been added after setting the balance.
+ if err := state.Error(); err != nil { + return err + }
1072-1075: The type of the
state
parameter in thedoCall
function has been changed from*state.StateDB
tostate.StateDBI
. Ensure all calls to this function have been updated to match the new signature.1093-1096: The way
blockCtx
is created has been changed. Make sure this new approach is compatible with the rest of your codebase.1185-1188: The type of the
state
parameter in theexecuteEstimate
function has been changed from*state.StateDB
tostate.StateDBI
. Ensure all calls to this function have been updated to match the new signature.1318-1320: A 20% increase has been added to the
hi
variable. This seems like a workaround and might cause issues in the future. It would be better to understand why this is needed and try to address the root cause.+ // TODO: FIGURE OUT WHY THIS HACK IS NEEDED THANKS + // bump the estimate gas by 20% for stateful precompiles + hi += uint64(float64(hi) * 0.2)1637:
The way precompiles are retrieved has been changed, and there's a TODO comment suggesting further changes. Make sure these changes don't break anything and that the suggested changes are tracked and implemented.- precompiles := vm.ActivePrecompiles(b.ChainConfig().Rules(header.Number, isPostMerge, header.Time)) + precompiles := vm.ActivePrecompiles(vm.NewEVM(vm.BlockContext{}, vm.TxContext{}, state, b.ChainConfig(), vm.Config{}), rules) // TODO: use evm precompile manager insteadeth/tracers/native/call_flat.go (1)
- 148-151: The
vm.ActivePrecompiles
function now takes two arguments instead of one. Ensure that this change is reflected across all calls to this function in the codebase.- t.activePrecompiles = vm.ActivePrecompiles(rules) + t.activePrecompiles = vm.ActivePrecompiles(env, rules)core/vm/runtime/runtime.go (5)
50-52: The type of
State
in theConfig
struct has been changed from a pointer tostate.StateDB
to an interfacestate.StateDBI
. This change enhances modularity and allows for different implementations of the state database. Ensure that all uses ofState
throughout the codebase are compatible with this new interface.105-108: The return type of the
Execute
function has been updated to match the newstate.StateDBI
interface. Make sure that all calls to this function handle the returned state correctly.123-126: The
vm.ActivePrecompiles
function now takes two arguments:vmenv
andrules
. Verify that this change is consistent with updates to thevm.ActivePrecompiles
function definition elsewhere in the codebase.156-159: Similar to the previous comment, the
vm.ActivePrecompiles
function now takes two arguments:vmenv
andrules
. Confirm that this change aligns with updates to thevm.ActivePrecompiles
function definition.184-187: Again, the
vm.ActivePrecompiles
function now takes two arguments:vmenv
andrules
. Validate that this change is consistent with updates to thevm.ActivePrecompiles
function definition.core/vm/evm.go (14)
19-23: The import of the
errors
package is new. Ensure that it's used appropriately in the code.53-60: The precompile lookup logic has been updated to use a
PrecompileManager
if the initial lookup fails. This change enhances modularity and allows for more flexible management of precompiled contracts.105-112: A new field
PrecompileManager
has been added to theEVM
struct. This change aligns with the introduction of thePrecompileManager
interface, enhancing modularity and abstraction.140-150: The creation of an
EVM
instance now includes setting up aPrecompileManager
. This change ensures that eachEVM
instance has its ownPrecompileManager
, which can be used to manage precompiled contracts.232-238: The call to run a precompiled contract now uses the
PrecompileManager
. This change aligns with the introduction of thePrecompileManager
interface and enhances modularity.254-260: Error handling has been updated to use the
errors.Is
function from the standard library. This is a more reliable way to compare errors than direct comparison.295-301: The call to run a precompiled contract now uses the
PrecompileManager
. This change aligns with the introduction of thePrecompileManager
interface and enhances modularity.307-313: Error handling has been updated to use the
errors.Is
function from the standard library. This is a more reliable way to compare errors than direct comparison.340-346: The call to run a precompiled contract now uses the
PrecompileManager
. This change aligns with the introduction of thePrecompileManager
interface and enhances modularity.351-357: Error handling has been updated to use the
errors.Is
function from the standard library. This is a more reliable way to compare errors than direct comparison.389-395: The call to run a precompiled contract now uses the
PrecompileManager
. This change aligns with the introduction of thePrecompileManager
interface and enhances modularity.407-413: Error handling has been updated to use the
errors.Is
function from the standard library. This is a more reliable way to compare errors than direct comparison.502-508: Error handling has been updated to use the
errors.Is
function from the standard library. This is a more reliable way to compare errors than direct comparison.535-539: New getter methods have been added for
BlockContext
andStateDB
. These changes improve encapsulation by providing controlled access to these fields.core/state_processor.go (5)
35-54: The type of
bc
inStateProcessor
struct has been changed from*BlockChain
toSPBlockchain
. Ensure that all methods required by theSPBlockchain
interface are implemented wherever aStateProcessor
is used. Also, verify that all calls toNewStateProcessor
have been updated to pass an instance of a type that implementsSPBlockchain
instead of*BlockChain
.66-66: The type of
statedb
parameter inProcess
method has been changed from*state.StateDB
tostate.StateDBI
. Make sure that all calls to this method throughout the codebase have been updated to match the new signature.113-113: The type of
statedb
parameter inapplyTransaction
function has been changed from*state.StateDB
tostate.StateDBI
. Ensure that all calls to this function have been updated to match the new signature.167-167: The type of
statedb
parameter inApplyTransaction
function has been changed from*state.StateDB
tostate.StateDBI
. Verify that all calls to this function have been updated to match the new signature.180-180: The type of
statedb
parameter inProcessBeaconBlockRoot
function has been changed from*state.StateDB
tostate.StateDBI
. Ensure that all calls to this function have been updated to match the new signature.core/state/statedb.go (10)
82-85: The type of
stateObjects
has been changed from*stateObject
to*StateObject
. Ensure that theStateObject
struct provides the same or enhanced functionality as thestateObject
struct. Also, verify that all methods and properties ofstateObject
are available inStateObject
.109-109: The type of
accessList
has been changed from*accessList
to*AccessList
. Ensure that theAccessList
struct provides the same or enhanced functionality as theaccessList
struct. Also, verify that all methods and properties ofaccessList
are available inAccessList
.158-166: The initialization of
stateObjects
andaccessList
has been updated to useStateObject
andAccessList
respectively. This change is consistent with the type changes mentioned above.506-507: The type of
obj
inupdateStateObject(obj *StateObject)
has been changed from*stateObject
to*StateObject
. This change is consistent with the type changes mentioned above.538-539: The type of
obj
indeleteStateObject(obj *StateObject)
has been changed from*stateObject
to*StateObject
. This change is consistent with the type changes mentioned above.553-556: The return type of
getStateObject(addr common.Address)
has been changed from*stateObject
to*StateObject
. This change is consistent with the type changes mentioned above.564-567: The return type of
getDeletedStateObject(addr common.Address)
has been changed from*stateObject
to*StateObject
. This change is consistent with the type changes mentioned above.617-625: The return and parameter types of
createObject(addr common.Address)
have been changed from*stateObject
to*StateObject
. This change is consistent with the type changes mentioned above.700-706: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [692-706]
The return type of
Copy()
has been changed from*StateDB
toStateDBI
. Ensure thatStateDBI
provides the same or enhanced functionality as*StateDB
. Also, verify that all methods and properties of*StateDB
are available inStateDBI
.
- 1318-1324: The initialization of
al
has been updated to useNewAccessList()
. This change is consistent with the type changes mentioned above.
CodeRabbit review skipped By default, CodeRabbit only reviews PRs on the default branch. If you wish to have PRs reviewed on additional branches, you can configure that under the repository settings in the UI. |
Summary by CodeRabbit
*state.StateDB
tostate.StateDBI
across multiple files for better abstraction and modularity.ethapi
package to ensure correct dependencies.PrecompileManager
interface and its implementation.AccessList
struct and its methods for improved functionality.stateObject
toStateObject
for consistency and clarity.