ethereumjs-monorepo
ethereumjs-monorepo copied to clipboard
ethereumjs
Possible VM bug in EIP 3860
Upon refactoring I stumbled across this:
https://github.com/ethereumjs/ethereumjs-monorepo/blob/2f42dcfedd18852fc95cc4a51da606dfcbf87387/packages/vm/src/evm/eei.ts#L634
The contract nonce is updated, and then the maxInitCodeSize check is done. Before that there are checks on the nonce, call depth, and balance checks. It seems to me that if those checks are violated then no further gas is consumed (so not the provided gasLimit is consumed) and the frame is directly returned. However upon the maxInitCodeSize check it seems that the nonce is updated, so this state change dangles (and seems wrong to me). I think there are tests for this EIP somewhere in an ethereum/tests PR, I do recall testing it, but this situation was not caught (it should catch this as a state root error though, since the nonce is 1 too high if the maxInitCodeSize is violated..?)
is the state reverted if it throws there? in that case the nonce wouldn't be updated in the trie.
it does make sense to me that the check can be moved up a few lines before the nonce update. (edit: andrei clarified below in case of failure nonce should still be updated)
looking further though, does it catch here first? is there a reason why we would have this check in two places
https://github.com/ethereumjs/ethereumjs-monorepo/blob/2f42dcfedd18852fc95cc4a51da606dfcbf87387/packages/vm/src/evm/evm.ts#L319-L331
@gumb0 @jochem-brouwer is there anything we need to clarify in terms of error cases in the EIP?
I think I just gotta see and test it, but if you CREATE/CREATE2 it first calls into EEI's create opcode which later (below the check of maxInitCodeSize) calls back into EVMs executeMessage, which then calls into _executeCreate. In that case it throws, it seems like different behavior in CREATE/CREATE2 than in txn and I do not think this is intended by the EIP.
@axic I think the EIP is clear enough here, but just to check, in case of the CREATE/CREATE2 opcode, if we try to deploy a contract with initcode > maxInitCodeSize, then 0 is put on the stack and the upfront cost of the CREATE (so the base fee, memory expansion, possible hashing cost) is spent in gas, and not the entire gasLimit? Our implementation (probably) increases the nonce of the contract which invokes CREATE/CREATE2 by 1 even if the maxInitCodeSize is violated, which I assume should not happen?
@axic nvm, just saw my comment on the ethereum/tests PR, I don't think the EIP is up-to-date yet? https://github.com/ethereum/tests/pull/990#issuecomment-1033630958 CC @gumb0
@axic nvm, just saw my comment on the
ethereum/testsPR, I don't think the EIP is up-to-date yet? ethereum/tests#990 (comment) CC @gumb0
Yes, we have not updated it yet, sorry.
@axic I think the EIP is clear enough here, but just to check, in case of the CREATE/CREATE2 opcode, if we try to deploy a contract with initcode > maxInitCodeSize, then 0 is put on the stack and the upfront cost of the CREATE (so the base fee, memory expansion, possible hashing cost) is spent in gas, and not the entire gasLimit? Our implementation (probably) increases the nonce of the contract which invokes CREATE/CREATE2 by 1 even if the maxInitCodeSize is violated, which I assume should not happen?
When initcode exceeds limit, gas for initcode execution is not deducted (so only upfront charges are: base cost, memory expansion, hashing, initcode charge, too).
Nonce is updated even in case of failure, which is similar to other creation errors like contract already existing or error during initcode execution.
The tests are not yet updated so we can't check against the existing implementation, so I'd say that this issue is not yet closed until the tests are finalized (the ethereum/tests tests of this EIP)
The tests are not yet updated so we can't check against the existing implementation, so I'd say that this issue is not yet closed until the tests are finalized (the
ethereum/teststests of this EIP)
I think tests had correct behavior all along (no update expected), and EIP wording was updated recently https://github.com/ethereum/EIPs/pull/5130
Can't completely judge this issue, can this be closed?
//cc @jochem-brouwer