fix(bb): handle msgpack errors and validate SRS size before allocation - #25487
Merged
Merged
Conversation
AztecBot
added
ci-barretenberg
ludamad
marked this pull request as ready for review
September 11, 2026 22:08
ludamad
requested review from
iakovenkos,
ledwards2225 and
ludamad
as code owners
September 11, 2026 22:08
ludamad
approved these changes
Sep 11, 2026
ludamad
enabled auto-merge (squash)
September 11, 2026 22:08
charlielye
added a commit
that referenced
this pull request
Sep 14, 2026
…e legacy bb::ipc Combined redo of #23612 + #23614 + #23613. Those predate envelope ids (the 8-byte request-id frame prefix), which broke wire compatibility between the old and new stacks, so bb, bb.js and bb-rs have to move together. bb now serves msgpack through ipc-runtime on every transport: `bb msgpack run` maps ""/"-" to a stdio pipe server, .sock to UDS and .shm to shared memory, all dispatching through one generated handler with envelope framing (a plain file keeps the offline bare-frame replay). The command surface comes from a checked-in bb_schema.json in the friendly ipc-codegen dialect, with handlers implemented as thin adapters over the existing domain commands. The typed cbind entrypoint is replaced by the ipc-codegen FFI contract ipc_ffi_entry, so wasm and static-linking consumers speak the same payload as the transports. That makes the legacy machinery consumer-free, so it goes: the in-tree ipc library and its benchmark, the nodejs_module msgpack client (bb.js now uses ipc-runtime's NAPI addon, and nodejs_module only exports LMDBStore), the Command/CommandResponse named unions with their execute dispatcher, and the func_traits/schema reflection behind the CBIND macros. Both client libraries now generate from ipc-codegen rather than forks of it. bb.js/src/cbind (a stale copy of the generator: schema visitor, TS and rust backends, naming, friendly-schema lowering) is deleted in favour of calling ipc-codegen, and barretenberg-rs drops its hand-written Backend trait, error type, Fr/Point types and both backends — codegen emits those, and ipc_runtime::IpcClient plugs in as the Backend, so the crate is ~89% generated with a deprecated BarretenbergApi shim keeping the published surface. The hand-rolled PipeBackend goes with them; its tests now run over ipc-runtime's UDS transport. ipc-codegen gains --strip-type-prefix (bb.js and barretenberg-rs publish unprefixed names while wire tags keep the service prefix) and three rust serde fixes: fq[2] pairs, [u8; N] and above-cutoff byte arrays were encoded as sequences of integers rather than msgpack bin, which the C++ side rejects. ipc-runtime gains the pipe transport (fd pair or stdio, peer EOF requests shutdown), a sync spawned-process backend for callers that cannot await, pre-spawn removal of stale shm segments (they are created O_EXCL, so a killed server's leftovers blocked the next one), and an unref option so a spawned backend cannot hold the Node event loop open while idle. Its rust and zig build files now discover the C++ sources instead of keeping hand-written copies of the CMake list, which had silently drifted. Also fixes two latent breakages from the ts/ -> ts/bb.js/ restructure that blocked wasm and pinned-flow tests: the barretenberg_wasm symlinks and the chonk pinned-inputs repo-root resolution were both one directory short. Raises MAX_FRAME_SIZE from 256 MiB to 1 GiB. bb.js reaches bb over ipc-runtime's socket transport now rather than the deleted nodejs_module msgpack client, so proving requests are newly subject to the frame cap: the rollup circuits exceed it, and bb_prover_full_rollup failed with the server refusing a 314525552-byte frame and the client seeing the closed connection as "write EPIPE". The TS client also checks the outgoing size, so a request over the cap names itself instead of surfacing as an unexplained EPIPE on the next write. Fixes two gaps in unref handling that left the Node event loop open. The shared-memory backends dropped the caller's option -- their new() did not take it and createBackend never passed it -- so unrefStdio was never set and the child's stdout/stderr pipes (which exist whenever a logger is set) kept the loop alive; they thread it through as the socket backend already does. And BarretenbergSync.initSingleton did not force unref: true, unlike its async counterpart, so the sync backend it creates held the loop for the life of the process. Nothing destroys a singleton, so it opts in now too. Together these had a labs pxe test pass all 7 cases and then hang until the job timed out. Rebased onto next, which added two things to the surface this replaces. #25487 hardened the msgpack entrypoints: the generated dispatch already answers a malformed envelope, an unknown command and a throwing handler with an error frame, but an unpack or convert failure still escapes it, so api_msgpack.cpp and ipc_ffi_entry catch that rather than lose the response. Poseidon2AbsorbChain was a new command registered in the NamedUnion this deletes, so it moves to bb_schema.json with a handler alongside the other crypto ones, and its malformed -length test is reexpressed against the FFI entrypoint. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016einpgfthfjLYGwCB3iQqD
charlielye
added a commit
that referenced
this pull request
Sep 14, 2026
…e legacy bb::ipc Combined redo of #23612 + #23614 + #23613. Those predate envelope ids (the 8-byte request-id frame prefix), which broke wire compatibility between the old and new stacks, so bb, bb.js and bb-rs have to move together. bb now serves msgpack through ipc-runtime on every transport: `bb msgpack run` maps ""/"-" to a stdio pipe server, .sock to UDS and .shm to shared memory, all dispatching through one generated handler with envelope framing (a plain file keeps the offline bare-frame replay). The command surface comes from a checked-in bb_schema.json in the friendly ipc-codegen dialect, with handlers implemented as thin adapters over the existing domain commands. The typed cbind entrypoint is replaced by the ipc-codegen FFI contract ipc_ffi_entry, so wasm and static-linking consumers speak the same payload as the transports. That makes the legacy machinery consumer-free, so it goes: the in-tree ipc library and its benchmark, the nodejs_module msgpack client (bb.js now uses ipc-runtime's NAPI addon, and nodejs_module only exports LMDBStore), the Command/CommandResponse named unions with their execute dispatcher, and the func_traits/schema reflection behind the CBIND macros. Both client libraries now generate from ipc-codegen rather than forks of it. bb.js/src/cbind (a stale copy of the generator: schema visitor, TS and rust backends, naming, friendly-schema lowering) is deleted in favour of calling ipc-codegen, and barretenberg-rs drops its hand-written Backend trait, error type, Fr/Point types and both backends — codegen emits those, and ipc_runtime::IpcClient plugs in as the Backend, so the crate is ~89% generated with a deprecated BarretenbergApi shim keeping the published surface. The hand-rolled PipeBackend goes with them; its tests now run over ipc-runtime's UDS transport. ipc-codegen gains --strip-type-prefix (bb.js and barretenberg-rs publish unprefixed names while wire tags keep the service prefix) and three rust serde fixes: fq[2] pairs, [u8; N] and above-cutoff byte arrays were encoded as sequences of integers rather than msgpack bin, which the C++ side rejects. ipc-runtime gains the pipe transport (fd pair or stdio, peer EOF requests shutdown), a sync spawned-process backend for callers that cannot await, pre-spawn removal of stale shm segments (they are created O_EXCL, so a killed server's leftovers blocked the next one), and an unref option so a spawned backend cannot hold the Node event loop open while idle. Its rust and zig build files now discover the C++ sources instead of keeping hand-written copies of the CMake list, which had silently drifted. Also fixes two latent breakages from the ts/ -> ts/bb.js/ restructure that blocked wasm and pinned-flow tests: the barretenberg_wasm symlinks and the chonk pinned-inputs repo-root resolution were both one directory short. Raises MAX_FRAME_SIZE from 256 MiB to 1 GiB. bb.js reaches bb over ipc-runtime's socket transport now rather than the deleted nodejs_module msgpack client, so proving requests are newly subject to the frame cap: the rollup circuits exceed it, and bb_prover_full_rollup failed with the server refusing a 314525552-byte frame and the client seeing the closed connection as "write EPIPE". The TS client also checks the outgoing size, so a request over the cap names itself instead of surfacing as an unexplained EPIPE on the next write. Fixes two gaps in unref handling that left the Node event loop open. The shared-memory backends dropped the caller's option -- their new() did not take it and createBackend never passed it -- so unrefStdio was never set and the child's stdout/stderr pipes (which exist whenever a logger is set) kept the loop alive; they thread it through as the socket backend already does. And BarretenbergSync.initSingleton did not force unref: true, unlike its async counterpart, so the sync backend it creates held the loop for the life of the process. Nothing destroys a singleton, so it opts in now too. Together these had a labs pxe test pass all 7 cases and then hang until the job timed out. Rebased onto next, which added two things to the surface this replaces. #25487 hardened the msgpack entrypoints: the generated dispatch already answers a malformed envelope, an unknown command and a throwing handler with an error frame, but an unpack or convert failure still escapes it, so api_msgpack.cpp and ipc_ffi_entry catch that rather than lose the response. Poseidon2AbsorbChain was a new command registered in the NamedUnion this deletes, so it moves to bb_schema.json with a handler alongside the other crypto ones, and its malformed -length test is reexpressed against the FFI entrypoint. barretenberg/acir_tests is its own yarn project and consumes bb.js through a portal, so it has to resolve bb.js's dependencies itself. bb.js declares @aztec-foundation/ipc-runtime with the placeholder specifier that only a resolution gives meaning to, which barretenberg/ts has and acir_tests did not: `yarn install` there failed with "isn't supported by any available resolver". It gets the same resolution. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016einpgfthfjLYGwCB3iQqD
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The C binding and pipe command loop decode requests outside their command-execution error handling. Wrap decoding, execution and response encoding so exceptions become
ErrorResponse; the C binding preserves the caller's scratch-or-allocate output buffer protocol. Move the existing SRS point-size check before allocating the point vector.Extracts the three implementation changes from koko1123's PR #25475, as requested in the minimal-fix gist. Keeps the existing
BB_NO_EXCEPTIONSbehavior. This code-only extraction omits the original PR's tests, fuzzer, corpus, build wiring and new diagnostic log.Validation: configured the default CMake preset with Clang 20 and
CMAKE_BUILD_TYPE=Debug; compiled all three changed translation units. The C binding and SRS translation units also passed-DBB_NO_EXCEPTIONS -fno-exceptions -fsyntax-only.clang-format-20 --dry-run --Werrorandgit diff --checkpass. No runtime or red/green regression tests were run locally; this checkout had no existing build or test binaries. Barretenberg CI is requested.Created by claudebox · group:
slackbot· requested by ludamad (@ludamad) · Slack thread