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

Add tests to keep server_api json.go in sync with parse_inputs.rs #2723

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

ganeshvanahalli
Copy link
Contributor

@ganeshvanahalli ganeshvanahalli commented Oct 4, 2024

This PR adds CI steps to generate a InputJSON file corresponding to a block generated in a system test - TestProgramStorage and run arbitrator prover binary on this json file.

To test locally do this


$> BLOCK_INPUT_JSON_PATH="some_valid_path/block_input.json" go test github.com/offchainlabs/nitro/system_tests --run TestProgramStorage$ --count=1
ok  	github.com/offchainlabs/nitro/system_tests	3.268s

$> make build-prover-bin
<... snip lots of output ...>

$> target/bin/prover target/machines/latest/machine.wavm.br -b --json-inputs="some_valid_path/block_input.json"
<... snip lots of output ...>

Resolves NIT-2820

@cla-bot cla-bot bot added the s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA. label Oct 4, 2024
tsahee
tsahee previously requested changes Oct 9, 2024
Copy link
Contributor

@tsahee tsahee left a comment

Choose a reason for hiding this comment

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

That's a very interesting approach. Took some good thinking..
However, I still prefer my original idea: make a system test that stores block to a path chosen by environment variable (or doesn't, if variable not set), have a ci step that runs this test to create a file, and then a ci step that launches prover with it and makes sure it passes.
The advantages of this approach is we see not only that format passed, but that we succeed in launching a machine from that info. For example, if we change "wavm" target name to something else.
Also, the test in the different approach could easily also test your other pr running a jit machine.
LMK if you disagree.

@ganeshvanahalli
Copy link
Contributor Author

That's a very interesting approach. Took some good thinking.. However, I still prefer my original idea: make a system test that stores block to a path chosen by environment variable (or doesn't, if variable not set), have a ci step that runs this test to create a file, and then a ci step that launches prover with it and makes sure it passes. The advantages of this approach is we see not only that format passed, but that we succeed in launching a machine from that info. For example, if we change "wavm" target name to something else. Also, the test in the different approach could easily also test your other pr running a jit machine. LMK if you disagree.

Makes sense, I agree with your approach, as it guarantees we cover all cases and not just the parsing.

…fication. will be undone after debug

fix env variable setting impl

fix gotestsum invocation

fix gotestsum invocation

fix gotestsum invocation
@ganeshvanahalli ganeshvanahalli force-pushed the testcompatibility-goinputjson-rustfiledata branch from 3c23148 to f0d14fb Compare October 10, 2024 08:12
- name: create block input json file
if: matrix.test-mode == 'defaults'
run: |
BLOCK_INPUT_JSON_PATH="${{ github.workspace }}/target/block_input.json" gotestsum --format short-verbose -- -run TestProgramStorage$ ./system_tests/... --count 1
Copy link
Member

Choose a reason for hiding this comment

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

This environment variable should not be needed. The location of the output file is extremely customizable already:
https:/OffchainLabs/nitro/blob/master/validator/inputs/writer_test.go

And, by default it is in a pretty predictable location.
So, in this CI step, it will be written to:
/home/user/.arbitrum/validation-inputs/TestProgramStorage/*/block_input_5.json
The * just matches a timestamped directory. And, you can read it just like that.

If you want to eliminate that directory to make it even more predictable, you could add a flag (preferred) or an environment variable like --validator.inputswriter.usetimestamp=false and then, check that flag when constructing the inputs.NewWriter and add the inputs.WithTimestampDirEnabled(false) option.

If you're worried about the brittleness of having the block ID in the name of the json file, you could add another option to suprress including the block ID in the name of the file.

Also, if you want the file to go into a workspace, specific directory, we could introduce a flag like --validator.inputswriter.basedir=${{ github.workspace }}/target/ and use it with the inputs.WithBaseDir option when constructing the inputs.NewWriter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh cool.
Yep, I was hesitant on having blockID in the fileName as that could change in the future (as it is not deterministic), introducing a new option sounds like a good idea to me!

Other than that, we wanted to have a way to control generation of this file and having a flag itself to control this SGTM.
Thanks for this, inputswriter is quite customizable!

Copy link
Contributor

@tsahee tsahee left a comment

Choose a reason for hiding this comment

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

seems good, but waiting for @eljobe to approve

system_tests/program_test.go Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design-approved s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants