PR Comments

Author: axelKingsley

op-batcher/batcher/channel_builder.go

@@ -87,14 +87,11 @@ func NewChannelBuilder(cfg ChannelConfig, rollupCfg rollup.Config, latestL1Origi
 	var co derive.ChannelOut
 	if cfg.BatchType == derive.SpanBatchType {
 		co, err = derive.NewSpanChannelOut(rollupCfg.Genesis.L2Time, rollupCfg.L2ChainID, cfg.CompressorConfig.TargetOutputSize)
-		if err != nil {
-			return nil, err
-		}
 	} else {
 		co, err = derive.NewSingularChannelOut(c)
-		if err != nil {
-			return nil, err
-		}
+	}
+	if err != nil {
+		return nil, fmt.Errorf("creating channel out: %w", err)
 	}
 
 	cb := &ChannelBuilder{
@@ -159,7 +156,7 @@ func (c *ChannelBuilder) AddBlock(block *types.Block) (*derive.L1BlockInfo, erro
 		return l1info, fmt.Errorf("converting block to batch: %w", err)
 	}
 
-	if _, err = c.co.AddSingularBatch(batch, l1info.SequenceNumber); errors.Is(err, derive.ErrTooManyRLPBytes) || errors.Is(err, derive.ErrCompressorFull) {
+	if err = c.co.AddSingularBatch(batch, l1info.SequenceNumber); errors.Is(err, derive.ErrTooManyRLPBytes) || errors.Is(err, derive.ErrCompressorFull) {
 		c.setFullErr(err)
 		return l1info, c.FullErr()
 	} else if err != nil {

op-batcher/batcher/channel_builder_test.go

@@ -365,11 +365,10 @@ func ChannelBuilder_OutputWrongFramePanic(t *testing.T, batchType uint) {
 
 	// Mock the internals of `ChannelBuilder.outputFrame`
 	// to construct a single frame
+	// the type of batch does not matter here because we are using it to construct a broken frame
 	c, err := channelConfig.CompressorConfig.NewCompressor()
 	require.NoError(t, err)
-	// co is only created to supply a random ID
 	co, err := derive.NewSingularChannelOut(c)
-
 	require.NoError(t, err)
 	var buf bytes.Buffer
 	fn, err := co.OutputFrame(&buf, channelConfig.MaxFrameSize)

op-e2e/actions/garbage_channel_out.go

@@ -54,7 +54,7 @@ type Writer interface {
 type ChannelOutIface interface {
 	ID() derive.ChannelID
 	Reset() error
-	AddBlock(rollupCfg *rollup.Config, block *types.Block) (uint64, error)
+	AddBlock(rollupCfg *rollup.Config, block *types.Block) error
 	ReadyBytes() int
 	Flush() error
 	Close() error
@@ -138,19 +138,19 @@ func (co *GarbageChannelOut) Reset() error {
 // error that it returns is ErrTooManyRLPBytes. If this error
 // is returned, the channel should be closed and a new one
 // should be made.
-func (co *GarbageChannelOut) AddBlock(rollupCfg *rollup.Config, block *types.Block) (uint64, error) {
+func (co *GarbageChannelOut) AddBlock(rollupCfg *rollup.Config, block *types.Block) error {
 	if co.closed {
-		return 0, errors.New("already closed")
+		return errors.New("already closed")
 	}
 	batch, err := blockToBatch(rollupCfg, block)
 	if err != nil {
-		return 0, err
+		return err
 	}
 	// We encode to a temporary buffer to determine the encoded length to
 	// ensure that the total size of all RLP elements is less than or equal to MAX_RLP_BYTES_PER_CHANNEL
 	var buf bytes.Buffer
 	if err := rlp.Encode(&buf, batch); err != nil {
-		return 0, err
+		return err
 	}
 	if co.cfg.malformRLP {
 		// Malform the RLP by incrementing the length prefix by 1.
@@ -160,13 +160,13 @@ func (co *GarbageChannelOut) AddBlock(rollupCfg *rollup.Config, block *types.Blo
 		buf.Write(bufBytes)
 	}
 	if co.rlpLength+buf.Len() > derive.MaxRLPBytesPerChannel {
-		return 0, fmt.Errorf("could not add %d bytes to channel of %d bytes, max is %d. err: %w",
+		return fmt.Errorf("could not add %d bytes to channel of %d bytes, max is %d. err: %w",
 			buf.Len(), co.rlpLength, derive.MaxRLPBytesPerChannel, derive.ErrTooManyRLPBytes)
 	}
 	co.rlpLength += buf.Len()
 
-	written, err := io.Copy(co.compress, &buf)
-	return uint64(written), err
+	_, err = io.Copy(co.compress, &buf)
+	return err
 }
 
 // ReadyBytes returns the number of bytes that the channel out can immediately output into a frame.

op-e2e/actions/l2_batcher.go

@@ -197,21 +197,20 @@ func (s *L2Batcher) Buffer(t Testing) error {
 
 			if s.l2BatcherCfg.ForceSubmitSingularBatch && s.l2BatcherCfg.ForceSubmitSpanBatch {
 				t.Fatalf("ForceSubmitSingularBatch and ForceSubmitSpanBatch cannot be set to true at the same time")
-			} else if s.l2BatcherCfg.ForceSubmitSingularBatch {
-				// use SingularBatchType
-				ch, err = derive.NewSingularChannelOut(c)
-			} else if s.l2BatcherCfg.ForceSubmitSpanBatch || s.rollupCfg.IsDelta(block.Time()) {
-				// If both ForceSubmitSingularBatch and ForceSubmitSpanbatch are false, use SpanBatch automatically if Delta HF is activated.
-				ch, err = derive.NewSpanChannelOut(s.rollupCfg.Genesis.L2Time, s.rollupCfg.L2ChainID, target)
 			} else {
-				// default to SingularBatchType if no other conditions are met
-				ch, err = derive.NewSingularChannelOut(c)
+				// use span batch if we're forcing it or if we're at/beyond delta
+				if s.l2BatcherCfg.ForceSubmitSpanBatch || s.rollupCfg.IsDelta(block.Time()) {
+					ch, err = derive.NewSpanChannelOut(s.rollupCfg.Genesis.L2Time, s.rollupCfg.L2ChainID, target)
+					// use singular batches in all other cases
+				} else {
+					ch, err = derive.NewSingularChannelOut(c)
+				}
 			}
 		}
 		require.NoError(t, err, "failed to create channel")
 		s.l2ChannelOut = ch
 	}
-	if _, err := s.l2ChannelOut.AddBlock(s.rollupCfg, block); err != nil {
+	if err := s.l2ChannelOut.AddBlock(s.rollupCfg, block); err != nil {
 		return err
 	}
 	ref, err := s.engCl.L2BlockRefByHash(t.Ctx(), block.Hash())

op-e2e/actions/sync_test.go

@@ -243,7 +243,7 @@ func TestBackupUnsafe(gt *testing.T) {
 			block = block.WithBody([]*types.Transaction{block.Transactions()[0], invalidTx}, []*types.Header{})
 		}
 		// Add A1, B2, B3, B4, B5 into the channel
-		_, err = channelOut.AddBlock(sd.RollupCfg, block)
+		err = channelOut.AddBlock(sd.RollupCfg, block)
 		require.NoError(t, err)
 	}
 
@@ -406,7 +406,7 @@ func TestBackupUnsafeReorgForkChoiceInputError(gt *testing.T) {
 			block = block.WithBody([]*types.Transaction{block.Transactions()[0], invalidTx}, []*types.Header{})
 		}
 		// Add A1, B2, B3, B4, B5 into the channel
-		_, err = channelOut.AddBlock(sd.RollupCfg, block)
+		err = channelOut.AddBlock(sd.RollupCfg, block)
 		require.NoError(t, err)
 	}
 
@@ -545,7 +545,7 @@ func TestBackupUnsafeReorgForkChoiceNotInputError(gt *testing.T) {
 			block = block.WithBody([]*types.Transaction{block.Transactions()[0], invalidTx}, []*types.Header{})
 		}
 		// Add A1, B2, B3, B4, B5 into the channel
-		_, err = channelOut.AddBlock(sd.RollupCfg, block)
+		err = channelOut.AddBlock(sd.RollupCfg, block)
 		require.NoError(t, err)
 	}
 
@@ -864,7 +864,7 @@ func TestInvalidPayloadInSpanBatch(gt *testing.T) {
 			block = block.WithBody([]*types.Transaction{block.Transactions()[0], invalidTx}, []*types.Header{})
 		}
 		// Add A1 ~ A12 into the channel
-		_, err = channelOut.AddBlock(sd.RollupCfg, block)
+		err = channelOut.AddBlock(sd.RollupCfg, block)
 		require.NoError(t, err)
 	}
 
@@ -913,7 +913,7 @@ func TestInvalidPayloadInSpanBatch(gt *testing.T) {
 			block = block.WithBody([]*types.Transaction{block.Transactions()[0], tx}, []*types.Header{})
 		}
 		// Add B1, A2 ~ A12 into the channel
-		_, err = channelOut.AddBlock(sd.RollupCfg, block)
+		err = channelOut.AddBlock(sd.RollupCfg, block)
 		require.NoError(t, err)
 	}
 	// Submit span batch(B1, A2, ... A12)

op-node/benchmarks/batchbuilding_test.go

@@ -12,12 +12,14 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-var (
+const (
 	// a really large target output size to ensure that the compressors are never full
 	targetOutput_huge = uint64(100_000_000_000)
 	// this target size was determiend by the devnet sepolia batcher's configuration
 	targetOuput_real = uint64(780120)
+)
 
+var (
 	// compressors used in the benchmark
 	rc, _ = compressor.NewRatioCompressor(compressor.Config{
 		TargetOutputSize: targetOutput_huge,
@@ -136,13 +138,13 @@ func BenchmarkFinalBatchChannelOut(b *testing.B) {
 				cout, _ := channelOutByType(tc.BatchType, tc.compKey)
 				// add all but the final batch to the channel out
 				for i := 0; i < tc.BatchCount-1; i++ {
-					_, err := cout.AddSingularBatch(batches[i], 0)
+					err := cout.AddSingularBatch(batches[i], 0)
 					require.NoError(b, err)
 				}
 				// measure the time to add the final batch
 				b.StartTimer()
 				// add the final batch to the channel out
-				_, err := cout.AddSingularBatch(batches[tc.BatchCount-1], 0)
+				err := cout.AddSingularBatch(batches[tc.BatchCount-1], 0)
 				require.NoError(b, err)
 			}
 		})
@@ -185,7 +187,7 @@ func BenchmarkIncremental(b *testing.B) {
 				}
 				b.StartTimer()
 				for i := 0; i < tc.BatchCount; i++ {
-					_, err := cout.AddSingularBatch(batches[i], 0)
+					err := cout.AddSingularBatch(batches[i], 0)
 					if err != nil {
 						done = true
 						return
@@ -246,7 +248,7 @@ func BenchmarkAllBatchesChannelOut(b *testing.B) {
 				b.StartTimer()
 				// add all batches to the channel out
 				for i := 0; i < tc.BatchCount; i++ {
-					_, err := cout.AddSingularBatch(batches[i], 0)
+					err := cout.AddSingularBatch(batches[i], 0)
 					require.NoError(b, err)
 				}
 			}

op-node/rollup/derive/channel_out.go

@@ -53,8 +53,8 @@ type Compressor interface {
 type ChannelOut interface {
 	ID() ChannelID
 	Reset() error
-	AddBlock(*rollup.Config, *types.Block) (uint64, error)
-	AddSingularBatch(*SingularBatch, uint64) (uint64, error)
+	AddBlock(*rollup.Config, *types.Block) error
+	AddSingularBatch(*SingularBatch, uint64) error
 	InputBytes() int
 	ReadyBytes() int
 	Flush() error
@@ -108,14 +108,14 @@ func (co *SingularChannelOut) Reset() error {
 // and an error if there is a problem adding the block. The only sentinel error
 // that it returns is ErrTooManyRLPBytes. If this error is returned, the channel
 // should be closed and a new one should be made.
-func (co *SingularChannelOut) AddBlock(rollupCfg *rollup.Config, block *types.Block) (uint64, error) {
+func (co *SingularChannelOut) AddBlock(rollupCfg *rollup.Config, block *types.Block) error {
 	if co.closed {
-		return 0, ErrChannelOutAlreadyClosed
+		return ErrChannelOutAlreadyClosed
 	}
 
 	batch, l1Info, err := BlockToSingularBatch(rollupCfg, block)
 	if err != nil {
-		return 0, err
+		return err
 	}
 	return co.AddSingularBatch(batch, l1Info.SequenceNumber)
 }
@@ -128,26 +128,26 @@ func (co *SingularChannelOut) AddBlock(rollupCfg *rollup.Config, block *types.Bl
 // AddSingularBatch should be used together with BlockToBatch if you need to access the
 // BatchData before adding a block to the channel. It isn't possible to access
 // the batch data with AddBlock.
-func (co *SingularChannelOut) AddSingularBatch(batch *SingularBatch, _ uint64) (uint64, error) {
+func (co *SingularChannelOut) AddSingularBatch(batch *SingularBatch, _ uint64) error {
 	if co.closed {
-		return 0, ErrChannelOutAlreadyClosed
+		return ErrChannelOutAlreadyClosed
 	}
 
 	// We encode to a temporary buffer to determine the encoded length to
 	// ensure that the total size of all RLP elements is less than or equal to MAX_RLP_BYTES_PER_CHANNEL
 	var buf bytes.Buffer
 	if err := rlp.Encode(&buf, NewBatchData(batch)); err != nil {
-		return 0, err
+		return err
 	}
 	if co.rlpLength+buf.Len() > MaxRLPBytesPerChannel {
-		return 0, fmt.Errorf("could not add %d bytes to channel of %d bytes, max is %d. err: %w",
+		return fmt.Errorf("could not add %d bytes to channel of %d bytes, max is %d. err: %w",
 			buf.Len(), co.rlpLength, MaxRLPBytesPerChannel, ErrTooManyRLPBytes)
 	}
 	co.rlpLength += buf.Len()
 
 	// avoid using io.Copy here, because we need all or nothing
-	written, err := co.compress.Write(buf.Bytes())
-	return uint64(written), err
+	_, err := co.compress.Write(buf.Bytes())
+	return err
 }
 
 // InputBytes returns the total amount of RLP-encoded input bytes.