From f6dbb46924292f242838e0b63c8b110ab25ef2ee Mon Sep 17 00:00:00 2001 From: Arseniy Klempner Date: Thu, 28 May 2026 13:53:56 -0600 Subject: [PATCH] cleanup(06-try-catch): notes - 3 fixes applied (waitForChainCommit, self-reg watcher, dead poll-loop except) --- cleanup/06-try-catch.md | 80 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) create mode 100644 cleanup/06-try-catch.md diff --git a/cleanup/06-try-catch.md b/cleanup/06-try-catch.md new file mode 100644 index 0000000..b83a651 --- /dev/null +++ b/cleanup/06-try-catch.md @@ -0,0 +1,80 @@ +# Cleanup 06 - defensive try/except removal + +Scope: in-scope Nim files (Level 1-5), C++ (logos-rln-module), Rust (lez-rln-ffi). +Restore tag: `restore/sim-15-15-pass-2026-05-21`. + +This file supersedes the first-pass audit. The first pass concluded "no removals"; +a second pass found three real issues and applied them. + +## Research notes + +`waku_client.nim` (Level 1): zero try/except. + +`node_factory.nim` (Levels 2 & 5): +- ~L240-246: parseJson on FFI register_member response (returns Result) - boundary, KEEP. +- ~L265-278: `waitForChainCommit` poll loop: parseJson on FFI is_member_registered response. ORIGINAL: silent `except CatchableError: continue`. APPLIED FIX: narrow to `JsonParsingError`/`JsonKindError` and `warn`-log before `continue`. +- ~L295: `waitForChainCommit` invoked via `await` - now wrapped in caller's outer flow; left as-is. +- ~L730-810: Gifter self-registration handler (logs via `warn` in outer except) - KEEP. + - INSIDE: ~L775-780 self-reg watcher poll loop. ORIGINAL: silent `except CatchableError: continue` around `await gifter.statusHandler(...)` and bare `await sleepAsync` (CancelledError unhandled). APPLIED FIX: add `CancelledError: return` carve-outs for both await sites; `debug`-log + `continue` for the libp2p I/O CatchableError branch. +- ~L820-832: gifter client registration RPC - KEEP. +- Other mountXxx wrappers and library-boundary catches - KEEP per first-pass audit. + +`rln_gifter/client.nim` (Levels 2 & 5): +- L61-127: libp2p stream I/O `writeLp`/`readLp` wrapped in narrow `LPStreamError` - KEEP (mandated by `{.async: (raises: [CancelledError]).}` annotation). +- L148-160: watcher poll loop - already has correct `CancelledError: return` + `CatchableError -> Result.err(...)` pattern (NOT silent). KEEP. + +`rln_gifter/protocol.nim` (Levels 2 & 5): server-side handlers - all narrow LPStreamError. KEEP. + +`logos_core_client.nim` (Levels 2 & 5): parseHexInt / parseJson at FFI deserialization boundary - all convert to `Result.err(...)`. KEEP. + +`onchain_group_manager.nim` (Level 3 - mix-rln-spam-protection-plugin): +- `pollLoop` L182-194 and L200-220: `try/except CatchableError as e: debug "..."` wrapping `await gm.fetchRoots()` and `await gm.fetchProof(...)`. APPLIED FIX: REMOVED both try/except blocks - **dead code**. The callbacks are typed `proc(): Future[...] {.gcsafe, raises: [].}` (see L25-26 of same file), so `await` cannot raise `CatchableError`. The Result inside the Future is the only error path, and the existing `if .isOk/.isErr` branches already log it. + +`spam_protection.nim` (Level 3): `nullifierLog` `Table[]` access wrapped in narrow `KeyError -> Result.err(...)`. KEEP. + +C++ (`logos_rln_module.cpp/.h`): no try/catch. +Rust (`lez-rln-ffi/src/lib.rs`): no defensive `match Err`. Out of scope. + +## Critical assessment by category + +1. **libp2p stream I/O catches** (`client.nim`, `protocol.nim`): KEEP. Narrow LPStreamError, converts to Result. Required by `(raises: [CancelledError])` annotation. +2. **chronos CancelledError**: KEEP and ADD where missing. Mandatory for graceful task cancellation. +3. **FFI parse boundaries** (`parseJson`, `parseHexInt`, `Address.fromHex`): KEEP. Real deserialization trust boundary; converts to typed `Result.err`. +4. **`await mountXxx` wrappers** (~11 sites): KEEP. Library-boundary `try -> err(...)`. +5. **`Table` KeyError**: KEEP. Library boundary. +6. **Outer wrappers that warn-log**: KEEP. Logged failures with intentional retry/continue. +7. **Poll-loop swallow + continue with NO log AND no Result conversion**: REMOVE or fix. Two sites in `node_factory.nim` (`waitForChainCommit` and self-reg watcher). +8. **try/except CatchableError around await of a `{.raises: [].}` proc**: REMOVE. Dead code by type system. + +## Applied + +### vendor/nwaku (Level 2) - branch `feat/sim-rln-gifter-auth` + +`waku/factory/node_factory.nim`: +- `waitForChainCommit`: narrow `except CatchableError: continue` to `except JsonParsingError as e: warn ...; continue` + `except JsonKindError as e: warn ...; continue`. +- Self-reg watcher poll loop: replace bare `await sleepAsync` with `try/except CancelledError: return`; replace silent `except CatchableError: continue` on `await gifter.statusHandler(...)` with `except CancelledError: return` + `except CatchableError as e: debug ...; continue`. + +`vendor/mix-rln-spam-protection-plugin/src/mix_rln_spam_protection/onchain_group_manager.nim` (Level 3 - branch `cleanup`): +- `pollLoop`: REMOVE both `try/except CatchableError as e: debug ...` blocks around `await gm.fetchRoots()` and `await gm.fetchProof(...)`. Callbacks are `{.raises: [].}` - the except branches were unreachable. + +### Mirrored to (byte-identical): + +- `vendor/logos-lez-rln/logos-delivery/waku/factory/node_factory.nim` +- `vendor/logos-lez-rln/logos-delivery-module/vendor/logos-delivery/waku/factory/node_factory.nim` +- `vendor/logos-lez-rln/logos-delivery/vendor/mix-rln-spam-protection-plugin/src/mix_rln_spam_protection/onchain_group_manager.nim` +- `vendor/logos-lez-rln/logos-delivery-module/vendor/logos-delivery/vendor/mix-rln-spam-protection-plugin/src/mix_rln_spam_protection/onchain_group_manager.nim` + +## Deferred + +- Two `parseJson`-wrapping `CatchableError` catches in `node_factory.nim` register_member (~L240) could be narrowed to `JsonParsingError`/`JsonKindError`. Deferred - already at boundary, logs in caller, low value. +- Gifter self-registration handler (`node_factory.nim` ~L742-803) outer `try/except CatchableError as e: warn`: could split the try to wrap only the `await gifter.registerHandler(...)`. Cosmetic; deferred. +- Rust `unwrap()` audit on FFI slice -> array conversions: out of try/catch scope (infallible-by-construction claims belong in a separate pass). + +## Builds + +- `make liblogoschat` - OK. +- `make liblogosdelivery` (under `vendor/logos-lez-rln/logos-delivery`) - OK. + +## Sim + +Skipped per agent-06 instructions; the user will run a single end-of-pass verification after all 8 agents.