-
Notifications
You must be signed in to change notification settings - Fork 3.8k
op-chain-ops: add check-jovian cmd #18269
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
base: develop
Are you sure you want to change the base?
Conversation
geoknee
commented
Nov 13, 2025
| if latest.BlobGasUsed == nil { | ||
| return fmt.Errorf("block %d has nil BlobGasUsed field", latest.Number) | ||
| } |
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.
This doesn't really show that Jovian is active: to do that, there must be some traffic (user transactions) on the chain, and then the value would be nonzero. This can be tested manually (our acceptance tests use a tx spammer to do this).
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.
Yeah, but as a first smoke test that's already good, because only with Jovian is that field non-nil.
If a more comprehensive correctness test is needed, then the acceptance tests should be ran.
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.
Actually, it is zero pre-Jovian (not nil) https://specs.optimism.io/protocol/jovian/exec-engine.html#da-footprint-block-limit. This step passes on OPM (pre Jovian).
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.
Ohh, good callout, we only omitted it in receipts where it is added with Jovian. Then this check is indeed invalid/useless. Let's print a message like "Zero blobGasUsed is inconclusive" and say that the check should be run again on a block with any user transactions in it to pass. But it shouldn't error.
| if latest.BlobGasUsed == nil { | ||
| return fmt.Errorf("block %d has nil BlobGasUsed field", latest.Number) | ||
| } |
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.
Yeah, but as a first smoke test that's already good, because only with Jovian is that field non-nil.
If a more comprehensive correctness test is needed, then the acceptance tests should be ran.
| if len(extra) != 17 { | ||
| return fmt.Errorf("extraData should be 17 bytes for Jovian, got %d", len(extra)) | ||
| } | ||
|
|
||
| // Check version byte - must be 1 for Jovian (incremented from Holocene's 0) | ||
| const JovianExtraDataVersionByte = uint8(0x01) | ||
| if extra[0] != JovianExtraDataVersionByte { | ||
| return fmt.Errorf("extraData version byte should be %d for Jovian, got %d", | ||
| JovianExtraDataVersionByte, extra[0]) | ||
| } | ||
|
|
||
| // Decode EIP-1559 parameters (denominator and elasticity) | ||
| denominator := binary.BigEndian.Uint32(extra[1:5]) | ||
| elasticity := binary.BigEndian.Uint32(extra[5:9]) | ||
|
|
||
| // Validate EIP-1559 params: denominator must be non-zero (unless elasticity is also 0) | ||
| if elasticity != 0 && denominator == 0 { | ||
| return fmt.Errorf("extraData has invalid EIP-1559 params: denominator cannot be 0 when elasticity is %d", elasticity) | ||
| } | ||
|
|
||
| // Decode minimum base fee | ||
| minBaseFee := binary.BigEndian.Uint64(extra[9:17]) |
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.
Why don't you just use any of ValidateOptimismExtraData or DecodeOptimismExtraData from op-geth package eip1559?
… to differentiate between zero and non-zero BlobGasUsed
|
|
||
| // A non-zero BlobGasUsed is hard evidence of Jovian being active | ||
| // A zero value is inconclusive (could be an empty block or pre-Jovian) | ||
| if *latest.BlobGasUsed == 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.
We can make the check fail if the last transaction in the block is a non-deposit tx.
| isJovian, err := cl.IsJovian(nil) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get jovian status: %w", err) | ||
| } | ||
| if !isJovian { |
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.
Denial of Service (DoS): Missing RPC call timeouts in checkGPO
The call to cl.IsJovian(nil) uses default CallOpts without a Context or timeout, allowing a malicious or unresponsive RPC endpoint to hang the CLI indefinitely.
Pass the CLI context into contract calls (e.g., use &bind.CallOpts{Context: ctx}) and configure the RPC client with timeouts to prevent hangs.
Don't like this finding? Reply "dismiss" and it won't appear again in future scans.
If it's acknowledged or addressed, reply "resolve" to mark it resolved.
