Repository navigation
Conversation
Add TestNewToleratesUnreachableStoredMint: seed a wallet DB with a keyset, counter and proof for a mint that accepts connections and closes them (EOF), plus a healthy mint; assert tollwallet.New() succeeds, the reachable mint is registered, and the dead mint's record is retained by the underlying wallet. Bump gonuts-tollgate to the isNetworkError(EOF) fix from OpenTollGate/gonuts-tollgate#45 (v0.14.2-0.20261030003712-ae77396240a2). On v0.14.1 the test fails with the exact production error: 'failed to create wallet: error getting keyset from mint: Get ...: EOF'; with the fix it passes. Note: TestV2Keyset_BothIdSpellingsReachSwapWithFullId and TestV2Keyset_V4TokenShortIdResolves fail on unmodified v0.14.1 as well; they are pre-existing and unrelated to this change.
E750 A/B verification (physical GL-E750)Cross-references: gonuts-tollgate#45 · tollgate-module-basic-go#1142 · physical-router-test-automation#330 The unit/integration tests were reproduced on the real router with the actual SetupA — old binary
|
Local QEMU lab A/B — the regression test catches the bugRan Cross-references: OpenTollGate/gonuts-tollgate#45 · #1142 A — stock
|
Follow-up: reproducible device build (same results)The E750 runs TollGate v0.6.0, which pins gonuts v0.13.0, while this PR targets Rebuilt and reinstalled on the GL-E750 (no The QEMU-lab RED/GREEN result is unchanged (the lab binary is the same source built for amd64). Cross-refs: gonuts#45, module#1142, PRTA#330, e750-repair#45. |
Local CI gate — PR #1142: test(tollwallet): tolerate an unreachable stored mint at startupRun of
Verdict: 5 leg(s) RED — 1 green. What this gate does not run (no vacuous green)
failing-leg log tailscontract-fences — tailpackaging-sequential — tailshell-suites — tailpackaging-concurrent — tailgo-battery — tail |
felixfelix-bot
left a comment
felixfelix-bot
left a comment
There was a problem hiding this comment.
Thanks — the diagnosis and the fork fix are right, and the RED/GREEN differential is real evidence rather than a claim. Two things need to land before this can merge; one of them currently breaks main.
1. BLOCK — the gonuts pin moved in one carrier, not all of them
src/tollwallet/go.mod goes to v0.14.2-0.20261030003712-ae77396240a2, but packaging/build-inputs.json (.gonuts.version) and the other four carriers stay at v0.14.1. That is the #791 drift shape, and it is not cosmetic — it fails two contract jobs and the root module build:
$ python3 tests/contract/check-gonuts-pin.py
manifest pin (packaging/build-inputs.json .gonuts.version): v0.14.1
carriers discovered by glob: 5 module(s)
❌ GONUTS PIN DRIFT — 1 finding(s):
🔴 src/tollwallet/go.mod: pins github.com/OpenTollGate/gonuts-tollgate v0.14.2-0.20261030003712-ae77396240a2, manifest says v0.14.1
$ python3 tests/contract/check-deps-sync.py
🔴 github.com/OpenTollGate/gonuts-tollgate — 2 versions:
cli: v0.14.1
merchant: v0.14.1
root: v0.14.1
tollwallet: v0.14.2-0.20261030003712-ae77396240a2
$ cd src && go build ./...
go: updates to go.mod needed; to update it:
go mod tidy
Both fences run in the deps-and-imports job of .github/workflows/test.yml, so CI will be red on the first run. The build failure is the sharp one: src/ is the root module and it still requires v0.14.1 while src/tollwallet requires the pseudo-version, so the module graph no longer resolves. The same command on 6aabea2d (this PR's base) exits 0.
I staged the mechanical fix on fix/stale-mint-resilience in my fork rather than describing it — manifest plus all five carriers (scripts/token-recovery, src/cli, src, src/merchant, src/tollwallet) to the same pseudo-version, then go mod tidy per module:
That also resolves two stale go.sum fragments in src/tollwallet/go.sum — the superseded v0.14.1 pair, and a v0.15.1-0.20261030003228-d6b9e39c686a pair that no go.mod in the tree names (looks like carry-over from a different branch; worth confirming it was intentional).
After it:
✅ one pin, everywhere: github.com/OpenTollGate/gonuts-tollgate v0.14.2-… across 5 carrier(s).
gonuts-pin_test.sh -> 7 cases, 0 failed
cd src && go build ./... -> 0 (was 1)
go vet ./... -> 0
check-deps-sync.py still reports 8 entries, but they are byte-identical to the base commit's set (Go directive on tollgate-ndsctl, x/mod, x/sync, x/tools, grpc, genproto×2) — pre-existing, and out of scope here.
Pseudo-version rather than the tag is right, and I checked it rather than assuming: gonuts-tollgate#45 is still open, and v0.15.0 does not carry the fix — isNetworkError at that tag still has no io.EOF arm. So bumping to the newest tag would silently drop the fix. One flag for whoever merges: #1151 (deps/gonuts-v0.15.0) moves the same files, so these two will conflict and need sequencing.
2. SHOULD-FIX — the test does not assert the retention it claims
The body says the dead mint's record is retained ("no delete, no overwrite"), and the test seeds a counter and a proof — but it never reads either back. The only assertions are that New() returned no error and the healthy mint was registered:
That gap is load-bearing, not theoretical. Deleting both the IncrementKeysetCounter(deadID, 2) and the SaveProofs(...) seed leaves the test green:
$ go test -run TestNewToleratesUnreachableStoredMint . # counter+proof seeding removed
ok …/src/tollwallet 16.136s
The claim itself does hold — I verified it independently by reading the DB back after New() (counter=2, keyset present, proofs=1 for deadID). But nothing in the suite enforces it, so a future regression that clobbers the counter or drops the proof would pass silently. Given AGENTS.md's monotonic-counter rule (#480), that is the exact class this test should be pinning. Suggested close: after tw.Shutdown(), reopen storage and assert GetKeysetCounter(deadID) == 2 and len(GetProofsByKeysetId(deadID)) == 1, in the shape the fork's own load_unreachable_mint_test.go already uses.
Test evidence
Same harness (.github/scripts/go-test-summary.sh), this PR's head vs its base:
- base
6aabea2d: 83 passed / 4 failed / 3 skipped - head
a672c706: 84 passed / 4 failed / 3 skipped - failure set byte-identical:
TestV2Keyset_BothIdSpellingsReachSwapWithFullId(+ its two subtests) andTestV2Keyset_V4TokenShortIdResolves— the two the body calls pre-existing, confirmed pre-existing here.
RED/GREEN differential on the new test, done by reverting only the pin:
- head:
--- PASS: TestNewToleratesUnreachableStoredMint (16.87s) go.mod/go.sumreverted tov0.14.1:--- FAIL: stale_mint_test.go:102: New must tolerate an unreachable stored mint, got: failed to create wallet: error getting keyset from mint: Get "http://127.0.0.1:40701/v1/keys/0092812ee35fa269": EOF
Also: no CI has ever run on this branch (no checks reported on 'fix/stale-mint-resilience'), so CI status is unknown rather than green.
Happy to open a small PR carrying the pin fix against your branch if that is easier than rebasing it yourself — say which you prefer.
|
Thanks — the diagnosis and the fork fix are right, and the RED/GREEN differential is real evidence rather than a claim. Two things need to land before this can merge; one of them currently breaks 1. BLOCK — the gonuts pin moved in one carrier, not all of them
Both fences run in the I staged the mechanical fix on That also resolves two stale After it:
Pseudo-version rather than the tag is right, and I checked it rather than assuming: 2. SHOULD-FIX — the test does not assert the retention it claimsThe body says the dead mint's record is retained ("no delete, no overwrite"), and the test seeds a counter and a proof — but it never reads either back. The only assertions are that That gap is load-bearing, not theoretical. Deleting both the The claim itself does hold — I verified it independently by reading the DB back after Test evidenceSame harness (
RED/GREEN differential on the new test, done by reverting only the pin:
Also: no CI has ever run on this branch ( Happy to open a small PR carrying the pin fix against your branch if that is easier than rebasing it yourself — say which you prefer. |
|
Following up on the earlier review — I independently reproduced findings 1 and 2 (pin half-update breaks One thing neither that review nor the PR body covers, and which changes the shape of the fix: the ordering the test does not exercise is the one that regresses.
Self-contained probe, identical fixtures, both orders, no external network ( So on this ordering the PR converts base's loud refusal into a quiet broken wallet: To be precise about causality, because my first pass got this wrong: the closed-DB defect is pre-existing in the fork, and Two closes, either acceptable:
Either way please add the ordering case: Two sequencing notes, both verified rather than assumed:
Evidence: |
felixfelix-bot
left a comment
felixfelix-bot
left a comment
There was a problem hiding this comment.
Review delta — independently reproduced, verdict unchanged: BLOCK.
I re-ran the full pass on a672c706 in a clean worktree (own worktree, -race, serialized Go lanes). Confirming the prior review rather than repeating it:
- Pin half-update — reproduced.
check-gonuts-pin.py→❌ GONUTS PIN DRIFT … manifest says v0.14.1, rc=1. Root-module build breaks:cd src && go build ./...→go: updates to go.mod needed, rc=1 (base6aabea2d: rc=0). Release lanego build -C src -o … -trimpath -buildvcs=false .(build-package.yml:355) rc=1, and themain package (testenv)lane (test.yml:77) rc=1 — 227/227 pass once the pin is aligned. The 10-file fix (manifest + all five carriers +go mod tidy) turns the fence green and every lane to rc=0. - Retention unasserted — reproduced, and the behaviour is correct. Test seeds counter=2 and a 1-sat proof and reads neither back. Reading the DB back after
New()/Shutdown()givescounter=2,proofs=1, keyset record present — so the claim holds, it is just not pinned. Given AGENTS.md's monotonic-counter rule (#480), that gap is the class this test should be closing. - RED/GREEN is real. Reverting only the pin (test untouched):
--- FAIL … Get "…/v1/keys/…": EOF; head--- PASS (16.64s).gofmt/go vetclean,-raceclean, no assertion weakened or skipped, full package count 83→84 passed with the same 4 pre-existing failures. - Pre-existing failures are the same 4 at base and head — and already owned by #1162.
New, and the reason this stays BLOCK: the ordering the test does not cover is the one that regresses. With the dead mint at acceptedMints[0] it becomes CurrentMintURL, gonuts' offline early-return leaves isErr true, and its deferred close shuts the DB of the wallet it just returned — so New() now succeeds with zero registered mints and an empty balance map (database not open on every later write), where base refused loudly with the exact production error. Detail, reproducer and the two suggested closes are in my comment above.
Net: the test is worth landing, the gonuts#45 diagnosis is right, and the branch needs the pin aligned plus a decision on the dead-mint-first ordering before it can merge.
|
Stacked the pin fix from my review as a PR against your branch, since the branch lives in the org repo and I can't push to it ( #1182 — deps(wallet): one gonuts pin, every carrier → #1182 Base is Feel free to cherry-pick, merge, or ignore — whichever suits your workflow. It deliberately does not touch the two open review blockers (unasserted retention, and the dead-mint-first ordering), since both need your call. |
BLOCKER found on review — the branch is red on
|
|
Status of my Re-checked your HEAD just now: still So the review state is intentional, not stale. But I do not want it to be a dead end, so: Two ways to clear it, both already builtOption A — merge my stacked fix, #1182 (recommended, zero work for you) #1182 — base is your branch Verified on that branch: Option B — do it yourself in two commands scripts/bump-gonuts.sh v0.14.2-0.20261030003712-ae77396240a2
python3 tests/contract/check-gonuts-pin.py # expect rc=0, 5 carriersNote that the manifest, not Why I am holding the review rather than approving on the test aloneYour test is genuinely load-bearing — I proved it. Reverting only the pin (leaving your test intact) turns So the work is real and the merged thing should keep it. But The diff is 2 lines of dependency metadata away from mergeable. Option A costs you one click. |
Why
A wallet DB can still hold a keyset (and a Cashu proof) for a mint that is currently down. In the field,
mint.minibits.cashreturnedEOFwhile five other accepted mints were healthy;wallet.LoadWallet()failed on that stale keyset and the merchant stayed in degraded mode, so the captive portal showedno-reachable-mintsand could not sell access.The underlying fix is in gonuts:
isNetworkError()did not recogniseio.EOF, soloadWalletMints()treated the unreachable mint as fatal instead of skipping the keyset and retaining its DB record/proof.v0.14.2-0.20261030003712-ae77396240a2Test
src/tollwallet/stale_mint_test.go—TestNewToleratesUnreachableStoredMintNew(dir, []string{healthy, dead}, false)must succeed and register the healthy mint;RED on
v0.14.1:GREEN with the fix:
Note
TestV2Keyset_BothIdSpellingsReachSwapWithFullIdandTestV2Keyset_V4TokenShortIdResolvesalso fail on unmodifiedv0.14.1in this workspace; they are pre-existing and unrelated to this change.Follow-up (separate)
GetBalanceByMints()reports 0 for a mint whose keysets were all skipped, even though the proofs remain in the DB. A follow-up could retain the offline mint inwalletMintsso the balance (and the portal) shows the stranded amount. The fund-safety guarantee — records and proofs are kept — is covered here.