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

  1. 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;
    }
 
}
  1. The bug is likely related to the NUMBER opcode. 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 change block.number to block.timestamp or block.chainid
// Simple contract
pragma solidity >=0.8.2 <0.9.0;

contract BlockHashGen {

    uint256 private number;

    constructor() {
        generateFromBlockHash();
    }

    function generateFromBlockHash() private   {
        number = block.number;
    }
}
  1. 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-temp dune 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 in TEMP_FILE_PATH/sc-rollup-node1/kernel.log)

  2. When reading through the related source code, It seems that we have an off-by-one error in block number. The block number in BlockConstant should 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 the blockConstant (seemingly more proper fix) does not work.

  3. 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 loop in handler.rs). The executions are exactly the same up to an SSTORE opcode when the seemingly proper fix returns OutOfGas.

  4. 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.

  5. 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.rst for the protocol and the environment, CHANGES.rst at the root of the repository for everything else).
  • Select suitable reviewers using the Reviewers field below.
  • Select as Assignee the next person who should take action on that MR
Edited by Hantang Sun

Merge request reports

Loading