Repository navigation
build: drop the Autotools build and the libsodium allocator - #138
Conversation
The library is built with CMake only; Dash Core compiles the sources with its own build system and does not use this Autotools setup. Remove the configure script, the Makefile fragments, autogen.sh, the m4 macros and the Autotools half of the CI matrix.
No build selects BLSALLOC_SODIUM: CMake compiles with BLSALLOC_MIMALLOC=1 and Dash Core defines no BLSALLOC_* macro, so it already takes the plain malloc/free branch. Remove the sodium branch of BLS::Init(), the Findsodium.cmake module that no CMakeLists references, and the commented-out sodium link lines in the bindings, so the library has no libsodium dependency, optional or otherwise.
|
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 configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Final review complete — no blockers (commit 028dd09) · triage: normal |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The removal of the unused libsodium allocator and Autotools build does not introduce a functional or cryptographic regression in the reviewed range. However, the edited SecAlloc comment still promises paging protection and zeroing that the retained default callbacks do not guarantee; this should be corrected. Native Linux checks have passed, while macOS and several binding/package checks remain queued.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — Removing an entire build system and its CI coverage alongside an unused allocator branch is a cross-cutting build cleanup, but the diff does not make large or intricate changes to cryptography or key handling. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(lane failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer
🤖 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 `include/dashbls/util.hpp`:
- [SUGGESTION] include/dashbls/util.hpp:82-85: Remove unsupported paging and zeroing guarantees from SecAlloc documentation
The comment was modified by this PR but still claims that SecAlloc prevents paging to disk and zeroes memory when freed. SecAlloc and SecFree only invoke the configured callbacks; the retained defaults are mi_malloc/mi_free or malloc/free, and neither implementation provides those guarantees by itself. Document the callback-dependent behavior so callers do not infer libsodium-level protections from the API.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Define explicit secret-memory erasure and paging guarantees — Private-key cleanup still relies on the configured free callback, and the default allocator paths do not explicitly erase or lock sensitive memory. This is pre-existing hardening work rather than a regression introduced by removing the unused sodium branch, but the library should eventually define and enforce its secret-memory protection contract.
- Follow-up: Track separately as a security-hardening issue covering non-optimizable erasure, sensitive derivation buffers, and any required locked-memory behavior.
| /* | ||
| * Securely allocates a portion of memory, using libsodium. This prevents | ||
| * paging to disk, and zeroes out the memory when it's freed. | ||
| * Securely allocates a portion of memory. This prevents paging to disk, | ||
| * and zeroes out the memory when it's freed. | ||
| */ |
There was a problem hiding this comment.
🟡 Suggestion: Remove unsupported paging and zeroing guarantees from SecAlloc documentation
The comment was modified by this PR but still claims that SecAlloc prevents paging to disk and zeroes memory when freed. SecAlloc and SecFree only invoke the configured callbacks; the retained defaults are mi_malloc/mi_free or malloc/free, and neither implementation provides those guarantees by itself. Document the callback-dependent behavior so callers do not infer libsodium-level protections from the API.
| /* | |
| * Securely allocates a portion of memory, using libsodium. This prevents | |
| * paging to disk, and zeroes out the memory when it's freed. | |
| * Securely allocates a portion of memory. This prevents paging to disk, | |
| * and zeroes out the memory when it's freed. | |
| */ | |
| /* | |
| * Allocates memory using the configured allocator callback. | |
| * Paging protection and zeroing on free depend on the installed allocator | |
| * callbacks. | |
| */ |
source: muse-spark-1.3-contributor (phase1-reviewer: general); gpt-6.1-sol (phase2-reviewer: general, security-auditor)
What was done here
Prerequisite for #132, split out on reviewer request so the blst migration does not carry build-system removals.
Dash Core is to integrate bls-signature's cpp files to own build, see dashpay/dash#7678 and it was the only consumer of autoconf's build.
Sodium is removed as it had been never used:
#132 (comment)
build: drop the Autotools build— removesconfigure.ac,Makefile.am, theMakefile.*.includefragments,autogen.sh, the m4 macros and the Autotools half of the CI matrix. The library is built with CMake only; Dash Core compiles the sources with its own build system.build: drop the libsodium secure allocator— removes theBLSALLOC_SODIUMbranch ofBLS::Init(), the unreferencedcmake_modules/Findsodium.cmakeand the commented-out sodium link lines in the Rust and Apple build scripts. No build selected it: CMake definesBLSALLOC_MIMALLOC=1, Dash Core defines noBLSALLOC_*macro and takes themalloc/freebranch.Removal only: 5 insertions, 3562 deletions. No behaviour change.
How it has been tested
CMake build on relic,
runtest: all tests passed (1460 assertions in 23 test cases).git grep -i sodiumandgit grep -i autogenare empty outsidedepends/relic.See also #7678 to test build of Dash Core with new system [could be really used only when #132 is merged].