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

Incorporate confirmation depth into the derivation pipeline #93

Merged
merged 15 commits into from
Nov 15, 2023

Conversation

jbearer
Copy link
Member

@jbearer jbearer commented Nov 12, 2023

Scary diff, but it's almost entirely plumbing. The crux of the change is in EspressoL1Origin. We simply subtract EspressoL1ConfDepth from the suggested L1 origin and then proceed as normal.

Plumbing: I had to add the new parameter to both SystemConfig and L1Block (the latter because the magic L1 block info transaction is used to reconstruct the SystemConfig via PayloadToSystemConfig in the derivation pipeline). For each of those contracts, the new parameter led to a "stack too deep" error, so I had to pull function parameters out into structs, which led to a lot of changes in the contract tests.

I also changed the way we pass information into CheckBatchEspresso, which actually reduced plumbing: instead of passing usingEspresso all through the derivation pipeline, we fetch the SystemConfig (which we now need anyways) in the batch queue just before calling CheckBatch. We then use the SystemConfig both to determine whether we are in Espresso mode and to get the value of EspressoL1ConfDepth.

Finally, instead of passing a function to retrieve the next L1 block into EspressoL1Origin, we now pass a general L1BlockRefByNumberFetcher. This allows EspressoL1Origin to be more careful about fetching only what is needed, to return a more useful L1BlockRef instead of just a block number, and to better encapsulate the business logic. Without this, we would need to adjust the suggested block number by EspressoL1ConfDepth at the call site (two places), since before this change the suggested L1BlockRef was fetched at the call site.

Testing: there are new unit tests, for acceptance and rejection, for the new logic in CheckBatchEspresso. I configured the first Espresso devnet to use conf depth 3 and the second to use conf depth 0, so we are testing both cases. This also means the e2e tests run with conf depth 3, since they use the config from the first devnet. I have also tested the devnet locally with conf depth 10. For that to work, I had to change the deposit waiting period from 12s to 36s for the ERC-20 deposit test, since all processing of deposit transactions is lagging by at least 10 L1 blocks (30s).

Depends on EspressoSystems/op-geth#12
Closes #81

@codecov-commenter
Copy link

codecov-commenter commented Nov 12, 2023

Codecov Report

Merging #93 (db30ffc) into integration (dba39dd) will increase coverage by 0.37%.
The diff coverage is 93.54%.

@@               Coverage Diff               @@
##           integration      #93      +/-   ##
===============================================
+ Coverage        45.19%   45.57%   +0.37%     
===============================================
  Files              178      178              
  Lines             7330     7337       +7     
  Branches          1161     1161              
===============================================
+ Hits              3313     3344      +31     
+ Misses            3894     3867      -27     
- Partials           123      126       +3     
Flag Coverage Δ
cannon-go-tests 63.47% <ø> (ø)
chain-mon-tests 26.95% <ø> (ø)
common-ts-tests 26.74% <ø> (ø)
contracts-bedrock-tests 39.75% <93.54%> (+0.91%) ⬆️
contracts-ts-tests 100.00% <ø> (ø)
core-utils-tests 44.03% <ø> (ø)
sdk-next-tests 41.41% <ø> (ø)
sdk-tests 41.41% <ø> (ø)

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

Files Coverage Δ
packages/contracts-bedrock/scripts/Deploy.s.sol 0.00% <ø> (ø)
...kages/contracts-bedrock/scripts/DeployConfig.s.sol 0.00% <ø> (ø)
packages/contracts-bedrock/src/L2/L1Block.sol 90.90% <100.00%> (+2.02%) ⬆️
packages/contracts-bedrock/src/L1/SystemConfig.sol 93.84% <90.47%> (-1.16%) ⬇️

... and 22 files with indirect coverage changes

@jbearer jbearer force-pushed the feat/l1-confirmations branch 2 times, most recently from 7cdc2aa to db30ffc Compare November 13, 2023 02:45
@jbearer jbearer force-pushed the feat/l1-confirmations branch from db30ffc to 4e0cca8 Compare November 13, 2023 03:32
@jbearer jbearer requested review from sveitser and nomaxg November 13, 2023 14:02
@jbearer jbearer marked this pull request as ready for review November 13, 2023 14:17
Copy link

@nomaxg nomaxg left a comment

Choose a reason for hiding this comment

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

Changes look good, but I'm getting the following error when I try pulling the latest images. (cd ops-bedrock; docker compose pull). Not sure whether its related to these changes.

WARNING: Some service image(s) must be built from source by running:
    docker compose build op_stack_go_builder
1 error occurred:
        * Error response from daemon: manifest for us-docker.pkg.dev/oplabs-tools-artifacts/images/op-stack-go:devnet not found: manifest unknown: Failed to fetch "devnet"

Devnet is working fine locally 👍

@jbearer
Copy link
Member Author

jbearer commented Nov 13, 2023

I'm not getting that error. Can you try the same command on integration and see if it fails there too? If so, I think docker compose build or make golang-docker will build the image that's missing. I have in the past been unable to download the OP stack go builder from the registry, I think it might be private or something

@nomaxg
Copy link

nomaxg commented Nov 15, 2023

I'm not getting that error. Can you try the same command on integration and see if it fails there too? If so, I think docker compose build or make golang-docker will build the image that's missing. I have in the past been unable to download the OP stack go builder from the registry, I think it might be private or something

Still not able to pull it, but I'm seeing the same issue on integration so probably just a local issue for me. Looks like this image is stored on google cloud instead of the github container registry so could be related to that. Anyways, this shouldn't block this PR.

@jbearer jbearer merged commit 77ffa54 into integration Nov 15, 2023
4 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.

Incorporate confirmation depth into the derivation pipeline
3 participants