-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
EIP4844: Add single minimal e2e test. #8115
Conversation
66c9aee
to
d3e2ab8
Compare
Rebased to incorporate https://github.com/ethereum-optimism/optimism/pull/8131/files. |
Rebased off of develop per conversation with @clabby. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Core of the test looks good. Two questions about making it slightly nicer.
Semgrep found 1
Prefer Semgrep found 1
Inputs to functions must be prepended with an underscore ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks ok, but would squash (and rebase) the commits before merging, it's a lot for what are pretty isolated changes.
The diff also shows merged changes from other PRs like #8196, squash+rebase should help! |
19dccba
to
47b27ff
Compare
@protolambda @sebastianst Got it - squashed commit history into a single commit based off current |
WalkthroughWalkthroughThe changes involve updates to Ethereum end-to-end testing utilities and the fake proof-of-stake (PoS) implementation. A new version of payload and fork choice update methods ( Changes
TipsChat with CodeRabbit Bot (
|
Semgrep found 7 Inputs to functions must be prepended with an underscore ( Semgrep found 1
Service 'op_stack_go_builder' is running with a writable root filesystem. This may allow malicious applications to download and run additional payloads, or modify container files. If an application inside a container has to save something temporarily consider using a tmpfs. Add 'read_only: true' to this service to prevent this. Ignore this finding from writable-filesystem-service.Semgrep found 1
Service 'op_stack_go_builder' allows for privilege escalation via setuid or setgid binaries. Add 'no-new-privileges:true' in 'security_opt' to prevent this. Ignore this finding from no-new-privileges. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (3)
- op-e2e/e2eutils/geth/fakepos.go (2 hunks)
- op-e2e/e2eutils/transactions/blobs.go (1 hunks)
- op-e2e/system_test.go (2 hunks)
Additional comments: 6
op-e2e/e2eutils/geth/fakepos.go (3)
138-144: The
GetPayloadV3
method is used to retrieve the payload for the new block. Ensure that the rest of the system is compatible with theV3
payload structure and that any necessary migrations or updates have been made.178-184: The
ForkchoiceUpdatedV3
method is called without attributes, which is different from the previous call toForkchoiceUpdatedV2
that includedattrs
. Ensure that this change is intentional and that the system's logic is correctly updated to reflect the absence of attributes in theV3
method call.181-184: The
ForkchoiceUpdatedV3
method is used to update the fork choice state. Ensure that the new block hash, safe block hash, and finalized block hash are correctly calculated and that the rest of the system is compatible with these changes.op-e2e/e2eutils/transactions/blobs.go (1)
- 38-40: The use of the
withSidecar
flag to conditionally include a sidecar in the transaction is clear and follows good practice.op-e2e/system_test.go (2)
31-38: The new imports
gethutils
andtransactions
are correctly added and used in the new test functionTestSystemE2EDencunAtGenesisWithBlobs
.176-218: The test function
TestSystemE2EDencunAtGenesisWithBlobs
is well-structured and follows best practices for writing tests in Go. It includes proper error handling for external calls, such as sending transactions and waiting for blocks, and does not appear to have any data races or concurrency issues. The test function is maintainable and not overly complex, and there is no dead or unnecessary code present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge queue status check failed, rebasing.... |
65a929a
to
f148caf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- op-e2e/e2eutils/geth/fakepos.go (2 hunks)
- op-e2e/e2eutils/transactions/blobs.go (1 hunks)
- op-e2e/system_test.go (2 hunks)
Additional comments: 9
op-e2e/e2eutils/geth/fakepos.go (4)
141-141: The summary states that
GetPayloadV2
was changed toGetPayloadV3
at line 138, but the change is actually at line 141. Please update the summary to reflect the correct line number for this change.181-181: The summary states that
ForkchoiceUpdatedV2
was replaced withForkchoiceUpdatedV3
at line 178, but the change is actually at line 181. Please update the summary to reflect the correct line number for this change.138-144: The change from
GetPayloadV2
toGetPayloadV3
is correctly implemented and aligns with the pull request description.178-184: The change from
ForkchoiceUpdatedV2
toForkchoiceUpdatedV3
is correctly implemented and aligns with the pull request description.op-e2e/system_test.go (5)
31-38: The import additions are correctly placed and necessary for the new functionality.
176-210: The new test
TestSystemE2EDencunAtGenesisWithBlobs
is added to verify L2 block finalization when blob-containing transactions are present on L1.190-191: Using a hardcoded private key in a test environment is acceptable.
189-210: The test sends a blob-containing transaction and waits for it to be included in a block on L1, then checks for the finalization on L2.
176-210: The test
TestSystemE2EDencunAtGenesisWithBlobs
does not have any logic issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- op-e2e/system_test.go (2 hunks)
Additional comments: 2
op-e2e/system_test.go (2)
31-37: The import additions look good and are consistent with the summary.
176-210: The new test function
TestSystemE2EDencunAtGenesisWithBlobs
is consistent with the summary and appears to be logically correct, testing L2 finalization with blob-containing transactions on L1.
5c43ad0
to
4f298ed
Compare
lint failed after the merge in from develop @EvanJRichard |
…show up on L2. Then, wait for that block to be finalized.
Per code rabbit AI. Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
per trianglesphere, double var causes miscompilation Co-authored-by: Joshua Gutow <jgutow@oplabs.co>
per protolambda Co-authored-by: protolambda <proto@protolambda.com>
783fd62
to
7e780be
Compare
Description
As an intermediate step to #8104 and therefore #7349, this PR aims to add a minimal system test that starts a Cancun-compatible L1, sends a blob-containing transaction on that L1, and ensures the L2 can finalize against that L1.
Tests
The test is called
TestSystemE2EDencunAtGenesisWithBlobs
insystem_test.go
.Metadata
Builds on top of #8131