Repository navigation
fix: expose IBD header height correctly in health metrics - #1204
Conversation
![rearden-grok[bot]](https://avatars.githubusercontent.com/u/730881?s=60&v=4){kind=small}
| let mut tips: Vec<(BlockHash, u32)> = self.header_tips.read().unwrap().entries().collect(); | ||
| tips.extend(*self.ibd_header_tip.read().unwrap()); | ||
| for (hash, h) in tips { | ||
| if !self.header_ancestry_invalid(hash) { |
There was a problem hiding this comment.
best_header_height runs header_ancestry_invalid on the IBD walk tip. The walk stops after 10,000 prev_of store reads and then returns false, so a tip more than 10,000 blocks above the active chain is still counted. invalidateblock on the connected tip does not call publish_ibd_header_tip, and retire_invalid returns immediately when that block is no longer in height_to_hash, so headers stays on a walk that extends the invalidated block. Every getblockchaininfo and metrics scrape pays for those 10,000 lookups through the header-ahead phase, and past the cap the result cannot change.
There was a problem hiding this comment.
Fixed in 00ce4aa. The IBD report now uses a generation check instead of an ancestry walk. Invalidations clear it, and old walks or delayed origin loads cannot publish into the new generation. The regression checks an unstored look-ahead tip after its connected ancestor is invalidated, then checks stale publications and recovery after reconsider/resume.
1768708 to
00ce4aa
Compare
There was a problem hiding this comment.
REQUEST_CHANGES
2 bugs, 0 suggestions, 0 nits
The docs/rpc.md field note is fixed. An epoch bump clears the IBD report, and a later publish that still holds the old epoch does not put that report back.
best_header_height still fail-opens the stored header queue past 10,000 prev_of reads. publish_header_tip never recaptures the generation, so the live walk goes silent after the next invalidation that is not an on-path retire.
| .unwrap_or(0) | ||
| .max(self.ibd_header_report.read().unwrap().height.unwrap_or(0)); | ||
| let tips: Vec<(BlockHash, u32)> = self.header_tips.read().unwrap().entries().collect(); | ||
| for (hash, h) in tips { |
There was a problem hiding this comment.
best_header_height still runs header_ancestry_invalid on every header_tips entry. That walk stops after 10,000 prev_of reads and returns not invalid, so the tip is counted. IBD stores the download queue up to ORDERED_HEADERS_SOFT_CAP (64,000) and ensure_headers_batch keeps that frontier in header_tips. Invalidating the connected tip clears the IBD report only; headers stays on this stored chain, and every getblockchaininfo and metrics scrape still pays for the capped walk. The new journey stops at height 6, under the cap. Count a tip only when the prev walk reaches the active chain, and do not repeat that walk on the scrape path.
There was a problem hiding this comment.
Fixed in 42a1a0a (rebased onto current master). best_header_height now reads one cached maximum for both reporting sources. Stored candidates are proved on mutation without a depth cutoff, not on each scrape. The regression reaches header height 10,003 and invalidates its connected ancestor; the header-only stretch is necessary to cross the old fixed limit, without building a tall block fixture.
| } | ||
|
|
||
| fn publish_header_tip(st: &IbdWorkState, hub: &ChainHub) { | ||
| if let Some(epoch) = st.header_walk.report_epoch { |
There was a problem hiding this comment.
publish_header_tip keeps the epoch captured when the origin was loaded. invalidateblock, submitblock, and any note_invalid_block bump the generation and clear the report, but extend_tip and rewind_to publish that same old epoch, so the update is dropped for the rest of the process and headers falls back to the block tip while catch-up continues. retire_invalid stores the new epoch only after the hash is on height_to_hash and header_of returns a parent; the earlier returns leave the walk stale. Recapture the current generation at publish when the base and checkpoints are not in the invalid set.
There was a problem hiding this comment.
Fixed in 42a1a0a (rebased onto current master). A live producer can refresh its generation from a revalidated proof, and the journey now continues after an unrelated invalidation. It also invalidates an ancestor between checkpoints rather than checking endpoint hashes alone. Unprovable look-ahead retains only its proved prefix. IBD queues these checks on the existing header thread, outside the coordination lock.
|
rebase plz! |
Publish the accepted IBD header walk into ChainHub on fresh sync, resume, and rewind so health metrics and RPC do not fall back to the validated block tip. Keep peer horizons separate and retain ancestry filtering without holding tip locks during the walk. Add an HTTP metrics and RPC regression through a real store reopen, plus assertions for peer horizons, look-ahead headers, and invalid candidate retirement.
Replace per-scrape ancestry checks for the IBD report with a reporting generation. Invalidating a block clears the report and prevents delayed origin loads or an old walk from publishing it again. Rebased and restored walks capture the current generation; reporting generations are not persisted. Extend the metrics and RPC journey with an unstored look-ahead tip, invalidation of its connected ancestor, stale origin publications, reconsideration, and resume. Keep the fixture small rather than building a 10,000-header chain to hit the old ancestry cap.
Cache the maximum from stored header tips and the accepted IBD path so scrapes never walk ancestry. Revalidate stored candidates to the active chain on invalidation and reconsideration without a depth cutoff. Reuse proved parents for header intake and retain only a proved prefix when a look-ahead tail cannot be checked. Refresh live IBD reporting after unrelated invalidations. Bound extensions by known invalid descendants and check intermediate ancestry rather than checkpoint hashes alone. Queue IBD proof checks on the existing header role so they run outside the coordination lock. Extend the journeys with a stored chain beyond the old 10,000-step cap, unrelated invalidation followed by live extension, an invalid ancestor between checkpoints, reconsideration, and the coordinator IO guard.
d1cd040 to
42a1a0a
Compare
done 🫡 |
There was a problem hiding this comment.
REQUEST_CHANGES
2 bugs, 0 suggestions, 0 nits
The cached read fixes the scrape walk and the 10,000-step fail-open. Refresh rewrites a live proof at the current generation, so an unrelated stored invalid block no longer leaves the walk on a stale epoch.
proved_stored_header_height returns at the first invalid hash, and check_ibd_header_proof then keeps only the walk base, so every valid header below that block disappears. A hash with no header row clamps look-ahead to the stored anchor; submitblock refreshes before ensure_header, and nothing proves that look-ahead again while header intake is idle.
| loop { | ||
| let hash = BlockHash::from_byte_array(rec.hash); | ||
| if invalid.contains(&hash) { | ||
| return None; |
There was a problem hiding this comment.
proved_stored_header_height returns None at the first invalid hash, so a stored frontier whose ancestor was invalidated contributes no height. check_ibd_header_proof then sees the highest stored anchor fail and returns only anchors[0] (the walk base), skipping every later anchor that still proves. Valid headers between the active chain and that invalid block disappear, and headers falls back to the block tip. The 10,003-header journey invalidates the connected ancestor, where the valid prefix is only the tip, and the interior-checkpoint case sits next to that base, so both stay green. Walk to the parent of the invalid block and report that height; for the IBD proof, keep the highest anchor that still proves.
There was a problem hiding this comment.
Fixed in ba01b4a. Each frontier candidate now retains its highest proved valid prefix, walking below invalid ancestors rather than discarding the candidate. The stored-header regression checks 5,000 invalid → 4,999, a second invalid ancestor at 7,000, and staged reconsideration back through 6,999 to 10,003. The IBD journey also checks a valid prefix above the walk base.
| let Some(invalid_height) = | ||
| self.proved_stored_header_height(hash, &none, &context.proved) | ||
| else { | ||
| safe_until = safe_until.min(stored_height); |
There was a problem hiding this comment.
An invalid hash that is not an anchor and has no header row sets safe_until to the stored anchor, which drops the whole unstored look-ahead. That hash cannot lie between checkpoints. submitblock calls note_invalid_block (and therefore refresh_header_report) before ensure_header, so the refresh runs while the row is missing and commits the clamp. Header intake then goes idle for the block download and nothing refreshes the proof after the row exists. Ignore an invalid hash that is not an anchor and not stored, and refresh again once its header row is written.
There was a problem hiding this comment.
Fixed in ba01b4a by refreshing when the marked header is ensured, before intake can return early. The actual rejected-block RPC path now checks that look-ahead recovers after header storage without further intake. I kept unknown hashes conservative: an unstored invalid hash can be between checkpoints, and a control case verifies that storing such a header does not restore its invalid descendants. A revision guard rejects refresh results superseded by newer registration evidence.
Keep each stored frontier candidate's highest proved valid prefix, including when multiple ancestors are invalid. Refresh reporting when a marked header is registered so rejected-block intake can restore unrelated look-ahead without waiting for more headers. Cover intermediate invalidation and staged reconsideration on a 10,003-header chain, the rejected-block RPC path, and an unstored invalid ancestor inside look-ahead. Reject refresh results superseded by newer registration evidence.
There was a problem hiding this comment.
APPROVE
0 bugs, 0 suggestions, 0 nits
A stored frontier now keeps the parent of its lowest invalid ancestor, including a second invalid block above that cut, and the IBD proof keeps that prefix when the old tip no longer proves. A rejected submitblock still marks the hash before the header row exists, then note_header_tip refreshes once the row is written, so an unrelated reject no longer leaves look-ahead clamped for the rest of block download. A hash that is still unstored stays a hard cut: it can sit between checkpoints, and storing it must not bring its descendants back.
reardencode#1204 built an RpcContext literal in the health metrics test. reardencode#1211 added package_dialect to RpcContext and updated the other test literals to false. The two pulls merged cleanly and left this initializer missing the field, so the node test crate does not compile. package_dialect is false here, the off default those other literals use. The rest of reardencode#1204 already matches the NodeEnv, NodeConfig, and RpcContext fields reardencode#1211 added, and it does not call the helpers that pull removed. Co-authored-by: Brandon Black <reardencode@users.noreply.github.com>
IBD tracks headers separately from the state read by health metrics and
getblockchaininfo. This can make headers equal blocks and verification progress show 1 while catch-up is still running.Share IBD’s accepted header tip with those readers during fresh sync and resume, and update it on rewind or invalid-header retirement. Peer-advertised heights do not count. The progress formula, validation, and readiness rules stay unchanged.