test: mutation testing for Nostr event handling - #849

No actionable comments were generated in the recent review. 🎉

Walkthrough

The PR updates mutation-test execution, configures LNURL test ports, strengthens event validation and Cashu routing, adds expiration-tag regression tests, and centralizes the restore-session timeout value.

Changes

Nostr testing improvements

Restore-session timeout consistency

Estimated code review effort: 3 (Moderate) | ~25 minutes

Merge Risk: 🟡 Moderate · up to 722c2

This change expands Cashu order and take handling, but a failure after an order is claimed can leave it stuck in WaitingPayment without the state needed to continue normally. Merge should wait for that partial-failure path to be made atomic or explicitly accepted, with follow-up also needed for the bounded mutation-test port and path-handling issues.

Suggested reviewers: arkanoider, grunch

Poem

A rabbit checks the ports at night
Tests hop through events just right
Signatures guard the messaging lane
Tags stay single, clean, and plain
One timeout keeps its measure bright

• Fix all pre-merge checks with AI

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

Both review comments addressed — one of them turned out to be the opposite of what it looked like

Pushed 3e05ed2 and e885508, kept separate since they are unrelated findings.

src/nip33.rs — the Custom("expiration") arm can't be tested, because it can't be reached

The suggestion was to add a companion test for the TagKind::Custom("expiration") arm. I wrote that test first and it failed on its own first assertion, which sent me to check the premise:

let t = Tag::custom(TagKind::Custom(Cow::Borrowed("expiration")), vec!["123456".to_string()]);
// kind = Expiration | is_expiration = true

nostr normalises the tag name at construction, so t.kind() can never return Custom("expiration"). The second arm of has_expiration_tag was unreachable — and that also makes it an equivalent mutant: deleting it changes nothing observable, so no test could ever have killed it. Adding a test there would have been writing an assertion that passes for the wrong reason.

Worth correcting one thing I had assumed too: this arm is not the production-load-bearing one. order_to_tags does build the tag as Tag::custom(TagKind::Custom("expiration"), ..), but by the time it reaches create_event it has already normalised to TagKind::Expiration — so production has always taken the first arm.

So I deleted the dead arm instead, and added a test that pins the real path end to end: the exact Tag::custom shape order_to_tags emits must normalise and suppress the auto-add, asserting the caller's 123456 is the only expiration tag on the event. Verified it actually bites by forcing has_expiration_tag = false — it goes red with left: ["1788381606", "123456"], the config auto-add stacked on top of the caller's tag.

Flagging this one as your call, since it is a production change inside a test: PR. I think it belongs here — an equivalent mutant is precisely the kind of thing a mutation-testing pass should surface, and leaving it in means leaving a mutant nobody can ever kill. But the smaller option is equally defensible: restore the arm with a comment saying it is unreachable, and keep this PR's diff purely additive. Say the word and I'll do that instead.

Either way the deletion is safe across every caller: create_event has five entry points, all passing tags built internally, and the only two that carry an expiration are order_to_tags (Tag::custom, normalises) and price/manager.rs:528 (Tag::expiration, already the first arm).

Makefile — two of the three points hold

set -o pipefail: no change. Makefile:1 is SHELL := $(shell which bash), which applies to every recipe, and six existing targets already rely on it. Verified the recipe runs under bash.

Caller's port was being overwritten: fixed. Now MOSTRO_TEST_LN_PORT=$${MOSTRO_TEST_LN_PORT:-18080}, so a busy 18080 can be worked around without editing the Makefile.

Parallel workers sharing the port: fixed, and this was the one that mattered. Each worker runs the full suite in its own temp dir but shares the host's TCP ports, so two workers collide on the LNURL listener. Under mutation testing that collision is not just noise — a test failing for its own reasons counts as a killed mutant, so it silently inflates the very score this target exists to measure. Dropped to CARGO_MUTANTS_JOBS=1. Slower, but the number means something.

That failure mode is easy to see locally: a plain cargo test on this machine gives 1058 passed / 1 failed with AddrInUse on 8080, and MOSTRO_TEST_LN_PORT=18080 cargo test gives 1059 passed / 0 failed.

On the mutation score

Since this touches create_event, I re-ran the measurement on the new head rather than carry the old number over:

$ make mutation-test ARGS="--file src/app.rs --file src/nip33.rs -F 'in accept_event|in create_event'"
Found 9 mutants to test
ok       Unmutated baseline in 140s build + 41s test
9 mutants tested in 12m: 9 caught

0 missed, 0 timeout, 0 unviable.

It reads 9/9 now, not 10/10, and the PR body should say so. Removing the unreachable arm removes its || — and with it the replace || with && mutant — from the set. Nothing became uncovered.

While tracing that: 2 of the original 10 were delete field .. in check_trade_index, which land in the run because cargo-mutants 27.1.0's -F doesn't filter "delete field from struct expression" mutants. They are caught, so they never distorted the pass/fail, but the headline was really 8 targeted + 2 incidental. It is 7 + 2 now.

PR body updated to match — score line, the mutation-test invocation it quoted, and the cargo test count.

Verification

• cargo test: 1059 passed, 2 ignored (with MOSTRO_TEST_LN_PORT set past the host's busy 8080)
• cargo fmt --check: clean
• cargo clippy --all-targets --all-features -- -D warnings: clean

Rebased onto main — and the nostr 0.45 bump turned this PR's own guard test into the thing that verified it

This had gone CONFLICTING. It's now rebased onto current main (541129f), no merge commit, same six commits.

The conflict itself was three small hunks, but resolving them surfaced the real issue: this branch was on nostr-sdk 0.44.1 and main is now on nostr + nostr-sdk 0.45.1. That's a breaking change, so the rebase is also a small 0.44 → 0.45 migration.

What the migration touched

TagKind is gone from the 0.45 API. The branch had matches!(t.kind(), TagKind::Expiration); main has t.kind() == "expiration". I kept main's form — not as a tiebreak, but because TagKind genuinely doesn't exist in 0.45 (it's only in 0.44.x). Same for the test helpers: Tag::custom(TagKind::Custom(Cow::Borrowed("expiration")), ..) is now just Tag::custom("expiration", ..), matching how admin_cancel.rs and admin_settle.rs already write custom tags on main.

nostr_sdk::secp256k1 no longer resolves, and sign_schnorr now takes AsRef<[u8]> rather than a secp256k1::Message. So app.rs goes from sign_schnorr(&nostr_sdk::secp256k1::Message::from_digest([7u8; 32])) to sign_schnorr([7u8; 32]).

extract_lnurl returns Url, not String (from the LNURL scheme validation on main). The port-override test now asserts extracted.to_string() against the formatted URL, keeping both main's type and this branch's MOSTRO_TEST_LN_PORT override.

The part worth flagging

Commit 45ca902 adds a_custom_named_expiration_tag_normalises_and_suppresses_the_auto_add, and the comment I wrote on it 11 days ago says:

If an sdk upgrade ever stopped normalising, the auto-add would start firing on top of the caller's tag and this test goes red.

The sdk upgrade arrived. The test is green against 0.45 — nostr still normalises a custom-named expiration tag to the canonical NIP-40 kind, so has_expiration_tag's single check is still correct and order events are not double-stamping expirations. That's the one behavioural question the 0.45 bump raised in this file, and it's now answered rather than assumed.

I rewrote that comment to describe the behaviour instead of the (now non-existent) TagKind variants, so it doesn't rot again.

Verification on the rebased tree

• cargo fmt --check — clean
• cargo clippy --all-targets -- -D warnings — clean
• cargo test --bin mostrod — 1189 passed, 0 failed, 2 ignored

One note on that test run: on a first pass, lightning::invoice::tests::test_lnurl_validation_with_test_server failed with AddrInUse on port 8080 — which is precisely what commit 88679a0 in this PR exists to fix. Re-running with MOSTRO_TEST_LN_PORT=18080 gives a clean 1189/0. The failure was a live demonstration of the problem the commit addresses, not a regression.

Diff against main is unchanged in shape: 339 insertions, 13 deletions, same seven files.

Thanks @Catrya — rebased, #826 folded in as you ruled, force-pushed. 5 commits
off main @ d2e114d; #826 will be closed pointing here. History rewritten
rather than stacked on: the old messages described changes no longer on this
base (point 2).

The build failure needs cargo build --all-targets to show — all five broken
call sites are in test code.

1. spam_gate.rs is out of the PR, byte-identical to main. You had the
cause right: the old param was is_v2: bool reading the process-wide
OnceLock. On main the gate is injected, so each test builds its own and
passes Some(&gate).

2. The comment was wrong. Tag::kind() is &self.buf[0]
(event/tag/mod.rs:142) and Tag::custom("expiration", ..) pushes that
literal into buf[0] (:337). Both constructors match by construction — no
normalisation exists, so the canary could never fire. Rewritten; test renamed
off ..._normalises_....

3. Score re-measured on accept_event, create_event and
restore_session: 17 mutants, 14 caught, 1 missed, 2 unviable. The
survivor is the whole-function restore_session_action -> Ok(()) replacement
— the one already documented rather than papered over.

CARGO_MUTANTS_JOBS — you were right twice. 27.1.0 is installed here and
keeps exactly one scratch dir on a 16-CPU host with no -j: one mutant at a
time by default. #826's 2 doubles, and #849's 1 is a no-op. Kept as an
explicit pin of the existing default, described as that rather than as a cap.
MOSTRO_TEST_LN_PORT is documented as not making the suite hermetic — two
workers still collide on 18080. Its three copy-pasted parses are now one
lnurl::test_ln_port().

Kept from #826: the bash array for the PR job's --file flags. Dropped:
JOBS=2, the .gitignore entry, and the tautological assert_eq!.

That last one had a tail. Dropping the test left 60 * 60 generating two
mutants (*→+, *→/) that nothing kills — and the only test that kills
them is the assertion you just argued away. The constant is now 3_600: the
product computes nothing and existed only to hand an operator to a mutation.
Two fewer mutants, no new test. The log reports minutes, not seconds.

CodeRabbit's round on the pushed branch, since it changed the diff: it
caught that the two first-contact tests paired a gift wrap with Some(gate) —
a combination production never produces, since the gate only reaches
accept_event on the v2 loop with DM_EVENT_KIND. Worse, I had rebuilt
fixtures accept_event_ordering_tests already has. Both tests moved there and
now use its v2_event/accept helpers on a real kind-14 event. Also took its
MOSTRO_TEST_LN_PORT=0 guard: 0 parses as a valid u16 but means "let the
OS choose" to a listener, while the URL would still say :0.

I did not take its third one — NUL-delimited git diff -z for the changed-file
list. Bash cannot hold NUL bytes in a variable, so the suggested
changed_rs=$(git diff -z ...) cannot work as written, and core.quotePath
defaults to on, so a newline in a path is emitted quoted on one line rather
than split across two. The array already removes the argv-injection vector,
which was the real problem.

Not a change here: ci.yml is on: push only, no pull_request, while
cashu.yml, mutation.yml and markdown.yml all have one — so no PR gets
fmt/clippy/build/test from upstream CI. This PR's checks are three skipped
jobs. That is the mechanism behind "no build ever ran here to tell either of
us". Happy to file it separately if useful.

Two nits from your review I hadn't answered. Both now closed out.

The 18080 / 8080 divergence is deliberate, and the Makefile now says so.
Plain cargo test keeps binding the code's own 8080 default, so the default
path stays exercised; only make mutation-test — which runs the suite hundreds
of times over — steps aside from a port a developer machine is likely to have
in use. Pushed as a comment on the target rather than left as something only
this thread explains.

The timeout-minutes one turned out to point at something bigger, so I filed
it separately: #930. The condition you raised it under doesn't apply —
cargo-mutants already tests one mutant at a time, so nothing here made the
weekly run slower. But checking the observation on its own terms: the scheduled
mutation-baseline job has been killed at GitHub's 6-hour limit on every run
since at least July 26 — six for six, all at 6.01 h. cargo mutants --list
gives 2226 mutants and observed throughput is ~1 min each single-worker, so a
full pass needs 35+ hours and gets through roughly 15% before it is cut.
continue-on-error: true is why nobody saw it: it correctly stops a low score
from gating merges, but it also swallows the job never finishing, which is a
different thing.

I deliberately did not fix that here. Picking the timeout is the decision the
issue asks for — sharding, scoping to a rotating subset, or accepting a partial
run and saying so — and a testing PR is not the place to change how the
project spends runner minutes.

Which is worth saying plainly: I nearly let that nit go, because I checked
whether this PR made the run slower, found it didn't, and stopped there. The
observation underneath it was a month-old defect. Same shape as the mistake on
#860 — verify the line you're pointed at, then keep reading.

All five addressed. Rebased onto afc39a2 (v0.18.7); still 5 commits, the fixes folded into the ones they belong to rather than stacked on top.

1. .mutants.toml

Split as you suggested, but the part that belongs here has a smaller fix than moving the file.

Here: RUST_TEST_THREADS=1. The problem was never "the config file is in the wrong place", it was "test threads run in parallel". libtest honours that env var with no config file at all — verified, the suite goes 18.5s → 53.8s, so it is genuinely taking effect. One variable on a line that already set two: no .cargo/, no key renames, no doc rewrites, and nothing in this PR depending on #958 landing.

The Makefile comment now says both levels explicitly and points at #958 for why the config's own attempt never worked.

There: #958. Filed with the measurements rather than the report. Your path and invalid-key analysis is credited there; what I added is that moving the file is not merely insufficient, it breaks — Config is deny_unknown_fields, so cargo mutants errors out with unknown field 'jobs' instead of falling back to defaults. Tested key by key; exactly four are invalid and your four replacements are all correct. Also: the docs describe the file as working in four places, and MUTATION_TESTING.md:255 tells the reader to raise a timeout in a file that is never read.

And on #930: commented with the corrected numbers. cargo mutants --list gives 2373 today against 1943 with the config fixed — so the 2226 that issue reasons from is measured against a scope nobody chose. You are right that it does not rescue the run: 1943 × ~1 min is still ~32h against a 6h limit, so sharding remains the answer. But it does weaken option 3 there — "accept a partial run and document the score" assumed the partial score is honest, and without --test-threads=1 every score this project has recorded is inflated by port collisions counted as kills.

2. The body

Rewritten around a re-measured score. You were right that this is the deliverable and not a footnote:

81%, with all three survivors named in the body and the commands to reproduce them. Two are the 60 * 60 arithmetic (see 4). The third is the whole-function restore_session_action -> Ok(()) you can see in the earlier comment, and chasing it turned up why it cannot be killed: its only fallible call, start_restore_session, always returns Ok(()) — the DB error is handled inside the spawned blocking closure and logged at db.rs:1882-1884, never propagated. So the ? cannot fire, and the mutant's only effect is skipping the tokio::spawn. The survivor is telling us that error path is already dead, not that the tests are thin.

restore_session.rs is in the body now, the spam_gate.rs and TagKind::Custom paragraphs are gone, and the test count is current.

3. accept_event_tests

Folded into accept_event_ordering_tests on create_migrated_ctx(); the duplicate fixture and its four imports are gone, −105 lines.

I did not switch the "accepts" test to NewOrder. With a migrated pool the module no longer depends on the action choice to stay green, which was the actual defect; NewOrder would need a hand-built Payload::Order, and check_trade_index_tests covers that path directly with eleven tests. Happy to do it if you'd rather it went through accept_event too.

4. restore_session.rs

60 * 60 is back, matching ORDER_TS_MAX_AGE_SECS in src/util.rs, and the paragraph about mutation operators moved to the commit message. You were right about the shape of that mistake and it is the one I'd least like to have made in this PR: writing 3_600 improves the score by deleting a mutant rather than killing one, which is the same inflation the rest of the branch exists to remove. Both arithmetic mutants now survive and are reported.

On the log: it prints the Duration handed to tokio::time::timeout rather than a derived string, so it cannot disagree with the timeout for any value of the constant. That also drops the "1 minutes" / "0 minutes" behaviour and the gratuitous "1 hour" → "60 minutes" change.

5. Smaller

• set -o pipefail removed — you were right, no pipeline.
• The CI job still sets its own env, since it must build --file args as a bash array from PR-diff filenames and cannot go through make ARGS=. Added an explicit keep-in-sync comment rather than machinery; if you'd prefer the values in one place I'll do it, but a shared env file felt like more than the drift is worth.
• Order decided, not discovered: I've asked on fix(privacy): drop the four log lines that pair two Nostr keys (#836) #842 for that one to land first and this to take the rebase — it is a single hunk and fix(privacy): drop the four log lines that pair two Nostr keys (#836) #842 is down to review. The resolution has changed shape there (its warn! no longer carries event.id), so the current version is written out in that thread.

One thing worth flagging rather than burying: serialising the test threads makes the mutation run roughly three times slower. That is the correct trade and I'd rather say it out loud than have it discovered — it was faster before because it was measuring wrong.

Green: cargo fmt --check, cargo clippy --all-targets --all-features, cargo test --bin mostrod — 1336 passed, 0 failed, 2 ignored.