-
Notifications
You must be signed in to change notification settings - Fork 15
Remove Block RLP hooks #304
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 18 commits
c96078f
17a2dbc
0331e83
9516b73
d97b5b7
332ff9b
54e9ebb
e2ff812
c63a15e
03c95af
9e05728
50ec497
76e0af5
c681534
8f27da7
73c8af6
d9378ad
b9fdfd4
dbdbcfa
ececab1
161b4eb
fea5b35
c7aa4e5
d34e7ce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ import ( | |
| "fmt" | ||
| "io" | ||
| "reflect" | ||
| "slices" | ||
| "strings" | ||
| "testing" | ||
|
|
||
|
|
@@ -33,6 +34,7 @@ import ( | |
| "github.com/ava-labs/libevm/internal/libevm/pseudo" | ||
| "github.com/ava-labs/libevm/libevm/ethtest" | ||
| "github.com/ava-labs/libevm/rlp" | ||
| "github.com/ava-labs/libevm/trie" | ||
| ) | ||
|
|
||
| type stubHeaderHooks struct { | ||
|
|
@@ -272,3 +274,210 @@ func TestBlockWithX(t *testing.T) { | |
| }) | ||
| } | ||
| } | ||
|
|
||
| // bodyPayload is a [types.BlockBodyHooks] implementation carrying an extra | ||
| // field that is {en,de}coded as if it was a regular RLP field of both the | ||
| // [types.Block] and [types.Body]. | ||
| type bodyPayload struct { | ||
| Data []byte | ||
|
|
||
| NOOPBlockBodyHooks | ||
| } | ||
|
|
||
| func (p *bodyPayload) Copy() *bodyPayload { | ||
| return &bodyPayload{ | ||
| Data: slices.Clone(p.Data), | ||
| } | ||
| } | ||
|
|
||
| var rlpBodyPayloads pseudo.Accessor[*Body, *bodyPayload] | ||
|
|
||
| func (*bodyPayload) BodyRLPFieldsForEncoding(b *Body) *rlp.Fields { | ||
| // Rather than using the receiver directly, we access it through b. This | ||
| // demonstrates that the hooks can access their own payload via the | ||
| // [types.Body] they are passed. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is an anti-pattern because it doesn't demonstrate that the hook was called on the correct payload. The idiomatic implementation is to use the receiver as it doesn't require hooks to have access to the Is there a specific reason you'd want a hook to access its payload via the carrying struct?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree. This test is specifically ensuring that a "non-idiomatic" implementation works.
I don't think an implementation SHOULD do this. But they CAN. So (imo) we MUST support that (or very clearly document that this isn't allowed, and vet that we don't do this in coreth / subnet-evm).
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I iterated on this for awhile and eventually realized the thing I was asserting wasn't even checking for the bug I was trying to prevent. I do think it could be useful to test that the receiver == the provided arg... But really the concern with this change is that we need to put the block extra (without a copy) as the body extra so that marshal and unmarshal works correctly. By switching just to this I was able to reuse I had initially considered adding this test as part of |
||
| p := rlpBodyPayloads.Get(b) | ||
|
StephenButtolph marked this conversation as resolved.
Outdated
|
||
| return &rlp.Fields{ | ||
| Required: []any{b.Transactions, b.Uncles, p.Data}, | ||
| Optional: []any{b.Withdrawals}, | ||
| } | ||
| } | ||
|
|
||
| func (*bodyPayload) BodyRLPFieldPointersForDecoding(b *Body) *rlp.Fields { | ||
| // See above comment on why we access the receiver through b rather than | ||
| // directly. | ||
|
StephenButtolph marked this conversation as resolved.
Outdated
|
||
| p := rlpBodyPayloads.Get(b) | ||
| return &rlp.Fields{ | ||
| Required: []any{&b.Transactions, &b.Uncles, &p.Data}, | ||
| Optional: []any{&b.Withdrawals}, | ||
| } | ||
| } | ||
|
|
||
| // TestBodyExtraRoundTrip demonstrates that the extra from the method receiver | ||
| // is the same as the extra from the argument for [types.BlockBodyHooks] | ||
| // functions. | ||
| func TestBodyExtraRoundTrip(t *testing.T) { | ||
| TestOnlyClearRegisteredExtras() | ||
| t.Cleanup(TestOnlyClearRegisteredExtras) | ||
|
|
||
| extras := RegisterExtras[ | ||
| NOOPHeaderHooks, *NOOPHeaderHooks, | ||
| bodyPayload, *bodyPayload, | ||
| struct{}, | ||
| ]() | ||
| rlpBodyPayloads = extras.Body | ||
|
|
||
| rng := ethtest.NewPseudoRand(142857) | ||
| wantBlock := NewBlock( | ||
| &Header{ParentHash: rng.Hash()}, | ||
| []*Transaction{ | ||
| NewTx(&LegacyTx{Nonce: rng.Uint64()}), | ||
| }, | ||
| []*Header{ | ||
| {ParentHash: rng.Hash()}, | ||
| }, | ||
| nil, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why no receipts?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I replaced |
||
| trie.NewStackTrie(nil), | ||
| ) | ||
| want := extras.Block.Get(wantBlock) | ||
| want.Data = rng.Bytes(8) | ||
|
|
||
| b, err := rlp.EncodeToBytes(wantBlock) | ||
| require.NoErrorf(t, err, "rlp.EncodeToBytes(%T)", wantBlock) | ||
|
|
||
| gotBlock := new(Block) | ||
| require.NoErrorf(t, rlp.DecodeBytes(b, gotBlock), "rlp.DecodeBytes(rlp.EncodeToBytes(%T), %T)", wantBlock, gotBlock) | ||
| got := extras.Block.Get(gotBlock) | ||
| assert.Equalf(t, want, got, "%T payload after RLP round trip", got) | ||
| } | ||
|
|
||
| // newHeader returns a [Header] with randomly populated fields. | ||
| func newHeader(rng *ethtest.PseudoRand) *Header { | ||
| return &Header{ | ||
| ParentHash: rng.Hash(), | ||
| UncleHash: rng.Hash(), | ||
| Coinbase: rng.Address(), | ||
| Root: rng.Hash(), | ||
| TxHash: rng.Hash(), | ||
| ReceiptHash: rng.Hash(), | ||
| Bloom: rng.Bloom(), | ||
| Difficulty: rng.BigUint64(), | ||
| Number: rng.BigUint64(), | ||
| GasLimit: rng.Uint64(), | ||
| GasUsed: rng.Uint64(), | ||
| Time: rng.Uint64(), | ||
| Extra: rng.Bytes(32), | ||
| MixDigest: rng.Hash(), | ||
| Nonce: rng.BlockNonce(), | ||
| BaseFee: rng.BigUint64(), | ||
| } | ||
| } | ||
|
|
||
| // bodySize describes the number of items in the [Body] of a test case. | ||
| type bodySize struct { | ||
| txs, uncles, withdrawals int | ||
| } | ||
|
|
||
| // bodySizes are the [Body] shapes covered by [FuzzBlockBytes] seeds and by | ||
| // [BenchmarkBlockBytes]. They cover every combination of empty and non-empty | ||
| // fields, the last of which is optional in RLP. | ||
| var bodySizes = []bodySize{ | ||
| {txs: 0, uncles: 0, withdrawals: 0}, | ||
| {txs: 1, uncles: 0, withdrawals: 0}, | ||
| {txs: 0, uncles: 1, withdrawals: 0}, | ||
| {txs: 0, uncles: 0, withdrawals: 1}, | ||
| {txs: 10, uncles: 0, withdrawals: 0}, | ||
| {txs: 10, uncles: 2, withdrawals: 4}, | ||
| {txs: 100, uncles: 0, withdrawals: 0}, | ||
| {txs: 100, uncles: 2, withdrawals: 16}, | ||
| } | ||
|
|
||
| // newBody returns a [Body] with randomly populated fields, holding the number | ||
| // of items described by `size`. | ||
| func newBody(rng *ethtest.PseudoRand, size bodySize) *Body { | ||
| body := &Body{} | ||
| for range size.txs { | ||
| body.Transactions = append(body.Transactions, NewTx(&LegacyTx{ | ||
| Nonce: rng.Uint64(), | ||
| GasPrice: rng.BigUint64(), | ||
| Gas: rng.Uint64(), | ||
| To: rng.AddressPtr(), | ||
| Value: rng.BigUint64(), | ||
| Data: rng.Bytes(64), | ||
| })) | ||
| } | ||
| for range size.uncles { | ||
| body.Uncles = append(body.Uncles, newHeader(rng)) | ||
| } | ||
| for range size.withdrawals { | ||
| body.Withdrawals = append(body.Withdrawals, &Withdrawal{ | ||
| Index: rng.Uint64(), | ||
| Validator: rng.Uint64(), | ||
| Address: rng.Address(), | ||
| Amount: rng.Uint64(), | ||
| }) | ||
| } | ||
|
StephenButtolph marked this conversation as resolved.
|
||
| return body | ||
| } | ||
|
|
||
| // encodeRLP RLP-encodes `v`, failing the test if it can't be encoded. | ||
| func encodeRLP(tb testing.TB, v any) []byte { | ||
| tb.Helper() | ||
| b, err := rlp.EncodeToBytes(v) | ||
| require.NoErrorf(tb, err, "rlp.EncodeToBytes(%T)", v) | ||
| return b | ||
| } | ||
|
|
||
| // referenceBlockBytes is the reference implementation against which | ||
| // [BlockBytes] is tested and benchmarked. | ||
| func referenceBlockBytes(headerBytes, bodyBytes []byte) ([]byte, error) { | ||
| header := new(Header) | ||
| if err := rlp.DecodeBytes(headerBytes, header); err != nil { | ||
| return nil, err | ||
| } | ||
| body := new(Body) | ||
| if err := rlp.DecodeBytes(bodyBytes, body); err != nil { | ||
| return nil, err | ||
| } | ||
| block := NewBlockWithHeader(header). | ||
| WithBody(*body). | ||
| WithWithdrawals(body.Withdrawals) | ||
| return rlp.EncodeToBytes(block) | ||
| } | ||
|
|
||
| // FuzzBlockBytes demonstrates that [BlockBytes] is equivalent to | ||
| // [referenceBlockBytes] for all inputs that the latter accepts. The seed corpus | ||
| // covers every shape in [bodySizes]. | ||
| func FuzzBlockBytes(f *testing.F) { | ||
| rng := ethtest.NewPseudoRand(20250806) | ||
| for _, size := range bodySizes { | ||
| f.Add( | ||
| encodeRLP(f, newHeader(rng)), | ||
| encodeRLP(f, newBody(rng, size)), | ||
| ) | ||
| } | ||
|
|
||
| f.Fuzz(func(t *testing.T, headerBytes, bodyBytes []byte) { | ||
| want, err := referenceBlockBytes(headerBytes, bodyBytes) | ||
| if err != nil { | ||
| t.Skip("invalid input bytes") | ||
| } | ||
|
|
||
| got, err := BlockBytes(headerBytes, bodyBytes) | ||
| require.NoError(t, err, "BlockBytes()") | ||
| assert.Equal(t, want, got, "referenceBlockBytes() == BlockBytes()") | ||
| }) | ||
| } | ||
|
|
||
| func BenchmarkBlockBytes(b *testing.B) { | ||
| for _, size := range bodySizes { | ||
| rng := ethtest.NewPseudoRand(2718281828) | ||
| headerBytes := encodeRLP(b, newHeader(rng)) | ||
| bodyBytes := encodeRLP(b, newBody(rng, size)) | ||
| b.Run(fmt.Sprintf("%d_txs_%d_uncles_%d_withdrawals", size.txs, size.uncles, size.withdrawals), func(b *testing.B) { | ||
| for b.Loop() { | ||
| _, _ = BlockBytes(headerBytes, bodyBytes) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was really the annoying part. It's kind of weird, since I don't think an implementation should actually use this... But I think we should guarantee that it is provided correctly. Thoughts on simplifying this @ARR4N?