fix: indexer wedging on startup

[gilcu3]

Closes #3623 (II)

[claude]

Pull request overview

Fixes #3623 — an MPC node that state-syncs from scratch (or returns from long downtime) was wedging its indexer at genesis. Root cause: sync_info.syncing reads false transiently at startup before neard learns it is behind, so wait_for_full_sync returned immediately and LatestSynced pinned the streamer to a stale head. The fix augments the sync check with a peer-height gate — the local head must be within a small tolerance of the highest connected peer's height — and applies it in both places that gate on sync: the node's IndexerClient::wait_for_full_sync and the chain-gateway IsSyncing trait.

Changes:

• Widened the IsSyncing trait contract from bool to a new SyncStatus { syncing, head_height, max_peer_height } and updated all impls + the mock.
• Added SyncStatus::is_caught_up (chain-gateway) and head_caught_up_to_peers (node) with identical SYNC_HEIGHT_TOLERANCE = 5.
• Switched Status { detailed } to true in both call sites so detailed_debug_status.network_info.connected_peers[*].height is populated.
• Added 7 unit tests per site covering syncing/caught-up/tolerance/no-peer paths.

Reviewed changes

Per-file summary

File	Description
crates/chain-gateway/src/primitives.rs	Introduces SyncStatus, SYNC_HEIGHT_TOLERANCE, and is_caught_up. Changes trait signature and wait_for_full_sync loop.
crates/chain-gateway/src/near_internals_wrapper/client.rs	Real impl now asks for detailed: true, extracts max_peer_height, returns SyncStatus.
crates/chain-gateway/src/chain_gateway.rs	Trait forwarding renamed is_syncing → sync_status.
crates/chain-gateway/src/mock.rs	Mock synthesizes a SyncStatus from the existing bool knob.
crates/node/src/indexer.rs	IndexerClient::wait_for_full_sync now polls with detailed: true and uses head_caught_up_to_peers — a verbatim copy of SyncStatus::is_caught_up.

Findings

Blocking (must fix before merge): none.

Non-blocking (nits, follow-ups, suggestions):

• crates/node/src/indexer.rs:424-492 vs crates/chain-gateway/src/primitives.rs:23-49 — the tolerance constant, the caught-up predicate, and the entire block-doc comment above them are byte-identical between the two crates. The seven unit tests at indexer.rs:552-657 are also line-for-line duplicates of the ones at primitives.rs:108-235. The reason the node doesn't reuse the chain-gateway IsSyncing trait is that it drives the raw ClientActor handle, but the predicate itself has no I/O and could live in one place. Worth a follow-up to extract head_caught_up_to_peers(syncing, head_height, max_peer_height, tolerance) (or the SyncStatus struct) into a shared crate — otherwise the two constants and the two comment blocks will drift the next time this logic is tuned.
• crates/chain-gateway/src/primitives.rs:29-37 — the doc comment on is_caught_up includes long-form rationale ("on a freshly state-syncing node…which would pin the streamer's LatestSynced cursor at a stale head it can never reach"). Per docs/engineering-standards.md §Write helpful code comments (item 5), long-form rationale is better carried by the issue than the doc comment; consider trimming to a one-liner plus TODO(#3623)-style pointer, or move the narrative to the PR body only. Same wording appears at crates/node/src/indexer.rs:471-479.
• crates/chain-gateway/src/primitives.rs:129-131, 147-148, 199-200, 218 (and the mirrored tests at crates/node/src/indexer.rs:567-570, 585-586, 601-602, 628-629, 644) — each test carries a 1-3-line prose narrative (e.g. "The MPC indexer wedges at genesis on a freshly state-synced node #3623 wedge: at fresh boot the node sits at genesis…"). The test name plus body already conveys the shape; the narrative repeats it. The #3623 back-reference is useful; the surrounding paraphrase is not. Consider /// See #3623. and dropping the rest, or leaving just the tests without the extra doc.
• crates/chain-gateway/src/mock.rs:141-142 — the mock's inline comment is fine, but it does hard-code max_peer_height: Some(100), so the max_peer_height: None branch of is_caught_up (and its "keep waiting" behavior) is not exercisable through any mock-driven caller. If the IsSyncing consumers in chain-gateway/src/state_viewer/traits.rs grow tests that care about the no-peers-yet path, extend the builder with a knob for it.
• crates/node/src/indexer.rs:432 — the loop still calls tokio::time::sleep(INTERVAL).await before the first poll, so first-boot always waits 500 ms before checking. Preserved from prior behavior, but worth noting since the fix is specifically about boot behavior. Not a bug, just an observation.
• Consider whether SYNC_HEIGHT_TOLERANCE = 5 at ~1 block/sec on NEAR could be flaky if a poll happens right after several peers advance ahead of the local head; if so, a small e2e regression around the wedge scenario would harden this. The unit tests exercise the predicate but not the wedge end-to-end.

⚠️ Issues found (all non-blocking — duplication and comment style)

[gilcu3]

the changes I am adding forced this constant to be bumped as tests would pass locally but not in CI

[gilcu3]

@claude review

this is a new solution, so please disregard your previous review

[claude]

Pull request overview

This is a fresh take on #3623 (the mpc-node indexer wedging at genesis on a freshly state-synced node). Instead of introducing a peer-height gate on Status { detailed: true }, this version derives catch-up purely from local signals: it requires REQUIRED_STABLE_POLLS = 4 consecutive non-syncing polls and the local head to have strictly advanced from where the run began. Because the wedge case (syncing == false reported transiently at genesis) leaves the head static, the additional head-progress check falls through and the loop keeps polling until neard reports syncing == true again.

Changes:

• Adds SyncProgress in crates/node/src/indexer.rs — a small state machine that observes (syncing, head_height) pairs and returns true only after a stable non-syncing run over which the head has advanced.
• Rewires IndexerClient::wait_for_full_sync to feed observations through SyncProgress.
• Adds 6 unit tests covering the syncing / static-head / not-enough-polls / caught-up / head-advances-late / resync-mid-run branches.
• Trims the over-long comment in real.rs (previous review flagged similar comments as too narrative).

Reviewed changes

Per-file summary

File	Description
crates/node/src/indexer.rs	Introduces SyncProgress + REQUIRED_STABLE_POLLS; wait_for_full_sync now returns only after a run of REQUIRED_STABLE_POLLS non-syncing polls with strict head advance. Adds 6 unit tests.
crates/node/src/indexer/real.rs	Comment trimmed to two-and-a-half lines (Streaming before the node is synced pins the LatestSynced cursor…). No logic change.

Findings

Blocking (must fix before merge): none.

Non-blocking (nits, follow-ups, suggestions):

• crates/chain-gateway/src/primitives.rs:26 — the chain-gateway crate still has its own IsSyncing::wait_for_full_sync that returns as soon as is_syncing() returns Ok(false). It's currently only wired up through ChainGateway::start in the tests (crates/chain-gateway/tests/common/node.rs:42), so this PR is scoped correctly, but the same wedge will land on that path the moment chain-gateway is promoted to a production streamer. Worth an issue / follow-up so the fix migrates alongside — a comment like TODO(#3623): mirror SyncProgress here once chain-gateway is a production path on the trait would make the dependency explicit.
• crates/node/src/indexer.rs:441 — wait_for_full_sync still calls tokio::time::sleep(INTERVAL).await before the first status poll, so first-boot always waits ~500 ms before observing anything, and the minimum wait from a fully-synced node to "caught up" is roughly 4 × 500 ms + INTERVAL ≈ 2.5 s. Preserved from prior behavior and probably fine, but the fix is about boot latency so worth noting. If desired, moving the sleep to the end of the loop body would let the first poll fire immediately.
• crates/node/src/indexer.rs:453 — the doc-comment on SyncProgress (Detects catch-up from head progress alone…pins the streamer's LatestSynced cursor at that stale head. So we require…) carries the long-form rationale that engineering-standards.md §Write helpful code comments item 5 recommends deferring to the issue. Consider trimming to a one-liner (Requires a run of non-syncing polls over which the head strictly advances; see #3623 for background.).
• crates/node/src/indexer.rs:474 — head_height > start_head is a fairly weak signal: any single-block advance during the 4-poll window (~2 s) counts. On a freshly state-syncing node the head genuinely does not advance while state sync is in progress, so this is safe in practice; but a stronger predicate — e.g. head_height >= start_head + REQUIRED_STABLE_POLLS - 1, requiring roughly one block per poll — would be more defensive against a hypothetical neard that reports syncing == false transiently while blocks tick over. Not required for correctness; noting the tradeoff since it is easy to strengthen later.
• crates/node/src/indexer.rs:463 — SyncProgress is pure business logic and could live in its own module (or at least a private sync_progress submodule) alongside its tests to keep indexer.rs focused on I/O. Purely organizational.
• No e2e regression around the wedge itself. Unit tests exercise the predicate exhaustively, but the wedge is a system-level bug and a regression here would revert to indefinite starvation. Consider a follow-up to add a test in crates/e2e-tests that boots a node whose neard is forced into the "transient syncing == false at genesis" state and asserts the streamer does not lock to genesis.
• crates/node/src/indexer.rs:422 vs crates/chain-gateway/src/primitives.rs:19 — INTERVAL: Duration = Duration::from_millis(500) now exists in two crates for two nearly-identical polling loops. Not new, but the divergence between the two wait_for_full_sync implementations widens with this PR; extracting a shared SyncProgress or at least the constant into a common crate would prevent them drifting further.

✅ Approved — logic is sound, well-tested, and appropriately scoped to the node-side wedge described in #3623. The items above are follow-ups, not merge blockers.

[barakeinav1]

Approving

Testing this on a fresh state-synced testnet CVM in parallel, since this bug's failure mode is in neard's runtime behavior rather than the predicate itself — the unit tests can't cover that. Will report back.

[kevindeforth]

Looks good, but I doubt the value of the LLM generated code comments.

[kevindeforth]

Thank you!

Note for future reference: this is a patch only that should be reverted once near/nearcore#16004 is fixed.

[barakeinav1]

Tested on testnet TEE — fix confirmed ✅

Built this branch (103c58b3) reproducibly and deployed 5 fresh idxtest CVMs, each state-syncing testnet from scratch — 5 independent rolls of the startup race.

Result: all 5 caught up to head, 0 wedges. The indexer streamer started at the live tip, not at genesis:

node	head	indexer height	streamer started at
15	257262010	257262010	#257260914
16	257262010	257262010	#257260853
17	257262011	257262011	#257260879
18	257262011	257262011	#257261024
19	257262011	257262011	#257260470

Throughout header→state→block sync the indexer correctly deferred (no starting streamer, no mpc_indexer_latest_block_height), then started at head only once fully synced. For contrast, on the old code a wedged node logged starting streamer ~1 s after Indexer waiting and pinned mpc_indexer_latest_block_height at genesis 42376888 while head advanced to ~257M.

Note: these are non-participant idxtest nodes, so this exercises the indexer/sync path only (not signing/attestation) — which is the scope of the fix.