Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Asterisc contracts VM tests - Forge Test #22

Merged
merged 22 commits into from
Mar 19, 2024

Conversation

pcw109550
Copy link
Contributor

@pcw109550 pcw109550 commented Feb 20, 2024

Description

Implements forge test for supported RISCV instruction and preimage operations. Basically ethereum-optimism/optimism#6638 but for asterisc.

This PR contains 95 tests, and over 2.5K LOC of code. I tried to break the diff by atomically commit the changes. Please
review each commit step by step.

I am somewhat confident that this new test suite covers almost every nontrivial(ignoring no-ops) state transitions.

References

  1. https://riscv.org/wp-content/uploads/2017/05/riscv-spec-v2.2.pdf. Page 116(starting from the first page) is especially useful. Please think this doc as a source of truth.
  2. https://msyksphinz-self.github.io/riscv-isadoc. This website helped me to understand the riscv operations. Not perfect and may contain invalid infos.
  3. https://github.com/ethereum-optimism/asterisc/blob/master/docs/riscv.md

Step by Step guideline

  1. 260cff2 fixes misimplementation of srliw and sraiw instruction.
  • Also fixed fast mode and slow mode for this. Original tests could not catch this case, because they did not have sign extended case for these instructions.
  • Ref(captured from Ref 1):
image
  1. fa0e247 added upper bound size check for atomic instructions; size may be never larger than 8.
  2. feedff1: update comment to mention RV64A is also supported.
  3. b6215aa: Correct from CSSR to CSRR.
  4. Leftover commits implements tests step by step. There are 6 instruction types: {R, I, S, B, U, J}. I tried to implement tests by grouping them having similar structure; not just grouping them by instruction types.

Tests

Added gas snapshots(by running forge snapshot) by file. Below is the output

RISCV_Test:test_add_succeeds() (gas: 175505)
RISCV_Test:test_addi_succeeds() (gas: 174720)
RISCV_Test:test_addiw_succeeds() (gas: 174987)
RISCV_Test:test_addw_succeeds() (gas: 175884)
RISCV_Test:test_amoaddd_succeeds() (gas: 382706)
RISCV_Test:test_amoaddw_succeeds() (gas: 369214)
RISCV_Test:test_amoandd_succeeds() (gas: 382221)
RISCV_Test:test_amoandw_succeeds() (gas: 368804)
RISCV_Test:test_amomaxd_succeeds() (gas: 364731)
RISCV_Test:test_amomaxud_succeeds() (gas: 364541)
RISCV_Test:test_amomaxuw_succeeds() (gas: 350877)
RISCV_Test:test_amomaxw_succeeds() (gas: 351148)
RISCV_Test:test_amomind_succeeds() (gas: 364548)
RISCV_Test:test_amominud_succeeds() (gas: 364454)
RISCV_Test:test_amominuw_succeeds() (gas: 350518)
RISCV_Test:test_amominw_succeeds() (gas: 351001)
RISCV_Test:test_amoord_succeeds() (gas: 382509)
RISCV_Test:test_amoorw_succeeds() (gas: 368930)
RISCV_Test:test_amoswapd_succeeds() (gas: 364122)
RISCV_Test:test_amoswapw_succeeds() (gas: 350330)
RISCV_Test:test_amoxord_succeeds() (gas: 382480)
RISCV_Test:test_amoxorw_succeeds() (gas: 368834)
RISCV_Test:test_and_succeeds() (gas: 175411)
RISCV_Test:test_andi_succeeds() (gas: 174708)
RISCV_Test:test_auipc_succeeds() (gas: 174390)
RISCV_Test:test_beq_succeeds() (gas: 176047)
RISCV_Test:test_bge_succeeds() (gas: 176534)
RISCV_Test:test_bgeu_succeeds() (gas: 176320)
RISCV_Test:test_blt_succeeds() (gas: 176262)
RISCV_Test:test_bltu_succeeds() (gas: 176180)
RISCV_Test:test_bne_succeeds() (gas: 176231)
RISCV_Test:test_csrrc_succeeds() (gas: 175051)
RISCV_Test:test_csrrci_succeeds() (gas: 174245)
RISCV_Test:test_csrrs_succeeds() (gas: 174929)
RISCV_Test:test_csrrsi_succeeds() (gas: 174174)
RISCV_Test:test_csrrw_succeeds() (gas: 174815)
RISCV_Test:test_csrrwi_succeeds() (gas: 174033)
RISCV_Test:test_div_succeeds() (gas: 175828)
RISCV_Test:test_divu_succeeds() (gas: 175560)
RISCV_Test:test_divuw_succeeds() (gas: 176054)
RISCV_Test:test_divw_succeeds() (gas: 176831)
RISCV_Test:test_ebreak_succeeds() (gas: 172772)
RISCV_Test:test_ecall_succeeds() (gas: 175739)
RISCV_Test:test_jal_succeeds() (gas: 175200)
RISCV_Test:test_jalr_succeeds() (gas: 175297)
RISCV_Test:test_lb_succeeds() (gas: 206810)
RISCV_Test:test_lbu_succeeds() (gas: 206019)
RISCV_Test:test_ld_succeeds() (gas: 217344)
RISCV_Test:test_lh_succeeds() (gas: 208942)
RISCV_Test:test_lhu_succeeds() (gas: 208063)
RISCV_Test:test_lrd_succeeds() (gas: 217802)
RISCV_Test:test_lrw_succeeds() (gas: 212114)
RISCV_Test:test_lui_succeeds() (gas: 173754)
RISCV_Test:test_lw_succeeds() (gas: 211910)
RISCV_Test:test_lwu_succeeds() (gas: 211074)
RISCV_Test:test_mul_succeeds() (gas: 175633)
RISCV_Test:test_mulh_succeeds() (gas: 175947)
RISCV_Test:test_mulhsu_succeeds() (gas: 175811)
RISCV_Test:test_mulhu_succeeds() (gas: 175464)
RISCV_Test:test_mulw_succeeds() (gas: 176107)
RISCV_Test:test_or_succeeds() (gas: 175463)
RISCV_Test:test_ori_succeeds() (gas: 174682)
RISCV_Test:test_preimage_read_succeeds() (gas: 357373)
RISCV_Test:test_preimage_write_succeeds() (gas: 207573)
RISCV_Test:test_rem_succeeds() (gas: 175850)
RISCV_Test:test_remu_succeeds() (gas: 175523)
RISCV_Test:test_remuw_succeeds() (gas: 175998)
RISCV_Test:test_remw_succeeds() (gas: 176811)
RISCV_Test:test_sb_succeeds() (gas: 309299)
RISCV_Test:test_scd_succeeds() (gas: 320930)
RISCV_Test:test_scw_succeeds() (gas: 315067)
RISCV_Test:test_sd_succeeds() (gas: 319248)
RISCV_Test:test_sh_succeeds() (gas: 310754)
RISCV_Test:test_sll_succeeds() (gas: 175474)
RISCV_Test:test_slli_succeeds() (gas: 174740)
RISCV_Test:test_slliw_succeeds() (gas: 174975)
RISCV_Test:test_sllw_succeeds() (gas: 175730)
RISCV_Test:test_slt_succeeds() (gas: 175683)
RISCV_Test:test_slti_succeeds() (gas: 174974)
RISCV_Test:test_sltiu_succeeds() (gas: 174735)
RISCV_Test:test_sltu_succeeds() (gas: 175443)
RISCV_Test:test_sra_succeeds() (gas: 176145)
RISCV_Test:test_srai_succeeds() (gas: 175413)
RISCV_Test:test_sraiw_succeeds() (gas: 175546)
RISCV_Test:test_sraw_succeeds() (gas: 176306)
RISCV_Test:test_srl_succeeds() (gas: 175561)
RISCV_Test:test_srli_succeeds() (gas: 174856)
RISCV_Test:test_srliw_succeeds() (gas: 175047)
RISCV_Test:test_srlw_succeeds() (gas: 175709)
RISCV_Test:test_step_abi_succeeds() (gas: 92452)
RISCV_Test:test_sub_succeeds() (gas: 175553)
RISCV_Test:test_subw_succeeds() (gas: 175894)
RISCV_Test:test_sw_succeeds() (gas: 313600)
RISCV_Test:test_xor_succeeds() (gas: 175384)
RISCV_Test:test_xori_succeeds() (gas: 174717)

Additional context

Current dependencies on/for this PR:

@pcw109550 pcw109550 force-pushed the tip/pcw109550/asterisc-contract-test-2 branch from 5a62887 to 9ac7ca0 Compare February 21, 2024 14:21
@pcw109550 pcw109550 self-assigned this Feb 21, 2024
@pcw109550 pcw109550 changed the base branch from tip/pcw109550/asterisc-contract-test to master March 11, 2024 12:55
@pcw109550 pcw109550 force-pushed the tip/pcw109550/asterisc-contract-test-2 branch from 59b7068 to 66116a4 Compare March 11, 2024 13:27
@pcw109550 pcw109550 marked this pull request as ready for review March 11, 2024 14:51
@pcw109550 pcw109550 requested a review from clabby March 11, 2024 14:52
@Inphi
Copy link
Collaborator

Inphi commented Mar 11, 2024

I'm not too familiar with riscv (not yet!). But some notes upon skimming the tests:

It'll be great to have actual coverage data on the VM. For now, having just a comment or an lcov screenshot in the PR description illustrating this. It'll be great to have codecov setup on asterisc though.

There are a couple reverts in the VM. The tests should cover these as well, however unlikely they are to occur.

Copy link
Collaborator

@ImTei ImTei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may not related to this PR, but can you add SPDX-License-Identifier to the new contract files? and, please follow the code convention of import lines.

import { Test } from "forge-std/Test.sol";

rvgo/fast/vm.go Outdated Show resolved Hide resolved
rvsol/test/RISCV.t.sol Outdated Show resolved Hide resolved
@pcw109550
Copy link
Contributor Author

pcw109550 commented Mar 18, 2024

I'm not too familiar with riscv (not yet!). But some notes upon skimming the tests:

It'll be great to have actual coverage data on the VM. For now, having just a comment or an lcov screenshot in the PR description illustrating this. It'll be great to have codecov setup on asterisc though.

There are a couple reverts in the VM. The tests should cover these as well, however unlikely they are to occur.

@Inphi Thanks for your feedback. Will try to harden VMs by adding tests which covers reverts.

I tried to extract the test coverage data, by running forge coverage command at rvsol dir, but I get below errors.

[⠒] Compiling...
[⠊] Compiling 29 files with 0.8.15
[⠒] Solc 0.8.15 finished in 3.93s
Error: 
Compiler run failed:
Error: Compiler error (/solidity/libyul/backends/evm/AsmCodeGen.cpp:68):Stack too deep. Try compiling with `--via-ir` (cli) or the equivalent `viaIR: true` (standard JSON) while enabling the optimizer. Otherwise, try removing local variables. When compiling inline assembly: Variable ptr is 1 slot(s) too deep inside the stack.

This seems to be nontrivial to fix, so will make this as a separate task. I tried the --via-ir flag as well, but problem still persists.

So, to sum up; additional tasks are:

  1. Add coverage data.
  2. Add tests for VM revert.

@ImTei
Copy link
Collaborator

ImTei commented Mar 18, 2024

This is a great PR. It'll be very helpful for our test coverage. Thank you!
I would like to note that there are such parts we haven't covered yet. There're variable instructions for same operation, like signed/unsigned or double precision. You have written test code to handle those case using if statement in such test cases, but we do not test all of the cases because there is a single input for each case. So we need to list up things that we need to handle for each operation, and make fuzz tests for them. Of course, it's not mandatory in this PR and would be the next task. This PR will be a great bedrock for better tests. 👍

@pcw109550
Copy link
Contributor Author

It may not related to this PR, but can you add SPDX-License-Identifier to the new contract files? and, please follow the code convention of import lines.

import { Test } from "forge-std/Test.sol";

Will update this in a separate PR, along with versioning up solidity to 0.8.15.

@pcw109550 pcw109550 added this pull request to the merge queue Mar 19, 2024
Merged via the queue into master with commit 7daecab Mar 19, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants