fix(dapi): truthful state sync status, health height field collision, client crash on absent sections - #4532
Conversation
…are absent Every message-typed field of GetStatusResponseV0 is optional on the wire. DAPI omits the state_sync section on nodes that are not state syncing, and omits node/chain/network when Tenderdash is unreachable, so createFromProto threw a TypeError on v0.getStateSync().getTotalSyncedTime() for any ordinary node. Guard every optional sub-message read: absent node/chain/network/state_sync sections now yield null instead of throwing, and absent version sub-messages yield undefined. Also covers protocol.drive (absent when Drive is down) and protocol.tenderdash, which had the same unguarded chain. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
build_state_sync_info gated the state sync section on the Tenderdash strings being non-empty, but Tenderdash serialises every one of those fields as a quoted integer that is always present. The gate passed on every reachable node and getStatus emitted an all-zero StateSync message that asserts "state sync ran and did nothing" - indistinguishable from "this node never state synced", and identical on a node that had just restored a snapshot. In Tenderdash 1.7 six of the eight values are hardcoded zero regardless: /status only copies the state sync counters when Environment.StateSyncMetricer is set (internal/rpc/core/status.go:91) and nothing ever assigns that field (internal/rpc/core/env.go:86 has no writer). total_synced_time and remaining_time are the only ones that can move, and they belong to the block sync reactor (internal/blocksync/reactor.go:308-328), not state sync. Emit the section only when Tenderdash actually reported a non-zero value, and omit it otherwise so clients can tell the difference. Also rename the /health Core RPC height from latestBlockHeight to coreBlockHeight. It carries Dash Core's getblockcount, while latestBlockHeight in getStatus's chain section is the Platform height; a freshly joined node showing a Core height of ~8000 next to a Platform height of 28 was read as a broken Platform height. The getStatus mapping itself is correct - Tenderdash's /status exposes no Dash Core chain fields at all. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
total_synced_time and remaining_time are owned by Tenderdash's block sync reactor and are set on any node merely replaying blocks, so treating them as evidence of state sync raised a section named after state sync on nodes that never touched a snapshot. Gate the section on the six genuinely state-sync-specific counters instead. Also make js-dapi-client's time section follow the same guard as the other optional sections rather than silently yielding undefined field values. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe JavaScript client now tolerates absent status protobuf sections. The Rust status service narrows state-sync detection and distinguishes Platform and Core block heights in health output. ChangesJavaScript status parsing
Rust status reporting
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to Status responses now omit misleading empty state-sync data, distinguish Core height naming, and safely represent absent optional sections without client exceptions. The covered changes are ready to merge. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Final review complete — no blockers (commit bdff957) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Sol-only technical fallback
The status builder now omits stateSync unless state-sync-specific counters justify the section, while preserving the existing timing payload when genuine state-sync data is available. The proposed blocker conflates presence gating with payload preservation; the implementation, regression tests, and existing protobuf schema make that distinction explicit, so no actionable findings remain.
Source: reviewer 1: gpt-5.6-sol (agent: sol-fallback-reviewer, role: general); reviewer 2: gpt-5.6-sol (agent: sol-fallback-reviewer, role: rust-quality); final verifier: gpt-5.6-sol (agent: sol-verifier, role: final-verifier)
One or more required Phase-1 GLM Flash lanes remained technically unusable after the bounded exact-model retry. Their evidence was discarded as authoritative, and the complete selected role cohort was rerun fresh on exact gpt-5.6-sol before this fresh Sol verifier produced the final decision. No additional Phase-2 reviewer pass ran.
Review provenance
- Phase 1 GLM evidence: technically unusable after bounded retry; discarded from the decision
- GLM failure attempts:
codex-general-259753a23f144b6392bd0f78eb193856(failed),codex-general-55495ae61dd540818d15ac60e0dd5ff6(failed),codex-rust-quality-478f1aae62394461b64986f1f9b6a58a(failed),codex-rust-quality-8d92ce1cc5b94fdebcf6375637ba6a5f(failed) - Sol-only fallback reasons:
launch_transport_or_nonzero_exit,launch_transport_or_nonzero_exit - Sol-only fallback reviewers:
gpt-5.6-sol— general (completed); agentsol-fallback-reviewer,gpt-5.6-sol— rust-quality (completed); agentsol-fallback-reviewer - Fresh verifier (Sol):
gpt-5.6-sol— final-verifier; agentsol-verifier - Additional Phase 2 pass: not run; the Sol-only fallback is final
|
requesting review @shumkov |
PastaPastaPasta
added
the
ready for final review
3 participants
Issue being fixed or feature implemented
Three status-reporting bugs found during state-sync QA (all observed live on a state-synced local node). Independent of the state-sync feature PRs — these affect today's v4.2-dev.
What was done?
stateSynccounters. Tenderdash 1.7 never populates the six state-sync counters in/status—env.StateSyncMetricerhas no assignment site anywhere in tenderdash (the reactor is dropped intonode.serviceswith no reference kept, and*statesync.Reactordoesn't even implement theMetricerinterface), so the fields are always"0". rs-dapi's emit gate was additionally inverted (json:",string"means the fields are never absent, so the "is any data present" check always passed). getStatus now emits thestateSyncsection only when one of the six state-sync-specific counters is non-zero — absent rather than "state sync ran and did nothing" — and block-sync progress fields no longer leak into the state-sync section. The precise tenderdash-side fix (wire the reactor intorpcEnv, implement the two missingMetricermethods, snapshot syncer values beforesyncComplete()nils them, persist across restart) is documented in the code and being addressed separately./healthno longer reports a Core height under the namelatestBlockHeight. getStatus'schain.latestBlockHeightwas always correct (tenderdash platform height); the collision waschecks.coreRpc.latestBlockHeighton the unversioned/healthendpoint, sourced from Coregetblockcount— the exact number QA observed. Renamed tocoreBlockHeight(no in-repo consumers of the old key), with a regression test pinning thatchain.latest_block_heightis the platform height.getStateSync(),getNode(),getChain(),getNetwork(),getTime(), and the nested version getters were all called unguarded; any of them absent (never-synced node, Drive or tenderdash unreachable) threw a TypeError. All optional sub-messages are now guarded, with unit tests for the absent-section shapes.How Has This Been Tested?
rs-dapi: 307 unit tests passing including 7 new/changed (mid-sync, post-sync, never-synced, block-sync-only fixtures; height-collision regression). js-dapi-client: 320 passing including new absent-section cases. fmt/clippy clean. code-review-validator gate run; both valid findings fixed.
Breaking Changes
/health'schecks.coreRpc.latestBlockHeightkey is renamed tocoreBlockHeight(unversioned diagnostics endpoint; no in-repo consumers — external scrapers of that exact key would need the one-word update). getStatus omits thestateSyncsection when there is no state-sync data instead of emitting zeros.Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Changes
latestBlockHeighttocoreBlockHeightto distinguish it from the Platform chain height.