erigontech / erigontech/erigon

execution/stagedsync: remove MemoryBatch from MiningStep — use TemporalTx throughout builder pipeline

Open
#19,822 1 comment 0 reactions 1 assignee Claimed by @mh0lt View on GitHub
performance tech debt reduction
Dominant language
Go
Stars
3.6k
Forks
1.5k
Avg merge
1d 16h
Merged PRs (30d)
455

Description

## Background

`MiningStep` (in `execution/stagedsync/stageloop/stageloop.go`) builds a new block by running the builder stage pipeline. It has historically opened a `BeginTemporalRw` (MDBX exclusive write transaction) as the base tx, wrapped it in a `MemoryBatch` overlay, and always rolled back the base tx when done. The write transaction was never needed — all actual writes from the mining stages go into the `MemoryBatch` in-memory buffer, which is discarded without flushing.

PR #19821 fixed the immediate symptom — switching `BeginTemporalRw` to `BeginTemporalRo` to stop holding the MDBX exclusive write lock during block builds. However, `MemoryBatch` is still present purely as a compatibility shim to satisfy the `kv.TemporalRwTx` type required by `Sync.Run` and `ExecFunc`.

## Why MemoryBatch is now unnecessary

The builder stage pipeline (`builderstages`) was refactored so that all state writes go through `SharedDomains`, not through the underlying `kv.TemporalRwTx`. Inspection of the builder stages confirms:

- `SpawnBuilderCreateBlockStage` — only reads from `tx` (`rawdb.ReadHeaderByNumber`, `rawdb.ReadHeadersByNumber`)
- `SpawnBuilderExecStage` — only reads from `tx` (`rawdb.ReadHeader`)
- `StageExecuteBlocksCfg` (forward path) — writes state exclusively through `SharedDomains`

No builder stage writes directly to `tx`. `MemoryBatch` exists only to provide a `kv.TemporalRwTx`-shaped object to `Sync.Run`.

## Proposed fix

Broaden the interfaces in the stage pipeline so the builder path can use a plain read-only tx:

1. **`ExecFunc` / `UnwindFunc`** (`execution/stagedsync/stage.go`): change `rwTx kv.TemporalRwTx` → `tx kv.TemporalTx`
2. **`Sync.Run`, `Sync.RunUnwind`, `Sync.RunNoInterrupt`, `Sync.RunSnapshots`** (`execution/stagedsync/sync.go`): change `tx kv.TemporalRwTx` → `tx kv.TemporalTx`
3. **Non-builder stages** that write to `tx` directly: add an internal cast `rwTx := tx.(kv.TemporalRwTx)` — these are only ever called with a real write tx from `StageLoop`/`ProcessFrozenBlocks`
4. **`MiningStep`**: remove `MemoryBatch`, pass `kv.TemporalTx` (from `BeginTemporalRo`) directly to `Sync.Run`

Existing callers of `Sync.Run` that pass a `kv.TemporalRwTx` continue to work unchanged since `TemporalRwTx` satisfies `TemporalTx`.

## Scope

~84 occurrences of `kv.TemporalRwTx` in stage signatures across `execution/`. Stages that need write access would cast internally. A good first step would be to identify and enumerate which stages actually write to `tx` vs which only read, to minimise unsafe casts.

## Related

- PR #19821 — immediate fix (BeginTemporalRw → BeginTemporalRo, MemoryBatch retained as shim)

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.