erigontech / erigontech/erigon

execmodule: send notifications before DB commit

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

Description

## Summary

Move `Hook.AfterRun` calls to fire **before** flush+commit in the FCU and ProcessFrozenBlocks paths. Currently notifications are sent after `tx.Commit()` via a fresh `db.View()`, adding unnecessary latency and coupling to committed DB state.

## Background

The original reason for post-commit notifications (comment in `events.go:276`): *"need send notification after rwtx.Commit (or user will recv notification, but can't request new data by RPC)"*.

Investigation confirms this concern is no longer valid:

| Consumer | Reads DB after notification? |
|----------|----------------------------|
| Filters.onNewHeader | No — decodes from RLP payload |
| RecentReceipts subscribers | No — uses payload proto |
| Coherent.OnNewBlock | No — updates in-memory btree from payload |
| EthBackendServer.Subscribe | No — forwards to gRPC stream |

All notification consumers use the payload data directly and never read back from the DB.

## Change

The pipeline's RwTx/overlay already contains all data that notifications read (headers, stage progress, forkchoice hashes, state version). Pass it directly to `AfterRun` before flush+commit.

**Two sites changed:**
- `runForkchoiceCommit` (forkchoice.go) — AfterRun moved before `sd.Flush` + `tx.Commit`
- `ProcessFrozenBlocks` (stageloop.go) — AfterRun moved before `tx.Commit`, `db.View()` removed

**Two sites already pre-commit (unchanged):**
- `stageLoopIteration` — already uses the RwTx before commit
- `ProcessFrozenBlocks` intermediate loop — already uses the RwTx

## Relationship to other work

- **#19623** (2-cache IBS rationalization): After Phase 3, SharedDomains becomes the single authoritative inter-block state. This change aligns the notification path with that direction — reads go through the in-memory layer rather than requiring a committed DB tx.
- **#19798** (event stream extraction): The `Accumulator.SendAndReset()` call moved pre-commit will be restructured into a fan-out `notifyConsumer`.
- **#19855** (TransactionState/BlockState separation): Interface changes there will further simplify the notification path.

Also documents the `execmodule.Cache` shim's role and relationship to these issues.

## Branch

`feat/pre-commit-notifications` — not ready to PR yet, waiting for dependent work.

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.