Skip to content

fix(sqlite-store): reject zero partial_blockchain_nodes.id instead of panicking - #2462

Closed
ygd58 wants to merge 2 commits into
0xMiden:nextfrom
ygd58:fix-partial-blockchain-node-zero-id
Closed

fix(sqlite-store): reject zero partial_blockchain_nodes.id instead of panicking#2462
ygd58 wants to merge 2 commits into
0xMiden:nextfrom
ygd58:fix-partial-blockchain-node-zero-id

Conversation

@ygd58

@ygd58 ygd58 commented Aug 27, 2026

Copy link
Copy Markdown

Searched for existing issues/PRs (NonZeroUsize, InOrderIndex, partial_blockchain_nodes, parse_partial_blockchain_nodes) before writing this - #1691 is the only related hit and it's a different concern (32-bit usize limiting the max representable index in WASM builds, not this panic).

Bug

parse_partial_blockchain_nodes called NonZeroUsize::new(id).unwrap() on a u64 id column read directly from the local sqlite database. InOrderIndex::new requires a NonZeroUsize (from_leaf_pos's minimum output is 1 - pos * 2 - 1 with pos >= 1), so a stored id of 0 is never something the client's own write path produces - it can only come from a corrupted or externally-tampered-with local database, and would panic the client instead of surfacing an error.

Fix

Routes this through the function's existing Result<_, StoreError> instead, using the already-existing StoreError::ParsingError variant - no new error variant needed, and matches the pattern the two adjacent conversions in the same function already use (the id/usize::try_from and the node/Word::read_from_bytes both already return errors rather than panicking; this brings the third field in line).

Test plan

Added a direct unit test constructing a zero-id row and asserting the error, without needing the full store/connection test harness. Ran locally, not just CI:

cargo test -p miden-client-sqlite-store parse_partial_blockchain_nodes_rejects_zero_id -- --nocapture

1 passed. Also ran the full crate suite: 91 passed, 0 failed - no regressions.

ygd58 added 2 commits August 27, 2026 23:56
… panicking

parse_partial_blockchain_nodes called NonZeroUsize::new(id).unwrap() on a
u64 id column read directly from the local sqlite database.
InOrderIndex::new requires a NonZeroUsize (from_leaf_pos's minimum output
is 1 - pos * 2 - 1 with pos >= 1), so a stored id of 0 is never something
the client's own write path produces - it can only come from a corrupted
or externally-tampered-with local database.

Routes this through the function's existing Result<_, StoreError> instead,
using the existing StoreError::ParsingError variant (no new variant
needed) - the same pattern already used for the sibling id/usize
try_from and the node Word::read_from_bytes conversions two lines below,
which already return errors rather than panicking.

Adds a direct unit test constructing a zero-id row and asserting the
error, without needing the full store/connection test harness.
@juan518munoz

Copy link
Copy Markdown
Collaborator

so a stored id of 0 is never something the client's own write path produces - it can only come from a corrupted or externally-tampered-with local database, and would panic the client instead of surfacing an error.

This is an edge case too far-fetched to consider handling by surfacing an error, the current unwrap is adequate, at most it could be expected with a more specific message, but the terminal would still throw that we tried to cast from zero in that specific line.

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