Skip to content

cl/forkchoice: harden Gloas envelope persistence - #23152

Open
domiwei wants to merge 8 commits into
mainfrom
kewei/gloas-envelope-storage-safety
Open

cl/forkchoice: harden Gloas envelope persistence#23152
domiwei wants to merge 8 commits into
mainfrom
kewei/gloas-envelope-storage-safety

Conversation

@domiwei

@domiwei domiwei commented Aug 10, 2026

Copy link
Copy Markdown
Member

Summary

  • replace Gloas envelope files through a temporary file and atomic rename so failed or concurrent replacements cannot expose partial data
  • record the envelope fork version in the file and decode with that version, including post-Gloas representations
  • reuse the fork graph SSZ buffer while detaching transaction bytes that alias decoder input
  • keep HasEnvelope on its cache-only hot path; only successful writes and validated reads promote the cache
  • serialize envelope reads, writes, and pruning so stale writers cannot restore pruned envelopes
  • retain structurally invalid files for diagnosis while marking them unusable in memory; transient I/O failures remain retryable
  • validate externally supplied envelopes before hashing, keep Gloas progressive resource guards distinct from protocol limits, and reject persisted representations that cannot round-trip through the configured decoder
  • enforce block-root identity, the 15 MiB transport bound, matching nested versions, collection bounds, and complete nested representations
  • bound prune lock hold time per root and remove envelope files after invalidating their in-memory lineage

Production scope

Caplin creates the fork-choice filesystem under dirs.Tmp/caplin-forkchoice and clears it before constructing the fork graph on every process start. This PR therefore provides intra-process atomicity, ownership safety, and prune ordering; it intentionally does not claim crash durability or startup recovery. Directory fsync, quarantine files, startup scavenging, and committed-with-durability-warning handling are outside the real lifecycle and are not included.

Beacon-state persistence remains unchanged. It has a different file format and lifecycle and should be hardened separately rather than expanding this envelope-focused change.

Root cause

Envelope writes truncated the final file in place, so a failed replacement could destroy the previously readable envelope and a concurrent reader could observe partial data. Reads decoded transaction slices from the fork graph's reusable SSZ buffer, allowing a later read or write to mutate an envelope already returned to a caller. Pruning also removed envelope files without coordinating with writers, allowing a concurrent rename to restore a pruned file.

Validation

  • go test ./cl/phase1/forkchoice/fork_graph ./cl/phase1/forkchoice ./cl/phase1/network/services ./cl/cltypes ./cl/cltypes/solid -count=1
  • focused go test -race for cache misses, read ownership, dump/read concurrency, prune/write ordering, write failures, and post-Gloas round trips
  • make lint twice
  • make erigon integration

The behavior changes were driven Red to Green through public network-service, forkchoice, and fork-graph methods. Regressions cover destructive replacement failures, shared-buffer mutation, prune/write resurrection, unrelated I/O during multi-root pruning, cache misses waiting on state I/O, transient open/read failures preserving trusted availability, structural corruption evicting warm cache, post-Gloas version loss, nested-version mismatch, null list members before hashing, protocol-versus-decoder bounds, alternate collection representations before persistence, double close, root/file identity mismatch, oversized allocation, malformed nested input, and failed-removal cache exposure.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Hardens Gloas execution payload envelope persistence in the fork-choice fork graph by making disk writes atomic/durable, reads ownership-safe (no shared buffer aliasing), and pruning/write/read operations serialized to avoid resurrecting or re-exposing pruned/corrupt envelopes.

Changes:

  • Switch envelope persistence to temp-file write + atomic rename + directory sync, plus startup cleanup of orphan temp/quarantine artifacts.
  • Make envelope reads decode from owned buffers, enforce root identity, and quarantine/remove structurally corrupt envelope files.
  • Add stronger envelope validation (Gloas-only, size bounds, complete representation) and tests covering races, corruption, and failure modes.

Reviewed changes

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

Show a summary per file
File Description
cl/phase1/forkchoice/payload_vote_test.go Extends test fork-graph stub to simulate dump errors.
cl/phase1/forkchoice/on_execution_payload.go Routes persistence through persistEnvelope to downgrade “committed with durability warning” to a logged warning.
cl/phase1/forkchoice/on_execution_payload_test.go Adds regression test ensuring bookkeeping completes when persistence returns a committed-warning marker.
cl/phase1/forkchoice/fork_graph/interface.go Introduces ErrEnvelopeCommitted marker error for “committed but durability warning” semantics.
cl/phase1/forkchoice/fork_graph/fork_graph_test.go Adds extensive tests for atomicity, corruption handling, quarantine, pruning/write serialization, and ownership/race safety.
cl/phase1/forkchoice/fork_graph/fork_graph_disk.go Serializes prune with envelope operations, quarantines pruned envelopes, removes temps, and syncs directory after prune.
cl/phase1/forkchoice/fork_graph/fork_graph_disk_fs.go Implements envelope temp naming, artifact cleanup, directory sync helper, quarantine helper, and hardened read/write paths.
cl/cltypes/execution_requests.go Exposes ExecutionRequests.Version() for safer version checks in persistence validation.
Suppressed comments (1)

cl/phase1/forkchoice/fork_graph/fork_graph_disk_fs.go:102

  • removeOrQuarantineEnvelope returns the original Remove error even when the quarantine Rename succeeds. That makes callers treat a successful quarantine as a hard failure (e.g., Prune joins and returns an error), and the log message in ReadEnvelopeFromDisk can claim removal failed even though the file was successfully moved out of the active name. Consider returning nil on successful quarantine, and only returning an error when both Remove and Rename fail (or when Rename fails with something other than IsNotExist).
	if renameErr := fs.Rename(filename, filename+suffix); renameErr != nil && !os.IsNotExist(renameErr) {
		return errors.Join(err, fmt.Errorf("quarantine envelope: %w", renameErr))
	}
	return err

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread cl/phase1/forkchoice/fork_graph/fork_graph_disk_fs.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

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

Comment thread cl/phase1/forkchoice/fork_graph/fork_graph_disk_fs.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

@domiwei
domiwei marked this pull request as ready for review August 11, 2026 07:02
@domiwei
domiwei requested a review from sudeepdino008 as a code owner August 11, 2026 07:02

@awskii awskii left a comment

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.

Review of the envelope-persistence hardening. Main concerns: HasEnvelope now takes the beacon-state dump lock on a path that runs under f.mu; a decode failure now destroys the file; and because cmd/caplin/caplin1/run.go:346 wipes the fork-choice directory on every start, most of the new durability machinery cannot fire in production. Details inline.

Comment thread cl/phase1/forkchoice/fork_graph/fork_graph_disk_fs.go Outdated
Comment thread cl/phase1/forkchoice/fork_graph/fork_graph_disk_fs.go
Comment thread cl/phase1/forkchoice/fork_graph/fork_graph_disk.go Outdated
Comment thread cl/phase1/forkchoice/fork_graph/fork_graph_disk_fs.go Outdated
Comment thread cl/phase1/forkchoice/fork_graph/fork_graph_disk_fs.go Outdated
Comment thread cl/phase1/forkchoice/fork_graph/fork_graph_disk_fs.go Outdated
Comment thread cl/phase1/forkchoice/fork_graph/fork_graph_disk_fs.go
Comment thread cl/phase1/forkchoice/fork_graph/fork_graph_disk_fs.go Outdated
Comment thread cl/phase1/forkchoice/fork_graph/fork_graph_disk_fs.go Outdated
Comment thread cl/phase1/forkchoice/on_execution_payload.go Outdated
@domiwei
domiwei force-pushed the kewei/gloas-envelope-storage-safety branch 3 times, most recently from d2de0bc to 9a4d23f Compare August 11, 2026 10:12
@domiwei
domiwei force-pushed the kewei/gloas-envelope-storage-safety branch from 9a4d23f to 6ad6496 Compare August 11, 2026 10:16
@domiwei
domiwei requested review from awskii and a lite review from Copilot August 12, 2026 04:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants