Skip to content

execution/commitment: fix wave-BFS livelock when a step budget fills exactly #23066

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lystopad merged 7 commits into from
Aug 10, 2026
+439 −16
Merged

execution/commitment: fix wave-BFS livelock when a step budget fills exactly #23066

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8a95996
execution/commitment: fix wave-BFS livelock when a step budget fills …
lystopad Aug 6, 2026
5be5ef7
execution/commitment: address review on the preload livelock tests
lystopad Aug 6, 2026
4e47f07
execution/commitment: widen the preload wave-end guard, cover the mix…
awskii Aug 8, 2026
03a83f4
execution/commitment: cover both no-pin wave paths, unify queueEmpty
lystopad Aug 9, 2026
810377d
execution/commitment: rename budgetHit to endStep
lystopad Aug 9, 2026
42c4aab
execution/commitment: make the preload tests pin what they claim
lystopad Aug 10, 2026
74c6b74
Merge branch 'main' into feature/lystopad/preload-parallel-livelock
lystopad Aug 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 39 additions & 16 deletions execution/commitment/preload_parallel.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ func estimatedEntryCost(key, value []byte) int {

// minEntryBytes: true lower bound on estimatedEntryCost for a storage-trunk
// branch (33 = shortest HexToCompact key at depth >= 64; value may be empty).
// Bounds a wave's file fetch so the budget is guaranteed exhausted inside it.
// Bounds a wave's file fetch to what the remaining budget could still pin; a
// wave that pins nothing anyway is ended by the no-progress check in Run.
const minEntryBytes = estimatedEntryOverheadBytes + 33

// maxStorageTrunkDepth: 64 (account path) + 64 (keccak256(slot)) = 128.
Expand Down Expand Up @@ -164,19 +165,19 @@ func (p *ContractTrunkPreloadParallel) Run(
return 0, false, fmt.Errorf("ContractTrunkPreloadParallel.Run: resolver is nil")
}
if stepBudgetBytes <= 0 {
return 0, len(p.frontier) == 0, nil
return 0, p.queueEmpty(), nil

@awskii awskii Aug 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracting queueEmpty() also changes what this early return reports when stepBudgetBytes<=0 — from len(p.frontier)==0 to also checking pendingChildren and the depth ceiling. The two can only disagree when frontier is non-empty, pendingChildren is empty, and nextDepth > maxStorageTrunkDepth all at once — looks unreachable given how these three are always mutated together, but no test calls Run with stepBudgetBytes<=0 and non-empty pendingChildren to pin that down, so the behavior change rides along inside what otherwise reads as a pure refactor.

}
defer p.releaseScratch()

stepCap := p.usedBytes + stepBudgetBytes
chunkPinned := 0
budgetHit := false
endStep := false

// pin records the entry and queues its children. Returns false on budget hit.
pin := func(pk pathKey, v []byte, depth int, next *[]pathKey) bool {
cost := estimatedEntryCost(pk.key, v)
if p.usedBytes+cost > stepCap {
budgetHit = true
endStep = true
return false
}
// step=0: a storage-trunk branch resolved across merged files has no single
Expand Down Expand Up @@ -210,13 +211,15 @@ func (p *ContractTrunkPreloadParallel) Run(
return true
}

for !budgetHit && p.nextDepth <= maxStorageTrunkDepth && len(p.frontier) > 0 {
for !endStep && p.nextDepth <= maxStorageTrunkDepth && len(p.frontier) > 0 {
depth := p.nextDepth
wavePinnedBefore := chunkPinned
dbHits, dbVals, fileMiss, dbHitsBytes := p.sortAndPartitionFrontier(dbBranches)

// Cap the file fetch by what the budget can absorb after dbHits.
// Cap the file fetch by what the budget can absorb after dbHits. Below
// minEntryBytes no file entry can be pinned, so fetching any is waste.
var fileMissDeferred []pathKey
if fileBudget := stepCap - p.usedBytes - dbHitsBytes; fileBudget <= 0 {
if fileBudget := stepCap - p.usedBytes - dbHitsBytes; fileBudget < minEntryBytes {
fileMissDeferred = fileMiss
fileMiss = nil
} else if maxFileFetch := fileBudget/minEntryBytes + 1; maxFileFetch < len(fileMiss) {
Expand Down Expand Up @@ -247,8 +250,9 @@ func (p *ContractTrunkPreloadParallel) Run(
}
p.dbHitsPinned++
}
fileMissStop := len(fileMiss)
if !budgetHit {
fileMissStop := 0
if !endStep {
fileMissStop = len(fileMiss)
for i, pk := range fileMiss {
v := fileVals[i]
if v == nil {
Expand All @@ -261,7 +265,16 @@ func (p *ContractTrunkPreloadParallel) Run(
}
}

if budgetHit {
// Re-entering only helps if the next iteration sees a different budget.
// It does not when the whole miss set was deferred (dbHits consume
// exactly the bytes the deferral already accounted for), nor when a
// capped fetch pinned nothing because its keys were absent from the file
// layer. Either way, end the step; the tail resumes on the next Run.
if len(fileMissDeferred) > 0 && (len(fileMiss) == 0 || chunkPinned == wavePinnedBefore) {
endStep = true
}

if endStep {

@awskii awskii Aug 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A pathKey that was a dbBranches hit but gets deferred into rest/p.frontier here loses that provenance. If a later Run call's dbBranches overlay rotates past it and the file resolver also misses, sortAndPartitionFrontier reclassifies it as fileMiss, resolve() returns nil, and it's dropped silently — indistinguishable from a genuine BFS-fringe absence. Pre-existing classification behavior, but this PR is what makes multi-call resumption of budget-deferred waves routine instead of hanging, so it's now reachable in practice (adaptive_pin.go's runExtensionLocked holds a persistent preload instance across blocks with a fresh overlay each call). No test covers a dbHit surviving a deferral into a call with a changed overlay.

// Preserve un-pinned items at current depth; pendingChildren stays
// at depth+1 for when this depth is drained on a future Run.
rest := make([]pathKey, 0, len(dbHits)-dbHitStop+len(fileMiss)-fileMissStop+len(fileMissDeferred))
Expand All @@ -273,8 +286,9 @@ func (p *ContractTrunkPreloadParallel) Run(
}

if len(fileMissDeferred) > 0 {
// Defensive: !budgetHit should mean no truncation. Clone out of the
// scratch-aliased slice so the next wave's partition can't overwrite it.
// Capped fetch that still pinned something: stay at this depth and
// resume with the tail. Clone out of the scratch-aliased slice so the
// next wave's partition can't overwrite it.
p.frontier = slices.Clone(fileMissDeferred)
} else {
p.frontier = p.pendingChildren
Expand All @@ -283,7 +297,7 @@ func (p *ContractTrunkPreloadParallel) Run(
}
}

queueEmpty = (len(p.frontier) == 0 && len(p.pendingChildren) == 0) || p.nextDepth > maxStorageTrunkDepth
queueEmpty = p.queueEmpty()
if logger != nil && (chunkPinned > 0 || queueEmpty) {
logger.Info("[trunk-preload-parallel] step",
"step_budget_mb", stepBudgetBytes/(1<<20),
Expand All @@ -310,6 +324,12 @@ func (p *ContractTrunkPreloadParallel) QueueRemaining() int {
return len(p.frontier) + len(p.pendingChildren)
}

// queueEmpty reports that no further Run can pin anything: the walk drained, or
// it reached the depth ceiling with entries still queued below it.
func (p *ContractTrunkPreloadParallel) queueEmpty() bool {
return p.QueueRemaining() == 0 || p.nextDepth > maxStorageTrunkDepth
}

// PinnedPrefixes returns slices aliasing internal storage — do not mutate.
func (p *ContractTrunkPreloadParallel) PinnedPrefixes() [][]byte { return p.pinnedPrefixes }

Expand All @@ -326,13 +346,16 @@ func PreloadContractTrunkParallel(
if ramBudgetBytes <= 0 {
return 0, fmt.Errorf("PreloadContractTrunkParallel: ramBudgetBytes must be positive, got %d", ramBudgetBytes)
}
p, err := NewContractTrunkPreloadParallel(contractHash)
if err != nil {
return 0, err
if cache == nil {

@awskii awskii Aug 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This guard is what's actually fixing a real bug, worth calling out: pre-fix, calling this wrapper with cache=nil and a non-nil logger reached the unconditional logger.Info(...cache.PinnedCount()) below (line 371) and panicked — p.Run returns an error on nil cache, but nothing here checked that error before logging. No test exercises the wrapper with cache=nil, logger!=nil: TestContractTrunkPreloadParallel_NilCacheError only calls p.Run directly with logger=nil. Worth a regression test at the wrapper level for this specific fix.

return 0, fmt.Errorf("PreloadContractTrunkParallel: cache is nil")
}
if resolve == nil {
return 0, fmt.Errorf("PreloadContractTrunkParallel: resolver is nil")
}
p, err := NewContractTrunkPreloadParallel(contractHash)
if err != nil {
return 0, err
}
pinned, queueEmpty, err := p.Run(ramBudgetBytes, dbBranches, resolve, cache, logger)
if logger != nil {
logger.Info("[trunk-preload-parallel] complete",
Expand Down
Loading
Loading