Repository navigation
perf(mobile): sign events with native libsecp256k1 on iOS and Android - #8654
Conversation
Add a buzz_secp256k1 package that builds vendored bitcoin-core/libsecp256k1 v0.8.0 (ecdh, extrakeys, schnorrsig) through a Dart build hook, and route every nostr.Event.from signing site through signNostrEvent, which keeps the nostr event-id serialization and moves pubkey derivation and BIP-340 signing to native code. Co-authored-by: george <973be878edd6d4f79692a3827f20a6f7fe0ed0362f269ff6b6ba8867033d7d8e@sprout-oss.stage.blox.sqprod.co> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Tom Brow <tomb@block.xyz>
|
@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. |
The BIP-340 vectors and native-vs-Dart cross checks live in the package and were not part of the mobile test run. Also drop a stale cost comment. Co-authored-by: george <973be878edd6d4f79692a3827f20a6f7fe0ed0362f269ff6b6ba8867033d7d8e@sprout-oss.stage.blox.sqprod.co> Signed-off-by: Tom Brow <tomb@block.xyz>
github-actions
Bot
added
the
codex-security-review-current
🔐 Codex Security Review
Review SummaryOverall Risk: NONE
FindingsNo concrete security, correctness, or reliability findings were identified. Notes
Generated by Codex Security Review | |
Creating the signing context randomizes it through the aux scratch slot, so the first schnorrSign in an isolate overwrote the caller's auxRand with zeros. Create the context before loading inputs. Co-authored-by: george <973be878edd6d4f79692a3827f20a6f7fe0ed0362f269ff6b6ba8867033d7d8e@sprout-oss.stage.blox.sqprod.co> Signed-off-by: Tom Brow <tomb@block.xyz>
|
🤖 Security review finding (first signature discards auxRand): valid. Addressed in ad11110 by creating the signing context before loading inputs, with a fresh-isolate test on BIP-340 vector 1 that fails without the fix. |
|
@codex review |
github-actions
Bot
added
codex-security-review-current
|
Codex Review: Didn't find any major issues. Bravo. 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". |
kalvinnchau
left a comment
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 Reviewed ad11110: signing serialization, FFI buffer/context ownership, key cleanup, and iOS/Android build contracts. No P0–P2 findings. The inline header-dependency P3 is nonblocking. Reported validation: 27 package tests and 340 affected mobile tests passed; mobile analysis clean. Device builds/workflows were not independently exercised.
| '$_vendor/src/precomputed_ecmult.c', | ||
| '$_vendor/src/precomputed_ecmult_gen.c', | ||
| 'src/buzz_secp256k1_ecdh.c', | ||
| ], |
There was a problem hiding this comment.
🤖 [P3] Declare vendored headers as build dependencies (nonblocking)
After an initial build, a header-only vendor update (for example src/modules/schnorrsig/main_impl.h) can reuse the old native library in incremental app/test builds. This hook declares four C sources but no header dependencies; pinned native_toolchain_c 0.17.6 registers sources and files under includes, rather than transitive C includes (lib/src/cbuilder/cbuilder.dart:221–236). Add includes: ['$_vendor'] or explicitly register the headers so changes invalidate the asset cache. Fresh builds are unaffected. Confirmed from source contracts; not reproduced with a native build.
Follow-up to #8654, from [Kalvin's review note](#8654 (comment)). `native_toolchain_c`'s `CBuilder` registers only the listed C sources (and files under `includes`) as build-hook dependencies. The `buzz_secp256k1` hook lists four sources and no headers, so after an initial build a header-only vendor update (for example `src/modules/schnorrsig/main_impl.h`) could reuse the old library in incremental app and test builds. The hook now also registers every `.h` file under the vendored tree. It does not add include paths, so compilation is unchanged. ### Validation - With the change, editing the content of `third_party/secp256k1/src/modules/schnorrsig/main_impl.h` makes the next `dart test` rebuild `libbuzz_secp256k1.dylib`. Without it, the library is reused unchanged. - The hook output now lists all 62 vendored headers as dependencies. - Package tests pass (27). buzz-review-completed (Codex and security review clean; agent verification above; build-only change, no app behavior to test) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Tom Brow <tomb@block.xyz> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes's GitHub account.
Post-merge review at ad11110: no findings.
- All 66 vendored files match bitcoin-core/secp256k1 v0.8.0 byte for byte, and the tarball SHA-256 matches
VENDORED.md. - The build hook defines match upstream defaults, including the precomputed table parameters. The ECDH shim keeps the constant-time path.
- Secret and keypair scratch are wiped, the signing context is randomized once per isolate, and aux randomness is fresh per signature.
- All 13 migrated signing sites keep their inputs and the
nostrevent-id serialization.
Validation gap: device timings and the iOS/Android runtime checks are taken from the PR description and were not reproduced.
…-handler * origin/main: feat(mobile): unify Messages navigation with Solo and Team filters (#8649) Smooth profile avatar editor transitions (#8640) chore(desktop): point `just staging` at the new Block staging relay (#8623) ci: repeat changed tests ten times on pull requests (#8613) fix(mobile): rebuild buzz_secp256k1 when vendored headers change (#8664) fix(push-gateway): log which App Attest certificate check failed (#8671) fix(mobile): resume reconnect replay from the last live point and spread reconnects (#8659) fix(mobile): push every message in your own Solo chats (#8615) perf(mobile): sign events with native libsecp256k1 on iOS and Android (#8654) fix(relay): revoke invitations and restrict claims (#8237) Signed-off-by: Ravneet Arora <rarora@squareup.com>
Follows #8654. Deriving an x-only public key from a secret key with `nostr.Keys(secret).public` costs about 5.6 ms in pure Dart on an iPhone SE (2nd gen, Profile). The app does it in 10 places, including relay config, the signed-event relay's `pubkey` getter, push lease checks, status updates, and the settings identity row's `build` method. This routes all of them through `derivePublicKeyHex` from #8654, which takes 16.5 µs on the same device. Behavior is unchanged for valid keys: output is the same lowercase hex. For a malformed or out-of-range key the thrown type changes from nostr's `InvalidKeyException` to `ArgumentError`; every affected site either catches all errors or rethrows its own `FormatException`/`StateError`, and nothing in `lib` or `test` matches on the old type. Sites that need a full `nostr.Keys` object (key generation, `AccountIdentityStore`) are unchanged. ### Validation - `derivePublicKeyHex` is checked against `nostr.Keys` for 64 random keys in #8654's `event_signing_test.dart`. - Full mobile analyze and test suite pass on this branch after rebasing onto main, including the package's own tests. - iPhone 12 mini, Profile build of this stack (with #8656 and #8657 on top): paired to the internal Buzz community through Desktop; relay auth, identity row, avatar, and channel/DM/inbox loading all worked under the derived key. buzz-review-completed (agent review, Codex and security review clean; device validation above; human app test not applicable under the mobile testing exception) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Tom Brow <tomb@block.xyz> Co-authored-by: george <973be878edd6d4f79692a3827f20a6f7fe0ed0362f269ff6b6ba8867033d7d8e@sprout-oss.stage.blox.sqprod.co> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
`ecdhSharedSecret`, the input to every NIP-44 conversation key (preferences, read state, reminders, themes, mutes, stars, sections, sort, pairing, agent observer), used pointycastle's BigInt point multiplication. That costs about 5.9 ms per call on an iPhone SE (2nd gen, Profile) and is variable time in the secret scalar. It now calls libsecp256k1's constant-time `secp256k1_ecdh` through `ecdhX` from #8654 (39.8 µs on the same device), returning the same raw x-coordinate NIP-44 requires. The decoded secret key bytes are zeroed after the call. Invalid keys now throw `ArgumentError` instead of pointycastle's errors; callers already treat any throw as a failed derivation. ### Validation - Added tests: `ecdh_test.dart` compares the native result with the previous pointycastle algorithm for 32 random key pairs, checks the NIP-44 v2 `get_conversation_key` vector (secret 1, peer pubkey of secret 2), and checks rejection of a zero secret, an off-curve public key and wrong lengths. - Full mobile analyze and test suite pass after rebasing onto main at da1190e, including the desktop nostr-rs NIP-44 interop vector. - On a device: an iPhone 12 mini Profile build of the full native stack, connected to the internal relay. buzz-review-completed 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Tom Brow <tomb@block.xyz> Co-authored-by: george <973be878edd6d4f79692a3827f20a6f7fe0ed0362f269ff6b6ba8867033d7d8e@sprout-oss.stage.blox.sqprod.co>
Mobile signs every Nostr event (sends, relay AUTH, media auth headers, preference writes) in pure Dart on the UI isolate. On an iPhone SE (2nd gen, Profile build) one BIP-340 signature costs about 10.7 ms and deriving the public key another 5.6 ms, so a burst of signed writes drops frames.
This adds
mobile/packages/buzz_secp256k1, a small FFI package that builds vendored bitcoin-core/libsecp256k1 v0.8.0 (ecdh, extrakeys, schnorrsig modules) through a Dart build hook, so the same C and the same Dart API ship on iOS and Android with no platform bridge code. All 13nostr.Event.fromsigning sites now go throughsignNostrEvent, which keeps thenostrpackage's event-id serialization and moves only key derivation and signing to native code. Calls stay synchronous.The package also exposes
schnorrVerifyandecdhX; follow-up PRs move verification and the NIP-44 conversation key over. Incoming relay events are still not verified, unchanged by this PR.Safety: the signing context is randomized once per isolate, secret keys and keypairs are wiped from the native scratch buffer after each call, aux randomness comes from
Random.secure(), and the static verify context runs libsecp256k1's self test before first use.Validation
event_signing_test.dartsigns with 64 random keys and checks pubkey, event id and signature against the existingnostrbuilder and verifier, plus malformed-key rejection.buzz_secp256k1.frameworkembedded; the signed-in app authenticated to a real community relay, loaded channels and history, and paged older messages. No native-asset or secp256k1 errors in the console.libbuzz_secp256k1.soand loads it. Against a local mock relay that verifies with the pure-Dartnostrpackage, the app's NIP-42 AUTH, NIP-98POST /queryheaders, and kind 20001/30078 events all verified./media/GETs. From inside the running app, the productionMediaGetAuthService.headersFor()header for a nonexistent hash got 404 (accepted, file missing), compared with 401 without it. The community has no relay-hosted image, so none was rendered.buzz-review-completed (agent review, Codex and security review clean; device validation above; human app test not applicable under the mobile testing exception)
🤖 Generated with Claude Code