Repository navigation
perf(mobile): skip re-decrypting read-state blobs - #8966
Conversation
ReadStateCrypto.decrypt (pure-Dart NIP-44 on the UI isolate) cost 106 ms of CPU in a 20 s capture and 30 ms in a 10 s capture on an iPhone 12 mini Profile build. Every publish re-fetches up to 500 of our own read-state events and decrypts each, and the relay echoes each publish back, though nearly all of those ciphertexts were already decrypted or produced by this device. ReadStateCrypto now keeps a bounded memo from ciphertext to plaintext. decrypt checks it first and encrypt records what it produced, so the echo and the next pre-publish fetch do no cipher work. A NIP-44 payload carries a random nonce and a MAC, so equal ciphertext means equal plaintext under this instance's conversation key. Failed decrypts are never cached and still throw each time. The memo is capped at 2M retained code units (ciphertext plus plaintext), evicting oldest first. Co-authored-by: studio <dc2fc481343672df31a689840c09299972e704dac592e0da6f18486646a7ee3e@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Tom Brow <[email protected]>
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
On an iPhone 12 mini the memo saved nothing: every resume replays all of the account's read-state slots (27 here, about 55 KB of ciphertext each) and their total, about 2.6M code units, was more than the 2M bound. An oldest-first memo walked in that order evicts each entry just before its next use, so all 27 decrypts missed on every resume. A hit now moves the entry to the newest end, so superseded blobs are evicted before live ones, and the bound is 8M code units (about 80 full slots, about 8 MB). New tests replay 27 full slots twice and interleave hits with new blobs under a small bound; both fail on the old memo. Co-authored-by: studio <dc2fc481343672df31a689840c09299972e704dac592e0da6f18486646a7ee3e@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Tom Brow <[email protected]>
…e-decrypt-cache Signed-off-by: Tom Brow <[email protected]>
Main now fetches read state with fetchReadStateSnapshot, so the fake session serves its history there too. Co-authored-by: studio <dc2fc481343672df31a689840c09299972e704dac592e0da6f18486646a7ee3e@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Tom Brow <[email protected]>
🔐 Codex Security Review
Review SummaryOverall Risk: NONE
FindingsNo concrete security, correctness, or reliability findings were identified. Notes
Generated by Codex Security Review | |
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Carl, an automated reviewer, commenting via Wes's GitHub account.
Approving at 04ad2e3. No blocking findings.
- The memo is keyed by exact ciphertext under one conversation key, and an account or key change builds a new
ReadStateCrypto. Failed decrypts are never cached. - A hit still goes through event decoding and marker merging, so it can't hide a newer marker. A new ciphertext always misses.
- Retention is bounded by code units (ciphertext plus plaintext), and oversized blobs are skipped. This is not an exact heap bound, which is fine for the purpose.
Validation gap: source and test review plus CI only. The device speedup and heap behavior were not measured independently.
## Summary Two pure-Dart crypto paths that run on the UI isolate during normal use: - **NIP-98 body hashes.** Every relay HTTP query (`RelaySessionNotifier._postForEvents`) hashes its request body for the NIP-98 auth header with pointycastle's `SHA256Digest`. This switches that hash, the media upload hash and the NIP-OA preimage hash to `package:crypto`'s `sha256`, which is already a dependency. The output is identical. - **Public key derivation.** `AccountIdentityStore.publicKeyForNsec` runs on every relay auth assertion and derived the key with the `nostr` package's pure-Dart curve math. It now uses the native `derivePublicKeyHex` (libsecp256k1) with the same validation. The default-agent writer now signs with the native `signNostrEvent` instead of `nostr.Event.from`. Left as they were: `nostr.Keys` construction for signing keys (nostr 2.0.0 always derives in Dart, with no way to pass a known public key), and `nostr.Keys.generate()` on the rare key-creation paths. ## Measurements - **Device** (iPhone 12 mini, Profile build, #4things captures): NIP-98 hashing took 17 to 24 ms of UI CPU per 10 to 20 s capture, mostly from channel notices queries. `publicKeyForNsec` took about 7 ms per relay auth assertion. - **Synthetic** (Mac, AOT, same output): SHA-256 of a 1 KB body takes 39.9 µs with pointycastle and 6.6 µs with `package:crypto`. 50 KB: 1,518 µs vs 293 µs. 1 MB: 29.6 ms vs 6.1 ms. - **Device A/B after the change** (iPhone 12 mini, Profile/AOT, Tom's Block community, two app resumes per build from the home screen, 15 s windows; A is `origin/main` 3d26ed7, B is A plus this PR and [#8966](#8966)): NIP-98 signing per resume went from 42 / 50 ms to 9 / 13 ms of UI CPU, and all SHA-256 work per resume from 74 / 73 ms to 33 / 33 ms. Dart public-key derivation through `nostr.Keys` (left as is, see above) still takes 15 to 23 ms per resume. - **Same A/B on current `main`** (14037e3): pointycastle SHA-256 per resume went from 39 / 45 ms to 7 / 0 ms, and NIP-98 header building from 4 / 5 ms to 2 / 0 ms. `nostr.Keys` derivation: 12 to 17 ms per resume on both builds. ## Testing - New known-vector tests for the NIP-98 header hash: an empty body, `abc`, and the one-million-`a` FIPS vector. They also check the event's kind, method, signature and pubkey. - New `publicKeyForNsec` tests: the generator-point vector, matching the pure-Dart derivation, and rejecting invalid keys (scalar 0 and n, garbage, an npub). - These are correctness tests only. A timing test would be flaky, so none would fail if the old pure-Dart code came back. - `just mobile-check mobile-test` passes (pre-push hook). - Automated device run in place of manual testing: the B build above on the 12 mini, launched and resumed twice into a signed-in Block community, loading the channel list, channel notices and read state over authenticated relay queries (NIP-98 headers and relay auth assertions on every request). Not exercised on the device: media upload and the default-agent writer. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Tom Brow <[email protected]> Co-authored-by: Tom Brow <[email protected]> Co-authored-by: studio <dc2fc481343672df31a689840c09299972e704dac592e0da6f18486646a7ee3e@buzz.block.builderlab.xyz> Co-authored-by: Claude Opus 5.5 <[email protected]>
Summary
Read-state blobs are NIP-44 encrypted, and the app decrypts them with pure-Dart ChaCha20 and HMAC-SHA256 on the UI isolate. Most of those decrypts repeat earlier work:
_handleIncomingEventdecrypts each one again. The measured account has 27 slots of about 55 KB ciphertext each._publishOncefetches the read-state snapshot and decrypts it, and decodes our own slots a second time to check they're readable.ReadStateCryptonow keeps a bounded memo from ciphertext to plaintext, one per conversation key.decryptchecks it first, andencryptrecords the ciphertext it produced. NIP-44 payloads carry a random nonce and a MAC, so under one key the same ciphertext always decrypts to the same plaintext. Failed decrypts aren't cached and still throw every time.The memo is least recently used: a hit moves the entry to the newest end, so our own superseded blobs are evicted before live ones. It holds at most 8,000,000 code units across keys and values (about 8 MB, about 80 full slots). The first version used oldest-first eviction with a 2M bound, which saved nothing on the device: the 27 slots total about 2.6M, and a replay in slot order evicted each entry just before its next use.
Measurements
iPhone 12 mini, Profile/AOT, Tom's Block community. Each run resumes the app twice from the home screen and samples UI-isolate CPU for 15 s from each resume.
ReadStateCrypto.decryptper resumemain14037e3main+ this PR + #8965On the older base 3d26ed7 the first version of this PR measured 74/70 ms before and 73/71 ms after (no gain). An instrumented build showed 27 decrypts per resume with 0 memo hits, all on one
ReadStateCryptoinstance. With the LRU fix, the same base measured 7/3 ms.Testing
encryptruns it zero times. Failed decrypts throw every time. The bound caps retained size.just mobile-check mobile-testpasses (pre-push hook), after merging currentmain.🤖 Generated with Claude Code