feat: revive Python binds requiring Python >=3.10, rename to dashbls, repair setup.py to work on Windows, update authorship, drop unmaintained parallel impl, shave down README, add build and publish script - #125
Conversation
It's a problem since Autoconf 2.73
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe project moved to CMake-based Python packaging under ChangesDash BLS packaging and binding migration
Estimated code review effort: 5 (Critical) | ~90+ minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
⛔ Blockers found — Opus deferred (commit fe6fbf7) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (4)
setup.py (1)
1-2: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the
# coding: latin-1declaration from both new files. Python 3 source defaults to UTF-8, so the declaration is obsolete and it actively misdecodes any non-ASCII character added later, for example an accented author name or a typographic dash in a docstring.
setup.py#L1-L2: delete the# coding: latin-1line..github/scripts/build_simple_index.py#L1-L2: delete the# coding: latin-1line.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@setup.py` around lines 1 - 2, Remove the obsolete “# coding: latin-1” declaration from setup.py lines 1-2 and .github/scripts/build_simple_index.py lines 1-2, leaving Python 3’s default UTF-8 source encoding in effect.binds/python/CMakeLists.txt (1)
14-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
${INCLUDE_DIRECTORIES}expands to nothing.
INCLUDE_DIRECTORIESis a directory property, not a variable. The reference on Line 15 is empty unless a same-named variable exists. Remove it, and prefer target-scoped includes ondashbls_py.♻️ Proposed cleanup
-include_directories( - ${INCLUDE_DIRECTORIES} - ${CMAKE_CURRENT_SOURCE_DIR} - ${CMAKE_CURRENT_SOURCE_DIR}/../../include -) - pybind11_add_module(dashbls_py ${CMAKE_CURRENT_SOURCE_DIR}/pythonbindings.cpp) +target_include_directories(dashbls_py PRIVATE + ${CMAKE_CURRENT_SOURCE_DIR} + ${CMAKE_CURRENT_SOURCE_DIR}/../../include +)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@binds/python/CMakeLists.txt` around lines 14 - 18, Remove the empty ${INCLUDE_DIRECTORIES} entry from the include_directories block and configure the dashbls_py target with target-scoped include directories instead, preserving the current source and ../../include paths..github/scripts/build_simple_index.py (1)
55-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider emitting
data-requires-pythonon each anchor.The project declares
requires-python = ">=3.10". Without adata-requires-pythonattribute, pip on Python 3.9 selects the sdist and fails during the build instead of reporting that no compatible distribution exists. The value is available from each release asset's metadata, or it can be a constant that mirrorspyproject.toml.The empty-index guard at Line 72 is a good safeguard against overwriting a working index.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/scripts/build_simple_index.py around lines 55 - 98, Update the distribution anchor generation in main to emit a data-requires-python attribute for every wheel and sdist, using the release asset metadata when available or a constant matching pyproject.toml’s >=3.10 requirement. Preserve the existing asset collection, sorting, and empty-index guard behavior..github/workflows/binds-py.yml (1)
22-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce the duplicated, manually-synced
python-version: '3.10'value.The value is hard-coded three times, each annotated with a comment reminding maintainers to keep it in sync with
.python-version. This is a manual-sync risk: an update to.python-versionwithout updating all three sites silently reintroduces drift.Since
astral-sh/setup-uvdoes not expose apython-version-fileinput (onlyversion-file, which pins theuvtool version, not the Python version), read.python-versiononce per job and pass it through a step output:- name: Read Python version id: pyver run: echo "version=$(cat .python-version)" >> "$GITHUB_OUTPUT" - name: Install uv uses: astral-sh/setup-uv@v5 with: python-version: ${{ steps.pyver.outputs.version }}Please confirm whether a newer
astral-sh/setup-uvrelease has since added native.python-versionfile support before applying this change.Also applies to: 59-60, 89-90
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/binds-py.yml around lines 22 - 23, Update each affected job in the workflow to read .python-version once via a step identified as pyver, expose the value through GITHUB_OUTPUT, and pass steps.pyver.outputs.version to astral-sh/setup-uv instead of hard-coding Python 3.10. First verify whether the current setup-uv release supports a native Python version file input; use that supported input if available, otherwise apply the read-and-output approach consistently to all three occurrences.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/build_simple_index.py:
- Around line 26-40: Update the jq expression in releases to exclude entries
where the release is marked draft before constructing the tag and assets object.
Preserve the existing pagination, asset digest fields, and literal subprocess
argument list.
In @.github/workflows/binds-py.yml:
- Around line 16-17: Disable persisted checkout credentials on every binding
workflow checkout step: add persist-credentials: false to the lint checkout in
.github/workflows/binds-py.yml (lines 16-17), and to the existing fetch-depth
blocks in its build checkout (lines 51-54) and sdist checkout (lines 81-84);
also add it to the checkout steps in .github/workflows/binds-go.yml (lines
21-22), .github/workflows/binds-js.yml (lines 20-21), and
.github/workflows/binds-rs.yml (lines 21-22).
- Around line 136-143: Update the release-upload step’s `run` block to pass
`github.ref_name` through the step environment, then use a quoted shell variable
for the `gh release upload` release argument instead of interpolating the GitHub
expression directly. Preserve the existing wheel and sdist upload behavior.
In @.github/workflows/build-docs.yml:
- Around line 25-32: Update the checkout step to set persist-credentials to
false, then add an actions/setup-python step before Generate index with an
explicit Python version of 3.10 or newer, and keep the existing script
invocation unchanged.
In @.python-version:
- Line 1: Align the Python 3.10 version contract by updating the CI
configuration’s python-version entry to the pinned 3.10.19 value used by
.python-version, or consistently revise the workflow comments and package
metadata to document that patch-level versions may vary.
In `@pyproject.toml`:
- Line 76: In the Ruff lint configuration in pyproject.toml, replace the
deprecated “TCH” selector with “TC” while preserving the existing type-checking
rule configuration.
In `@README.md`:
- Line 87: Update the license code fence in README.md to specify the text
language, or remove the fence while preserving the copyright block content.
- Around line 85-90: Update the package distribution metadata around
pyproject.toml and MANIFEST.in to include a NOTICE or third-party license file
containing the vendored MIT, Apache-2.0, and LGPL-2.1 dependency notices,
alongside the existing root LICENSE. Ensure the file is included in both source
distributions and wheels via the project’s established license-file
configuration.
- Around line 54-55: Update the README dependency references for pytest and ruff
to consistently use the declared development extra name `.[dev]`, correcting any
`[.den]` spelling in the surrounding prose or install command.
- Around line 29-46: Update the README “Build library” instructions to be
cross-platform: replace generator-sensitive parallel build syntax with `cmake
--build . --parallel 4`, provide Windows-compatible executable paths for
`runtest` and `runbench`, and include Windows command or activation equivalents,
or clearly label any remaining Unix-only commands.
In `@setup.py`:
- Around line 104-121: Add a CMake directory-creation step using
file(MAKE_DIRECTORY ...) for the repository build directory before the existing
file(APPEND ...) calls that write include_paths.txt and gmp_libraries.txt. Keep
the current append behavior unchanged and ensure the default in-source
configuration creates the directory before writing.
- Around line 90-102: Update the Windows branch around the generator selection
so non-Visual-Studio generators receive the existing cfg value through
CMAKE_BUILD_TYPE. Keep -A and /m arguments restricted to the Visual Studio
generator path, while preserving the current Windows architecture handling and
multi-config behavior.
---
Nitpick comments:
In @.github/scripts/build_simple_index.py:
- Around line 55-98: Update the distribution anchor generation in main to emit a
data-requires-python attribute for every wheel and sdist, using the release
asset metadata when available or a constant matching pyproject.toml’s >=3.10
requirement. Preserve the existing asset collection, sorting, and empty-index
guard behavior.
In @.github/workflows/binds-py.yml:
- Around line 22-23: Update each affected job in the workflow to read
.python-version once via a step identified as pyver, expose the value through
GITHUB_OUTPUT, and pass steps.pyver.outputs.version to astral-sh/setup-uv
instead of hard-coding Python 3.10. First verify whether the current setup-uv
release supports a native Python version file input; use that supported input if
available, otherwise apply the read-and-output approach consistently to all
three occurrences.
In `@binds/python/CMakeLists.txt`:
- Around line 14-18: Remove the empty ${INCLUDE_DIRECTORIES} entry from the
include_directories block and configure the dashbls_py target with target-scoped
include directories instead, preserving the current source and ../../include
paths.
In `@setup.py`:
- Around line 1-2: Remove the obsolete “# coding: latin-1” declaration from
setup.py lines 1-2 and .github/scripts/build_simple_index.py lines 1-2, leaving
Python 3’s default UTF-8 source encoding in effect.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a143dbb1-bf47-4883-bc02-8f54ac4ead75
📒 Files selected for processing (53)
.flake8.github/scripts/build_simple_index.py.github/workflows/binds-go.yml.github/workflows/binds-js.yml.github/workflows/binds-py.yml.github/workflows/binds-rs.yml.github/workflows/build-binds.yml.github/workflows/build-docs.yml.github/workflows/build-wheels.yml.github/workflows/js-bindings.yml.github/workflows/relic-nightly.yml.gitignore.python-version.vscode/extensions.json.vscode/settings.jsonCMakeLists.txtMANIFEST.inREADME.mdbinds/python/CMakeLists.txtbinds/python/conftest.pybinds/python/pythonbindings.cppbinds/python/samples/aggregate_signatures.pybinds/python/samples/creating_keys_and_signatures.pybinds/python/samples/hd_keys.pybinds/python/samples/loading_from_bytes.pybinds/python/samples/proof_of_possession.pybinds/python/samples/serializing_to_bytes.pybinds/python/samples/tree_aggregates.pybinds/python/test_bench.pybinds/python/test_unit.pydepends/catch2/CMakeLists.txtjs-bindings/CMakeLists.txtlgtm.ymlmypi.inipyproject.tomlpython-bindings/CMakeLists.txtpython-bindings/README.mdpython-bindings/benchmark.pypython-bindings/test.pypython-impl/README.mdpython-impl/bls12381.pypython-impl/ec.pypython-impl/fields.pypython-impl/hash_to_field.pypython-impl/hd_keys.pypython-impl/hkdf.pypython-impl/impl-test.pypython-impl/op_swu_g2.pypython-impl/pairing.pypython-impl/private_key.pypython-impl/schemes.pypython-impl/util.pysetup.py
💤 Files with no reviewable changes (23)
- .flake8
- python-bindings/CMakeLists.txt
- lgtm.yml
- .github/workflows/js-bindings.yml
- python-impl/README.md
- python-bindings/README.md
- .github/workflows/relic-nightly.yml
- python-bindings/test.py
- python-impl/schemes.py
- .github/workflows/build-wheels.yml
- mypi.ini
- python-impl/hash_to_field.py
- python-impl/private_key.py
- python-bindings/benchmark.py
- python-impl/pairing.py
- python-impl/fields.py
- python-impl/op_swu_g2.py
- python-impl/hkdf.py
- python-impl/hd_keys.py
- python-impl/ec.py
- python-impl/util.py
- python-impl/impl-test.py
- python-impl/bls12381.py
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The Python packaging revival exposes three blocking issues that must be fixed before publishing: non-contiguous buffers permit out-of-bounds native reads, the wheel configuration combines a shared RELIC context with pervasive GIL release, and the standalone index cannot build its advertised sdist fallback. The new DST guard is correct for normal inputs but should inspect the Python byte length before making the potentially multi-gigabyte copy it is intended to reject.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `binds/python/pythonbindings.cpp`:
- [BLOCKING] binds/python/pythonbindings.cpp:65-75: Reject non-contiguous buffers before reading native memory
Checking only the format, dimension, and logical element count does not establish that `info.ptr` addresses `info.size` contiguous bytes. A strided view such as `memoryview(bytearray(range(64)))[::2]` is parsed from the first 32 contiguous backing bytes instead of the bytes in the view. A reversed 32-byte view has stride -1 and points at the final backing byte, so the forward `std::copy` reads 31 bytes beyond the allocation. At this head, `PrivateKey.from_bytes(memoryview(bytearray(32))[::-1])` accepted and serialized adjacent heap contents, confirming a native-memory disclosure primitive. The same issue affects the G1, G2, and GT constructors and their `from_bytes`/`from_bytes_unchecked` methods. Require `itemsize == 1`, one dimension, and `strides[0] == 1` at every buffer entry point, or gather the logical bytes according to the supplied stride; add strided and reversed-buffer tests.
- [SUGGESTION] binds/python/pythonbindings.cpp:43-51: Check the DST length before copying its contents
`CopyDst` constructs a `std::string` containing the entire Python bytes object before enforcing the 255-byte limit. A multi-gigabyte value of the kind this guard is specifically intended to reject therefore requires another multi-gigabyte allocation and may raise `MemoryError` instead of the documented `ValueError`. Read the Python object's length first, reject oversized values, and copy only accepted inputs.
In `setup.py`:
- [BLOCKING] setup.py:72-79: Do not release the GIL while RELIC uses a shared context
Passing `-DMULTI=` overrides the root CMake defaults and produces a RELIC build with `MULTI` undefined, where `core_ctx` is a single process-wide static context. The binding nevertheless has 78 `py::gil_scoped_release` sites around parsing, signing, verification, and other RELIC operations, so ordinary CPython threads can concurrently access that shared mutable context. In particular, RELIC operations and `BLS::CheckRelicErrors()` race on `core_ctx->code`; an error from malformed input can be cleared or observed by an unrelated valid verification, making verification nondeterministic and potentially raising `ValueError: Relic library error`. Restore a per-thread RELIC configuration, retain the GIL around every RELIC operation, or serialize all RELIC access with one process-wide native mutex. Skipping free-threaded Python wheels does not prevent races after an explicit GIL release.
In `.github/scripts/build_simple_index.py`:
- [BLOCKING] .github/scripts/build_simple_index.py:63-70: Make source distributions installable through the standalone index
The generated index deliberately includes `.tar.gz` assets as the fallback for platforms without a compatible wheel, but it contains distributions only for `dashbls`. When pip selects an sdist through this index, its isolated PEP 517 build environment uses the same index and cannot resolve the requirements declared in `pyproject.toml`: `cmake`, `pybind11`, `setuptools_scm`, and `setuptools`. Installation therefore fails before compilation begins, for example with no matching distribution for `cmake>=3.18`. Supply build dependencies through the installation design, document a direct-sdist flow that resolves dependencies from the normal package index, or stop listing the sdist as an installable fallback in this standalone index.
A lot of the descriptions are out of date, include example code that cannot be validated as correct API use through CI and has still links to our parent that we've fairly diverged from.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
autogen.sh (1)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the compatibility handling in
autoreconf.Line 12 already invokes
autoheaderthroughautoreconf. On older Autoconf versions, Line 13 therefore reports an unsupported-option error and runsautoheadera second time. Newer Autoconf versions support--replace-handwrittenonautoreconfand pass it toautoheader; Autoconf 2.72 does not document this option. (gnu.googlesource.com)Feature-detect the option on
autoreconfand invokeautoreconfonce.Proposed refactor
autoreconf --install --force --warnings=all -autoheader --force --replace-handwritten || autoheader --force +if autoreconf --help 2>&1 | grep -q 'replace-handwritten'; then + autoreconf --install --force --replace-handwritten --warnings=all +else + autoreconf --install --force --warnings=all +fiVerify the Autoconf versions used by CI:
#!/bin/sh set -eu sh -n autogen.sh command -v autoreconf >/dev/null command -v autoheader >/dev/null autoreconf --version | sed -n '1p' autoheader --version | sed -n '1p' if autoreconf --help 2>&1 | grep -q 'replace-handwritten'; then echo "autoreconf supports --replace-handwritten" else echo "autoreconf requires the fallback path" fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@autogen.sh` at line 13, Update the autogen.sh autoreconf/autoheader flow to feature-detect whether autoreconf supports --replace-handwritten, then invoke autoreconf exactly once with that option when available and without it otherwise. Remove the separate autoheader fallback invocation while preserving compatibility with older Autoconf versions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@setup.py`:
- Around line 106-108: Update the non-Windows build argument construction in
setup.py to use CMake’s generator-independent “--parallel” option with the
existing CPU count, instead of passing “-- -j” directly to the selected
generator. Keep the existing CMAKE_BUILD_TYPE configuration and Windows-specific
behavior unchanged.
---
Nitpick comments:
In `@autogen.sh`:
- Line 13: Update the autogen.sh autoreconf/autoheader flow to feature-detect
whether autoreconf supports --replace-handwritten, then invoke autoreconf
exactly once with that option when available and without it otherwise. Remove
the separate autoheader fallback invocation while preserving compatibility with
older Autoconf versions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b00b5741-f735-41aa-83b4-afdfb889d878
📒 Files selected for processing (55)
.flake8.github/scripts/build_simple_index.py.github/workflows/binds-go.yml.github/workflows/binds-js.yml.github/workflows/binds-py.yml.github/workflows/binds-rs.yml.github/workflows/build-binds.yml.github/workflows/build-docs.yml.github/workflows/build-wheels.yml.github/workflows/js-bindings.yml.github/workflows/relic-nightly.yml.gitignore.python-version.vscode/extensions.json.vscode/settings.jsonCMakeLists.txtMANIFEST.inNOTICEREADME.mdautogen.shbinds/python/CMakeLists.txtbinds/python/conftest.pybinds/python/pythonbindings.cppbinds/python/samples/aggregate_signatures.pybinds/python/samples/creating_keys_and_signatures.pybinds/python/samples/hd_keys.pybinds/python/samples/loading_from_bytes.pybinds/python/samples/proof_of_possession.pybinds/python/samples/serializing_to_bytes.pybinds/python/samples/tree_aggregates.pybinds/python/test_bench.pybinds/python/test_unit.pydepends/catch2/CMakeLists.txtjs-bindings/CMakeLists.txtlgtm.ymlmypi.inipyproject.tomlpython-bindings/CMakeLists.txtpython-bindings/README.mdpython-bindings/benchmark.pypython-bindings/test.pypython-impl/README.mdpython-impl/bls12381.pypython-impl/ec.pypython-impl/fields.pypython-impl/hash_to_field.pypython-impl/hd_keys.pypython-impl/hkdf.pypython-impl/impl-test.pypython-impl/op_swu_g2.pypython-impl/pairing.pypython-impl/private_key.pypython-impl/schemes.pypython-impl/util.pysetup.py
💤 Files with no reviewable changes (23)
- python-impl/pairing.py
- python-bindings/CMakeLists.txt
- .flake8
- python-bindings/README.md
- python-bindings/benchmark.py
- .github/workflows/build-wheels.yml
- python-impl/README.md
- lgtm.yml
- python-impl/ec.py
- python-impl/util.py
- python-impl/private_key.py
- .github/workflows/relic-nightly.yml
- python-impl/impl-test.py
- python-impl/hash_to_field.py
- python-impl/hd_keys.py
- mypi.ini
- python-impl/op_swu_g2.py
- python-impl/bls12381.py
- python-bindings/test.py
- python-impl/schemes.py
- .github/workflows/js-bindings.yml
- python-impl/fields.py
- python-impl/hkdf.py
🚧 Files skipped from review as they are similar to previous changes (25)
- .vscode/extensions.json
- .gitignore
- CMakeLists.txt
- .python-version
- binds/python/samples/creating_keys_and_signatures.py
- .github/workflows/binds-js.yml
- binds/python/samples/proof_of_possession.py
- binds/python/samples/aggregate_signatures.py
- depends/catch2/CMakeLists.txt
- binds/python/samples/hd_keys.py
- binds/python/conftest.py
- .github/workflows/build-binds.yml
- .github/workflows/build-docs.yml
- js-bindings/CMakeLists.txt
- binds/python/CMakeLists.txt
- binds/python/samples/serializing_to_bytes.py
- MANIFEST.in
- binds/python/test_unit.py
- binds/python/samples/tree_aggregates.py
- .github/workflows/binds-go.yml
- binds/python/samples/loading_from_bytes.py
- .vscode/settings.json
- binds/python/pythonbindings.cpp
- pyproject.toml
- .github/workflows/binds-rs.yml
|
pls see 551d801 for some findings by Claude |
|
Thanks — I reviewed The removal of the 78 Two blocking issues from my review are still open in that commit:
One proposed change should not be carried over: removing the The PR head itself has not changed yet; the exact-head automated re-review is still queued. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Latest tip is now |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
binds/python/pythonbindings.cpp (1)
212-242: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winApply the message length bound before all BLS message hash operations.
Messages at or beyond 2 GiB are rejected in
g2_from_messagebyCopyMsg, butsign,verify,aggregate_verify, andfast_aggregate_verifystill convertmsgwithout that check. If the input is larger thanINT_MAX, passing it throughSign/Verify/AggregateVerify/FastAggregateVerifycan still reach relic hash-to-curve functions, becauseCoreMPL::SignandCoreMPL::Verifycast the message size tointbeforeG2Element::FromMessage. UseCopyMsgfor these Python binding message conversions before constructingvector<uint8_t>.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@binds/python/pythonbindings.cpp` around lines 212 - 242, Update the Python binding lambdas for sign, verify, aggregate_verify, and fast_aggregate_verify to convert each message through CopyMsg before constructing vector<uint8_t>. Ensure the existing message length bound is applied before calling BasicSchemeMPL Sign, Verify, AggregateVerify, or FastAggregateVerify, while preserving their current inputs and return behavior.
🧹 Nitpick comments (2)
binds/python/pythonbindings.cpp (1)
47-61: 🧹 Nitpick | 🔵 TrivialNote: a single process-wide mutex serializes all RELIC operations.
RelicGuardcorrectly fixes the shared-context race, but it also means every guarded call (signing, verification, aggregation, element construction) is now fully serialized across all Python threads, sinceMULTIis unset in the RELIC build. This is a deliberate, documented trade-off given the single process-wide RELIC context, and matches the PR's stated scope of prioritizing correctness over feature/performance completeness. Worth keeping in mind if multi-threaded verification throughput becomes a concern later; a per-thread RELIC context would be the eventual path to remove this bottleneck.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@binds/python/pythonbindings.cpp` around lines 47 - 61, No code change is required: retain the process-wide RelicMutex and RelicGuard serialization for correctness with the single shared RELIC context. Treat per-thread contexts as future optimization scope, not part of this change..github/workflows/binds-js.yml (1)
90-94: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftConsider npm trusted publishing (OIDC) instead of
NPM_TOKEN.zizmor flags this
npm publishstep for trusted publishing. npm's OIDC trusted publishing is generally available and removes the need to store, rotate, or risk leaking a long-livedNPM_TOKENsecret.Adopting it requires:
permissions: id-token: writeon this job, npm CLI ≥ 11.5.1 (vianpm install -g npm@latestor a Node version bundling it), and configuring a trusted publisher for this package on npmjs.com pointing at this workflow file.♻️ Proposed direction (requires npmjs.com-side trusted publisher setup first)
- name: Set up Node uses: actions/setup-node@v6 with: node-version: '20' registry-url: https://registry.npmjs.org + + - name: Update npm for trusted publishing + run: npm install -g npm@latestpublish: name: Publish (releases), Javascript if: startsWith(github.ref, 'refs/tags/') needs: build runs-on: ubuntu-latest + permissions: + id-token: write- name: Publish to npm working-directory: js_build/js-bindings - env: - NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} run: npm publish --access public🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/binds-js.yml around lines 90 - 94, Update the Publish to npm job to use npm OIDC trusted publishing instead of secrets.NPM_TOKEN: grant the job id-token: write permission, install or select npm CLI version 11.5.1 or newer before publishing, and remove NODE_AUTH_TOKEN from the step. Preserve the existing npm publish command and configure the package’s trusted publisher to reference this workflow.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 61-64: Update the Python installation documentation in README.md
to target an actually published dashbls distribution: either publish the
configured PEP 503 index with a resolvable dashbls page and required sdist, or
change the documented install command and index URL to match the distribution
currently available. Keep the guidance using --extra-index-url so PyPI remains
available for build dependencies.
---
Outside diff comments:
In `@binds/python/pythonbindings.cpp`:
- Around line 212-242: Update the Python binding lambdas for sign, verify,
aggregate_verify, and fast_aggregate_verify to convert each message through
CopyMsg before constructing vector<uint8_t>. Ensure the existing message length
bound is applied before calling BasicSchemeMPL Sign, Verify, AggregateVerify, or
FastAggregateVerify, while preserving their current inputs and return behavior.
---
Nitpick comments:
In @.github/workflows/binds-js.yml:
- Around line 90-94: Update the Publish to npm job to use npm OIDC trusted
publishing instead of secrets.NPM_TOKEN: grant the job id-token: write
permission, install or select npm CLI version 11.5.1 or newer before publishing,
and remove NODE_AUTH_TOKEN from the step. Preserve the existing npm publish
command and configure the package’s trusted publisher to reference this
workflow.
In `@binds/python/pythonbindings.cpp`:
- Around line 47-61: No code change is required: retain the process-wide
RelicMutex and RelicGuard serialization for correctness with the single shared
RELIC context. Treat per-thread contexts as future optimization scope, not part
of this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fcfab7cc-9fc5-4f5c-ab48-30a8e9a09bad
📒 Files selected for processing (13)
.github/scripts/build_simple_index.py.github/workflows/binds-go.yml.github/workflows/binds-js.yml.github/workflows/binds-py.yml.github/workflows/binds-rs.yml.github/workflows/build-binds.yml.github/workflows/build-docs.yml.gitignoreREADME.mdbinds/python/pythonbindings.cppbinds/python/test_unit.pypyproject.tomlsetup.py
🚧 Files skipped from review as they are similar to previous changes (7)
- .github/workflows/binds-rs.yml
- .github/workflows/build-docs.yml
- .github/workflows/binds-py.yml
- .github/workflows/binds-go.yml
- .gitignore
- pyproject.toml
- binds/python/test_unit.py
|
Reconciled the review against current tip
I replied on each existing PastaClaw thread that had a GitHub comment; GitHub does not grant my account permission to resolve those threads. All current CI checks are green. One blocking issue remains at this tip: The exact-head PastaClaw automated review is still queued. I’ll update the formal preliminary review disposition from that validated result rather than approving while this blocker remains. |
|
The exact-head automated review has now completed for
The remaining non-blocking documentation suggestion is to add PowerShell/Command Prompt activation commands (or label the current activation block POSIX-only). The earlier non-contiguous-buffer, shared RELIC context, standalone-index build-dependency, DST-copy, and CMake parallelism findings are confirmed fixed at this tip. Existing PastaClaw threads were already updated where GitHub allowed replies; this account still cannot resolve them. All CI checks are green. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The revived Python bindings and publishing workflows are substantially improved, but two security blockers remain at exact head: several Python byte inputs still reach RELIC through unchecked size narrowing, and the documented merged-index installation exposes users to dependency confusion while dashbls remains unreserved on PyPI. The Python development instructions also remain POSIX-only despite the documented Windows support.
Source: reviewer backends — gpt-5.6-sol (Codex general) and gpt-5.6-sol (Codex security-auditor); final verifier backend — gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `binds/python/pythonbindings.cpp`:
- [BLOCKING] binds/python/pythonbindings.cpp:171-177: Validate every byte length before RELIC narrows it to int
`CopyMsg` protects only the direct `g2_from_message` and element `from_message` APIs. `Util.hash256` still passes `str.size()` through `Util::Hash256` to RELIC's signed `int` parameter, all three `key_gen` wrappers pass unrestricted seeds through HKDF to `md_hmac`, and the Basic/Aug/Pop `sign`, `verify`, `aggregate_verify`, and `fast_aggregate_verify` wrappers copy unrestricted messages before native hashing. Values above `INT_MAX` therefore undergo implementation-defined narrowing and can be hashed incompletely or interpreted as negative lengths. The key-generation path is especially unsafe because `md_hmac` computes `block_size + in_len`, allocates from that signed result, and then passes `in_len` to `memcpy`; augmented operations also prepend 48 bytes before the final narrowing. Check the Python length before the first copy and use limits appropriate to each path: at most `INT_MAX` for direct hashes, `INT_MAX - 48` for augmented messages, and a bound accounting for the appended key-generation byte and HMAC block arithmetic.
In `README.md`:
- [BLOCKING] README.md:57-64: Avoid dependency confusion when combining the private index with PyPI
`--extra-index-url` does not prioritize the Dash index; pip merges candidates from PyPI and the additional index and selects the best compatible version. The `dashbls` project currently returns 404 from PyPI, so another party can register it and publish a higher compatible version that this documented command will download and execute. Hash fragments attached to Dash-hosted links do not constrain a different candidate selected from PyPI. Reserve and securely control the PyPI project name, use a direct hashed Dash-hosted artifact URL, or redesign installation so the `dashbls` candidate comes exclusively from a Dash-controlled source while build requirements are resolved separately.
- [SUGGESTION] README.md:79-84: Document Windows Python environment activation
The development block still provides only `source dashbls/bin/activate`, which does not work in PowerShell or Command Prompt even though this PR presents the bindings as cross-platform and publishes Windows wheels. Add the `dashbls\Scripts\Activate.ps1` and `dashbls\Scripts\activate.bat` equivalents, or explicitly label the entire development block as POSIX-shell-only.
| ```sh | ||
| pip install --extra-index-url https://dashpay.github.io/bls-signatures/pep503/ dashbls | ||
| ``` | ||
|
|
||
| ## Very fast verification with Proof of Possession scheme | ||
| > [!IMPORTANT] | ||
| > Use `--extra-index-url`, not `--index-url`. Platforms we do not ship a wheel for fall back to the sdist, and building | ||
| > it needs `cmake`, `pybind11`, `setuptools_scm` and `setuptools` from PyPI. `--index-url` would replace PyPI with an | ||
| > index that carries only `dashbls`, so the build would fail before it started. |
There was a problem hiding this comment.
🔴 Blocking: Avoid dependency confusion when combining the private index with PyPI
--extra-index-url does not prioritize the Dash index; pip merges candidates from PyPI and the additional index and selects the best compatible version. The dashbls project currently returns 404 from PyPI, so another party can register it and publish a higher compatible version that this documented command will download and execute. Hash fragments attached to Dash-hosted links do not constrain a different candidate selected from PyPI. Reserve and securely control the PyPI project name, use a direct hashed Dash-hosted artifact URL, or redesign installation so the dashbls candidate comes exclusively from a Dash-controlled source while build requirements are resolved separately.
source: ['codex']
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The prior RELIC integer-narrowing paths are now guarded, but key generation still accepts seeds large enough to exhaust the native stack inside RELIC, and the documented merged-index installation remains vulnerable to dependency confusion while dashbls is unreserved on PyPI. The release pipeline should also pin third-party wheel-building actions to immutable commits before publishing cryptographic binaries.
Source: reviewer backends gpt-5.6-sol (Codex general) and gpt-5.6-sol (Codex security-auditor); final verifier backend gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `binds/python/pythonbindings.cpp`:
- [BLOCKING] binds/python/pythonbindings.cpp:108-112: Prevent large key-generation seeds from exhausting the native stack
`CopySeed` permits seeds up to approximately 2 GiB, but the wheel uses RELIC's default `ALLOC=AUTO`. In that configuration, `RLC_ALLOCA` expands to `alloca`, and `md_hmac` allocates `block_size + in_len` bytes on the native stack. Each scheme's `key_gen` appends one byte and passes the resulting seed through this path, so a seed far below the documented limit terminates the interpreter rather than raising a Python exception. This was reproduced against the exact-head local extension: `BasicSchemeMPL.key_gen(b"x" * (16 << 20))` exited with status 139. Impose a conservative seed-size limit before copying, build RELIC with dynamic allocation for this path, or change HMAC to process accepted inputs without an input-sized stack allocation.
In `.github/workflows/binds-py.yml`:
- [SUGGESTION] .github/workflows/binds-py.yml:59-65: Pin third-party wheel-build actions to immutable commits
The release build executes `astral-sh/setup-uv@v9.0.0` and `pypa/cibuildwheel@v4.1.1` through mutable Git tags, and the resulting wheels are published without an independent source-to-binary verification step. If either upstream action repository is compromised or one of those tags is moved, the action can modify the source or generated cryptographic extension before publication. Pin these third-party build actions to reviewed full commit SHAs and update the pins through reviewed dependency changes.
In `README.md`:
- [BLOCKING] README.md:57-64: Avoid dependency confusion when combining the private index with PyPI
(existing thread: https://github.com/dashpay/bls-signatures/pull/125#discussion_r3753583021)
`--extra-index-url` does not prioritize the Dash index. Pip merges candidates from PyPI and the additional index, then selects the best compatible version. The `dashbls` project still returns HTTP 404 from PyPI at this exact head, so another party can register the name and publish a higher compatible version that users following this command will download and execute. Hash fragments on Dash-hosted links do not constrain a different candidate selected from PyPI. Reserve and securely control the PyPI project name, use a direct hashed Dash-hosted artifact URL, or redesign installation so the `dashbls` candidate comes exclusively from a Dash-controlled source while build requirements are resolved separately.
| - name: Install uv | ||
| uses: astral-sh/setup-uv@v9.0.0 | ||
| with: | ||
| enable-cache: false | ||
|
|
||
| - name: Build and test wheels | ||
| uses: pypa/cibuildwheel@v4.1.1 |
There was a problem hiding this comment.
🟡 Suggestion: Pin third-party wheel-build actions to immutable commits
The release build executes astral-sh/setup-uv@v9.0.0 and pypa/cibuildwheel@v4.1.1 through mutable Git tags, and the resulting wheels are published without an independent source-to-binary verification step. If either upstream action repository is compromised or one of those tags is moved, the action can modify the source or generated cryptographic extension before publication. Pin these third-party build actions to reviewed full commit SHAs and update the pins through reviewed dependency changes.
source: ['codex']
relic reaches for `_umul128`, `_udiv128` and `__lzcnt64` behind a bare `_MSC_VER` guard, which are AMD64-only. Patching relic is out of the question.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The 64 KiB seed cap fixes the prior native stack-exhaustion blocker, and the corresponding boundary tests cover both rejection and the accepted maximum. Two release-security issues remain: the documented merged-index installation permits dependency confusion while dashbls is unclaimed on PyPI, and the wheel workflow executes mutable third-party action tags before publishing the cryptographic extension.
Source: reviewers gpt-5.6-sol (Codex general) and gpt-5.6-sol (Codex security-auditor); final verifier gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `README.md`:
- [BLOCKING] README.md:57-64: Avoid dependency confusion when combining the private index with PyPI
(existing thread: https://github.com/dashpay/bls-signatures/pull/125#discussion_r3753583021)
`--extra-index-url` does not prioritize the Dash-controlled index. Pip merges candidates from PyPI and every additional index, then selects the best compatible version. The `dashbls` project returns HTTP 404 from PyPI at this exact head, so another party can claim the name and publish a higher compatible version that users following this command will download and execute. Hash fragments on Dash-hosted links only verify those specific links; they do not constrain a different candidate selected from PyPI. Reserve and securely control the PyPI project name, use a direct hashed Dash-hosted artifact requirement, or redesign installation so the `dashbls` candidate comes exclusively from a Dash-controlled source while build requirements are resolved separately.
In `.github/workflows/binds-py.yml`:
- [SUGGESTION] .github/workflows/binds-py.yml:59-65: Pin third-party wheel-build actions to immutable commits
(existing thread: https://github.com/dashpay/bls-signatures/pull/125#discussion_r3758700847)
The release build executes `astral-sh/setup-uv@v9.0.0` and `pypa/cibuildwheel@v4.1.1` through mutable Git tags. If either upstream repository is compromised or a tag is moved, the substituted action can alter the source or generated native extension before the publish job distributes it. Because these wheels expose consensus-critical cryptographic operations and there is no independent source-to-binary verification step, pin both third-party actions to reviewed full commit SHAs and update those pins through reviewed dependency changes.
5 participants
Motivation
By and large since forking from upstream the Python binds have been unmaintained as we've had no active consumers for it but work on
base-sdk'spkccrate likedashpay/base-sdk#20and its cousinagora-blsfulare both in requirement of one common thing, the ability to invoke this library as and when new edge cases are discovered when establishing implementation conformity.At least for
base-sdk'spkc, this was done by writing C++ code linked againstdashblsto generate the necessary vectors (see corpus) and then import them butdashpay/base-sdk#20was a result of newer edge cases surfacing as a result of the API being refined and as it revealed a need to extend test vectors, provenance became a sticking issue, the needThis pull request attempts to address this by reviving the Python bindings to allow
dashblsto serve as an oracle both during development and during optimisation work. This pull request does not include updating the binds to be API-complete, the scope is limited to getting to a point where the binds work at all.Additional Information
G1Element and G2Element exposed
__mul__and__rmul__takingbn_t, arelicarray typedef thatpybind11has no type caster for, so every call raised TypeError regardless of the argument and the operators were unreachable from Python. This has been resolved by binding againstconst PrivateKey&.from_message()was bound throughpy::overload_castonto a C++ signature taking a bare constuint8_t*with a separate length argument, a shape pybind11 cannot convert a Python object into, which left the domain separation tag impossible to supply from Python. This has been resolved with an explicit static method takingmsganddstas bytes.reliccaps the domain separation tag at 255 bytes but compares the length as asigned int(source), so a tag >= 2 GiB, passes the check and is then widened back to an enormous length inside the hash. The binds now reject any tag longer than 255 bytes before that cast can happen.Upstream had retired their codebase on Jul 3, 2025 (source) and the contents of our codebase have reasonably diverged to the point that the README says almost nothing about our codebase. As updating references to
blspy(Chia's name for their Python bindings) were part of this PR, it was preferable to rip the bandaid off and rewrite the README to be minimal.The Python-only parallel implementation has bitrotted due to lack of supervision or usage, on that account it has been dropped wholesale. Binds, while posing their own disadvantages, are a facade over the C++ implementation used in Dash Core and serves as a more reliable oracle.
Since bls-signatures#116 the de facto minimum CMake version was 3.18 (Debian
trixieships with 3.31, source) but some files, notably the root definition still used 3.14, which gives off an incorrect impression as attempting to use 3.14 would result in hard errors.To make the package maintainable the set of released wheels reasonable (as every version of Python from minimum supported to tip needs to be built times the number of supported platforms), we have opted to set the minimum required Python to 3.10 (
trixieships with 3.13, source), the same minimum version used by Dash Core (source)The Python-specific
READMEwas dropped in favour of a samples directory that is then included in the unit test suite to prove the API correctness of them in every build.The macOS wheels do not use GMP because Homebrew ships their prebuilt packages targeting the version of macOS requesting the download, since our target version is macOS 14 and the GitHub Runners are on macOS 26, we are unable to generate the wheel. For an oracle, the performance degradation is tolerable.
The CI builds for Linux use
manylinux_2_28as Dash Core requires glibc 2.31 (source) and the closest version without going over that has available builds is targeting glibc 2.28.To allow casual installation without having to guide a user through wheels or placing a package that very likely is liable to be sunset in the not-too-distant-future on PyPi, we generate a PEP 503 index and simply host it using GitHub Pages, installation therefore would look something the command below with the added bonus of being pre-built, so no compilation needed!
Note that we need to update Environments to allow
v*tags or the Pages deployment will fail.microsoft/quicksand, hence their copyright notice.Windows on ARM (
windows-arm64) had to be dropped despite Python itself supporting it since the vendored copy of relic does not properly treat Windows for ARM as a valid target due to its unconditional use of_umul128,_udiv128and__lzcnt64, which are AMD64-specific.As bumping
relicwas decided against indashpay/bls-signatures#93and patchingrelichas a unknown payoff, it was decided to drop the target instead.To allow the binds to build at all
MULTIwas disabled, meaning relic shares a shared global context. This was necessary because MSVC rejects OpenMP'sthreadprivatepragma and glibc rejected thread-local storage blocks at the sizes demanded byrelic. This is only acceptable because we don't offer free-threaded builds and extension modules hold the GIL.Breaking changes
None expected.
How Was This Tested?
Build
Summary by CodeRabbit
New Features
dashblsPython package with CMake-based builds and improved validation.Documentation