fix(storage): give /b0x/ack its own minimal wire contract (route-contract mismatch broke every acknowledgement) - #630
Merged
Conversation
…full-envelope validation
`ack_b0x_batch` validated every batch entry with `validate_canonical_envelope_v3_bytes`, which
requires `Envelope.version == 3`, `Envelope.headers`, and `Envelope.message_id`. An acknowledgement
semantically consumes ONLY the 16-byte transport `message_id` — the client is retiring ids it
already pulled and has no version, headers, or payload to send. Every well-formed ack was therefore
rejected 400 by every node, surfacing to the client as `ack quorum not met: 0/3 (K=3)`: inbox rows
were never retired, the receiver re-pulled the same item each cycle down the §5.2
already-accepted-duplicate path, and the sender's gate stayed held.
This is a route-contract mismatch, not a deployment fault. Probing the route unauthenticated
returns 401 (the endpoint exists; a stale deploy would 404), and `/ack` shares one `device_auth`
layer with `/retrieve`, which succeeds on the same token in the same sync cycle. Both halves were
current and simply disagreed on the wire shape, so no redeploy could have fixed it.
Fix, node-side, because the client already sends exactly what an acknowledgement needs — and
scoped to this route rather than loosening protobuf validation globally:
BatchEnvelope { repeated Envelope envelopes = 1 }
└─ Envelope { bytes message_id = 3 } // exactly once, exactly 16 bytes
Unexpected fields are REJECTED rather than decoded-and-ignored (`batch_signature`,
`atomic_execution`, any unknown tag at either level), along with wrong wire types, non-canonical
field ordering, duplicate `message_id`, and batches over `MAX_ACK_BATCH` (mirrors
`MAX_BATCH_RETRIEVE`). That keeps the ack representation deterministic and stops the route becoming
a generic envelope parser carrying unused attacker-controlled material. Body size stays capped by
`RequestBodyLimitLayer(MAX_ENVELOPE_BYTES)` and ack scoping by the canonical `x-dsm-b0x-address`
check. Validation completes before `spool_ack`, so a mixed batch fails whole and never partially
acks preceding entries. A zero-entry batch remains a 204 no-op: the client short-circuits empty
acknowledgements and the existing test documents that behaviour.
`validate_batch_envelope_bytes` had no other caller and is deleted rather than left beside the new
contract.
Client: `B0xSDK::build_ack_batch_body` extracted from `acknowledge_b0x_v2` (behaviour unchanged) so
the node test can validate the CLIENT's own encoder output — a handcrafted fixture would only prove
the node agrees with the test author.
18 ungated wire-contract tests (the node's DB-backed handler tests return early without
`DSM_RUN_DB_TESTS=1`, so contract protection must not depend on a database). Mutation-verified:
restoring the canonical-v3 validation in the ack path turns
`ack_accepts_message_id_only_entries` and `client_ack_body_satisfies_the_node_ack_contract` red
with `left: Err(400)` — the same 400 observed on the rig — while the reject-tests stay green.
Scope note: this fixes acknowledgement cleanup/dedup behaviour, NOT value-transfer correctness.
The online transfer path already settles correctly on hardware (8XK 600->575, 9FF 600->625).
Gates: cargo fmt --all -- --check; cargo clippy --all-targets -- -D warnings;
cargo test -p dsm_storage_node 236/0; cargo test -p dsm_sdk --lib 1668/0;
ci/production_safety_checks.sh (production clippy + TLA+).
…ly sends `v2_b0x_routing_and_ack_scope_end_to_end` acked with a full `make_env(..)` envelope, which matched the route's OLD full-canonical-v3 validation — the very validation that rejected every real acknowledgement. Under the new minimal ack contract that body is refused, so the test would have failed the moment anyone enabled the DB harness. It now acks via `B0xSDK::build_ack_batch_body`, i.e. message_id only, exactly what `acknowledge_b0x_v2` sends. The test encoded the broken contract; updating it is part of the fix, not an accommodation to it. Harness note (unchanged behaviour, recorded for whoever runs the DB gate): `maybe_state_and_auth` returns None on any pool/init failure, so DB-backed tests pass VACUOUSLY (0.01s) rather than fail. Against the pre-existing `dsm_storage` database `init_db` errors (mixed table ownership), so those tests have been silently inert. Against a clean database the harness engages, but the auth fixture then fails submit with 401 — verified as PRE-EXISTING: unmodified `main` fails identically at the same line with the same 401/204, so it is not introduced by the ack contract change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug — a route-contract mismatch, not a deployment fault
ack_b0x_batchvalidated every batch entry withvalidate_canonical_envelope_v3_bytes, which requires:An acknowledgement semantically consumes only the 16-byte transport
message_id— the client is retiring ids it already pulled and has no version, headers, or payload to send. So every well-formed ack was rejected400by every node, surfacing to the client as:Inbox rows were never retired, so the receiver re-pulled the same item every cycle down the
§5.2 already-accepted duplicatepath and the sender's gate stayed held.It was not a stale deployment and not auth, and both were ruled out with evidence before touching code:
/api/v2/b0x/ackunauthenticated returns 401, not 404 — the endpoint exists/ackand/retrieveshare onedevice_authmiddleware layer, and/retrievesucceeds on the same token in the same sync cycleBoth halves were current and simply disagreed on the wire shape. No redeploy could have fixed this.
The fix — a strict minimal contract for
/ack, not looser protobuf validation globallyThe fix is node-side because the client already sends exactly what an acknowledgement needs.
Unexpected fields are rejected, not decoded-and-ignored —
batch_signature,atomic_execution, and any unknown tag at either level — along with wrong wire types, non-canonical field ordering, duplicatemessage_id, and batches overMAX_ACK_BATCH(mirrorsMAX_BATCH_RETRIEVE— you can only ack what a retrieve handed out). That keeps the ack representation deterministic and stops the route becoming a generic envelope parser carrying unused attacker-controlled material into the node.Everything else is preserved:
RequestBodyLimitLayer(MAX_ENVELOPE_BYTES)x-dsm-b0x-addresscheckspool_ack, so a mixed batch fails whole and never partially acks preceding entriesvalidate_batch_envelope_byteshad no other caller and is deleted, not left beside the new contract.Empty batch stays
204(no-op) — the client short-circuits empty acknowledgements and the existingv2_b0x_ack_and_retrieve_basictest documents that behaviour. Changing it would be unrelated churn.Tests
18 wire-contract tests, all ungated. The node's DB-backed handler tests return early without
DSM_RUN_DB_TESTS=1(vacuous in CI), so contract protection must not depend on a database.ack_accepts_message_id_only_entriesclient_ack_body_satisfies_the_node_ack_contractack_allows_empty_batch_as_noop204no-op preservedack_rejects_full_canonical_v3_envelopeack_rejects_wrong_length_or_missing_message_idack_rejects_duplicate_message_id_in_one_entryack_rejects_extra_batch_level_fieldsbatch_signature,atomic_executionack_rejects_wrong_wire_type_and_noncanonical_orderack_rejects_oversized_batchMAX_ACK_BATCH, rejects +1ack_rejects_mixed_batch_without_partially_ackingack_rejects_truncated_and_malformed_protobufThe client/node agreement test is the one that matters: it extracts
B0xSDK::build_ack_batch_body(used byacknowledge_b0x_v2, behaviour unchanged) and feeds the client's own encoder output to the node's validator. A handcrafted fixture would only prove the node agrees with the test author.Mutation-verified. Restoring
validate_canonical_envelope_v3_bytesin the ack path turns exactly the right tests red:Err(400)— the same 400 observed on the rig. The reject-tests stayed green, which is how you can tell they are not the ones carrying the proof.Gates (local)
cargo fmt --all -- --check✅ ·cargo clippy --all-targets -- -D warnings✅ ·cargo test -p dsm_storage_node236 / 0 ✅ ·cargo test -p dsm_sdk --lib1668 / 0 ✅ ·ci/production_safety_checks.sh(production clippy + TLA+) ✅Scope
This fixes acknowledgement cleanup/dedup behaviour, not value-transfer correctness. The online transfer path already settles correctly on hardware — a live 25 ERA online transfer moved 8XK 600 → 575 and 9FF 600 → 625 while the ack was still failing. That result stands independently of this PR.
Also in this PR: the e2e test encoded the broken contract
v2_b0x_routing_and_ack_scope_end_to_endacked with a fullmake_env(..)envelope — matching the route's old validation, i.e. the very thing that rejected every real acknowledgement. Under the new contract that body is refused, so the test would have failed the instant anyone enabled the DB harness. It now acks viaB0xSDK::build_ack_batch_body(message_id only), the shape the client actually sends. Updating it is part of the fix, not an accommodation to it.Before merge — DB gate still OUTSTANDING, and the harness needs repair first
The three production-state properties are not pinned:
204and the row becomes ackedI attempted them and could not land them honestly, so I did not ship tests I never saw pass. What the attempt established about the harness:
maybe_state_and_authreturnsNoneon any pool/init failure, so the test returns early and reports green. Against the existingdsm_storagedatabaseinit_dberrors (mixed table ownership:dsmvscryptskii), so these tests have been silently inert —0.01s, "ok".createdb dsm_ack_test→ it gets pastinit_dbinto the test body).401— an auth-fixture gap, and pre-existing: unmodifiedmainfails identically at the same line with the same401/204. Not introduced by this change.So the gate needs the harness fixed first (clean DB + a working auth fixture). The contract defect itself is already behaviourally proven by the 18 ungated tests and the mutation, which is why this is a merge gate rather than a blocker to review.
After merge
The storage fleet must be redeployed for this to reach devices; then re-run the phone flow and require ack quorum success with no repeating
§5.2 already-accepted duplicatecycle.