feat(blst): use external buffers for blst operations - #358
Conversation
This is one of possible likely causes for increased GC pressure on experiments to swap out blst-ts for lodestar-z/bls, as observed on feat2 and feat3 deployments in [this PR](ChainSafe/lodestar#9342). With external array buffers, V8 is only aware of the pointer to the backing memory, instead of having to track both the pointer and the backing memory. This means that during marking phase the GC does not have to walk the backing memory to mark it as 'live' - the frequency of the GC firing off is still the same, but each cycle does less work. This of course comes with a tradeoff, we need a **finalizer** to let V8 know how much external memory is in native heap so that the GC tells the native impl to free the useless memory. Though, regardless of the effect, we should still probably do this anyway, since [napi-rs does the same](https://github.com/napi-rs/napi-rs/blob/159395b365c583a6642ad481edc5708d9f36a24b/crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs#L175), and only defaults to V8 managed array buffers if it is disallowed (like in Electron).
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request optimizes memory handling for blst operations by switching to external array buffers. This change aims to reduce GC overhead during marking phases by minimizing the work required for V8 to track memory, ultimately improving performance in high-load scenarios. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request updates the zapi dependency and modifies the toBytes implementations for PublicKey, Signature, and SecretKey to use js.Uint8Array.fromExternal. The reviewer identified critical use-after-free vulnerabilities in all three methods: since the source data is stack-allocated, the JavaScript objects created via fromExternal point to memory that becomes invalid upon function return. To resolve this, the data must be copied to heap-allocated memory before being wrapped for JavaScript.
| if (try boolOrDefault(compress, true)) { | ||
| const bytes = self.raw.compress(); | ||
| return js.Uint8Array.from(bytes[0..]); | ||
| return js.Uint8Array.fromExternal(bytes[0..]); | ||
| } | ||
| const bytes = self.raw.serialize(); | ||
| return js.Uint8Array.from(bytes[0..]); | ||
| return js.Uint8Array.fromExternal(bytes[0..]); |
There was a problem hiding this comment.
This implementation introduces a critical use-after-free vulnerability.
The self.raw.compress() and self.raw.serialize() functions return arrays that are stored on the current function's stack. The slice passed to js.Uint8Array.fromExternal then points to this stack memory.
When toBytes returns, its stack frame is deallocated, making the pointer held by the JavaScript Uint8Array invalid. Any subsequent access to this Uint8Array in JS will read from deallocated memory, leading to undefined behavior, memory corruption, or a crash.
To fix this, you must allocate the buffer on the heap. The fromExternal function can then wrap this heap-allocated buffer, and its associated finalizer (which I assume zapi sets up) will be responsible for freeing the memory.
Here is a suggested implementation that correctly allocates memory on the heap:
const do_compress = try boolOrDefault(compress, true);
const len = if (do_compress) NativePublicKey.COMPRESS_SIZE else NativePublicKey.SERIALIZE_SIZE;
const heap_bytes = try allocator.alloc(u8, len);
// This errdefer is important in case fromExternal throws.
errdefer allocator.free(heap_bytes);
if (do_compress) {
const stack_bytes = self.raw.compress();
@memcpy(heap_bytes, stack_bytes[0..]);
} else {
const stack_bytes = self.raw.serialize();
@memcpy(heap_bytes, stack_bytes[0..]);
}
return js.Uint8Array.fromExternal(heap_bytes);
| if (try boolOrDefault(compress, true)) { | ||
| const bytes = self.raw.compress(); | ||
| return js.Uint8Array.from(bytes[0..]); | ||
| return js.Uint8Array.fromExternal(bytes[0..]); | ||
| } | ||
| const bytes = self.raw.serialize(); | ||
| return js.Uint8Array.from(bytes[0..]); | ||
| return js.Uint8Array.fromExternal(bytes[0..]); |
There was a problem hiding this comment.
Similar to PublicKey.toBytes, this function has a use-after-free vulnerability because it passes a slice of stack-allocated memory to js.Uint8Array.fromExternal. The memory must be allocated on the heap to prevent the JavaScript Uint8Array from pointing to invalid memory after this function returns.
const do_compress = try boolOrDefault(compress, true);
const len = if (do_compress) NativeSignature.COMPRESS_SIZE else NativeSignature.SERIALIZE_SIZE;
const heap_bytes = try allocator.alloc(u8, len);
// This errdefer is important in case fromExternal throws.
errdefer allocator.free(heap_bytes);
if (do_compress) {
const stack_bytes = self.raw.compress();
@memcpy(heap_bytes, stack_bytes[0..]);
} else {
const stack_bytes = self.raw.serialize();
@memcpy(heap_bytes, stack_bytes[0..]);
}
return js.Uint8Array.fromExternal(heap_bytes);
| const bytes = self.raw.serialize(); | ||
| return js.Uint8Array.from(bytes[0..]); | ||
| return js.Uint8Array.fromExternal(bytes[0..]); |
There was a problem hiding this comment.
This function also has a use-after-free vulnerability. The serialize() method returns a stack-allocated array, and a slice of it is passed to fromExternal. This memory will be invalid after the function returns. You need to copy the bytes to a heap-allocated buffer.
const stack_bytes = self.raw.serialize();
const heap_bytes = try allocator.alloc(u8, stack_bytes.len);
errdefer allocator.free(heap_bytes);
@memcpy(heap_bytes, stack_bytes[0..]);
return js.Uint8Array.fromExternal(heap_bytes);
|
The gemini reviews are inaccurate, since the data is indeed copied (but hidden in the zapi API) |
4f7549e to
cc7d466
Compare
This reverts commit 78e4678. To investigate GC regressions observed on metrics
Depends on ChainSafe/zapi#30 (we need to release zapi and update the dep) This is one of possible likely causes for increased GC pressure on experiments to swap out blst-ts for lodestar-z/bls, as observed on feat2 and feat3 deployments in [this PR](ChainSafe/lodestar#9342). With external array buffers, V8 is only aware of the pointer to the backing memory, instead of having to track both the pointer and the backing memory. This means that during marking phase the GC does not have to walk the backing memory to mark it as 'live' - the frequency of the GC firing off is still the same, but each cycle does less work. This of course comes with a tradeoff, we need a **finalizer** to let V8 know how much external memory is in native heap so that the GC tells the native impl to free the useless memory. Though, regardless of the effect, we should still probably do this anyway, since [napi-rs does the same](https://github.com/napi-rs/napi-rs/blob/159395b365c583a6642ad481edc5708d9f36a24b/crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs#L175), and only defaults to V8 managed array buffers if it is disallowed (like in Electron).
🤖 I have created a release *beep* *boop* --- ## [1.0.0](v0.1.2...v1.0.0) (2026-08-19) ### Features * add `state.getBuildersLength()` binding ([#472](#472)) ([be2b5ab](be2b5ab)) * **beacon-node:** add block state cache and checkpoint datastore ([#452](#452)) ([2145faa](2145faa)) * bindings to `getExpectedWithdrawals` and native tweaks ([#350](#350)) ([f47bc66](f47bc66)) * **bindings:** add pubkey cache syncPubkeys ([#537](#537)) ([542779f](542779f)) * **bindings:** aggregate cached public keys by validator index ([#397](#397)) ([2f90603](2f90603)) * **bindings:** align `BeaconStateView` with `IBeaconStateView` ([#347](#347)) ([b8ec273](b8ec273)) * **bindings:** configurable pubkey cache growth step ([#481](#481)) ([133ef24](133ef24)) * **bindings:** expose more APIs for STF ([#444](#444)) ([7fe2609](7fe2609)) * **bls:** add small MSM for npoints < 32 ([#393](#393)) ([b430638](b430638)) * **blst:** use external buffers for blst operations ([#358](#358)) ([78e4678](78e4678)) * **ci:** conditionally publish bindings with tag ([#355](#355)) ([ea77919](ea77919)) * **clock:** add clock module for slot/epoch timing ([#354](#354)) ([385b077](385b077)) * **fork_choice:** add Prometheus metrics module ([#309](#309)) ([cbc9d8d](cbc9d8d)) * **forkchoice:** implement the forkchoice module ([#246](#246)) ([7c62a9b](7c62a9b)) * getSyncCommitteesWitness ([#367](#367)) ([ef77649](ef77649)) * implement `loadState` API and binding ([#165](#165)) ([f903519](f903519)), closes [#159](#159) * **metrics:** metrics bindings ([#455](#455)) ([dd41999](dd41999)) * migrate blst,pubkeys to use zapi js dsl ([#331](#331)) ([fcd26ca](fcd26ca)) * **pubkeys:** add getPubkeyBytes binding ([#555](#555)) ([4ca51cf](4ca51cf)) * publish ARM64 musl bindings ([#482](#482)) ([ac764c9](ac764c9)) * **shuffle:** add swap-or-not shuffling module and binding ([#559](#559)) ([c2db37c](c2db37c)) * split nextValue fn ([#464](#464)) ([b47faeb](b47faeb)) * support getLatestWeakSubjectivityCheckpointEpoch ([#366](#366)) ([dcf3883](dcf3883)) * update fulu deposit processing ([#442](#442)) ([064335c](064335c)) ### Bug Fixes * avoid set ([#484](#484)) ([2e25d97](2e25d97)) * better generation of rand scalar ([#388](#388)) ([74dce77](74dce77)) * **bindings:** accept `dontTransferCache` in processSlots for backward compatibility ([#460](#460)) ([65df5af](65df5af)) * **bindings:** check signature infinity by default ([#509](#509)) ([2f5f281](2f5f281)) * **bindings:** clean up failed async BLS work ([#527](#527)) ([1111b00](1111b00)) * **bindings:** free metrics writer on scrape failure ([#529](#529)) ([4c8d94a](4c8d94a)) * **bindings:** harden random aggregate scalars ([#528](#528)) ([8e89a63](8e89a63)) * **bindings:** log level for missing fields ([#435](#435)) ([08faf41](08faf41)) * **bindings:** misordering of print for cpu count ([#381](#381)) ([752a972](752a972)) * **bindings:** populate epoch participation for test fixtures ([#436](#436)) ([8dbdd2e](8dbdd2e)) * **bindings:** refcount Pool to fix teardown panic ([#352](#352)) ([23b2f68](23b2f68)) * **bindings:** roll back partial N-API initialization ([#491](#491)) ([31c5ebb](31c5ebb)) * **bindings:** size BLS thread pool by cgroup-aware CPU count ([#386](#386)) ([3ae9522](3ae9522)) * **bindings:** validate class types before unwrap ([#514](#514)) ([2fd2ad5](2fd2ad5)) * **bindings:** validate secret key hex length ([#517](#517)) ([136e415](136e415)) * **bls:** align PublicKey.uncompress validation with Signature.uncompress ([#508](#508)) ([5a8dbe9](5a8dbe9)) * **bls:** bound randomized aggregation inputs ([#548](#548)) ([779d0bf](779d0bf)), closes [#542](#542) * **bls:** clean up partial thread pool initialization ([#490](#490)) ([d55e598](d55e598)) * **bls:** convert pippenger scratch bytes to element counts ([#513](#513)) ([a12ca92](a12ca92)) * **bls:** enforce 32-byte signing roots ([#545](#545)) ([72fd308](72fd308)) * **bls:** make batch cardinality structural ([#547](#547)) ([a06d8b2](a06d8b2)) * **bls:** preserve aggregate outputs on failure ([#521](#521)) ([e0b6dd1](e0b6dd1)) * **bls:** reject empty keygen salts ([#524](#524)) ([d2a9c86](d2a9c86)) * **bls:** reject unknown BLST error codes ([#525](#525)) ([9e4a6ad](9e4a6ad)) * **bls:** size pairing buffers for 32-bit targets ([#531](#531)) ([dc64a27](dc64a27)) * **blst:** default signature infinity check to true if not provided ([#387](#387)) ([021cdcb](021cdcb)) * **build:** remove `zig-out` from `files` ([#360](#360)) ([c52af09](c52af09)) * **ci:** fix caching spec test version ([#439](#439)) ([96885a1](96885a1)) * dangling state pointer in loadOtherState ([#450](#450)) ([81cbd5f](81cbd5f)) * **epoch_cache:** compute missing `next_proposers` ([#447](#447)) ([0088a29](0088a29)) * **epoch_cache:** populate decision roots in afterProcessEpoch ([#453](#453)) ([4b70a5e](4b70a5e)) * export asyncAggregateWithRandomness through napi binding ([#371](#371)) ([1d04c2b](1d04c2b)) * harden memory safety across PMT, SSZ tree views, and state transition ([#377](#377)) ([d6f5897](d6f5897)) * improve atomic ordering in ThreadPool and NAPI init ([#310](#310)) ([4b0a1cc](4b0a1cc)) * interface compatbility with NativeBeaconStateView ([#445](#445)) ([89e13d1](89e13d1)) * missing deinits in loadOtherState ([#459](#459)) ([094d278](094d278)) * missing state commits ([#454](#454)) ([a432b55](a432b55)) * no-op when syncPubkeys run on a pk cache with shrinking validator set ([#432](#432)) ([ed05a99](ed05a99)) * param order in BeaconBlockBody ([#348](#348)) ([d8b9c06](d8b9c06)) * pendingConsolidations bindings ([#449](#449)) ([b9c497e](b9c497e)) * **pmt,ssz:** harden chunked-leaf and zero-copy tree-view memory safety ([#400](#400)) ([de50c53](de50c53)) * populate cache balances during rewards/penalties processing ([#474](#474)) ([5bf23dc](5bf23dc)) * re-expose sizes ([#369](#369)) ([64b81f3](64b81f3)) * remove `slashValidator` gating on active status ([#448](#448)) ([d319a0d](d319a0d)) * **ssz:** drop redundant default-init pass in fixed-list decode ([#468](#468)) ([0c757be](0c757be)) * **ssz:** publish child cache entries after lookup ([#565](#565)) ([21e78c9](21e78c9)) * state transition binding exports ([#456](#456)) ([895982c](895982c)) * **state-transition:** group-check signature sets ([#515](#515)) ([42774e9](42774e9)), closes [#502](#502) * **state-transition:** isolate epoch step cache mutations ([#535](#535)) ([a83741a](a83741a)) * **state-transition:** repair Pool.init call broken by [#346](https://github.com/ChainSafe/lodestar-z/issues/346)×[#367](https://github.com/ChainSafe/lodestar-z/issues/367) merge skew ([#394](#394)) ([b42944f](b42944f)) * various fixes around config ([#433](#433)) ([c4f082c](c4f082c)) ### Performance Improvements * **bindings:** drop TS BLS comparison benches and report benchmarks on PRs ([#552](#552)) ([c909c6f](c909c6f)) * **bls:** add cache-aware signature verifier ([#562](#562)) ([063857e](063857e)) * **bls:** bypass worker queue for small batches ([#553](#553)) ([3f8a6df](3f8a6df)) * **epoch:** replace AutoHashMap with array lookup in reward/penalty caches ([#286](#286)) ([e4e181b](e4e181b)), closes [#243](#243) * **pmt:** chunked-leaf packing for basic lists and container_struct ([#346](#346)) ([ba156c4](ba156c4)) ### Code Refactoring * allocate `AsyncAggRandData` in one obj ([#384](#384)) ([459750f](459750f)) * **bindings/pubkeys:** simplify allocation strategy for aggregate ([#518](#518)) ([b82750f](b82750f)) * **bindings:** rename blst Lifecycle to State ([#516](#516)) ([0a9c179](0a9c179)) * **bindings:** use zapi js.io() instead of local io module ([#469](#469)) ([2b34cc0](2b34cc0)) * **bindings:** wake only required number of workers ([#383](#383)) ([1db57f1](1db57f1)) * **bls:** allocations around VMAS ([#395](#395)) ([dfda58c](dfda58c)) * **bls:** clean up bls ([#398](#398)) ([e0f3b9b](e0f3b9b)) * **bls:** remove need for tracking results for verifyMultipleAggregateSignatures ([#389](#389)) ([6fe5c3f](6fe5c3f)) * **bls:** remove single-threaded fallback ([#390](#390)) ([e057713](e057713)) * **clock:** single public Clock; internalize SlotClock ([#463](#463)) ([fbab1fa](fbab1fa)) * make XXXDecisionRoot fns return `js.String` ([#342](#342)) ([aef4420](aef4420)) * move shuffle into swap_or_not_shuffle module ([#558](#558)) ([e56efb2](e56efb2)) * **pubkeys:** centralize the process-wide cache ([#522](#522)) ([dc9669d](dc9669d)) ### Miscellaneous Chores * avoid slow tests in AGENTS.md ([#546](#546)) ([c60f2a9](c60f2a9)) * bump zapi to include musl build ([#485](#485)) ([0b488cc](0b488cc)) * **ci:** pin github actions with sha hashes ([#507](#507)) ([167b8f5](167b8f5)) * deprecate unused blst APIs ([#575](#575)) ([7b547fa](7b547fa)) * **deps:** bump zapi v2.1.0 -> v2.2.0 ([#376](#376)) ([0c240d8](0c240d8)) * **deps:** bump zbuild ([#403](#403)) ([e2545de](e2545de)) * **deps:** compile blst with ReleaseFast ([#391](#391)) ([753a896](753a896)) * **deps:** update zapi to 3.1.0 ([#483](#483)) ([f3e5827](f3e5827)) * **deps:** use zapi v2.1.0 ([#372](#372)) ([88f403a](88f403a)) * disable gemini auto code review ([#382](#382)) ([63e42a4](63e42a4)), closes [#380](#380) * **docs:** add comments section in AGENTS.md ([#566](#566)) ([0c09750](0c09750)) * move state clones out of benchmark run functions ([#324](#324)) ([e4035de](e4035de)) * prepare 1.0.0 release ([#576](#576)) ([20b657b](20b657b)) * release v0.1.2-rc.3 ([#370](#370)) ([e4fc551](e4fc551)) * **release:** 0.1.2-rc.2 ([#365](#365)) ([7046128](7046128)) * **release:** v0.1.2-rc.10 ([#477](#477)) ([9a4fad5](9a4fad5)) * **release:** v0.1.2-rc.4 ([#373](#373)) ([09468f1](09468f1)) * **release:** v0.1.2-rc.5 ([#374](#374)) ([f344efa](f344efa)) * **release:** v0.1.2-rc.6 ([#375](#375)) ([bdf5b67](bdf5b67)) * **release:** v0.1.2-rc.8 ([#401](#401)) ([06f91c2](06f91c2)) * **release:** v0.1.2-rc.9 ([#404](#404)) ([6024800](6024800)) * remove merge transition code ([#359](#359)) ([09b175d](09b175d)) * remove stale epoch cache TODOs ([#534](#534)) ([27a547a](27a547a)) * rename era shortHistoricalRoot to shortEraRoot ([#473](#473)) ([c75a4d3](c75a4d3)) * **scripts:** build bindings with preset ([#434](#434)) ([a1b5ef7](a1b5ef7)) * silence debug log when used in release builds ([#486](#486)) ([c5377d7](c5377d7)) * support dev workflow ([#364](#364)) ([fcb9a78](fcb9a78)) * update gloas types to align with the latest specs ([#431](#431)) ([1f065b5](1f065b5)) * update spec test version to v1.7.0-alpha.11 ([#451](#451)) ([5875660](5875660)) * update spec-test-version: v1.6.0-beta.2 -> v1.7.0-alpha.10 ([#441](#441)) ([f932b1c](f932b1c)) * update zapi to 4.0.0 ([#571](#571)) ([de8e3fd](de8e3fd)) ### Documentation * document security threat model ([#557](#557)) ([e678b87](e678b87)) * more comprehensive AGENTS.md ([#520](#520)) ([c74b386](c74b386)) * **pkix:** document load provenance requirement ([#556](#556)) ([37e0aa2](37e0aa2)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Depends on ChainSafe/zapi#30 (we need to release zapi and update the dep)
This is one of possible likely causes for increased GC pressure on experiments to swap out blst-ts for lodestar-z/bls, as observed on feat2 and feat3 deployments in this
PR.
With external array buffers, V8 is only aware of the pointer to the backing memory, instead of having to track both the pointer and the backing memory. This means that during marking phase the GC does not have to walk the backing memory to mark it as 'live' - the frequency of the GC firing off is still the same, but each cycle does less work.
This of course comes with a tradeoff, we need a
finalizer to let V8 know how much external memory is in native heap so that the GC tells the native impl to free the useless memory.
Though, regardless of the effect, we should still probably do this anyway, since napi-rs does the same, and only defaults to V8 managed array buffers if it is disallowed (like in Electron).