Fix int512 literal parsing and CMake package export

[huangminghuang]

Summary

Fixes int512 literal parsing and adjacent int512 operator bugs, corrects the installed and build-tree CMake package exports, and updates CI to use the current Ubuntu runner.

What changed

• Reject non-decimal characters correctly when parsing int512 decimal literals.
• Fix int512_t binary operator| and operator& to return the computed result.
• Fix int512_t::operator>> for whole-limb and mixed limb/bit shifts.
• Add compile-time coverage for >>, |, and & behavior.
• Rename a few typoed local identifiers in src/int512.h and remove a stray semicolon.
• Export installed and build-tree CMake targets as bn256::bn256.
• Load bn256-targets.cmake from bn256-config.cmake and call check_required_components(bn256).
• Consolidate duplicate install(TARGETS bn256 ...) rules into the root CMakeLists.txt.
• Apply BN256_INSTALL_COMPONENT consistently to headers, library, target exports, and package config files.
• Bump the project version to 2.0.2 and sync the README version.
• Update GitHub Actions to use Ubuntu 24.04 instead of Ubuntu 20.04/22.04, and remove the obsolete 20.04 compiler setup step.

Why

The previous int512 literal digit check used an impossible condition, so invalid decimal characters were accepted. Nearby binary bitwise operators also returned the wrong object, and right shifts dropped limbs when shifting by 64-bit boundaries.

The installed CMake package config was also not usable for downstream find_package(bn256) consumers because it included the wrong generated file and did not expose a consistent namespaced target. The install rules had duplicate target installs and partial component handling, so this PR makes the package/export behavior consistent.

Testing

• cmake -S . -B build -DBN256_ENABLE_TEST=ON
• cmake --build build
• ctest --test-dir build --output-on-failure
• cmake --install build --prefix /tmp/bn256-pr3-install
• Component install smoke test with -DBN256_INSTALL_COMPONENT=runtime

Notes

Base branch: master
Head branch: feature/fix-cmake

[heifner]

PR Review: feature/fix-cmake → master

Files changed: 3 (CMakeLists.txt, config.cmake.in, src/int512.h)

Commits: 2

Review date: 2026-05-13

This branch contains two small, surgical fixes plus light reformatting:

1. 7a3eabd — fix logical bug in the _i512 user-defined literal digit validator
2. 0f9ae3c — fix the installed CMake package config so downstream find_package(bn256) actually works

Both fixes look correct and necessary. The interesting things to talk about are:
adjacent pre-existing bugs in the same file, the implications of introducing a namespace
for already-published consumers, and a couple of inconsistencies in the install rules.

---

Critical — likely bugs or consensus risks

This is a math/crypto library (BN256 curve), not blockchain consensus code, so the
"determinism vs. forks" lens doesn't quite apply. But this library is used downstream
inside consensus contexts (it implements pairing arithmetic used in zk-friendly
constructions), so determinism and correctness still matter a lot. I did not find any
new consensus risks introduced by this PR. However:

operator| and operator& are silently broken — pre-existing, but next to the diff

src/int512.h:144-148 and src/int512.h:155-159:

friend constexpr int512_t operator|(const int512_t& a, const int512_t& b) {
   int512_t c{};
   for (int i = 0; i < 8; ++i) { c.limbs_[i] = a.limbs_[i] | b.limbs_[i]; }
   return a;   // <-- should be `c`
}

friend constexpr int512_t operator&(const int512_t& a, const int512_t& b) {
   int512_t c{};
   for (int i = 0; i < 8; ++i) { c.limbs_[i] = a.limbs_[i] & b.limbs_[i]; }
   return a;   // <-- should be `c`
}

Both functions build the correct result in c and then return a. The compound
assignment forms (operator|=, operator&=) are fine — they mutate a in place
and return it. Only the non-assignment binary forms are broken.

A grep of src/ shows neither operator is currently called as a binary form
(r |= digit_value; in parse_heximal uses operator|=, not operator|), so this is
latent — but it's a footgun that will bite the next person who writes a | b. Same
file you're already touching for an unrelated digit-check fix; would be a clean
two-character follow-up.

operator>> drops a limb at multiples of 64

src/int512.h:122-132:

friend constexpr int512_t operator>>(const int512_t& a, int n) {
   int512_t r{};
   auto     limb_offset      = n / 64;
   auto     limb_shfit_count = n % 64;

   for (int i = limb_offset + 1, j = 0; i < 8; ++i, ++j)
      r.limbs_[j] = shr128(a.limbs_[i - 1], a.limbs_[i], limb_shfit_count);

   r.limbs_[7] = (a.limbs_[7 - limb_offset] >> limb_shfit_count);
   return r;
}

For n = 64 (limb_offset = 1): the loop fills r.limbs_[0..5]. The last line
writes to r.limbs_[7] from a.limbs_[6], leaving r.limbs_[6] = 0. The result is
that the high limb is correct, the low six limbs are correct, but limb 6 is dropped.
I suspect the final line should target r.limbs_[7 - limb_offset], not r.limbs_[7].

The current code is saved by usage: unsigned_divmod only ever calls >>= 1, so
limb_offset is always 0 in practice and the bug never fires. Still, I'd flag this
as something to fix or at least add an assertion / comment to.

(operator<< looks correct in the analogous structure.)

---

Worth discussing — potential issues

NAMESPACE bn256:: is an ABI-style break for any existing consumer

CMakeLists.txt:37 — after this change, a downstream find_package(bn256) consumer
must write:

target_link_libraries(my_app PRIVATE bn256::bn256)   # new

…rather than what they may have today:

target_link_libraries(my_app PRIVATE bn256)          # old

That said: the reason this PR exists is that the prior config.cmake.in recursively
included itself (see next finding), so find_package(bn256) couldn't have been
working for any consumer to begin with. So in practice this isn't breaking anyone
who's currently consuming the installed package — there are no working consumers.
But if internal code at Wire (or anywhere downstream of this fork) consumed it via
add_subdirectory and happened to link against the bare bn256 target name, that
keeps working. Worth confirming there isn't a find_package(bn256) call somewhere
that links to the un-namespaced target.

Project version is already at 2.0.1 from prior work; if any downstream had
genuinely worked around the broken config, this would normally warrant a minor
bump. Probably fine, but I'd at least mention it in the PR description.

export(TARGETS bn256 FILE ...) has no NAMESPACE — build-tree export drifts

CMakeLists.txt:57:

export(TARGETS bn256 FILE "${CMAKE_CURRENT_BINARY_DIR}/bn256-targets.cmake")

The install(EXPORT ...) now sets NAMESPACE bn256::, but the build-tree
export(TARGETS ...) does not. Anyone consuming this from the build tree
(find_package(bn256 PATHS <build_dir>) or via CMAKE_PREFIX_PATH pointing at
the build dir) would get a bare bn256 target, while the installed package now
gives bn256::bn256. Suggest adding NAMESPACE bn256:: here too for consistency.

Duplicate install(TARGETS bn256 ...) rules

CMakeLists.txt:29-32 and src/CMakeLists.txt:38-40 both install the bn256
target:

# root CMakeLists.txt:
install(TARGETS bn256
        EXPORT bn256
        LIBRARY
        ARCHIVE)

# src/CMakeLists.txt:
install(TARGETS bn256
        LIBRARY DESTINATION ${CMAKE_INSTALL_FULL_LIBDIR} ${INSTALL_COMPONENT_ARGS}
        ARCHIVE DESTINATION ${CMAKE_INSTALL_FULL_LIBDIR} ${INSTALL_COMPONENT_ARGS})

The root rule is the one wired into EXPORT bn256; the src/ rule duplicates the
install without the EXPORT and adds the INSTALL_COMPONENT_ARGS. This is
pre-existing, but it means:

• Each make install runs two install rules for the same target
• Only one of them honors BN256_INSTALL_COMPONENT (the src/ one)
• Only one of them participates in the export set (the root one)

Probably worth consolidating since this PR is already touching the install/export
chain. Not strictly required for this PR.

No test for the digit-check fix

src/int512.h:271 — the fix is real (the prior digit < '0' && digit > '9' was
tautologically false; no character can be both less than '0' AND greater than '9'
simultaneously, so any character including garbage would pass validation).

But: nothing in the diff exercises the throw "invalid digits" path. The existing
static_assert tests in this file only pass valid numeric literals. A negative
test would need to be a SFINAE or requires check that confirms an
ill-formed literal fails to compile — possible but a bit awkward for this pattern.

Lower-cost option: add a static_assert covering a literal whose character set
is clearly outside [0-9] so a regression would be caught at compile-time. Since
the throw is constexpr-evaluated, a malformed literal will produce a compile error
referencing "invalid digits", so this is testable. Not a must-have, but called
out so it doesn't get forgotten.

throw "invalid digits" of a string literal

Throwing a const char* is unusual style. In the constexpr context it's fine
(the compiler renders the literal as part of the diagnostic), but if this function
ever gets called at runtime (int512_t x = ""_i512; — well, that's still constexpr),
the thrown type would be const char*, which is hard to catch usefully and isn't
caught by catch(const std::exception&). Since operator""_i512() is constexpr
and templated on a parameter pack, runtime invocation isn't really possible, so
this is OK in practice. Worth a comment explaining the intent if anyone revisits.

---

Minor — style and quality

Wrong field name comment? Typo carry-over

src/int512.h:109 and :125 — limb_shfit_count (typo: should be
limb_shift_count). Pre-existing, not in the diff. Not worth fixing if it'd churn
review noise, but if you're already cleaning up this file, two replace_all away.

Project version mismatch

README.md:2 says Version : 2.0, but CMakeLists.txt:3 says
project(bn256 VERSION 2.0.1). Pre-existing. Worth syncing eventually but
outside this PR's scope.

Also I think we need to update that to 2.0.2 for vcpkg to work correctly.

---

Nitpicks

• src/int512.h:218 has a stray semicolon on its own line after the function body
  in operator%:

  return std::get<1>(divmod(a, b));
  ;            // <-- stray
  }

  Pre-existing. Harmless but ugly.

• config.cmake.in is missing a trailing newline check — file ends at line 4
  with check_required_components(bn256). CMake doesn't care, but POSIX text-file
  convention prefers a final newline. (May already be there; I just see line 4 as
  the last numbered line.)

• CMakeLists.txt ordering quirk: write_basic_package_version_file is invoked
  on line 51-55 after install(FILES ... bn256-config-version.cmake ...) on
  lines 45-49. This works because CMake's install(FILES) is a deferred action
  evaluated at install time, by which point the version file has been generated.
  But reading top-down, it looks wrong — the file is installed before it is
  written. Pre-existing, but moving write_basic_package_version_file up would
  improve readability.

• is_negtive (misspelling of is_negative) appears twice at
  src/int512.h:266 and :276. Pre-existing.

• parse_heximal (misspelling of parse_hexadecimal) at src/int512.h:236.
  Pre-existing.

[heifner]

PR Review (follow-up): feature/fix-cmake → master

New commit reviewed: c1f6bcb ("address PR review feedback")

Files changed in this commit: 4 (CMakeLists.txt, README.md, src/CMakeLists.txt, src/int512.h)

Review date: 2026-05-13

This follow-up commit addresses the four discussion points from the first review pass.
Short version: all four are fixed correctly, and three new static_assert tests are
added to lock in the fixes. A few small observations below, none blocking.

---

Critical — likely bugs or consensus risks

None.

---

Worth discussing — potential issues

BN256_INSTALL_COMPONENT is still partial

CMakeLists.txt:30-37 — the INSTALL_COMPONENT_ARGS is applied to the
install(TARGETS bn256 ...) rule, but the other install rules in this file
don't receive it:

• install(DIRECTORY include/ ...) on line 28 — no component args
• install(EXPORT bn256 ...) on lines 39-42 — no component args
• install(FILES bn256-config.cmake bn256-config-version.cmake ...) on lines 50-54 —
  no component args

The practical effect: if a downstream packager builds with
-DBN256_INSTALL_COMPONENT=runtime, they'll get a runtime component containing
only the .a / .so, while the headers and CMake config files land unconditionally in
the default component. Either every install rule should honor the component args,
or the headers/cmake-config should explicitly go in a separate component
(common pattern: runtime for libraries, dev for headers and CMake exports).

Possibly out of scope for this PR, but it's the natural follow-up to consolidating
the install rules into the root CMakeLists.txt. Worth a tracking issue if not
addressed here.

static_assert for >>64 is great — coverage for larger shifts would be stronger

src/int512.h:319 adds:

static_assert((int512_t{ 1, 2, 3, 4, 5, 6, 7, 8 } >> 64) == int512_t{ 2, 3, 4, 5, 6, 7, 8, 0 });

This catches the exact bug that was fixed (limb_offset = 1). But the original bug
generalizes to any limb_offset > 0, so a couple more shifts would be ideal:

• >> 128 (limb_offset = 2, limb_shift_count = 0) — exercises the "drop two limbs" path
• >> 65 (limb_offset = 1, limb_shift_count = 1) — exercises the "shift across limbs while also offsetting" combination
• >> 0 (no-op) — should round-trip the input

None of these are strictly required since the geometry is the same and the fix is
mechanically correct, but a few more lines of static_assert is cheap insurance and
documents the operator's contract. Optional.

Pre-existing: operator<< has UB for n >= 512

src/int512.h:113:

r.limbs_[limb_offset] = (a.limbs_[0] << limb_shfit_count);

If n >= 512, limb_offset >= 8 and we write past the end of the 8-element
limbs_ array. Same on operator>> line 130 (r.limbs_[7 - limb_offset] with
limb_offset = 8 writes to r.limbs_[-1]).

In practice, unsigned_divmod is the only caller, and it bounds the shift to at most
leading_zeros(b) - leading_zeros(a), which is in [0, 511] for the current sizes.
So this is theoretical UB, not exploitable, and isn't introduced by this PR — but
since you're already touching this function, an assert(n < 512) (or
if (n >= 512) return int512_t{};) would catch it cheaply. Or just leave it as a
known precondition. Either way, not a blocker.

---

Minor — style and quality

Switch from CMAKE_INSTALL_FULL_LIBDIR to CMAKE_INSTALL_LIBDIR is a quiet improvement

CMakeLists.txt:36-37:

LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR} ${INSTALL_COMPONENT_ARGS}
ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR} ${INSTALL_COMPONENT_ARGS})

The prior src/CMakeLists.txt used CMAKE_INSTALL_FULL_LIBDIR (absolute path,
ignores CMAKE_INSTALL_PREFIX overrides). Switching to CMAKE_INSTALL_LIBDIR
(relative, respects the prefix at install time) is what the CMake docs recommend
for install(TARGETS). Good change.

include(GNUInstallDirs) moved up before add_subdirectory(src)

CMakeLists.txt:13 — this is now invoked before src/ is added. Currently
src/CMakeLists.txt doesn't reference any CMAKE_INSTALL_* variables, so this
isn't strictly necessary right now, but it's defensive and harmless. If someone
later adds an install rule in src/, this saves a future debugging session. Fine.

export(TARGETS ...) now has the namespace

CMakeLists.txt:62 — NAMESPACE bn256:: is now consistent across both the
build-tree export and the install-tree export. Good.

Version bump 2.0.1 → 2.0.2

CMakeLists.txt:3 and README.md:2 — version bumped and README is now
in sync with CMakeLists.txt. Good catch on both fronts.

Trailing newline scare on src/CMakeLists.txt

The diff showed \ No newline at end of file on the removed-block side, which is
just git telling us the old version was missing a final newline. The new version
does have a trailing newline (verified). Non-issue.

---

Nitpicks

• src/int512.h:109, :125, :131 still spell limb_shfit_count. Pre-existing
  typo, untouched by this PR. Fine to leave; would just churn diff noise.
• src/int512.h:266, :276 still have is_negtive. Same — pre-existing.
• src/int512.h:236 still spells parse_heximal. Same.
• The stray ; on src/int512.h:218 (inside operator%) is still there.
  Pre-existing.
• New static_assert tests use single-character spacing inside the brace
  initializers ({ 1, 2, 3, 4, 5, 6, 7, 8 }) which matches existing style in the
  same testing namespace. Consistent, no change needed.

---

What looks good

• operator>> fix is correct. Swapping r.limbs_[7] = a.limbs_[7 - limb_offset]
  for r.limbs_[7 - limb_offset] = a.limbs_[7] correctly places the high limb
  of the input into the corresponding output slot for any limb_offset.
  Traced through n = 0, 64, 65, 128, 192 — all produce the expected result.
• operator| and operator& now return c as they always should have.
  Trivial fix, but eliminates a latent footgun.
• New static_assert tests lock the fixes in at compile time. Cheap, effective.
• Install rule consolidation removes the duplicate install(TARGETS bn256 ...)
  from src/CMakeLists.txt. Now there's a single source of truth in the root
  CMakeLists.txt, and the INSTALL_COMPONENT_ARGS block is co-located with the
  install rule that uses it.
• Namespace consistency across install(EXPORT ...) and export(TARGETS ...)
  means build-tree and install-tree consumers see the same target name
  (bn256::bn256).
• README ↔ CMakeLists.txt version sync. Tiny thing but easy to forget.
• Commit message "address PR review feedback" is fine; bundling all four review
  points into one commit reads well in the history.

Overall, this is a clean responsiveness pass. From my side, I'd merge as-is. The
BN256_INSTALL_COMPONENT consistency point is the only thing I'd consider
following up on, and it's an existing wart, not something introduced here.