perf(contract): stop re-verifying the same Ed25519 signatures on every apply

[sanity]

Scope correction, 2026-07-29. An earlier revision of this description claimed the change is output-identical and that it fixes #422. Both claims were wrong and are corrected below: independent review found a real behavioural divergence (since fixed), and measuring the live room showed the headline table did not describe it. What follows is the corrected account. The commit is retitled Refs #422, not Closes.

What this actually is

A complexity cleanup to the room contract's signature verification, worth roughly 2x on update_state and 1.1x on validate_state at the live Freenet Official room's measured shape, and up to 45x on validate_state for invite trees far deeper than any room measured so far.

It does not demonstrate a fix for the 5-second-budget breach #422 reports. See "What remains unexplained".

Measurements

All under wasmtime with Cranelift at OptLevel::None, which is what freenet-core sets for contracts (crates/core/src/wasm_runtime/engine/wasmtime_engine.rs). One Ed25519 verification costs 0.4224 ms there. Native numbers are not usable for this — see the note at the bottom.

Verification counts are exact and machine-independent, so they are the primary evidence; times are count x 0.4224 ms.

The live Freenet Official room, measured 2026-07-29 with cli/examples/invite_depth_probe.rs (in this PR): 496 members, 200 bans, 2000 messages, 497 member_info, 1.44 MB, invite depth max 4 / mean 2.02 / median 2 (histogram: 1 member at depth 1, 484 at depth 2, 10 at depth 3, 1 at depth 4).

entry point	verifications before	after	time before	after
update_state (1-message delta)	800	400	338 ms	169 ms	2.0x
validate_state	3440	3192	1453 ms	1348 ms	1.08x

validate_state barely moves because that room's 2000 message signatures dominate, and the chain fix cannot touch them — each message signature was already verified exactly once.

Synthetic depth sweep (400 members, 100 messages, no bans), validate_state:

invite depth	before	after
10	2,700 (1.14 s)	900 (0.38 s)	3.0x
50	10,700 (4.52 s)	900 (0.38 s)	11.9x
200	40,700 (17.19 s)	900 (0.38 s)	45.2x

Only the depth-50 and depth-200 rows breach the 5 s budget, and no measured room has a tree remotely that deep. Treat them as the shape of the curve this change flattens, not as a description of any real room. An earlier revision of this PR led with them, which was misleading.

Harness committed as common/tests/room_scale_bench.rs (#[ignore]d; it is a measurement, not an assertion) so the numbers can be re-run and disputed.

What remains unexplained

The contract in #422 was frozen at 154,375 bytes with every merge exceeding 5 s. At that size — an order of magnitude smaller than the Official room's 1.44 MB — neither redundancy I found gets near 5 s by these measurements. So the cause of the reported timeouts is still open. Candidates checked and eliminated:

• The triage assessment's hypothesis (quadratic loops over members in member_info.rs): measured at 0.06–0.14 ms at 200 members. Not the driver. Left untouched deliberately.
• get_downstream_members being O(subtree x members) (raised in review as the one thing a verification-count metric is structurally blind to): added a fixture where bans target real members with subtrees rather than absent users. validate_state 1435 -> 537 ms, update_state 207 -> 74 ms — no blow-up. Self-limiting, because banning a member with a large subtree cascade-removes it and the walk shortens.
• A pathological invite tree on that specific contract: plausible and would be fully explained by this change, but unverifiable — the contract times out before it can emit identifying log lines, and its state cannot be fetched.

The honest position: this removes real, measurable redundancy and raises the ceiling substantially for deep trees, but #422 should stay open until something reproduces its actual shape.

The behavioural divergence found in review, and fixed

The earlier claim of output-identity rested on this, in InviteChainCache's doc:

"Member X's chain is valid" is a property of X and its ancestors ONLY — the starting member of a walk never enters the checks.

That is false. The start's MemberId seeds the cycle guard's visited set, and the memo lookup sat before that guard, so a memo hit could short-circuit a walk the original terminated with a circular-chain error. Same input, two verdicts:

before fix : MembersV1::verify -> Ok
origin/main : MembersV1::verify -> Err("Circular invite chain detected for member ...")

Trigger: the walk's start is a non-canonical duplicate (members_by_id[start.member.id()] resolves to a different AuthorizedMember) and some node on the walk is already memoized Ok. Reachable from both call sites — verify's map is last-wins, apply_delta's is or_insert over wire-supplied deltas. It needs only an ordinary member's own key: B, invited by A, mints a second entry for A's key claiming B invited it; with members = [B, A', A], walking B memoizes B -> Ok, and A''s walk then hits that memo instead of walking into A's already-visited id.

Fixed by bypassing the memo entirely for a non-canonical start — it neither reads nor writes a verdict, so it runs as the untouched original walk. Every node reached after the start comes out of members_by_id and is canonical by construction, so the start is the only place this arises. Note that moving the memo lookup below the cycle guard does not fix it: the hit lands on B before the walk ever reaches A.

With that, the change is output-identical again — and now it is tested rather than asserted.

Why four rounds of self-review missed it

Every equivalence test used InviteChainCache::new(...) as the "fresh walk" oracle — the same new implementation with an empty cache. A one-member cache reproduces the identical short-circuit, so the divergence was structurally invisible to them. MembersV1::get_invite_chain is still live and pub and is the real oracle: the untouched original. A differential against it catches this on the first run, and is now differential_against_the_original_walk_on_a_straight_chain / differential_when_a_walk_starts_at_a_non_canonical_duplicate / verify_agrees_with_the_original_walk_on_a_non_canonical_duplicate. This is the single most valuable test in the PR — validate_chain is a hand re-transcription of a security-critical loop, and nothing else compares it to what it replaced.

Approach

Memoize within a single operation; nothing is cached across calls, as the contract is stateless.

• BanSignatureCache — post_apply_cleanup builds one and threads it through all its ban passes. Keyed on (whole ban, resolved verifying key), the complete input to verify_signature, so a hit answers a byte-identical question. Safe across the member-set mutations the cleanup performs: a banner resolving to a different key is a different cache entry, and one that stops resolving returns false without consulting the cache.
• InviteChainCache — MembersV1::verify and MembersV1::apply_delta each build one, so each member's invite signature is verified once. Keyed on the whole AuthorizedMember, and bound to (parameters, members_by_id) at construction.
• [profile.release.package.curve25519-dalek] opt-level = 3.

Two hazards closed by construction rather than convention:

• Memo keys are the objects, never their ids. BanId is fast_hash(signature), MemberId is fast_hash(verifying key) — 64-bit hashes of attacker-chosen bytes. Keying on them would let an attacker who grinds a collision (a birthday search over candidates they generate themselves, ~2^33 work, not 2^64) have a forgery inherit a genuine object's "verified" verdict, reopening the forged-ban enforcement hole feat: member deputies for ban authority (invite-subtree moderation) #411 round 4 A closed and letting a forged member entry claim an unearned position in the invite tree. Pinned by two tests that construct the collisions exactly — two bans sharing a signature necessarily share a BanId; two entries sharing a verifying key necessarily share a MemberId — so no grinding is needed.
• The chain cache is bound to its context. A verdict is meaningful only under the owner it was earned against and the lookup its ancestors resolved through. Both are held for the cache's life, so one instance cannot span two rooms, nor span verify and apply_delta (whose map deliberately includes not-yet-verified delta members and can therefore grant a weaker Ok). Chosen over a stored owner_id + debug_assert because the contract ships as a release build, where debug assertions are compiled out — that would have documented the hazard while defending nothing. Verified rather than assumed: a cache-sharing test fails to compile with error[E0716].

AuthorizedUserBan's Hash now covers every field rather than the signature alone, since caching made bucketing load-bearing and a signature is attacker-supplied and replayable across bodies. Safe to change: no map keyed on the type is ever iterated into state, so bucket order cannot reach the wire or affect convergence.

Testing

common/tests/signature_verification_cost_test.rs, 22 tests. They count verifications rather than measuring wall-clock time — exact, machine-independent, and not flaky.

• Differential against MembersV1::get_invite_chain, the implementation this replaced, over every fixture including the non-canonical-duplicate shape and through MembersV1::verify itself.
• Cost pins: one verification per member, not 1+2+...+N; four passes over N bans cost N verifications.
• Equivalence: valid chains, forged links, missing inviters, cycles, self-invitation mid-chain; ban answers vs. the uncached function for owner / member / forged / absent-banner; re-verification when a banner resolves to a different key.
• Collision resistance: the two exact-collision tests above.
• AuthorizedMember's structural Eq, which the memo key depends on and which nothing previously pinned — weakening it to ignore the signature passed the entire suite before this test existed.
• The Err-memo path, via a branching tree with a broken shared ancestor. Previously every fixture was a straight chain, so the fan-out case that makes memoization worthwhile was only tested when all-valid.
• The shared ban cache across an actual member-set mutation: banner pruned mid-cleanup, asked again by the step-5 sweep. Previously the fixture removed nobody, so the cache never spanned a mutation — the whole reason sharing it needs an argument.
• Source pins that the caches are routed through and that the profile exception survives; both made argument-name-insensitive after review noted a literal pin fails open on a rename.

Mutation-tested, 10 independent reversions, each caught by exactly one test — or by the compiler: disable either memoization; revert a post_apply_cleanup call site; drop the Cargo.toml exception; revert either memo key to the id; revert the Hash impl; weaken AuthorizedMember's Eq; revert the non-canonical-start bypass; unbind the chain cache (fails to build). No test is vacuous.

Full river-core suite: 428 passed, 0 failed, including the retention-monoid proptests, convergence and deputy-ban suites, all unmodified. cargo fmt clean; clippy warning count identical to origin/main (11, all pre-existing).

Publishing implications — needs @sanity

Both WASM artifacts move, so a publish needs two migration entries. Measured against origin/main with cargo build --locked --profile release --target wasm32-unknown-unknown:

artifact	before	after	delta
room_contract.wasm	792,831	808,800	+15,969 (+2.0%)
chat_delegate.wasm	737,365	754,652	+17,287 (+2.3%)

The delegate moves because it also depends on ed25519-dalek and is built --profile release. The room contract key is BLAKE3(wasm, params); the delegate key is BLAKE3(BLAKE3(wasm) || params). So publishing requires a legacy_room_contracts.toml entry and a legacy_delegates.toml entry.

check-room-contract-migration and check-delegate-migration both pass on this PR, and that is not evidence they would catch it: they diff the committed WASM, which no source change regenerates. Stated here rather than discovered at publish time. (The baseline delegate build reproduces the committed chat_delegate.wasm hash exactly, 6f65e45c…, so that artifact is byte-reproducible from a standalone build — unlike the room contract, which needs cargo make sync-wasm.)

Also for Ian: the curve25519-dalek exception is inherited by the wasm-release profile, so the UI WASM gains the same faster verification and roughly the same +16 KB.

Filed separately rather than bundled

• Room contract: MembersV1::apply_delta does not bound delta.added, so a member can force unbounded signature verification #551 — MembersV1::apply_delta puts no bound on delta.added.len(), so a member can force unbounded signature verification. The ban path has had this guard since feat: member deputies for ban authority (invite-subtree moderation) #411 round 3 item C; the member path never got it. Kept out of this PR because a length bound rejects deltas that previously succeeded, which is a merge-semantics change with a rollout story, and bundling it would destroy the one property that makes this PR reviewable.
• UI: enforced_banned_member_ids does 200 Ed25519 verifications per call on the render path #554 — the UI calls the uncached banned_member_ids on the render path, paying 200 Ed25519 verifications per call on the Official room. Same redundancy, UI-side.

Note on native profiling

Do not characterise this contract from a native build. Natively the profile change measures as 46x (2.27 ms vs 0.05 ms per verification, while signing is unaffected at 0.03 ms — which is why it stayed invisible, since the contract only ever verifies). In WASM it is 1.6x, because Cranelift re-optimises the module on load and recovers most of the lost inlining. An earlier revision of this PR quoted 46x. Correcting it is what redirected the investigation from the constant factor to the algorithmic cause.

Refs #422

[AI-assisted - Claude]

[sanity]

Full-tier review (5 lenses, run serially, blind-ish: each lens committed to its findings before the next started)

Consensus/WASM surface, so Full tier per the review rule. No external models (opt-in only).

1. code-first — intent vs. implementation

Read the diff before the description. Implementation matches intent. Notes:

• verify_member_invite_with_lookup dropped &self and became an associated fn. Verified safe: the body never used self, and the walk it delegated to (get_invite_chain_with_lookup) resolves ancestors purely through the passed members_by_id.
• get_invite_chain_with_lookup is retained and still reachable from the public get_invite_chain, which returns the assembled chain. Not orphaned.
• sort_by_cached_key's closure mutably borrows the cache while self.bans.0 is mutably borrowed; compiles, and the key is still computed at most once per element.
• Verified, not assumed: the [profile.release.package.curve25519-dalek] override is inherited by wasm-release (checked with cargo build -v), so the UI WASM is affected too. Called out in the PR body rather than left implicit.

2. merge-semantics — the critical lens

Question asked: does this change the merge OUTPUT for any input, is the merge still a commutative/associative monoid, are all signatures still verified, is serialization byte-identical?

• Serialization: untouched. No signed struct, wire type, or field order changes.
• Commutativity/associativity: untouched. No ordering, sorting, retention or conflict-resolution rule changed. The retention-monoid proptests pass unmodified.
• Signature coverage: every signature that was verified before is still verified. The caches only skip recomputing an answer for an input already answered.

This lens found the one real defect, and it was fixed before the PR opened. Both memos were originally keyed on BanId / MemberId. Those are 64-bit fast_hash values over attacker-chosen bytes (a ban's signature; a member's verifying key). Keying on them means a colliding forgery inherits a genuine object's "signature verified" verdict without its own check ever running. The attacker does not need a 2^64 second preimage — a birthday search over candidates they generate themselves is ~2^33. Concretely that reopens the forged-ban enforcement path #411 round 4 A closed, and lets a forged member entry claim an unearned inviter. Fixed by keying on the whole object plus the resolved verifying key, and pinned by two tests that build the collisions exactly rather than by grinding.

Also checked, and correct: the cache is keyed on the resolved banner key, so it stays sound across the member-set mutations post_apply_cleanup performs at steps 0 and 3. A banner that resolves to a different key is a different key; one that stops resolving returns false early without consulting the cache. Walked through all four passes against the rebuilt members_by_id maps.

3. skeptical — assume bugs exist

Edge cases worked through: empty room (early return, cache unused); single member; duplicate identical member entries (memo hit, same verdict); over-cap ban set at step 0-cap followed by drain (same members_by_id, hits are correct); a banner removed at step 0 and absent at steps 2/5 (early false, no cache involvement); duplicate member ids where the surviving entry differs between passes (different resolved key → re-verified).

Two findings, both handled:

• Cycle verdicts are start-dependent. A cycle error names the first node the walk revisits, which is where that walk entered the cycle. Memoizing it would hand a different starting member a differently-worded error. Caught by an equivalence test I had written expecting it to pass; I fixed the code rather than weakening the test. Cycle verdicts are now excluded from the memo, which costs nothing because both callers propagate the first error with ?.
• Self-invitation break happens before the node is recorded, so the offending node itself is not memoized. Verified correct: descendants still get the right error, and re-walking that node recomputes the same verdict.

Determinism: both caches are HashMap, but are only ever queried by key — never iterated, never serialized. No CRDT-convergence exposure of the kind that caused the earlier summary-determinism incidents.

Memory: cache size is bounded by ban count and member count. MembersV1::apply_delta does not bound delta.added, so a hostile delta grows the memo — but the pre-existing code did O(M x D) verifications on the same input and would blow the time budget first. Not a regression; noted in the PR.

4. testing — would each test fail if the fix were reverted?

Answered empirically rather than by inspection. Six mutations, each reverting one part of the fix:

mutation	result
InviteChainCache stops memoizing	1 test fails
BanSignatureCache stops memoizing	1 test fails
post_apply_cleanup step 5 back to the uncached helper	1 test fails
Cargo.toml profile exception removed	1 test fails
ban memo key reverted to BanId	1 test fails
invite memo key reverted to MemberId	1 test fails

Each caught by exactly one test; nothing vacuous. The source pins are deliberately whitespace-insensitive (so cargo fmt cannot disarm them) and cut production source at mod tests rather than at the first #[cfg(test)], per the traps this repo has hit before.

Deliberate choice: the committed tests count verifications instead of asserting wall-clock budgets. A timing assertion would be flaky across CI hardware, and flaky tests are broken tests. The wall-clock evidence lives in the PR body, produced by throwaway probes.

5. big-picture — does this actually fix the symptom at real scale?

Yes, and structurally rather than by shaving a constant: after the change the cost is flat in invite depth (149-156 ms across depths 1 → 200 at 400 members), where before it grew linearly to 16.9 s. The remaining cost is one verification per signed object, which is the irreducible floor for validating a state.

Honest residuals, in the PR body: the room-configuration caps have no hard ceiling, so an owner can still configure past any budget (you would now need ~19,000 signed objects rather than a few hundred members); and MembersV1::apply_delta still does not bound delta.added.

Scope check: the triage assessment's member_info.rs hypothesis was measured (0.06-0.14 ms at 200 members) and deliberately left alone rather than opportunistically "fixed" — it is not the driver, and touching it would add review surface to consensus-critical code for no gain.

Status

Draft, per the standing guardrail: this touches verify / apply_delta internals on the room contract, and publishing it re-keys the contract. Not auto-merging — wants @sanity's sign-off on the approach and on the +2% WASM size for the curve25519-dalek profile exception.

[AI-assisted - Claude]

[sanity]

Security review findings addressed — pushed as 8400c42c

Thanks for the proof on the caching being sound; I've folded the reviewer's verify_signature-depends-on-a-subset-of-the-key argument into the BanSignatureCache doc comment, since that is the invariant a future editor most needs to not break.

FINDING 1 — filed separately, NOT bundled: #551

Agreed on the reasoning, and it is the right call: a length bound rejects deltas that previously succeeded, which is a merge-semantics change, and hiding one inside a PR whose entire safety argument is "output-identical" would destroy the property that makes this reviewable.

Filed as #551 with the attack write-up, the ban.rs #411-round-3-item-C precedent, the 50 MiB MAX_STATE_SIZE calculation showing no wire cap bounds it, and an explicit note that the fix needs a rollout story (old peers keep emitting deltas new peers would reject) plus @sanity's call on the bound. Also recorded there that #548 makes this path cheaper — O(M x D) to O(M) — without closing it, so nobody reads the perf win as a fix.

One nuance I added: the bound is not a straight copy of the ban guard. A legitimate members delta CAN exceed the current member count during a cold full-state merge, in a way a legitimate ban delta cannot, so max_members is not obviously the right number.

FINDING 2 — fixed in #548, and made stronger than suggested

Rather than storing owner_id + debug_assert, I bound the context at construction: InviteChainCache::new(parameters, members_by_id) now holds both for the cache's life, so one instance is structurally incapable of spanning two contexts.

The reason for going further than suggested: the room contract ships as a release build, where debug_assert! is compiled out. A debug assertion would have documented the hazard without defending against it in the only build that matters. The borrow is checked by the compiler in every build.

That also closes the second half of the finding, which a stored owner_id would have missed entirely — the verify vs apply_delta lookup asymmetry, where apply_delta's map deliberately includes not-yet-verified delta members and can therefore grant a weaker Ok.

On the missing pin: with the binding, a test that shares one cache across two owners cannot compile, so the runtime test you asked for is not expressible. I verified this is a real guarantee rather than a claim — mutating verify to pass a temporary lookup fails to build with error[E0716]: temporary value dropped while borrowed. In its place:

• Two behavioural tests establishing that the binding is load-bearing, by showing the verdict genuinely differs across each context a shared cache could have spanned: a_chain_verdict_is_owner_specific (same member, Ok under owner A, Err under owner B) and a_chain_verdict_is_specific_to_the_lookup_it_was_earned_against (member C Ok against apply_delta's combined map, Err against verify's stored-only map).
• Source pins that both call sites bind, so a refactor that reintroduces an unbound cache fails CI rather than compiling quietly.

FINDING 3 — fixed in #548

AuthorizedUserBan::hash now covers all three fields. Checked before changing it that no HashSet/HashMap keyed on AuthorizedUserBan is iterated into state, so bucket order cannot reach the wire or affect convergence — noted in the doc comment so the next person does not have to re-derive it. Pinned by ban_hash_covers_the_whole_struct_not_just_the_signature, which constructs the replay shape exactly (same signature, different body) and asserts they land in different buckets.

Verification

16 tests in the file now, up from 13. Mutation-tested the new work: reverting the Hash impl fails exactly one test; unbinding the cache fails the build. Full suite 422 passed, 0 failed. cargo fmt clean; clippy warning count unchanged from origin/main (11, all pre-existing).

Rebased onto current origin/main before pushing. CI running on 8400c42c; I'll confirm green.

Still DRAFT. Auto-merge not enabled. Not merging.

[AI-assisted - Claude]

[sanity]

Output-identity restored, and both magnitude claims corrected. Head 7d32786d.

1. The divergence — reproduced, then fixed

I reproduced it before touching anything, via the differential test rather than by trusting the report:

before fix : Ok(())
oracle     : Err("Circular invite chain detected for member MemberId(QP2W2KVI)")

and through MembersV1::verify itself: true where the oracle says false.

Fixed with the non-canonical-start bypass. I confirmed the trap you flagged: moving the memo lookup below the cycle guard does not work, because the hit lands on B before the walk reaches A. The bypass neither reads nor writes a verdict for such a start, so it runs as the untouched original walk and leaves nothing start-dependent behind. Every node reached after the start comes out of members_by_id and is canonical by construction, which is why the start is the only place this arises.

The false sentence at member.rs:221-223 is corrected in place, and it now says explicitly that the earlier claim was wrong — a future editor should not have to rediscover why the bypass is there.

Mutation-tested: reverting the bypass fails both differential tests.

2. The differential test — added, and it is the one that mattered

MembersV1::get_invite_chain is the oracle, over every fixture plus the duplicate shape plus verify end-to-end. Your diagnosis of why four rounds of my own review missed it is exactly right and I've put it in the PR body: my equivalence tests used InviteChainCache::new(...) as the "fresh walk", which is the same new implementation with an empty cache, and a one-member cache reproduces the identical short-circuit. The lesson generalises past this PR — when you hand-re-transcribe a loop and delete the reference implementation, the only real oracle is the code you replaced, and here it was still pub the whole time.

3. The perf claims — measured, and worse for my case than you estimated

I wrote the probe you asked for (cli/examples/invite_depth_probe.rs, committed). The live Official room is depth max 4, mean 2.02, median 2 over 496 members — so your instinct was right and my headline table did not describe it. Also: 200 bans, 2000 messages, max_members 2000, 1.44 MB serialized. Both our earlier member counts were stale; it is growing.

Then I counted verifications exactly, with a throwaway counter on verify_struct (reverted — an atomic in the contract hot path is not worth shipping):

	before	after
update_state, 1-msg delta	800	400	2.0x, 338 -> 169 ms
validate_state	3440	3192	1.08x

So F2 confirmed — "≈4s" was wrong; it is 338 ms. The comment now carries the measured count. F4 confirmed — 4 passes -> 2, not 4 -> 1, and I've documented why the remaining pair cannot share a cache (they straddle ComposableState::apply_delta; sharing needs a trait-level change). F3 fixed — the doc comments now say which entry point each half serves. F1 fixed — the table is relabelled as a synthetic worst case, with the measured room first.

The uncomfortable consequence, now stated plainly at the top of the PR: validate_state gains 1.08x at the real room, because its 2000 message signatures dominate and the chain fix cannot touch them. And since the #422 contract is 154 KB — an order of magnitude smaller than the Official room — this PR does not demonstrate a fix for the reported timeout. I've retitled the commit Refs #422, not Closes, and written up what is and is not explained.

F5 checked and eliminated rather than left suspected. I added a fixture where bans target real members with subtrees instead of absent users: validate_state 1435 -> 537 ms, update_state 207 -> 74 ms. No blow-up — it is self-limiting, because banning a member with a large subtree cascade-removes it and the walk shortens. F6 fixed (~272 bytes, the decompressed EdwardsPoint). F7 fixed — harness committed as common/tests/room_scale_bench.rs, #[ignore]d, with the wasmtime invocation in its header.

4. The delegate — settled by measurement, and you were right to insist

origin/main : 6f65e45c…  737,365 bytes   (identical to committed chat_delegate.wasm)
this PR     : 447e3f3d…  754,652 bytes   (+17,287, +2.3%)

The delegate WASM does move. The green check-delegate-migration is not evidence: it diffs the committed artifact, which no source change regenerates. So publishing needs a legacy_delegates.toml entry as well as legacy_room_contracts.toml — now stated in both the PR body and the Cargo.toml comment. A useful side-finding: the baseline delegate build reproduces the committed hash exactly, so unlike the room contract it is byte-reproducible from a standalone -p chat-delegate build.

5. Testing gaps from the testing review — all four closed

AuthorizedMember's structural Eq is now pinned (your mutation passed the whole suite; it now fails one test). The Err-memo path is exercised by a branching tree with a broken shared ancestor. The shared ban cache is exercised across an actual member-set mutation — banner pruned mid-cleanup, asked again at the step-5 sweep. And the fails-open literal pin is replaced with argument-name-insensitive assertions.

Mutation count is now 10, each caught by exactly one test or by the compiler. Suite: 428 passed, 0 failed. clippy back to 11, matching origin/main.

Rebased onto current origin/main (#546/#547/#552 picked up). Filed #554 for the UI-side copy of the same redundancy on the render path.

Still DRAFT. Auto-merge not enabled. Stopping for Ian.

[AI-assisted - Claude]

[sanity]

Independent review — four lenses (output-identity, security, testing, performance)

Reviewed by four reviewers that did not write the code, each blind to the others, reading the source before the PR description. Several claims settled by execution.

Summary: the code is sound and now genuinely output-identical. The original justification was not, and it shrank substantially under review. The author's own final position — Refs #422 rather than Closes, ~2x on update_state, 1.08x on validate_state at the real room — is the accurate one.

Output-identity — was FALSE, now restored (both confirmed by execution)

The claim at member.rs:221-223 that "the starting member of a walk never enters the checks" was false: the start seeds visited, and the memo lookup sat before the cycle guard. Same input, two verdicts:

earlier head : MembersV1::verify -> Ok
origin/main  : MembersV1::verify -> Err("Circular invite chain detected for member MemberId(FWRXVEGZ)")

Trigger: the walk's start is a non-canonical duplicate (members_by_id[start.member.id()] exists and is a different AuthorizedMember) and some node on that walk is already memoized Ok. Reachable from both call sites — verify's map is last-wins, apply_delta's is or_insert over wire-supplied deltas. Repro needs only an ordinary member's own key.

Characterised precisely by the reviewer: the change was a strict relaxation (new == Err implies old == Err), so this suppressed error was the only behavioural delta in the PR. Everything else is genuinely equivalent — BanSignatureCache is exactly output-identical (the resolved key is in the cache key; a non-resolving banner returns false without caching), monoid properties hold, no wire change, neither map is ever iterated, and the opt-level bump cannot change verification results.

Verified fixed at 7d32786d: the same input now returns Err, matching main.

Note that moving the memo lookup below the cycle guard does not fix this — the hit lands before the cycle node is reached. The non-canonical-start bypass is the correct fix.

Why four rounds of self-review missed it

The equivalence tests used InviteChainCache::new(...) and BansV1::ban_signature_matches_current_key as the "uncached" reference — but after this PR those are the new implementation, the originals having been deleted. A one-entry cache reproduces the identical short-circuit. MembersV1::get_invite_chain was still pub and still wrapped the original walk the whole time; a ~10-line differential against it catches this on the first run. That differential is now present.

Security — clean (confirmed)

No forged or unverified object can inherit a cached positive verdict, attacked from four angles, each closed by construction. The structural re-keying is complete (no MemberId/BanId/fast_hash in any cache key), the key is a strict superset of what the signature covers, and #411 is not weakened — its round-4 regression tests now exercise the cached path end-to-end. Two hardening items found and fixed: the cache now binds (parameters, members_by_id) at construction (the author correctly rejected a debug_assert guard, since the contract ships as a release build where it compiles out), and AuthorizedUserBan::hash covers all three fields.

Confirmed by execution before the fix: weakening AuthorizedMember's Eq to compare only .member left all 13 tests green, and one cache shared across two rooms returned cached Ok(()) where a fresh cache returns Err. Both now pinned — the second by the compiler.

Performance — mechanism real, magnitudes were wrong

The O(M×D) mechanism is confirmed by reading, the fix is genuinely O(M), the cache build is O(M) not O(M×D), and there is no cold/warm distinction (every call builds a fresh cache, so the "after" numbers reproduce on every invocation). OptLevel::None at wasmtime_engine.rs:1350 and the 5.0s budget at runtime.rs:395 both confirmed. The author's 46x → 1.6x correction on the curve25519-dalek profile — native measurement versus wasmtime, where Cranelift re-optimizes on load — was right, and catching it is what redirected the investigation to the algorithmic cause.

What was wrong: the "≈ 4s of WASM CPU per update" figure (a room at 200/200 skips the cap pass, so 3 passes ≈ 253 ms), the 4→1 pass claim (it is 4→2), and the headline table's depth-50/200 shape, which contradicted the PR's own fixture comment describing the live room as a two-level star. Measured on the live room — depth max 4, mean 2.02 over 496 members — the real gains are 2.0x on update_state and 1.08x on validate_state, the latter because 2000 message signatures dominate. All corrected in the PR.

Migration implications (both required to publish)

• Room contract re-key, per the normal river-publish ritual.
• The delegate WASM bytes move: origin/main 737,365 bytes / 6f65e45c… versus this PR 754,652 / 447e3f3d…. A green check-delegate-migration is not evidence here — it diffs the committed artifact, which no source change regenerates. Publishing needs a legacy_delegates.toml entry as well as legacy_room_contracts.toml.

Filed separately rather than bundled

• Room contract: MembersV1::apply_delta does not bound delta.added, so a member can force unbounded signature verification #551 — MembersV1::apply_delta has no bound on delta.added.len(); verification runs before remove_excess_members trims. The ban path has this guard (ban.rs:512, feat: member deputies for ban authority (invite-subtree moderation) #411 round 3 item C); the member path never got it. ~19,000 members reaches the 5s budget and no wire cap prevents it (MAX_STATE_SIZE is 50 MiB). This PR makes the path cheaper without closing it. Bundling a length bound would have destroyed output-identity, which is why it is separate.
• UI: enforced_banned_member_ids does 200 Ed25519 verifications per call on the render path #554 — the UI render path carries the same ban-verification redundancy.

Scope correction

This PR does not demonstrate a fix for #422. The reported contract is 154 KB, an order of magnitude smaller than the Official room, and at these measured rates neither redundancy approaches 5s at that size. Three candidate causes were checked and eliminated. #422 should stay open.

[AI-assisted - Claude]