diff --git a/op-batcher/batcher/channel_builder_test.go b/op-batcher/batcher/channel_builder_test.go index 1103214783901..03e8c4d9b5915 100644 --- a/op-batcher/batcher/channel_builder_test.go +++ b/op-batcher/batcher/channel_builder_test.go @@ -132,10 +132,10 @@ func FuzzDurationTimeoutZeroMaxChannelDuration(f *testing.F) { }) } -// FuzzDurationZero ensures that when whenever the MaxChannelDuration +// FuzzChannelBuilder_DurationZero ensures that when whenever the MaxChannelDuration // is not set to 0, the channel builder will always have a duration timeout // as long as the channel builder's timeout is set to 0. -func FuzzDurationZero(f *testing.F) { +func FuzzChannelBuilder_DurationZero(f *testing.F) { for i := range [10]int{} { f.Add(uint64(i), uint64(i)) } @@ -313,8 +313,8 @@ func FuzzSeqWindowZeroTimeoutClose(f *testing.F) { }) } -// TestBuilderNextFrame tests calling NextFrame on a ChannelBuilder with only one frame -func TestBuilderNextFrame(t *testing.T) { +// TestChannelBuilder_NextFrame tests calling NextFrame on a ChannelBuilder with only one frame +func TestChannelBuilder_NextFrame(t *testing.T) { channelConfig := defaultTestChannelConfig // Create a new channel builder @@ -353,8 +353,8 @@ func TestBuilderNextFrame(t *testing.T) { require.PanicsWithValue(t, "no next frame", func() { cb.NextFrame() }) } -// TestBuilderInvalidFrameId tests that a panic is thrown when a frame is pushed with an invalid frame id -func TestBuilderWrongFramePanic(t *testing.T) { +// TestChannelBuilder_OutputWrongFramePanic tests that a panic is thrown when a frame is pushed with an invalid frame id +func TestChannelBuilder_OutputWrongFramePanic(t *testing.T) { channelConfig := defaultTestChannelConfig // Construct a channel builder @@ -383,15 +383,14 @@ func TestBuilderWrongFramePanic(t *testing.T) { }) } -// TestOutputFrames tests the OutputFrames function -func TestOutputFrames(t *testing.T) { +// TestChannelBuilder_OutputFramesWorks tests the [ChannelBuilder] OutputFrames is successful. +func TestChannelBuilder_OutputFramesWorks(t *testing.T) { channelConfig := defaultTestChannelConfig - channelConfig.MaxFrameSize = 2 + channelConfig.MaxFrameSize = 24 // Construct the channel builder cb, err := newChannelBuilder(channelConfig) require.NoError(t, err) - require.False(t, cb.IsFull()) require.Equal(t, 0, cb.NumFrames()) @@ -400,40 +399,34 @@ func TestOutputFrames(t *testing.T) { require.NoError(t, cb.OutputFrames()) // There should be no ready bytes yet - readyBytes := cb.co.ReadyBytes() - require.Equal(t, 0, readyBytes) + require.Equal(t, 0, cb.co.ReadyBytes()) // Let's add a block - err = addMiniBlock(cb) - require.NoError(t, err) + require.NoError(t, addMiniBlock(cb)) + require.NoError(t, cb.co.Flush()) // Check how many ready bytes - readyBytes = cb.co.ReadyBytes() - require.Equal(t, 2, readyBytes) - + // There should be more than the max frame size ready + require.Greater(t, uint64(cb.co.ReadyBytes()), channelConfig.MaxFrameSize) require.Equal(t, 0, cb.NumFrames()) // The channel should not be full // but we want to output the frames for testing anyways - isFull := cb.IsFull() - require.False(t, isFull) - - // Since we manually set the max frame size to 2, - // we should be able to compress the two frames now - err = cb.OutputFrames() - require.NoError(t, err) + require.False(t, cb.IsFull()) - // There should be one frame in the channel builder now - require.Equal(t, 1, cb.NumFrames()) + // We should be able to output the frames + require.NoError(t, cb.OutputFrames()) - // There should no longer be any ready bytes - readyBytes = cb.co.ReadyBytes() - require.Equal(t, 0, readyBytes) + // There should be many frames in the channel builder now + require.Greater(t, cb.NumFrames(), 1) + for _, frame := range cb.frames { + require.Len(t, frame.data, int(channelConfig.MaxFrameSize)) + } } -// TestMaxRLPBytesPerChannel tests the [channelBuilder.OutputFrames] +// TestChannelBuilder_MaxRLPBytesPerChannel tests the [channelBuilder.OutputFrames] // function errors when the max RLP bytes per channel is reached. -func TestMaxRLPBytesPerChannel(t *testing.T) { +func TestChannelBuilder_MaxRLPBytesPerChannel(t *testing.T) { t.Parallel() channelConfig := defaultTestChannelConfig channelConfig.MaxFrameSize = derive.MaxRLPBytesPerChannel * 2 @@ -449,13 +442,13 @@ func TestMaxRLPBytesPerChannel(t *testing.T) { require.ErrorIs(t, err, derive.ErrTooManyRLPBytes) } -// TestOutputFramesMaxFrameIndex tests the [channelBuilder.OutputFrames] +// TestChannelBuilder_OutputFramesMaxFrameIndex tests the [ChannelBuilder.OutputFrames] // function errors when the max frame index is reached. -func TestOutputFramesMaxFrameIndex(t *testing.T) { +func TestChannelBuilder_OutputFramesMaxFrameIndex(t *testing.T) { channelConfig := defaultTestChannelConfig - channelConfig.MaxFrameSize = 1 + channelConfig.MaxFrameSize = 24 channelConfig.TargetNumFrames = math.MaxInt - channelConfig.TargetFrameSize = 1 + channelConfig.TargetFrameSize = 24 channelConfig.ApproxComprRatio = 0 // Continuously add blocks until the max frame index is reached @@ -477,6 +470,7 @@ func TestOutputFramesMaxFrameIndex(t *testing.T) { Number: big.NewInt(0), }, txs, nil, nil, trie.NewStackTrie(nil)) _, err = cb.AddBlock(a) + require.NoError(t, cb.co.Flush()) if cb.IsFull() { fullErr := cb.FullErr() require.ErrorIs(t, fullErr, ErrMaxFrameIndex) @@ -489,17 +483,15 @@ func TestOutputFramesMaxFrameIndex(t *testing.T) { } } -// TestBuilderAddBlock tests the AddBlock function -func TestBuilderAddBlock(t *testing.T) { +// TestChannelBuilder_AddBlock tests the AddBlock function +func TestChannelBuilder_AddBlock(t *testing.T) { channelConfig := defaultTestChannelConfig // Lower the max frame size so that we can batch - channelConfig.MaxFrameSize = 2 + channelConfig.MaxFrameSize = 30 // Configure the Input Threshold params so we observe a full channel - // In reality, we only need the input bytes (74) below to be greater than - // or equal to the input threshold (3 * 2) / 1 = 6 - channelConfig.TargetFrameSize = 3 + channelConfig.TargetFrameSize = 30 channelConfig.TargetNumFrames = 2 channelConfig.ApproxComprRatio = 1 @@ -508,8 +500,8 @@ func TestBuilderAddBlock(t *testing.T) { require.NoError(t, err) // Add a nonsense block to the channel builder - err = addMiniBlock(cb) - require.NoError(t, err) + require.NoError(t, addMiniBlock(cb)) + require.NoError(t, cb.co.Flush()) // Check the fields reset in the AddBlock function require.Equal(t, 74, cb.co.InputBytes()) @@ -519,23 +511,22 @@ func TestBuilderAddBlock(t *testing.T) { // Since the channel output is full, the next call to AddBlock // should return the channel out full error - err = addMiniBlock(cb) - require.ErrorIs(t, err, ErrInputTargetReached) + require.ErrorIs(t, addMiniBlock(cb), ErrInputTargetReached) } -// TestBuilderReset tests the Reset function -func TestBuilderReset(t *testing.T) { +// TestChannelBuilder_Reset tests the [Reset] function +func TestChannelBuilder_Reset(t *testing.T) { channelConfig := defaultTestChannelConfig // Lower the max frame size so that we can batch - channelConfig.MaxFrameSize = 2 + channelConfig.MaxFrameSize = 24 cb, err := newChannelBuilder(channelConfig) require.NoError(t, err) // Add a nonsense block to the channel builder - err = addMiniBlock(cb) - require.NoError(t, err) + require.NoError(t, addMiniBlock(cb)) + require.NoError(t, cb.co.Flush()) // Check the fields reset in the Reset function require.Equal(t, 1, len(cb.blocks)) @@ -546,22 +537,20 @@ func TestBuilderReset(t *testing.T) { require.NoError(t, cb.fullErr) // Output frames so we can set the channel builder frames - err = cb.OutputFrames() - require.NoError(t, err) + require.NoError(t, cb.OutputFrames()) // Add another block to increment the block count - err = addMiniBlock(cb) - require.NoError(t, err) + require.NoError(t, addMiniBlock(cb)) + require.NoError(t, cb.co.Flush()) // Check the fields reset in the Reset function require.Equal(t, 2, len(cb.blocks)) - require.Equal(t, 1, len(cb.frames)) + require.Greater(t, len(cb.frames), 1) require.Equal(t, timeout, cb.timeout) require.NoError(t, cb.fullErr) // Reset the channel builder - err = cb.Reset() - require.NoError(t, err) + require.NoError(t, cb.Reset()) // Check the fields reset in the Reset function require.Equal(t, 0, len(cb.blocks)) diff --git a/op-batcher/batcher/channel_manager_test.go b/op-batcher/batcher/channel_manager_test.go index 335859ca5141e..ec04fb1f9dd00 100644 --- a/op-batcher/batcher/channel_manager_test.go +++ b/op-batcher/batcher/channel_manager_test.go @@ -32,8 +32,7 @@ func TestPendingChannelTimeout(t *testing.T) { require.False(t, timeout) // Set the pending channel - err := m.ensurePendingChannel(eth.BlockID{}) - require.NoError(t, err) + require.NoError(t, m.ensurePendingChannel(eth.BlockID{})) // There are no confirmed transactions so // the pending channel cannot be timed out @@ -85,14 +84,10 @@ func TestChannelManagerReturnsErrReorg(t *testing.T) { ParentHash: common.Hash{0xff}, }, nil, nil, nil, nil) - err := m.AddL2Block(a) - require.NoError(t, err) - err = m.AddL2Block(b) - require.NoError(t, err) - err = m.AddL2Block(c) - require.NoError(t, err) - err = m.AddL2Block(x) - require.ErrorIs(t, err, ErrReorg) + require.NoError(t, m.AddL2Block(a)) + require.NoError(t, m.AddL2Block(b)) + require.NoError(t, m.AddL2Block(c)) + require.ErrorIs(t, m.AddL2Block(x), ErrReorg) require.Equal(t, []*types.Block{a, b, c}, m.blocks) } @@ -111,16 +106,14 @@ func TestChannelManagerReturnsErrReorgWhenDrained(t *testing.T) { a := newMiniL2Block(0) x := newMiniL2BlockWithNumberParent(0, big.NewInt(1), common.Hash{0xff}) - err := m.AddL2Block(a) - require.NoError(t, err) + require.NoError(t, m.AddL2Block(a)) - _, err = m.TxData(eth.BlockID{}) + _, err := m.TxData(eth.BlockID{}) require.NoError(t, err) _, err = m.TxData(eth.BlockID{}) require.ErrorIs(t, err, io.EOF) - err = m.AddL2Block(x) - require.ErrorIs(t, err, ErrReorg) + require.ErrorIs(t, m.AddL2Block(x), ErrReorg) } // TestChannelManagerNextTxData checks the nextTxData function. @@ -136,8 +129,7 @@ func TestChannelManagerNextTxData(t *testing.T) { // Set the pending channel // The nextTxData function should still return EOF // since the pending channel has no frames - err = m.ensurePendingChannel(eth.BlockID{}) - require.NoError(t, err) + require.NoError(t, m.ensurePendingChannel(eth.BlockID{})) returnedTxData, err = m.nextTxData() require.ErrorIs(t, err, io.EOF) require.Equal(t, txData{}, returnedTxData) @@ -164,8 +156,10 @@ func TestChannelManagerNextTxData(t *testing.T) { require.Equal(t, expectedTxData, m.pendingTransactions[expectedChannelID]) } -// TestClearChannelManager tests clearing the channel manager. -func TestClearChannelManager(t *testing.T) { +// TestChannelManager_Clear tests clearing the channel manager. +func TestChannelManager_Clear(t *testing.T) { + require := require.New(t) + // Create a channel manager log := testlog.Logger(t, log.LvlCrit) rng := rand.New(rand.NewSource(time.Now().UnixNano())) @@ -176,15 +170,17 @@ func TestClearChannelManager(t *testing.T) { ChannelTimeout: 10, // Have to set the max frame size here otherwise the channel builder would not // be able to output any frames - MaxFrameSize: 1, + MaxFrameSize: 24, + TargetFrameSize: 24, + ApproxComprRatio: 1.0, }) // Channel Manager state should be empty by default - require.Empty(t, m.blocks) - require.Equal(t, common.Hash{}, m.tip) - require.Nil(t, m.pendingChannel) - require.Empty(t, m.pendingTransactions) - require.Empty(t, m.confirmedTransactions) + require.Empty(m.blocks) + require.Equal(common.Hash{}, m.tip) + require.Nil(m.pendingChannel) + require.Empty(m.pendingTransactions) + require.Empty(m.confirmedTransactions) // Add a block to the channel manager a, _ := derivetest.RandomL2Block(rng, 4) @@ -193,28 +189,25 @@ func TestClearChannelManager(t *testing.T) { Hash: a.Hash(), Number: a.NumberU64(), } - err := m.AddL2Block(a) - require.NoError(t, err) + require.NoError(m.AddL2Block(a)) // Make sure there is a channel builder - err = m.ensurePendingChannel(l1BlockID) - require.NoError(t, err) - require.NotNil(t, m.pendingChannel) - require.Equal(t, 0, len(m.confirmedTransactions)) + require.NoError(m.ensurePendingChannel(l1BlockID)) + require.NotNil(m.pendingChannel) + require.Len(m.confirmedTransactions, 0) // Process the blocks // We should have a pending channel with 1 frame // and no more blocks since processBlocks consumes // the list - err = m.processBlocks() - require.NoError(t, err) - err = m.pendingChannel.OutputFrames() - require.NoError(t, err) - _, err = m.nextTxData() - require.NoError(t, err) - require.Equal(t, 0, len(m.blocks)) - require.Equal(t, newL1Tip, m.tip) - require.Equal(t, 1, len(m.pendingTransactions)) + require.NoError(m.processBlocks()) + require.NoError(m.pendingChannel.co.Flush()) + require.NoError(m.pendingChannel.OutputFrames()) + _, err := m.nextTxData() + require.NoError(err) + require.Len(m.blocks, 0) + require.Equal(newL1Tip, m.tip) + require.Len(m.pendingTransactions, 1) // Add a new block so we can test clearing // the channel manager with a full state @@ -222,20 +215,19 @@ func TestClearChannelManager(t *testing.T) { Number: big.NewInt(1), ParentHash: a.Hash(), }, nil, nil, nil, nil) - err = m.AddL2Block(b) - require.NoError(t, err) - require.Equal(t, 1, len(m.blocks)) - require.Equal(t, b.Hash(), m.tip) + require.NoError(m.AddL2Block(b)) + require.Len(m.blocks, 1) + require.Equal(b.Hash(), m.tip) // Clear the channel manager m.Clear() // Check that the entire channel manager state cleared - require.Empty(t, m.blocks) - require.Equal(t, common.Hash{}, m.tip) - require.Nil(t, m.pendingChannel) - require.Empty(t, m.pendingTransactions) - require.Empty(t, m.confirmedTransactions) + require.Empty(m.blocks) + require.Equal(common.Hash{}, m.tip) + require.Nil(m.pendingChannel) + require.Empty(m.pendingTransactions) + require.Empty(m.confirmedTransactions) } // TestChannelManagerTxConfirmed checks the [ChannelManager.TxConfirmed] function. @@ -251,8 +243,7 @@ func TestChannelManagerTxConfirmed(t *testing.T) { // Let's add a valid pending transaction to the channel manager // So we can demonstrate that TxConfirmed's correctness - err := m.ensurePendingChannel(eth.BlockID{}) - require.NoError(t, err) + require.NoError(t, m.ensurePendingChannel(eth.BlockID{})) channelID := m.pendingChannel.ID() frame := frameData{ data: []byte{}, @@ -270,7 +261,7 @@ func TestChannelManagerTxConfirmed(t *testing.T) { require.Equal(t, expectedTxData, returnedTxData) require.Equal(t, 0, m.pendingChannel.NumFrames()) require.Equal(t, expectedTxData, m.pendingTransactions[expectedChannelID]) - require.Equal(t, 1, len(m.pendingTransactions)) + require.Len(t, m.pendingTransactions, 1) // An unknown pending transaction should not be marked as confirmed // and should not be removed from the pending transactions map @@ -281,14 +272,14 @@ func TestChannelManagerTxConfirmed(t *testing.T) { blockID := eth.BlockID{Number: 0, Hash: common.Hash{0x69}} m.TxConfirmed(unknownTxID, blockID) require.Empty(t, m.confirmedTransactions) - require.Equal(t, 1, len(m.pendingTransactions)) + require.Len(t, m.pendingTransactions, 1) // Now let's mark the pending transaction as confirmed // and check that it is removed from the pending transactions map // and added to the confirmed transactions map m.TxConfirmed(expectedChannelID, blockID) require.Empty(t, m.pendingTransactions) - require.Equal(t, 1, len(m.confirmedTransactions)) + require.Len(t, m.confirmedTransactions, 1) require.Equal(t, blockID, m.confirmedTransactions[expectedChannelID]) } @@ -300,8 +291,7 @@ func TestChannelManagerTxFailed(t *testing.T) { // Let's add a valid pending transaction to the channel // manager so we can demonstrate correctness - err := m.ensurePendingChannel(eth.BlockID{}) - require.NoError(t, err) + require.NoError(t, m.ensurePendingChannel(eth.BlockID{})) channelID := m.pendingChannel.ID() frame := frameData{ data: []byte{}, @@ -319,7 +309,7 @@ func TestChannelManagerTxFailed(t *testing.T) { require.Equal(t, expectedTxData, returnedTxData) require.Equal(t, 0, m.pendingChannel.NumFrames()) require.Equal(t, expectedTxData, m.pendingTransactions[expectedChannelID]) - require.Equal(t, 1, len(m.pendingTransactions)) + require.Len(t, m.pendingTransactions, 1) // Trying to mark an unknown pending transaction as failed // shouldn't modify state @@ -348,8 +338,7 @@ func TestChannelManager_TxResend(t *testing.T) { a, _ := derivetest.RandomL2Block(rng, 4) - err := m.AddL2Block(a) - require.NoError(err) + require.NoError(m.AddL2Block(a)) txdata0, err := m.TxData(eth.BlockID{}) require.NoError(err) diff --git a/op-node/rollup/derive/channel_out.go b/op-node/rollup/derive/channel_out.go index f644aab68ff97..90614a81d687b 100644 --- a/op-node/rollup/derive/channel_out.go +++ b/op-node/rollup/derive/channel_out.go @@ -14,9 +14,17 @@ import ( "github.com/ethereum/go-ethereum/rlp" ) +var ErrMaxFrameSizeTooSmall = errors.New("maxSize is too small to fit the fixed frame overhead") var ErrNotDepositTx = errors.New("first transaction in block is not a deposit tx") var ErrTooManyRLPBytes = errors.New("batch would cause RLP bytes to go over limit") +// FrameV0OverHeadSize is the absolute minimum size of a frame. +// This is the fixed overhead frame size, calculated as specified +// in the [Frame Format] specs: 16 + 2 + 4 + 1 = 23 bytes. +// +// [Frame Format]: https://github.com/ethereum-optimism/optimism/blob/develop/specs/derivation.md#frame-format +const FrameV0OverHeadSize = 23 + type ChannelOut struct { id ChannelID // Frame ID of the next frame to emit. Increment after emitting @@ -141,19 +149,23 @@ func (co *ChannelOut) Close() error { // OutputFrame writes a frame to w with a given max size and returns the frame // number. // Use `ReadyBytes`, `Flush`, and `Close` to modify the ready buffer. -// Returns io.EOF when the channel is closed & there are no more frames +// Returns an error if the `maxSize` < FrameV0OverHeadSize. +// Returns io.EOF when the channel is closed & there are no more frames. // Returns nil if there is still more buffered data. -// Returns and error if it ran into an error during processing. +// Returns an error if it ran into an error during processing. func (co *ChannelOut) OutputFrame(w *bytes.Buffer, maxSize uint64) (uint16, error) { f := Frame{ ID: co.id, FrameNumber: uint16(co.frame), } + // Check that the maxSize is large enough for the frame overhead size. + if maxSize < FrameV0OverHeadSize { + return 0, ErrMaxFrameSizeTooSmall + } + // Copy data from the local buffer into the frame data buffer - // Don't go past the maxSize with the fixed frame overhead. - // Fixed overhead: 16 + 2 + 4 + 1 = 23 bytes. - maxDataSize := maxSize - 23 + maxDataSize := maxSize - FrameV0OverHeadSize if maxDataSize > uint64(co.buf.Len()) { maxDataSize = uint64(co.buf.Len()) // If we are closed & will not spill past the current frame diff --git a/op-node/rollup/derive/channel_out_test.go b/op-node/rollup/derive/channel_out_test.go index ea6eeac3670de..09d6f15422380 100644 --- a/op-node/rollup/derive/channel_out_test.go +++ b/op-node/rollup/derive/channel_out_test.go @@ -29,6 +29,22 @@ func TestChannelOutAddBlock(t *testing.T) { }) } +// TestOutputFrameSmallMaxSize tests that calling [OutputFrame] with a small +// max size that is below the fixed frame size overhead of 23, will return +// an error. +func TestOutputFrameSmallMaxSize(t *testing.T) { + cout, err := NewChannelOut() + require.NoError(t, err) + + // Call OutputFrame with the range of small max size values that err + var w bytes.Buffer + for i := 0; i < 23; i++ { + fid, err := cout.OutputFrame(&w, uint64(i)) + require.ErrorIs(t, err, ErrMaxFrameSizeTooSmall) + require.Zero(t, fid) + } +} + // TestRLPByteLimit ensures that stream encoder is properly limiting the length. // It will decode the input if `len(input) <= inputLimit`. func TestRLPByteLimit(t *testing.T) {