fix(rpc): report correct sync status in pruned mode

[cicr99]

Fixes #3791

starknet_syncing previously read the starting block hash from blockchain storage using the sync start block number. In pruned mode, that starting block header may have already been removed, causing the RPC method to return false even while the node is still syncing.

This PR stores the starting block header in the synchronizer once the starting block is successfully stored, and updates RPC v8/v9/v10 to read the starting block metadata from the synchronizer instead of prunable chain history.

Changes

• Add StartingBlockHeader() to the sync reader interface.
• Cache the starting block header in Synchronizer when the sync start block is stored.
• Clear the cached starting header when the sync loop exits.
• Update starknet_syncing in RPC v8/v9/v10 to use the synchronizer’s cached start header.
• Update mocks, fake syncers, sequencer implementation, and RPC sync tests.

[github-actions]

Claude finished @cicr99's task in 4m 50s —— View job

---

Review: fix(rpc): report correct sync status in pruned mode

• Gather context (diff, sync/sync.go, RPC handlers, node wiring)
• Understand the request (code review)
• Analyse correctness / concurrency / test coverage
• Post inline comments + summary

The core approach is right: caching the starting header in the synchronizer decouples starknet_syncing from prunable chain history, which is exactly what #3791 needs. Two things need attention before merge.

Important

1. Read-only blockchain mode (--remote-db) regresses to permanently false — sync/sync.go:568
startingBlockHeader is only populated from storeTask, but when readOnlyBlockchain is true syncBlocks returns right after pollLatest (sync/sync.go:476-479) and storeTask never runs. So StartingBlockHeader() always errors and starknet_syncing returns false forever — even though pollLatest keeps highestBlockHeader fresh and the head (advanced by the writer node) is behind. The old code worked here because it read the hash from the shared DB, which is being written. Suggested fix: cache as fast path, fall back to BlockHeaderByNumber when nil — that keeps the pruned-mode fix and restores read-only mode.

2. (nil, nil) from Sequencer.StartingBlockHeader() is a nil-deref landmine — sequencer/sequencer.go:226, rpc/v8/sync.go:41
The sequencer is the RPC syncReader (node/node.go:339), and Syncing() dereferences the header after checking only err. It's safe today purely because Sequencer.HighestBlockHeader() returns nil and the handler bails out at that check first — an accidental ordering dependency. Returning an error (as NoopSynchronizer does) plus a defensive startingBlockHeader == nil guard in all three handlers removes the hazard. The three new fakeSyncer stubs have the same shape.

Nits

• rpc/v10/sync_test.go:93 — the new "syncing with pruned starting block header" subtest is byte-for-byte identical to the "syncing" subtest in all three RPC versions; the only net effect is .Times(2) → .Times(3). It adds no coverage, and the behaviour this PR actually changes (Synchronizer.StartingBlockHeader() caching surviving a pruned storage lookup) has no test in sync/.
• sync/sync.go:381 — s.startingBlockNumber (plain *uint64) is now read from the store-callback goroutine. Currently race-free only because the deferred = nil runs after verifiers.Wait(); a CompareAndSwap(nil, block.Header) or an atomic height would not depend on that invariant.
• sync/sync.go:77 — StartingBlockNumber() now has no production callers, and the two methods mean subtly different things (run start height vs. first-stored header). Worth removing or documenting.

Verified as fine

• Reorg staleness is self-healing: startingBlockNumber is not reset across stream restarts, so re-storing the starting block overwrites the cached header.
• The "no sync status until the starting block is stored" window is not a regression — the old code's BlockHeaderHashByNumber(height+1) also failed until that block was stored.
• atomic.Pointer use and the cleanup in the syncBlocks defer are correct.

Note: I did not run the test suite — go build ./... was still fetching dependencies (the Rust VM build makes this slow in a fresh checkout), so the findings above are from reading the code. CI results should be checked independently.
• Branch

[github-actions]

(nil, nil) is a nil-deref landmine. The sequencer is wired in as the RPC syncReader (rpc.New(chain, &seq, ...) in node/node.go:339), and Handler.Syncing() now does startingBlockHeader.Hash / &startingBlockHeader.Number with no nil check after a nil error.

Today this happens to be safe only because Sequencer.HighestBlockHeader() returns nil and the handler bails out at the highestBlockHeader == nil check before the deref. That's an accidental ordering dependency: reorder the checks in Syncing(), or ever make HighestBlockHeader() return non-nil for the sequencer, and starknet_syncing panics.

Mirror what NoopSynchronizer does and return an error instead:

Suggested change

	func (s *Sequencer) StartingBlockHeader() (*core.Header, error) {
	return nil, nil // Not relevant for Sequencer. Todo: clean Reader
	}
	func (s *Sequencer) StartingBlockHeader() (*core.Header, error) {
	// Not relevant for Sequencer. Todo: clean Reader
	return nil, errors.New("StartingBlockHeader() not implemented")
	}

Same applies to the three fakeSyncer.StartingBlockHeader() implementations added in rpc/v{8,9,10}/subscriptions_test.go. Independently, I'd add a defensive if err != nil || startingBlockHeader == nil guard in each Syncing() handler so a misbehaving Reader can't panic an RPC handler.

Fix this →

[codecov]

Codecov Report

❌ Patch coverage is 81.39535% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.31%. Comparing base (2c10d48) to head (dd0db66).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
sync/sync.go	71.42%	6 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3864      +/-   ##
==========================================
- Coverage   75.43%   75.31%   -0.12%     
==========================================
  Files         442      442              
  Lines       39862    39920      +58     
==========================================
- Hits        30068    30065       -3     
- Misses       7713     7760      +47     
- Partials     2081     2095      +14     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

Claude finished @cicr99's task in 6m 35s —— View job

---

Review: fix(rpc): report correct sync status in pruned mode

• Gather context (re-read diff after 5 new commits)
• Verify previous review points were addressed
• Analyse correctness / concurrency / test coverage
• Run go vet on changed packages; check CI
• Post inline comments + summary

All five points from the previous round were addressed: the read-only/--remote-db regression now has a storage fallback, Sequencer.StartingBlockHeader() returns an error, all three Syncing() handlers guard startingBlockHeader == nil, the duplicated "pruned starting block header" subtests are gone, and there are real tests in sync/. Ege's requests are in too — partial header decode via core.GetBlockHeaderHashByNumber, subtest-scoped mock expectations, StartingBlockNumber() retired. The design is sound.

Blocker

golangci-lint is red — sequencer/sequencer.go:223
lll: line is 114 chars, max 100. One-line fix (suggestion posted).

Important

1. s.startingBlockNumber races, and can nil-deref an RPC handler — sync/sync.go:556-575, sync/sync.go:376
It's a plain *uint64 written by the syncBlocks goroutine, and this PR adds two new readers on other goroutines: storeTask (verifier) and StartingBlockHeader() (RPC). The nil-check at line 561 and the derefs at 565/570 are separate statements, so a shutdown landing in between panics inside starknet_syncing. Making it atomic.Pointer[uint64] removes the race and the panic.

2. Non-atomic Load → DB read → Store on the cache — same comment
The partial fallback header can clobber a full header written concurrently by storeTask, and can clobber the defer's Store(nil), leaving a stale starting header cached with err == nil after the sync loop exits. Neither is user-visible today (only Hash/Number are consumed; Syncing() bails on nil highestBlockHeader first) — but both are silent invariant breaks. CompareAndSwap(nil, header) fixes both.

3. Coverage gap: the fix for #3791 itself is untested — sync/sync_test.go:184-229
All three new tests pass readOnlyBlockchain: true, which returns from syncBlocks right after pollLatest — so storeTask never runs and the startingBlockHeader.Store(block.Header) line is never executed. What's covered is the fallback path (the --remote-db fix), not the pruned path. The distinction is real: with the fallback in place, a pruned node only needs the storeTask cache when nobody calls starknet_syncing before the block is pruned — exactly #3791's scenario.

Nits

• sync/sync.go:76 — StartingBlockHeader() returns a full header on the cached path but a Number+Hash-only header on the fallback path. Worth documenting on the interface, or returning (hash, number, err) so the partial shape is in the type.
• sync/sync_test.go:253-278 — TestStartingBlockHeaderFallbackUnavailable's EventuallyWithT(assert.Error) passes on the first iteration and passes identically in the "not running" state, so it can't fail for the reason it's named.
• Read-only mode caches the starting hash permanently once the writer node stores that block; a writer-side reorg of the starting block leaves a stale hash where the old code re-read it each call. Self-healing in normal mode (re-storing the starting height overwrites the cache), but not here. Low impact.

Verified as fine

• RPC handlers: moving StartingBlockHeader() below the highestBlockHeader.Number <= head.Number check is behaviour-preserving and skips the read on the synced path, as requested.
• v8/v9/v10 Syncing() all guard err != nil || startingBlockHeader == nil — nil-deref hazard closed for Sequencer and the fakeSyncers.
• New "nil starting block header" subtest covers the (nil, nil) case explicitly; mock expectations are now subtest-scoped.
• Reorg staleness self-heals in normal sync mode: startingBlockNumber survives stream restarts, so re-storing the starting block overwrites the cache.

go vet ./sync/... ./rpc/v{8,9,10}/... ./sequencer/... passes locally (this compiles the test files, so the new tests build). I could not run go test — the command isn't in this job's allowlist, so add Bash(go test:*) to --allowedTools if you want test execution here. The Juno Test CI run was still in progress at review time; check it independently.
• Branch

[github-actions]

Claude finished @cicr99's task in 5m 51s —— View job

---

Review: fix(rpc): report correct sync status in pruned mode

• Gather context (diff after latest push: e25f68d, 31d0c71)
• Verify previous review points were addressed
• Analyse correctness / concurrency / test coverage
• Run go vet on changed packages; check lint constraints
• Post inline comments + summary

Everything from the last round is fixed: startingBlockNumber is now atomic.Pointer[uint64], the fallback uses CompareAndSwap so it can't clobber the full header from storeTask, the lll blocker on sequencer.go is gone, TestStartingBlockHeaderFallbackUnavailable now waits for the run to start, and TestStartingBlockHeaderCachesStoredHeader finally covers the actual #3791 path (readOnlyBlockchain: false, header deleted from storage, full header still returned). No blockers this round — what's left is one simplification and two test nits.

Nits

1. Consider not caching the fallback header at all — sync/sync.go:559-589
The caching is what forces the identity re-check + CAS + CAS-lost branch, and it still leaves a two-statement window: if syncBlocks's defer lands between the startingBlockNumber.Load() != startingBlockNumber check (579) and the CompareAndSwap (583), a header gets cached with no run in flight, and since the cache is consulted first, every later call returns it with err == nil instead of "not running". Unreachable in practice (Run() is single-shot, Syncing() bails on nil highestBlockHeader), but the fallback is one key lookup on a path that only runs while unsynced and before this node stored the starting block — caching buys little. Dropping it also removes the stale-hash-after-writer-reorg case for --remote-db. Suggestion posted; alternative is re-checking identity after the CAS and rolling back.

2. Pin the error in TestStartingBlockHeaderFallbackUnavailable — sync/sync_test.go:334
require.Error passes identically for the intended "fallback lookup missed" state and a future "not running" regression. require.ErrorIs(t, err, db.ErrKeyNotFound) makes it match the test's name.

3. Deleting the header mid-run causes a store-retry spin — sync/sync_test.go:275
Benign as written (assertion + cancel() follow immediately), and deleting before cancel() is unavoidable since the defer clears the cache. Worth a comment at most.

4. Known gap, probably out of scope: --remote-db and a pruned DB together still report syncing: false — storeTask never runs so the cache is never populated, and the fallback lookup fails. Same shape as #3791 but for read-only nodes; fixing it would need the starting hash from somewhere other than prunable history.

Verified as fine

• Sequencer.StartingBlockHeader() returns an error, and all three Syncing() handlers guard err != nil || startingBlockHeader == nil — no nil-deref path left for Sequencer or the fakeSyncers. Behaviour for sequencer nodes is unchanged (false either way, since HighestBlockHeader() is nil).
• The Reader interface now documents that the header may be partial (Number/Hash only).
• storeTask uses Store (not CAS) so a reorg re-storing the starting height, or a full header replacing a partial fallback one, both overwrite correctly; the Store happens before OnSyncStepDone, so it's published before observers see the step.
• startingBlockHeader.Store from the verifier goroutine still can't race the syncBlocks defer — that defer only runs after verifiers.Wait().
• block.Header isn't mutated on the sync Store path (updateBlockHash is only reached via Finalise/Simulate), so require.Equal(t, block0.Header, header) is a sound assertion.
• RPC mock expectations are now subtest-scoped, the duplicated "pruned starting block header" subtests are gone, and the new "nil starting block header" subtest covers (nil, nil) explicitly.
• No stale StartingBlockNumber references remain outside mocks/.

go vet ./sync/... ./sequencer/... ./rpc/v{8,9,10}/... passes (this compiles the test files). I could not run go test — it's not in this job's allowlist; add Bash(go test:*) to --allowedTools for test execution here. golangci-lint was still in progress; I checked manually that no changed file has a line over 100 chars beyond the pre-existing ones, so the previous lll failure should be resolved. Juno Test was also still running — check it independently.
• Branch

[github-actions]

Claude finished @cicr99's task in 5m 16s —— View job

---

Review: fix(rpc): report correct sync status in pruned mode

• Gather context (diff after latest push: dd0db66)
• Verify previous review points were addressed
• Analyse correctness / concurrency / test coverage
• Run go vet on changed packages; check CI
• Post inline comments + summary

Everything raised last round is now settled. No blockers; one test-robustness nit is all that's left, and it's optional.

On the fallback-caching discussion

Reordering the startingBlockNumber == nil guard before the cache read (sync/sync.go:560-568) does close the user-visible hole I flagged, and your reasoning holds: the defer stores nil to startingBlockNumber before startingBlockHeader, so once the run ends every call short-circuits to "not running" regardless of what's left in the cache. A header cached by the CAS after the defer has landed is inert — nothing can read it, and Run() is single-shot in node.go:409, so no later run can pick it up either. Keeping the caching for --remote-db is a fair trade, and TestStartingBlockHeaderFallsBackToBlockchain now actually pins that behaviour (delete the header, still get the cached one). Dropping this thread.

Nit

Blocking send on the sync goroutine in TestStartingBlockHeaderCachesStoredHeader — sync/sync_test.go:255-261
storedStartingBlock has a buffer of 1 and the test drains one value, so a third OpStore/block-0 callback would block storeTask — and syncBlocks only returns after verifiers.Wait(), so <-done would hang to the go test timeout instead of failing. Reachable in principle because the test deletes block 0's header mid-run and the store-failure path can reach revertTask → RevertHead, resetting the height. A non-blocking select/default send fixes it; suggestion posted.

Verified as fine

• StartingBlockHeader(): the post-DB-read identity check (startingBlockNumber.Load() != startingBlockNumber) is sound — startingHeight is a fresh local per syncBlocks call and is not reassigned on stream restarts, so pointer identity is a valid run token. The CAS-lost branch correctly handles Load() returning nil (defer landing between the failed CAS and the reload).
• core.GetBlockHeaderHashByNumber partial decode is safe: Hash is the first field of core.Header (core/block.go:37), and the accessor errors out if Hash is nil — so the fallback header can never carry a nil Hash into Sync.StartingBlockHash.
• s.db is always non-nil (node.go:415 passes database), so the fallback can't nil-deref.
• No StartingBlockNumber references remain outside the Sync JSON field and mocks/.
• All three Syncing() handlers guard err != nil || startingBlockHeader == nil; Sequencer and the fakeSyncers return an error, so no nil-deref path.
• TestStartingBlockHeaderFallbackUnavailable now pins db.ErrKeyNotFound, so it can't pass for a "not running" regression.
• storeTask publishes the full header with Store (not CAS), so it correctly upgrades a partial fallback header and survives a reorg re-storing the starting height; the Store precedes OnSyncStepDone, so the listener signal in the test can't fire early.

go vet ./sync/... ./sequencer/... ./rpc/v{8,9,10}/... passes (compiles the test files). golangci-lint, CodeQL and Code Quality are green on dd0db66; Juno Test was still in progress at review time — check it independently. I could not run go test here (not in this job's allowlist; add Bash(go test:*) to --allowedTools if you want test execution).
• Branch