all: implement eip-7702 set code tx #30078
Open
+1,303
−79
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 [email protected] agaist this branch, still some failed test cases, 3 cases related: |
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 <[email protected]> Co-authored-by: Mario Vega <[email protected]>
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.
Comment on lines
+4237
to
+4240
| // Master: BenchmarkReorg-8 10000 899591 ns/op 820154 B/op 1440 allocs/op 1549443072 bytes of heap used | ||
| // WithoutOldChain: BenchmarkReorg-8 10000 1147281 ns/op 943163 B/op 1564 allocs/op 1163870208 bytes of heap used | ||
| // WithoutNewChain: BenchmarkReorg-8 10000 1018922 ns/op 943580 B/op 1564 allocs/op 1171890176 bytes of heap used | ||
|
|
There was a problem hiding this comment.
This seems to be some artifact that could be removed
| case auth.R.BitLen() > 256: | ||
| return fmt.Errorf("%w: address %v, authorization %d", ErrAuthSignatureVeryHigh, msg.From.Hex(), i) | ||
| case auth.S.BitLen() > 256: | ||
| return fmt.Errorf("%w: address %v, authorization %d", ErrAuthSignatureVeryHigh, msg.From.Hex(), i) |
There was a problem hiding this comment.
I'm wondering whether we need to do a signature malleability check here? We're doing one in authorization.Authority(). So if a signature is malleable, the pre-check would pass, so the transaction would be valid, but the authorization itself would be skipped afaict
| @@ -436,20 +450,80 @@ func (st *StateTransition) TransitionDb() (*ExecutionResult, error) { | |||
| return nil, fmt.Errorf("%w: code size %v limit %v", ErrMaxInitCodeSizeExceeded, len(msg.Data), params.MaxInitCodeSize) | |||
| } | |||
|
|
|||
| // Verify authorization list is not empty. | |||
There was a problem hiding this comment.
Suggested change
| // Verify authorization list is not empty. | |
| // Verify a non-nil authorization list is not empty. |
There was a problem hiding this comment.
Okay maybe a bad wording, but I think you get what I mean. This message should convey that the authorization list could either be non-existent or have at least one entry
| continue | ||
| } | ||
| // If the account already exists in state, refund the new account cost | ||
| // charged in the initrinsic calculation. |
There was a problem hiding this comment.
Suggested change
| // charged in the initrinsic calculation. | |
| // charged in the intrinsic calculation. |
|
|
||
| // WithSignature updates the signature of an Authorization to be equal the | ||
| // decoded signature provided in sig. | ||
| func (a *Authorization) WithSignature(sig []byte) *Authorization { |
There was a problem hiding this comment.
Looks like this doesn't need to be exported as its not used anywhere but internally in the types package. Do you feel like this an important api to have?
|
|
||
| func TestParseDelegation(t *testing.T) { | ||
| addr := common.Address{0x42} | ||
| d := append(DelegationPrefix, addr.Bytes()...) |
There was a problem hiding this comment.
Would be nice to get some more cases here, where parsedelegation fails, too short, too long, wrong prefix
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