fix(node,server): rate-limit and harden TEE attestation-verify entrypoints

[xilosada]

What

Attestation verification — DCAP quote verify plus an outbound Intel-PCS collateral fetch — is expensive, and two entrypoints reached it from untrusted input with no bound. This adds rate-limiting / dedup / concurrency caps to both, and tightens error hygiene on the HTTP path.

node — inbound TeeAttestationAnnounce gossip

New TeeAdmissionThrottle (crates/node/src/handlers/tee_attestation_throttle.rs) gates the announce before the verify task is spawned, with three independent checks:

• per-(group, quote_hash) dedup (reuses the existing is_quote_hash_used check, lifted earlier),
• a per-peer token bucket keyed on the announcing peer,
• a global bounded inflight-verify semaphore.

Liveness preserved: identical fleet re-announces drop as duplicates without consuming per-peer tokens; transient-failure retries use a fresh nonce/quote (new quote_hash), so admission recovery still works.

server — POST /admin-api/tee/verify-quote

• Rate-limited with an inflight cap → 429 Too Many Requests when exceeded (verify_quote_throttle.rs).
• Errors are now scrubbed: internal/verification failures return a generic 500, a malformed quote returns a fixed 400; the real cause is logged server-side only (previously the PCS endpoint / cert / TCB details could surface in the response body).
• /tee/attest is unchanged.

/verify-quote is intentionally kept reachable without an admin token — it is consumed unauthenticated by the control plane — so this hardens it in place rather than gating it behind auth.

Notes

• No public API change (verify_attestation signature untouched); no new dependencies (reuses tokio::sync::Semaphore, LazyLock, sha2, calimero-governance-store).
• Throttle defaults are conservative module constants, not load-tuned — easy to adjust.

Tests

• calimero-node --lib: 408 passed (incl. 7 new throttle-logic tests; existing specialized handler tests green).
• calimero-server --lib: 39 passed (3 throttle + 3 error-scrub tests).
• cargo check + clippy clean; cargo fmt --check (1.88) clean.
• Not covered: a full axum integration test exercising the real 500/429 path needs a live PCS endpoint + a valid TDX quote; the deterministic pieces (error-scrub mapping, throttle decision logic) are unit-tested instead.

[cursor]

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

[meroreviewer]

🤖 MeroReviewer

Reviewed by 1 agents | Quality score: 28% | Review time: 74.8s

🟡 2 warnings, 💡 1 suggestions. See inline comments.

---

_{🤖 Generated by MeroReviewer | Review ID: review-fe2a6013}

[meroreviewer]

Documentation Review

The following documentation may need updates based on the changes in this PR:

• 🟡 architecture/crates/node.html: Files matching crates/node/** were changed but architecture/crates/node.html was not updated (per source_to_docs_mapping).
• 🟡 architecture/crates/server.html: Files matching crates/server/** were changed but architecture/crates/server.html was not updated (per source_to_docs_mapping).
• 🟡 architecture/crates/sync.html: Files matching crates/node/** were changed but architecture/crates/sync.html was not updated (per source_to_docs_mapping).

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 3 agents | Quality score: 64% | Review time: 251.5s

🟡 5 warnings, 💡 2 suggestions. See inline comments.

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-b2f97206}

[meroreviewer]

🤖 MeroReviewer

Reviewed by 1 agents | Quality score: 28% | Review time: 79.8s

🟡 2 warnings, 💡 2 suggestions. See inline comments.

---

_{🤖 Generated by MeroReviewer | Review ID: review-28351fb6}

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 3 agents | Quality score: 45% | Review time: 1250.9s

🟡 7 warnings, 💡 5 suggestions. See inline comments.

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-8114fc13}

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 3 agents | Quality score: 59% | Review time: 800.2s

🟡 6 warnings, 💡 1 suggestions. See inline comments.

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-5d415319}

[meroreviewer]

🟡 rejections_do_not_slow_token_recovery test comment contradicts the actual implementation

The test comment says "this call advances last to t0+0.5s" for the RateLimited rejection at t0+500ms. But the implementation explicitly does NOT advance bucket.last on rejection (the transactional commit only runs on Proceed). The comment is misleading: it describes the old (broken) behaviour the test is guarding against, not the current behaviour. A reader will be confused about whether the rejection advances the clock. The test itself is correct; only the comment is wrong.

Suggested fix:

Change the comment to: "Half a second in: only 0.5 token refilled → rejected. `bucket.last` is NOT advanced (transactional commit only on Proceed), so the full 1s of elapsed time is credited at t0+1s."

[meroreviewer]

🟡 New peer bucket inserted before rate-limit check, leaking memory on rejection

At Gate 2, self.peers.entry(source).or_insert(...) unconditionally inserts a new PeerBucket for every previously-unseen peer, even when the announce is subsequently rejected at Gate 2 (bucket empty) or Gate 3 (AtCapacity). A flood of unique source peers will therefore fill self.peers up to MAX_TRACKED_PEERS with zero-token buckets, each of which the prune logic keeps alive until idle_cutoff (2 × refill = 4 s by default) has elapsed. Under adversarial churn this means the map stays near the cap continuously, forcing frequent O(N) eviction sweeps. The fix is to read the bucket without inserting on rejection: compute refilled from the existing entry (or treat a missing entry as burst tokens), and only call or_insert in the commit block after all gates pass.

Suggested fix:

Replace the `entry(...).or_insert(...)` call with a read-only lookup: `let (tokens, last) = self.peers.get(&source).map(|b| (b.tokens, b.last)).unwrap_or((burst, now));`. Compute `refilled` from those values. Only after all gates pass, write back: `self.peers.entry(source).or_insert(PeerBucket { tokens: refilled - 1.0, last: now }).tokens = refilled - 1.0; bucket.last = now;`

[meroreviewer]

🟡 Prune evicts by bucket.last (last grant time), not by last-seen time, so active-but-throttled peers are evicted first

evict_oldest_peers sorts by b.last, which is the timestamp of the last granted token (only updated on Proceed). A peer that is actively flooding but being rate-limited will have a stale last and will therefore be evicted before an idle peer whose last grant was recent. This inverts the desired eviction order: the attacker's entries survive while legitimate-but-quiet peers are dropped. A separate last_seen: Instant field updated on every check call (regardless of outcome) would give correct LRU semantics.

Suggested fix:

Add a `last_seen: Instant` field to `PeerBucket`, updated unconditionally at the top of `check` (before any gate). Use `b.last_seen` as the eviction key in `evict_oldest_peers`.

[meroreviewer]

🟡 AtCapacity does not commit the token, but the token was already computed inside the lock — no actual bug, but the comment is misleading

The comment says "Acquire the inflight permit while holding the bucket lock so the token and permit are committed atomically". On AtCapacity the permit acquisition fails and the function returns without writing bucket.tokens or bucket.last — correct. However, the comment implies the token is committed together with the permit, which is only true on the Ok branch. The Err branch silently leaves the bucket untouched. This is the right behaviour, but the comment should make the AtCapacity non-commit explicit to avoid future maintainers accidentally adding a token-burn before the match.

Suggested fix:

Add to the comment: "On `Err(_)` (AtCapacity) the bucket is left untouched — no token is burned and the refill clock is not advanced."

[meroreviewer]

🟡 AtCapacity does not consume a token but the comment says token+permit are committed atomically — token is silently not burned on AtCapacity

The check_at comment on line 100 says "On Decision::Proceed one token and one inflight permit are consumed; on rejection nothing is consumed." The implementation correctly does NOT update bucket.tokens/bucket.last on AtCapacity (the Err(_) arm returns without writing). However, the Mutex is held across the try_acquire_owned call. If try_acquire_owned returns Err, the function returns Decision::AtCapacity while still holding the MutexGuard — the guard is dropped at the closing brace of the block on line 157. This is correct (no deadlock, no data corruption), but it means the lock is held slightly longer than necessary on the rejection path. More importantly, the comment on line 140 says "Acquire the inflight permit while holding the bucket lock so the token and permit are committed atomically" — this is the right rationale for the Proceed path, but on AtCapacity the lock is held for no reason after the try_acquire_owned fails. This is a minor performance issue under high concurrency (many concurrent rejects all serialize on the Mutex unnecessarily), not a correctness bug.

Suggested fix:

Restructure to release the lock before returning AtCapacity: compute `refilled`, check `< 1.0`, then drop the lock, attempt `try_acquire_owned`, and only re-acquire the lock to commit `tokens`/`last` on success. Alternatively, accept the current structure as correct and update the comment to clarify that the lock is held across the permit acquisition intentionally.

[meroreviewer]

🟡 Idle-peer prune condition uses > not >=, may retain one extra refill cycle

The idle-peer retention predicate is now.saturating_duration_since(b.last) > idle_cutoff (strict greater-than). idle_cutoff is per_peer_refill.saturating_mul(2). A peer whose bucket is full and whose last grant was exactly idle_cutoff ago is retained for one more prune cycle. This is a minor off-by-one in the eviction policy — not a security issue, but inconsistent with the dedup TTL check on line 278 which uses < (strict less-than, i.e. evicts at exactly TTL). Both should use the same boundary convention.

Suggested fix:

Change `> idle_cutoff` to `>= idle_cutoff` to be consistent with the dedup TTL boundary and evict peers that have been idle for exactly the cutoff duration.

[meroreviewer]

💡 SHA-256 of the full quote bytes computed on the actor thread before throttle check

Sha256::digest(quote_bytes) hashes the entire quote (up to 64 KiB) synchronously on the NodeManager actor thread, before the throttle gates have had a chance to reject the announce cheaply. A flood of unique-quote announces will therefore burn actor-thread CPU on hashing even when the per-peer rate limit or inflight cap would immediately reject them. Moving the hash computation to after the durable dedup check (which only needs group_id) and before Gate 1 of the in-memory throttle is the minimal fix; alternatively, pass the raw quote_bytes to the throttle and hash lazily inside check only when needed.

Suggested fix:

Reorder: perform the durable `is_quote_hash_used` check with a lazily-computed hash (compute it only once, after the topic parse succeeds), but consider whether the hash can be skipped entirely when the per-peer bucket is already empty (Gate 2 rejects before Gate 1 in the throttle). At minimum, document that the hash is computed unconditionally and is acceptable given the 64 KiB quote size.

[github-actions]

E2E Rust Apps Failed

One or more E2E workflows (scaffolding-e2e, xcall-example) failed.

Please check the workflow logs for more details.

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 3 agents | Quality score: 56% | Review time: 720.4s

🟡 6 warnings, 💡 3 suggestions. See inline comments.

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-05371d71}

[meroreviewer]

🟡 New peer entry inserted into peers map even on RateLimited / AtCapacity paths

At Gate 2, self.peers.entry(source).or_insert(PeerBucket { tokens: burst, last: now }) is called unconditionally. If the peer is brand-new and the announce is subsequently rejected at Gate 3 (AtCapacity), the peer entry is still inserted into the map with a full bucket. This means an attacker flooding with unique peer IDs (e.g. spoofed PeerIds) while the semaphore is saturated will grow self.peers without bound between prune sweeps, up to MAX_TRACKED_PEERS. The entry should only be inserted on Proceed, or the entry should be removed on non-Proceed outcomes.

Suggested fix:

Use `self.peers.get(&source)` to read the bucket for the rate-limit check without inserting, and only call `self.peers.entry(source).or_insert(...)` in the commit block after all gates pass.

[xilosada]

Fixed in 4f2f797 — the per-peer bucket is now read without inserting on the reject paths. Gate 2 does a read-only get_mut (treating a brand-new peer as a full bucket, so first contact is never rate-limited), and the bucket is only inserted/advanced in the commit block on Proceed. So a flood of unique source peers that never proceed can't grow peers, and a rejected first announce no longer plants a last-stamped bucket. Added the refused_new_peer_is_not_tracked test asserting an AtCapacity-refused new peer is not inserted.

[meroreviewer]

🟡 Prune sweep runs BEFORE the dedup/rate-limit gates it is meant to amortise

The prune call at the top of check runs unconditionally when due || over_cap, which includes the very first call on a new Instant. More importantly, the prune sweep (which is O(tracked entries)) executes before Gate 1 (dedup) and Gate 2 (rate-limit) can cheaply reject the announce. Under an adversarial flood of unique-quote, unique-peer frames the maps grow quickly, the size-cap guard fires, and every call pays the O(N) eviction cost before the cheap gates can reject it. The comment says "amortise it so an adversarial flood doesn't pay an O(N) sweep on every announce before the cheap gates can reject it" — but the current ordering does exactly that when the cap is exceeded. The fix is to run the cheap gates first and only prune on Proceed (or at least after the dedup gate).

Suggested fix:

Move the prune block to after Gate 1 (dedup) and Gate 2 (rate-limit), so cheap rejections never pay the O(N) sweep cost. Alternatively, only prune on `Proceed` paths.

[xilosada]

Assessed — keeping prune at the top, no change. With the insert-on-Proceed fix (4f2f797), both maps grow only on the commit path: recent_quotes.insert and the peer entry().or_insert both live in the Proceed block. A flood of unique-quote / unique-peer frames that get RateLimited or AtCapacity therefore grows neither map, so the size-cap guard is not triggered by adversarial rejected traffic — the premise of the O(N)-on-every-call concern. The remaining time-based sweep is amortized to ≤1/PRUNE_INTERVAL (1s). Prune stays before the gates so the memory bound is enforced before any work and the dedup map reflects TTL expiry.

[meroreviewer]

🟡 Rejected announce advances bucket.last for a new peer, stealing refill credit

When a new peer's first announce is rejected at Gate 3 (AtCapacity), the or_insert at line ~295 creates a bucket with last: now. On the next announce from that peer (after the semaphore frees), the elapsed time is computed from this now, not from the actual first-seen time. This is the same "refill clock stolen by rejection" bug the test rejections_do_not_slow_token_recovery guards against for existing peers — but it is not guarded for the first announce of a new peer that hits AtCapacity.

Suggested fix:

Only insert the peer bucket entry in the commit block (after all gates pass), consistent with the stated invariant that rejections leave `tokens` and `last` untouched.

[xilosada]

Fixed in 4f2f797 (and behaviorally already by the transactional commit in an earlier round). A new peer refused at Gate 3 is no longer inserted at all, so there's no last-stamped bucket to steal refill credit on the next announce. More generally tokens/last are written only on Proceed, so no rejection — first or not — advances the refill clock.

[meroreviewer]

🟡 Eviction removes entries that may still be within dedup TTL, silently allowing replay

evict_oldest is called after retain has already removed all expired entries. If the map is still over MAX_TRACKED_QUOTES after TTL-based pruning (i.e., there are more than 8192 non-expired entries), evict_oldest removes the oldest non-expired entries by insertion time. Those evicted entries are within their dedup TTL, so the same (group, quote_hash) can be re-submitted and will pass Gate 1 as if it were new. Under an adversarial flood of 8192+ distinct quotes this silently degrades the dedup guarantee to a best-effort one. The PR description does not acknowledge this trade-off.

Suggested fix:

Add a comment explicitly documenting that eviction of non-expired entries is an intentional trade-off (memory bound wins over dedup completeness under extreme adversarial churn), so future readers understand the security implication and don't accidentally "fix" it in a way that removes the memory bound.

[xilosada]

Done — added the requested doc comment in 4f2f797 (in prune, at the recent_quotes hard-cap eviction). It records that evicting non-expired dedup entries is an intentional, bounded trade-off: the memory bound is the hard guarantee, while the per-peer rate limit + global inflight cap (plus the durable is_quote_hash_used check for admitted quotes) remain the real DoS backstop, and that the cap must not be 'fixed' away. Note reaching it now requires >8192 distinct admitted quotes within the TTL, since entries are only inserted on Proceed.

[meroreviewer]

🟡 New peer bucket inserted even on RateLimited path, advancing last on first rejection

At Gate 2, self.peers.entry(source).or_insert(PeerBucket { tokens: burst, last: now }) is called unconditionally before the refilled < 1.0 check. For a brand-new peer this inserts a bucket with tokens = burst and last = now. The subsequent refilled computation then yields exactly burst (elapsed = 0), so the first call always passes — that is correct. However, the entry is now in the map even if the announce is later rejected at Gate 3 (AtCapacity). The comment says "the bucket is still unmodified at this point" for AtCapacity, but the or_insert has already inserted a fresh entry with tokens = burst. This means a peer that is rejected only at Gate 3 still gets a bucket entry created, which is a minor state-leak but not a correctness issue for the token count itself. More importantly, the comment in the Proceed commit block says "last advances only here", which is true for existing peers but misleading for new peers where last = now was already set by or_insert. This is a documentation/logic inconsistency that could confuse future maintainers.

Suggested fix:

Use `self.peers.entry(source).or_insert_with(|| PeerBucket { tokens: burst, last: now })` (already done) but add a note that for a brand-new peer the `or_insert` sets `last = now` as the initial baseline, and the commit block's `bucket.last = now` is a no-op for that case. Alternatively, compute `refilled` without inserting first: look up the entry with `.get()`, compute the refilled value, and only call `.entry().or_insert()` after all gates pass.

[xilosada]

Fixed in 4f2f797 — the per-peer bucket is now read without inserting on the reject paths. Gate 2 does a read-only get_mut (treating a brand-new peer as a full bucket, so first contact is never rate-limited), and the bucket is only inserted/advanced in the commit block on Proceed. So a flood of unique source peers that never proceed can't grow peers, and a rejected first announce no longer plants a last-stamped bucket. Added the refused_new_peer_is_not_tracked test asserting an AtCapacity-refused new peer is not inserted.

[meroreviewer]

🟡 AtCapacity path does not consume a token but still holds the bucket lock while acquiring the semaphore permit

The bucket Mutex is held for the entire duration of try_acquire_owned(). While try_acquire_owned is non-blocking, the comment justifying this says "the token and permit are committed atomically". However, on the AtCapacity branch the token is NOT committed (correctly), but the lock is still held across the semaphore acquire attempt. This is fine for correctness today, but the comment "Acquire the inflight permit while holding the bucket lock so the token and permit are committed atomically" is misleading: on AtCapacity the token is not committed, so the atomicity argument only applies to the Ok branch. More critically, if tokio::sync::Semaphore::try_acquire_owned ever becomes blocking in a future version, this would hold a std::sync::Mutex across an await — a classic async deadlock risk. The fix is to acquire the permit before locking the bucket, or document the non-blocking guarantee explicitly.

Suggested fix:

Acquire the semaphore permit before locking the bucket mutex, then commit both atomically only on success. This eliminates any risk of holding a sync Mutex across a potentially-blocking call and makes the atomicity argument cleaner.

[xilosada]

Clarified the comment in 4f2f797; keeping the structure. The bucket Mutex is intentionally held across try_acquire_owned (which never blocks — it returns in a few instructions). The reviewer's alternative (drop the lock, acquire, re-take to commit) would open a TOCTOU window where a concurrent grant could be clobbered, so it'd be a regression. The comment now states the hold is deliberate and that on Err(_)/AtCapacity the bucket is left untouched.

[meroreviewer]

💡 Idle-peer pruning uses per_peer_refill * 2 as cutoff, which is 4 seconds at defaults — too aggressive

The idle cutoff for peer bucket eviction is per_peer_refill.saturating_mul(2), which at the default of 2 seconds gives a 4-second idle window. A legitimate peer that sends one announce every 5 seconds (well within the rate limit) will have its bucket evicted between announces, causing it to restart with a full burst on every announce. This is not a correctness issue (the bucket resets to burst on re-insertion, which is the same as a fresh peer), but it means the per-peer rate-limit history is lost for any peer that pauses for more than 4 seconds, defeating the purpose of tracking per-peer state across a longer window.

Suggested fix:

Use a larger idle cutoff, e.g. `self.dedup_ttl` or a fixed `Duration::from_secs(60)`, so that a peer's token history is retained for at least as long as a typical announce interval.

[xilosada]

Assessed — no change; the 4s cutoff is safe. A peer is idle-pruned only when it is full (tokens >= burst) and idle. A full bucket carries no restrictive state — re-inserting a fresh full bucket on the next announce is identical, so nothing is lost and there's no rate-limit bypass (full == unthrottled). A peer with depleted tokens (actually being throttled) is retained regardless of idle duration, and after 4f2f797 that retention is keyed on last_seen (last activity), so an active-but-throttled peer is never pruned. Raising the cutoff would only keep unthrottled buckets around longer, costing memory for no protection benefit.

[meroreviewer]

💡 SHA-256 of the full quote bytes computed on the actor thread before throttle check

Sha256::digest(quote_bytes) hashes the entire quote (up to 64 KiB) synchronously on the NodeManager actor thread before the throttle is consulted. The throttle's Gate 1 (dedup) and Gate 2 (rate limit) are cheap map lookups that could reject the announce without needing the hash. Computing the hash first means a flood of rate-limited announces still pays the full SHA-256 cost on the actor thread, partially defeating the purpose of the throttle.

Suggested fix:

Move the `Sha256::digest` call to after the per-peer rate-limit check (Gate 2), or use a cheaper identity (e.g. the first 32 bytes of `quote_bytes` as a probabilistic key) for the dedup map and only compute the full hash when needed. Alternatively, accept the current approach as "good enough" given the actor-thread serialisation already limits concurrency, and document the trade-off.

[xilosada]

Documented at the hash site in 4f2f797. The hash can't be deferred behind the gates: quote_hash is the key for both the durable is_quote_hash_used check and the throttle's Gate 1 (dedup), so it must exist before either runs. It's bounded — quote_bytes is capped at gossipsub's 64 KiB default at the transport, and SHA-256 over ≤64 KiB is tens of microseconds, negligible beside the PCS-fetch + DCAP verify the hash gates. The comment now states this.

[meroreviewer]

💡 Process-global LazyLock<VerifyQuoteThrottle> prevents per-test isolation

The VERIFY_QUOTE_THROTTLE static is shared across all tests in the process. The existing tests only exercise scrub_verify_error (a pure function) and the throttle's own unit tests (in verify_quote_throttle.rs), so there is no current conflict. However, any future integration test that calls handler(...) directly will share throttle state with other tests, causing non-deterministic failures depending on test ordering. This is a common pitfall with process-global state in async test suites.

Suggested fix:

Consider making the throttle injectable (e.g. pass it as an `Extension` or embed it in `AdminState`) so tests can construct an isolated instance. If the static is intentional for simplicity, add a comment warning that `handler` tests must not be run in parallel with other tests that exercise the throttle.

[xilosada]

Assessed — keeping the process-global (consistent with the round-4 reply on the same point). The throttle's logic is unit-tested directly via check_at(now), the injection seam; the singleton only matters for handler-level integration tests, of which none exist (they'd need a live Intel-PCS endpoint + valid TDX quote — the documented follow-up gap). If such a test lands, injecting via Extension<Arc<VerifyQuoteThrottle>> is the right move; until then it's router plumbing for a non-existent test.

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 3 agents | Quality score: 65% | Review time: 257.4s

🟡 5 warnings, 💡 1 suggestions. See inline comments.

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-9a1c86cb}

[meroreviewer]

🟡 New peer entry inserted before rate-limit check, leaking memory on rejection

At line 183 self.peers.entry(source).or_insert(...) unconditionally inserts a new PeerBucket for every previously-unseen peer, even when the announce is subsequently rejected at Gate 2 (RateLimited) or Gate 3 (AtCapacity). A flood of unique spoofed source peers will fill self.peers up to MAX_TRACKED_PEERS (4096) entries, each with a full burst token, before the size-cap prune fires. The comment says 'do NOT write it back yet', but the or_insert IS a write — it creates the entry. The fix is to read the bucket without inserting, and only insert on Proceed. For example: compute refilled from self.peers.get(&source) (treating absence as a full bucket), and only call self.peers.entry(source).or_insert(...) in the commit block at the bottom.

Suggested fix:

Replace `self.peers.entry(source).or_insert(PeerBucket { tokens: burst, last: now })` with a read-only lookup: `let (tokens_before, last_before) = self.peers.get(&source).map(|b| (b.tokens, b.last)).unwrap_or((burst, now));`. Compute `refilled` from those values. In the commit block, write `self.peers.entry(source).or_insert(PeerBucket { tokens: burst, last: now }).tokens = refilled - 1.0; ... .last = now;`.

[xilosada]

Fixed in 4f2f797 — the per-peer bucket is now read without inserting on the reject paths. Gate 2 does a read-only get_mut (treating a brand-new peer as a full bucket, so first contact is never rate-limited), and the bucket is only inserted/advanced in the commit block on Proceed. So a flood of unique source peers that never proceed can't grow peers, and a rejected first announce no longer plants a last-stamped bucket. Added the refused_new_peer_is_not_tracked test asserting an AtCapacity-refused new peer is not inserted.

[meroreviewer]

🟡 rejections_do_not_slow_token_recovery test comment contradicts the actual implementation

The test comment at line 270 says 'this call advances last to t0+0.5s', but the implementation explicitly does NOT advance last on a rejection — bucket.last is only written in the commit block on Proceed. The comment is a copy-paste error from an earlier design where rejections did advance the clock. While the test assertion itself is correct (it verifies the right behaviour), the misleading comment could cause a future reader to think the rejection DOES advance the clock and then 'fix' the implementation to match the comment, reintroducing the bug the test guards against.

Suggested fix:

Change the comment to: '// Half a second in: only 0.5 token refilled → rejected. This call must NOT advance `last` (the transactional bucket only commits on Proceed).'

[xilosada]

Fixed in 4f2f797 — the comment now matches the transactional implementation: it states the rejection does not advance last (the bucket is committed only on Proceed), so the full 1s of elapsed time is credited at t0+1s. Good catch — the old comment described the pre-transactional behaviour.

[meroreviewer]

🟡 AtCapacity does not commit the token, but the comment implies it should

When try_acquire_owned fails (line 120), the function returns Decision::AtCapacity without updating bucket.tokens or bucket.last. This is the intended behavior per the comment ('rejected request leaves tokens and last untouched'). However, the comment at line 107 says 'Acquire the inflight permit while holding the bucket lock so the token and permit are committed atomically'. The word 'atomically' is misleading: the token is NOT committed on AtCapacity, only on Proceed. This is correct behavior but the comment is misleading. More importantly: the AtCapacity path holds the bucket MutexGuard while calling try_acquire_owned on the semaphore. Since try_acquire_owned is non-blocking this is fine, but if a future maintainer changes this to acquire_owned (async), the mutex would be held across an await point, causing a deadlock or blocking the executor. The current code is safe, but the structure is fragile.

Suggested fix:

Add a comment explicitly noting that `try_acquire_owned` must remain non-blocking here because the mutex guard is held. Consider acquiring the permit before locking the mutex to make the structure more robust.

[xilosada]

Clarified in 4f2f797. The Err(_)/AtCapacity arm now carries an explicit comment that the bucket is left untouched — no token burned, last not advanced — and the Ok arm notes the commit. The implementation already did this; only the comment needed to make the non-commit explicit.

[meroreviewer]

🟡 Prune idle-peer retention condition is inverted — active peers are evicted, idle peers are kept

The retain predicate at line 237 is !(b.tokens >= burst && elapsed > idle_cutoff), which keeps a peer when it is NOT (full AND idle). This correctly drops fully-refilled idle peers. However, a peer that is partially drained (tokens < burst) but has not been seen for a very long time is also retained forever, because the condition requires BOTH tokens >= burst AND elapsed > idle_cutoff to evict. A peer that was rate-limited and then went silent will never be pruned by the time-based path — only the hard-cap eviction will eventually remove it. This is a minor memory leak under adversarial churn but contradicts the comment 'A peer whose bucket has refilled to full and hasn't been seen for a while carries no state worth keeping' — the symmetric case (drained AND long-idle) is equally worthless. Consider also evicting when elapsed > idle_cutoff regardless of token level.

Suggested fix:

Change the retain predicate to: `self.peers.retain(|_, b| { let elapsed = now.saturating_duration_since(b.last); elapsed <= idle_cutoff });` — evict any peer not seen within the idle window, regardless of token level. The hard-cap eviction already handles the adversarial case; this just makes the time-based cleanup symmetric.

[xilosada]

Fixed in 4f2f797. The idle/LRU key is now last_seen (refreshed on every check, any outcome), not last (grant time). So an active-but-throttled peer — recent last_seen, stale last — is retained rather than evicted; evicting it would have handed it a fresh full bucket and reset its limit. evict_oldest_peers keys on last_seen too.

[meroreviewer]

🟡 evict_oldest_entries removes wrong entries when timestamps collide

The eviction logic calls entries.select_nth_unstable_by_key(remove - 1, ...) which partitions so that elements [0..=remove-1] are the remove smallest timestamps. It then removes entries[..remove] from the map. This is correct when all timestamps are distinct. However, when many entries share the same Instant (e.g. all inserted in the same check call under a flood, or in tests using a fixed now), select_nth_unstable is not stable and the partition boundary is arbitrary among equal elements. The code removes exactly remove entries, which is the right count, so the size cap is enforced. The real issue is that evict_oldest_peers uses v.last as the timestamp, but last is only updated on Proceed — a peer bucket inserted by the or_insert path (see finding above) has last = now of the flood, so it looks as new as legitimate peers and may be evicted preferentially over older, legitimate peers. This is a secondary consequence of the first finding; fixing the or_insert issue resolves this too.

Suggested fix:

Fix the peer-bucket insertion issue (only insert on Proceed). If equal-timestamp ties need deterministic eviction, sort by key as a tiebreaker, but this is secondary.

[xilosada]

Fixed in 4f2f797, exactly as you note (you flagged it as secondary to the or_insert issue). Peers are now inserted only on Proceed, so flood-inserted same-now buckets no longer exist, and evict_oldest_peers keys on last_seen. On the residual tie point: select_nth_unstable evicts the correct count of smallest-timestamp entries; among exactly-equal timestamps the choice is arbitrary, which is fine for a size cap (the tied entries are equally old). No deterministic tiebreak needed.

[meroreviewer]

💡 SHA-256 of quote_bytes computed before throttle but after info! log leaks timing

The quote_hash is computed via Sha256::digest(quote_bytes) on the actor thread (line 157), which is synchronous and O(quote_size). For a 64 KiB quote this is ~microseconds and acceptable. However, the info! log at line 148 fires unconditionally before any throttle check, meaning a flood of announces will produce an info! log entry per announce even for ones that will be immediately dropped by the durable dedup or throttle. Under a replay flood this could itself become a log-amplification vector. Consider moving the info! log to after the throttle gates pass (or downgrading it to debug! before the gates).

Suggested fix:

Move the `info!(... "Received TEE attestation announce on namespace topic")` log to after the throttle `Decision::Proceed` arm, or change it to `debug!` before the gates and `info!` only on proceed.

[xilosada]

Assessed — no change. The info! is server-side structured logging, not part of any response, so it isn't an attacker-observable side channel: there's no response-timing differential exposed to the caller (the announce path returns nothing to the peer; gossip delivery is fire-and-forget). The hash itself is bounded (≤64 KiB, tens of µs) and is the required dedup key. Reordering the log wouldn't change any externally-observable timing.

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 3 agents | Quality score: 73% | Review time: 255.8s

🟡 3 warnings, 💡 1 suggestions. See inline comments.

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-d40e2a90}

[meroreviewer]

🟡 Idle-peer eviction uses per_peer_refill * 2 as cutoff, which is 4 seconds at default — far too aggressive

The idle cutoff for peer eviction is self.per_peer_refill.saturating_mul(2) (line 302), which at the default DEFAULT_PER_PEER_REFILL = 2s is only 4 seconds. A peer that sends one announce and then goes quiet for 5 seconds will be evicted and get a fresh full bucket on its next announce. This effectively resets the per-peer rate limit for any peer that pauses between bursts for more than 4 seconds, undermining the rate-limit's ability to track sustained abusers. The dedup TTL is 300 seconds, so a peer can replay a new quote every 5 seconds and always get a fresh bucket.

Suggested fix:

Use a much longer idle cutoff — at minimum `dedup_ttl` (300s) or a dedicated constant like `Duration::from_secs(300)` — so a peer's rate-limit state is retained for at least as long as the dedup window. The current 4-second cutoff is almost certainly unintentional.

[xilosada]

Assessed — defused by the prune-predicate fix in 27c17bc; keeping the 4s cutoff. With the elapsed-based fullness check, a peer is pruned only once its bucket would have fully refilled (≡ a fresh peer) AND it's been idle past the cutoff. A sustained abuser that bursts and pauses has a depleted bucket — at default burst 5 / refill 0.5·s⁻¹ it needs ~10s to refill, so a 5s replay cadence leaves it partially-refilled and it is NOT pruned (its throttle debt is kept). Only fully-recovered peers are dropped, and those are byte-for-byte equivalent to a fresh peer, so the cutoff value can't grant any rate-limit bypass. A longer cutoff would only retain fresh-equivalent buckets longer — more memory, zero protection gain — so 4s (prompt reclamation, bounded by MAX_TRACKED_PEERS regardless) is deliberate.

[meroreviewer]

🟡 Idle-peer pruning condition uses tokens >= burst but tokens can slightly exceed burst due to f64 rounding

The prune retention predicate is !(b.tokens >= burst && ...). The token count is computed as (cur_tokens + elapsed * refill_per_sec).min(burst), so it is capped at burst by .min(). However, b.tokens is stored as refilled - 1.0 after a grant, which is always strictly less than burst. The only way b.tokens >= burst can be true is if the bucket was never spent after the last prune refill — but since tokens are only written as refilled - 1.0 on the commit path, a bucket that has been granted at least once will have tokens < burst until enough time passes for a full refill.

The real issue: after a grant, b.tokens = refilled - 1.0. On the next check call for the same peer, refilled = (b.tokens + elapsed * rate).min(burst). If elapsed is large enough, refilled == burst, and the grant sets b.tokens = burst - 1.0. The prune condition b.tokens >= burst can therefore never be true for a peer that has ever been granted a token, because the stored value is always refilled - 1.0 <= burst - 1.0 < burst. This means the idle-peer prune path (retain) never actually evicts any peer based on the tokens >= burst condition — it only evicts via the hard-cap evict_oldest_peers fallback. Peers that have been granted tokens but gone idle will accumulate until the hard cap is hit.

Suggested fix:

Change the prune condition to check whether the bucket *would* be full after accounting for elapsed time since `last_seen`, rather than checking the stored `tokens` field directly. For example: `let would_be_full = (b.tokens + now.saturating_duration_since(b.last).as_secs_f64() * refill_per_sec) >= burst; !(would_be_full && now.saturating_duration_since(b.last_seen) >= idle_cutoff)`. Alternatively, document explicitly that the `tokens >= burst` check is intentionally unreachable for granted peers and that eviction relies solely on the hard-cap path.

[xilosada]

Good catch — real bug, fixed in 27c17bc. You're right that after the transactional change b.tokens is only written on Proceed, so the stored value is stale between grants and b.tokens >= burst essentially never holds for a granted peer (idle full peers only got reclaimed via the hard cap). The prune predicate now computes fullness from elapsed time since the last grant: b.tokens + elapsed_since(last) * refill_per_sec >= burst. A fully-recovered idle peer is reclaimed promptly; a partially-refilled (still-throttled) peer is retained. Added idle_prune_drops_recovered_peer_keeps_throttled_peer to lock this in, and extracted a shared refill_per_sec helper.

[meroreviewer]

🟡 New peer treated as full bucket even when rate-limited at Gate 2

When a brand-new peer is seen for the first time, cur_last is set to now (line 247: None => (burst, now)). This means elapsed = 0 and refilled = burst, so the peer always passes Gate 2 on first contact — which is the intended behaviour. However, if the peer is then rejected at Gate 3 (AtCapacity), it is NOT inserted into self.peers. On the very next call from the same peer (still within the same instant), cur_last is again now and elapsed is again 0, so the peer gets a fresh full bucket every time it is rejected at Gate 3. A peer that is perpetually rejected at Gate 3 never has its token count decremented, effectively bypassing the per-peer rate limit for as long as the global cap is saturated. This is a minor but real logic gap: a peer that is always refused at capacity never accumulates a rate-limit debt.

Suggested fix:

Track a 'seen but not yet admitted' state for new peers, or insert the peer bucket on first contact (with full tokens) regardless of Gate 3 outcome, so subsequent calls correctly compute elapsed time and token refill from the first-seen timestamp rather than always from `now`.

[xilosada]

Assessed — not a gap; keeping insert-on-Proceed. A peer that is only ever refused at Gate 3 (AtCapacity) triggers zero verifies — the global inflight cap refuses them — so there is nothing for the per-peer limit to meter during that window; the global cap is the control there. The moment the cap frees and the peer's announce actually Proceeds, it's inserted and subsequent rapid announces are rate-limited normally. So a perpetually-AtCapacity peer can't amplify anything.

Note the suggested fix (insert on first contact regardless of Gate 3 outcome) is the exact memory-growth the round-6 finding asked to remove — a flood of unique never-proceeding peers would fill peers up to the cap. Since it buys no protection (zero verives triggered), insert-on-Proceed is the right trade.

[meroreviewer]

💡 Durable dedup check uses ContextGroupId but throttle uses raw [u8; 32] — the two dedup keys are computed from the same bytes but via different types, risking silent divergence

The durable is_quote_hash_used check constructs group_id = ContextGroupId::from(namespace_id_bytes) and passes &group_id. The in-memory throttle's check(now, source, namespace_id_bytes, quote_hash) passes the raw namespace_id_bytes: [u8; 32] directly. Both ultimately key on the same 32 bytes, but the two dedup mechanisms use different type wrappers. If ContextGroupId ever adds a transformation (e.g. a prefix or hash) to its From<[u8;32]> impl, the durable check would use a different key than the throttle, silently breaking the dedup invariant. This is a latent maintenance hazard rather than a current bug.

Suggested fix:

Pass `group_id.as_bytes()` (or equivalent) to `throttle.check` instead of `namespace_id_bytes` directly, so both checks always derive their key from the same `ContextGroupId` value. Alternatively, add a comment noting that `ContextGroupId::from` is a no-op newtype wrap and must remain so for the two keys to stay in sync.

[xilosada]

Fixed in 27c17bc — the throttle is now keyed on group_id.to_bytes(), i.e. the same ContextGroupId value the durable is_quote_hash_used check uses, so the two dedup keys are derived from one source and can't silently diverge. I confirmed ContextGroupId::from([u8;32]) is a transparent newtype wrap today (Self(Identity(value)), to_bytes returns the raw bytes), so this is behavior-preserving and just removes the latent hazard you flagged.

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 3 agents | Quality score: 51% | Review time: 213.9s

🟡 8 warnings. See inline comments.

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-06bb7430}

[meroreviewer]

🟡 Same NaN hazard when refill is Duration::ZERO

In check_at, refill_per_sec is set to f64::INFINITY when self.refill.as_secs_f64() <= 0.0. Then elapsed * f64::INFINITY where elapsed == 0.0 (e.g. two calls at the same Instant) produces NaN. NaN.min(self.burst) is NaN, and NaN < 1.0 is false, so the rate-limit gate is silently bypassed. While Duration::ZERO is not a default, VerifyQuoteThrottle::new does not reject it (only max_inflight == 0 and burst < 1.0 are asserted), so a caller passing Duration::ZERO as refill would get a throttle that never rate-limits.

Suggested fix:

Either add `assert!(refill > Duration::ZERO, "refill must be positive")` in `new`, or guard the infinity path: `let refilled = if refill_per_sec.is_infinite() { self.burst } else { (bucket.tokens + elapsed * refill_per_sec).min(self.burst) };`.

[xilosada]

Fixed in deba1f4. You're right — refill_per_sec() returned f64::INFINITY for a zero refill interval, and 0.0 * INFINITY = NaN then silently bypassed the gate (NaN < 1.0 is false) and stored NaN - 1.0 into the bucket, permanently breaking it. I took the assert route: new() now asserts refill > Duration::ZERO (documented in the # Panics section), and refill_per_sec is simplified to a plain 1.0 / secs that is always finite. No in-tree caller passes zero, so this just turns a silent footgun into a construction-time contract.

[meroreviewer]

🟡 New peer treated as full bucket when now is used as cur_last

When a peer is not yet tracked, (cur_tokens, cur_last) is set to (burst, now). The elapsed time is then now.saturating_duration_since(now) == 0, so refilled = burst + 0 * refill_per_sec = burst. This is correct for the token count, but it means the refill clock for a brand-new peer is anchored to now (the current call). If the peer is then inserted in the commit block with last: now, the clock is consistent. However, if refill_per_sec is f64::INFINITY (zero-duration refill), 0.0 * f64::INFINITY is NaN, and NaN.min(burst) returns NaN in Rust (since f64::min propagates NaN). A NaN token count would then fail the refilled < 1.0 check (NaN comparisons are always false), causing the peer to always proceed — bypassing the rate limit entirely when per_peer_refill is Duration::ZERO.

Suggested fix:

Guard the infinity case explicitly: `let refilled = if refill_per_sec.is_infinite() { burst } else { (cur_tokens + elapsed * refill_per_sec).min(burst) };`. The same pattern applies in `prune`'s `would_be_full` calculation and in `VerifyQuoteThrottle::check_at`.

[xilosada]

Fixed in deba1f4. You're right — refill_per_sec() returned f64::INFINITY for a zero refill interval, and 0.0 * INFINITY = NaN then silently bypassed the gate (NaN < 1.0 is false) and stored NaN - 1.0 into the bucket, permanently breaking it. I took the assert route: new() now asserts refill > Duration::ZERO (documented in the # Panics section), and refill_per_sec is simplified to a plain 1.0 / secs that is always finite. No in-tree caller passes zero, so this just turns a silent footgun into a construction-time contract.

[meroreviewer]

🟡 New peer treated as full bucket even when now is used as cur_last, causing zero elapsed time

When a brand-new peer is not yet in self.peers, the code sets cur_last = now (line 232: None => (burst, now)). Then on line 234 it computes elapsed = now.saturating_duration_since(cur_last).as_secs_f64() which is always 0.0. The refilled value is therefore burst + 0 * refill_per_sec = burst, which is correct by coincidence — the intent is to give a new peer a full bucket. However, the comment says "A brand-new peer is treated as a full bucket" but the mechanism is fragile: if refill_per_sec were f64::INFINITY (zero-duration refill interval), the expression burst + 0 * INFINITY evaluates to NaN in IEEE 754 (0.0 * f64::INFINITY = NaN), and NaN < 1.0 is false, so the peer would proceed — but NaN - 1.0 would then be stored as NaN in bucket.tokens, permanently corrupting that peer's bucket. The refill_per_sec helper returns f64::INFINITY when per_peer_refill is zero. While the new() constructor does not assert per_peer_refill > 0, a caller passing Duration::ZERO would trigger this. Suggested fix: for a new peer, skip the elapsed calculation entirely and use burst directly as the starting token count, rather than computing cur_tokens + elapsed * refill_per_sec.

Suggested fix:

Replace `None => (burst, now)` with a direct early-proceed path for new peers, or guard the `0 * INFINITY` case: `let refilled = if elapsed == 0.0 { cur_tokens } else { (cur_tokens + elapsed * refill_per_sec).min(burst) };`

[xilosada]

Fixed in deba1f4. You're right — refill_per_sec() returned f64::INFINITY for a zero refill interval, and 0.0 * INFINITY = NaN then silently bypassed the gate (NaN < 1.0 is false) and stored NaN - 1.0 into the bucket, permanently breaking it. I took the assert route: new() now asserts refill > Duration::ZERO (documented in the # Panics section), and refill_per_sec is simplified to a plain 1.0 / secs that is always finite. No in-tree caller passes zero, so this just turns a silent footgun into a construction-time contract.

[meroreviewer]

🟡 Commit block overwrites tokens/last with stale values for an already-tracked peer

When a peer is already in self.peers, the commit block does self.peers.entry(source).or_insert(PeerBucket { tokens: refilled, last: now, last_seen: now }). Because the entry already exists, or_insert is a no-op and the PeerBucket literal is never used. The subsequent bucket.tokens = refilled - 1.0 and bucket.last = now lines then correctly update the existing entry. This is functionally correct but the or_insert argument constructs a PeerBucket with tokens: refilled (pre-deduction) that is immediately discarded — the dead construction is misleading and could mask a future bug if the logic is refactored. Consider entry(source).or_insert_with(|| PeerBucket { tokens: burst, last: now, last_seen: now }) to make the insert-only-for-new-peers intent explicit, and document that the three lines below always overwrite.

Suggested fix:

Use `or_insert_with(|| PeerBucket { tokens: burst, last: now, last_seen: now })` (inserting a full bucket for a genuinely new peer) and rely on the three unconditional assignments below to set the correct post-grant state for both new and existing peers. Add a comment that the assignments always run.

[xilosada]

Fixed in deba1f4 — switched to or_insert_with(|| PeerBucket { tokens: burst, .. }) so the placeholder is constructed only for a genuinely new peer, and added a comment that the three assignments below always run (setting the real post-grant state for both new and existing peers). Removes the misleading dead tokens: refilled construction for the existing-peer case.

[meroreviewer]

🟡 New-peer token calculation uses now as cur_last, producing elapsed = 0 and refilled = burst + 0 * refill_per_sec; when refill_per_sec is f64::INFINITY this yields NaN

When per_peer_refill is Duration::ZERO, refill_per_sec() returns f64::INFINITY. For a brand-new peer, cur_last = now, so elapsed = 0.0. The expression cur_tokens + elapsed * refill_per_sec becomes burst + 0.0 * f64::INFINITY = burst + NaN = NaN. NaN < 1.0 is false, so Gate 2 passes. Then bucket.tokens = NaN - 1.0 = NaN is stored. Every subsequent check for this peer computes NaN + elapsed * refill_per_sec = NaN, so refilled = NaN, NaN < 1.0 = false, and the peer is never rate-limited again — the per-peer rate limit is permanently broken for that peer. The constructor does not assert per_peer_refill > Duration::ZERO, so this is reachable. Fix: assert per_peer_refill > Duration::ZERO in new(), or handle the new-peer case by directly assigning burst without the elapsed calculation.

Suggested fix:

In `new()`, add: `assert!(per_peer_refill > Duration::ZERO || per_peer_burst == f64::INFINITY, "zero refill with finite burst makes rate limiting unreachable");` — or more simply, handle the new-peer path as `None => { /* skip elapsed calc */ let refilled = burst; ... }` to avoid the `0 * INFINITY` expression entirely.

[xilosada]

Fixed in deba1f4. You're right — refill_per_sec() returned f64::INFINITY for a zero refill interval, and 0.0 * INFINITY = NaN then silently bypassed the gate (NaN < 1.0 is false) and stored NaN - 1.0 into the bucket, permanently breaking it. I took the assert route: new() now asserts refill > Duration::ZERO (documented in the # Panics section), and refill_per_sec is simplified to a plain 1.0 / secs that is always finite. No in-tree caller passes zero, so this just turns a silent footgun into a construction-time contract.

[meroreviewer]

🟡 Idle-prune cutoff is too short for the default configuration, evicting peers that are still throttled

idle_cutoff is per_peer_refill.saturating_mul(2). With the default DEFAULT_PER_PEER_REFILL = 2s and DEFAULT_PER_PEER_BURST = 5.0, a peer that spent all 5 burst tokens needs 5 * 2s = 10s to fully refill. The idle_cutoff of 4s means a depleted peer that goes quiet for 4 seconds is evicted and handed a fresh full bucket — effectively resetting its rate limit after only 4s instead of 10s. The comment says "A peer whose bucket would have refilled to full" but the would_be_full check correctly guards against this; the bug is that idle_cutoff is too short relative to the full-refill time, so a peer can be evicted while still throttled if it happens to be quiet for idle_cutoff seconds.

Suggested fix:

Set `idle_cutoff` to at least `per_peer_refill * burst.ceil() as u32` (the time to fully refill from empty), or use a fixed multiple like `per_peer_refill.saturating_mul(burst.ceil() as u32 + 1)`. Alternatively, document explicitly that the cutoff is intentionally shorter than full-refill time and that the `would_be_full` guard is the real protection.

[xilosada]

Assessed — no change; the would_be_full guard (not the cutoff) is the protection, as your own note acknowledges ("the would_be_full check does guard against this"). A peer is pruned only when its bucket would have fully refilled AND it's been idle past the cutoff. A depleted abuser at default burst 5 / refill 0.5·s⁻¹ needs ~10s to refill, so at a 4s idle point would_be_full is false (~2 tokens) and it is retained — there is no 4s reset, so the "75 verifies/min" figure doesn't occur. Only fully-recovered peers are dropped, and those are byte-identical to a fresh peer, so the cutoff can't grant a bypass.

Lengthening the cutoff to the full-refill time would make would_be_full redundant and would hold fresh-equivalent buckets longer (more memory, bounded by MAX_TRACKED_PEERS anyway) for zero protective gain. The short cutoff is a deliberate memory-reclamation responsiveness knob; correctness rests on would_be_full. The idle_prune_drops_recovered_peer_keeps_throttled_peer test pins exactly this (recovered peer pruned, still-throttled peer retained, same idle duration).

[meroreviewer]

🟡 New peer treated as full bucket even when rate-limited at Gate 2

When a brand-new peer is seen, (cur_tokens, cur_last) is set to (burst, now), giving elapsed = 0 and refilled = burst. This is intentional for the first legitimate contact. However, if the peer is then rejected at Gate 3 (AtCapacity), it is NOT inserted into peers. On the very next call from the same peer (still not in peers), it again gets a fresh full bucket — so a peer that is repeatedly rejected at Gate 3 never accumulates any rate-limit debt. This means the per-peer rate limit is only effective after at least one Proceed, which is the documented design, but it also means a peer that is always rejected at Gate 3 (e.g. during a sustained capacity-saturation attack) can never be rate-limited. The comment at line 237 acknowledges this ('a peer is only inserted on the commit (Proceed) path below, so a flood of unique source peers that never proceed can't grow peers'), but the flip side — that a single persistent peer also never accumulates debt until it proceeds — is not acknowledged. This is a minor but real gap: a single peer that keeps hammering while the semaphore is saturated by other peers will get a fresh full bucket every time capacity frees up.

Suggested fix:

Consider tracking a lightweight 'seen but not yet proceeded' counter or timestamp for peers that hit Gate 3, separate from the full PeerBucket, so repeated AtCapacity hits from the same peer can be rate-limited. Alternatively, document this as an accepted trade-off in the module-level doc.

[xilosada]

Documented as an accepted trade-off in the module header in deba1f4. To restate: a peer always refused at the global inflight cap accrues no per-peer debt, but that's harmless by construction — it triggers zero verifies (the global cap is the limiter there), and the moment one of its announces proceeds it's tracked and metered normally. The suggested 'seen-but-not-proceeded' counter is exactly the unique-peer map growth the round-6 finding asked to remove, for no protective gain (no verifies are happening). Keeping insert-on-Proceed.

[meroreviewer]

🟡 Idle-cutoff for peer pruning is only 2× refill interval — too aggressive for default config

The idle cutoff is per_peer_refill.saturating_mul(2), which at the default of Duration::from_secs(2) gives a 4-second idle window. A peer that sends one burst of 5 announces and then goes quiet for 4 seconds will be pruned and get a fresh full bucket on its next announce. With a 5-token burst and a 4-second reset window, a peer can sustain ~75 verifies/minute (5 tokens every 4s × 60/4) rather than the intended ~30/min. The comment says 'indistinguishable from a fresh peer' but that is only true if the bucket would be fully refilled — at 1 token/2s, 4 seconds only refills 2 tokens, not 5. The would_be_full check does guard against this (a 2-token-refilled peer is not full), but the idle cutoff is still very short relative to the burst size.

Suggested fix:

Use a larger idle cutoff, e.g. `per_peer_refill.saturating_mul(burst as u32 + 1)` so a peer must be idle long enough for its bucket to actually refill to full before it is pruned. Or set `idle_cutoff` to a fixed minimum like `Duration::from_secs(60)`.

[xilosada]

Assessed — no change; the would_be_full guard (not the cutoff) is the protection, as your own note acknowledges ("the would_be_full check does guard against this"). A peer is pruned only when its bucket would have fully refilled AND it's been idle past the cutoff. A depleted abuser at default burst 5 / refill 0.5·s⁻¹ needs ~10s to refill, so at a 4s idle point would_be_full is false (~2 tokens) and it is retained — there is no 4s reset, so the "75 verifies/min" figure doesn't occur. Only fully-recovered peers are dropped, and those are byte-identical to a fresh peer, so the cutoff can't grant a bypass.

Lengthening the cutoff to the full-refill time would make would_be_full redundant and would hold fresh-equivalent buckets longer (more memory, bounded by MAX_TRACKED_PEERS anyway) for zero protective gain. The short cutoff is a deliberate memory-reclamation responsiveness knob; correctness rests on would_be_full. The idle_prune_drops_recovered_peer_keeps_throttled_peer test pins exactly this (recovered peer pruned, still-throttled peer retained, same idle duration).

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 3 agents | Quality score: 63% | Review time: 217.2s

🟡 6 warnings. See inline comments.

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-adad28cc}

[meroreviewer]

🟡 Dedup entry inserted before verify succeeds — a failed verify still blocks re-verification for the full TTL

On Decision::Proceed, the (group_id, quote_hash) key is inserted into recent_quotes at line 344 (let _ = self.recent_quotes.insert(key, now)). This happens synchronously before the spawned verify task runs. If the verify subsequently fails (e.g. PCS fetch error, invalid quote), the dedup entry remains for the full DEFAULT_DEDUP_TTL (300 s). A legitimate re-announce of the same quote (e.g. after a transient PCS outage) will be dropped as Duplicate for up to 5 minutes. The PR description says "transient-failure retries use a fresh nonce/quote (new quote_hash)" — but the quote bytes (and therefore the hash) are the same across retries; only the announce nonce changes. If the quote hash is the same, the retry is silently dropped. This is a correctness issue for the liveness guarantee the PR claims to preserve.

Suggested fix:

Only insert into `recent_quotes` after a successful verify, or use a shorter TTL for failed verifies. One approach: pass a callback/channel back from the spawned task that removes the dedup entry on verify failure, allowing a retry. Alternatively, document explicitly that a failed verify blocks re-announcement for the full TTL and that operators must wait or use a new quote.

[xilosada]

Real bug — fixed in 007d5a9. I verified your claim by tracing the sender: the fleet-join re-announce loop (fleet_join.rs) generates the nonce and quote once (the nonce is baked into the quote's report_data) and re-publishes the identical cloned payload each cycle, so Sha256(quote_bytes) is indeed stable across retries — your read is correct, and the PR description's 'fresh nonce/quote per retry' was wrong.

Worse than the description implied: MAX_ADMISSION_WAIT is 30s while the dedup TTL is 300s, so a single transient verify error would suppress every re-announce for the whole admission window and sink the join.

Fix: the spawned verify now chains a .map that runs back on the actor thread and, on Err (a transient infra failure — e.g. the Intel-PCS fetch errored), calls a new TeeAdmissionThrottle::forget_quote to drop the dedup entry so the next re-announce is re-verified. Crucially the handler returns Ok(()) for a definite invalid result (bad signature/nonce) — those are not forgotten, so replay-DoS suppression is preserved — and the per-peer rate limit + inflight cap bound re-verification either way. Added forget_quote_allows_reverify_after_transient_failure.

[meroreviewer]

🟡 evict_oldest_entries may remove wrong entries when entries slice is shorter than map

In evict_oldest, entries is built from map.iter() and then passed to evict_oldest_entries. The remove = entries.len().saturating_sub(keep) calculation is correct when entries is a full snapshot of map. However, evict_oldest_entries is a generic function that takes entries: &mut [(K, Instant)] — if a caller ever passes a partial slice (e.g. a pre-filtered subset), remove would be computed from the slice length, not the map length, and the map would not be reduced to keep entries. Currently both callers (evict_oldest and evict_oldest_peers) pass a full snapshot, so this is not a current bug. But the API is misleading: the keep parameter is semantically "keep this many entries in map", yet the function computes how many to remove from the slice, not from the map. If entries.len() < map.len() (possible if the map was mutated between snapshot and call), fewer entries than intended are removed and the hard cap is not enforced.

Suggested fix:

Assert or document that `entries.len() == map.len()` is a precondition, or compute `remove` as `map.len().saturating_sub(keep)` instead of `entries.len().saturating_sub(keep)` to make the cap enforcement robust against snapshot/map divergence.

[xilosada]

Fixed in 007d5a9 — evict_oldest_entries now derives remove from map.len() (the cap's source of truth), clamped to the snapshot length: map.len().saturating_sub(keep).min(entries.len()). The hard cap is enforced even if the snapshot ever diverged from the map, and the select_nth index stays in bounds.

[meroreviewer]

🟡 Dedup entry inserted with now of the check, not the time the verify completes — replay window starts too early

The recent_quotes entry is inserted with now (the time check() is called) as the timestamp, and the dedup TTL is checked as now.saturating_duration_since(*seen) < self.dedup_ttl. This means the TTL countdown starts when the verify is admitted, not when it completes. If a verify takes, say, 10 seconds (outbound PCS fetch), the effective dedup window is dedup_ttl - 10s. For the default 300s TTL this is negligible, but it is a minor logical inconsistency: the dedup is meant to prevent re-verifying the same quote, yet the window starts before the verify result is known. More importantly, if a verify fails (e.g. PCS fetch error), the quote hash is still in recent_quotes and will be deduped for the full TTL, preventing a legitimate retry of a transiently-failed verify. The durable is_quote_hash_used check only covers admitted quotes, so a failed-but-deduped quote is stuck until the TTL expires.

Suggested fix:

Consider inserting the dedup entry only after a successful verify (pass a callback or channel back from the spawned task), or document that failed verifies are intentionally suppressed for the TTL window and rely on the TTL expiry for recovery.

[xilosada]

Real bug — fixed in 007d5a9. I verified your claim by tracing the sender: the fleet-join re-announce loop (fleet_join.rs) generates the nonce and quote once (the nonce is baked into the quote's report_data) and re-publishes the identical cloned payload each cycle, so Sha256(quote_bytes) is indeed stable across retries — your read is correct, and the PR description's 'fresh nonce/quote per retry' was wrong.

Worse than the description implied: MAX_ADMISSION_WAIT is 30s while the dedup TTL is 300s, so a single transient verify error would suppress every re-announce for the whole admission window and sink the join.

Fix: the spawned verify now chains a .map that runs back on the actor thread and, on Err (a transient infra failure — e.g. the Intel-PCS fetch errored), calls a new TeeAdmissionThrottle::forget_quote to drop the dedup entry so the next re-announce is re-verified. Crucially the handler returns Ok(()) for a definite invalid result (bad signature/nonce) — those are not forgotten, so replay-DoS suppression is preserved — and the per-peer rate limit + inflight cap bound re-verification either way. Added forget_quote_allows_reverify_after_transient_failure.

[meroreviewer]

🟡 New peer treated as full bucket at now, not at epoch zero — first-contact elapsed is always 0

When a brand-new peer is first seen, the code synthesises (burst, now) as the initial (cur_tokens, cur_last). The elapsed time is then now.saturating_duration_since(now) == 0, so refilled = burst + 0 * rate = burst, which is correct for the first call. However, the comment says 'first contact is never rate-limited', which is the intended behaviour. The real subtle issue is that a peer that was previously evicted from the map (pruned as idle) is indistinguishable from a brand-new peer and gets a fresh full bucket — this is acknowledged in the module docs. This is a design trade-off, not a bug, but it means a peer can reset its rate-limit state by staying quiet for idle_cutoff (2× refill interval = 4 s at defaults), then flooding again. At the default burst of 5 and refill of 2 s, an adversary can get 5 free verifies every 4 s (75/min) rather than the intended ~30/min. Consider using a longer idle_cutoff (e.g. dedup_ttl) or a separate eviction TTL to close this reset window.

Suggested fix:

Set `idle_cutoff` to `self.dedup_ttl` (300 s) instead of `self.per_peer_refill.saturating_mul(2)` (4 s at defaults). This ensures a peer cannot reset its rate-limit state faster than the dedup window, aligning the two defences.

[xilosada]

Adjusted in 007d5a9, along your suggested lines. The idle predicate is now simply: evict a peer once it hasn't been seen for the full bucket-refill time, burst * per_peer_refill (= 10s at defaults, not 4s). That's exactly the window after which a fresh full bucket grants no more than the configured rate, so the 'reset to a fresh bucket' you describe can't exceed the intended ~30/min. A peer flooding faster than that is seen more recently than the window and so is retained. Because last_seen >= last always, a peer idle that long has provably refilled, so the separate would_be_full check is gone — simpler and with the cutoff aligned to the refill time. (I used the refill time rather than dedup_ttl=300s to avoid holding fresh-equivalent buckets for 5 min; happy to lengthen if you'd prefer aligning with the dedup window.)

[meroreviewer]

🟡 Mutex held across try_acquire_owned — lock contention under concurrent requests

The Mutex<Bucket> is held from the start of check_at through the try_acquire_owned call on the Arc<Semaphore>. While try_acquire_owned is non-blocking, holding the mutex across it means every concurrent HTTP handler thread serializes through this single lock for the entire duration of the token-check + semaphore-acquire. Under the default DEFAULT_MAX_INFLIGHT = 4 and DEFAULT_BURST = 10, this is a global serialization point for all /verify-quote requests. The comment justifies this as preventing a TOCTOU window, but the TOCTOU concern only applies if two threads could both see refilled >= 1.0 and both try to commit — which is already prevented by the mutex itself. The lock could be released after the token check and re-acquired only for the commit, with the semaphore acquired in between. As written, the lock is held longer than necessary, increasing tail latency under load.

Suggested fix:

Release the lock after computing `refilled` and checking `>= 1.0`, acquire the semaphore permit outside the lock, then re-acquire the lock only to commit `bucket.tokens` and `bucket.last`. Use a compare-and-swap style: if the semaphore acquire fails, no commit is needed. This eliminates the serialization of semaphore acquisition under the mutex.

[xilosada]

Assessed — keeping the lock across the (non-blocking) try_acquire_owned; the proposed release-and-reacquire would be a correctness regression. The TOCTOU isn't 'already prevented by the mutex' once you drop it: if thread A releases after reading refilled=5, then B also reads 5 before A re-acquires to commit, both pass >= 1.0, both take a permit, and both commit tokens = 5 - 1 = 4 — two tokens consumed but the bucket reads 4, an over-grant. Holding the lock across the permit acquire is what makes the token+permit commit atomic. try_acquire_owned is non-blocking (a few instructions, never across an await), so the critical section is tiny; with DEFAULT_MAX_INFLIGHT=4 the realistic contention is negligible versus the verify path this guards.

[meroreviewer]

🟡 Prune runs BEFORE the dedup/rate gates on every call when over_cap is true, making the per-call cost O(N) under a cap-saturating flood

The prune guard fires immediately when over_cap is true (either map exceeds its hard cap), regardless of PRUNE_INTERVAL. Under an adversarial flood that keeps recent_quotes.len() > MAX_TRACKED_QUOTES, every single check() call will run the O(N) retain + evict_oldest pass before the cheap dedup gate can reject the announce. The comment in the code acknowledges the amortisation intent but the over_cap || due short-circuit means the size-cap path bypasses the interval guard entirely. Since entries are only inserted on Proceed (gated by rate-limit + inflight cap), reaching MAX_TRACKED_QUOTES requires 8192 distinct admitted quotes within the TTL — which at the default 4 inflight × 5 burst is hard but not impossible over 300 s. If reached, the O(N) prune on every call is the exact amplification the throttle is meant to prevent.

Suggested fix:

Track a `last_over_cap_prune: Option<Instant>` and apply a shorter (e.g. 100 ms) minimum interval even for the over-cap path, so the prune is still bounded to ~10/s rather than once per announce.

[xilosada]

Assessed — no change; the over-cap path can't run O(N) on every call. evict_oldest reduces recent_quotes back to exactly MAX_TRACKED_QUOTES, so immediately after a sweep len > MAX is false and the next call's over_cap guard is false — no prune. The map only climbs back over the cap via an insert, and inserts happen only on Proceed (gated by the per-peer rate limit and the inflight cap of 4). So the O(N) sweep is bounded by the Proceed rate, not the announce rate — a rejected flood inserts nothing and triggers no over-cap prune. Adding a min-interval to the over-cap path would instead let the map exceed its hard memory bound between sweeps, which is the property the immediate guard exists to keep.

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 3 agents | Quality score: 74% | Review time: 912.5s

🟡 2 warnings, 💡 1 suggestions. See inline comments.

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-d660a247}

[meroreviewer]

🟡 Dedup entry inserted before inflight permit is actually held by the spawned task

In check, the dedup entry (group_id, quote_hash) → now is inserted into recent_quotes at the end of the commit block (line ~302), before Decision::Proceed(permit) is returned to the caller. The caller then moves the permit into the spawned async task. Between the check return and the task actually starting, the permit is held by the caller's stack frame. If the actor thread panics or the task is dropped before it starts (e.g. the actor is stopping), the permit is dropped but the dedup entry remains — the quote is suppressed for the full TTL even though no verify ever ran. The forget_quote call in the .map closure only fires on Err, not on task cancellation. This is a narrow window (actor shutdown during an in-flight announce), but it means a legitimate re-announce after a node restart could be suppressed until the TTL expires. The per-peer rate limit and inflight cap still apply on retry, so this is not a DoS vector, but it can delay legitimate admission.

Suggested fix:

Document this as a known limitation: if the spawned task is dropped before completion (actor shutdown), the dedup entry is not cleared. Alternatively, call `forget_quote` from a `Drop` guard on the permit wrapper so cancellation also clears the entry.

[xilosada]

Assessed — added a clarifying note in 852da58; no behavioral change needed. The key fact is that recent_quotes is in-memory state on the NodeManager actor, not durable. The only way the spawned future is dropped before completion is the actor stopping — and that tears down the throttle (and its dedup map) along with it. So there is no stale entry that outlives its map: a node that restarts comes up with an empty map and re-verifies freely. Within a running actor, ctx.spawn futures run to completion (dropping the ignored SpawnHandle does not cancel them), so the .map always fires. A Drop-guard would therefore add machinery for a window that can't actually leave observable state behind.

[meroreviewer]

🟡 Mutex held across try_acquire_owned — correct but fragile; comment justification is incomplete

The comment at line 107 correctly explains why the Mutex is held across try_acquire_owned: to make the token-decrement and permit-acquire atomic, avoiding a TOCTOU window. However, try_acquire_owned clones the Arc<Semaphore> (line 109: Arc::clone(&self.inflight).try_acquire_owned()), which itself acquires an internal lock inside tokio::sync::Semaphore. Holding the outer Mutex while acquiring the semaphore's internal lock creates a lock-ordering dependency: always bucket_mutex → semaphore_internal. Since try_acquire_owned never blocks and the semaphore is never acquired elsewhere while holding the bucket mutex (the only other semaphore use is dropping the permit in the spawned task, which never holds the bucket mutex), there is no deadlock risk in the current code. This is correct but should be noted explicitly. The current comment does not mention the lock-ordering invariant, which makes it harder to verify safety during future refactors.

Suggested fix:

Add a comment: `// Lock ordering: bucket_mutex is always acquired before the semaphore's internal lock (via try_acquire_owned). The permit is only dropped in spawned tasks that never hold bucket_mutex, so this ordering is never inverted.`

[xilosada]

Done in 852da58 — added the lock-ordering note you suggested: this is the only place the bucket mutex and the semaphore's internal lock are held together, always in the order bucket-mutex → semaphore (via try_acquire_owned); the permit is released only by dropping it in the handler / spawned verify, which never holds the bucket mutex, so the ordering is never inverted. Agreed the code was correct — this just makes it verifiable on a future refactor.

[meroreviewer]

💡 forget_quote is called on verify error but the distinction between error and invalid-quote depends on handle_tee_attestation_announce returning Err only for transient failures

The forget_quote call in the .map closure (line ~249 in the diff, line ~195 in the full file) is predicated on result being Err. The comment says 'an invalid quote instead returns Ok above', implying handle_tee_attestation_announce returns Ok for a definitively-invalid quote and Err only for transient infrastructure failures. If that contract is ever violated — e.g. a future refactor makes the function return Err for a bad quote — the dedup entry will be cleared and the invalid quote can be re-verified, defeating the dedup protection. This is a fragile cross-module contract. Consider documenting the expected error semantics of handle_tee_attestation_announce explicitly (e.g. with a doc comment on that function) so the invariant is visible at the definition site.

Suggested fix:

Add a doc comment to `handle_tee_attestation_announce` stating: 'Returns `Ok(())` for both successful admission and definitively-invalid quotes (quote rejected by policy); returns `Err` only for transient infrastructure failures (e.g. PCS fetch error). Callers that clear dedup state on `Err` rely on this distinction.'

[xilosada]

Done in 852da58 — documented the error contract directly on handle_tee_attestation_announce: Ok(()) for a successful admission or a definitively rejected quote (verification ran, result invalid); Err only for a transient failure where the verify couldn't complete (PCS fetch error, store/governance publish). The doc explicitly calls out that the network_event::specialized caller clears the dedup entry on Err and keeps it on Ok, and warns against returning Err for an invalid-but-evaluated quote without updating that caller. Good call making the cross-module invariant visible at the definition.

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 3 agents | Quality score: 73% | Review time: 861.1s

🟡 3 warnings, 💡 1 suggestions. See inline comments.

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-a5f60b79}

[meroreviewer]

🟡 evict_oldest_entries may remove wrong entries when entries is a partial snapshot

The doc comment states remove is clamped to entries.len() for robustness when entries is a partial view of map. However, remove is computed as map.len().saturating_sub(keep) — the number of entries to remove from map — but then select_nth_unstable_by_key(remove - 1, ...) partitions entries (the snapshot) so that the first remove elements are the oldest. If entries.len() < map.len() (partial snapshot), the remove count is derived from map.len() but applied to entries, so the loop for (k, _) in &entries[..remove] removes remove keys from map — but those keys are the oldest in the snapshot, not necessarily the oldest in the full map. The remaining map.len() - entries.len() unsnapshotted entries are never considered for eviction. In practice entries is always a full snapshot (both callers collect all entries), so this is not a live bug, but the doc comment's claim of robustness for partial snapshots is misleading — a partial snapshot would silently under-evict.

Suggested fix:

Either remove the claim about partial-snapshot robustness from the doc comment, or assert `entries.len() == map.len()` at the top of the function to make the assumption explicit.

[xilosada]

Fixed in 1fc907b — you're right, my earlier doc overclaimed. The .min(entries.len()) clamp didn't actually make a partial snapshot correct (it would under-evict / pick snapshot-locals). Both callers always pass a full snapshot, so I dropped the misleading 'robust for partial snapshots' note, made the full-snapshot requirement explicit in the doc, and added a debug_assert_eq!(entries.len(), map.len()) so any future partial-snapshot caller trips in tests. remove is derived from map.len() and is in-bounds given the (asserted) full snapshot.

[meroreviewer]

🟡 Token bucket last timestamp set at construction, not at first request

The Bucket is initialized with last: Instant::now() at construction time (inside VerifyQuoteThrottle::new). If the LazyLock is initialized well before the first request arrives, the elapsed time since construction is credited as refill tokens on the first call to check_at. With DEFAULT_BURST = 10 and DEFAULT_REFILL = 2s, any idle period longer than 20 seconds means the bucket is already full — which is the intended steady-state — but if the server is idle for, say, hours, the elapsed * refill_per_sec term is large and then clamped to burst, so the effective behavior is correct. However, the real risk is the opposite: if LazyLock initialization is triggered by the first request (which it is, since LazyLock::new is lazy), Instant::now() inside new and the now passed to check_at are nearly identical, so elapsed ≈ 0 and the bucket starts full — this is actually fine. The subtle bug is that last is set to Instant::now() inside new, not to the now passed into check_at. If two threads race to call check_at with the same now value (e.g. in tests using a fixed now), the elapsed = now.saturating_duration_since(bucket.last) can underflow to zero if bucket.last (set during new) is after the test's now. This is observable in the test bursts_then_rate_limits where now = Instant::now() is captured after VerifyQuoteThrottle::new runs — if new's internal Instant::now() is slightly later than the test's captured now, saturating_duration_since returns zero, the bucket appears to have burst tokens (correct by coincidence), but the refill clock is anchored to a future point. In production this is harmless, but it makes the throttle's behavior subtly dependent on wall-clock ordering between construction and first use.

Suggested fix:

Initialize `last` to a sentinel that forces the first `check_at` to treat the bucket as full regardless of timing: use `Instant::now() - refill * burst.ceil() as u32` (i.e. far enough in the past that the bucket is guaranteed full), or accept `now` as a parameter to `new` and initialize `last: now - refill * burst_ceil`. Alternatively, initialize `tokens: burst` and `last: Instant::now()` but clamp elapsed to zero when `now < bucket.last` (which `saturating_duration_since` already does) — the current code is actually safe in production; the suggestion is to make it robust in tests too by passing `now` into `new`.

[xilosada]

Assessed — harmless as you note; added a clarifying comment in 1fc907b, no behavior change. The bucket starts full (tokens: burst) anchored at construction, and check_at uses saturating_duration_since, so a now captured slightly before the constructor's Instant::now() (the test case) yields 0 elapsed — the bucket simply stays full, never a panic or under-credit. On the next call with a later now, refill resumes and is clamped to burst. I'd rather not thread now into new() (it'd complicate the constructor for a production-irrelevant timing nuance the saturating subtraction already covers).

[meroreviewer]

🟡 Prune evicts by insertion time, not expiry proximity — may evict still-valid entries while leaving near-expired ones

evict_oldest sorts by the Instant stored in recent_quotes, which is the time the entry was inserted (i.e. when the quote was first seen). Entries inserted earlier are evicted first. However, an entry inserted long ago may still have a long remaining TTL if dedup_ttl is large, while a more-recently inserted entry could be closer to expiry. The eviction policy is therefore not strictly LRU-by-remaining-TTL. Under the hard cap this means a quote that was admitted early in the TTL window is evicted before one admitted just before the cap was hit — the opposite of what a TTL-aware eviction would do. The practical impact is limited (the cap is only hit under adversarial churn, and the rate-limit + inflight gates are the real backstop), but the comment says "evicting the oldest non-expired entries" which is misleading — it evicts by insertion order, not by remaining TTL.

Suggested fix:

Either update the comment to say "evicts by insertion time (oldest first)" rather than implying TTL-awareness, or change the eviction key to `dedup_ttl - elapsed` (remaining TTL) if true TTL-aware eviction is desired. Given the stated trade-off (memory bound is the hard guarantee), updating the comment is sufficient.

[xilosada]

Clarified the comment in 1fc907b. The eviction key is the insert time, and because the dedup TTL is uniform, insert order is exactly expiry order — the oldest-inserted entries are the nearest to expiry, so insertion-time eviction is TTL-aware here (expiry = insert + constant TTL). The comment now says 'oldest by insertion time (= nearest to expiry, since the TTL is uniform)'. No behavior change; a remaining-TTL key would order identically.

[meroreviewer]

💡 Durable dedup check runs before throttle, consuming a store read on every announce

The is_quote_hash_used store read (a RocksDB lookup) is performed before the in-memory throttle gates. This means a flood of announces with distinct quote hashes (each a new SHA-256 value) will each trigger a store read before hitting the per-peer rate limit or global inflight cap. The store read is cheaper than a full verify, but it is still an I/O operation that can be driven by an unauthenticated gossip peer. The in-memory throttle (Gate 1 dedup, Gate 2 rate limit, Gate 3 inflight cap) should ideally run first to reject cheap-to-compute cases before touching the store.

Suggested fix:

Reorder: run `tee_admission_throttle.check(...)` first (all three in-memory gates), and only on `Decision::Proceed` perform the `is_quote_hash_used` store read as a final durable dedup before spawning the verify task. This keeps the store read on the hot path only for announces that have already passed the cheap in-memory gates.

[xilosada]

Done in 1fc907b — reordered so the in-memory throttle (dedup + per-peer rate limit + inflight cap) runs before the is_quote_hash_used store read. Now an unauthenticated announce flood is rejected on cheap in-memory state and only a throttle-Proceed reaches the RocksDB lookup. On a durable hit we drop there; the held permit is released as it goes out of scope, and the dedup entry the throttle just recorded keeps suppressing re-announces — so the reorder costs at most one per-peer token for an already-admitted re-announce (rate-limited anyway, and rare since fleet-join stops re-announcing once admitted).

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 3 agents | Quality score: 69% | Review time: 363.2s

🟡 4 warnings, 💡 1 suggestions. See inline comments.

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-34e73395}

[meroreviewer]

🟡 Dedup entry inserted before durable store check; a store-admitted quote consumes a per-peer token

In check, the dedup entry (recent_quotes.insert(key, now)) and the per-peer token deduction (bucket.tokens = refilled - 1.0) are committed at the end of check (the Proceed path), but the durable is_quote_hash_used store read happens after check returns in specialized.rs. When the store returns Ok(true) (quote already admitted), the function returns early and drops verify_permit, but the dedup entry and the consumed token are already committed. This means a legitimate re-announce of an already-admitted quote (e.g. after a node restart where the in-memory map is empty) burns a per-peer token and records a dedup entry that suppresses the next re-announce for the full TTL. The comment in specialized.rs acknowledges the dedup entry is kept on a store-hit, but does not acknowledge the token cost. For a fleet that re-announces on every restart this could cause spurious rate-limiting. Consider moving the store check before check(), or returning a distinct Decision::AlreadyAdmitted that skips token/dedup recording.

Suggested fix:

Move `calimero_governance_store::is_quote_hash_used` before `this.tee_admission_throttle.check(...)` in `specialized.rs`, or add a `Decision::AlreadyAdmitted` variant that the throttle emits when the quote_hash is in `recent_quotes` but the store confirms it is admitted (so no token is burned).

[xilosada]

Assessed — documented the trade-off in 60057c6; keeping the throttle-before-store order. This is the flip side of the reorder you (correctly) asked for last round: an already-admitted quote that is re-announced burns one per-peer token before the store read reveals it's admitted. It's bounded (the dedup entry the throttle just recorded suppresses the rest of the burst — subsequent re-announces hit Duplicate, no token, no store read) and rare (a node stops announcing once admitted; fleet-join breaks on admission). Your own numbers bear this out: 5 re-announces = 10s of budget inside a 30s window with 1 token/2s refill, so no real admission risk.

I deliberately did not take the forget_quote-on-admitted route: dropping the entry would re-open a store read on every following re-announce (defeating the reorder), and restoring the token has the same effect. Keeping the entry is what makes the follow-ups cheap. A dedicated AlreadyAdmitted decision variant would need the throttle to perform the store read itself, coupling it to the datastore — net worse. The one-token cost on a rare path is the right price.

[meroreviewer]

🟡 evict_oldest_entries removes entries by snapshot index, not by map key — can silently under-evict if the snapshot is stale

Both evict_oldest and evict_oldest_peers collect a full snapshot of the map immediately before calling evict_oldest_entries, and the debug_assert_eq checks entries.len() == map.len(). However, evict_oldest_entries calls map.remove(k) for the first remove entries in the snapshot (sorted by select_nth_unstable). If any key in entries[..remove] is not present in map (e.g. because prune's retain pass already removed it between the snapshot and the eviction), map.remove(k) silently returns None and the map ends up with more than keep entries — the hard cap is violated. In the current code both calls happen inside prune with no interleaving, so this cannot happen today. But the debug_assert_eq only fires in debug builds; a future refactor that calls evict_oldest_entries after any map mutation between snapshot and eviction would silently break the cap in release builds. The fix is to either (a) assert the remove count matches after the loop, or (b) merge the retain + eviction into a single pass.

Suggested fix:

After the `for (k, _) in &entries[..remove]` loop, add `debug_assert!(map.len() <= keep, "evict_oldest_entries: map still over cap after eviction");` to catch stale-snapshot bugs in debug builds.

[xilosada]

Added in 60057c6 — a post-loop debug_assert!(map.len() <= keep, ...) right after the removals, exactly as suggested, so a future caller that passes a stale snapshot (a key in entries[..remove] already gone from map, so remove deletions don't bring it under the cap) trips in debug/test builds. Combined with the entry debug_assert_eq!(entries.len(), map.len()), both the precondition and the post-condition are now guarded. In the live code both calls are inside prune with a fresh snapshot and no interleaving, so neither fires today.

[meroreviewer]

🟡 New-peer cur_last initialised to now, making elapsed always 0 for first call

For a brand-new peer (not yet in self.peers) the code sets cur_last = now (line ~196). The elapsed time is then now.saturating_duration_since(now) = 0, so refilled = cur_tokens + 0 * rate = burst. This is intentional for the first-contact full-bucket behaviour, but it also means that if a new peer's first announce arrives at a time far in the future relative to the throttle's construction, the elapsed-since-construction time is silently discarded. More importantly, the comment says "a brand-new peer is treated as a full bucket" — but the cur_last = now initialisation means the refill clock for that peer starts at now, not at the time the throttle was constructed. This is correct for the first grant, but the comment in the commit block sets bucket.last = now (the grant time), which is consistent. No actual bug here, but the cur_last = now for a new peer is subtly different from cur_last = construction_time and the distinction is not documented. The real issue: if the global inflight cap is saturated and the same new peer retries 1 second later, the elapsed is computed from now of the retry call (since the peer was never inserted), not from the first attempt — so the peer never accumulates any refill debt across AtCapacity rejections. This is documented as intentional in the module doc, but it means a peer that is always refused at Gate 3 can burst at full rate the moment capacity opens, regardless of how many times it tried. This is a design trade-off, not a bug, but it should be explicitly noted in the AtCapacity arm.

Suggested fix:

Add a comment to the `AtCapacity` return path noting that a never-inserted peer retains a full bucket across capacity-refused attempts, and that this is intentional (the global cap is the binding constraint, not per-peer fairness).

[xilosada]

Documented at the AtCapacity arm in 60057c6. The comment now states that a brand-new peer refused at Gate 3 is never inserted and so keeps a full bucket across capacity-refused attempts — intentional: while saturated the global inflight cap (not per-peer fairness) is the binding constraint, and not tracking never-proceeding peers is what stops a unique-peer flood from growing peers. (Also covered in the module docs; now visible at the return site too.)

[meroreviewer]

🟡 Peer idle-eviction threshold uses ceil() cast to u32, which overflows for large per_peer_burst values

In prune, full_refill is computed as self.per_peer_refill.saturating_mul(self.per_peer_burst.ceil() as u32). If per_peer_burst is very large (e.g. f64::MAX or even u32::MAX as f64 + 1.0), per_peer_burst.ceil() as u32 overflows: in Rust, casting an f64 that exceeds u32::MAX to u32 is saturating (produces u32::MAX) since Rust 1.45. So saturating_mul(u32::MAX) with any non-zero Duration will produce Duration::MAX (saturating), meaning peers are never evicted by the idle check. This is a correctness issue only for pathological configurations (burst > ~4 billion), but since per_peer_burst is a public f64 constant with no upper-bound assertion in new, a misconfigured node would silently disable idle pruning. The default DEFAULT_PER_PEER_BURST = 5.0 is safe.

Suggested fix:

Add an upper-bound assertion in `TeeAdmissionThrottle::new`: `assert!(per_peer_burst <= u32::MAX as f64, "per_peer_burst too large");` — or use `u64`/`u128` for the cast and `Duration::saturating_mul` with a checked conversion.

[xilosada]

Fixed in 60057c6 — new() now asserts per_peer_burst <= u32::MAX (alongside >= 1.0), and the # Panics doc is updated. That keeps the per_peer_burst.ceil() as u32 cast in the burst * per_peer_refill idle window exact. Worth noting the pre-existing behavior wasn't a memory leak even for an absurd burst: the cast saturates and saturating_mul caps at Duration::MAX, which only disables idle pruning — the MAX_TRACKED_PEERS hard cap still bounds the map. The assert just rejects the nonsensical config up front.

[meroreviewer]

💡 Durable store check consumes a throttle token even on a store-hit early return

When calimero_governance_store::is_quote_hash_used returns Ok(true), the handler returns true early (line ~200), dropping verify_permit. The inflight semaphore permit is correctly released. However, the per-peer token and the dedup entry were already committed by tee_admission_throttle.check() before this store read. A quote that is already admitted (common after a node restart with an existing fleet) will burn one token and occupy a dedup slot on every re-announce until the TTL expires, even though no verify work is done. Over a 300 s TTL with a burst of 5 tokens, a restarting fleet member that re-announces 5 times will exhaust its per-peer budget for 5 * 2 s = 10 s. This is unlikely to cause a real admission failure (the fleet-join window is 30 s and the refill rate is 1 token/2 s), but it is an unintended side-effect. Consider calling tee_admission_throttle.forget_quote (and ideally restoring the token) on the Ok(true) store-hit path, analogous to the transient-error path.

Suggested fix:

After the `Ok(true)` early return, call `this.tee_admission_throttle.forget_quote(group_id.to_bytes(), quote_hash)` so the dedup slot is freed. Optionally expose a `restore_token` method on the throttle to also return the consumed token.

[xilosada]

Assessed — documented the trade-off in 60057c6; keeping the throttle-before-store order. This is the flip side of the reorder you (correctly) asked for last round: an already-admitted quote that is re-announced burns one per-peer token before the store read reveals it's admitted. It's bounded (the dedup entry the throttle just recorded suppresses the rest of the burst — subsequent re-announces hit Duplicate, no token, no store read) and rare (a node stops announcing once admitted; fleet-join breaks on admission). Your own numbers bear this out: 5 re-announces = 10s of budget inside a 30s window with 1 token/2s refill, so no real admission risk.

I deliberately did not take the forget_quote-on-admitted route: dropping the entry would re-open a store read on every following re-announce (defeating the reorder), and restoring the token has the same effect. Keeping the entry is what makes the follow-ups cheap. A dedicated AlreadyAdmitted decision variant would need the throttle to perform the store read itself, coupling it to the datastore — net worse. The one-token cost on a rare path is the right price.

[cursor]

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 3 agents | Quality score: 76% | Review time: 444.7s

🟡 2 warnings, 💡 2 suggestions. See inline comments.

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-d303391c}

[meroreviewer]

🟡 New peer treated as full bucket when now == cur_last (zero elapsed)

For a brand-new peer the code synthesises (burst, now) as the initial (cur_tokens, cur_last). When elapsed = now.saturating_duration_since(now) = 0, refilled = burst + 0 * rate = burst, which is correct. However, the comment says 'first contact is never rate-limited', but if the same now is reused for many distinct new peers in the same call (e.g. a tight loop in tests or a burst of simultaneous announces), each one independently gets a full bucket and passes Gate 2. This is the intended behaviour per the module docs, but it means a flood of distinct peers each with a distinct quote can each consume one inflight permit per call until the global cap is hit. The global cap (DEFAULT_MAX_INFLIGHT_VERIFIES = 4) is the only backstop in that scenario. This is documented and intentional, but worth confirming the cap is sufficient for the threat model.

Suggested fix:

No code change required if the global cap of 4 is acceptable. Consider documenting explicitly that the per-peer bucket provides no protection against a flood of unique peers — only the global inflight cap does — so operators know to tune DEFAULT_MAX_INFLIGHT_VERIFIES for their threat model.

[xilosada]

Confirmed — and already documented; no code change (you note none is required). The module header and the Gate-3 / AtCapacity comment both state that the per-peer bucket does not bound a flood of distinct peers — the global inflight cap (DEFAULT_MAX_INFLIGHT_VERIFIES = 4) is the binding backstop in that scenario, and it's the knob operators tune for their threat model. For the TEE-01 threat (a mesh peer on a namespace topic), 4 concurrent verifies caps the CPU + outbound-PCS fan-out, which is the property the issue asked for.

[meroreviewer]

🟡 Dedup entry inserted before verify completes — a concurrent verify of the same quote is silently dropped

The dedup entry (group_id, quote_hash) is inserted in check() at the moment the announce is admitted, not when the verify succeeds. If two announces for the same quote arrive close together (within the gossipsub propagation window), the second is Duplicate-dropped even though the first verify is still in-flight and may fail transiently. forget_quote is called on Err, which restores the entry — but only after the first verify completes. During that window (which can be seconds if PCS is slow) a legitimate re-announce from a different peer is silently discarded. This is a documented trade-off, but the forget_quote path only fires when the spawned future's .map closure runs, which requires the actor to still be alive. If the actor is stopped while a verify is in-flight, the future is dropped without running .map, so forget_quote is never called — but since the throttle is torn down with the actor this is harmless. The real concern is the liveness window during a slow PCS fetch.

Suggested fix:

This is an accepted trade-off per the PR description. Consider adding a comment in `check()` at the insert site noting that the entry is intentionally pre-inserted and that `forget_quote` is the recovery path on transient error.

[xilosada]

Working as intended — and recovered. Pre-inserting the dedup entry on admission (not on verify success) is the whole point: it's what stops N concurrent/near-simultaneous announces of the same quote from each spawning a verify — the exact amplification this guards. For a slow but eventually-successful PCS fetch, dropping the concurrent re-announces is correct (the in-flight verify will admit; re-verifying would be pure waste). The only case that needs recovery is a transient failure, and that's exactly what forget_quote (called from the spawned task's .map on Err) handles — documented on the forget_quote method and at the .map site. The actor-stop sub-case you raise is, as you say, harmless (the throttle is torn down with the actor). So the trade-off is bounded and the liveness gap is closed.

[meroreviewer]

💡 SHA-256 of quote bytes computed on the actor thread before throttle check — bounded but worth noting

The comment correctly notes that SHA-256 over ≤64 KiB is tens of microseconds. However, this computation happens synchronously on the NodeManager actor thread for every inbound TeeAttestationAnnounce, including those that will be immediately rejected by the throttle. A flood of 64 KiB frames (gossipsub's max) at high rate will keep the actor thread busy hashing before the cheap in-memory gates can reject. The gossipsub transport bound (64 KiB) and the actor's single-threaded nature mean this is bounded, but it is a small amplification that could be avoided by moving the hash computation to after the per-peer rate-limit check (Gate 2), since Gate 1 requires the hash but Gate 2 does not.

Suggested fix:

Restructure: check Gate 2 (per-peer rate limit) first using only `source`, then compute `quote_hash` and check Gate 1 (dedup) and Gate 3 (inflight). This eliminates the SHA-256 cost for rate-limited peers. Requires splitting `check()` into two phases or exposing a `check_rate_limit_only(source)` helper.

[xilosada]

Already documented at the hash site (commit 4f2f797). The hash can't be deferred behind the gates — it's the key for both Gate 1 (dedup) and the durable is_quote_hash_used check — and it's bounded: quote_bytes is capped at gossipsub's 64 KiB default, so SHA-256 is tens of µs, negligible beside the PCS-fetch + DCAP verify it gates. The in-code comment already states this; no change.

[meroreviewer]

💡 Process-global LazyLock<VerifyQuoteThrottle> makes the throttle untestable in integration tests that share the same process

The VERIFY_QUOTE_THROTTLE static is initialised once per process. Any integration test that exercises the handler will share state with other tests running in the same process (Rust test harness runs tests in the same process by default). A burst in one test can exhaust the token bucket and cause unrelated tests to receive 429s. The node-side throttle avoids this by being a field on NodeManager (one per actor instance). Consider injecting the throttle via Extension or a newtype wrapper in AdminState so tests can construct isolated instances.

Suggested fix:

Add `throttle: Arc<VerifyQuoteThrottle>` to `AdminState` (or a dedicated `TeeState`) and inject it via `Extension`. The `LazyLock` default can remain as the production default. This matches the pattern used for `NodeManager::tee_admission_throttle`.

[xilosada]

Same assessment as the earlier rounds on this point: the throttle's logic is unit-tested directly via the check_at(now) injection seam, so it isn't untested. The process-global only matters for handler-level integration tests, of which none exist — exercising the real handler needs a live Intel-PCS endpoint + a valid TDX quote (the documented follow-up gap). If such a test is added, injecting via Extension<Arc<VerifyQuoteThrottle>> is the right move; until then it's router plumbing for a test that doesn't exist (YAGNI).

[github-actions]

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.