core/vm: implement EIP-3860: Limit and meter initcode #23847
Conversation
gumb0
force-pushed
the
eip-3860
branch
2 times, most recently
from
d96f2af to
91542cc
Compare
core/vm/gas_table.go
Outdated
| if wordGas, overflow = math.SafeMul(toWordSize(wordGas), params.Sha3WordGas); overflow { | ||
| lenWords := toWordSize(len) | ||
|
|
||
| hashingWordGas, overflow := math.SafeMul(lenWords, params.Sha3WordGas) |
There was a problem hiding this comment.
If this is moved to be the last step, then the size-related check can be deduplicated between gasCreate and gasCreate2. I think we also mentioned this in the EIP.
Not sure if this is a concern in geth, but thought to mention it.
There was a problem hiding this comment.
It would be a bit tricky without having to repeat len, overflow := stack.Back(2).Uint64WithOverflow() and lenWords := toWordSize(len) calls, so I decided that maybe it's simpler to leave duplication.
core/state_transition.go
Outdated
| eip3860 := false | ||
| for _, eip := range st.evm.Config.ExtraEips { | ||
| if eip == 3860 { | ||
| eip3860 = true | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think this iteration should happen in here. IIRC we already do this somewhere else?
There was a problem hiding this comment.
Yes in a private function in vm currently
There was a problem hiding this comment.
Made HasEip3860() a public method of evm.Config, this iteration is not repeated now.
gumb0
force-pushed
the
eip-3860
branch
4 times, most recently
from
cb25f4d to
7c6961f
Compare
core/tx_pool.go
Outdated
| @@ -636,7 +636,7 @@ func (pool *TxPool) validateTx(tx *types.Transaction, local bool) error { | |||
| return ErrInsufficientFunds | |||
| } | |||
| // Ensure the transaction has more gas than the basic tx fee. | |||
| intrGas, err := IntrinsicGas(tx.Data(), tx.AccessList(), tx.To() == nil, true, pool.istanbul) | |||
| intrGas, err := IntrinsicGas(tx.Data(), tx.AccessList(), tx.To() == nil, true, pool.istanbul, true) | |||
There was a problem hiding this comment.
Here TxPool is being conservative and always expects creation transactions to have enough gas to cover initcode charge.
Not sure yet how would we check here for extra EIP being activated and whether it's worth it.
There was a problem hiding this comment.
Maybe it's better to change it to always assume EIP-3860 is not activated, then nothing changes for the tx pool right now, but with EIP activated it will not drop some txs that don't have enough gas to pay for initcode.
Which would be easier to solve when EIP is accepted to the fork, and not is an "extra EIP".
| intrGas, err := IntrinsicGas(tx.Data(), tx.AccessList(), tx.To() == nil, true, pool.istanbul, true) | |
| intrGas, err := IntrinsicGas(tx.Data(), tx.AccessList(), tx.To() == nil, true, pool.istanbul, false) |
core/vm/eips.go
Outdated
| @@ -174,3 +175,13 @@ func opBaseFee(pc *uint64, interpreter *EVMInterpreter, scope *ScopeContext) ([] | |||
| scope.Stack.push(baseFee) | |||
| return nil, nil | |||
| } | |||
|
|
|||
| func enable3860(jt *JumpTable) { | |||
| createOp := *jt[CREATE] | |||
There was a problem hiding this comment.
I clone operation struct here, because doing just jt[CREATE].dynamicCost = ... would modify existing operation in underlying global londonInstructionSet, and that would influence unit tests that switch between enabling / disabling EIP3860.
Not sure if this is also a problem for other enable... functions above (enable2200, enable3529)
There was a problem hiding this comment.
Shouldn't be an issue -- NewXXInstructionSet uses a brand new instructionset.
There was a problem hiding this comment.
But newLondonInstructionSet() is called only once when global is initialized
go-ethereum/core/vm/jump_table.go
Line 60 in 17f1c2d
NewEVMInterpreter then creates a new JumpTable instance, but it contains the pointers to the same opertations of londonInstructionSet global.
go-ethereum/core/vm/interpreter.go
Line 78 in 17f1c2d
There was a problem hiding this comment.
Better fix for this is in #23977
There was a problem hiding this comment.
Does this code needs any changes after #23977?
There was a problem hiding this comment.
Better fix for this is in #23977
Actually this hack is still needed, because JumpTable contains pointers to operations, so modification like jt[CREATE].dynamicGas = gasCreateEip3860 would change the operation for londonInstructionSet, too.
There was a problem hiding this comment.
This is now not needed, I think, we changed it so ops are deep-copied.
There was a problem hiding this comment.
In the latest commit I removed it anyway, because the new initcode charge is applied inside create implementation and not in dynamicGas calculation, so new cost functions are not needed.
gumb0
force-pushed
the
eip-3860
branch
2 times, most recently
from
aab6c14 to
d986fb2
Compare
|
@holiman Please take a look. Suggestions how to improve this are appreciated. |
gumb0
force-pushed
the
eip-3860
branch
4 times, most recently
from
a11480f to
3fa0c16
Compare
Implementation of https://eips.ethereum.org/EIPS/eip-3860
Tests are in ethereum/tests#990 and ethereum/tests#1012