EVM: Fix simulation to use current blockConstant
Context
This MR aims to fix the bug that causes the deployment of the following contract to fail.
pragma solidity >=0.8.2 <0.9.0;
contract BlockHashGen {
uint256 private number;
constructor() {
generateFromBlockHash();
}
function generateFromBlockHash() private {
bytes32 blockH = blockhash(block.number - 1);
number = uint256(keccak256(abi.encodePacked(blockH, block.timestamp))) % 11;
}
}
@vch9 has created a Tezt test for the scenario and I have cherry-picked the commit to this MR.
TLDR
The MR fixes the bug that in simulation.rs we used BlockConstants::first_block to create the blockConstant for execution. The correct approach is to use read the current blockConstant from the host. This would not matter in most transactions.
The BlockHashGen contract deployment triggers the bug because:
block.number is 0 in simulation(since we used BlockConstants::first_block)
=> block.number - 1 cause simulation to fail early on and returns a very low gas limit
=> transaction is sent to etherlink with insufficient gas limit
=> transaction fail with OutOfGas
Investigation Steps
- The bug still exists outside a constructor(which contradicts the info on the scoru-wasm-evm channel). Calling the public function in the following function fails in ghostnet.
pragma solidity >=0.8.2 <0.9.0;
contract BlockHashGen {
uint256 private number;
constructor() {
}
function generateFromBlockHash() public {
bytes32 blockH = blockhash(block.number - 1);
number = uint256(keccak256(abi.encodePacked(blockH, block.timestamp))) % 11;
}
}
- The bug is likely related to the
NUMBERopcode. When I tried to track down the bug, I tried the following contract and its deployment failed on ghostnet(Further dubugging is done mainly using this contract). The same contract works if we changeblock.numbertoblock.timestamporblock.chainid
// Simple contract
pragma solidity >=0.8.2 <0.9.0;
contract BlockHashGen {
uint256 private number;
constructor() {
generateFromBlockHash();
}
function generateFromBlockHash() private {
number = block.number;
}
}
-
I then go to the tezt test for further investigation. With logging added to the relevant function, we can see that the block number(3 in the tezt tes) can be successfully read from
BlockConstant, which seems to suggest that the opcode is working. (To view the debug log, build the kernel with debug feature turned on.EVM_KERNEL_FEATURES=debug make -f kernels.mk evm_installer.wasm. Run the test with --keep-tempdune exec tezt/tests/main.exe -- --file evm_rollup.ml --title 'Alpha: Random generation based on block hash and timestamp' --keep-temp. And kernel log can be found inTEMP_FILE_PATH/sc-rollup-node1/kernel.log) -
When reading through the related source code, It seems that we have an off-by-one error in block number. The block number in
BlockConstantshould be the number of the new block so we should add 1 to its value. I tried a simple hack to always add one when reading the block number from the handler, and then the tezt scenario magically works. It is really strange that have a value differing by 1 can cause the contract deployment to fail(instead of setting the field to a wrong value). Also, adding 1 to block number when constructing theblockConstant(seemingly more proper fix) does not work. -
With the simple contract, I printed the execution information (opcode + Stack) for the quick fix and the seemingly proper fix (by logging runtime.machine().inspect() inside
loopinhandler.rs). The executions are exactly the same up to an SSTORE opcode when the seemingly proper fix returnsOutOfGas. -
To investigate further I dumped the value of the gasometers in the logs. They are exactly the same except for the gaslimit: 69692(seemingly proper fix) vs 89592(simple fix). And 89592 - 69692 = 19900 = 20000(SSTORE writing different value) - 100(SSTORE writing same value). It seems that the client has an incorrect estimation of gas used.
-
When looking into the source code for simulation, I found the bug that simulation wasn't using the current blockConstant stored in host, and was creating a new blockConstant for genesis block. That explains the 19900 difference: simulation suggests that SSTORE costs 100 gas(with blockConstant of genesis block, block.number = 0), and it actually costs 20000
I fixed the actual bug and the integration test works
Note: The off by 1 error is still relevant and requires further investigation, but it would belong to a different MR.
Manually testing the MR
Checklist
-
Document the interface of any function added or modified (see the coding guidelines) -
Document any change to the user interface, including configuration parameters (see node configuration) -
Provide automatic testing (see the testing guide). -
For new features and bug fixes, add an item in the appropriate changelog ( docs/protocols/alpha.rstfor the protocol and the environment,CHANGES.rstat the root of the repository for everything else). -
Select suitable reviewers using the Reviewersfield below. -
Select as Assigneethe next person who should take action on that MR