Fjord: Add Brotli channel compression support #10358
Conversation
cody-wang-cb
requested review from
protolambda,
trianglesphere,
ajsutton and
a team
as code owners
WalkthroughWalkthroughThe changes introduce a new compression algorithm option for the Optimism batcher and related components. This includes adding a Changes
Recent Review DetailsConfiguration used: .coderabbit.yml Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Out of diff range and nitpick comments (3)
op-batcher/compressor/config.go (1)
20-21: Add documentation forCompressionAlgoandCompressLevel.It's good practice to document struct fields, especially when they are public. This helps other developers understand the purpose and usage of these fields quickly.
op-node/rollup/derive/channel.go (1)
Line range hint
155-204: RefactorBatchReaderto improve readability and error handling.func BatchReader(r io.Reader) (func() (*BatchData, error), error) { var buffer strings.Builder if _, err := io.Copy(&buffer, r); err != nil { return nil, err } tmpReader := strings.NewReader(buffer.String()) compressionType := make([]byte, 1) if _, err := tmpReader.Read(compressionType); err != nil { return nil, err } fmt.Println("Compression type") fmt.Println(strconv.FormatInt(int64(compressionType[0]), 2)) fmt.Println(compressionType[0]) r = strings.NewReader(buffer.String()) var decompress func(io.Reader) (io.Reader, error) if compressionType[0]&0x0F == 8 { fmt.Println("Using zlib reader") decompress = func(r io.Reader) (io.Reader, error) { return zlib.NewReader(r) } } else if compressionType[0]&0x0F == 2 { fmt.Println("Using brotli reader") r.Read(make([]byte, 1)) // Skip the compression type byte decompress = func(r io.Reader) (io.Reader, error) { return brotli.NewReader(r), nil } } zr, err := decompress(r) if err != nil { return nil, err } rlpReader := rlp.NewStream(zr, MaxRLPBytesPerChannel) return func() (*BatchData, error) { var batchData BatchData if err = rlpReader.Decode(&batchData); err != nil { if err != io.EOF { fmt.Println("DECODE ERROR") fmt.Println(err) } return nil, err } return &batchData, nil }, nil }Refactoring the
BatchReaderfunction improves readability and error handling. The proposed changes make the function easier to understand and maintain, especially the handling of different compression types.op-batcher/batcher/config.go (1)
Line range hint
70-138: Ensure comprehensive validation inCLIConfig.Check.func (c *CLIConfig) Check() error { + if c.CompressionAlgo != "zlib" && c.CompressionAlgo != "brotli" { + return errors.New("unsupported compression algorithm") + } + if (c.CompressionAlgo == "zlib" && (c.CompressLevel < 0 || c.CompressLevel > 9)) || + (c.CompressionAlgo == "brotli" && (c.CompressLevel < 1 || c.CompressLevel > 11)) { + return fmt.Errorf("invalid compression level %d for algorithm %s", c.CompressLevel, c.CompressionAlgo) + } if c.L1EthRpc == "" { return errors.New("empty L1 RPC URL") } if c.L2EthRpc == "" { return errors.New("empty L2 RPC URL") } if c.RollupRpc == "" { return errors.New("empty rollup RPC URL") } if strings.Count(c.RollupRpc, ",") != strings.Count(c.L2EthRpc, ",") { return errors.New("number of rollup and eth URLs must match") } if c.PollInterval == 0 { return errors.New("must set PollInterval") } if c.MaxL1TxSize <= 1 { return errors.New("MaxL1TxSize must be greater than 1") } if c.TargetNumFrames < 1 { return errors.New("TargetNumFrames must be at least 1") } if c.Compressor == compressor.RatioKind && (c.ApproxComprRatio <= 0 || c.ApproxComprRatio > 1) { return fmt.Errorf("invalid ApproxComprRatio %v for ratio compressor", c.ApproxComprRatio) } if c.BatchType > 1 { return fmt.Errorf("unknown batch type: %v", c.BatchType) } if c.CheckRecentTxsDepth > 128 { return fmt.Errorf("CheckRecentTxsDepth cannot be set higher than 128: %v", c.CheckRecentTxsDepth) } if c.DataAvailabilityType == flags.BlobsType && c.TargetNumFrames > 6 { return errors.New("too many frames for blob transactions, max 6") } if !flags.ValidDataAvailabilityType(c.DataAvailabilityType) { return fmt.Errorf("unknown data availability type: %q", c.DataAvailabilityType) } if err := c.MetricsConfig.Check(); err != nil { return err } if err := c.PprofConfig.Check(); err != nil { return err } if err := c.TxMgrConfig.Check(); err != nil { return err } if err := c.RPC.Check(); err != nil { return err } return nil }Adding comprehensive validation for the new compression settings in the
CLIConfig.Checkmethod ensures that the configuration is always valid. This prevents runtime errors due to incorrect configuration.
cody-wang-cb
force-pushed
the
cody/brotli-impl
branch
from
5ee1a1a
to
6566283
Compare
cody-wang-cb
force-pushed
the
cody/brotli-impl
branch
from
08e579a
to
7dd27e9
Compare
cody-wang-cb
requested review from
a team,
0x00101010,
zhwrd and
mslipper
as code owners
There was a problem hiding this comment.
Actionable comments posted: 13
Out of diff range and nitpick comments (15)
op-challenger/game/fault/trace/asterisc/provider.go (1)
39-40: Clarify the role ofPrestateProviderin the class.It would be beneficial to add a comment explaining the role and usage of
PrestateProviderwithinAsteriscTraceProviderfor better code readability.op-challenger/game/fault/contracts/gamefactory_test.go (1)
64-64: Ensure consistency in game metadata structure.It's good practice to ensure that the structure and naming of fields in
GameMetadataare consistent across different tests and actual usage in the application.docs/postmortems/2022-02-02-inflation-vuln.md (7)
Line range hint
8-8: Correct the spelling of "Geth" in the URL.- [Geth](https://github.com/ethereum/go-ethereu) + [Geth](https://github.com/ethereum/go-ethereum)
Line range hint
19-19: Start the section title "Lead up" with an uppercase letter.- ## Lead up + ## Lead Up
Line range hint
22-22: Consider replacing "for several months prior to" with "for several months before".- for several months prior to identifying this bug. + for several months before identifying this bug.
Line range hint
41-41: Replace "to" with "on" for correct preposition usage.- no impact to ordinary users. + no impact on ordinary users.
Line range hint
53-53: Capitalize "Slack" to refer to the communication tool properly.- private slack channel. + private Slack channel.
Line range hint
58-58: Capitalize "GitHub" to refer to the platform properly.- Using github handles as identifiers + Using GitHub handles as identifiers
Line range hint
72-72: Replace "infra" with a more formal term like "infrastructure".- instructions to infra providers on how to upgrade. + instructions to infrastructure providers on how to upgrade.bedrock-devnet/devnet/__init__.py (5)
Line range hint
7-7: Consider removing the unused importcalendar.- import calendar
Line range hint
12-12: Consider removing the unused importgzip.- import gzip
Line range hint
18-18: Consider removing the unused importdevnet.log_setup.- import devnet.log_setup
Line range hint
219-219: Simplify the boolean checks by using direct truthy or falsy evaluations.- if os.path.exists(l2_allocs_path) == False or DEVNET_FPAC == True: + if not os.path.exists(l2_allocs_path) or DEVNET_FPAC:
Line range hint
308-308: The local variableeis assigned but never used. Consider logging the exception or removing it if not needed.- except Exception as e: + except Exception:packages/contracts-bedrock/src/L1/OptimismPortal2.sol (1)
97-99: Consider adding a comment explaining the purpose of the spacer_balance.While the code is functionally correct, adding a comment explaining the purpose of the spacer
_balancewould improve code readability and maintainability.
cody-wang-cb
force-pushed
the
cody/brotli-impl
branch
from
8b376d0
to
934ed5e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Out of diff range and nitpick comments (2)
op-node/rollup/derive/channel_test.go (2)
Line range hint
36-36: Replacerequire.Nilwithrequire.NoErrorfor better error formatting.- require.Nil(t, err) + require.NoError(t, err)
Line range hint
36-36: Replacerequire.NotNilwithrequire.Errorto explicitly check for errors.- require.NotNil(t, err) + require.Error(t, err)
sebastianst
dismissed
axelKingsley’s stale review
Axel's concerns have been addressed.
4b8f6f4
Description
In the batcher
ChannelCompressorthat abstracts away compression logic, it determines how it compresses based on the algo set. For Brotli, it prepends one byte (0x01) as channel version for Brotli every time it resets. So that the channel will always have the channel version byte.span_channel_outit uses theChannelCompressor.RatioCompressorandShadowCompressorboth integratesChannelCompressor.NonCompressordoes not integrate given it's not compressing the data.For
ChannelBankBatchReader, it determines which compression algorithm by reading the first byte.Tests
Additional context
Add any other context about the problem you're solving.
Metadata