-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Some Superchain L2 Genesis Fixes #10231
Conversation
WalkthroughWalkthroughThe recent updates introduce new fields Changes
Recent Review DetailsConfiguration used: .coderabbit.yml Files selected for processing (1)
Additional comments not posted (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #10231 +/- ##
============================================
- Coverage 42.40% 29.22% -13.18%
============================================
Files 73 31 -42
Lines 4830 2898 -1932
Branches 766 614 -152
============================================
- Hits 2048 847 -1201
+ Misses 2676 1976 -700
+ Partials 106 75 -31
Flags with carried forward coverage won't be shown. Click here to find out more. |
Semgrep found 13
Prefer Semgrep found 5
No Semgrep found 1 When working with web applications that involve rendering user-generated content, it's important to properly escape any HTML content to prevent Cross-Site Scripting (XSS) attacks. In Go, the Semgrep found 1 Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code. Ignore this finding from dangerous-exec-command. |
Co-authored-by: George C. Knee <georgeknee@googlemail.com>
Semgrep found 4
Named return arguments to functions must be appended with an underscore ( Semgrep found 3 Prefer Semgrep found 3 Inputs to functions must be prepended with an underscore ( Semgrep found 10
No 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.Semgrep found 1 When working with web applications that involve rendering user-generated content, it's important to properly escape any HTML content to prevent Cross-Site Scripting (XSS) attacks. In Go, the Semgrep found 1 Detected a network listener listening on 0.0.0.0 or an empty string. This could unexpectedly expose the server publicly as it binds to all available interfaces. Instead, specify another IP address that is not 0.0.0.0 nor the empty string. Ignore this finding from avoid-bind-to-all-interfaces.Semgrep found 1 Do not use Semgrep found 1 superfluous nil err check before return Ignore this finding from err-nil-check. |
@@ -65,7 +65,7 @@ config=$(cat << EOL | |||
"l1FeeVaultWithdrawalNetwork": 0, | |||
"sequencerFeeVaultWithdrawalNetwork": 0, | |||
|
|||
"gasPriceOracleOverhead": 2100, | |||
"gasPriceOracleOverhead": 0, |
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.
wouldn't this break?
optimism/op-node/rollup/types.go
Lines 281 to 286 in 93891de
if cfg.Genesis.SystemConfig.Overhead == (eth.Bytes32{}) { | |
return ErrMissingOverhead | |
} | |
if cfg.Genesis.SystemConfig.Scalar == (eth.Bytes32{}) { | |
return ErrMissingScalar | |
} |
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.
optimism/op-chain-ops/genesis/config.go
Line 639 in 93891de
Overhead: eth.Bytes32(common.BigToHash(new(big.Int).SetUint64(d.GasPriceOracleOverhead))), |
a 0 Overhead will turn into:
SystemConfig: eth.SystemConfig{
BatcherAddr: d.BatchSenderAddress,
Overhead: eth.Bytes32(common.BigToHash(new(big.Int).SetUint64(d.GasPriceOracleOverhead))),
Scalar: eth.Bytes32(d.FeeScalar()),
GasLimit: uint64(d.L2GenesisBlockGasLimit),
},
empty 32 bytes.
After which op-node validation fails:
optimism/op-node/rollup/types.go
Lines 262 to 264 in 58a3fde
if cfg.Genesis.SystemConfig.Overhead == (eth.Bytes32{}) { | |
return ErrMissingOverhead | |
} |
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.
Thanks for highlighting. This will be addressed here #10226
* add l2GenesisEcotoneTimeOffset to getting-started/config.sh * set l2GenesisDeltaTimeOffset to 0 * remove unecessary line * parse 4844 params in registry-data tool * update overhead and scalar * add 4844 fields to the other Genesis literal * scalar: revert Co-authored-by: George C. Knee <georgeknee@googlemail.com> --------- Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com>
Closes https://github.com/ethereum-optimism/security-pod/issues/107