EVM: writeContract fixes, never return contract code as RETURNDATA
#901
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
zah
reviewed
Dec 7, 2021
nimbus/vm/computation.nim
Outdated
| proc writeContract*(c: Computation) = | ||
| template withExtra(tracer: untyped, args: varargs[untyped]) = | ||
| tracer args, newContract=($c.msg.contractAddress), | ||
| blockNumber=c.vmState.blockNumber, blockHash=($c.vmState.blockHash) |
There was a problem hiding this comment.
Looks like you are trying to re-invent the logScope feature of Chronicles here:
https://github.com/status-im/nim-chronicles#logscope
There was a problem hiding this comment.
How come this is not covered by any test?
Is it possible to add it to the test suite by capturing it a premix test?
I guess I'd need a Goerli archive node to capture in premix? On top, I don't know how to make a premix test, but that's a good idea. Probably several tests covering different issues here.
There is a lot to learn (and a I think lot of time) just to make one of those, though - and passing consensus on Goerli is an effective test for the practical issue. I think we have to choose where to spend this time.
That said, @jangko offered to make such a test in this case (a while ago) - and that would be better as he's familiar with it, and perhaps has the archive node set up. It would be even better if they were submitted to fixtures/Hive though - all these kinds of tests should go there really, as they apply to all clients.
I've generally found our premix-generated tests rather opaque: They must be passed, but don't explain what they are testing and are hard to change if you didn't write them.
jangko
approved these changes
Dec 11, 2021
This fixes #867 "EIP-170 related consensus error at Goerli block 5080941", and equivalent on other networks. This combines a change on the EVM-caller side with an EVM-side change from @jangko 6548ff9 "fixes CREATE/CREATE2's `returndata` bug", making the caller EVM ignore any data except from `REVERT`. Either change works by itself. The reason for both is to ensure we definitely comply with ambiguous EVMC expectations from either side of that boundary, and it makes the internal API clearer. As well as fixing a specific consensus issue, there are some other EVM logic changes too: Refactored `writeContract`, how `RETURNDATA` is handled inside the EVM, and changed behaviour with quirks before EIP-2 (Homestead). The fix allows sync to pass block 5080941 on Goerli, and probably equivalent on other networks. Here's a trace at batch 5080897..5081088: ``` TRC 2021-10-01 21:18:12.883+01:00 Persisting blocks file=persist_blocks.nim:43 fromBlock=5080897 toBlock=5081088 ... DBG 2021-10-01 21:18:13.270+01:00 Contract code size exceeds EIP170 topics="vm computation" file=computation.nim:236 limit=24577 actual=31411 DBG 2021-10-01 21:18:13.271+01:00 gasUsed neq cumulativeGasUsed file=process_block.nim:68 block=5080941/0A3537BC5BDFC637349E1C77D9648F2F65E2BF973ABF7956618F854B769DF626 gasUsed=3129669 cumulativeGasUsed=3132615 TRC 2021-10-01 21:18:13.271+01:00 peer disconnected file=blockchain_sync.nim:407 peer=<IP:PORT> ``` Although it says "Contract code size" and "gasUsed", this bug is more general than either contract size or gas. It's due to incorrect behaviour of EVM instructions `RETURNDATA` and `RETURNDATASIZE`. Sometimes when `writeContract` decides to reject writing the contract for any of several reasons (for example just insufficient gas), the unwritten contract code was being used as the "return data", and given to the caller. If the caller used `RETURNDATA` or `RETURNDATASIZE` ops, those incorrectly reported the contract code that didn't get written. EIP-211 (https://eips.ethereum.org/EIPS/eip-211) describes `RETURNDATA`: > "`CREATE` and `CREATE2` are considered to return the empty buffer in the > success case and the failure data in the failure case". The language is ambiguous. In fact "failure case" means when the contract uses `REVERT` to finish. It doesn't mean other failures like out of gas, EIP-170 limit, EIP-3541, etc. To be thorough, and to ensure we always do the right thing with real EVMC when that's finalised, this patch fixes the `RETURNDATA` issue in two places, either of which make Goerli block 5080941 pass. `writeContract` has been refactored to be caller, and so has where it's called. It sets an error in the usual way if contract writing is rejected -- that's anticipating EVMC, where we'll use different error codes later. Overall four behaviour changes: 1. On the callee side, it doesn't set `c.outputData` except for `REVERT`. 2. On the caller side, it doesn't read `child.outputData` except for `REVERT`. 3. There was a bug in processing before Homestead fork (EIP-2). We did not match the spec or other implementations; now we do. When there's insufficient gas, before Homestead it's treated as success but with an empty contract. https://github.com/ethereum/pyethereum/blob/d117c8f3fd93359fc641fd850fa799436f7c43b5/ethereum/processblock.py#L304 https://github.com/ethereum/go-ethereum/blob/401354976bb4/core/vm/instructions.go#L586 4. The Byzantium check has been removed, as it's unnecessary. Signed-off-by: Jamie Lokier <[email protected]>
jangko
force-pushed
the
jl/evm-writedata-fix
branch
from
4217396
to
4b89ca3
Compare
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.
This fixes #867 "EIP-170 related consensus error at Goerli block 5080941", and equivalent on other networks.
This combines a change on the EVM-caller side with an EVM-side change from @jangko 6548ff9 "fixes CREATE/CREATE2's
returndatabug", making the caller EVM ignore any data except fromREVERT.Either change works by itself. The reason for both is to ensure we definitely comply with ambiguous EVMC expectations from either side of that boundary, and it makes the internal API clearer.
As well as fixing a specific consensus issue, there are some other EVM logic changes too: Refactored
writeContract, howRETURNDATAis handled inside the EVM, and changed behaviour with quirks before EIP-2 (Homestead).The fix allows sync to pass block 5080941 on Goerli, and probably equivalent on other networks. Here's a trace at batch 5080897..5081088:
Although it says "Contract code size" and "gasUsed", this bug is more general than either contract size or gas. It's due to incorrect behaviour of EVM instructions
RETURNDATAandRETURNDATASIZE.Sometimes when
writeContractdecides to reject writing the contract for any of several reasons (for example just insufficient gas), the unwritten contract code was being used as the "return data", and given to the caller. If the caller usedRETURNDATAorRETURNDATASIZEops, those incorrectly reported the contract code that didn't get written.EIP-211 (https://eips.ethereum.org/EIPS/eip-211) describes
RETURNDATA:The language is ambiguous. In fact "failure case" means when the contract uses
REVERTto finish. It doesn't mean other failures like out of gas, EIP-170 limit, EIP-3541, etc.To be thorough, and to ensure we always do the right thing with real EVMC when that's finalised, this patch fixes the
RETURNDATAissue in two places, either of which make Goerli block 5080941 pass.writeContracthas been refactored to be caller, and so has where it's called. It sets an error in the usual way if contract writing is rejected -- that's anticipating EVMC, where we'll use different error codes later.Overall four behaviour changes:
On the callee side, it doesn't set
c.outputDataexcept forREVERT.On the caller side, it doesn't read
child.outputDataexcept forREVERT.There was a bug in processing before Homestead fork (EIP-2). We did not match the spec or other implementations; now we do. When there's insufficient gas, before Homestead it's treated as success but with an empty contract.
https://github.com/ethereum/pyethereum/blob/d117c8f3fd93359fc641fd850fa799436f7c43b5/ethereum/processblock.py#L304
https://github.com/ethereum/go-ethereum/blob/401354976bb4/core/vm/instructions.go#L586
The Byzantium check has been removed, as it's unnecessary.