all: implement eip-7702 set code tx #30078
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lightclient
force-pushed
the
eip-7702
branch
2 times, most recently
from
bc22287
to
95524bb
Compare
|
Should we propose another EIP to revamp EIP158? Otherwise, as we discussed previously, the leftover storage of an "empty" EOA could be cleared at the end of block. |
|
@rjl493456442 I think the proposal which will get accepted for devnet-2 and on will avoid the 158 problem, so it's probably okay to just let it play out. ethereum/EIPs#8677 |
lightclient
force-pushed
the
eip-7702
branch
5 times, most recently
from
ff4dba2
to
955384a
Compare
1 task
lightclient
force-pushed
the
eip-7702
branch
2 times, most recently
from
e3834ac
to
20ea148
Compare
core/vm/operations_acl.go
Outdated
| evm.StateDB.AddAddressToAccessList(addr) | ||
| // Check if code is a delegation and if so, charge for resolution | ||
| cost := params.ColdAccountAccessCostEIP2929 - params.WarmStorageReadCostEIP2929 | ||
| if addr, ok := types.ParseDelegation(evm.StateDB.GetCode(addr)); ok { |
There was a problem hiding this comment.
shouldn't delegation resolution costs (warm or cold) be paid each time? irrespective of whether addr is in access list or not.
lightclient
force-pushed
the
eip-7702
branch
4 times, most recently
from
94389ba
to
8578fb7
Compare
buddh0
reviewed
Sep 18, 2024
buddh0
reviewed
Sep 18, 2024
buddh0
reviewed
Sep 19, 2024
core/vm/operations_acl.go
Outdated
|
|
||
| func gasExtCodeCopyEIP7702(evm *EVM, contract *Contract, stack *Stack, mem *Memory, memorySize uint64) (uint64, error) { | ||
| // memory expansion first (dynamic part of pre-2929 implementation) | ||
| gas, err := gasExtCodeCopy(evm, contract, stack, mem, memorySize) |
There was a problem hiding this comment.
reuse func gasExtCodeCopyEIP2929?
// memory expansion first (dynamic part of pre-2929 implementation)
gas, err := gasExtCodeCopy(evm, contract, stack, mem, memorySize)
if err != nil {
return 0, err
}
addr := common.Address(stack.peek().Bytes20())
// Check slot presence in the access list
if !evm.StateDB.AddressInAccessList(addr) {
evm.StateDB.AddAddressToAccessList(addr)
var overflow bool
// We charge (cold-warm), since 'warm' is already charged as constantGas
if gas, overflow = math.SafeAdd(gas, params.ColdAccountAccessCostEIP2929-params.WarmStorageReadCostEIP2929); overflow {
return 0, ErrGasUintOverflow
}
}
-->
// memory expansion first (dynamic part of pre-2929 implementation)
gas, err := gasExtCodeCopyEIP2929(evm, contract, stack, mem, memorySize)
if err != nil {
return 0, err
}
buddh0
reviewed
Sep 19, 2024
core/vm/operations_acl.go
Outdated
| } | ||
|
|
||
| func gasEip7702CodeCheck(evm *EVM, contract *Contract, stack *Stack, mem *Memory, memorySize uint64) (uint64, error) { | ||
| addr := common.Address(stack.peek().Bytes20()) |
There was a problem hiding this comment.
reuse gasEip2929AccountCheck?
addr := common.Address(stack.peek().Bytes20())
cost := uint64(0)
// fmt.Println("checking", addr, evm.StateDB.AddressInAccessList(addr))
// Check slot presence in the access list
if !evm.StateDB.AddressInAccessList(addr) {
// If the caller cannot afford the cost, this change will be rolled back
evm.StateDB.AddAddressToAccessList(addr)
// The warm storage read cost is already charged as constantGas
cost = params.ColdAccountAccessCostEIP2929 - params.WarmStorageReadCostEIP2929
}
--->
cost, _ :=gasEip2929AccountCheck(evm, contract, stack, mem, memorySize)
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
hello, I run pectra-devnet-3@v1.5.0 agaist this branch, still some failed test cases, 3 cases related: |
holiman
reviewed
Oct 16, 2024
zhiqiangxu
reviewed
Oct 18, 2024
zhiqiangxu
reviewed
Oct 18, 2024
| // Set code 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 { |
There was a problem hiding this comment.
chain id 0 is not allowed here, which is not consistent with here.
Voidkai
reviewed
Oct 18, 2024
There was a problem hiding this comment.
I think we need to add Resolve* to the new statedb_hooked, otherwise the compile fails.
We also need to change in core/vm/eips.go:L340 from GetCode to ResolveCode for ExtCodeCopy in verkle I think.
One question I had is whether eth_getCode and eth_getProof should resolve the delegation or not, I presume they should not (as it is right now) but I would like to confirm.
| return fmt.Errorf("%w: address %v, codehash: %s", ErrSenderNoEOA, | ||
| msg.From.Hex(), codeHash) | ||
| code := st.state.GetCode(msg.From) | ||
| if len(code) > 0 && !bytes.HasPrefix(code, types.DelegationPrefix) { |
There was a problem hiding this comment.
This checks only for the delegation prefix, while most other code uses types.ParseDelegation which checks the size as well. Maybe we should add types.IsDelegation and use that here and within parsedelegation to keep the checks consistent (not an issue right now as long as we only have one length delegations, but might be a problem on chains with predeployed contracts)
|
|
||
| // ResolveCodeHash retrieves the code at addr, resolving any delegation | ||
| // designations that may exist. | ||
| func (s *StateDB) ResolveCodeHash(addr common.Address) common.Hash { |
There was a problem hiding this comment.
wdyt about making GetCodeHash an alias of ResolveCodeHash, seems like we almost never use GetCodeHash anymore, maybe we don't need the two behaviors. (we do need them for getCode/resolveCode, but maybe not for the codehash)
| } | ||
| slot.SetUint64(uint64(interpreter.evm.StateDB.GetCodeSize(slot.Bytes20()))) | ||
| slot.SetUint64(uint64(len(interpreter.evm.StateDB.ResolveCode(slot.Bytes20())))) |
There was a problem hiding this comment.
I don't love this change, but we need to do it for EOF as well. It means that extcodesize gets a lot heavier now
|
We really need to look into worst cases for DOS with this |
lightclient
force-pushed
the
eip-7702
branch
5 times, most recently
from
f3bee21
to
f5cc7bd
Compare
I think they should not resolve the code. |
Co-authored-by: lightclient <lightclient@protonmail.com> Co-authored-by: Mario Vega <marioevz@gmail.com>
9 tasks
karalabe
pushed a commit
that referenced
this pull request
Oct 31, 2024
I think the core code should generally be agnostic about the witness and the statedb layer should determine what elements need to be included in the witness. Because code is accessed via `GetCode`, and `GetCodeLength`, the statedb will always know when it needs to add that code into the witness. The edge case is block hashes, so we continue to add them manually in the implementation of `BLOCKHASH`. It probably makes sense to refactor statedb so we have a wrapped implementation that accumulates the witness, but this is a simpler change that makes #30078 less aggressive.
GrapeBaBa
pushed a commit
to optimism-java/shisui
that referenced
this pull request
Nov 2, 2024
I think the core code should generally be agnostic about the witness and the statedb layer should determine what elements need to be included in the witness. Because code is accessed via `GetCode`, and `GetCodeLength`, the statedb will always know when it needs to add that code into the witness. The edge case is block hashes, so we continue to add them manually in the implementation of `BLOCKHASH`. It probably makes sense to refactor statedb so we have a wrapped implementation that accumulates the witness, but this is a simpler change that makes ethereum#30078 less aggressive.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Spec: EIP-7702: Set EOA account code