[ALPHANET] Quantum - #27
Conversation
a7d8764 to
8223aba
Compare
82514a2 to
3f29a0d
Compare
![sentinel-ai-reviewer[bot]](https://avatars.githubusercontent.com/in/320396?s=60&v=4){kind=small}
| CONFIGURE_COMMAND "" | ||
| LOG_BUILD ON | ||
| BUILD_IN_SOURCE 0 | ||
| BUILD_COMMAND |
There was a problem hiding this comment.
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"
| @@ -0,0 +1,62 @@ | |||
| include(FetchContent) | |||
There was a problem hiding this comment.
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.
| // First, set our own public key. Only secp256k1 (legacy) and dilithium | ||
| // (post-quantum) are valid for signing validations; Ed25519 has never | ||
| // been supported here. | ||
| if (auto const kt = publicKeyType(pk); |
There was a problem hiding this comment.
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.
| std::uint8_t buf_[kSize]{}; | ||
| // Dilithium secret keys are 2528 bytes; ed25519/secp256k1 are 32. | ||
| // Buffer sized for the largest supported key; actual length in size_. | ||
| std::uint8_t buf_[2560]{}; |
There was a problem hiding this comment.
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.
| // (post-quantum) are valid for signing validations; Ed25519 has never | ||
| // been supported here. | ||
| if (auto const kt = publicKeyType(pk); | ||
| kt != KeyType::Secp256k1 && kt != KeyType::Dilithium) |
There was a problem hiding this comment.
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.
|
|
||
| private: | ||
| std::uint8_t buf_[kSize]{}; | ||
| // Dilithium secret keys are 2528 bytes; ed25519/secp256k1 are 32. |
There was a problem hiding this comment.
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.
| // first strip that prefix. | ||
| return ed25519_sign_open(m.data(), m.size(), publicKey.data() + 1, sig.data()) == 0; | ||
| } | ||
| else if (*type == KeyType::Dilithium) |
There was a problem hiding this comment.
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).
| @@ -215,6 +231,10 @@ publicKeyType(Slice const& slice) | |||
| if (slice[0] == kEcCompressedPrefixEvenY || slice[0] == kEcCompressedPrefixOddY) | |||
There was a problem hiding this comment.
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.
| return 0; | ||
| } | ||
|
|
||
| int |
There was a problem hiding this comment.
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.
| @@ -362,9 +631,16 @@ parseBase58(TokenType type, std::string const& s) | |||
| auto const result = decodeBase58Token(s, type); | |||
There was a problem hiding this comment.
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.
| key = rhoprime + SEEDBYTES; | ||
|
|
||
| /* Expand matrix */ | ||
| expand_mat(mat, rho); |
There was a problem hiding this comment.
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.
| { | ||
| uint8_t pk[CRYPTO_PUBLICKEYBYTES]; | ||
| uint8_t buf[CRYPTO_SECRETKEYBYTES]; | ||
| auto key = sha512HalfS(Slice(seed.data(), seed.size())); |
There was a problem hiding this comment.
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.
| } | ||
| } | ||
|
|
||
| std::string |
There was a problem hiding this comment.
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.
Includes post-merge compile fixes (build-verified).
| COMMAND make -C <BINARY_DIR>/ref clean | ||
| COMMAND /bin/sh -c "CFLAGS='-DDILITHIUM_MODE=2 -DDILITHIUM_RANDOMIZED_SIGNING' make -C <BINARY_DIR>/ref libdilithium2_ref.a libfips202_ref.a" | ||
| INSTALL_COMMAND "" | ||
| BUILD_BYPRODUCTS |
There was a problem hiding this comment.
The build compiles Dilithium with DILITHIUM_MODE=2 (ML-DSA-44), the minimum NIST post-quantum security level — equivalent to AES-128 classical security. For a financial ledger where validator keys and signatures must remain unforgeable over a multi-decade horizon, this choice leaves the system at the lowest end of the acceptable range; NSA CNSA 2.0 and general post-quantum deployment guidance explicitly recommend ML-DSA-65 (Mode 3) or higher for new infrastructure with long key lifetimes. The performance cost of Mode 3 is modest (larger public keys/signatures but similar signing speed), and the security margin it buys against a stronger-than-anticipated quantum adversary is significant. Upgrade to DILITHIUM_MODE=3 and update all dependent symbol names (pqcrystals_dilithium3_*) and build byproduct names (libdilithium3_ref.a).
Suggested fix
Change DILITHIUM_MODE=2 to DILITHIUM_MODE=3 in the CFLAGS, update BUILD_BYPRODUCTS to libdilithium3_ref.a, rename the imported CMake targets to dilithium::dilithium3_ref, and update all pqcrystals_dilithium2_* symbol references in SecretKey.cpp, PublicKey.cpp, and Transactor.cpp to their dilithium3 equivalents.
|
No failures yet, but checks are still running: |
1 similar comment
|
No failures yet, but checks are still running: |
Applied by Sentinel /doctor
|
Resolved merge conflicts with Conflicted files resolved (2):
Summary: Summary
|
| # is a supply-chain decision that must be reviewed; never revert to a | ||
| # branch tag here. Upstream: | ||
| # https://github.com/Transia-RnD/dilithium/commit/3032292cfd4d94e0df9bd49a0098669ca9166aa1 | ||
| GIT_REPOSITORY https://github.com/Transia-RnD/dilithium.git |
There was a problem hiding this comment.
The Dilithium library is fetched from Transia-RnD/dilithium — a fork maintained by the PR author's own organization — rather than the official pq-crystals/dilithium reference implementation. For a post-quantum signing primitive that will be used for all validator key operations, this is an unmitigated supply-chain risk: the fork ships its own Makefile that is executed verbatim at build time via /bin/sh -c 'make -C <BINARY_DIR>/ref ...', and there is no content hash (URL_HASH or equivalent) to verify the fetched sources match any known-good artifact. A git commit SHA pin prevents drift but does not provide cryptographic assurance against a compromised GitHub account or a SHA-1 collision (git's object model is still SHA-1 on most hosted repos). Any modification to sign.c, poly.c, or polyvec.c in the fork could introduce a subliminal channel that leaks the Dilithium secret key through observable signatures without any detectable on-chain anomaly. This dependency should be switched to pq-crystals/dilithium directly, or the fork should have a documented, reproducible diff audit against the official commit it claims to track.
Suggested fix
Replace GIT_REPOSITORY with https://github.com/pq-crystals/dilithium.git (the NIST reference implementation). If the fork is required for packaging changes (e.g., a different Makefile), document the exact diff against the canonical commit and add a CI step that asserts the diff never changes cryptographic files. Additionally, add a URL_HASH or GIT_HASH_ALGORITHM SHA256 check to provide content integrity beyond the git SHA-1 pin.
|
No failures yet, but checks are still running: |
|
No failures yet, but checks are still running: |
|
No failures yet, but checks are still running: |
|
No failures yet, but checks are still running: |
2 participants
High Level Overview of Change
Context of Change
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)