[ALPHANET] Quantum

[dangell7]

High Level Overview of Change

Context of Change

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Refactor (non-breaking change that only restructures code)
• Performance (increase or change in throughput and/or latency)
• Tests (you added tests for code that already exists, or your new feature included in this PR)
• Documentation update
• Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
• Release

API Impact

• Public API: New feature (new methods and/or new fields)
• Public API: Breaking change (in general, breaking changes should only impact the next api_version)
• libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
• Peer protocol change (must be backward compatible or bump the peer protocol version)

[sentinel-ai-reviewer]

The Conan recipe revision for secp256k1 changed from b1f450b7f78a36fff75bb6934a356f3a to 481881709eb0bdd0185a12b912bbe8ad while the library version stayed at 0.7.1. A recipe revision change in Conan means the build recipe was modified — different compile flags, applied or removed patches, changed dependency linkage — without bumping the library version. secp256k1 is on the hot path for all XRPL ECDSA transaction signing (SecretKey.cpp secp256k1_ecdsa_sign) and verification (PublicKey.cpp secp256k1_ecdsa_verify), and there is no cmake/deps/secp256k1.cmake to independently control compile flags — the Conan recipe is the sole source of truth for how the library is built. In a PR adding Dilithium alongside secp256k1, an unexplained recipe change to the ECDSA library requires explicit confirmation that no constant-time guarantees or side-channel hardening were removed.

Suggested fix

Diff the XRPLF conan-center-index recipe between revisions b1f450b7f78a36fff75bb6934a356f3a and 481881709eb0bdd0185a12b912bbe8ad for secp256k1/0.7.1. Confirm all compile-time hardening flags are identical or improved. Document the reason for the recipe revision change in the PR description.

[sentinel-ai-reviewer]

gRPC is downgraded three minor versions from 1.81.1 to 1.78.1 without documentation. The XRPL overlay uses gRPC with mutual TLS (GRPC_SSL_REQUEST_AND_REQUIRE_CLIENT_CERTIFICATE_AND_VERIFY in GRPCServer.cpp) and SslServerCredentials for all peer-to-peer validator communication including consensus messages and validator list propagation. A three-minor-version rollback spans months of security patches — HTTP/2 CONTINUATION flood mitigations, certificate chain validation fixes, and TLS renegotiation hardening have all appeared in gRPC minor releases in this range. This rollback should have an explicit justification and a review of the gRPC changelog from 1.78.1 through 1.81.1 confirming no security-relevant fixes are being abandoned.

Suggested fix

Restore grpc/1.81.1 or document the specific build incompatibility requiring the downgrade. Review gRPC security advisories and changelogs for versions 1.79 through 1.81 and confirm no relevant HTTP/2, TLS, or certificate-handling fixes affect XRPL's peer communication paths.

[sentinel-ai-reviewer]

OpenSSL is rolled back from 3.6.3 to 3.6.2 in a PR that specifically modifies the cryptographic signing pipeline to add post-quantum signatures. OpenSSL is the foundation of the entire XRPL security stack here: peer TLS handshakes use SSL_get_finished/SSL_get_peer_finished to derive identity cookies (Handshake.cpp), gRPC uses SslServerCredentials with mutual TLS, RAND_bytes is the primary CSPRNG for all key generation (csprng.cpp), and SHA-256/SHA-512/RIPEMD-160 all run through OpenSSL EVP. Rolling back a patch release of a security library during a crypto-critical PR reintroduces any CVEs fixed in 3.6.3; verify the 3.6.2→3.6.3 security advisory list before merging and restore the higher version unless a documented build incompatibility forces the downgrade.

Suggested fix

Restore openssl/3.6.3 or higher. If a build incompatibility requires 3.6.2, document it explicitly and confirm that no security advisories in 3.6.3 affect XRPL's usage paths: TLS peer connections, RAND_bytes-based key generation, gRPC SslServerCredentials, and EVP digest functions.

[sentinel-ai-reviewer]

The Conan registry URL has been migrated from conan.xrplf.org/repository/conan/ (the XRPL Foundation's Nexus repository) to conan.ripplex.io — a different organizational domain with a completely different URL structure (no explicit repository path, suggesting a different server type). This remote is registered at --index 0, giving it highest resolution priority over all other remotes. This script's purpose is to regenerate the lockfile from scratch, meaning Conan will accept whatever package references this server returns and write them into conan.lock, which all subsequent builds then pin to. If this domain is not under equivalent security controls as the previous XRPL Foundation registry, or if the domain transfer was not formally verified, a compromised or substituted registry could silently poison the lockfile with malicious package revisions that propagate to every developer and CI build.

Suggested fix

Verify that conan.ripplex.io is formally owned and operated by Ripple/XRPLF under the same access controls as the previous registry. Document the migration rationale in the PR. Consider adding a checksum or signature verification step after lockfile regeneration, and ensure the new server enforces the same recipe patching guarantees that motivated the --index 0 priority (as noted in the comment above the command).

[sentinel-ai-reviewer]

The condition compiler_version < 13 compares a string against an integer literal. compiler_version is populated from detect_api.detect_default_compiler() and detect_api.default_compiler_version(), both of which return plain Python strings in Conan 2.x — confirmed by the lockfile profiles (e.g. compiler.version=13). On any GCC host, compiler == "gcc" passes and Jinja2 evaluates the str-vs-int comparison, which raises TypeError in Python 3 and causes the entire profile to fail to render. The -Wno-restrict suppression that was intended to paper over a GCC < 13 false-positive therefore never reaches the compilers that need it, and worse, the profile is broken for all GCC users. Fix by quoting the literal (compiler_version < "13") for a lexicographic comparison — this is safe for two-digit GCC major versions — or use compiler_version | int < 13 in Jinja2 for explicit integer conversion.

Suggested fix

Change the comparison to use a string literal: {% if compiler == 'gcc' and compiler_version < '13' %}. Alternatively use Jinja2's int filter: {% if compiler == 'gcc' and (compiler_version | int) < 13 %}. Validate by running conan profile show --profile default on a GCC 12 and GCC 13 system before merging.

[sentinel-ai-reviewer]

The dilithium library is fetched from https://github.com/Transia-RnD/dilithium.git, a private Transia-RnD fork of the NIST reference implementation, with no content integrity verification — no URL_HASH SHA256 or equivalent. Only a git commit SHA pins the dependency, which provides no protection if the fork repository is force-pushed, if the GitHub account is compromised, or if the CI environment DNS-spoofs the host. This library is linked unconditionally into xrpl_libs, making it a hard dependency of every rippled node's post-quantum key generation and signing path — a supply-chain compromise silently backdoors all Dilithium validator key material across every build. Replace the GIT_REPOSITORY fetch with a URL pointing to a pinned release tarball from the upstream pq-crystals/dilithium reference repository, combined with URL_HASH SHA256=<hash> verification, or vendor the code as a reviewed git submodule.

Suggested fix

Use FetchContent or ExternalProject_Add with a pinned tarball URL from the upstream pq-crystals/dilithium official repository and a URL_HASH SHA256=... computed out-of-band against that specific release. Alternatively, vendor the code as a git submodule with the commit pinned in .gitmodules so every clone has an auditable history. Add a post-build KAT step to verify the compiled artifact produces correct known-answer test outputs before linking it into xrpl_libs.

[sentinel-ai-reviewer]

The Dilithium library is compiled with -DDILITHIUM_MODE=2 -DDILITHIUM_RANDOMIZED_SIGNING via raw CFLAGS inside the ExternalProject_Add build command, but these definitions are never propagated to CMake consumers via target_compile_definitions(... INTERFACE ...) on dilithium::dilithium2_ref. Adding NIH::dilithium2_ref to xrpl.imports.main as INTERFACE places the Dilithium ref/ headers on every downstream target's include path without the matching compile definitions — any translation unit that includes params.h without DILITHIUM_MODE=2 sees the Mode 3 defaults (CRYPTO_PUBLICKEYBYTES=1952, CRYPTO_SECRETKEYBYTES=4000), while the compiled archive only produces 1312-byte keys; the #ifndef CRYPTO_PUBLICKEYBYTES fallback guards in SecretKey.cpp and PublicKey.cpp are dead because params.h is explicitly #included first and defines the macro unconditionally. The fix is to add target_compile_definitions(dilithium::dilithium2_ref INTERFACE DILITHIUM_MODE=2 DILITHIUM_RANDOMIZED_SIGNING) in cmake/deps/dilithium.cmake and add a static_assert(CRYPTO_PUBLICKEYBYTES == 1312) guard at each Dilithium inclusion site.

Suggested fix

In cmake/deps/dilithium.cmake, after set_target_properties(dilithium::dilithium2_ref ...), add: target_compile_definitions(dilithium::dilithium2_ref INTERFACE DILITHIUM_MODE=2 DILITHIUM_RANDOMIZED_SIGNING). Add static_assert(CRYPTO_PUBLICKEYBYTES == 1312 && CRYPTO_SECRETKEYBYTES == 2528) in SecretKey.cpp and PublicKey.cpp immediately after the Dilithium headers are included to catch any future mode drift at compile time.

[sentinel-ai-reviewer]

The BUILD_COMMAND assigns CFLAGS via CFLAGS='...' — a bare environment-variable assignment — containing only the two preprocessor defines. This clobbers any inherited flags before invoking make, so the Dilithium reference library is compiled without rippled's own -fstack-protector (set via XrplCompiler.cmake on the common INTERFACE) or CMake's -fPIC (from CMAKE_POSITION_INDEPENDENT_CODE=ON), nor would sanitizer flags from the conan/profiles/sanitizers profile ever reach it. Because verifyDigest() in PublicKey.cpp passes peer-supplied signature bytes directly into this library and it ships inside the final xrpld binary, any latent buffer-handling bug in the reference implementation lacks the stack-canary protection that every other rippled translation unit receives. Append the parent flags rather than replacing them: CFLAGS='${CMAKE_C_FLAGS} -DDILITHIUM_MODE=2 -DDILITHIUM_RANDOMIZED_SIGNING'.

Suggested fix

Change the CFLAGS assignment to CFLAGS='${CMAKE_C_FLAGS} -DDILITHIUM_MODE=2 -DDILITHIUM_RANDOMIZED_SIGNING' inside the /bin/sh -c invocation, and also pass -fstack-protector explicitly if CMAKE_C_FLAGS does not include it in all build configurations. This ensures the Dilithium build inherits the same stack protection and PIC/PIC settings as the rest of rippled.

[sentinel-ai-reviewer]

The featureQuantum amendment is declared in features.macro but has zero references anywhere in the production codebase — no view.rules().enabled(featureQuantum) guard exists in transaction signing, manifest parsing, peer handshake verification, or consensus validation. Adding KeyType::Dilithium as a parseable and recognized type makes Dilithium acceptance unconditionally active on every network: a peer submitting a transaction with a 1312-byte Dilithium public key, a validator manifest signed with a Dilithium master key, or a Dilithium-signed consensus proposal will be cryptographically verified and accepted by any node running this build, regardless of whether the amendment has been voted in. Unpatched nodes will reject the same data, causing irreconcilable ledger divergence.

Suggested fix

Wire every Dilithium acceptance point to view.rules().enabled(featureQuantum) before this key type is exposed. Required gate locations: publicKeyType() in PublicKey.cpp, Transactor::checkSign(), manifest deserialization in Manifest.cpp, peer handshake in Handshake.cpp, and STValidation signature verification. Until those gates are in place, keyTypeFromString must not return KeyType::Dilithium on production builds.

[sentinel-ai-reviewer]

Adding 'dilithium' as a parseable key_type string creates a remote process-termination vulnerability. channel_authorize is a publicly accessible RPC method (Role::USER) that routes key_type through keyTypeFromString into keypairForSignature, which calls logicError — implemented as std::abort() — when it encounters KeyType::Dilithium, because its allowlist is still restricted to Secp256k1 and Ed25519. Before this diff, keyTypeFromString('dilithium') returned an empty optional, causing keypairForSignature to return a proper RPC error response; now it returns KeyType::Dilithium, which passes the early parse check and reaches the fatal assertion. Any unauthenticated client can crash a publicly-reachable rippled node with a single JSON-RPC request.

Suggested fix

Do not add 'dilithium' to keyTypeFromString until all consumers — specifically keypairForSignature in RPCHelpers.cpp — are updated to handle it as a user-facing error rather than a logic invariant violation. In the interim, change the guard at RPCHelpers.cpp:338 to return an RPC error code instead of calling logicError for key types it does not yet support.

[sentinel-ai-reviewer]

The constructor lower-bounds slice.size() against kSize (33) but has no upper-bound check against sizeof(buf_) (1312) before the unconditional std::memcpy(buf_, slice.data(), size_). Safety today depends entirely on publicKeyType only recognising slices of exactly 33 or 1312 bytes; the memcpy itself has no independent guard. If a future post-quantum variant (e.g. Dilithium3 at 1952 bytes) is added to publicKeyType without also expanding buf_, the copy silently overflows the stack buffer, corrupting adjacent frames. The fix is a single explicit guard inserted before the memcpy: if (slice.size() > sizeof(buf_)) logicError("PublicKey::PublicKey - Input slice exceeds buffer");.

Suggested fix

In PublicKey.cpp, add if (slice.size() > sizeof(buf_)) logicError("PublicKey::PublicKey - Input slice exceeds buffer"); immediately after the existing lower-bound check and before the publicKeyType() call. This makes the safety invariant explicit and locally enforced rather than depending on publicKeyType() remaining the sole gatekeeper.

[sentinel-ai-reviewer]

The comment added here explicitly frames Dilithium acceptance as a post-amendment behavior during a rolling upgrade, but featureQuantum — declared in features.macro with VoteBehavior::DefaultNo — is never consulted anywhere in the codebase (zero references in src/). Upgraded nodes unconditionally accept and relay Dilithium-keyed validations from peers the moment this code ships, regardless of whether the Quantum amendment has activated across the network. During a mixed deployment, a validator that switches to a Dilithium key before the amendment clears the required support threshold will have its votes counted by upgraded nodes but discarded as unparseable by non-upgraded nodes, creating divergent trusted-validator-set views that can contribute to a consensus split without any formal network vote. The fix belongs at the deserialization call site in PeerImp::onMessage(TMValidation): check that featureQuantum is active in the current ledger before constructing an STValidation from a peer message carrying a Dilithium-sized signing key.

Suggested fix

Gate Dilithium validation acceptance on featureQuantum activation. Since the deserialization constructor has no access to ledger rules, add the check at the call site in PeerImp::onMessage(TMValidation) before constructing the STValidation object: if the raw signing public key field is 1312 bytes (Dilithium) and featureQuantum is not yet active in the current ledger, discard the message and charge the peer a fee as with any other malformed message. Alternatively, pass a std::optional<Rules> into the constructor and reject KeyType::Dilithium when the amendment is absent. featureQuantum is already declared — it just needs to be wired into the enforcement path.

[sentinel-ai-reviewer]

The diff comment explicitly describes this as a 'pre-amendment / post-amendment' change, yet the Quantum amendment declared in features.macro is a dead symbol — featureQuantum is never referenced anywhere in the codebase. Both constructors, isValid(), PeerImp::checkValidation, PeerImp::onMessage(TMValidation), and handleNewValidation in RCLValidations.cpp all accept Dilithium-signed validations unconditionally, with no rules().enabled(featureQuantum) gate anywhere in the chain. This means the Dilithium upgrade is not coordinated through XRPL's amendment mechanism at all: the moment a validator switches to a Dilithium key, its validations are accepted and counted toward consensus by all upgraded peers regardless of whether the network has voted in the Quantum amendment. Either wire up featureQuantum checks in the validation acceptance path (or document explicitly that the upgrade is intentionally ungated at the consensus layer), and remove the misleading 'post-amendment' language from the comments.

Suggested fix

Either (a) generate the featureQuantum symbol from features.macro and add rules().enabled(featureQuantum) gates in the deserialization constructor and/or in PeerImp::checkValidation / handleNewValidation before counting Dilithium validations toward consensus, or (b) remove the Quantum entry from features.macro and update the comments to accurately reflect that Dilithium support is a wire-protocol-level change not gated by an amendment.

[sentinel-ai-reviewer]

The BUILD_COMMAND block uses multiple COMMAND sub-keywords to sequence three steps, but ExternalProject_Add only gained support for that syntax in CMake 3.19. The project declares cmake_minimum_required(VERSION 3.16) in CMakeLists.txt, so any developer or CI runner on CMake 3.16–3.18 will have the literal string COMMAND treated as the executable name — causing the external project build step to fail immediately and leaving the dilithium static libraries unbuilt. The fix is to either raise cmake_minimum_required to VERSION 3.19, or collapse the three build steps into a single /bin/sh -c invocation chaining them with &&.

Suggested fix

Either raise cmake_minimum_required(VERSION 3.19) in CMakeLists.txt (CMakeLists.txt:1), or rewrite BUILD_COMMAND as a single shell invocation: BUILD_COMMAND /bin/sh -c "${CMAKE_COMMAND} -E copy_directory <SOURCE_DIR>/ref <BINARY_DIR>/ref && make -C <BINARY_DIR>/ref clean && CFLAGS='-DDILITHIUM_MODE=2 -DDILITHIUM_RANDOMIZED_SIGNING' make -C <BINARY_DIR>/ref libdilithium2_ref.a libfips202_ref.a"

[sentinel-ai-reviewer]

include(FetchContent) at the top of the file loads the FetchContent module but no FetchContent_Declare, FetchContent_MakeAvailable, or any other FetchContent_* function is ever called anywhere in this file — the file uses ExternalProject_Add exclusively. FetchContent is already pulled in by the parent CMakeLists.txt before this file is included, making this doubly redundant. Remove the line to avoid misleading readers into thinking this file uses the FetchContent-based dependency model.

Suggested fix

Delete the include(FetchContent) line entirely; it has no effect and signals an intent (FetchContent-based dependency) that this file does not implement.

[sentinel-ai-reviewer]

The signing constructor now permits a local validator node to sign validations with a Dilithium key with no check that the Quantum amendment is active on the network. A validator operator who rotates to a Dilithium signing key before the amendment reaches quorum will produce validations that upgraded peers accept and non-upgraded peers discard, contributing directly to the same consensus-split risk as the deserialization-side change. The logicError guard is correct in principle — it simply needs to enforce Dilithium only after the amendment activates — but RCLConsensus::onClose calls this constructor without any amendment check either, so the gate must be inside the constructor. Until the amendment is active, Dilithium should trigger logicError the same way Ed25519 does today.

Suggested fix

Before calling signDigest with a Dilithium key, verify featureQuantum is active in the current ledger Rules. Pass the Rules reference into the signing constructor or enforce the check at the RCLConsensus.cpp call site where this constructor is invoked. If the amendment is not yet active, reject the Dilithium key and fall back to the configured secp256k1 key, or log a fatal error and halt signing to prevent the node from contributing to a consensus split.

[sentinel-ai-reviewer]

The header comment says Dilithium secret keys are 2528 bytes, matching the NIST Dilithium2 standard, but the buffer is declared as 2560 bytes and the new array constructor takes a 2560-element array. The Slice constructor in SecretKey.cpp hard-codes the accepted sizes as exactly 32 or 2560 — any other size calls logicError(). This means if CRYPTO_SECRETKEYBYTES ever resolves to its NIST-standard value of 2528 (e.g., if the pinned Transia-RnD fork is updated to align with the standard), every call to randomSecretKey(KeyType::Dilithium) and generateSecretKey(KeyType::Dilithium) will abort the process. If the fork genuinely produces 2560-byte keys, the inaccurate comment masks a deliberate and undocumented deviation from NIST ML-DSA-44 that makes XRPL Dilithium secret keys incompatible with any external Dilithium2 tooling — and toBase58()/parseBase58() will silently encode/expect 32 bytes more than the standard format. The buffer size and the Slice guard should both be derived from a single CRYPTO_SECRETKEYBYTES constant with a static_assert tying it to the expected value, and the comment should document why 2560 rather than 2528 is used.

Suggested fix

Replace the raw literal 2560 with CRYPTO_SECRETKEYBYTES wherever used (buf_ declaration, array constructor, Slice validation guard in SecretKey.cpp, Manifest.cpp key-size check, parseBase58 gate). Add a static_assert(CRYPTO_SECRETKEYBYTES == , ...) so any library version drift is caught at compile time. Correct the comment to reflect the actual fork-defined size, or add a comment explaining the 32-byte difference from the NIST standard.

[sentinel-ai-reviewer]

The signing constructor accepts Dilithium keys and will sign and broadcast validations with them unconditionally, with no check that featureQuantum is active. The call site in RCLConsensus::Adaptor::validate has access to the current ledger's rules but does not consult them before passing a Dilithium key into this constructor. A validator operator who sets [validator_key_type]=dilithium in their config will immediately begin emitting Dilithium-signed validations to all peers, regardless of whether the Quantum amendment has been voted in by the network. Non-upgraded peers will discard these validations, silently reducing the operator's effective vote weight and potentially splitting the trusted-validator-set views across the network. The fix must live at the RCLConsensus::Adaptor::validate call site, where ledger.ledger->rules() is already accessible.

Suggested fix

Reject Dilithium keys in ValidatorKeys::load() (ValidatorKeys.cpp:32-43) unless featureQuantum is currently active on the network. The signing constructor cannot access ledger rules, so the gate must live in the caller. Alternatively, RCLConsensus::Adaptor::validate() should check rules().enabled(featureQuantum) before passing a Dilithium key to the STValidation constructor, falling back to the node's secp256k1 master key when the amendment is inactive.

[sentinel-ai-reviewer]

The comment says 'Dilithium secret keys are 2528 bytes' — the CRYSTALS-Dilithium round-3 value — but the Slice constructor (confirmed in SecretKey.cpp) hard-codes acceptance of only 32 or 2560 bytes, and both generateSecretKey(KeyType::Dilithium, ...) and randomSecretKey(KeyType::Dilithium) construct SecretKey via Slice{buf, CRYPTO_SECRETKEYBYTES}. If the linked library uses CRYPTO_SECRETKEYBYTES=2528 (round-3) rather than 2560 (ML-DSA / FIPS 204), every Dilithium key generation call terminates the node via logicError with no compile-time diagnostic. The current fork produces 2560-byte secrets so the code works today, but the comment actively documents the wrong value and would guide any future maintainer toward the wrong constant. Fix the comment to say '2560 bytes' and add a static_assert(CRYPTO_SECRETKEYBYTES == 2560, ...) in SecretKey.cpp to make a library mismatch a compile error rather than a runtime crash.

Suggested fix

Change the comment to '2560 bytes' and add static_assert(CRYPTO_SECRETKEYBYTES == 2560, "Dilithium secret key size mismatch — Slice constructor and buffer require exactly 2560"); in SecretKey.cpp immediately after the CRYPTO_SECRETKEYBYTES #define.

[sentinel-ai-reviewer]

The new Dilithium branch (else if (*type == KeyType::Dilithium)) is syntactically attached as the else clause of the Ed25519 if block rather than as an independent parallel branch. Today this is accidentally safe because both paths through the Ed25519 block unconditionally return — the non-canonical early-return path calls return false and the success path returns ed25519_sign_open(...) == 0, so the else if is unreachable for Ed25519 keys in all current inputs. However, the structure misrepresents the design intent: Dilithium should be a sibling dispatch arm alongside Secp256k1 and Ed25519, not a subordinate of Ed25519. Any refactor that adds a non-returning code path to the Ed25519 block (e.g., a logging branch or early continue) would silently cause Dilithium transactions signed with an Ed25519 key's public key to attempt Dilithium verification rather than failing immediately. Change else if (*type == KeyType::Dilithium) to a standalone if (*type == KeyType::Dilithium) — or, better, restructure all three branches into if / else if / else if — to make the dispatch explicit and resistant to future edits.

Suggested fix

Add a featureQuantum amendment check in STTx::checkSign() before accepting a KeyType::Dilithium signing key — returning temBAD_SIGNATURE or tefBAD_AUTH if the amendment is not active. Since checkSingleSign() does not currently receive a Rules argument, thread the Rules parameter through checkSign(Rules) → checkSingleSign(Rules, sigObject) → singleSignHelper(Rules, sigObject, data), and gate the Dilithium branch on rules.enabled(featureQuantum).

[sentinel-ai-reviewer]

Dilithium public keys are identified solely by byte length (1312 bytes) with no prefix byte, unlike Ed25519 which requires the first byte to be 0xED and secp256k1 which requires 0x02 or 0x03. Any 1312-byte blob presented as sfSigningPubKey in a transaction is unconditionally classified as KeyType::Dilithium and its raw bytes are forwarded directly to pqcrystals_dilithium2_ref_verify in the external Dilithium library fork. This exposes the full lattice polynomial deserialization and verification logic in an external, minimally-audited library to arbitrary attacker-controlled input with no pre-validation, and also creates a fragile disambiguation scheme that breaks the moment any second 1312-byte key type is introduced.

Suggested fix

Assign Dilithium keys a reserved prefix byte (e.g., 0xDF or another byte not used by secp256k1/Ed25519/P256) and enforce it in publicKeyType(): if (slice.size() == 1 + CRYPTO_PUBLICKEYBYTES && slice[0] == kDilithiumPrefix) return KeyType::Dilithium;. Update key generation and serialization accordingly. This eliminates size-only ambiguity, protects the external library from arbitrary input, and matches the defensive pattern used by all existing key types.

[sentinel-ai-reviewer]

This function unpacks the full Dilithium secret key — extracting private polynomials s1, s2, t0, and the signing seed (key) into stack locals — then returns without calling secureErase on any of them. Contrast with pqcrystals_dilithium2_ref_keypair_seed directly above it, which diligently erases every sensitive intermediate (s1, s1hat, s2, t0, seedbuf) before returning. This function is called on every derivePublicKey(KeyType::Dilithium, sk) invocation — key generation, manifest signing, validator rotation — leaving the private polynomial coefficients and seed material as stack residue after each call. Add secureErase calls for seedbuf, s1, s1hat, s2, and t0 before the return, matching the erasure discipline in the sibling function.

Suggested fix

Add secureErase(seedbuf, sizeof(seedbuf)); secureErase((void*)&s1, sizeof(s1)); secureErase((void*)&s1hat, sizeof(s1hat)); secureErase((void*)&s2, sizeof(s2)); secureErase((void*)&t0, sizeof(t0)); before the return statement, mirroring the cleanup in pqcrystals_dilithium2_ref_keypair_seed.

[sentinel-ai-reviewer]

The PR extends parseBase58<SecretKey> to accept 2560-byte Dilithium keys, but decodeBase58Token returns the decoded bytes in a plain std::string that is never securely wiped before the function returns. For Dilithium keys at 2560 bytes, this is unambiguously a heap allocation — the full private key material lingers in a freed heap block after the string destructs. The 32-byte secp256k1/ed25519 case has the same pattern but uses small-buffer optimization on most implementations; at 2560 bytes there is no such mitigation. The decoded bytes should be moved directly into the SecretKey constructor and the source string wiped, or decodeBase58Token should accept a caller-supplied secure buffer.

Suggested fix

After constructing the SecretKey from the decoded result, call secureErase on the underlying string data before the function returns (e.g., if result has a data() pointer, call secureErase(const_cast<char*>(result.data()), result.size()) before returning). Alternatively, refactor decodeBase58Token to write into a caller-provided SecureBuffer to avoid ever holding plaintext key material in an unmanaged std::string.

[sentinel-ai-reviewer]

Key generation uses a locally-defined expand_mat to build the Dilithium matrix from rho, while the sibling public-key recovery function pqcrystals_dilithium2_ref_publickey calls the upstream library's polyvec_matrix_expand for the identical computation. Both code paths must expand to the same matrix A from the same rho — if they ever diverge, derivePublicKey returns a public key that does not correspond to any signature the secret key can produce, silently breaking every seed-derived Dilithium key pair with no observable error. This is compounded by a second difference: pqcrystals_dilithium2_ref_keypair_seed implements the matrix-vector product as a manual per-row loop while pqcrystals_dilithium2_ref_publickey calls polyvec_matrix_pointwise_montgomery. Remove expand_mat and replace the call at line 451 with polyvec_matrix_expand, and replace the manual per-row loop with polyvec_matrix_pointwise_montgomery, so both code paths share the same library implementations.

Suggested fix

Delete the local expand_mat function and replace its call with polyvec_matrix_expand(mat, rho). Replace the manual for (i = 0; i < K; ++i) { polyvecl_pointwise_acc_montgomery(...); poly_invntt_tomont(...); } loop with polyvec_matrix_pointwise_montgomery(&t1, mat, &s1hat); polyveck_reduce(&t1); polyveck_invntt_tomont(&t1); to match the library path in pqcrystals_dilithium2_ref_publickey. Both functions should share a single implementation for each cryptographic operation.

[sentinel-ai-reviewer]

The Dilithium branch derives a 32-byte key seed via sha512HalfS and passes it to pqcrystals_dilithium2_ref_keypair_seed, but never calls secureErase on it before returning — leaving the derivation seed on the stack. The Ed25519 and Secp256k1 branches immediately above both call secureErase(key.data(), key.size()) on their equivalent material; the Dilithium branch simply omits this step. That 32-byte seed is sufficient to re-derive the entire 2560-byte Dilithium secret key, making it higher-value residue than the packed secret key bytes that ARE erased in buf. Add secureErase(key.data(), key.size()) after the secureErase(buf, ...) call, consistent with the existing pattern in the two sibling branches.

Suggested fix

Add secureErase(key.data(), key.size()); after the existing secureErase(buf, CRYPTO_SECRETKEYBYTES); call, before the return statement. This mirrors lines 532 and 540 in the Ed25519 and Secp256k1 branches respectively.

[sentinel-ai-reviewer]

toHexString is added to SecretKey.cpp but is never called anywhere in the codebase — it is dead code. Its presence alongside #include <iostream> and #include <iomanip> is consistent with debug-logging scaffolding that was partially removed. More importantly, the function is semantically unsafe for cryptographic use: it writes into a std::ostringstream backed by a heap buffer with no secure-erasure on destruction, so any future call on Dilithium secret key material would leave the hex-encoded key in a freed heap block. Delete the function and its associated unused I/O headers.

Suggested fix

Remove the toHexString function definition and the associated unused #include directives (, , , ) from SecretKey.cpp. If hex formatting of key material is ever needed for diagnostics, it should go through a purpose-built secure channel (e.g. a redacted log that masks all but the first few bytes) rather than a general-purpose string formatter.