erigontech / erigontech/erigon

Remove TxNum & BlockNum from SharedDomains

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

Description

As a follow up from the race conditon caused by setting txnum in execution at the wrong place I have done the following background research and would like to remove both of these fileds from shared domains to avoid this possibility happening again.

The summary impact assessment is that this is a low-risk mechanical refactor. The field is not structurally load-bearing — it's a counter stored in a convenient place. Every caller that reads it already has the same value available locally. The only tricky part is the stateCache step derivation, which would need a small interface adjustment.

Please see the details below:

## TxNum

1. Bookkeeping / progress tracking (TxNum())

Used by ~30 callers to answer "where are we in execution?":

  • exec3.go: Determining resume point after restart (inputTxNum = doms.TxNum())
  • exec3.go: Computing commitment (ComputeCommitment(..., doms.TxNum(), ...))
  • exec3.go: Building files in background (agg.BuildFilesInBackground(doms.TxNum()))
  • exec3.go: Tracking last committed txNum
  • SeekCommitment: Finding where to resume
  • stage_mining_exec.go: Getting current txNum for writer
  • RPC receipt generator: Computing state roots
  • Fork validator: Merging domains
  • Squeeze: Tracking progress

2. Step derivation (currentStep)

Only used in one place — domain_shared.go:417:

if v, ok := sd.stateCache.Get(domain, k); ok {

return v, sd.currentStep, nil // <-- here
}

The stateCache hit returns currentStep (derived from txNum/stepSize) as the step for the cached value. This is an approximation — the actual step could be different, but the comment at line 412-415 explains why it's acceptable.

What would change if you removed it

The txNum field itself is barely used internally by SharedDomains. It's not used by DomainPut/DomainDel — those take txNum as an explicit parameter already. It's not used by GetLatest or GetAsOf. It's essentially just a counter stored on SharedDomains for the convenience of callers.

If you removed it, the changes would be:


Impact | Details
-- | --
Move txNum to callers | The ~30 doms.TxNum() call sites would need their own local txNum tracking. Most already have it (e.g., exec3_parallel.go has applyResult.txNum, serial has txTask.TxNum).
currentStep for cache | The one stateCache usage at line 417 would need step passed in from the caller, or computed from the txNum the caller already knows.
SetTxNum callers disappear | The ~35 SetTxNum calls go away entirely.
SeekCommitment | Currently does sd.SetTxNum(txNum) after finding the resume point — would instead just return the txNum for the caller to store.
Merge | Sets sd.txNum = otherTxNum — caller would track this.
Tests | ~15 test SetTxNum calls would move to local variables.

The real win: Removing txNum from SharedDomains eliminates the confusion about whose txNum it represents. Right now there's a mismatch — the parallel executor calls pe.rs.SetTxNum(blockNum, txNum) from the apply loop (line 181), but the block-finalize path and worker paths don't touch it at all. Since DomainPut/DomainDel already take txNum as an explicit parameter (not from sd.txNum), the field is truly just a convenience counter that creates a false impression of being authoritative.

Net assessment: Low-risk mechanical refactor. The field is not structurally load-bearing — it's a counter stored in a convenient place. Every caller that reads it already has the same value available locally. The only tricky part is the stateCache step derivation, which would need a small interface adjustment.

## BlockNum

Internal usage

Zero. The sd.blockNum field is never read internally by any SharedDomains method for logic. It's purely stored and returned. No domain operation (GetLatest, GetAsOf, DomainPut, DomainDel, ComputeCommitment, etc.) reads sd.blockNum — they all take blockNum as an explicit parameter where needed.

Callers that SET it (~23 sites)

All just keeping the counter in sync:

  • SeekCommitment — sets it after finding resume point
  • exec3.go:107 / exec3_serial.go:531 / exec3_parallel.go:272 — sets during apply loop
  • rw_v3.go:189  StateV3.SetTxNum(blockNum, txNum) propagates to sd.SetBlockNum
  • stage_execute.go:344 — unwind
  • Tests (~10 sites) — setup
  • squeeze.go, t8ntool, eth_simulation — misc

Callers that READ it (~9 sites)


Caller | File:Line | What it does with it
-- | -- | --
Resume point | exec3.go:152 | blockNum = doms.BlockNum() — initial block to execute
Restart check | exec3.go:202 | Same — re-reads after potential reset
Assertion | exec3.go:832-833 | Panics if doms.BlockNum() != header.Number.Uint64() — sanity check
Logging | exec3_serial.go:242 | Log message
Integrity check | integrity/commitment.go:240,247 | Logging + ComputeCommitment call
Squeeze | squeeze.go:499 | Commitment state

The blockNum is even more clearly a pure convenience counter than txNum — it has zero internal structural usage. Every caller that reads it already has the blockNum available from the task/context it's operating in. The assertion at exec3.go:832 would just move to comparing two local values.

If you removed both txNum and blockNum from SharedDomains, the only structural change needed is the currentStep derivation for the stateCache hit at line 417. Everything else is mechanical: callers store their own local counter instead of reading it from SharedDomains.

Internal usage
Zero. The sd.blockNum field is never read internally by any SharedDomains method for logic. It's purely stored and returned. No domain operation (GetLatest, GetAsOf, DomainPut, DomainDel, ComputeCommitment, etc.) reads sd.blockNum — they all take blockNum as an explicit parameter where needed.

Callers that SET it (~23 sites)
All just keeping the counter in sync:

SeekCommitment — sets it after finding resume point
exec3.go:107 / exec3_serial.go:531 / exec3_parallel.go:272 — sets during apply loop
rw_v3.go:189 — StateV3.SetTxNum(blockNum, txNum) propagates to sd.SetBlockNum
stage_execute.go:344 — unwind
Tests (~10 sites) — setup
squeeze.go, t8ntool, eth_simulation — misc
Callers that READ it (~9 sites)
Caller File:Line What it does with it
Resume point [exec3.go:152](vscode-webview://0sb39evn291jadr4a45c5437jrjembs4vci8qr7r7jc40mff17qo/execution/stagedsync/exec3.go#L152) blockNum = doms.BlockNum() — initial block to execute
Restart check [exec3.go:202](vscode-webview://0sb39evn291jadr4a45c5437jrjembs4vci8qr7r7jc40mff17qo/execution/stagedsync/exec3.go#L202) Same — re-reads after potential reset
Assertion [exec3.go:832-833](vscode-webview://0sb39evn291jadr4a45c5437jrjembs4vci8qr7r7jc40mff17qo/execution/stagedsync/exec3.go#L832) Panics if doms.BlockNum() != header.Number.Uint64() — sanity check
Logging [exec3_serial.go:242](vscode-webview://0sb39evn291jadr4a45c5437jrjembs4vci8qr7r7jc40mff17qo/execution/stagedsync/exec3_serial.go#L242) Log message
Integrity check [integrity/commitment.go:240,247](vscode-webview://0sb39evn291jadr4a45c5437jrjembs4vci8qr7r7jc40mff17qo/db/integrity/commitment.go#L240) Logging + ComputeCommitment call
Squeeze [squeeze.go:499](vscode-webview://0sb39evn291jadr4a45c5437jrjembs4vci8qr7r7jc40mff17qo/db/state/squeeze.go#L499) Commitment state
Comparison to txNum
Property txNum blockNum
Used internally by SD? Only for currentStep (1 cache hit path) No
Callers reading it ~30 ~9
Callers setting it ~35 ~23
Has structural impact? Derives currentStep for stateCache None
Atomic? No (plain uint64) Yes (atomic.Uint64)
The blockNum is even more clearly a pure convenience counter than txNum — it has zero internal structural usage. Every caller that reads it already has the blockNum available from the task/context it's operating in. The assertion at exec3.go:832 would just move to comparing two local values.

If you removed both txNum and blockNum from SharedDomains, the only structural change needed is the currentStep derivation for the stateCache hit at line 417. Everything else is mechanical: callers store their own local counter instead of reading it from SharedDomains.1. Bookkeeping / progress tracking (TxNum())
Used by ~30 callers to answer "where are we in execution?":

exec3.go: Determining resume point after restart (inputTxNum = doms.TxNum())
exec3.go: Computing commitment (ComputeCommitment(..., doms.TxNum(), ...))
exec3.go: Building files in background (agg.BuildFilesInBackground(doms.TxNum()))
exec3.go: Tracking last committed txNum
SeekCommitment: Finding where to resume
stage_mining_exec.go: Getting current txNum for writer
RPC receipt generator: Computing state roots
Fork validator: Merging domains
Squeeze: Tracking progress
2. Step derivation (currentStep)
Only used in one place — [domain_shared.go:417](vscode-webview://0sb39evn291jadr4a45c5437jrjembs4vci8qr7r7jc40mff17qo/db/state/execctx/domain_shared.go#L417):

if v, ok := sd.stateCache.Get(domain, k); ok {
return v, sd.currentStep, nil // <-- here
}
The stateCache hit returns currentStep (derived from txNum/stepSize) as the step for the cached value. This is an approximation — the actual step could be different, but the comment at line 412-415 explains why it's acceptable.

What would change if you removed it
The txNum field itself is barely used internally by SharedDomains. It's not used by DomainPut/DomainDel — those take txNum as an explicit parameter already. It's not used by GetLatest or GetAsOf. It's essentially just a counter stored on SharedDomains for the convenience of callers.

If you removed it, the changes would be:

Impact Details
Move txNum to callers The ~30 doms.TxNum() call sites would need their own local txNum tracking. Most already have it (e.g., exec3_parallel.go has applyResult.txNum, serial has txTask.TxNum).
currentStep for cache The one stateCache usage at line 417 would need step passed in from the caller, or computed from the txNum the caller already knows.
SetTxNum callers disappear The ~35 SetTxNum calls go away entirely.
SeekCommitment Currently does sd.SetTxNum(txNum) after finding the resume point — would instead just return the txNum for the caller to store.
Merge Sets sd.txNum = otherTxNum — caller would track this.
Tests ~15 test SetTxNum calls would move to local variables.
The real win: Removing txNum from SharedDomains eliminates the confusion about whose txNum it represents. Right now there's a mismatch — the parallel executor calls pe.rs.SetTxNum(blockNum, txNum) from the apply loop (line 181), but the block-finalize path and worker paths don't touch it at all. Since DomainPut/DomainDel already take txNum as an explicit parameter (not from sd.txNum), the field is truly just a convenience counter that creates a false impression of being authoritative.

Net assessment: Low-risk mechanical refactor. The field is not structurally load-bearing — it's a counter stored in a convenient place. Every caller that reads it already has the same value available locally. The only tricky part is the stateCache step derivation, which would need a small interface adjustment.

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.