fix(rpc): report syncing status when starting block header is pruned

[Ehsan-saradar]

Summary

Fixes #3791.

In pruned mode, Handler.Syncing (rpc v8/v9/v10) could return false ("not syncing") while the node was in fact still syncing.

It read the starting block's header only to obtain that block's hash:

startingBlockHeader, err := h.bcReader.BlockHeaderByNumber(startingBlockNumber)
if err != nil {
    return defaultSyncState, nil // <-- pruned starting header lands here
}

With history pruning enabled, once the starting block falls outside the retained window its header no longer exists in the DB. The lookup errors, and the handler falls through to the default "not syncing" response — so a node that is actively syncing reports itself as fully synced.

Fix

The sync decision only depends on head vs highest block, so make that decision first. The starting block hash is purely informational metadata (starting_block_hash), so it is now read best-effort afterwards:

• if the starting header is available, starting_block_hash is set as before;
• if it has been pruned, the field is simply omitted instead of the node being reported as synced.

starting_block_num now comes from StartingBlockNumber() (already in memory) rather than the header, so it no longer depends on a DB read that pruning can invalidate.

Applied identically to rpc/v8, rpc/v9 and rpc/v10.

Testing

• Reworked TestSyncing in all three versions to match the new call order.
• Added a syncing with pruned starting block header case asserting the node still reports syncing when BlockHeaderByNumber fails.
• make lint and the rpc/v8, rpc/v9, rpc/v10 test suites pass locally.

[Copilot]

Pull request overview

This PR fixes incorrect starknet_syncing reporting in history-pruning mode by ensuring the syncing decision is made without depending on the starting block header (which may be pruned), and by fetching starting_block_hash only on a best-effort basis afterward.

Changes:

• Reorders Syncing() logic in rpc v8/v9/v10 to decide syncing from head vs highest first.
• Makes starting_block_hash optional (best-effort DB lookup) while always returning starting_block_num from StartingBlockNumber().
• Updates and extends tests to reflect the new call order and pruned-header behavior.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
rpc/v8/sync.go	Makes starting header lookup best-effort so pruning doesn’t cause false “not syncing”.
rpc/v8/sync_test.go	Reworks expectations for new call order; adds pruned-header syncing case.
rpc/v9/sync.go	Same best-effort starting header lookup and corrected sync decision order.
rpc/v9/sync_test.go	Updates expectations; adds pruned-header syncing case.
rpc/v10/sync.go	Same logic update as v8/v9.
rpc/v10/sync_test.go	Updates expectations; adds pruned-header syncing case.

Comments suppressed due to low confidence (3)

rpc/v9/sync_test.go:55

• This subtest is named as if it exercises the highestBlockHeader.Number <= head.Number branch, but HighestBlockHeader() is currently mocked to return nil (via the .Times(2) expectation above), so the handler returns early at highestBlockHeader == nil. This means the head>highest condition isn’t covered and the test name is misleading. Mock a non-nil highest header with a lower number (and adjust the nil expectation count) so the intended branch is actually tested.

	t.Run("block height is greater than highest block", func(t *testing.T) {
		mockReader.EXPECT().HeadsHeader().Return(&core.Header{Number: 1}, nil)

		syncing, err := handler.Syncing()
		assert.Nil(t, err)
		assert.Equal(t, &rpc.Sync{Syncing: &defaultSyncState}, syncing)
	})

rpc/v8/sync_test.go:55

• This subtest is named as if it exercises the highestBlockHeader.Number <= head.Number branch, but HighestBlockHeader() is currently mocked to return nil (via the .Times(2) expectation above), so the handler returns early at highestBlockHeader == nil. This means the head>highest condition isn’t covered and the test name is misleading. Mock a non-nil highest header with a lower number (and adjust the nil expectation count) so the intended branch is actually tested.

	t.Run("block height is greater than highest block", func(t *testing.T) {
		mockReader.EXPECT().HeadsHeader().Return(&core.Header{Number: 1}, nil)

		syncing, err := handler.Syncing()
		assert.Nil(t, err)
		assert.Equal(t, &rpc.Sync{Syncing: &defaultSyncState}, syncing)
	})

rpc/v10/sync_test.go:55

• This subtest is named as if it exercises the highestBlockHeader.Number <= head.Number branch, but HighestBlockHeader() is currently mocked to return nil (via the .Times(2) expectation above), so the handler returns early at highestBlockHeader == nil. This means the head>highest condition isn’t covered and the test name is misleading. Mock a non-nil highest header with a lower number (and adjust the nil expectation count) so the intended branch is actually tested.

	t.Run("block height is greater than highest block", func(t *testing.T) {
		mockReader.EXPECT().HeadsHeader().Return(&core.Header{Number: 1}, nil)

		syncing, err := handler.Syncing()
		assert.Nil(t, err)
		assert.Equal(t, &rpc.Sync{Syncing: &defaultSyncState}, syncing)
	})

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.38%. Comparing base (4940509) to head (8f06c43).
⚠️ Report is 23 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3797      +/-   ##
==========================================
+ Coverage   74.91%   75.38%   +0.46%     
==========================================
  Files         433      438       +5     
  Lines       38815    39489     +674     
==========================================
+ Hits        29079    29769     +690     
+ Misses       7733     7650      -83     
- Partials     2003     2070      +67     

☔ 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.

[EgeCaner]

We cannot let StartingBlockHash to be nil since field is required by the spec. Lets either return zero as default, or somehow cache the starting block hash to read from memory later

[EgeCaner]

StartingBlockHash is a required field of starknet_syncing response, see the spec ref.

When we leave empty (nil), field will be omitted. I guess we should either return zero when best effort fails, or we somehow cache the starting block hash then directly read the value from memory. Second approach sounds better to me since it will keep API functioning as is, but not sure if it would be easy to implement

[Ehsan-saradar]

Good catch on the spec, you're right that starting_block_hash is required — fixed it to fall back to 0x0 instead of omitting the field.

About caching: we can't cache the hash when sync starts, because the starting block is head+1 at that point — it hasn't been synced yet, so its hash simply doesn't exist. We could cache it later once the block is stored, but that means touching the sync package and its interfaces, and we'd still need the zero fallback for the window before the block lands. Since the field is purely informational, the zero default felt like the right trade-off.

[Ehsan-saradar]

@EgeCaner following up on the caching idea — sketched out what it would take, in case you think it's worth doing:

• a startingBlockHash atomic.Pointer[felt.Felt] on the Synchronizer, set in the store path when block.Number == startingBlockNumber, and reset together with startingBlockNumber in syncBlocks
• a new StartingBlockHash() on the sync.Reader interface (+ noop impl, mocks), used by the three RPC versions instead of the DB read
• the 0x0 fallback stays regardless, for the window before the starting block is stored, and for read-only nodes that never fill the cache

It fully covers the pruned case (in a long-running session the starting block always passes through Store before it can be pruned), at the cost of touching the sync package and its interfaces. Do you think it's worth it? Happy to do it as a follow-up to keep this PR small — or here if you'd rather have it now.