Skip to content

feat: pin the Sync State Contract with a validator, attack corpus, and read enforcement - #2582

Open
zancas wants to merge 1 commit into
pr2574_review_suggestionsfrom
feat/sync-state-contract
Open

feat: pin the Sync State Contract with a validator, attack corpus, and read enforcement#2582
zancas wants to merge 1 commit into
pr2574_review_suggestionsfrom
feat/sync-state-contract

Conversation

@zancas

@zancas zancas commented Jul 29, 2026

Copy link
Copy Markdown
Member

A session auditing every site that sets sync_state found the documented scan-range invariant ("in block height order with no overlaps or gaps") enforced only by construction inside the sync engine, and unchecked at the one production seam that can produce a non-trivial state: SyncState::read on wallet load. A wallet file violating the invariants did not fail loudly — it silently corrupted every consumer that trusts them, from fully_scanned_height to the spend horizon, and a file carrying an inverted scan range panicked the load inside the ScanRange::from_parts assert. This PR states the contract explicitly and makes the seam enforce it.

SyncState::validate is a pure, side-effect-free validator over the self-contained invariants: scan-range well-formedness and contiguity, shard-range order per pool, and tree-bound orientation. It reports the first violation in deterministic field-then-index order as a SyncStateIntegrityError, one variant per distinct violation, each carrying the convicting evidence. SyncState::read now rejects inverted scan-range bytes as InvalidData before the constructor assert can panic the process, and validates the assembled state before returning it.

The contract was built test-first from an adversarial corpus: one hostile wallet state per test, each violating a distinct invariant, plus honest controls, two file-seam attacks mounted through serialized bytes, and a maximally adversarial wallet whose violations the closing test peels off one at a time in the documented reporting order. Constructing the corpus proved two boundaries. Inverted scan ranges are unrepresentable — the constructor asserts and the fields are private — so the enum carries no variant for them, and a should-panic test witnesses why. And the previously-scanned counts admit no bound at all: a mid-session reorg truncation legally shrinks the scanned ranges below the counts recorded at session start, so rejecting such states would convict legally produced wallets. The defect there is the status math, whose six u32 subtractions in get_sync_status now saturate instead of underflowing in the post-truncation window; a corpus control pins that truncation drift passes validation.

The base is #2575, whose NoSyncData contract work is adjacent; this PR merges after it in the stack.

🤖 Generated with Claude Code

…d read enforcement

A session auditing every site that sets sync_state found the documented
scan-range invariant enforced only by construction inside the sync
engine, and unchecked at the one production seam that can produce a
non-trivial state: SyncState::read on wallet load. This commit states
the contract explicitly and makes that seam enforce it.

SyncState::validate is a pure validator over the self-contained
invariants: scan-range well-formedness and contiguity, shard-range
order per pool, and tree-bound orientation. It reports the first
violation as a SyncStateIntegrityError carrying the convicting
evidence. SyncState::read now rejects inverted scan-range bytes as
InvalidData before the ScanRange::from_parts assert can panic the
process, and validates the assembled state before returning it.

The contract was built test-first from an attack corpus: one wallet
per test, each violating a distinct invariant, plus two file-seam
attacks and a maximally adversarial wallet whose violations the peel
test removes one at a time in the documented reporting order.
Constructing the corpus proved two boundaries. Inverted scan ranges
are unrepresentable (the constructor asserts, pinned by a should-panic
witness), so the enum carries no variant for them. And the
previously-scanned counts admit no bound at all: a mid-session reorg
truncation legally shrinks the scanned ranges below the counts
recorded at session start, so rejecting such states would convict
legally produced wallets. The defect there is the status math, whose
six u32 subtractions in get_sync_status now saturate instead of
underflowing in the post-truncation window.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
});
}
if let Some(next) = shard_ranges.get(index + 1)
&& range.end > next.start

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in most cases, a single block will contain note commitments for the end of one shard and the start of the next shard. this means that the exclusive range end will often be 1 higher than the next shard ranges start bound, but sometimes be equal to it where the boundary falls exactly at the end of the block. this means that we should be asserting that each range end is either equal to or 1 larger than the next shard ranges start bound

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be clear:

  • scan ranges are contiguous
  • shard ranges may contain the same block twice at the boundary

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another note for context, if there are blocks between shards with no note commitments the end bound is already extended up to the next shard range to prevent gaps during shard range construction. gaps are not permitted but overlaps of 1 block are permitted. this prevents holes in scanning by covering the entire chain by at least one shard range but allows an entire shard to be prioritised, guaranteeing all note commitments for the prioritised shard will be scanned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants