refactor(core/types): simplify Body RLP override (#120)

## Why this should be merged

Simplification of `types.Body` RLP overriding, resulting in reduced code
at both the implementation and consumer ends.

## How this works

Introduction of `rlp.Fields` type, to mirror regular RLP encoding of a
struct. The RLP override hook now only needs to return the fields of
interest, which MAY come from either the `Body` or the registered extra.

This pattern allows for arbitrary modification of upstream fields via
(1) reordering; (2) addition; (3) deletion; and (4) inverting required
vs optional status. While less important for `Body`, this allows for
complete support of `ava-labs/coreth` `Header` modifications, which make
use of 1-3.

## How this was tested

Existing backwards-compatibility tests + new unit tests for introduced
functionality.

---------

Signed-off-by: Arran Schlosberg <519948+ARR4N@users.noreply.github.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@avalabs.org>

core/types/block.libevm.go

@@ -112,79 +112,26 @@ func (*NOOPHeaderHooks) DecodeRLP(h *Header, s *rlp.Stream) error {
 }
 func (*NOOPHeaderHooks) PostCopy(dst *Header) {}
 
-var (
-	_ interface {
-		rlp.Encoder
-		rlp.Decoder
-	} = (*Body)(nil)
-
-	// The implementations of [Body.EncodeRLP] and [Body.DecodeRLP] make
-	// assumptions about the struct fields and their order, which we lock in here as a change
-	// detector. If this breaks then it MUST be updated and the RLP methods
-	// reviewed + new backwards-compatibility tests added.
-	_ = &Body{[]*Transaction{}, []*Header{}, []*Withdrawal{}}
-)
+var _ interface {
+	rlp.Encoder
+	rlp.Decoder
+} = (*Body)(nil)
 
 // EncodeRLP implements the [rlp.Encoder] interface.
-func (b *Body) EncodeRLP(dst io.Writer) error {
-	w := rlp.NewEncoderBuffer(dst)
-
-	return w.InList(func() error {
-		if err := rlp.EncodeListToBuffer(w, b.Transactions); err != nil {
-			return err
-		}
-		if err := rlp.EncodeListToBuffer(w, b.Uncles); err != nil {
-			return err
-		}
-
-		hasLaterOptionalField := b.Withdrawals != nil
-		if err := b.hooks().AppendRLPFields(w, hasLaterOptionalField); err != nil {
-			return err
-		}
-		if !hasLaterOptionalField {
-			return nil
-		}
-		return rlp.EncodeListToBuffer(w, b.Withdrawals)
-	})
+func (b *Body) EncodeRLP(w io.Writer) error {
+	return b.hooks().RLPFieldsForEncoding(b).EncodeRLP(w)
 }
 
 // DecodeRLP implements the [rlp.Decoder] interface.
 func (b *Body) DecodeRLP(s *rlp.Stream) error {
-	return s.FromList(func() error {
-		txs, err := rlp.DecodeList[Transaction](s)
-		if err != nil {
-			return err
-		}
-		uncles, err := rlp.DecodeList[Header](s)
-		if err != nil {
-			return err
-		}
-		*b = Body{
-			Transactions: txs,
-			Uncles:       uncles,
-		}
-
-		if err := b.hooks().DecodeExtraRLPFields(s); err != nil {
-			return err
-		}
-		if !s.MoreDataInList() {
-			return nil
-		}
-
-		ws, err := rlp.DecodeList[Withdrawal](s)
-		if err != nil {
-			return err
-		}
-		b.Withdrawals = ws
-		return nil
-	})
+	return b.hooks().RLPFieldPointersForDecoding(b).DecodeRLP(s)
 }
 
 // BodyHooks are required for all types registered with [RegisterExtras] for
 // [Body] payloads.
 type BodyHooks interface {
-	AppendRLPFields(_ rlp.EncoderBuffer, mustWriteEmptyOptional bool) error
-	DecodeExtraRLPFields(*rlp.Stream) error
+	RLPFieldsForEncoding(*Body) *rlp.Fields
+	RLPFieldPointersForDecoding(*Body) *rlp.Fields
 }
 
 // TestOnlyRegisterBodyHooks is a temporary means of "registering" BodyHooks for
@@ -209,5 +156,22 @@ func (b *Body) hooks() BodyHooks {
 // having been registered.
 type NOOPBodyHooks struct{}
 
-func (NOOPBodyHooks) AppendRLPFields(rlp.EncoderBuffer, bool) error { return nil }
-func (NOOPBodyHooks) DecodeExtraRLPFields(*rlp.Stream) error        { return nil }
+// The RLP-related methods of [NOOPBodyHooks] make assumptions about the struct
+// fields and their order, which we lock in here as a change detector. If this
+// breaks then it MUST be updated and the RLP methods reviewed + new
+// backwards-compatibility tests added.
+var _ = &Body{[]*Transaction{}, []*Header{}, []*Withdrawal{}}
+
+func (NOOPBodyHooks) RLPFieldsForEncoding(b *Body) *rlp.Fields {
+	return &rlp.Fields{
+		Required: []any{b.Transactions, b.Uncles},
+		Optional: []any{b.Withdrawals},
+	}
+}
+
+func (NOOPBodyHooks) RLPFieldPointersForDecoding(b *Body) *rlp.Fields {
+	return &rlp.Fields{
+		Required: []any{&b.Transactions, &b.Uncles},
+		Optional: []any{&b.Withdrawals},
+	}
+}

core/types/rlp_backwards_compat.libevm_test.go

@@ -116,8 +116,8 @@ func TestBodyRLPBackwardsCompatibility(t *testing.T) {
 	newHdr := func(hashLow byte) *Header { return &Header{ParentHash: common.Hash{hashLow}} }
 	newWithdraw := func(idx uint64) *Withdrawal { return &Withdrawal{Index: idx} }
 
-	// We build up test-case [Body] instances from the power set of each of
-	// these components.
+	// We build up test-case [Body] instances from the Cartesian product of each
+	// of these components.
 	txMatrix := [][]*Transaction{
 		nil, {}, // Must be equivalent for non-optional field
 		{newTx(1)},
@@ -197,35 +197,33 @@ type cChainBodyExtras struct {
 
 var _ BodyHooks = (*cChainBodyExtras)(nil)
 
-func (e *cChainBodyExtras) AppendRLPFields(b rlp.EncoderBuffer, _ bool) error {
-	b.WriteUint64(uint64(e.Version))
-
-	var data []byte
-	if e.ExtData != nil {
-		data = *e.ExtData
+func (e *cChainBodyExtras) RLPFieldsForEncoding(b *Body) *rlp.Fields {
+	// The Avalanche C-Chain uses all of the geth required fields (but none of
+	// the optional ones) so there's no need to explicitly list them. This
+	// pattern might not be ideal for readability but is used here for
+	// demonstrative purposes.
+	//
+	// All new fields will always be tagged as optional for backwards
+	// compatibility so this is safe to do, but only for the required fields.
+	return &rlp.Fields{
+		Required: append(
+			NOOPBodyHooks{}.RLPFieldsForEncoding(b).Required,
+			e.Version, e.ExtData,
+		),
 	}
-	b.WriteBytes(data)
-
-	return nil
 }
 
-func (e *cChainBodyExtras) DecodeExtraRLPFields(s *rlp.Stream) error {
-	if err := s.Decode(&e.Version); err != nil {
-		return err
+func (e *cChainBodyExtras) RLPFieldPointersForDecoding(b *Body) *rlp.Fields {
+	// An alternative to the pattern used above is to explicitly list all
+	// fields for better introspection.
+	return &rlp.Fields{
+		Required: []any{
+			&b.Transactions,
+			&b.Uncles,
+			&e.Version,
+			rlp.Nillable(&e.ExtData), // equivalent to `rlp:"nil"`
+		},
 	}
-
-	buf, err := s.Bytes()
-	if err != nil {
-		return err
-	}
-	if len(buf) > 0 {
-		e.ExtData = &buf
-	} else {
-		// Respect the `rlp:"nil"` field tag.
-		e.ExtData = nil
-	}
-
-	return nil
 }
 
 func TestBodyRLPCChainCompat(t *testing.T) {
@@ -256,12 +254,14 @@ func TestBodyRLPCChainCompat(t *testing.T) {
 		wantRLPHex string
 	}{
 		{
+			name: "nil_ExtData",
 			extra: &cChainBodyExtras{
 				Version: version,
 			},
 			wantRLPHex: `e5dedd2a80809400000000000000000000000000decafc0ffeebad8080808080c08304cb2f80`,
 		},
 		{
+			name: "non-nil_ExtData",
 			extra: &cChainBodyExtras{
 				Version: version,
 				ExtData: &[]byte{1, 4, 2, 8, 5, 7},

rlp/fields.libevm.go

@@ -0,0 +1,139 @@
+// Copyright 2025 the libevm authors.
+//
+// The libevm additions to go-ethereum are free software: you can redistribute
+// them and/or modify them under the terms of the GNU Lesser General Public License
+// as published by the Free Software Foundation, either version 3 of the License,
+// or (at your option) any later version.
+//
+// The libevm additions are distributed in the hope that they will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser
+// General Public License for more details.
+//
+// You should have received a copy of the GNU Lesser General Public License
+// along with the go-ethereum library. If not, see
+// <http://www.gnu.org/licenses/>.
+
+package rlp
+
+import (
+	"errors"
+	"fmt"
+	"io"
+	"reflect"
+)
+
+// Fields mirror the RLP encoding of struct fields.
+type Fields struct {
+	Required []any
+	Optional []any // equivalent to those tagged with `rlp:"optional"`
+}
+
+var _ interface {
+	Encoder
+	Decoder
+} = (*Fields)(nil)
+
+// EncodeRLP encodes the `f.Required` and `f.Optional` slices to `w`,
+// concatenated as a single list, as if they were fields in a struct. The
+// optional values are treated identically to those tagged with
+// `rlp:"optional"`.
+func (f *Fields) EncodeRLP(w io.Writer) error {
+	includeOptional, err := f.optionalInclusionFlags()
+	if err != nil {
+		return err
+	}
+
+	b := NewEncoderBuffer(w)
+	err = b.InList(func() error {
+		for _, v := range f.Required {
+			if err := Encode(b, v); err != nil {
+				return err
+			}
+		}
+
+		for i, v := range f.Optional {
+			if !includeOptional[i] {
+				return nil
+			}
+			if err := Encode(b, v); err != nil {
+				return err
+			}
+		}
+		return nil
+	})
+	if err != nil {
+		return err
+	}
+	return b.Flush()
+}
+
+var errUnsupportedOptionalFieldType = errors.New("unsupported optional field type")
+
+// optionalInclusionFlags returns a slice of booleans, the same length as
+// `f.Optional`, indicating whether or not the respective field MUST be written
+// to a list. A field must be written if it or any later field value is non-nil;
+// the returned slice is therefore monotonic non-increasing from true to false.
+func (f *Fields) optionalInclusionFlags() ([]bool, error) {
+	flags := make([]bool, len(f.Optional))
+	var include bool
+	for i := len(f.Optional) - 1; i >= 0; i-- {
+		switch v := reflect.ValueOf(f.Optional[i]); v.Kind() {
+		case reflect.Slice, reflect.Pointer:
+			include = include || !v.IsNil()
+		default:
+			return nil, fmt.Errorf("%w: %T", errUnsupportedOptionalFieldType, f.Optional[i])
+		}
+		flags[i] = include
+	}
+	return flags, nil
+}
+
+// DecodeRLP implements the [Decoder] interface. All destination fields, be they
+// required or optional, MUST be pointers and all optional fields MUST be
+// provided in case they are present in the RLP being decoded.
+//
+// Typically, the arguments to this method mirror those passed to
+// [Fields.EncodeRLP] except for being pointers. See the example.
+func (f *Fields) DecodeRLP(s *Stream) error {
+	return s.FromList(func() error {
+		for _, v := range f.Required {
+			if err := s.Decode(v); err != nil {
+				return err
+			}
+		}
+
+		for _, v := range f.Optional {
+			if !s.MoreDataInList() {
+				return nil
+			}
+			if err := s.Decode(v); err != nil {
+				return err
+			}
+		}
+		return nil
+	})
+}
+
+// Nillable wraps `field` to mirror the behaviour of an `rlp:"nil"` tag; i.e. if
+// a zero-sized RLP item is decoded into the returned Decoder then it is dropped
+// and `*field` is set to nil, otherwise the RLP item is decoded directly into
+// `field`. The return argument is intended for use with [Fields].
+func Nillable[T any](field **T) Decoder {
+	return &nillable[T]{field}
+}
+
+type nillable[T any] struct{ v **T }
+
+func (n *nillable[T]) DecodeRLP(s *Stream) error {
+	_, size, err := s.Kind()
+	if err != nil {
+		return err
+	}
+	if size > 0 {
+		return s.Decode(n.v)
+	}
+	*n.v = nil
+	_, err = s.Raw() // consume the item
+	return err
+}

rlp/fields.libevm_test.go

@@ -0,0 +1,218 @@
+// Copyright 2025 the libevm authors.
+//
+// The libevm additions to go-ethereum are free software: you can redistribute
+// them and/or modify them under the terms of the GNU Lesser General Public License
+// as published by the Free Software Foundation, either version 3 of the License,
+// or (at your option) any later version.
+//
+// The libevm additions are distributed in the hope that they will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser
+// General Public License for more details.
+//
+// You should have received a copy of the GNU Lesser General Public License
+// along with the go-ethereum library. If not, see
+// <http://www.gnu.org/licenses/>.
+
+package rlp
+
+import (
+	"bytes"
+	"io"
+	"testing"
+
+	"github.com/kr/pretty"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/ava-labs/libevm/common"
+)
+
+func TestFields(t *testing.T) {
+	type foo struct {
+		A uint64
+		B uint64
+		C *uint64
+		D *uint64   `rlp:"optional"`
+		E []uint64  `rlp:"optional"`
+		F *[]uint64 `rlp:"optional"`
+	}
+
+	const (
+		a uint64 = iota
+		b
+		cVal
+		dVal
+	)
+	c := common.PointerTo(cVal)
+	d := common.PointerTo(dVal)
+	e := []uint64{40, 41}
+	f := &[]uint64{50, 51}
+
+	tests := []foo{
+		{a, b, c, d, e, f},       // 000 (which of d/e/f are nil)
+		{a, b, c, d, e, nil},     // 001
+		{a, b, c, d, nil, f},     // 010
+		{a, b, c, d, nil, nil},   // 011
+		{a, b, c, nil, e, f},     // 100
+		{a, b, c, nil, e, nil},   // 101
+		{a, b, c, nil, nil, f},   // 110
+		{a, b, c, nil, nil, nil}, // 111
+		// Empty and nil slices are treated differently when optional
+		{a, b, c, nil, []uint64{}, nil},
+		{a, b, c, nil, nil, &[]uint64{}},
+	}
+
+	for _, obj := range tests {
+		t.Run("", func(t *testing.T) {
+			t.Logf("\n%s", pretty.Sprint(obj))
+
+			wantRLP, err := EncodeToBytes(obj)
+			require.NoErrorf(t, err, "EncodeToBytes([actual struct])")
+
+			t.Run("Fields.EncodeRLP", func(t *testing.T) {
+				got, err := EncodeToBytes(&Fields{
+					Required: []any{obj.A, obj.B, obj.C},
+					Optional: []any{obj.D, obj.E, obj.F},
+				})
+				require.NoError(t, err)
+				assert.Equal(t, wantRLP, got, "vs EncodeToBytes([actual struct])")
+			})
+
+			t.Run("Fields.DecodeRLP", func(t *testing.T) {
+				var got foo
+				err := DecodeBytes(wantRLP, &Fields{
+					Required: []any{&got.A, &got.B, &got.C},
+					Optional: []any{&got.D, &got.E, &got.F},
+				})
+				require.NoError(t, err, "DecodeBytes(..., %T)", &Fields{})
+
+				var want foo
+				err = DecodeBytes(wantRLP, &want)
+				require.NoError(t, err, "DecodeBytes(..., [actual struct])")
+
+				assert.Equal(t, want, got, "vs DecodeBytes(..., [original struct])")
+			})
+		})
+	}
+}
+
+//nolint:testableexamples // Demonstrating code equivalence, not outputs.
+func ExampleFields() {
+	type inner struct {
+		X uint64
+	}
+
+	type outer struct {
+		A uint64
+		B *inner `rlp:"nil"`
+		C *inner `rlp:"optional"`
+	}
+
+	val := outer{
+		A: 42,
+		B: &inner{X: 42},
+		C: &inner{X: 99},
+	}
+
+	// Errors are dropped for brevity for the sake of the example only.
+
+	_ = Encode(io.Discard, val)
+	// is equivalent to
+	_ = Encode(
+		io.Discard,
+		&Fields{
+			Required: []any{val.A, val.B},
+			Optional: []any{val.C},
+		},
+	)
+
+	var (
+		r       *bytes.Reader // arbitrary RLP buffer
+		decoded outer
+	)
+
+	_ = Decode(r, &decoded)
+	// is equivalent to
+	_ = Decode(r, &Fields{
+		Required: []any{
+			&decoded.A,
+			Nillable(&decoded.B),
+		},
+		Optional: []any{&decoded.C},
+	})
+
+	// Note the parallels between the arguments passed to
+	// Fields.{En,De}codeRLP() and that, when decoding an optional or
+	// `rlp:"nil`-tagged field, a pointer to the _field_ is required even though
+	// in this example it will be a `**inner`.
+}
+
+func TestNillable(t *testing.T) {
+	type inner struct {
+		X uint64
+	}
+
+	type outer struct {
+		A *uint64   `rlp:"nil"`
+		B *inner    `rlp:"nil"`
+		C *[]uint64 `rlp:"nil"`
+	}
+
+	// Unlike the `rlp:"optional"` tag, there is no interplay between nil-tagged
+	// fields so we don't need the Cartesian product of all possible
+	// combinations.
+	var tests []outer
+	for _, a := range []*uint64{
+		nil,
+		common.PointerTo[uint64](0),
+	} {
+		tests = append(tests, outer{a, nil, nil})
+	}
+	for _, b := range []*inner{
+		nil,
+		{0},
+	} {
+		tests = append(tests, outer{nil, b, nil})
+	}
+	for _, c := range []*[]uint64{
+		nil,
+		{},
+		{0},
+	} {
+		tests = append(tests, outer{nil, nil, c})
+	}
+
+	// When a Nillable encounters an empty list it MUST set the field to nil,
+	// not just ignore it.
+	corruptInitialValue := func() outer {
+		return outer{common.PointerTo[uint64](42), &inner{42}, &[]uint64{42}}
+	}
+
+	for _, obj := range tests {
+		t.Run("", func(t *testing.T) {
+			rlp, err := EncodeToBytes(obj)
+			require.NoErrorf(t, err, "EncodeToBytes(%+v)", obj)
+			t.Logf("%s => %#x", pretty.Sprint(obj), rlp)
+
+			// Although this is an immediate inversion of the line above, it
+			// provides us with the canonical RLP decoding, which our input
+			// struct may not honour.
+			want := corruptInitialValue()
+			err = DecodeBytes(rlp, &want)
+			require.NoErrorf(t, err, "DecodeBytes(%#x, %T)", rlp, &want)
+
+			got := corruptInitialValue()
+			err = DecodeBytes(rlp, &Fields{
+				Required: []any{
+					Nillable(&got.A),
+					Nillable(&got.B),
+					Nillable(&got.C),
+				},
+			})
+			require.NoErrorf(t, err, "DecodeBytes(..., %T)", &Fields{})
+
+			assert.Equal(t, want, got, "DecodeBytes(..., [actual struct]) vs DecodeBytes(..., [fields wrapped in Nillable()])")
+		})
+	}
+}