execution: eradicate block.SetBlockAccessList

[taratorio]

• removes block.SetBlockAccessList in favour of always passing it via block constructors as we do for withdrawals, transactions, header, etc
• we can now also simplify our API by removing bal from execModule.InsertBlocks because they are now attached to the block after execution: carry the block access list as a first-class block object #22894

[Copilot]

Pull request overview

This PR refactors block insertion and block construction to fully eliminate block.SetBlockAccessList, making the EIP-7928 Block Access List (BAL) a constructor-provided sidecar (similar to withdrawals/txs/header) and simplifying insertion APIs by removing separate BAL plumbing.

Changes:

• Switch ExecutionModule.InsertBlocks (and callers) from []*types.RawBlock to []*types.Block, removing separate BAL arguments/feeds.
• Update block constructors (NewBlockFromNetwork, NewBlockFromStorage*, NewBlockForAsembling) to accept BAL bytes directly, and remove SetBlockAccessList / RawBlock.BlockAccessList.
• Propagate the signature changes through Engine API, P2P downloader paths, Polygon sync, and tests/mocks.

Reviewed changes

Copilot reviewed 42 out of 43 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
execution/types/block.go	Removes SetBlockAccessList/RawBlock.BlockAccessList, renames the sidecar field to bal, and threads BAL into constructors.
execution/execmodule/interface.go	Updates the exec module interface to accept []*types.Block for insertion.
execution/execmodule/inserters.go	Adjusts insertion logic to persist BAL bytes sourced from the types.Block object.
execution/execmodule/chainreader/chain_reader.go	Simplifies chain reader insertion API to no longer accept separate BAL slices.
execution/p2p/bbd*.go	Removes BAL side-channel from downloader result feed and attaches BAL directly to blocks when available.
execution/engineapi/*	Updates Engine API insertion paths and related tests to match the new insertion/block-construction model.
execution/builder/finish.go	Encodes BAL bytes for Amsterdam+ and passes them via block assembly constructor.
db/rawdb/accessors_chain.go	Reconstructs blocks from DB while carrying BAL bytes as a block sidecar.
polygon/*, cl/*, cmd/*, execution/tests/*	Mechanical updates to constructor signatures and insertion APIs across integrations and tests.

Files not reviewed (1)

• cl/phase1/execution_client/execution_engine_mock.go: Generated file

Suppressed comments (3)

execution/types/block.go:1176

• bal is stored by reference. To preserve the previous non-aliasing guarantee, clone bal when constructing the block.
  execution/types/block.go:1195
• bal is stored by reference. This can alias request/network buffers; cloning here matches the old SetBlockAccessList behavior and prevents post-construction mutation from affecting the block.
  execution/types/block.go:1168
• bal is stored by reference. To avoid callers mutating the provided slice after block construction (the old SetBlockAccessList explicitly cloned), store a clone in the block.

---

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

[AskAlexSharov]

Reviewed the whole diff. Looks good — this is a real simplification, not just a signature shuffle. One point I would like you to consider before merge, plus two nits. Nothing blocking.

What this fixes beyond the stated goal

Attaching the BAL to the block removes two parallel-collection alignments that had to stay in sync by hand:

• execution/p2p/bbd.go built bals[len(blocks)-1] = bal while iterating nested batches, so the BAL slice index had to track the flattened block index across batch boundaries.
• cl/.../persistent_block_collector.go kept a balByHash map for the whole Flush and re-projected it into bals[i] per batch, while blocks in between could be dropped by the min-height filter or the pending look-ahead.

Both are now "the block carries its own BAL". That is strictly harder to get wrong.

Main point: the bytes.Clone that tidy removed

SetBlockAccessList cloned, and said why:

// SetBlockAccessList attaches the RLP-encoded BAL sidecar to the block, copying
// the input so a transaction-owned or later-mutated source cannot alias it.

Commit b62445d kept that clone inside the constructors; tidy (5eb1edc) dropped it and instead added an explicit bytes.Clone at the ReadBlock call site (db/rawdb/accessors_chain.go:817).

That one clone is load-bearing. ReadBlockAccessListBytes returns db.GetOne(...) unchanged, so the bytes point into MDBX-owned memory that is only valid for the tx. A *Block that outlives the tx would carry a dangling BAL.

I checked every call site on the branch and none of them is broken today: ByteListSSZ.Bytes() already clones (both CL paths), engine_server's *req.BlockAccessList is per-request JSON, bbd.go's comes from a decoded response, finish.go's is freshly encoded, and ReadBlock now clones. So this is not a live bug.

My concern is only that the invariant moved from "enforced once in the callee" to "must be remembered at each call site", and the codebase already contains the one source that needs it. Two ways out, your call:

1. Put bytes.Clone back in NewBlockFromStorage / NewBlockFromStorageWithBinaryTxs (execution/types/block.go:1165,1173) — the two that are documented as the read-from-DB constructors.
2. Keep it as is and note on the bal param that the caller must pass memory it owns.

I can see the argument for leaving it: txs, uncles and withdrawals are not copied either, and NewBlockFromStorage says "no reason to copy parts". The difference is that those are decoded structures the caller already owns, while bal is the one param that can be raw MDBX bytes.

Nits

execution/execmodule/inserters.go:103 — body := block.RawBody() runs before the frozen-block skip at line 106, so every frozen block gets its transactions RLP-encoded into a fresh [][]byte that is then discarded. Same waste existed on main inside blocksToRaw, so this is not a regression, but the code just moved here and moving the call below the continue is free.

execution/execmodule/inserters.go:102 — dropping the b.Header() deep copy is a nice win. One side effect: CopyHeader returns mutable=false, so Header.Hash() cached; HeaderNoCopy() on a block from NewBlockForAsembling returns mutable=true, and Hash() skips the cache for mutable headers. The loop calls header.Hash() four times per block, so assembled blocks now do four RlpHash runs instead of one. Only the builder / testing-API path makes mutable blocks, so the impact is small — hoisting hash := header.Hash() once covers it.

One behaviour change worth confirming is intended

testing_api.go CommitBlockV1 used to encode assembled.Block.BlockAccessList with no fork gate and pass it to InsertBlock. It now relies on finishBlock, which only attaches the BAL when IsEIPEnabled(7928, header.Time).

That looks like an improvement rather than a regression: on a pre-Amsterdam chain with ExperimentalBAL, finishBlock computes the BAL but deliberately leaves BlockAccessListHash nil, so the old path fed a non-empty BAL into inserters.go and hit block access list provided without hash. The new path passes nil there and the error goes away. Just flagging it so it is a decision and not a side effect.

Checked and fine

• RawBlock.EncodingSize() never counted the BAL, so removing the field does not move ValidateMaxRlpSize.
• RawBlock.AsBlock() no longer propagates a BAL; its only caller is polygon/bridge/service_test.go, and polygon has no BAL.
• nil in freezeblocks/block_reader.go and blocks_read_ahead.go is right — BALs are not in snapshots.
• Builds clean; CI is 129 green / 5 skipped.

[taratorio]

Reviewed the whole diff. Looks good — this is a real simplification, not just a signature shuffle. One point I would like you to consider before merge, plus two nits. Nothing blocking.

What this fixes beyond the stated goal

Attaching the BAL to the block removes two parallel-collection alignments that had to stay in sync by hand:

• execution/p2p/bbd.go built bals[len(blocks)-1] = bal while iterating nested batches, so the BAL slice index had to track the flattened block index across batch boundaries.
• cl/.../persistent_block_collector.go kept a balByHash map for the whole Flush and re-projected it into bals[i] per batch, while blocks in between could be dropped by the min-height filter or the pending look-ahead.

Both are now "the block carries its own BAL". That is strictly harder to get wrong.

Main point: the bytes.Clone that tidy removed

SetBlockAccessList cloned, and said why:

// SetBlockAccessList attaches the RLP-encoded BAL sidecar to the block, copying
// the input so a transaction-owned or later-mutated source cannot alias it.

Commit b62445d kept that clone inside the constructors; tidy (5eb1edc) dropped it and instead added an explicit bytes.Clone at the ReadBlock call site (db/rawdb/accessors_chain.go:817).

That one clone is load-bearing. ReadBlockAccessListBytes returns db.GetOne(...) unchanged, so the bytes point into MDBX-owned memory that is only valid for the tx. A *Block that outlives the tx would carry a dangling BAL.

I checked every call site on the branch and none of them is broken today: ByteListSSZ.Bytes() already clones (both CL paths), engine_server's *req.BlockAccessList is per-request JSON, bbd.go's comes from a decoded response, finish.go's is freshly encoded, and ReadBlock now clones. So this is not a live bug.

My concern is only that the invariant moved from "enforced once in the callee" to "must be remembered at each call site", and the codebase already contains the one source that needs it. Two ways out, your call:

1. Put bytes.Clone back in NewBlockFromStorage / NewBlockFromStorageWithBinaryTxs (execution/types/block.go:1165,1173) — the two that are documented as the read-from-DB constructors.
2. Keep it as is and note on the bal param that the caller must pass memory it owns.

I can see the argument for leaving it: txs, uncles and withdrawals are not copied either, and NewBlockFromStorage says "no reason to copy parts". The difference is that those are decoded structures the caller already owns, while bal is the one param that can be raw MDBX bytes.

Nits

execution/execmodule/inserters.go:103 — body := block.RawBody() runs before the frozen-block skip at line 106, so every frozen block gets its transactions RLP-encoded into a fresh [][]byte that is then discarded. Same waste existed on main inside blocksToRaw, so this is not a regression, but the code just moved here and moving the call below the continue is free.

execution/execmodule/inserters.go:102 — dropping the b.Header() deep copy is a nice win. One side effect: CopyHeader returns mutable=false, so Header.Hash() cached; HeaderNoCopy() on a block from NewBlockForAsembling returns mutable=true, and Hash() skips the cache for mutable headers. The loop calls header.Hash() four times per block, so assembled blocks now do four RlpHash runs instead of one. Only the builder / testing-API path makes mutable blocks, so the impact is small — hoisting hash := header.Hash() once covers it.

One behaviour change worth confirming is intended

testing_api.go CommitBlockV1 used to encode assembled.Block.BlockAccessList with no fork gate and pass it to InsertBlock. It now relies on finishBlock, which only attaches the BAL when IsEIPEnabled(7928, header.Time).

That looks like an improvement rather than a regression: on a pre-Amsterdam chain with ExperimentalBAL, finishBlock computes the BAL but deliberately leaves BlockAccessListHash nil, so the old path fed a non-empty BAL into inserters.go and hit block access list provided without hash. The new path passes nil there and the error goes away. Just flagging it so it is a decision and not a side effect.

Checked and fine

• RawBlock.EncodingSize() never counted the BAL, so removing the field does not move ValidateMaxRlpSize.
• RawBlock.AsBlock() no longer propagates a BAL; its only caller is polygon/bridge/service_test.go, and polygon has no BAL.
• nil in freezeblocks/block_reader.go and blocks_read_ahead.go is right — BALs are not in snapshots.
• Builds clean; CI is 129 green / 5 skipped.

yes, the bytes.Clone was removed on purpose
there is only 1 place where we need to clone and that is inside https://github.com/erigontech/erigon/pull/23060/changes#diff-7714b74f0e7da029d3bfc5a1d9ba4ce717f2d8fc6a26dbf4182a7a2270d37f9eL817 because it is inside a mdbx txn - instead of always doing it in the constructor we do it just in that 1 place where it is needed

[taratorio]

will address the other nits