fix(node): re-anchor cNIGHT observation window near tip on from-genesis sync#1836
fix(node): re-anchor cNIGHT observation window near tip on from-genesis sync#1836rsporny wants to merge 4 commits into
Conversation
…sis syncs narrow The cNIGHT observation sliding window trims to `follower - window_size` during steady operation, so `window_size` is a reorg-safety lookback behind the follower, not a general cache size. A validator that syncs from genesis advances the follower contiguously and never takes `plan_refresh`'s narrow jump re-anchor, so it keeps `window_size` blocks behind the follower forever. At the old 100_000 default (100x the runtime's own 1000-block observation window) that pinned it at a ~100k-wide window; every refresh then queried a Cardano range far wider than a healthy peer's, overrunning the 6s slot and starving block authoring until a manual restart. Lower DEFAULT_WINDOW_SIZE to 10_000: 10x the runtime window and well above Cardano's security parameter k (~2160), so it still covers the deepest reorg while converging a from-genesis node to roughly the narrow window a restart-with-state node already gets. No consensus impact — the window is node-local and out-of-window reads fall back to the live db source. Also corrects the misleading config/const docs and adds regression tests. Assisted-by: Claude:claude-opus-4-8 Signed-off-by: Radosław Sporny <404@rspo.dev>
This comment has been minimized.
This comment has been minimized.
PR: #1836 Assisted-by: Claude:claude-opus-4-8 Signed-off-by: Radosław Sporny <404@rspo.dev>
rsporny
changed the title
m2ux
left a comment
m2ux
left a comment
There was a problem hiding this comment.
PR Review Summary
PR: #1836 — fix(node): re-anchor cNIGHT observation window near tip on from-genesis sync
Reviewers: Code Review Agent · Test Suite Review Agent · Validation Agent · Strategic Review Agent
Reports: Prior Feedback Triage · Code Review · Test Suite Review · Strategic Review
Branch / Commit reviewed: worktree-rspo-fix-1835-cnight-window @ ee928b30 (merge-base b246fb5a)
Date: 2026-07-07
Executive Summary
This draft PR addresses #1835: from-genesis validators can keep a ~100k-block cNIGHT observation lookback in steady state, making background refreshes heavy enough to starve block authoring. The branch at ee928b30 lowers DEFAULT_WINDOW_SIZE to 10_000, clarifies that window_size is a reorg lookback (not general cache size), and adds two solid plan_refresh regression tests. The mechanism is sound — contiguous steady-state trim already sets width to window_size; the bug was the default magnitude.
Important: The PR description describes a further refactor (re-anchor via computed reorg margin, remove the config knob) that is not in this diff. Treat current commits as an interim constant mitigation; align description or push follow-on before marking ready.
Overall Rating: Comment Only (draft; no correctness blocker on current commits; process/doc alignment needed before merge)
Prior Feedback Triage
| # | Prior Comment | Author | Disposition | Reasoning |
|---|---|---|---|---|
| 1 | CI Changes Check failed on 858fd4c |
datadog-official | Superseded | Minimized/outdated; check-changes passes on latest head |
| 2 | Thumbs-up reaction | chatgpt-codex-connector | Refuted | Not actionable |
Finding Details
No human review threads or blocker-class comments. Rating cap unset.
Code Review Findings
| # | Finding | Severity |
|---|---|---|
| 1 | PR body promises reorg-margin re-anchor + config removal — not in diff at ee928b30 |
High |
| 2 | REFRESH_THRESHOLD (10k) now equals DEFAULT_WINDOW_SIZE — confirm lookahead intent |
Medium |
| 3 | Constant-only fix vs issue "Expected" logic re-anchor — interim mitigation | Medium |
| 4 | Change file title ("Shrink default") vs PR title ("re-anchor") | Low |
Finding Details
CR-1. PR description vs branch (High — process)
PR body: re-anchor to security parameter + stability margin; remove cnight_observation_window_size. Branch: DEFAULT_WINDOW_SIZE 100k→10k, docs, unit tests only.
Suggestion: Push follow-on commits or revise PR/change file to match landed behaviour before ready-for-review.
CR-2. REFRESH_THRESHOLD coupling (Medium)
Both constants are 10_000. Refresh target uses tip + REFRESH_THRESHOLD + stability_margin. Pre-existing but now coupled to lookback size.
Suggestion: Document relationship or derive threshold from window size.
CR-3. Fix class (Medium)
Issue #1835 expected re-anchor near tip (logic). Branch implements default reduction (mitigation #2). Regression test proves trim at 10k width, not production slot timing.
Suggestion: State interim intent in PR; optional follow-on using stability_margin as lookback.
CR-4. Title mismatch (Low)
Change file heading vs PR title — editorial only.
Verified correct: plan_refresh trim logic, reorg bound [2160, 20000], backward-compatible config, no consensus impact.
Test Review Findings
| # | Gap | Severity |
|---|---|---|
| 1 | No test on RefreshContext::refresh / bulk_pull span after steady-state trim |
Medium |
Finding Details
TR-1. Integration gap (Medium)
Unit tests cover plan_refresh only. Operational failure is wide bulk_pull during refresh.
Suggestion: Test recording pull block range on simulated caught-up from-genesis refreshes.
Strengths: plan_refresh_from_genesis_steady_state_tracks_follower_not_genesis, default_window_size_is_a_modest_reorg_lookback; 15/15 lib tests pass locally.
Validation Findings
| # | Check | Severity |
|---|---|---|
| 1 | cargo test -p midnight-primitives-mainchain-follower --lib — 15/15 pass (local) |
— |
| 2 | cargo fmt --check on touched crates — pass (local) |
— |
Finding Details
VF-1. Local validation (Pass)
All 15 unit tests in midnight-primitives-mainchain-follower pass; plan_refresh suite including new regressions green. Matches PR testing evidence.
CI on latest head: check-changes pass; several jobs still pending at review time (draft). No validation failures observed.
Branch Hygiene
| # | Item | Severity |
|---|---|---|
| 1 | Branch 1 commit behind origin/main |
Warning |
Finding Details
BH-1. Branch freshness (Warning)
One commit behind main. Rebase before merge and re-run CI.
Action Items
Must Address (Blocking):
- None on current code correctness.
Should Address (Recommended):
- Align PR description with branch diff or push the described reorg-margin refactor (CR-1)
- Mark PR ready only after description/commits match (CR-1)
- Rebase on
mainbefore merge (BH-1)
Could Address (Suggested):
- Add integration test for refresh
bulk_pullspan (TR-1) - Document
REFRESH_THRESHOLDvsDEFAULT_WINDOW_SIZErelationship (CR-2) - Align change file heading with PR title (CR-4)
Nice to Have (Optional):
- Note interim vs complete fix relative to issue #1835 Expected (CR-3)
Review conducted via work-package workflow (review mode). Artifacts under .engineering/artifacts/planning/2026-07-07-review-midnight-node-pr-1836-cnight-window/.
…is sync The sliding window trimmed to a large fixed lookback behind the follower. A from-genesis node advances the follower contiguously and never takes the narrow re-anchor a restart-with-state node does, so it trailed the tip by the whole window forever instead of tracking it — holding a ~100k-block in-memory window (hundreds of MB) where a restarted peer holds a few thousand. Re-anchor to a small reorg-safety margin (cardano_security_parameter + block_stability_margin) behind the follower instead. The runtime only reads at or ahead of the follower, so anything further back is never served from cache; the only reason to keep any is a reorg, which can't run deeper than that margin, and deeper re-reads fall back to the live source. From-genesis now converges to the same near-tip window as a restart. Removes the now-redundant cnight_observation_window_size knob and DEFAULT_WINDOW_SIZE; existing configs that still set the key keep loading (it is ignored). No consensus impact — the window is node-local. The per-refresh db-sync pull is already incremental, so the oversized window cost memory, not steady-state query time; the slow refreshes in the linked issue were during a concurrent db-sync incident. Assisted-by: Claude:claude-opus-4-8 Signed-off-by: Radosław Sporny <404@rspo.dev>
rsporny
force-pushed
the
worktree-rspo-fix-1835-cnight-window
branch
from
46f4895 to
5094975
Compare
Rewrite the change file as a concise why/what/removed explanation, and tighten the stability_margin and plan_refresh doc comments. Assisted-by: Claude:claude-opus-4-8 Signed-off-by: Radosław Sporny <404@rspo.dev>
2 participants
Overview
A validator that syncs from genesis ends up holding an oversized in-memory cNIGHT observation window that never narrows, unlike a restart-with-state node.
The observation sliding window trims to a lookback behind the follower in steady state. That lookback was a large fixed window. On a restart with state the cache starts behind the runtime and re-anchors narrowly near tip; but a from-genesis node advances the follower contiguously the whole way, never takes that narrow re-anchor, and so trails the tip by the entire window forever. It ends up holding a ~100k-block window (hundreds of MB) where a restarted peer holds a few thousand, with the retained width depending on how the node happened to sync rather than on the chain.
Fix
Re-anchor the window to a small reorg-safety margin behind the follower — the Cardano security parameter plus stability margin (the maximum reorg depth), which the node already computes. The runtime only ever reads at or ahead of the follower, so anything further back is never served from cache; the only reason to keep any is a reorg, and reorgs can't run deeper than that margin (deeper re-reads fall back to the live source). A from-genesis node now converges to the same near-tip window as a restart-with-state one.
This makes the fixed-size window knob redundant, so
cnight_observation_window_size(andDEFAULT_WINDOW_SIZE) are removed — the reorg margin the node already tracks is the right lookback, and there's nothing to tune. Existing configs that still carry the key keep loading (it's ignored; nodeny_unknown_fields).No consensus/correctness impact: the window is node-local and out-of-window reads fall back to the live db-sync source.
Scope / note on the latency figures in #1835
The per-refresh db-sync pull is already incremental (
[old_end + 1, target_end], clamped to the stable tip), so the oversized window cost memory, not steady-state query time — a caught-up node already queries only a few blocks near tip. The wide/slow refreshes in the issue were captured during a concurrent db-sync incident (the issue disclaims those numbers). So this change fixes the window-tracking and memory behavior and makes from-genesis match restart; it is not, on its own, a steady-state latency fix.🗹 TODO before merging
📌 Submission Checklist
git commit -s) for the DCO🧪 Testing Evidence
cargo test -p midnight-primitives-mainchain-follower --libpasses, including a regression test (plan_refresh_from_genesis_steady_state_tracks_follower_not_genesis) that a caught-up from-genesis node trims to the reorg-margin lookback rather than staying anchored at genesis.cargo check -p midnight-node,cargo fmt --check, andcargo clippy --testson both changed crates are clean.🔱 Fork Strategy
Links
Closes #1835