From 104acb8f4781913d1c7c3646c649e14752553fec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lex?= Date: Tue, 19 May 2026 14:48:58 +0200 Subject: [PATCH] fix(build): Fix symbol resolution conflicts between circuits (#22) --- .github/resources/witness-generator/Makefile | 24 +++++++- .github/workflows/ci.yml | 4 +- CONTRIBUTING.md | 58 +++++++++++++++++++ rust/Cargo.lock | 8 +++ rust/Cargo.toml | 1 + .../Cargo.toml | 10 ++++ .../src/lib.rs | 22 +++++++ .../tests/conflicts.rs | 24 ++++++++ 8 files changed, 147 insertions(+), 4 deletions(-) create mode 100644 rust/logos-blockchain-circuits-tests/Cargo.toml create mode 100644 rust/logos-blockchain-circuits-tests/src/lib.rs create mode 100644 rust/logos-blockchain-circuits-tests/tests/conflicts.rs diff --git a/.github/resources/witness-generator/Makefile b/.github/resources/witness-generator/Makefile index 5733fb3..227f451 100644 --- a/.github/resources/witness-generator/Makefile +++ b/.github/resources/witness-generator/Makefile @@ -52,15 +52,35 @@ windows: $(BIN) windows-lib: CXXFLAGS=$(CXXFLAGS_COMMON) -fPIC -I/include -Duint="unsigned int" windows-lib: $(LIB) +# Localizes all circuit-specific code to prevent conflicts when multiple circuit +# libraries are linked into the same binary. See CONTRIBUTING.md § "Symbol Isolation". +# On Linux/ELF: llvm-objcopy is required — it clears GRP_COMDAT when localizing +# COMDAT signature symbols, preventing "relocation refers to symbol in discarded +# section" errors that GNU objcopy causes. On Windows/COFF: GNU objcopy suffices +# because COFF COMDAT is per-section (not group-based) and is already deduplicated +# automatically by the linker — the ELF GRP_COMDAT problem does not apply. +PUBLIC_SYMS := $(PROJECT)_generate_witness $(PROJECT)_generate_witness_from_files +LOCAL_OBJ := $(PROJECT)_local.o +OBJCOPY := $(if $(filter windows,$(OS)),objcopy,llvm-objcopy) + +UNAME := $(shell uname -s) + # ---- Rules ---- $(BIN): $(COMMON_OBJS) $(CXX) $(LDFLAGS) $^ $(LDLIBS) -o $@ $(LIB): $(LIB_OBJS) - ar rcs $@ $^ +ifeq ($(UNAME),Darwin) + ar rcs $@ $^ # macOS: two-level namespace, conflicts don't arise +else + ld -r -o $(LOCAL_OBJ) $(filter-out fr.o,$^) + $(OBJCOPY) $(foreach s,$(PUBLIC_SYMS),--keep-global-symbol=$(s)) $(LOCAL_OBJ) + ar rcs $@ fr.o $(LOCAL_OBJ) + rm $(LOCAL_OBJ) +endif %.o: %.cpp $(DEPS_HPP) $(CXX) $(CXXFLAGS) -c $< -o $@ clean: - rm -f $(COMMON_OBJS) $(LIB_ONLY_OBJS) $(BIN) $(LIB) + rm -f $(COMMON_OBJS) $(LIB_ONLY_OBJS) $(BIN) $(LIB) $(LOCAL_OBJ) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4ee863d..eb99f99 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -197,7 +197,7 @@ jobs: run: sudo apt install -y build-essential cmake libgmp-dev libsodium-dev nasm curl m4 - name: Install Dependencies [Witness Generator] - run: sudo apt install nlohmann-json3-dev + run: sudo apt install -y nlohmann-json3-dev llvm - name: Replace Prover Makefile # TODO: Make a fork generate the appropriate Linux Makefile run: cp .github/resources/prover/Makefile rapidsnark/Makefile @@ -434,7 +434,7 @@ jobs: run: sudo apt install -y build-essential cmake libgmp-dev libsodium-dev nasm curl m4 - name: Install Dependencies [Witness Generator] - run: sudo apt install -y nlohmann-json3-dev + run: sudo apt install -y nlohmann-json3-dev llvm - name: Replace Prover Makefile run: cp .github/resources/prover/Makefile rapidsnark/Makefile diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 884bd75..e0d5c5d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -6,6 +6,7 @@ - [Rust](https://rustup.rs/) — the pinned toolchain version is in `rust-toolchain.toml` and will be installed automatically by `rustup`. - [pre-commit](https://pre-commit.com/) — used to run formatting, linting, and audit checks before each commit. +- `llvm-objcopy` — required to build circuit static libraries (symbol isolation) on Linux. Install via `sudo apt install -y llvm`. On Windows (MSYS2), GNU `objcopy` from `mingw-w64-x86_64-toolchain` is used instead (no extra install needed). On macOS, symbol isolation is skipped entirely, so no objcopy tool is required. ### Installing the Pre-Commit Hooks @@ -37,6 +38,63 @@ When bumping the stable toolchain, update `channel` in `rust-toolchain.toml`. Th --- +## Symbol Isolation in Circuit Libraries + +Each circuit (PoQ, PoL, PoC, Signature) is compiled into a static archive (`libpoq.a`, `libpol.a`, etc.). +All four archives share the same internal C++ runtime — `loadCircuit`, `get_size_of_witness`, the `fr_*` field +arithmetic functions, `calcwit_*` functions, and others. They are compiled from the same source files but with +**different constant values per circuit** (e.g. `get_size_of_witness()` returns 18149 for PoQ and 20531 for PoL). + +### The Problem + +When two or more circuit libraries are linked into the same binary, the GNU linker silently picks the first definition +it encounters for each symbol and discards the rest. +No error, no warning. +The result is that one circuit's constants end up hardwired into functions shared by both circuits, corrupting witness +parsing. +In practice: the wrong `get_size_of_witness()` value causes `loadCircuit` to compute an incorrect buffer size, `pu32` +walks off the end of the buffer, reads garbage as a length field, and the subsequent `memcpy` reads past the stack guard +page, which results in a **SIGSEGV**. + +### The Fix + +The Makefile's `$(LIB)` rule uses a two-step process on Linux and Windows to localize all circuit-specific code before +archiving: + +1. **Partial link** (`ld -r`): merges all circuit-specific `.o` files — everything except `fr.o` (pure field +arithmetic, no circuit-specific calls) — into a single relocatable object. No symbols are resolved yet; this is +consolidation only. +2. **Symbol localization** (`llvm-objcopy --keep-global-symbol` on Linux, `objcopy --keep-global-symbol` on Windows): demotes every global symbol to local *except* the +circuit's two public FFI entry points (`$(PROJECT)_generate_witness` and `$(PROJECT)_generate_witness_from_files`). +Local symbols are invisible to the final linker, so each archive retains a private copy of every internal symbol — no +conflict is possible regardless of how many circuits are linked together. + +`llvm-objcopy` is required rather than GNU `objcopy`. GNU `objcopy` only changes the binding of COMDAT signature +symbols to local, which confuses the linker's deduplication logic and causes "relocation refers to symbol in discarded +section" errors. `llvm-objcopy` additionally clears the `GRP_COMDAT` flag on affected section groups, turning them into +regular non-COMDAT sections that are simply kept as-is rather than deduplicated. The result is a slightly larger binary +(each circuit keeps its own copy of shared template instantiations), but no linker errors. + +`fr.o` is excluded from the merge because it contains only field arithmetic (`Fr_*`) with no circuit-specific calls. +It is safe to deduplicate across circuits — the linker picks one copy, which is correct since the code is identical. + +On macOS, localization is skipped. macOS uses a two-level namespace by default, meaning symbols are qualified by which +library they come from, so the conflict does not arise. + +On Windows, GNU `objcopy` (from MinGW binutils) is used instead of `llvm-objcopy`. `llvm-objcopy --keep-global-symbol` +is not supported for COFF objects, but GNU `objcopy --keep-global-symbol` works correctly on COFF — it maps the local +binding to COFF storage class `C_STAT`. The ELF `GRP_COMDAT` problem that required `llvm-objcopy` on Linux does not +apply on Windows: COFF COMDAT is per-section rather than group-based, and the linker already deduplicates it +automatically. + +### Maintenance + +`PUBLIC_SYMS` is hardcoded to `$(PROJECT)_generate_witness` and `$(PROJECT)_generate_witness_from_files` in the +Makefile. If the public FFI API ever changes — entry points renamed or new ones added — update that variable, +otherwise the affected symbols will be localized and linking will fail. + +--- + ## Triggering a New Release for Logos Blockchain Circuits To trigger a release build: diff --git a/rust/Cargo.lock b/rust/Cargo.lock index 597742d..a73e5ad 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -237,6 +237,14 @@ dependencies = [ "logos-blockchain-circuits-types", ] +[[package]] +name = "logos-blockchain-circuits-tests" +version = "0.5.0" +dependencies = [ + "logos-blockchain-circuits-pol-sys", + "logos-blockchain-circuits-poq-sys", +] + [[package]] name = "logos-blockchain-circuits-types" version = "0.5.0" diff --git a/rust/Cargo.toml b/rust/Cargo.toml index ef61e69..4c72d98 100644 --- a/rust/Cargo.toml +++ b/rust/Cargo.toml @@ -15,6 +15,7 @@ members = [ "logos-blockchain-circuits-pol-sys", "logos-blockchain-circuits-poq-sys", "logos-blockchain-circuits-signature-sys", + "logos-blockchain-circuits-tests", "logos-blockchain-circuits-types", "logos-blockchain-circuits-common", ] diff --git a/rust/logos-blockchain-circuits-tests/Cargo.toml b/rust/logos-blockchain-circuits-tests/Cargo.toml new file mode 100644 index 0000000..8f4ac9b --- /dev/null +++ b/rust/logos-blockchain-circuits-tests/Cargo.toml @@ -0,0 +1,10 @@ +[package] +name = "logos-blockchain-circuits-tests" +edition.workspace = true +license.workspace = true +version.workspace = true +publish = false + +[dev-dependencies] +lbc-pol-sys = { workspace = true, features = ["prebuilt"] } +lbc-poq-sys = { workspace = true, features = ["prebuilt"] } diff --git a/rust/logos-blockchain-circuits-tests/src/lib.rs b/rust/logos-blockchain-circuits-tests/src/lib.rs new file mode 100644 index 0000000..5aaee20 --- /dev/null +++ b/rust/logos-blockchain-circuits-tests/src/lib.rs @@ -0,0 +1,22 @@ +pub mod roots { + use std::path::{Path, PathBuf}; + use std::sync::LazyLock; + + pub static TESTS: LazyLock = + LazyLock::new(|| PathBuf::from(env!("CARGO_MANIFEST_DIR"))); + pub static REPOSITORY: LazyLock<&Path> = + LazyLock::new(|| TESTS.parent().expect("Failed to find the repository root.")); + pub static POL: LazyLock = + LazyLock::new(|| REPOSITORY.join("logos-blockchain-circuits-pol-sys")); + pub static POQ: LazyLock = + LazyLock::new(|| REPOSITORY.join("logos-blockchain-circuits-poq-sys")); +} + +pub mod inputs { + use super::roots; + use std::path::PathBuf; + use std::sync::LazyLock; + + pub static POL: LazyLock = LazyLock::new(|| roots::POL.join("sample.input.json")); + pub static POQ: LazyLock = LazyLock::new(|| roots::POQ.join("sample.input.json")); +} diff --git a/rust/logos-blockchain-circuits-tests/tests/conflicts.rs b/rust/logos-blockchain-circuits-tests/tests/conflicts.rs new file mode 100644 index 0000000..131bc91 --- /dev/null +++ b/rust/logos-blockchain-circuits-tests/tests/conflicts.rs @@ -0,0 +1,24 @@ +#[cfg(test)] +mod tests { + use lbc_poq_sys::PoqWitnessInput; + use logos_blockchain_circuits_tests::inputs; + + #[test] + fn test_both_circuits_generate_witness() { + let pol_inputs_raw = std::fs::read_to_string(inputs::POL.as_path()).unwrap(); + let pol_witness_input = lbc_pol_sys::PolWitnessInput::new(pol_inputs_raw).unwrap(); + + // Each sys crate compiles a copy of the same C++ runtime (loadCircuit, get_size_of_witness, + // ...) under identical symbol names. When two crates are linked into the same binary, the + // linker silently keeps one definition of each symbol, so one circuit ends up using the + // other's size constants — corrupting dat parsing and causing a SIGSEGV. + // This test reproduces the conflict by calling generate_witness on both circuits in the + // same binary. + let _pol_witness = lbc_pol_sys::generate_witness(&pol_witness_input); + + let inputs_json_raw = std::fs::read_to_string(inputs::POQ.as_path()).unwrap(); + let inputs_json = PoqWitnessInput::new(inputs_json_raw).unwrap(); + let poq_result = lbc_poq_sys::generate_witness(&inputs_json); + assert!(poq_result.is_ok()); + } +}