Merge pull request #4867 from ethereum-optimism/seb/fix-frame-parsing

op-node: Fix `Frame` parsing

op-node/rollup/derive/frame.go

@@ -68,42 +68,56 @@ type ByteReader interface {
 // UnmarshalBinary consumes a full frame from the reader.
 // If `r` fails a read, it returns the error from the reader
 // The reader will be left in a partially read state.
+//
+// If r doesn't return any bytes, returns io.EOF.
+// If r unexpectedly stops returning data half-way, returns io.ErrUnexpectedEOF.
 func (f *Frame) UnmarshalBinary(r ByteReader) error {
 	if _, err := io.ReadFull(r, f.ID[:]); err != nil {
-		return fmt.Errorf("error reading ID: %w", err)
+		// Forward io.EOF here ok, would mean not a single byte from r.
+		return fmt.Errorf("reading channel_id: %w", err)
 	}
 	if err := binary.Read(r, binary.BigEndian, &f.FrameNumber); err != nil {
-		return fmt.Errorf("error reading frame number: %w", err)
+		return fmt.Errorf("reading frame_number: %w", eofAsUnexpectedMissing(err))
 	}
 
 	var frameLength uint32
 	if err := binary.Read(r, binary.BigEndian, &frameLength); err != nil {
-		return fmt.Errorf("error reading frame length: %w", err)
+		return fmt.Errorf("reading frame_data_length: %w", eofAsUnexpectedMissing(err))
 	}
 
 	// Cap frame length to MaxFrameLen (currently 1MB)
 	if frameLength > MaxFrameLen {
-		return fmt.Errorf("frameLength is too large: %d", frameLength)
+		return fmt.Errorf("frame_data_length is too large: %d", frameLength)
 	}
 	f.Data = make([]byte, int(frameLength))
 	if _, err := io.ReadFull(r, f.Data); err != nil {
-		return fmt.Errorf("error reading frame data: %w", err)
+		return fmt.Errorf("reading frame_data: %w", eofAsUnexpectedMissing(err))
 	}
 
-	if isLastByte, err := r.ReadByte(); err != nil && err != io.EOF {
-		return fmt.Errorf("error reading final byte: %w", err)
+	if isLastByte, err := r.ReadByte(); err != nil {
+		return fmt.Errorf("reading final byte (is_last): %w", eofAsUnexpectedMissing(err))
 	} else if isLastByte == 0 {
 		f.IsLast = false
-		return err
 	} else if isLastByte == 1 {
 		f.IsLast = true
-		return err
 	} else {
 		return errors.New("invalid byte as is_last")
 	}
+	return nil
+}
+
+// eofAsUnexpectedMissing converts an io.EOF in the error chain of err into an
+// io.ErrUnexpectedEOF. It should be used to convert intermediate io.EOF errors
+// in unmarshaling code to achieve idiomatic error behavior.
+// Other errors are passed through unchanged.
+func eofAsUnexpectedMissing(err error) error {
+	if errors.Is(err, io.EOF) {
+		return fmt.Errorf("fully missing: %w", io.ErrUnexpectedEOF)
+	}
+	return err
 }
 
-// Frames on stored in L1 transactions with the following format:
+// Frames are stored in L1 transactions with the following format:
 // data = DerivationVersion0 ++ Frame(s)
 // Where there is one or more frames concatenated together.
 
@@ -123,8 +137,8 @@ func ParseFrames(data []byte) ([]Frame, error) {
 	var frames []Frame
 	for buf.Len() > 0 {
 		var f Frame
-		if err := (&f).UnmarshalBinary(buf); err != io.EOF && err != nil {
-			return nil, err
+		if err := f.UnmarshalBinary(buf); err != nil {
+			return nil, fmt.Errorf("parsing frame %d: %w", len(frames), err)
 		}
 		frames = append(frames, f)
 	}

op-node/rollup/derive/frame_test.go

@@ -2,7 +2,15 @@ package derive
 
 import (
 	"bytes"
+	"io"
+	"math"
+	"math/rand"
+	"strconv"
 	"testing"
+	"time"
+
+	"github.com/ethereum-optimism/optimism/op-node/testutils"
+	"github.com/stretchr/testify/require"
 )
 
 func FuzzFrameUnmarshalBinary(f *testing.F) {
@@ -23,3 +31,217 @@ func FuzzParseFrames(f *testing.F) {
 		}
 	})
 }
+
+func TestFrameMarshaling(t *testing.T) {
+	rng := rand.New(rand.NewSource(time.Now().UnixNano()))
+	for i := 0; i < 16; i++ {
+		t.Run(strconv.Itoa(i), func(t *testing.T) {
+			frame := randomFrame(rng)
+			var data bytes.Buffer
+			require.NoError(t, frame.MarshalBinary(&data))
+
+			frame0 := new(Frame)
+			require.NoError(t, frame0.UnmarshalBinary(&data))
+			require.Equal(t, frame, frame0)
+		})
+	}
+}
+
+func TestFrameUnmarshalNoData(t *testing.T) {
+	frame0 := new(Frame)
+	err := frame0.UnmarshalBinary(bytes.NewReader([]byte{}))
+	require.Error(t, err)
+	require.ErrorIs(t, err, io.EOF)
+}
+
+func TestFrameUnmarshalTruncated(t *testing.T) {
+	rng := rand.New(rand.NewSource(time.Now().UnixNano()))
+
+	// 16 (channel_id) ++ 2 (frame_number) ++ 4 (frame_data_length) ++
+	// frame_data_length (frame_data) ++ 1 (is_last)
+	for _, tr := range []struct {
+		desc     string
+		truncate func([]byte) []byte
+		genData  bool // whether data should be generated
+	}{
+		{
+			desc: "truncate-channel_id-half",
+			truncate: func(data []byte) []byte {
+				return data[:8]
+			},
+		},
+		{
+			desc: "truncate-frame_number-full",
+			truncate: func(data []byte) []byte {
+				return data[:16]
+			},
+		},
+		{
+			desc: "truncate-frame_number-half",
+			truncate: func(data []byte) []byte {
+				return data[:17]
+			},
+		},
+		{
+			desc: "truncate-frame_data_length-full",
+			truncate: func(data []byte) []byte {
+				return data[:18]
+			},
+		},
+		{
+			desc: "truncate-frame_data_length-half",
+			truncate: func(data []byte) []byte {
+				return data[:20]
+			},
+			genData: true, // for non-zero frame_data_length
+		},
+		{
+			desc: "truncate-frame_data-full",
+			truncate: func(data []byte) []byte {
+				return data[:22]
+			},
+			genData: true, // for non-zero frame_data_length
+		},
+		{
+			desc: "truncate-frame_data-last-byte",
+			truncate: func(data []byte) []byte {
+				return data[:len(data)-2]
+			},
+			genData: true,
+		},
+		{
+			desc: "truncate-is_last",
+			truncate: func(data []byte) []byte {
+				return data[:len(data)-1]
+			},
+			genData: true,
+		},
+	} {
+		t.Run(tr.desc, func(t *testing.T) {
+			var opts []frameOpt
+			if !tr.genData {
+				opts = []frameOpt{frameWithDataLen(0)}
+			}
+			frame := randomFrame(rng, opts...)
+			var data bytes.Buffer
+			require.NoError(t, frame.MarshalBinary(&data))
+
+			tdata := tr.truncate(data.Bytes())
+
+			frame0 := new(Frame)
+			err := frame0.UnmarshalBinary(bytes.NewReader(tdata))
+			require.Error(t, err)
+			require.ErrorIs(t, err, io.ErrUnexpectedEOF)
+		})
+	}
+}
+
+func TestFrameUnmarshalInvalidIsLast(t *testing.T) {
+	rng := rand.New(rand.NewSource(time.Now().UnixNano()))
+	frame := randomFrame(rng, frameWithDataLen(16))
+	var data bytes.Buffer
+	require.NoError(t, frame.MarshalBinary(&data))
+
+	idata := data.Bytes()
+	idata[len(idata)-1] = 2 // invalid is_last
+
+	frame0 := new(Frame)
+	err := frame0.UnmarshalBinary(bytes.NewReader(idata))
+	require.Error(t, err)
+	require.ErrorContains(t, err, "invalid byte")
+}
+
+func TestParseFramesNoData(t *testing.T) {
+	frames, err := ParseFrames(nil)
+	require.Empty(t, frames)
+	require.Error(t, err)
+}
+
+func TestParseFramesInvalidVer(t *testing.T) {
+	frames, err := ParseFrames([]byte{42})
+	require.Empty(t, frames)
+	require.Error(t, err)
+}
+
+func TestParseFrames(t *testing.T) {
+	rng := rand.New(rand.NewSource(time.Now().UnixNano()))
+	numFrames := rng.Intn(16) + 1
+	frames := make([]Frame, 0, numFrames)
+	for i := 0; i < numFrames; i++ {
+		frames = append(frames, *randomFrame(rng))
+	}
+	data, err := txMarshalFrames(frames)
+	require.NoError(t, err)
+
+	frames0, err := ParseFrames(data)
+	require.NoError(t, err)
+	require.Equal(t, frames, frames0)
+}
+
+func TestParseFramesTruncated(t *testing.T) {
+	rng := rand.New(rand.NewSource(time.Now().UnixNano()))
+	numFrames := rng.Intn(16) + 1
+	frames := make([]Frame, 0, numFrames)
+	for i := 0; i < numFrames; i++ {
+		frames = append(frames, *randomFrame(rng))
+	}
+	data, err := txMarshalFrames(frames)
+	require.NoError(t, err)
+	data = data[:len(data)-2] // truncate last 2 bytes
+
+	frames0, err := ParseFrames(data)
+	require.Error(t, err)
+	require.ErrorIs(t, err, io.ErrUnexpectedEOF)
+	require.Empty(t, frames0)
+}
+
+// txMarshalFrames creates the tx payload for the given frames, i.e., it first
+// writes the version byte to a buffer and then appends all binary-marshaled
+// frames.
+func txMarshalFrames(frames []Frame) ([]byte, error) {
+	var data bytes.Buffer
+	if err := data.WriteByte(DerivationVersion0); err != nil {
+		return nil, err
+	}
+	for _, frame := range frames {
+		if err := frame.MarshalBinary(&data); err != nil {
+			return nil, err
+		}
+	}
+	return data.Bytes(), nil
+}
+
+func randomFrame(rng *rand.Rand, opts ...frameOpt) *Frame {
+	var id ChannelID
+	_, err := rng.Read(id[:])
+	if err != nil {
+		panic(err)
+	}
+
+	frame := &Frame{
+		ID:          id,
+		FrameNumber: uint16(rng.Int31n(math.MaxUint16 + 1)),
+		IsLast:      testutils.RandomBool(rng),
+	}
+
+	// evaulaute options
+	for _, opt := range opts {
+		opt(rng, frame)
+	}
+
+	// default if no option set field
+	if frame.Data == nil {
+		datalen := int(rng.Intn(MaxFrameLen + 1))
+		frame.Data = testutils.RandomData(rng, datalen)
+	}
+
+	return frame
+}
+
+type frameOpt func(*rand.Rand, *Frame)
+
+func frameWithDataLen(l int) frameOpt {
+	return func(rng *rand.Rand, frame *Frame) {
+		frame.Data = testutils.RandomData(rng, l)
+	}
+}

op-node/testutils/random.go

@@ -14,6 +14,13 @@ import (
 	"github.com/ethereum/go-ethereum/crypto"
 )
 
+func RandomBool(rng *rand.Rand) bool {
+	if b := rng.Intn(2); b == 0 {
+		return false
+	}
+	return true
+}
+
 func RandomHash(rng *rand.Rand) (out common.Hash) {
 	rng.Read(out[:])
 	return