cl/forkchoice: harden Gloas envelope persistence - #23152
Conversation
There was a problem hiding this comment.
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.
awskii
left a comment
There was a problem hiding this comment.
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.
d2de0bc to
9a4d23f
Compare
9a4d23f to
6ad6496
Compare
Summary
HasEnvelopeon its cache-only hot path; only successful writes and validated reads promote the cacheProduction scope
Caplin creates the fork-choice filesystem under
dirs.Tmp/caplin-forkchoiceand 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=1go test -racefor cache misses, read ownership, dump/read concurrency, prune/write ordering, write failures, and post-Gloas round tripsmake linttwicemake erigon integrationThe 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.