feat(sync): delta-aware pre_confirmed polling

[thiagodeev]

A follow-up work of #3601, implementing another feeder improvement.

Delta sync for get_preconfirmed_block — the poller now echoes knownBlockIdentifier and knownTransactionCount on every tick so the feeder gateway can answer with a no-change marker, a delta of appended transactions/receipts/state diffs, or a full block when the round identifier no longer matches. The sync layer reconciles the three response shapes against the stored pre_confirmed instead of unconditionally re-decoding the whole block on every poll.
The PR also uses the FeederAdapter type from the starknetdata pkg to make the node work normally with both old and new versions of the feeder, switching to the new approach as soon as the new feeder updates are detected in the network.

Reference: https://demerzelsolutions.slack.com/archives/C03090TS3TK/p1777375140952279

Note: candidate txs are no longer supported with this new update, but since old sequencer still uses it, it can only be fully removed in the future.

[codecov]

Codecov Report

❌ Patch coverage is 86.29442% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.51%. Comparing base (b44be8e) to head (a5e3520).

Files with missing lines	Patch %	Lines
adapters/sn2core/sn2core.go	78.26%	5 Missing and 5 partials ⚠️
sync/pending_polling.go	85.41%	5 Missing and 2 partials ⚠️
clients/feeder/feeder.go	71.42%	4 Missing ⚠️
starknetdata/feeder/feeder_adapter.go	76.47%	2 Missing and 2 partials ⚠️
core/pending/pending.go	93.33%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3617      +/-   ##
==========================================
+ Coverage   76.44%   76.51%   +0.06%     
==========================================
  Files         402      402              
  Lines       36770    36952     +182     
==========================================
+ Hits        28110    28273     +163     
- Misses       6681     6697      +16     
- Partials     1979     1982       +3     

☔ View full report in Codecov by Sentry.
📢 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.

[RafaelGranza]

LGTM. I only left a few nits.

I'll halt the approve because I'm waiting for the decision mechanism, which I think may be similar to the includeSignature one.

[github-actions]

Claude finished @thiagodeev's task in 7m 3s —— View job

---

PR Review

• Read changed files in clients/feeder/
• Read changed files in starknetdata/feeder/
• Read changed files in core/pending/
• Read changed files in sync/
• Read changed files in adapters/sn2core/
• Identify correctness/concurrency/error-handling issues
• Post inline comments for specific findings
• Post summary

---

Summary

The delta-aware polling design is clean and the reconciler in handlePreConfirmed correctly handles all three response modes (NoChange, Delta, Full) including the identifier-drift edge case. The FeederAdapter transition mechanism is well-tested. A few concerns below — none are blockers.

Important

• Nil-deref risk in AdaptPreConfirmedDelta (sn2core.go:651) — passing a nil *starknet.StateDiff to AdaptStateDiff panics on line 451. Legacy path guards via IsCandidateTx; delta path has no guard. Relies entirely on the upstream invariant — worth either an explicit nil check or a comment documenting the assumption.

Nits

• Non-atomic snapshot in pollPreConfirmed (pending_polling.go:282) — targetBlockNum and currentPreConf can drift if the target advances mid-loop. Self-heals via Full response, so just a wasted poll, but skipping the tick when they disagree would tighten it.
• Legacy-path identifier stamping (feeder_adapter.go:150) — BlockIdentifier = blockIdentifier makes the round-changed branch in shouldPreservePreConfirmed unreachable on legacy. Intended behavior, but it took a careful read to confirm. A short comment would help.
• runVerificationLoop retry cadence — not part of this PR's diff, but worth flagging: the 30-min interval with no shorter initial backoff means one transient first-probe failure costs 30 min on the slower legacy path. Low priority follow-up.

Other observations (no action needed)

• The Copy() before the delta-drift branch in handlePreConfirmed (pending_polling.go:376) is justified by the nextPreConfirmed = existing assignment in the drift branch (already discussed in the existing review thread) — good.
• shouldPreservePreConfirmed correctly handles the round-restart case raised in the earlier review — confirmed identifier comparison is in place.
• Concurrency between the three handlers (handleHead, handlePreLatest, handlePreConfirmed) is serialized through pollPendingData's single goroutine, so no internal race. Only cross-goroutine read is the snapshot in pollPreConfirmed, addressed above.
• Test coverage for the Full → Delta → New identifier → NoChange sequence in TestPollPendingData is solid.
  • Branch: thiagodeev/feeder-preconfirmed-optimization

[github-actions]

Potential nil-pointer panic if delta contains nil state diffs.

AdaptStateDiff immediately dereferences response.OldDeclaredContracts at the top of its body (sn2core.go:451), so passing a nil *starknet.StateDiff will panic. The legacy AdaptPreConfirmedBlock (above) guards against this via IsCandidateTx, but AdaptPreConfirmedDelta has no such guard — it relies entirely on the upstream invariant that delta responses never include candidate-style entries.

If the new feeder ever ships a malformed delta (e.g., padded with nil placeholders), this loop will panic the sync goroutine instead of returning a clean error. Worth either:

• adding an early if response.TransactionStateDiffs[i] == nil { return ..., errors.New("nil state diff in delta") }, or
• documenting the upstream guarantee explicitly here so the brittleness is intentional.

Same concern would apply to response.Receipts[i], but AdaptTransactionReceipt already nil-checks (sn2core.go:68), so receipts are safe.

[thiagodeev]

IMO, not relevant.
Delta responses are by definition never empty. If the sequencer breaks its response contract, everything else breaks

[github-actions]

Minor: non-atomic snapshot of targetBlockNum and currentPreConf.

blockNumberToPoll.Load() (line 267) and s.preConfirmed.Load() (line 278) aren't taken together, so they can disagree if pollPendingData advances the target (via handleHead/handlePreLatest) and replaces the stored pre_confirmed in between. The worst case is sending (targetBlockNum=N+1, blockIdentifier=oldId_for_N, txCount=oldCount_for_N): the server will see the identifier mismatch and return Full, which the reconciler then accepts — so it self-heals after one wasted round trip. Not a correctness bug, just a one-tick stale poll.

If you want to tighten it, you can either skip the poll when currentPreConf != nil && currentPreConf.Block.Number != targetBlockNum, or just derive targetBlockNum from currentPreConf when present. Up to you whether it's worth the noise.

[thiagodeev]

Not relevant IMO.
As said, if a new PreConfirmed block is stored between block number load and currentPreConfirmed load, and the node requests block X with a different identifier, the sequencer will return the full block, and the code will handle it properly

[github-actions]

Subtle: stamping the caller's identifier onto a legacy-path Full block.

On the legacy fallback, the upstream get_preconfirmed_block endpoint doesn't return an identifier, but we set adaptedPreConfirmed.BlockIdentifier = blockIdentifier (the caller's previously stored identifier — typically "0x0" on first poll, or whatever was stored before).

The downstream effect is that shouldPreservePreConfirmed (sync/pending_polling.go:54) will treat the new block as having the same identifier as the stored one, so the "round changed" branch is effectively unreachable on the legacy path. The reconciler then falls back to comparing TransactionCount only — which is the intended legacy behavior, but it took a careful read to figure that out.

A short comment here explaining the round-identifier semantics on the legacy path (or, alternatively, leaving BlockIdentifier empty/"legacy" so the intent is explicit) would save future maintainers a re-read.

[thiagodeev]

Not relevant IMO.
This code (FeederAdapter) will only live for a month, no need to be that strict for "future maintenance"

[RafaelGranza]

LGTM

[rodrodros]

This PR is wrong on several levels.

Architecture wise:

• One type containing three types instead of using Go's Polymorphism.
• Constructors that are called and then fields being initialized separately
• Two overlapping types sharing the same field
• Adapter functions not building the right type instead 3 arrays.

Style wise is also wrong with many style decisions that have been discussed numerous times and not being followed here.

[rodrodros]

This doesn't look good, calling the constructor for then setting a property.

[rodrodros]

Wouldn't you say that every function called PreConfirmedSmth is kind of extra, why not call it just Smth.

[rodrodros]

As far as I can see BlockIdentifier is not a string but always a smaller than a uint64. Why are you storing a string?

[rodrodros]

Why BlockIdentifier is defined twice, once here and nother inside PreConfirmed?

[rodrodros]

This description is non descriptive and explained in unnecessary complex vocabulary. What is a sync layer reconciler? Only the fileds appropriate for Mode are populated <- why does this sentece adds?

Update to:

// PreConfirmedUpdate represents the three types of responses we can have when fetching for 
// a pre-confirmed block:
//  1. NoChange the struct is empty
//  2. PreConfirmedDelta smth smth
//  3. smth smth

[rodrodros]

Why don't you add information to the error such as which size were expected, of what is the miss-match.

[rodrodros]

Remove, tired of mentioning the same thing

[rodrodros]

I don't see any adaptation, just three types being returned? Shouldn't it return the DeltaUpdated type?

[rodrodros]

Question: Why does this function returns a new PreConfirmed instead of re-using the current one. And if it is necessary to create a new one, why does it returns a reference to it, instead of returning it by value?

[rodrodros]

Re-mergeing all the state diffs again seems awfully inneficient. At least start from a copy of the other state diff