Remove Block RLP hooks by StephenButtolph · Pull Request #304 · ava-labs/libevm

StephenButtolph · 2026-08-06T19:08:21Z

Why this should be merged

While I do think it's nice to reduce the number of required hooks to implement. The primary point of this PR is introducing an efficient types.BlockBytes function that efficiently combines header bytes and body bytes into block bytes.

Technically, this BlockBytes function doesn't need to live in libevm (I could implement it in SAE) - but I think that it makes sense to solidify the hook implementations against a concrete requirement.

benchstat: referenceBlockBytes (reencode.txt) vs BlockBytes (blockbytes.txt)

goos: darwin
goarch: arm64
pkg: github.com/ava-labs/libevm/core/types
cpu: Apple M2 Max
                                              │  reencode.txt  │            blockbytes.txt            │
                                              │     sec/op     │    sec/op     vs base                │
BlockBytes/0_txs_0_uncles_0_withdrawals-12       2618.5n ± 58%   201.6n ±  5%  -92.30% (p=0.000 n=10)
BlockBytes/1_txs_0_uncles_0_withdrawals-12       4069.0n ± 18%   237.3n ± 12%  -94.17% (p=0.000 n=10)
BlockBytes/0_txs_1_uncles_0_withdrawals-12       4823.5n ±  5%   349.1n ±  6%  -92.76% (p=0.000 n=10)
BlockBytes/0_txs_0_uncles_1_withdrawals-12       3167.5n ±  1%   211.6n ±  6%  -93.32% (p=0.000 n=10)
BlockBytes/10_txs_0_uncles_0_withdrawals-12     11912.5n ±  6%   536.8n ± 11%  -95.49% (p=0.000 n=10)
BlockBytes/10_txs_2_uncles_4_withdrawals-12     16616.5n ±  6%   727.9n ± 12%  -95.62% (p=0.000 n=10)
BlockBytes/100_txs_0_uncles_0_withdrawals-12     98.445µ ±  9%   2.546µ ± 11%  -97.41% (p=0.000 n=10)
BlockBytes/100_txs_2_uncles_16_withdrawals-12   101.776µ ± 24%   3.007µ ± 13%  -97.05% (p=0.000 n=10)
geomean                                          11.58µ          566.3n        -95.11%

                                              │  reencode.txt  │            blockbytes.txt            │
                                              │      B/op      │     B/op       vs base               │
BlockBytes/0_txs_0_uncles_0_withdrawals-12        3421.0 ± 0%     640.0 ± 0%   -81.29% (p=0.000 n=10)
BlockBytes/1_txs_0_uncles_0_withdrawals-12        4143.0 ± 0%     768.0 ± 0%   -81.46% (p=0.000 n=10)
BlockBytes/0_txs_1_uncles_0_withdrawals-12       6.217Ki ± 0%   1.250Ki ± 0%   -79.89% (p=0.000 n=10)
BlockBytes/0_txs_0_uncles_1_withdrawals-12        3638.0 ± 0%     640.0 ± 0%   -82.41% (p=0.000 n=10)
BlockBytes/10_txs_0_uncles_0_withdrawals-12      9.714Ki ± 0%   2.000Ki ± 0%   -79.41% (p=0.000 n=10)
BlockBytes/10_txs_2_uncles_4_withdrawals-12     15.417Ki ± 0%   3.376Ki ± 0%   -78.10% (p=0.000 n=10)
BlockBytes/100_txs_0_uncles_0_withdrawals-12     66.85Ki ± 0%   13.26Ki ± 0%   -80.17% (p=0.000 n=10)
BlockBytes/100_txs_2_uncles_16_withdrawals-12    75.21Ki ± 0%   16.01Ki ± 0%   -78.72% (p=0.000 n=10)
geomean                                          11.07Ki        2.188Ki        -80.23%

                                              │  reencode.txt  │            blockbytes.txt            │
                                              │   allocs/op    │  allocs/op     vs base               │
BlockBytes/0_txs_0_uncles_0_withdrawals-12        42.000 ± 0%     1.000 ± 0%   -97.62% (p=0.000 n=10)
BlockBytes/1_txs_0_uncles_0_withdrawals-12        56.000 ± 0%     1.000 ± 0%   -98.21% (p=0.000 n=10)
BlockBytes/0_txs_1_uncles_0_withdrawals-12        62.000 ± 0%     1.000 ± 0%   -98.39% (p=0.000 n=10)
BlockBytes/0_txs_0_uncles_1_withdrawals-12        49.000 ± 0%     1.000 ± 0%   -97.96% (p=0.000 n=10)
BlockBytes/10_txs_0_uncles_0_withdrawals-12      161.000 ± 0%     1.000 ± 0%   -99.38% (p=0.000 n=10)
BlockBytes/10_txs_2_uncles_4_withdrawals-12      210.000 ± 0%     1.000 ± 0%   -99.52% (p=0.000 n=10)
BlockBytes/100_txs_0_uncles_0_withdrawals-12    1163.000 ± 0%     1.000 ± 0%   -99.91% (p=0.000 n=10)
BlockBytes/100_txs_2_uncles_16_withdrawals-12   1230.000 ± 0%     1.000 ± 0%   -99.92% (p=0.000 n=10)
geomean                                           155.7           1.000        -99.36%

Block reencoding showed up as a significant performance bottleneck while serving blocks. Reencoding currently happens when calling rawdb.ReadBlock. By implementing this change the GetAncestors optimization observes:

benchstat: GetAncestors optimization in avalanchego before & after this change

                        │      old      │                 new                  │
                        │    sec/op     │    sec/op     vs base                │
GetAncestors/batched-12   16223.1µ ± 5%   655.8µ ± 17%  -95.96% (p=0.000 n=10)

                        │     B/op      │     B/op      vs base                │
GetAncestors/batched-12   14.247Mi ± 0%   4.008Mi ± 0%  -71.87% (p=0.000 n=10)

                        │   allocs/op   │  allocs/op   vs base                │
GetAncestors/batched-12   251.167k ± 0%   8.277k ± 0%  -96.70% (p=0.000 n=10)

How this works

While the goal of this PR was adding BlockBytes - most of the effort fell to removing BlockRLPFieldsForEncoding and BlockRLPFieldPointersForDecoding.

These functions needed to be removed because they allowed a hook provider to break the Header + Body = Block assumption. There was nothing stopping someone from re-ordering body fields when they were serialized just as a body vs when they were serialized as part of a block.

This PR re-uses BodyRLPFieldsForEncoding and BodyRLPFieldPointersForDecoding so that we are guaranteed that the body bytes follow the same order as the block bytes.

How this was tested

Added fuzz test and benchmarks for BlockBytes
Extended testing to assert extras equvilance.

StephenButtolph · 2026-08-06T19:37:03Z

+		// The body provided to the hooks is expected to contain the same extra
+		// as the method receiver.
+		extra: b.extra,


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?

…a-labs/libevm into StephenButtolph/remove-block-rlp-hook

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Updates block/body RLP encoding to route extras through Body hooks (removing the block-specific proxy), and adds a fast path to combine RLP-encoded header/body into block bytes.

Changes:

Replace BlockRLPProxy-based block encoding/decoding with Body hook-driven field assembly.
Introduce BlockBytes(headerBytes, bodyBytes) fast-path and add fuzz/bench coverage for equivalence/performance.
Update tests and temporary extras to use Body hooks and payload access patterns.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
core/types/tempextras.libevm_test.go	Updates temporary extras test to set/get body extras via `extras.Body` and `BodyRLPFieldsForEncoding`.
core/types/rlp_payload.libevm.go	Adds `(*Block).extraOrNil()` helper to avoid allocating extras when unregistered.
core/types/block.libevm_test.go	Adds body-extra roundtrip test plus fuzz/bench coverage for the new `BlockBytes` constructor.
core/types/block.libevm.go	Reworks block RLP encoding/decoding to use `Body` hook fields; adds `BlockBytes`.
core/types/block.go	Replaces `extblock.hooks` with `extblock.extra` and wires encode/decode through `extraOrNil()`.
core/types/backwards_compat.libevm_test.go	Removes now-obsolete `BlockRLP*` hook methods from compat extras test type.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

ARR4N · 2026-08-07T10:33:40Z

+	// 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.


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 pseudo.Accessor.

Is there a specific reason you'd want a hook to access its payload via the carrying struct?

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 pseudo.Accessor.

I agree. This test is specifically ensuring that a "non-idiomatic" implementation works.

Is there a specific reason you'd want a hook to access its payload via the carrying struct?

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).

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 blockPayload and reduce the testing surface.

I had initially considered adding this test as part of TestBlockWithX (and instead making that TestBlockHooks) - but I feel like that would be missing a bunch of coverage.

ARR4N · 2026-08-07T10:35:31Z

+		[]*Header{
+			{ParentHash: rng.Hash()},
+		},
+		nil,


Why no receipts?

I replaced NewBlock with NewBlockWithHeader. I had originally not provided receipts because they don't impact the serialized format... But this test doesn't really care about any of the fields (other than the extras) - so I just made it as minimal as I could.

StephenButtolph · 2026-08-07T17:36:16Z

 	]()

-	typ := reflect.TypeOf(&Block{})
+	typ := reflect.TypeFor[*Block]()


Not required - but my IDE lints these... So figured I might as well modernize this while I was here.

StephenButtolph · 2026-08-07T17:42:50Z

+	// 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.


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 blockPayload and reduce the testing surface.

I had initially considered adding this test as part of TestBlockWithX (and instead making that TestBlockHooks) - but I feel like that would be missing a bunch of coverage.

StephenButtolph added 11 commits August 6, 2026 12:18

AI slop

c96078f

wip simplify

17a2dbc

wip simplify

0331e83

wip simplify

9516b73

wip

d97b5b7

nit

332ff9b

nit

54e9ebb

nit

e2ff812

nit

c63a15e

nit

03c95af

nits

9e05728

StephenButtolph commented Aug 6, 2026

View reviewed changes

StephenButtolph added 7 commits August 6, 2026 16:03

Merge branch 'main' into StephenButtolph/remove-block-rlp-hook

50ec497

nit

76e0af5

Merge branch 'StephenButtolph/remove-block-rlp-hook' of github.com:av…

c681534

…a-labs/libevm into StephenButtolph/remove-block-rlp-hook

nit

8f27da7

nit

73c8af6

nit

d9378ad

nit

b9fdfd4

StephenButtolph marked this pull request as ready for review August 6, 2026 20:58

StephenButtolph requested review from ARR4N and Copilot August 6, 2026 21:00

StephenButtolph self-assigned this Aug 6, 2026

StephenButtolph added the Category: Performance 🚀 label Aug 6, 2026

Copilot AI reviewed Aug 6, 2026

View reviewed changes

Comment thread core/types/block.libevm_test.go

Comment thread core/types/block.libevm.go Outdated

Comment thread core/types/block.libevm.go Outdated

Comment thread core/types/block.libevm.go

Comment thread core/types/block.libevm_test.go Outdated

rename

dbdbcfa

ARR4N reviewed Aug 7, 2026

View reviewed changes

StephenButtolph added 3 commits August 7, 2026 09:16

add comment

ececab1

reuse helpers

161b4eb

Stop using NewBlock

fea5b35

simplify

c7aa4e5

StephenButtolph commented Aug 7, 2026

View reviewed changes

prefer typed values

d34e7ce

Navigation Menu

Uh oh!

Remove Block RLP hooks - #304

Remove Block RLP hooks#304
StephenButtolph wants to merge 24 commits into
mainfrom
StephenButtolph/remove-block-rlp-hook

StephenButtolph commented Aug 6, 2026 •

edited

Loading

Uh oh!

StephenButtolph Aug 6, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ARR4N Aug 7, 2026

Uh oh!

StephenButtolph Aug 7, 2026

Uh oh!

StephenButtolph Aug 7, 2026

Uh oh!

Uh oh!

ARR4N Aug 7, 2026

Uh oh!

StephenButtolph Aug 7, 2026

Uh oh!

StephenButtolph Aug 7, 2026

Uh oh!

StephenButtolph Aug 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

StephenButtolph commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why this should be merged

benchstat: referenceBlockBytes (reencode.txt) vs BlockBytes (blockbytes.txt)

benchstat: GetAncestors optimization in avalanchego before & after this change

How this works

How this was tested

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

StephenButtolph commented Aug 6, 2026 •

edited

Loading