From ad15c31b50591fb15187033fe5443a0992bb2258 Mon Sep 17 00:00:00 2001 From: Quentin Mc Gaw Date: Tue, 11 Feb 2025 15:34:32 +0100 Subject: [PATCH 1/7] feat(core/types): body RLP hooks registration --- core/state/state.libevm_test.go | 5 +- core/state/state_object.libevm_test.go | 15 ++- core/types/block.go | 4 +- core/types/block.libevm.go | 40 +++--- core/types/block.libevm_test.go | 6 +- .../types/rlp_backwards_compat.libevm_test.go | 123 ++++++++++++++++-- core/types/rlp_payload.libevm.go | 37 ++++-- core/types/state_account.libevm_test.go | 10 +- .../state_account_storage.libevm_test.go | 20 ++- 9 files changed, 204 insertions(+), 56 deletions(-) diff --git a/core/state/state.libevm_test.go b/core/state/state.libevm_test.go index 8b682cb49b7..406425b07ce 100644 --- a/core/state/state.libevm_test.go +++ b/core/state/state.libevm_test.go @@ -45,7 +45,10 @@ func TestGetSetExtra(t *testing.T) { t.Cleanup(types.TestOnlyClearRegisteredExtras) // Just as its Data field is a pointer, the registered type is a pointer to // test deep copying. - payloads := types.RegisterExtras[types.NOOPHeaderHooks, *types.NOOPHeaderHooks, *accountExtra]().StateAccount + payloads := types.RegisterExtras[ + types.NOOPHeaderHooks, *types.NOOPHeaderHooks, + types.NOOPBodyHooks, *types.NOOPBodyHooks, + *accountExtra]().StateAccount rng := ethtest.NewPseudoRand(42) addr := rng.Address() diff --git a/core/state/state_object.libevm_test.go b/core/state/state_object.libevm_test.go index d04de5a1aaa..859c8847573 100644 --- a/core/state/state_object.libevm_test.go +++ b/core/state/state_object.libevm_test.go @@ -46,21 +46,30 @@ func TestStateObjectEmpty(t *testing.T) { { name: "explicit false bool", registerAndSet: func(acc *types.StateAccount) { - types.RegisterExtras[types.NOOPHeaderHooks, *types.NOOPHeaderHooks, bool]().StateAccount.Set(acc, false) + types.RegisterExtras[ + types.NOOPHeaderHooks, *types.NOOPHeaderHooks, + types.NOOPBodyHooks, *types.NOOPBodyHooks, + bool]().StateAccount.Set(acc, false) }, wantEmpty: true, }, { name: "implicit false bool", registerAndSet: func(*types.StateAccount) { - types.RegisterExtras[types.NOOPHeaderHooks, *types.NOOPHeaderHooks, bool]() + types.RegisterExtras[ + types.NOOPHeaderHooks, *types.NOOPHeaderHooks, + types.NOOPBodyHooks, *types.NOOPBodyHooks, + bool]() }, wantEmpty: true, }, { name: "true bool", registerAndSet: func(acc *types.StateAccount) { - types.RegisterExtras[types.NOOPHeaderHooks, *types.NOOPHeaderHooks, bool]().StateAccount.Set(acc, true) + types.RegisterExtras[ + types.NOOPHeaderHooks, *types.NOOPHeaderHooks, + types.NOOPBodyHooks, *types.NOOPBodyHooks, + bool]().StateAccount.Set(acc, true) }, wantEmpty: false, }, diff --git a/core/types/block.go b/core/types/block.go index 336275917db..53c318c42f9 100644 --- a/core/types/block.go +++ b/core/types/block.go @@ -176,6 +176,8 @@ type Body struct { Transactions []*Transaction Uncles []*Header Withdrawals []*Withdrawal `rlp:"optional"` + + extra *pseudo.Type // See [RegisterExtras] } // Block represents an Ethereum block. @@ -338,7 +340,7 @@ func (b *Block) EncodeRLP(w io.Writer) error { // Body returns the non-header content of the block. // Note the returned data is not an independent copy. func (b *Block) Body() *Body { - return &Body{b.transactions, b.uncles, b.withdrawals} + return &Body{b.transactions, b.uncles, b.withdrawals, nil /* unexported extras field */} } // Accessors for body data. These do not return a copy because the content diff --git a/core/types/block.libevm.go b/core/types/block.libevm.go index ca5413a61fc..efe0dba1db6 100644 --- a/core/types/block.libevm.go +++ b/core/types/block.libevm.go @@ -22,7 +22,6 @@ import ( "io" "github.com/ava-labs/libevm/libevm/pseudo" - "github.com/ava-labs/libevm/libevm/testonly" "github.com/ava-labs/libevm/rlp" ) @@ -45,7 +44,7 @@ func (h *Header) hooks() HeaderHooks { return new(NOOPHeaderHooks) } -func (e ExtraPayloads[HPtr, SA]) hooksFromHeader(h *Header) HeaderHooks { +func (e ExtraPayloads[HPtr, BPtr, SA]) hooksFromHeader(h *Header) HeaderHooks { return e.Header.Get(h) } @@ -134,22 +133,11 @@ type BodyHooks interface { RLPFieldPointersForDecoding(*Body) *rlp.Fields } -// TestOnlyRegisterBodyHooks is a temporary means of "registering" BodyHooks for -// the purpose of testing. It will panic if called outside of a test. -func TestOnlyRegisterBodyHooks(h BodyHooks) { - testonly.OrPanic(func() { - todoRegisteredBodyHooks = h - }) -} - -// todoRegisteredBodyHooks is a temporary placeholder for "registering" -// BodyHooks, before they are included in [RegisterExtras]. -var todoRegisteredBodyHooks BodyHooks = NOOPBodyHooks{} - func (b *Body) hooks() BodyHooks { - // TODO(arr4n): when incorporating BodyHooks into [RegisterExtras], the - // [todoRegisteredBodyHooks] variable MUST be removed. - return todoRegisteredBodyHooks + if r := registeredExtras; r.Registered() { + return r.Get().hooks.hooksFromBody(b) + } + return new(NOOPBodyHooks) } // NOOPBodyHooks implements [BodyHooks] such that they are equivalent to no type @@ -160,7 +148,7 @@ type NOOPBodyHooks 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{}} +var _ = &Body{[]*Transaction{}, []*Header{}, []*Withdrawal{}, nil /* extra unexported type */} func (NOOPBodyHooks) RLPFieldsForEncoding(b *Body) *rlp.Fields { return &rlp.Fields{ @@ -175,3 +163,19 @@ func (NOOPBodyHooks) RLPFieldPointersForDecoding(b *Body) *rlp.Fields { Optional: []any{&b.Withdrawals}, } } + +func (e ExtraPayloads[HPtr, BPtr, SA]) hooksFromBody(b *Body) BodyHooks { + return e.Body.Get(b) +} + +func (b *Body) extraPayload() *pseudo.Type { + r := registeredExtras + if !r.Registered() { + // See params.ChainConfig.extraPayload() for panic rationale. + panic(fmt.Sprintf("%T.extraPayload() called before RegisterExtras()", r)) + } + if b.extra == nil { + b.extra = r.Get().newBody() + } + return b.extra +} diff --git a/core/types/block.libevm_test.go b/core/types/block.libevm_test.go index 84f13984e6c..39898c7aec3 100644 --- a/core/types/block.libevm_test.go +++ b/core/types/block.libevm_test.go @@ -86,7 +86,11 @@ func TestHeaderHooks(t *testing.T) { TestOnlyClearRegisteredExtras() defer TestOnlyClearRegisteredExtras() - extras := RegisterExtras[stubHeaderHooks, *stubHeaderHooks, struct{}]() + extras := RegisterExtras[ + stubHeaderHooks, *stubHeaderHooks, + NOOPBodyHooks, *NOOPBodyHooks, + struct{}, + ]() rng := ethtest.NewPseudoRand(13579) suffix := rng.Bytes(8) diff --git a/core/types/rlp_backwards_compat.libevm_test.go b/core/types/rlp_backwards_compat.libevm_test.go index 673bf548c39..f5b3c6b7a09 100644 --- a/core/types/rlp_backwards_compat.libevm_test.go +++ b/core/types/rlp_backwards_compat.libevm_test.go @@ -18,6 +18,7 @@ package types_test import ( "encoding/hex" + "reflect" "testing" "github.com/google/go-cmp/cmp" @@ -44,7 +45,11 @@ func TestHeaderRLPBackwardsCompatibility(t *testing.T) { { name: "no-op header hooks", register: func() { - RegisterExtras[NOOPHeaderHooks, *NOOPHeaderHooks, struct{}]() + RegisterExtras[ + NOOPHeaderHooks, *NOOPHeaderHooks, + NOOPBodyHooks, *NOOPBodyHooks, + struct{}, + ]() }, }, } @@ -138,7 +143,7 @@ func TestBodyRLPBackwardsCompatibility(t *testing.T) { for _, tx := range txMatrix { for _, u := range uncleMatrix { for _, w := range withdrawMatrix { - bodies = append(bodies, &Body{tx, u, w}) + bodies = append(bodies, makeBody(tx, u, w)) } } } @@ -165,7 +170,17 @@ func TestBodyRLPBackwardsCompatibility(t *testing.T) { err := rlp.DecodeBytes(wantRLP, got) require.NoErrorf(t, err, "rlp.DecodeBytes(%v, %T)", wantRLP, got) - want := body + // Note we do not specify field names to enforce all fields are set. + gotWithoutExtra := testBodyWithoutExtra{ + got.Transactions, + got.Uncles, + got.Withdrawals, + } + want := testBodyWithoutExtra{ + body.Transactions, + body.Uncles, + body.Withdrawals, + } // Regular RLP decoding will never leave these non-optional // fields nil. if want.Transactions == nil { @@ -179,14 +194,46 @@ func TestBodyRLPBackwardsCompatibility(t *testing.T) { cmpeth.CompareHeadersByHash(), cmpeth.CompareTransactionsByBinary(t), } - if diff := cmp.Diff(body, got, opts); diff != "" { - t.Errorf("rlp.DecodeBytes(rlp.EncodeToBytes(%#v)) diff (-want +got):\n%s", body, diff) + if diff := cmp.Diff(want, gotWithoutExtra, opts); diff != "" { + t.Errorf("rlp.DecodeBytes(rlp.EncodeToBytes(%#v)) diff (-want +got):\n%s", want, diff) } }) }) } } +// makeBody creates a [*Body] with the given arguments and ensures all exported +// fields are set through the test [TestmakeBody]. +func makeBody(txs []*Transaction, uncles []*Header, withdrawals []*Withdrawal) *Body { + return &Body{ + Transactions: txs, + Uncles: uncles, + Withdrawals: withdrawals, + } +} + +func Test_makeBody(t *testing.T) { + t.Parallel() + + txs := []*Transaction{} + uncles := []*Header{} + withdrawals := []*Withdrawal{} + + body := makeBody(txs, uncles, withdrawals) + + bodyType := reflect.TypeOf(*body) + for i := 0; i < bodyType.NumField(); i++ { + field := bodyType.Field(i) + if !field.IsExported() { + continue + } + if reflect.ValueOf(*body).Field(i).IsZero() { + t.Errorf("body created with makeBody has its exported field %q not set. "+ + "Please update makeBody to set all exported fields of Body.", field.Name) + } + } +} + // cChainBodyExtras carries the same additional fields as the Avalanche C-Chain // (ava-labs/coreth) [Body] and implements [BodyHooks] to achieve equivalent RLP // {en,de}coding. @@ -230,10 +277,13 @@ func TestBodyRLPCChainCompat(t *testing.T) { // The inputs to this test were used to generate the expected RLP with // ava-labs/coreth. This serves as both an example of how to use [BodyHooks] // and a test of compatibility. - - t.Cleanup(func() { - TestOnlyRegisterBodyHooks(NOOPBodyHooks{}) - }) + TestOnlyClearRegisteredExtras() + t.Cleanup(TestOnlyClearRegisteredExtras) + extras := RegisterExtras[ + NOOPHeaderHooks, *NOOPHeaderHooks, + cChainBodyExtras, *cChainBodyExtras, + struct{}, + ]() body := &Body{ Transactions: []*Transaction{ @@ -276,7 +326,7 @@ func TestBodyRLPCChainCompat(t *testing.T) { require.NoErrorf(t, err, "hex.DecodeString(%q)", tt.wantRLPHex) t.Run("Encode", func(t *testing.T) { - TestOnlyRegisterBodyHooks(tt.extra) + extras.Body.Set(body, tt.extra) got, err := rlp.EncodeToBytes(body) require.NoErrorf(t, err, "rlp.EncodeToBytes(%+v)", body) assert.Equalf(t, wantRLP, got, "rlp.EncodeToBytes(%+v)", body) @@ -284,21 +334,66 @@ func TestBodyRLPCChainCompat(t *testing.T) { t.Run("Decode", func(t *testing.T) { var extra cChainBodyExtras - TestOnlyRegisterBodyHooks(&extra) - got := new(Body) + extras.Body.Set(got, &extra) err := rlp.DecodeBytes(wantRLP, got) require.NoErrorf(t, err, "rlp.DecodeBytes(%#x, %T)", wantRLP, got) assert.Equal(t, tt.extra, &extra, "rlp.DecodeBytes(%#x, [%T as registered extra in %T carrier])", wantRLP, &extra, got) + // Note we do not specify field names to enforce all fields are set. + wantWithoutExtra := testBodyWithoutExtra{ + body.Transactions, + body.Uncles, + body.Withdrawals, + } + gotWithoutExtra := testBodyWithoutExtra{ + got.Transactions, + got.Uncles, + got.Withdrawals, + } + opts := cmp.Options{ cmpeth.CompareHeadersByHash(), cmpeth.CompareTransactionsByBinary(t), } - if diff := cmp.Diff(body, got, opts); diff != "" { - t.Errorf("rlp.DecodeBytes(%#x, [%T while carrying registered %T extra payload]) diff (-want +got):\n%s", wantRLP, got, &extra, diff) + if diff := cmp.Diff(wantWithoutExtra, gotWithoutExtra, opts); diff != "" { + t.Errorf("rlp.DecodeBytes(%#x, [%T while carrying registered %T extra payload]) diff (-want +got):\n%s", wantRLP, gotWithoutExtra, &extra, diff) } }) }) } } + +type testBodyWithoutExtra struct { + Transactions []*Transaction + Uncles []*Header + Withdrawals []*Withdrawal +} + +// Test_testBodyWithoutExtra enforces the [bodyWithoutExtra] has its fields the same, +// in terms of name and type, as all the exported fields of [Body], using [reflect]. +func Test_testBodyWithoutExtra(t *testing.T) { + t.Parallel() + + refTyp := reflect.TypeOf(Body{}) + refFieldNameToType := make(map[string]reflect.Type, refTyp.NumField()) + for i := 0; i < refTyp.NumField(); i++ { + f := refTyp.Field(i) + if !f.IsExported() { + continue + } + refFieldNameToType[f.Name] = f.Type + } + + testTyp := reflect.TypeOf(testBodyWithoutExtra{}) + testFieldNameToType := make(map[string]reflect.Type, testTyp.NumField()) + for i := 0; i < testTyp.NumField(); i++ { + f := testTyp.Field(i) + if !f.IsExported() { + t.Fatalf("field %s is not exported", f.Name) + } + testFieldNameToType[f.Name] = f.Type + } + + assert.Equal(t, refFieldNameToType, testFieldNameToType) +} diff --git a/core/types/rlp_payload.libevm.go b/core/types/rlp_payload.libevm.go index 90d5b7e43e3..3925a1857aa 100644 --- a/core/types/rlp_payload.libevm.go +++ b/core/types/rlp_payload.libevm.go @@ -46,13 +46,21 @@ func RegisterExtras[ HeaderHooks *H }, + B any, BPtr interface { + BodyHooks + *B + }, SA any, -]() ExtraPayloads[HPtr, SA] { - extra := ExtraPayloads[HPtr, SA]{ +]() ExtraPayloads[HPtr, BPtr, SA] { + extra := ExtraPayloads[HPtr, BPtr, SA]{ Header: pseudo.NewAccessor[*Header, HPtr]( (*Header).extraPayload, func(h *Header, t *pseudo.Type) { h.extra = t }, ), + Body: pseudo.NewAccessor[*Body, BPtr]( + (*Body).extraPayload, + func(b *Body, t *pseudo.Type) { b.extra = t }, + ), StateAccount: pseudo.NewAccessor[StateOrSlimAccount, SA]( func(a StateOrSlimAccount) *pseudo.Type { return a.extra().payload() }, func(a StateOrSlimAccount, t *pseudo.Type) { a.extra().t = t }, @@ -63,10 +71,11 @@ func RegisterExtras[ var x SA return fmt.Sprintf("%T", x) }(), - // The [ExtraPayloads] that we returns is based on [HPtr,SA], not [H,SA] - // so our constructors MUST match that. This guarantees that calls to - // the [HeaderHooks] methods will never be performed on a nil pointer. + // The [ExtraPayloads] that we returns is based on [HPtr,BPtr,SA], not + // [H,B,SA] so our constructors MUST match that. This guarantees that calls to + // the [HeaderHooks] and [BodyHooks] methods will never be performed on a nil pointer. newHeader: pseudo.NewConstructor[H]().NewPointer, // i.e. non-nil HPtr + newBody: pseudo.NewConstructor[B]().NewPointer, // i.e. non-nil B newStateAccount: pseudo.NewConstructor[SA]().Zero, cloneStateAccount: extra.cloneStateAccount, hooks: extra, @@ -87,11 +96,14 @@ func TestOnlyClearRegisteredExtras() { var registeredExtras register.AtMostOnce[*extraConstructors] type extraConstructors struct { - stateAccountType string - newHeader, newStateAccount func() *pseudo.Type - cloneStateAccount func(*StateAccountExtra) *StateAccountExtra - hooks interface { + stateAccountType string + newHeader func() *pseudo.Type + newBody func() *pseudo.Type + newStateAccount func() *pseudo.Type + cloneStateAccount func(*StateAccountExtra) *StateAccountExtra + hooks interface { hooksFromHeader(*Header) HeaderHooks + hooksFromBody(*Body) BodyHooks } } @@ -105,14 +117,15 @@ func (e *StateAccountExtra) clone() *StateAccountExtra { } // ExtraPayloads provides strongly typed access to the extra payload carried by -// [Header], [StateAccount], and [SlimAccount] structs. The only valid way to +// [Header], [Body], [StateAccount], and [SlimAccount] structs. The only valid way to // construct an instance is by a call to [RegisterExtras]. -type ExtraPayloads[HPtr HeaderHooks, SA any] struct { +type ExtraPayloads[HPtr HeaderHooks, BPtr BodyHooks, SA any] struct { Header pseudo.Accessor[*Header, HPtr] + Body pseudo.Accessor[*Body, BPtr] StateAccount pseudo.Accessor[StateOrSlimAccount, SA] // Also provides [SlimAccount] access. } -func (ExtraPayloads[HPtr, SA]) cloneStateAccount(s *StateAccountExtra) *StateAccountExtra { +func (ExtraPayloads[HPtr, BPtr, SA]) cloneStateAccount(s *StateAccountExtra) *StateAccountExtra { v := pseudo.MustNewValue[SA](s.t) return &StateAccountExtra{ t: pseudo.From(v.Get()).Type, diff --git a/core/types/state_account.libevm_test.go b/core/types/state_account.libevm_test.go index 6fd601ef9d7..2458df8ace4 100644 --- a/core/types/state_account.libevm_test.go +++ b/core/types/state_account.libevm_test.go @@ -46,7 +46,10 @@ func TestStateAccountRLP(t *testing.T) { explicitFalseBoolean := test{ name: "explicit false-boolean extra", register: func() { - RegisterExtras[NOOPHeaderHooks, *NOOPHeaderHooks, bool]() + RegisterExtras[ + NOOPHeaderHooks, *NOOPHeaderHooks, + NOOPBodyHooks, *NOOPBodyHooks, + bool]() }, acc: &StateAccount{ Nonce: 0x444444, @@ -76,7 +79,10 @@ func TestStateAccountRLP(t *testing.T) { { name: "true-boolean extra", register: func() { - RegisterExtras[NOOPHeaderHooks, *NOOPHeaderHooks, bool]() + RegisterExtras[ + NOOPHeaderHooks, *NOOPHeaderHooks, + NOOPBodyHooks, *NOOPBodyHooks, + bool]() }, acc: &StateAccount{ Nonce: 0x444444, diff --git a/core/types/state_account_storage.libevm_test.go b/core/types/state_account_storage.libevm_test.go index 9db9ee552d8..adf3ed15bc0 100644 --- a/core/types/state_account_storage.libevm_test.go +++ b/core/types/state_account_storage.libevm_test.go @@ -73,7 +73,10 @@ func TestStateAccountExtraViaTrieStorage(t *testing.T) { { name: "true-boolean payload", registerAndSetExtra: func(a *types.StateAccount) (*types.StateAccount, assertion) { - e := types.RegisterExtras[types.NOOPHeaderHooks, *types.NOOPHeaderHooks, bool]() + e := types.RegisterExtras[ + types.NOOPHeaderHooks, *types.NOOPHeaderHooks, + types.NOOPBodyHooks, *types.NOOPBodyHooks, + bool]() e.StateAccount.Set(a, true) return a, func(t *testing.T, got *types.StateAccount) { //nolint:thelper assert.Truef(t, e.StateAccount.Get(got), "") @@ -84,7 +87,10 @@ func TestStateAccountExtraViaTrieStorage(t *testing.T) { { name: "explicit false-boolean payload", registerAndSetExtra: func(a *types.StateAccount) (*types.StateAccount, assertion) { - e := types.RegisterExtras[types.NOOPHeaderHooks, *types.NOOPHeaderHooks, bool]() + e := types.RegisterExtras[ + types.NOOPHeaderHooks, *types.NOOPHeaderHooks, + types.NOOPBodyHooks, *types.NOOPBodyHooks, + bool]() e.StateAccount.Set(a, false) // the explicit part return a, func(t *testing.T, got *types.StateAccount) { //nolint:thelper @@ -96,7 +102,10 @@ func TestStateAccountExtraViaTrieStorage(t *testing.T) { { name: "implicit false-boolean payload", registerAndSetExtra: func(a *types.StateAccount) (*types.StateAccount, assertion) { - e := types.RegisterExtras[types.NOOPHeaderHooks, *types.NOOPHeaderHooks, bool]() + e := types.RegisterExtras[ + types.NOOPHeaderHooks, *types.NOOPHeaderHooks, + types.NOOPBodyHooks, *types.NOOPBodyHooks, + bool]() // Note that `a` is reflected, unchanged (the implicit part). return a, func(t *testing.T, got *types.StateAccount) { //nolint:thelper assert.Falsef(t, e.StateAccount.Get(got), "") @@ -107,7 +116,10 @@ func TestStateAccountExtraViaTrieStorage(t *testing.T) { { name: "arbitrary payload", registerAndSetExtra: func(a *types.StateAccount) (*types.StateAccount, assertion) { - e := types.RegisterExtras[types.NOOPHeaderHooks, *types.NOOPHeaderHooks, arbitraryPayload]() + e := types.RegisterExtras[ + types.NOOPHeaderHooks, *types.NOOPHeaderHooks, + types.NOOPBodyHooks, *types.NOOPBodyHooks, + arbitraryPayload]() p := arbitraryPayload{arbitraryData} e.StateAccount.Set(a, p) return a, func(t *testing.T, got *types.StateAccount) { //nolint:thelper From eafcb289f474042763b8698a9bffcbe131a29fa4 Mon Sep 17 00:00:00 2001 From: Quentin Mc Gaw Date: Tue, 11 Feb 2025 16:10:22 +0100 Subject: [PATCH 2/7] Quiet down TestBodyRLPBackwardsCompatibility --- core/types/rlp_backwards_compat.libevm_test.go | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/core/types/rlp_backwards_compat.libevm_test.go b/core/types/rlp_backwards_compat.libevm_test.go index f5b3c6b7a09..e9284746182 100644 --- a/core/types/rlp_backwards_compat.libevm_test.go +++ b/core/types/rlp_backwards_compat.libevm_test.go @@ -150,7 +150,11 @@ func TestBodyRLPBackwardsCompatibility(t *testing.T) { for _, body := range bodies { t.Run("", func(t *testing.T) { - t.Logf("\n%s", pretty.Sprint(body)) + t.Cleanup(func() { + if t.Failed() { + t.Logf("\n%s", pretty.Sprint(body)) + } + }) // The original [Body] doesn't implement [rlp.Encoder] nor // [rlp.Decoder] so we can use a methodless equivalent as the gold @@ -161,14 +165,15 @@ func TestBodyRLPBackwardsCompatibility(t *testing.T) { t.Run("Encode", func(t *testing.T) { got, err := rlp.EncodeToBytes(body) - require.NoErrorf(t, err, "rlp.EncodeToBytes(%#v)", body) - assert.Equalf(t, wantRLP, got, "rlp.EncodeToBytes(%#v)", body) + require.NoErrorf(t, err, "rlp.EncodeToBytes(%T)", body) + assert.Equalf(t, wantRLP, got, "rlp.EncodeToBytes(%T)", body) }) t.Run("Decode", func(t *testing.T) { got := new(Body) err := rlp.DecodeBytes(wantRLP, got) - require.NoErrorf(t, err, "rlp.DecodeBytes(%v, %T)", wantRLP, got) + require.NoErrorf(t, err, "rlp.DecodeBytes(rlp.EncodeToBytes(%T), %T) resulted in %s", + (*withoutMethods)(body), got, pretty.Sprint(got)) // Note we do not specify field names to enforce all fields are set. gotWithoutExtra := testBodyWithoutExtra{ @@ -195,7 +200,7 @@ func TestBodyRLPBackwardsCompatibility(t *testing.T) { cmpeth.CompareTransactionsByBinary(t), } if diff := cmp.Diff(want, gotWithoutExtra, opts); diff != "" { - t.Errorf("rlp.DecodeBytes(rlp.EncodeToBytes(%#v)) diff (-want +got):\n%s", want, diff) + t.Errorf("rlp.DecodeBytes(rlp.EncodeToBytes(%T)) diff (-want +got):\n%s", (*withoutMethods)(body), diff) } }) }) From 0f2b8ee2beb106a023396ca8c931e6700f73459c Mon Sep 17 00:00:00 2001 From: Quentin Mc Gaw Date: Tue, 11 Feb 2025 18:05:37 +0100 Subject: [PATCH 3/7] use cmpopts.IgnoreUnexported(Body{}) --- .../types/rlp_backwards_compat.libevm_test.go | 67 ++----------------- 1 file changed, 7 insertions(+), 60 deletions(-) diff --git a/core/types/rlp_backwards_compat.libevm_test.go b/core/types/rlp_backwards_compat.libevm_test.go index e9284746182..e72fec72822 100644 --- a/core/types/rlp_backwards_compat.libevm_test.go +++ b/core/types/rlp_backwards_compat.libevm_test.go @@ -22,6 +22,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/kr/pretty" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -175,17 +176,7 @@ func TestBodyRLPBackwardsCompatibility(t *testing.T) { require.NoErrorf(t, err, "rlp.DecodeBytes(rlp.EncodeToBytes(%T), %T) resulted in %s", (*withoutMethods)(body), got, pretty.Sprint(got)) - // Note we do not specify field names to enforce all fields are set. - gotWithoutExtra := testBodyWithoutExtra{ - got.Transactions, - got.Uncles, - got.Withdrawals, - } - want := testBodyWithoutExtra{ - body.Transactions, - body.Uncles, - body.Withdrawals, - } + want := body // Regular RLP decoding will never leave these non-optional // fields nil. if want.Transactions == nil { @@ -198,8 +189,9 @@ func TestBodyRLPBackwardsCompatibility(t *testing.T) { opts := cmp.Options{ cmpeth.CompareHeadersByHash(), cmpeth.CompareTransactionsByBinary(t), + cmpopts.IgnoreUnexported(Body{}), } - if diff := cmp.Diff(want, gotWithoutExtra, opts); diff != "" { + if diff := cmp.Diff(want, got, opts); diff != "" { t.Errorf("rlp.DecodeBytes(rlp.EncodeToBytes(%T)) diff (-want +got):\n%s", (*withoutMethods)(body), diff) } }) @@ -345,60 +337,15 @@ func TestBodyRLPCChainCompat(t *testing.T) { require.NoErrorf(t, err, "rlp.DecodeBytes(%#x, %T)", wantRLP, got) assert.Equal(t, tt.extra, &extra, "rlp.DecodeBytes(%#x, [%T as registered extra in %T carrier])", wantRLP, &extra, got) - // Note we do not specify field names to enforce all fields are set. - wantWithoutExtra := testBodyWithoutExtra{ - body.Transactions, - body.Uncles, - body.Withdrawals, - } - gotWithoutExtra := testBodyWithoutExtra{ - got.Transactions, - got.Uncles, - got.Withdrawals, - } - opts := cmp.Options{ cmpeth.CompareHeadersByHash(), cmpeth.CompareTransactionsByBinary(t), + cmpopts.IgnoreUnexported(Body{}), } - if diff := cmp.Diff(wantWithoutExtra, gotWithoutExtra, opts); diff != "" { - t.Errorf("rlp.DecodeBytes(%#x, [%T while carrying registered %T extra payload]) diff (-want +got):\n%s", wantRLP, gotWithoutExtra, &extra, diff) + if diff := cmp.Diff(body, got, opts); diff != "" { + t.Errorf("rlp.DecodeBytes(%#x, [%T while carrying registered %T extra payload]) diff (-want +got):\n%s", wantRLP, got, &extra, diff) } }) }) } } - -type testBodyWithoutExtra struct { - Transactions []*Transaction - Uncles []*Header - Withdrawals []*Withdrawal -} - -// Test_testBodyWithoutExtra enforces the [bodyWithoutExtra] has its fields the same, -// in terms of name and type, as all the exported fields of [Body], using [reflect]. -func Test_testBodyWithoutExtra(t *testing.T) { - t.Parallel() - - refTyp := reflect.TypeOf(Body{}) - refFieldNameToType := make(map[string]reflect.Type, refTyp.NumField()) - for i := 0; i < refTyp.NumField(); i++ { - f := refTyp.Field(i) - if !f.IsExported() { - continue - } - refFieldNameToType[f.Name] = f.Type - } - - testTyp := reflect.TypeOf(testBodyWithoutExtra{}) - testFieldNameToType := make(map[string]reflect.Type, testTyp.NumField()) - for i := 0; i < testTyp.NumField(); i++ { - f := testTyp.Field(i) - if !f.IsExported() { - t.Fatalf("field %s is not exported", f.Name) - } - testFieldNameToType[f.Name] = f.Type - } - - assert.Equal(t, refFieldNameToType, testFieldNameToType) -} From e3d2e20dffa9392c190137b3211672523549cdb1 Mon Sep 17 00:00:00 2001 From: Quentin McGaw Date: Wed, 12 Feb 2025 09:19:48 +0100 Subject: [PATCH 4/7] Body hooks default is Noop value not pointer Co-authored-by: Arran Schlosberg <519948+ARR4N@users.noreply.github.com> --- core/types/block.libevm.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/types/block.libevm.go b/core/types/block.libevm.go index efe0dba1db6..a02abbdb423 100644 --- a/core/types/block.libevm.go +++ b/core/types/block.libevm.go @@ -137,7 +137,7 @@ func (b *Body) hooks() BodyHooks { if r := registeredExtras; r.Registered() { return r.Get().hooks.hooksFromBody(b) } - return new(NOOPBodyHooks) + return NOOPBodyHooks{} } // NOOPBodyHooks implements [BodyHooks] such that they are equivalent to no type From 948c104622fd0765eb152c9d7c88c24007687992 Mon Sep 17 00:00:00 2001 From: Quentin McGaw Date: Wed, 12 Feb 2025 09:24:08 +0100 Subject: [PATCH 5/7] Fix comment on `newBody` constructor Co-authored-by: Arran Schlosberg <519948+ARR4N@users.noreply.github.com> --- core/types/rlp_payload.libevm.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/types/rlp_payload.libevm.go b/core/types/rlp_payload.libevm.go index 3925a1857aa..db9863908c8 100644 --- a/core/types/rlp_payload.libevm.go +++ b/core/types/rlp_payload.libevm.go @@ -75,7 +75,7 @@ func RegisterExtras[ // [H,B,SA] so our constructors MUST match that. This guarantees that calls to // the [HeaderHooks] and [BodyHooks] methods will never be performed on a nil pointer. newHeader: pseudo.NewConstructor[H]().NewPointer, // i.e. non-nil HPtr - newBody: pseudo.NewConstructor[B]().NewPointer, // i.e. non-nil B + newBody: pseudo.NewConstructor[B]().NewPointer, // i.e. non-nil BPtr newStateAccount: pseudo.NewConstructor[SA]().Zero, cloneStateAccount: extra.cloneStateAccount, hooks: extra, From 2c46b58c16df2f2b73458f00acce998b723bc169 Mon Sep 17 00:00:00 2001 From: Quentin Mc Gaw Date: Wed, 12 Feb 2025 09:27:03 +0100 Subject: [PATCH 6/7] Fix formatting issues --- core/state/state.libevm_test.go | 3 ++- core/state/state_object.libevm_test.go | 9 ++++++--- core/types/state_account.libevm_test.go | 6 ++++-- core/types/state_account_storage.libevm_test.go | 12 ++++++++---- 4 files changed, 20 insertions(+), 10 deletions(-) diff --git a/core/state/state.libevm_test.go b/core/state/state.libevm_test.go index 406425b07ce..bd909203102 100644 --- a/core/state/state.libevm_test.go +++ b/core/state/state.libevm_test.go @@ -48,7 +48,8 @@ func TestGetSetExtra(t *testing.T) { payloads := types.RegisterExtras[ types.NOOPHeaderHooks, *types.NOOPHeaderHooks, types.NOOPBodyHooks, *types.NOOPBodyHooks, - *accountExtra]().StateAccount + *accountExtra, + ]().StateAccount rng := ethtest.NewPseudoRand(42) addr := rng.Address() diff --git a/core/state/state_object.libevm_test.go b/core/state/state_object.libevm_test.go index 859c8847573..034764d419f 100644 --- a/core/state/state_object.libevm_test.go +++ b/core/state/state_object.libevm_test.go @@ -49,7 +49,8 @@ func TestStateObjectEmpty(t *testing.T) { types.RegisterExtras[ types.NOOPHeaderHooks, *types.NOOPHeaderHooks, types.NOOPBodyHooks, *types.NOOPBodyHooks, - bool]().StateAccount.Set(acc, false) + bool, + ]().StateAccount.Set(acc, false) }, wantEmpty: true, }, @@ -59,7 +60,8 @@ func TestStateObjectEmpty(t *testing.T) { types.RegisterExtras[ types.NOOPHeaderHooks, *types.NOOPHeaderHooks, types.NOOPBodyHooks, *types.NOOPBodyHooks, - bool]() + bool, + ]() }, wantEmpty: true, }, @@ -69,7 +71,8 @@ func TestStateObjectEmpty(t *testing.T) { types.RegisterExtras[ types.NOOPHeaderHooks, *types.NOOPHeaderHooks, types.NOOPBodyHooks, *types.NOOPBodyHooks, - bool]().StateAccount.Set(acc, true) + bool, + ]().StateAccount.Set(acc, true) }, wantEmpty: false, }, diff --git a/core/types/state_account.libevm_test.go b/core/types/state_account.libevm_test.go index 2458df8ace4..8ae8185d38e 100644 --- a/core/types/state_account.libevm_test.go +++ b/core/types/state_account.libevm_test.go @@ -49,7 +49,8 @@ func TestStateAccountRLP(t *testing.T) { RegisterExtras[ NOOPHeaderHooks, *NOOPHeaderHooks, NOOPBodyHooks, *NOOPBodyHooks, - bool]() + bool, + ]() }, acc: &StateAccount{ Nonce: 0x444444, @@ -82,7 +83,8 @@ func TestStateAccountRLP(t *testing.T) { RegisterExtras[ NOOPHeaderHooks, *NOOPHeaderHooks, NOOPBodyHooks, *NOOPBodyHooks, - bool]() + bool, + ]() }, acc: &StateAccount{ Nonce: 0x444444, diff --git a/core/types/state_account_storage.libevm_test.go b/core/types/state_account_storage.libevm_test.go index adf3ed15bc0..29311a8d1db 100644 --- a/core/types/state_account_storage.libevm_test.go +++ b/core/types/state_account_storage.libevm_test.go @@ -76,7 +76,8 @@ func TestStateAccountExtraViaTrieStorage(t *testing.T) { e := types.RegisterExtras[ types.NOOPHeaderHooks, *types.NOOPHeaderHooks, types.NOOPBodyHooks, *types.NOOPBodyHooks, - bool]() + bool, + ]() e.StateAccount.Set(a, true) return a, func(t *testing.T, got *types.StateAccount) { //nolint:thelper assert.Truef(t, e.StateAccount.Get(got), "") @@ -90,7 +91,8 @@ func TestStateAccountExtraViaTrieStorage(t *testing.T) { e := types.RegisterExtras[ types.NOOPHeaderHooks, *types.NOOPHeaderHooks, types.NOOPBodyHooks, *types.NOOPBodyHooks, - bool]() + bool, + ]() e.StateAccount.Set(a, false) // the explicit part return a, func(t *testing.T, got *types.StateAccount) { //nolint:thelper @@ -105,7 +107,8 @@ func TestStateAccountExtraViaTrieStorage(t *testing.T) { e := types.RegisterExtras[ types.NOOPHeaderHooks, *types.NOOPHeaderHooks, types.NOOPBodyHooks, *types.NOOPBodyHooks, - bool]() + bool, + ]() // Note that `a` is reflected, unchanged (the implicit part). return a, func(t *testing.T, got *types.StateAccount) { //nolint:thelper assert.Falsef(t, e.StateAccount.Get(got), "") @@ -119,7 +122,8 @@ func TestStateAccountExtraViaTrieStorage(t *testing.T) { e := types.RegisterExtras[ types.NOOPHeaderHooks, *types.NOOPHeaderHooks, types.NOOPBodyHooks, *types.NOOPBodyHooks, - arbitraryPayload]() + arbitraryPayload, + ]() p := arbitraryPayload{arbitraryData} e.StateAccount.Set(a, p) return a, func(t *testing.T, got *types.StateAccount) { //nolint:thelper From a450d87ed1a72dfd6f0a70e54b71ebeda42e7579 Mon Sep 17 00:00:00 2001 From: Quentin Mc Gaw Date: Wed, 12 Feb 2025 09:42:02 +0100 Subject: [PATCH 7/7] Shorten `Test_makeBody` --- core/types/rlp_backwards_compat.libevm_test.go | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/core/types/rlp_backwards_compat.libevm_test.go b/core/types/rlp_backwards_compat.libevm_test.go index e72fec72822..3963c9c3688 100644 --- a/core/types/rlp_backwards_compat.libevm_test.go +++ b/core/types/rlp_backwards_compat.libevm_test.go @@ -210,21 +210,15 @@ func makeBody(txs []*Transaction, uncles []*Header, withdrawals []*Withdrawal) * } func Test_makeBody(t *testing.T) { - t.Parallel() + body := *makeBody([]*Transaction{}, []*Header{}, []*Withdrawal{}) - txs := []*Transaction{} - uncles := []*Header{} - withdrawals := []*Withdrawal{} - - body := makeBody(txs, uncles, withdrawals) - - bodyType := reflect.TypeOf(*body) + bodyType := reflect.TypeOf(body) for i := 0; i < bodyType.NumField(); i++ { field := bodyType.Field(i) if !field.IsExported() { continue } - if reflect.ValueOf(*body).Field(i).IsZero() { + if reflect.ValueOf(body).Field(i).IsZero() { t.Errorf("body created with makeBody has its exported field %q not set. "+ "Please update makeBody to set all exported fields of Body.", field.Name) }