fix(discovery-service): decide block canonicality, not mere existence

[avi-starkware]

Stacked on #938 — review that one first. This PR's diff against it is the canonicality change only.

Split: this PR is the canonicality fix. Answering it from the node's reorg notifications, and everything that entails, moved to #941.

What

ChainState::is_canonical promised canonical-chain membership but RpcBackend implemented an existence check. Answers canonicality from a hash→height→hash round trip instead.

Why

match self.inner.provider.get_block_transaction_count(BlockId::Hash(block_hash)).await {
    Ok(_) => Ok(true),
    Err(ProviderError::StarknetError(StarknetError::BlockNotFound)) => Ok(false),
    Err(e) => Err(ChainStateError::RpcError(e)),
}

"The node can still serve this hash" is not "this hash is on the chain". api/validators.rs relies on is_canonical to answer 409 / BLOCK_REORGED when a client's last_known_block was reorged out; with an existence check the reorged cursor is accepted and the client keeps extending state built on notes that have left the chain.

Being precise about the scope, because I tested the premise and it is narrower than it looks. Against this repo's own devnet, after devnet_abortBlocks the aborted hash immediately returns BlockNotFound — so once a node has locally processed a reorg, the existence check answers correctly. The real gap is the transient window before that specific endpoint has processed the fork, during which its storage legitimately still holds the old block. Structurally Pathfinder and Juno must behave the same way, since both key storage by height.

Impact is also bounded: validate_block_ref uses last_known_block only to decide whether to 409 — when it is supplied and block_ref is omitted, the query BlockId is get_head(), not last_known_block. So a false "canonical" verdict does not pin queries to orphaned data; it fails to fire the resync signal. A doomed spend still fails safely at the contract's nullifier check. And Starknet reorgs are incident-level rare — one documented mainnet case (Jan 5 2026, an 18-minute reversion to height 5,187,263).

How

1. Hash→height→hash. BlockId::Number addresses exactly one block, so comparing the hash the node currently carries at that height is a true canonicality test. New private block_by_id reuses the Block/PreConfirmedBlock match shape already in resolve_block_number; a pre-confirmed block reports None since it carries no hash.
2. L1-finality short-circuit. The second call is skipped when the first reports ACCEPTED_ON_L1 — that block's state update is finalized on Ethereum, so no reorg can replace it and the height lookup cannot change the answer. status is already on the block being fetched, so this costs nothing to read.
3. Docstring corrected to describe what is implemented, including the pre-confirmed case and that Err means "unknown", not a verdict.

Cost

Two RPC round trips per check instead of one — but only for cursors newer than L1 finality. Measured against hosted Sepolia:

head-0      13246784   ACCEPTED_ON_L2     2 calls
head-500    13246284   ACCEPTED_ON_L2     2 calls
head-5000   13241784   ACCEPTED_ON_L1     1 call
l1_accepted 13245300   (~1484 blocks behind head)

So anything older than roughly 1.5k blocks costs one round trip. The payload does grow — getBlockTransactionCount returns 35 B where getBlockWithTxHashes returns 1283–1354 B on those blocks (≈ 1143 B + 70 B per transaction) — but at ~310 ms round-trip latency the size difference was ~8 ms, i.e. noise. The dominant cost is the extra RTT, which the short-circuit removes for old cursors and #941 removes for recent ones.

The check runs only on first requests carrying last_known_block (validators.rs:26), never on pagination, and never for SimplePrivateTransfers, which drops its cursor in build().

Bounds and safety

No deadlock at max_concurrent_requests: 1 (relevant given #938): no permit is held between the two calls — each round trip acquires and releases inside the transport. Every test in the new file runs with a single permit, and test_canonicity_burst_completes_under_single_permit drives 8 concurrent double-checks (16 calls) through it.

Tests

Why this shipped unnoticed: test_api.rs's test_incoming_sync_block_reorged and the e2e reorg-recovery.test.ts both simulate a reorg with a hash that never existed (0xdeadbeef…), which only exercises the BlockNotFound path — trivially correct before and after.

New tests/test_chain_state_canonicity.rs (7 tests, mock node) covers: orphaned-at-reused-height → false (2 calls); canonical → true (2 calls); L1-accepted → true in 1 call; BlockNotFound → false (1 call); RPC error → Err; pre-confirmed height → false without panicking; single-permit burst. The mock deliberately keeps what each height resolves to now separate from every block still reachable by hash — that gap is what a canonicity check has to see through, and what a node's storage looks like right after a reorg.

Verified the tests have teeth. Restoring the old existence check verbatim fails 6 of 7 — though partly because the mock pins the method it serves, so get_block_transaction_count is rejected outright. Isolating just the verdict, by keeping get_block_with_tx_hashes but returning is_some():

test test_orphaned_hash_at_reused_height_is_not_canonical ... FAILED
  panicked: a hash the node still serves is not canonical once its height carries another block
test test_canonical_hash_is_canonical ... FAILED
test test_canonicity_burst_completes_under_single_permit ... FAILED
test result: FAILED. 4 passed; 3 failed

3 of 7, and the first is the semantic case this PR exists for. The four that still pass do not distinguish the two behaviours: an unknown hash and a pre-confirmed height are false either way, and an RPC failure is Err regardless. test_l1_accepted_hash_is_canonical_in_one_call also passes it — an existence check happens to return true in one call too — so that test pins the call count, not the verdict; the verdict is guarded by the tests above it. Removing the short-circuit alone fails it with left: 2, right: 1.

Honest caveat: test_aborted_block_is_not_canonical (real devnet, devnet_abortBlocks) also passes against the old code — devnet stops serving the aborted hash, so BlockNotFound answers it either way. Refilling the vacated height would not help either: devnet runs in --lite-mode, where a block's hash is derived from its number, so the replacement reuses the same hash (measured: both 0x2). It is there to validate the round trip against a real node's payloads, not to catch this regression, and its docstring now says so.

Verification

cargo fmt --check                  exit 0
cargo clippy --all-targets         0 warnings
cargo test -p discovery-service    86 passed, 0 failed

Includes #938's test_rpc_concurrency_limit.rs (4 tests), untouched and passing.

Specs

11-reorg-handling.md gains §11.1 "Canonicity Check (implemented)" with the old §11.1–11.5 renumbered, marking the boundary between today's behavior and the planned local cache; 05-security-considerations.md §5.4 and 06-api-design.md §6.4 updated. §11.2–11.6 and the reorgs_handled metric remain aspirational — no cache exists yet, so I labelled the boundary rather than rewriting them.

Cross-layer

No SDK change needed: it already sends last_known_block and maps 409/BLOCK_REORGED to ReorgError → resync. The only delta clients see is that a last_known_block replaced by a competing block at the same height now 409s where it previously returned 200 — which is what the existing SDK wiring wants.

Follow-up

e2e/reorg-recovery.test.ts still uses the never-existed hash. Worth upgrading to create a block, use its hash as last_known_block, then devnet_abortBlocks (needs --state-archive-capacity full). e2e was out of scope here.

---

This change is

[Yoni-Starkware]

@Yoni-Starkware reviewed 5 files and all commit messages, and made 2 comments.
Reviewable status: 5 of 8 files reviewed, 2 unresolved discussions (waiting on avi-starkware).

---

crates/discovery-service/src/rpc_backend.rs line 463 at r1 (raw file):

        // Two round trips: a height addresses exactly one block, so hash ->
        // height -> hash decides canonicity where knowing the hash cannot.
        let block = match self.block_by_id(BlockId::Hash(block_hash)).await? {

Are you fetching the enite block? can you fetch just the header/number or something like this?

Code quote:

let block = match self.block_by_id(BlockId::Hash(block_hash)).await? {

---

crates/discovery-service/src/chain_state.rs line 46 at r1 (raw file):

    ///   never be the one asked about.
    /// - `Err`: the node could not answer, leaving canonicity unknown. Callers
    ///   must not read this as either verdict.

Revert the over-documentation

Code quote:

    /// Check whether `block_hash` is the block the chain currently carries at
    /// that block's height.
    ///
    /// Canonicity is not existence: a node keeps serving an orphaned block by
    /// hash while its storage holds it. An L1-accepted block is the exception —
    /// finality settles its identity outright.
    ///
    /// - `Ok(true)`: `block_hash` is the canonical block at its height.
    /// - `Ok(false)`: the node has no such block, or now carries a different one
    ///   at that height. A height that resolves to a pre-confirmed block also
    ///   reports `false`, since a pre-confirmed block carries no hash and so can
    ///   never be the one asked about.
    /// - `Err`: the node could not answer, leaving canonicity unknown. Callers
    ///   must not read this as either verdict.

[avi-starkware]

@avi-starkware+AGNT made 1 comment.
Reviewable status: 5 of 15 files reviewed, 2 unresolved discussions (waiting on avi-starkware and Yoni-Starkware).

---

crates/discovery-service/src/rpc_backend.rs line 463 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Are you fetching the enite block? can you fetch just the header/number or something like this?

Checked the spec rather than the client, and there's no header-by-id request at any version: 0.10.4-rc.1 (newest, published 3 days ago) still exposes only getBlockWithTxHashes / getBlockWithTxs / getBlockWithReceipts, and the sole getBlockHeader in starknet-specs is in p2p/proto/sync/header.proto, i.e. the p2p sync protocol, not the JSON-RPC API.

So getBlockWithTxHashes is already the lightest of the three — the only excess over a header is the transactions array — but note BlockHeader carries no status field, which the L1-accepted short-circuit reads, so a header-only call would actually cost us that optimization.

The header-shaped alternative is the newHeads subscription (headers only, backfill capped at 1024 blocks back), which is the approach in #941.

[avi-starkware]

@avi-starkware+AGNT made 1 comment.
Reviewable status: 5 of 15 files reviewed, 2 unresolved discussions (waiting on Yoni-Starkware).

---

crates/discovery-service/src/chain_state.rs line 46 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Revert the over-documentation

Trimmed to 4 lines — this file's contribution to the PR went from 13 added lines, all of them comments, down to 3.

I didn't revert literally, because the original text said Ok(false) means "not found (orphaned or non-existent)", which is exactly the existence-vs-canonicity conflation this PR fixes — restoring it would re-document the bug. Kept the summary line, the "canonicity is not existence" claim, and the Err semantics:

/// Check whether `block_hash` is the block the chain carries at its height.
///
/// Canonicity is not existence: a node keeps serving an orphaned block by
/// hash. `Err` means the node could not answer, which is neither verdict.

The two details I cut were duplicated rather than lost: the L1-accepted exception is documented at the impl in rpc_backend.rs where the short-circuit lives, and the pre-confirmed case is already in specs/11-reorg-handling.md §11.1.