Merge pull request #5187 from ethereum-optimism/refcell/fix/channelo

fix(op-node): Fix Channel Out MaxSize Underflow

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))

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)

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

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) {