Skip to content

fix(state): canonical SMT value from 64-byte wallet commit messages - #135

Merged
TaprootFreak merged 1 commit into
developfrom
fix/canonical-smt-value-from-wallet-commit-message
May 28, 2026
Merged

fix(state): canonical SMT value from 64-byte wallet commit messages#135
TaprootFreak merged 1 commit into
developfrom
fix/canonical-smt-value-from-wallet-commit-message

Conversation

@TaprootFreak

Copy link
Copy Markdown
Contributor

Summary

The e2e regression test second_send_succeeds_without_prev_commitment_pubkey_field added in PR #132 fails on DEV with 500 prove_account_update_with_in_and_out_coins_and_sources failed. This PR fixes the underlying protocol-level mismatch between what the server stores into the commitment SMT and what the in-circuit gadget reconstructs — the bug surfaces on every second send from the same account, regardless of prev_commitment_pubkey.

Root cause

Two on-the-wire commitment shapes hit /api/commit and the scanner:

Source Commitment.message Schnorr signs
Mint flow (shared::ClientAccount::create_commitment) 32 bytes = digest_to_bytes(hash_concat(ash, ocr)) (canonical Poseidon digest) the 32-byte digest directly
Wallet send-commit (zk-coins/app/rust/client/src/lib.rs::create_commitment) and the e2e TestWallet::sign_commit 64 bytes = account_state_hash || output_coins_root sha256(message)

State::update stored digest_from_bytes(commitment.get_account_state_hash()) into the SMT — which round-trips correctly for the 32-byte path (mint), but for the 64-byte path produced digest_from_bytes(sha256(ash \|\| ocr)). The in-circuit gadget (CommitmentMerkleProofs::commitment() and the matching circuit/main.rs SMT-inclusion check) expects hash_concat(ash, ocr) — Poseidon, not sha256. The two never reconcile, so any account that built its account.proof through a wallet-shaped commit silently couldn't ever pass prove_account_update.

PR #132's new e2e is the first to exercise two consecutive sends from the same account; everything before only tested prove_initial, which doesn't consult prev_cmp. The bug had shipped years ago but stayed unobserved until that test landed.

Fix

State::update now branches on commitment.message.len():

  • 64 bytes (wallet wire): split into 32-byte ash + 32-byte ocr, lift each through digest_from_bytes, store hash_concat(&ash, &ocr). Matches the in-circuit canonical commitment exactly.
  • Anything else: existing path digest_from_bytes(commitment.get_account_state_hash()) — preserves the 32-byte mint canonical (round-trips back to the same hash_concat(ash, ocr) digest as the 64-byte path) and keeps existing arbitrary-length test fixtures working.

Mint commitments and scanner-replay commitments are unaffected (they're 32-byte canonical and round-trip identically before and after this PR).

Why no migration

Migration 0012 (added in PR #132) wiped the accounts table. The stale wallet-shaped SMT entries persisted before that wipe are unreferenced — no account.proof points at any pre-fix commitment_history_root. The MMR keeps growing on top of the existing sequence; old leaves are dead weight, not corruption.

Test plan

  • cargo fmt --check
  • cargo clippy -p node -p shared -- -D warnings
  • cargo clippy -p node --all-features -- -D warnings
  • cargo check --workspace --all-features --tests
  • cargo test -p node --lib state::tests::update_with -- --test-threads=1 — both new unit tests green; existing test_update_with_* re-run, no regression
  • CI green (ci:full label set)
  • After deploy to DEV: second_send_succeeds_without_prev_commitment_pubkey_field passes in the api_remote suite
  • After deploy to DEV: account_node_tests::test_send_coins_second_send_succeeds_without_prev_commitment_pubkey keeps passing in the slim lane (uses the 32-byte canonical path; behaviour preserved by the else branch)

New regression tests

  • state::tests::update_with_64_byte_wallet_commitment_stores_canonical_hash_concat — pins the 64-byte wallet wire format → canonical SMT value contract.
  • state::tests::update_with_32_byte_canonical_commitment_stores_same_hash_concat_as_64_byte_form — equivalence test between the two wire formats for the same (ash, ocr) pair.

Follow-up (out of scope)

  • zk-coins/app can migrate to canonical 32-byte commit messages once the wasm exposes a Poseidon helper. Until then this PR keeps the wallet's current wire format working end-to-end.

`State::update` previously inserted the BIP-340 message digest into
the commitment SMT, which only happened to match the in-circuit
`CommitmentMerkleProofs::commitment()` invariant for 32-byte
canonical-digest messages (mint flow). The wallet wire format ships
a 64-byte `account_state_hash || output_coins_root` concatenation,
and `get_account_state_hash` returned `sha256(message)` for that
shape — so the SMT leaf was sha256(asth||ocr) instead of the
canonical `hash_concat(asth, ocr)`. The in-circuit SMT inclusion
check rejected wallet-shaped commitments, surfacing as
`prove_account_update_with_in_and_out_coins_and_sources failed` on
the second send from any wallet-built account
(`second_send_succeeds_without_prev_commitment_pubkey_field`,
PR #132).

Special-case the 64-byte message shape: split into `ash || ocr`,
reinterpret each half via `digest_from_bytes`, and store
`hash_concat(ash, ocr)`. The 32-byte canonical path round-trips
through `digest_from_bytes` to the same canonical digest, so mint
commitments keep working unchanged. Non-32/non-64 fixtures
(test-only) keep the legacy sha256 fallback to avoid a tests-only
refactor; production callers never produce that shape.

Regression tests pin the contract that 64-byte wallet messages
produce the in-circuit canonical SMT value and that the two
on-the-wire shapes agree on the SMT entry over identical (ash, ocr)
halves.

No migration needed: stale wallet-commitment entries refer to
account rows wiped by migration 0012 and are unreferenced.

Signed-off-by: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com>
@TaprootFreak TaprootFreak added the ci:full Trigger heavy CI jobs (Server + Shared Tests + Coverage Gate, ~60-90 min on M3 Ultra) label May 28, 2026
@TaprootFreak
TaprootFreak marked this pull request as ready for review May 28, 2026 10:23
@TaprootFreak TaprootFreak added ci:full Trigger heavy CI jobs (Server + Shared Tests + Coverage Gate, ~60-90 min on M3 Ultra) and removed ci:full Trigger heavy CI jobs (Server + Shared Tests + Coverage Gate, ~60-90 min on M3 Ultra) labels May 28, 2026
@TaprootFreak
TaprootFreak merged commit 7ddbc94 into develop May 28, 2026
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:full Trigger heavy CI jobs (Server + Shared Tests + Coverage Gate, ~60-90 min on M3 Ultra)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant