Skip to content

feat(ctb): OutputBisectionGame#8351

Merged
clabby merged 12 commits intodevelopfrom
cl/ctb/output-bisection-contract
Dec 4, 2023
Merged

feat(ctb): OutputBisectionGame#8351
clabby merged 12 commits intodevelopfrom
cl/ctb/output-bisection-contract

Conversation

@clabby
Copy link
Contributor

@clabby clabby commented Nov 29, 2023

Overview

Implements the first iteration of the OutputBisectionGame, specified in Notion.

Notes

  • Couple of follow-up nits from @tynes addressed.
  • Change local key context value.
  • Refactor unit tests that were disabled / add unit tests for changed functionality.
    • Adding local inputs has changed a decent bit; Need new coverage for this.
    • Claim verification on the split level - verification of the VM status byte has moved from when the contract is created to each time an exec trace subgame begins.
  • Handle attack on genesis in the output root bisection phase.
  • Protect against misconfigured depths (odd/even - who makes the step? who makes the exec trace root claims?).

Metadata
closes https://github.com/ethereum-optimism/client-pod/issues/94
closes https://github.com/ethereum-optimism/client-pod/issues/256

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 29, 2023

Walkthrough

Walkthrough

The changes across the codebase involve updates to contract deployment configurations, error handling enhancements, and test modifications. A new field related to output bisection game genesis output root has been added to several contracts, and hard-coded values have replaced configurable ones in some instances. Error handling has been improved in various functions, and new test scenarios have been added to ensure robustness. Additionally, there are updates to logging, signal handling, and temporary directory creation for end-to-end testing.

Changes

File Path Summary
op-chain-ops/genesis/config.go Added OutputBisectionGameGenesisOutputRoot field to DeployConfig struct.
packages/contracts-bedrock/scripts/Deploy.s.sol Added _genesisOutputRoot parameter and changed _maxGameDepth to a hard-coded value.
packages/contracts-bedrock/scripts/DeployConfig.s.sol Added outputBisectionGameGenesisOutputRoot variable and updated constructor.
packages/contracts-bedrock/src/dispute/OutputBisectionGame.sol Added GENESIS_OUTPUT_ROOT variable, updated constructor, and modified functions.
packages/contracts-bedrock/src/dispute/interfaces/IOutputBisectionGame.sol Updated resolveClaim function comment.
packages/contracts-bedrock/test/dispute/OutputBisectionGame.t.sol Changed constants, modified game instantiation, and added new test functions.
op-challenger/game/fault/... Modified registerOutputCannon and NewOutputCannonTraceAccessor functions to use splitDepth.
op-e2e/... Added cleanup functions, timeout contexts, and updated signal handling and logging.
op-node/rollup/derive/... Improved error handling, added new functions, and updated test functions.
op-node/cmd/batch_decoder/... Updated batch_decoder logic and added new flags and configuration loading.

Note: The table groups files with similar changes together to save space and omits parts of the file paths that are not essential for identifying the file.


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

@clabby
Copy link
Contributor Author

clabby commented Nov 29, 2023

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@codecov
Copy link

codecov bot commented Nov 29, 2023

Codecov Report

Merging #8351 (833e355) into develop (179aea6) will increase coverage by 0.38%.
The diff coverage is 0.00%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #8351      +/-   ##
===========================================
+ Coverage    40.01%   40.40%   +0.38%     
===========================================
  Files          164      132      -32     
  Lines         6180     5336     -844     
  Branches      1004      855     -149     
===========================================
- Hits          2473     2156     -317     
+ Misses        3633     3106     -527     
  Partials        74       74              
Flag Coverage Δ
cannon-go-tests 63.48% <ø> (ø)
chain-mon-tests 27.14% <ø> (ø)
common-ts-tests ?
contracts-bedrock-tests 20.28% <0.00%> (-0.11%) ⬇️
contracts-ts-tests 100.00% <ø> (ø)
core-utils-tests ?
sdk-next-tests 42.18% <ø> (ø)
sdk-tests 42.18% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...tracts-bedrock/src/dispute/OutputBisectionGame.sol 0.00% <0.00%> (ø)

... and 32 files with indirect coverage changes

@clabby clabby force-pushed the cl/ctb/output-bisection-contract branch 6 times, most recently from 27f6d8e to 579fdcc Compare December 1, 2023 02:23
@clabby clabby marked this pull request as ready for review December 1, 2023 02:25
@clabby clabby requested review from a team as code owners December 1, 2023 02:25
@clabby clabby requested review from Inphi and tynes December 1, 2023 02:25
Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

Logic looks good to me. I'm guessing not all the bindings changes will be required when updated to the latest develop. I haven't tried to review the solidity tests carefully but all the solidity code should be reviewed by someone who knows what they're doing there anyway. :)

The local context calculation and finding the starting and disputed claims matches what the challenger does.

@clabby clabby self-assigned this Dec 1, 2023
@clabby clabby force-pushed the cl/ctb/output-bisection-contract branch from 6e58b8b to 6a20e8d Compare December 1, 2023 17:13
Copy link
Contributor

@tynes tynes left a comment

Choose a reason for hiding this comment

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

I wouldn't say that I have the context to thoroughly review the implementation details but the code looks sane to me. Going to need to rely on testing for correctness

Copy link
Contributor

@refcell refcell left a comment

Choose a reason for hiding this comment

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

dogfood, otherwise lgtm

@clabby clabby force-pushed the cl/ctb/output-bisection-contract branch from 6a20e8d to 263c041 Compare December 1, 2023 20:31
@refcell refcell enabled auto-merge December 2, 2023 17:07
@refcell
Copy link
Contributor

refcell commented Dec 2, 2023

Need this in to fix tests in #8394 since it relies on having the GENESIS_OUTPUT_ROOT exposed to validate the output bisection game top split

@refcell refcell added this pull request to the merge queue Dec 2, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Dec 2, 2023
@refcell
Copy link
Contributor

refcell commented Dec 3, 2023

image

@ajsutton
Copy link
Contributor

ajsutton commented Dec 3, 2023

The test failing in the merge queue is for the output bisection game - exactly what this PR is changing. I'd say it's legit broken when this PR combines with the latest changes on develop, but either way it definitely is getting flakier so we shouldn't just keep retrying the merge an need to work out how to fix it.

@ajsutton ajsutton enabled auto-merge December 3, 2023 22:41
@ajsutton ajsutton force-pushed the cl/ctb/output-bisection-contract branch from 78d5475 to d37b1c3 Compare December 3, 2023 22:51
@ajsutton ajsutton disabled auto-merge December 3, 2023 23:18
@refcell
Copy link
Contributor

refcell commented Dec 4, 2023

The test failing in the merge queue is for the output bisection game - exactly what this PR is changing. I'd say it's legit broken when this PR combines with the latest changes on develop, but either way it definitely is getting flakier so we shouldn't just keep retrying the merge an need to work out how to fix it.

The test failing is the codecov test which is saying 0% of the output bisection game is covered with tests (incorrect). This is why i was attempting to add to the merge queue a few times...

clabby and others added 12 commits December 4, 2023 10:18
Co-authored-by: Adrian Sutton <adrian@oplabs.co>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Load the split depth from the contract when creating output_cannon games
Increase the max game depth so the cannon trace completes.
Legacy cannon games now have a fixed depth of 30 to avoid making those tests even slower.
Fix extra data creation in e2e helpers so the l2 block number is set correctly.
@clabby clabby force-pushed the cl/ctb/output-bisection-contract branch from d37b1c3 to 833e355 Compare December 4, 2023 15:18
@clabby clabby enabled auto-merge December 4, 2023 18:03
@clabby clabby added this pull request to the merge queue Dec 4, 2023
Merged via the queue into develop with commit 581b7a1 Dec 4, 2023
@clabby clabby deleted the cl/ctb/output-bisection-contract branch December 4, 2023 18:25
@ajsutton
Copy link
Contributor

ajsutton commented Dec 4, 2023

The test failing is the codecov test which is saying 0% of the output bisection game is covered with tests (incorrect). This is why i was attempting to add to the merge queue a few times...

image

TestOutputCannonGame e2e test was failing and preventing the merge. The code coverage isn't actually a merge blocker. 😀

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.

4 participants