Repository navigation
fix(zeronym): the shim discarded the hub's queue-hit sentinel - #80
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Address the digest reproducibility issue and both smoke-test retry issues.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes Zeronym queued-transaction handling, adds bounded smoke-test retries, and updates Zallet proposal funding error mapping.
Changes:
- Relays height-0 empty-body queue sentinels.
- Adds regression tests and arrival-order retry handling.
- Maps insufficient-funds proposal errors to RPC code
-6.
File summaries
| File | Summary |
|---|---|
zeronym/smoke.sh |
Adds bounded lookup retries; restrict retries to NOT_FOUND and respect the remaining timeout budget. |
zeronym/shim/tests/divert.rs |
Adds clearnet sentinel and transaction-validation tests. |
zeronym/shim/tests/divert_nym.rs |
Adds mixnet sentinel coverage. |
zeronym/shim/src/intercept.rs |
Relays queued sentinels before transaction validation; refresh the deployment digest. |
zallet/zallet-core/src/components/json_rpc/payments.rs |
Maps proposal funding failures to legacy RPC errors. |
zallet/CHANGELOG.md |
Documents the error-code change. |
Review details
Suppressed comments (1)
zeronym/smoke.sh:657
- The check compares the previous
_waitedvalue and then always sleeps a full two seconds, so a non-multiple budget is exceeded; for example,SMOKE_DIVERT_SETTLE_SECS=1still sends another lookup at about 2 seconds. Test the next retry interval against the remaining budget (or clamp the sleep) before sleeping.
[ "$_waited" -lt "$SMOKE_DIVERT_SETTLE_SECS" ] || break
sleep "$DIVERT_RETRY_STEP_SECS"
_waited=$((_waited + DIVERT_RETRY_STEP_SECS))
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
45e408f stopped the hub from serving a queued migration's bytes: a queue hit is now answered found, height 0, with an EMPTY body, so a not-yet-published transaction cannot be taken from an unauthenticated lookup and broadcast first. smoke.sh was updated to accept that reply. The shim was not, and it rejects it. `get_transaction` ran every `Lookup::Found` through the L4 guard, which deserializes the returned bytes and compares txids. An empty body does not deserialize, so the guard returned false and the shim answered NOT_FOUND with `not_found_message`, which synthesizes lightwalletd's "getrawtransaction ... failed: -5" text LOCALLY. That is why the failure reads as though the operator's indexer answered; the operator was never dialled. This is not a smoke-test problem. With this shim a wallet that diverts a migration can never see it as pending: the height-0 queue answer is the entire existence-and-status signal the stateless-shim design depends on, and the shim converted every one of them into NOT_FOUND. The sentinel now takes its own arm ahead of the guard. The guard's intent is right, a hub must not substitute a different transaction's bytes, but an empty body carries no bytes to substitute, so there is nothing for it to verify and nothing for it to protect. Height 0 only: a mined transaction always has bytes, so an empty body at a nonzero height is not a queue hit and still falls through to the guard, which refuses it. One arm covers both transports. The clearnet path was checked and needs no change: the hub's `found()` sets content-type and x-tx-height unconditionally, `HubClient::get_transaction` keys on that header shape rather than on body length, and `LookupReplyV1` carries tx_len = 0 cleanly in both directions. `intercept.rs` was the single point of failure. Confirmed against the real public Nym mixnet on 2026-09-15, with a 20s delay between the divert submit and the lookup so no race is possible: the hub logged `migration admitted to the batch` and, twenty seconds later, `transaction lookup answered source="queue"`, while the shim logged the L4 refusal. Not a flush race, and not a failure to queue. Tests, on both transports, because every existing mock hub returned the full V6_MIGRATION bytes for a height-0 hit and none modelled the post-45e408f0ff reply. The new pending test fails on the parent commit; the guard tests pass with and without the fix, so the fix is not what makes them green. divert.rs also gains the mismatched-txid refusal it was missing, which divert_nym.rs already had, so widening the guard cannot pass unnoticed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
aphelionz
force-pushed
the
fix/zeronym-shim-queue-hit-sentinel
branch
from
September 16, 2026 01:40
0dcfbfb to
e93da3f
Compare
`check_shim_divert` submits a migration and then looks it back up once. Over the mixnet those are two independent Sphinx packets with no ordering guarantee between them: `NymHandle::submit` is fire-and-forget, returning as soon as the frame is handed to the Nym client and answering the wallet with a locally-computed txid, so the lookup can reach the hub first. On 2026-09-15 the hub logged the admit and the lookup miss 44 ms apart, and the check failed for a migration that was queued correctly. Retry a NOT_FOUND every 2s up to SMOKE_DIVERT_SETTLE_SECS (10), rather than sleep before the lookup. A run whose packets arrive in order pays nothing; a hub that never queued the transaction still fails with the same message, now saying how long it waited; and SMOKE_LOOKUP_MAX_SECS keeps measuring one call, not the sum, so the latency assertion is unchanged. NOT_FOUND alone is retried. Arrival order is the only thing a retry can fix, and it is the only thing a 5 is ambiguous about; UNAVAILABLE means the hub is unreachable, which a deployer needs told at once, and INVALID_ARGUMENT can never become true by waiting. The queue explanation is printed under a 5 for the same reason: under a 14 it would send someone looking for a flush that never happened. Separate from the shim fix in the parent commit on purpose. That was the real bug and this would have hidden part of it; the retry is worth having on its own merits, against a transport that genuinely does not order its packets. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
aphelionz
force-pushed
the
fix/zeronym-shim-queue-hit-sentinel
branch
from
September 16, 2026 01:42
e93da3f to
9a424f7
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate findings remain in the smoke retry handling and deployment hash baseline.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
zeronym/shim/src/intercept.rs:391
- This source change changes the shim binary, but
zeronym/shim/deploy/EXPECTED_SHA256remains at the previous digest (the PR description explicitly notes it is stale).deploy/reproduce.shwill therefore fail its published-hash check and the attestation/provenance baseline does not describe this commit's binary; include the cold-build re-baseline, along with the corresponding deploy README record, before merging.
Ok(Lookup::Found { data, height }) if data.is_empty() && height == 0 => {
zeronym/smoke.sh:654
- This condition retries every nonzero gRPC status, not just
NOT_FOUND. For example, a fast hub-unreachable response isgrpc-status: 14, so the smoke check sends repeated mixnet lookups and waits through the full settle budget before reporting the same outage. Gate the retry on status 5 so other failures remain immediate.
# wallet needs told immediately, and INVALID_ARGUMENT (3) can never become
zeronym/smoke.sh:657
- The retry step can exceed the configured settle budget whenever
SMOKE_DIVERT_SETTLE_SECSis not a multiple of 2. For example, setting the documented override to 1 still sleeps for 2 seconds and performs a retry after the budget has expired. Sleep for the smaller of the fixed step and the remaining budget.
# YET", and only a 5 is retried.
_waited=0
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Rebaseline the deployed hash and provenance, and ensure retries remain within the configured settle budget.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
zeronym/shim/src/intercept.rs:393
- This source change alters the deployed shim binary, but
zeronym/shim/deploy/EXPECTED_SHA256still contains the pre-change hash (da0b12a6…).deploy/reproduce.shtherefore fails its published-hash check and the attestation/reproducibility documentation continues to identify the old binary; please include the cold-build rebaseline (and its accompanying provenance update) in this PR.
Ok(Lookup::Found { data, height }) if data.is_empty() && height == 0 => {
Ok(get_transaction_response(&data, height))
}
zeronym/smoke.sh:664
- This does not actually bound retries to
SMOKE_DIVERT_SETTLE_SECS:_waitedcounts only the sleeps, so each NOT_FOUND call may first consume the fullSMOKE_LOOKUP_HARD_SECS(90s by default). A slow NOT_FOUND can therefore trigger all retries and make the advertised 10s settle take several minutes; track a wall-clock deadline or cap each call to the remaining budget.
[ "$_waited" -lt "$SMOKE_DIVERT_SETTLE_SECS" ] || break
sleep "$DIVERT_RETRY_STEP_SECS"
_waited=$((_waited + DIVERT_RETRY_STEP_SECS))
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
The parent commits change `intercept.rs`, so the published binary hash moves with them. Measured the way deploy/README.md's procedure asks: the source landed first, then `EXPECTED= sh zeronym/shim/deploy/reproduce.sh` ran from that commit, so the value describes a tree that exists rather than a working tree that may never be committed. dae86efed5dbbce185539afa0bdb033ff8a61aec306e83d44c4c28ba00f132db, agreed by four cold builds across two machines and two architectures: two on a native x86_64 CI runner and two locally on an arm64 Mac under Rosetta, with `zebra/` and `zaino/` clean each time. The recipe did not move, and neither did `Cargo.toml` or `Cargo.lock`; the tests and `smoke.sh` in the parent commits are not compiled inputs. The string check has nothing to report this time, which is itself worth recording. The fix is control flow and introduces no string, so `strings` cannot distinguish this binary from da0b12a6...; the L4 warning is still present exactly once, as it must be, since a genuine txid mismatch is still refused. The behavioural distinguisher is the test suite and smoke-local.sh. EXPECTED_SHA256 and the Recorded hashes table move together, as the file's own rule requires, with da0b12a6... going down to the superseded row. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
zeronym/smoke.sh can sleep beyond SMOKE_DIVERT_SETTLE_SECS for non-multiple-of-two budgets.
Review details
Suppressed comments (1)
zeronym/smoke.sh:664
- This fixed two-second sleep can exceed the configured settle budget whenever
SMOKE_DIVERT_SETTLE_SECSis not a multiple of two (for example, a value of 1 sleeps for 2 seconds and performs another lookup). Since this variable is documented as how long retries may run, cap the final sleep to the remaining budget.
[ "$_waited" -lt "$SMOKE_DIVERT_SETTLE_SECS" ] || break
sleep "$DIVERT_RETRY_STEP_SECS"
_waited=$((_waited + DIVERT_RETRY_STEP_SECS))
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
aphelionz
force-pushed
the
fix/zeronym-shim-queue-hit-sentinel
branch
from
September 23, 2026 14:28
0c5b888 to
bc72d49
Compare
The retry loop slept a fixed 2s step, so a budget below or off a multiple of the step overran it (1 slept 2). The last step now sleeps only what is left, and the NOT_FOUND note reports the seconds actually waited. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The shim answers a txid-guard refusal with NOT_FOUND, identical on the wire to a queue miss by design, so the smoke note blamed the queue for both. The note now names the guard case and the log line that identifies it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
45e408f stopped the hub from serving a queued migration's bytes: a queue hit is answered found, height 0, with an empty body, so a not-yet-published transaction cannot be taken from an unauthenticated lookup and broadcast first.
smoke.shwas updated to accept that reply. The shim was not.get_transactionran everyLookup::Foundthrough the L4 guard, which deserializes the returned bytes and compares txids. An empty body does not deserialize, so the guard returned false and the shim answered NOT_FOUND withnot_found_message, which synthesizes lightwalletd'sgetrawtransaction ... failed: -5text locally. That is why the failure reads as though the operator's indexer answered; the operator was never dialled.This is not a smoke-test problem. With this shim a wallet that diverts a migration can never see it as pending. The height-0 queue answer is the entire existence-and-status signal the stateless-shim design depends on, and the shim converted every one of them into NOT_FOUND.
The fix
The sentinel takes its own arm ahead of the guard. The guard's intent is right (a hub must not substitute a different transaction's bytes) but an empty body carries no bytes to substitute, so there is nothing to verify and nothing to protect. Height 0 only: a mined transaction always has bytes, so an empty body at a nonzero height is not a queue hit and still falls through to the guard, which refuses it.
One arm covers both transports. The clearnet path was checked and needs no change: the hub's
found()sets content-type andx-tx-heightunconditionally,HubClient::get_transactionkeys on that header shape rather than on body length, andLookupReplyV1carriestx_len = 0cleanly in both directions.intercept.rswas the single point of failure.Evidence
Confirmed against the real public Nym mixnet on 2026-09-15, with a 20s delay between the divert submit and the lookup so no race is possible:
The only flush in that run was the shutdown flush at 13:45:27, well after the lookup. Not a flush race, and not a failure to queue.
Tests
Every existing mock hub returned the full
V6_MIGRATIONbytes for a height-0 hit; none modelled the post-45e408f0ff reply. Five new tests across both transports:divert.rsgains the mismatched-txid refusal it was missing, whichdivert_nym.rsalready had, so widening the guard cannot pass unnoticedThe new pending test fails on the parent commit. The guard tests pass with and without the fix, so the fix is not what makes them green.
Second commit: smoke.sh arrival ordering
Separate and independent. Over the mixnet the submit is fire-and-forget (
NymHandle::submitreturns as soon as the frame is handed to the Nym client, with a locally-computed txid), so the submit and the lookup are two Sphinx packets with no ordering guarantee. In a run without a settle delay the hub logged the admit and the lookup miss 44 ms apart.The divert lookup now retries a NOT_FOUND every 2s up to
SMOKE_DIVERT_SETTLE_SECS(10) rather than sleeping before it: an in-order run pays nothing, a hub that never queued the transaction still fails with the same message, andSMOKE_LOOKUP_MAX_SECSkeeps measuring one call rather than the sum.Verification
zero-indexer-shimtest suite greensh zeronym/smoke-local.shagainst the public Nym mixnet: 9/9,shim:divertreportingheight=0 from the hub's queue, body withheldEXPECTED_SHA256is stale as of the shim source change and will be re-baselined on this branch from this PR'szeronym-shim-reproducecold build.🤖 Generated with Claude Code