fix: enforce BLS spec KeyValidate and normalize error handling#123
fix: enforce BLS spec KeyValidate and normalize error handling#123UdjinM6 wants to merge 6 commits into
Conversation
Per draft-irtf-cfrg-bls-signature-05, KeyValidate must reject the identity element and CoreVerify must call KeyValidate before pairing. - Reject identity public keys and signatures in CoreMPL::Verify, CoreMPL::AggregateVerify, and CoreMPL::VerifySecure per spec KeyValidate: "If xP is the identity element, return INVALID" - Add IsValid() and identity checks to LegacySchemeMPL::Verify and LegacySchemeMPL::AggregateVerify, matching CoreMPL::Verify - Override VerifySecure in AugSchemeMPL to call CoreMPL::AggregateVerify directly, preventing double-augmentation from virtual dispatch - Make PopSchemeMPL::PopVerify(Bytes, Bytes) return false on invalid input instead of throwing, per spec PopVerify returns VALID/INVALID Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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 Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a public Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant Aug as AugSchemeMPL
participant Normalizer as Hasher/Normalizer
participant Core as CoreMPL
Client->>Aug: VerifySecure(vecPubKeys, signature, message)
Aug->>Normalizer: serialize, sort, compute weights/t-factors
Normalizer-->>Aug: weighted keys & t-factors
Aug->>Aug: build weightedPubKeys & augmented messages
Aug->>Core: AggregateVerify(weightedPubKeys, signature, messages)
Core->>Core: deserialize & validate pubkeys/signature (reject defaults)
Core-->>Aug: verification result (true/false)
Aug-->>Client: return result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/schemes.cpp (1)
235-242: Consider addingIsValid()checks for consistency with other verification methods.The identity checks are correct, but
CoreMPL::VerifyandCoreMPL::AggregateVerifyalso checkIsValid()before the identity check. While the inputG1Elementobjects should already be valid (sinceFromBytesthrows on invalid points), addingIsValid()checks here would provide defense-in-depth and maintain consistency across all verification entry points.🔧 Suggested change for consistency
bool CoreMPL::VerifySecure(const std::vector<G1Element>& vecPublicKeys, const G2Element& signature, const Bytes& message, const bool fLegacy) { for (const auto& pk : vecPublicKeys) { - if (pk == G1Element()) { + if (!pk.IsValid() || pk == G1Element()) { return false; } } - if (signature == G2Element()) { + if (!signature.IsValid() || signature == G2Element()) { return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/schemes.cpp` around lines 235 - 242, Add explicit IsValid() checks for the public keys and signature before the identity comparisons: iterate vecPublicKeys and for each pk call pk.IsValid() (return false if any invalid) prior to comparing against G1Element(), and call signature.IsValid() (return false if invalid) before comparing to G2Element(); this mirrors CoreMPL::Verify/CoreMPL::AggregateVerify behavior and provides defense-in-depth while keeping the existing identity checks (symbols: vecPublicKeys, pk, signature).src/test.cpp (1)
1634-1663: Consider adding test coverage forAugSchemeMPL::VerifySecure.The tests validate
BasicSchemeMPL::VerifySecurebut the PR specifically adds an override inAugSchemeMPL::VerifySecureto prevent double-augmentation. A test verifying thatAugSchemeMPL::VerifySecureworks correctly (and doesn't double-augment) would strengthen coverage for this key change.💡 Suggested test addition
SECTION("AugSchemeMPL VerifySecure round-trips correctly") { auto sk1 = AugSchemeMPL().KeyGen(getRandomSeed()); auto sk2 = AugSchemeMPL().KeyGen(getRandomSeed()); auto pk1 = sk1.GetG1Element(); auto pk2 = sk2.GetG1Element(); auto msg = getRandomSeed(); auto sig1 = AugSchemeMPL().Sign(sk1, msg); auto sig2 = AugSchemeMPL().Sign(sk2, msg); vector<G1Element> pks = {pk1, pk2}; vector<G2Element> sigs = {sig1, sig2}; auto aggSig = AugSchemeMPL().AggregateSecure(pks, sigs, Bytes(msg)); // This should work without double-augmentation REQUIRE(AugSchemeMPL().VerifySecure(pks, aggSig, Bytes(msg))); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test.cpp` around lines 1634 - 1663, Add a new test SECTION mirroring the BasicScheme case but using AugSchemeMPL to ensure AugSchemeMPL::VerifySecure does not double-augment: generate two keys with AugSchemeMPL().KeyGen(getRandomSeed()), get their G1Element() pubkeys, sign the same message with AugSchemeMPL().Sign, build vectors of G1Element and G2Element, create an aggregate signature with AugSchemeMPL().AggregateSecure(pks, sigs, Bytes(msg)), then assert REQUIRE(AugSchemeMPL().VerifySecure(pks, aggSig, Bytes(msg))); place this with the existing VerifySecure tests so it covers the override in AugSchemeMPL::VerifySecure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/schemes.cpp`:
- Around line 235-242: Add explicit IsValid() checks for the public keys and
signature before the identity comparisons: iterate vecPublicKeys and for each pk
call pk.IsValid() (return false if any invalid) prior to comparing against
G1Element(), and call signature.IsValid() (return false if invalid) before
comparing to G2Element(); this mirrors CoreMPL::Verify/CoreMPL::AggregateVerify
behavior and provides defense-in-depth while keeping the existing identity
checks (symbols: vecPublicKeys, pk, signature).
In `@src/test.cpp`:
- Around line 1634-1663: Add a new test SECTION mirroring the BasicScheme case
but using AugSchemeMPL to ensure AugSchemeMPL::VerifySecure does not
double-augment: generate two keys with AugSchemeMPL().KeyGen(getRandomSeed()),
get their G1Element() pubkeys, sign the same message with AugSchemeMPL().Sign,
build vectors of G1Element and G2Element, create an aggregate signature with
AugSchemeMPL().AggregateSecure(pks, sigs, Bytes(msg)), then assert
REQUIRE(AugSchemeMPL().VerifySecure(pks, aggSig, Bytes(msg))); place this with
the existing VerifySecure tests so it covers the override in
AugSchemeMPL::VerifySecure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 809086fc-aba6-4df3-bdcc-05f138cc6842
📒 Files selected for processing (3)
include/dashbls/schemes.hppsrc/schemes.cppsrc/test.cpp
CoreMPL::VerifySecure and AugSchemeMPL::VerifySecure only checked for identity elements but not IsValid(), unlike CoreMPL::Verify and CoreMPL::AggregateVerify which check both. Add the missing IsValid() checks for consistency and early rejection of invalid elements. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…essages AugSchemeMPL::Sign hashes H(pk_i || msg) per signer, so VerifySecure cannot combine pubkeys into a single key (the augmentation prefix differs per signer). Pass T-weighted individual pubkeys with their per-signer augmented messages to CoreMPL::AggregateVerify instead. Add AugSchemeMPL VerifySecure round-trip test to exercise this path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/test.cpp (1)
1685-1690: Split the invalidPopVerifybytes case by failure mode.Using a bad pubkey and a bad proof in the same assertion only proves that some parse failure returns
false. SeparatebadPk + validProofandvalidPk + badProofcases would lock in both error-handling branches individually.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test.cpp` around lines 1685 - 1690, Split the combined-failure test into two focused cases: one that supplies a malformed pubkey with a valid proof, and one that supplies a valid pubkey with a malformed proof. For the first SECTION (e.g. "Invalid pubkey returns false instead of throwing") construct badPk of length G1Element::SIZE filled with 0xff and obtain a known-good proof bytes (from a fixture or by generating a proof with PopSchemeMPL::PopProve/valid keypair) then assert REQUIRE_NOTHROW(PopSchemeMPL().PopVerify(Bytes(badPk), Bytes(validProof))) and REQUIRE(...PopVerify(...) == false). For the second SECTION (e.g. "Invalid proof returns false instead of throwing") construct badProof of length G2Element::SIZE filled with 0xff and use a known-good pubkey bytes (from a fixture/valid keypair) then assert REQUIRE_NOTHROW(PopSchemeMPL().PopVerify(Bytes(validPk), Bytes(badProof))) and REQUIRE(...PopVerify(...) == false). Ensure you reference PopSchemeMPL::PopVerify, G1Element::SIZE, G2Element::SIZE and Bytes when locating the test to edit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/test.cpp`:
- Around line 1634-1681: Add failing-case tests to cover
AugSchemeMPL::VerifySecure and identity-signature rejection: extend the
TEST_CASE to include a SECTION that mirrors "VerifySecure rejects identity
pubkey" but calls AugSchemeMPL().VerifySecure(pks, sig, Bytes(msg)) and asserts
false, and add another SECTION that constructs a valid public key list and
passes an identity G2Element() as the signature (for both
BasicSchemeMPL().VerifySecure and AugSchemeMPL().VerifySecure) asserting the
call returns false; reference AggregateSecure/VerifySecure and the
AugSchemeMPL/BasicSchemeMPL classes to locate where to add these checks.
- Around line 1595-1632: Update the LegacySchemeMPL tests to also cover the new
!IsValid() paths by creating malformed (non-identity) G1Element and G2Element
values via FromBytesUnchecked(..., true) and using those in the existing Verify
and AggregateVerify sections (in addition to the default-constructed identity
cases); specifically, call LegacySchemeMPL::Verify with a malformed G1Element
and with a malformed G2Element and assert false, and call
LegacySchemeMPL::AggregateVerify with malformed entries in the pks or sig
parameter and assert false, reusing the existing legacy invalid fixtures present
later in this file to construct the invalid elements.
---
Nitpick comments:
In `@src/test.cpp`:
- Around line 1685-1690: Split the combined-failure test into two focused cases:
one that supplies a malformed pubkey with a valid proof, and one that supplies a
valid pubkey with a malformed proof. For the first SECTION (e.g. "Invalid pubkey
returns false instead of throwing") construct badPk of length G1Element::SIZE
filled with 0xff and obtain a known-good proof bytes (from a fixture or by
generating a proof with PopSchemeMPL::PopProve/valid keypair) then assert
REQUIRE_NOTHROW(PopSchemeMPL().PopVerify(Bytes(badPk), Bytes(validProof))) and
REQUIRE(...PopVerify(...) == false). For the second SECTION (e.g. "Invalid proof
returns false instead of throwing") construct badProof of length G2Element::SIZE
filled with 0xff and use a known-good pubkey bytes (from a fixture/valid
keypair) then assert REQUIRE_NOTHROW(PopSchemeMPL().PopVerify(Bytes(validPk),
Bytes(badProof))) and REQUIRE(...PopVerify(...) == false). Ensure you reference
PopSchemeMPL::PopVerify, G1Element::SIZE, G2Element::SIZE and Bytes when
locating the test to edit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d53e702a-fd79-45b6-a423-ba0808207efc
📒 Files selected for processing (2)
src/schemes.cppsrc/test.cpp
✅ Files skipped from review due to trivial changes (1)
- src/schemes.cpp
- Add LegacySchemeMPL tests for malformed non-identity elements using FromBytesUnchecked to exercise IsValid() guards in Verify and AggregateVerify - Add AugSchemeMPL VerifySecure rejection tests for identity pubkeys and identity signatures - Add BasicSchemeMPL VerifySecure rejection test for identity signatures - Split PopVerify combined parse-failure test into separate malformed pubkey and malformed proof cases for independent branch coverage Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@coderabbitai review |
1 similar comment
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/schemes.cpp (1)
388-426:⚠️ Potential issue | 🟠 MajorRoute the
vector<uint8_t>overload through the guarded bytes path.Line 393 still deserializes
signaturewithG2Element::FromByteVector(signature)before delegation, soBasicSchemeMPL::AggregateVerify(vector<vector<uint8_t>>...)can still throw on malformed input while the newBytesoverload returnsfalse. That leaves the public API inconsistent for the same operation.♻️ Suggested fix
bool BasicSchemeMPL::AggregateVerify(const vector<vector<uint8_t>> &pubkeys, const vector<vector<uint8_t>> &messages, const vector<uint8_t> &signature) { - const size_t nPubKeys = pubkeys.size(); - auto arg_check = VerifyAggregateSignatureArguments(nPubKeys, messages.size(), G2Element::FromByteVector(signature)); - if (arg_check != CONTINUE) { - return arg_check; - } - - const std::set<vector<uint8_t>> setMessages(messages.begin(), messages.end()); - if (setMessages.size() != nPubKeys) { - return false; - } - return CoreMPL::AggregateVerify(pubkeys, messages, signature); + const std::vector<Bytes> vecPubKeyBytes(pubkeys.begin(), pubkeys.end()); + const std::vector<Bytes> vecMessagesBytes(messages.begin(), messages.end()); + return BasicSchemeMPL::AggregateVerify(vecPubKeyBytes, vecMessagesBytes, Bytes(signature)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/schemes.cpp` around lines 388 - 426, The vector<uint8_t> overload BasicSchemeMPL::AggregateVerify should not directly call G2Element::FromByteVector(signature); instead convert the signature vector into a Bytes (or a Bytes-compatible wrapper) and delegate to the Bytes overload BasicSchemeMPL::AggregateVerify(const vector<Bytes>&, const vector<Bytes>&, const Bytes&) to reuse its guarded deserialization (G2Element::FromBytes) and argument checking; update the function to build a Bytes from the incoming vector<uint8_t> (and similarly ensure messages/pubkeys are converted to the expected types or forwarded unchanged) and return the result of CoreMPL::AggregateVerify only via the Bytes path so malformed signatures return false consistently.
♻️ Duplicate comments (1)
src/test.cpp (1)
1595-1676:⚠️ Potential issue | 🟡 MinorAdd malformed aggregate-signature coverage for
LegacySchemeMPL::AggregateVerify.The new sections pin identity signatures and malformed pubkeys, but the non-identity
!signature.IsValid()branch inLegacySchemeMPL::AggregateVerifyis still untested. A malformedG2Elementin the aggregate path would close the remaining gap.➕ Suggested test addition
+ SECTION("AggregateVerify rejects malformed non-identity signature") + { + g2_t point_native; + g2_set_infty(point_native); + fp2_rand(point_native->x); + fp2_rand(point_native->y); + fp2_rand(point_native->z); + G2Element badSig = G2Element::FromNative(point_native); + REQUIRE(badSig.IsValid() == false); + + auto sk = BasicSchemeMPL().KeyGen(getRandomSeed()); + auto pk = sk.GetG1Element(); + auto msg = getRandomSeed(); + vector<G1Element> pks = {pk}; + vector<Bytes> msgs = {Bytes(msg)}; + REQUIRE(LegacySchemeMPL().AggregateVerify(pks, msgs, badSig) == false); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test.cpp` around lines 1595 - 1676, Add a new SECTION that constructs a malformed non-identity G2Element (e.g. using native point creation like in the "Verify rejects malformed non-identity signature" test: set g2_t to non-infinity and fp2_rand on coordinates, then wrap with G2Element::FromNative and assert badSig.IsValid() == false), then create a valid pk via BasicSchemeMPL().KeyGen()->GetG1Element(), put that pk and a message into vectors and call LegacySchemeMPL().AggregateVerify(pks, msgs, badSig) and REQUIRE the result is false; this exercises the non-identity !signature.IsValid() branch in LegacySchemeMPL::AggregateVerify.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/schemes.cpp`:
- Around line 388-426: The vector<uint8_t> overload
BasicSchemeMPL::AggregateVerify should not directly call
G2Element::FromByteVector(signature); instead convert the signature vector into
a Bytes (or a Bytes-compatible wrapper) and delegate to the Bytes overload
BasicSchemeMPL::AggregateVerify(const vector<Bytes>&, const vector<Bytes>&,
const Bytes&) to reuse its guarded deserialization (G2Element::FromBytes) and
argument checking; update the function to build a Bytes from the incoming
vector<uint8_t> (and similarly ensure messages/pubkeys are converted to the
expected types or forwarded unchanged) and return the result of
CoreMPL::AggregateVerify only via the Bytes path so malformed signatures return
false consistently.
---
Duplicate comments:
In `@src/test.cpp`:
- Around line 1595-1676: Add a new SECTION that constructs a malformed
non-identity G2Element (e.g. using native point creation like in the "Verify
rejects malformed non-identity signature" test: set g2_t to non-infinity and
fp2_rand on coordinates, then wrap with G2Element::FromNative and assert
badSig.IsValid() == false), then create a valid pk via
BasicSchemeMPL().KeyGen()->GetG1Element(), put that pk and a message into
vectors and call LegacySchemeMPL().AggregateVerify(pks, msgs, badSig) and
REQUIRE the result is false; this exercises the non-identity
!signature.IsValid() branch in LegacySchemeMPL::AggregateVerify.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 31c7e839-ddb0-48ef-9c17-a9655f43d4be
📒 Files selected for processing (2)
src/schemes.cppsrc/test.cpp
|
Addressed #123 (review) @coderabbitai review |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/schemes.cpp (1)
577-578:⚠️ Potential issue | 🟡 MinorSigned/unsigned mismatch in loop counter.
nPubKeysissize_tbut the loop counteriis declared asint. This causes a signed/unsigned comparison warning and could theoretically cause issues if the collection size exceedsINT_MAX.🐛 Proposed fix
- for (int i = 0; i < nPubKeys; ++i) { + for (size_t i = 0; i < nPubKeys; ++i) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/schemes.cpp` around lines 577 - 578, The loop uses a signed int counter i against an unsigned size_t nPubKeys causing a signed/unsigned mismatch; change the loop counter to size_t (or auto) to match nPubKeys and avoid warnings and potential overflow—update the for loop that iterates to nPubKeys (the one initializing augMessages) so i is declared as size_t (or use std::size_t/auto) and ensure any uses of i within the loop are compatible.
🧹 Nitpick comments (1)
src/schemes.cpp (1)
402-418: Minor: Signature is parsed twice.The signature is parsed at line 404, but at line 418 the raw
signatureBytes is passed toCoreMPL::AggregateVerify, which will parse it again. This could be avoided by calling the element-based overload withsignatureElement.♻️ Proposed optimization
- return CoreMPL::AggregateVerify(pubkeys, messages, signature); + vector<G1Element> pubkeyElements; + pubkeyElements.reserve(nPubKeys); + try { + for (const auto& pk : pubkeys) { + pubkeyElements.push_back(G1Element::FromBytes(pk)); + } + } catch (...) { + return false; + } + return CoreMPL::AggregateVerify(pubkeyElements, messages, signatureElement);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/schemes.cpp` around lines 402 - 418, The code parses the signature into a G2Element (signatureElement) but then calls CoreMPL::AggregateVerify with the raw signature bytes, causing a second parse; change the final call to use the element-based overload by passing signatureElement (the parsed G2Element) to CoreMPL::AggregateVerify (instead of the raw signature) so the already-parsed element is reused; locate the usage around G2Element::FromBytes, VerifyAggregateSignatureArguments, and the current CoreMPL::AggregateVerify call to update the parameter accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/schemes.cpp`:
- Around line 577-578: The loop uses a signed int counter i against an unsigned
size_t nPubKeys causing a signed/unsigned mismatch; change the loop counter to
size_t (or auto) to match nPubKeys and avoid warnings and potential
overflow—update the for loop that iterates to nPubKeys (the one initializing
augMessages) so i is declared as size_t (or use std::size_t/auto) and ensure any
uses of i within the loop are compatible.
---
Nitpick comments:
In `@src/schemes.cpp`:
- Around line 402-418: The code parses the signature into a G2Element
(signatureElement) but then calls CoreMPL::AggregateVerify with the raw
signature bytes, causing a second parse; change the final call to use the
element-based overload by passing signatureElement (the parsed G2Element) to
CoreMPL::AggregateVerify (instead of the raw signature) so the already-parsed
element is reused; locate the usage around G2Element::FromBytes,
VerifyAggregateSignatureArguments, and the current CoreMPL::AggregateVerify call
to update the parameter accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d4c78b6-ccf6-4c96-9ba4-8b3676b45087
📒 Files selected for processing (2)
src/schemes.cppsrc/test.cpp
Should be fixed now. @coderabbitai review |
|
✅ Actions performedReview triggered.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR correctly implements BLS spec KeyValidate with consistent identity-element rejection across all verification paths. The AugSchemeMPL::VerifySecure per-signer augmented message fix is cryptographically sound. Three minor defense-in-depth gaps remain, none exploitable.
Reviewed commit: 180a8cf
🟡 2 suggestion(s) | 💬 1 nitpick(s)
2 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 `src/schemes.cpp`:
- [SUGGESTION] lines 695-709: FastAggregateVerify omits signature identity/validity check at entry point
Every other native-element verification path in this PR checks `!signature.IsValid() || signature == G2Element()` before doing work. `FastAggregateVerify(vector<G1Element>, Bytes, G2Element)` skips this and proceeds directly to `CoreMPL::Aggregate(pubkeys)` — an O(n) group addition — before delegating to `CoreMPL::Verify`, which does check the signature. Adding the check early avoids unnecessary aggregation on identity/invalid signatures and keeps the defense-in-depth pattern consistent across all entry points.
- [SUGGESTION] lines 262-266: Uncaught FromBytes in CoreMPL::VerifySecure and AugSchemeMPL::VerifySecure inner loops
Lines 264 and 630 call `G1Element::FromBytes()` on round-tripped serialized data from elements that already passed `IsValid()` and identity checks, so these should never throw in practice. However, every other `FromBytes` call in the PR's verification paths is wrapped in try-catch. Wrapping these two sites would complete the pattern and guard against hypothetical serialization round-trip bugs.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean, correct incremental fix that adds early identity-signature and validity rejection to PopSchemeMPL::FastAggregateVerify, consistent with the pattern in CoreMPL::Verify (line 152) and CoreMPL::VerifySecure (line 246). No blocking issues. Two minor observations worth noting.
Reviewed commit: a78ad28
🟡 1 suggestion(s) | 💬 1 nitpick(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 `src/test.cpp`:
- [SUGGESTION] lines 1902-1909: No coverage for the !signature.IsValid() branch
The test exercises `signature == G2Element()` but not `!signature.IsValid()`. The new guard at schemes.cpp:702 has two conditions — only the identity half is tested. A section constructing a malformed G2Element (e.g. via FromBytes with invalid data, caught and stored before IsValid is false) would exercise the other branch.
Add IsValid() and identity checks for the signature before aggregating pubkeys in PopSchemeMPL::FastAggregateVerify, avoiding O(n) group additions when the signature is already known to be invalid. The spec (draft-irtf-cfrg-bls-signature-05) does not explicitly require FastAggregateVerify to check the signature for identity — it delegates to CoreVerify (section 2.7) which does. But section 2.5 (KeyValidate) establishes the principle that identity elements are invalid. This is a defense-in-depth optimization, not a spec deviation — it fails faster on the same inputs that would fail anyway downstream in CoreVerify. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Trivial incremental: renames test case to broader title and adds a new SECTION exercising the !signature.IsValid() branch of FastAggregateVerify, directly addressing both findings from the prior review. The malformed-G2Element construction pattern follows existing test conventions (lines 1579-1584, 1650-1654). No issues found.
Reviewed commit: b2cfcdf
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Incremental push addresses both prior findings (test case rename + malformed-signature branch coverage). LGTM.
|
✅ Review complete (commit b2cfcdf) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR substantively hardens verification across schemes by rejecting identity G1/G2 elements per the BLS KeyValidate/CoreVerify spec and normalizing malformed-byte handling to return false. Two legitimate concerns remain: the broad catch (...) in bytes overloads now hides operational errors like missing BLS::Init(), and the new identity-sig rejection in VerifySecure breaks the empty round-trip with AggregateSecure. Also worth a chain-state check before legacy identity-rejection lands in a Dash Core release.
Reviewed commit: b2cfcdf
🟡 3 suggestion(s)
1 additional finding
🟡 suggestion: Legacy identity rejection is a consensus-relevant behavioral change — confirm via chain-state scan
src/schemes.cpp (lines 750-767)
Adding pubkey == G1Element() / signature == G2Element() rejection to LegacySchemeMPL::Verify and LegacySchemeMPL::AggregateVerify (line 781) is correct per the BLS spec, but LegacySchemeMPL exists specifically to keep historical Dash chain verification stable for pre-V19 data. Any historical artifact (masternode list entry, quorum commitment, IS/CL signature) that contains an identity public key or identity signature would previously verify and would now be rejected — causing a hard fork for nodes upgrading past this commit. In practice no rational signer produces identity elements and there is no known on-chain instance, but please confirm with a chain-state scan of observed BLS pubkeys/signatures before this lands in a Dash Core release. The Basic/Aug/Pop changes carry no equivalent risk because they post-date V19.
🤖 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 `src/schemes.cpp`:
- [SUGGESTION] lines 121-131: `catch (...)` in bytes overloads also swallows operational errors like missing BLS::Init()
The new `try { ... } catch (...) { return false; }` blocks across `CoreMPL::Verify`, `CoreMPL::AggregateVerify`, `BasicSchemeMPL::AggregateVerify`, `AugSchemeMPL::AggregateVerify`, `PopSchemeMPL::PopVerify`, and `PopSchemeMPL::FastAggregateVerify` (bytes overloads) catch *every* exception, not just malformed-input ones. `G1Element::FromBytes`/`G2Element::FromBytes` call `BLS::CheckRelicErrors()`, which throws `std::runtime_error("Library not initialized properly. Call BLS::Init()")` (`src/bls.cpp:81-85`). After this PR those operational faults are indistinguishable from invalid signatures: callers see `false` and have no way to detect that the library was never initialized. Narrow the catch to the specific exception types thrown by element parsing (e.g. `std::invalid_argument`) and let `std::runtime_error`/other operational exceptions propagate so initialization bugs remain diagnosable.
- [SUGGESTION] lines 237-248: VerifySecure no longer accepts the empty aggregate that AggregateSecure still produces
`CoreMPL::AggregateSecure({}, {}, msg)` returns `G2Element()` (identity) because `CoreMPL::Aggregate` over an empty vector yields identity. Previously `VerifySecure({}, identity, msg)` round-tripped to true through `AggregateVerify({identity_pk}, {msg}, identity_sig)` — `VerifyAggregateSignatureArguments` itself documents the empty-aggregate convention at `src/schemes.cpp:69-71` (zero pubkeys + zero messages + identity signature is GOOD). The new identity-signature rejection at lines 246-247 (and the same code path at `src/schemes.cpp:611-612` in `AugSchemeMPL::VerifySecure`) makes the same identity aggregate unverifiable, breaking the `AggregateSecure`/`VerifySecure` round-trip the public API previously supported. If empty secure aggregates are intentionally unsupported now, also reject `vecPublicKeys.empty()` in `AggregateSecure` so the contract is symmetric; otherwise the early identity-signature reject should allow the empty-pubkey case through.
- [SUGGESTION] lines 750-767: Legacy identity rejection is a consensus-relevant behavioral change — confirm via chain-state scan
Adding `pubkey == G1Element()` / `signature == G2Element()` rejection to `LegacySchemeMPL::Verify` and `LegacySchemeMPL::AggregateVerify` (line 781) is correct per the BLS spec, but `LegacySchemeMPL` exists specifically to keep historical Dash chain verification stable for pre-V19 data. Any historical artifact (masternode list entry, quorum commitment, IS/CL signature) that contains an identity public key or identity signature would previously verify and would now be rejected — causing a hard fork for nodes upgrading past this commit. In practice no rational signer produces identity elements and there is no known on-chain instance, but please confirm with a chain-state scan of observed BLS pubkeys/signatures before this lands in a Dash Core release. The Basic/Aug/Pop changes carry no equivalent risk because they post-date V19.
| bool CoreMPL::Verify(const Bytes& pubkey, const Bytes& message, const Bytes& signature) | ||
| { | ||
| return CoreMPL::Verify(G1Element::FromBytes(pubkey), message, G2Element::FromBytes(signature)); | ||
| G1Element pubkeyElement; | ||
| G2Element signatureElement; | ||
| try { | ||
| pubkeyElement = G1Element::FromBytes(pubkey); | ||
| signatureElement = G2Element::FromBytes(signature); | ||
| } catch (...) { | ||
| return false; | ||
| } | ||
| return CoreMPL::Verify(pubkeyElement, message, signatureElement); |
There was a problem hiding this comment.
🟡 Suggestion: catch (...) in bytes overloads also swallows operational errors like missing BLS::Init()
The new try { ... } catch (...) { return false; } blocks across CoreMPL::Verify, CoreMPL::AggregateVerify, BasicSchemeMPL::AggregateVerify, AugSchemeMPL::AggregateVerify, PopSchemeMPL::PopVerify, and PopSchemeMPL::FastAggregateVerify (bytes overloads) catch every exception, not just malformed-input ones. G1Element::FromBytes/G2Element::FromBytes call BLS::CheckRelicErrors(), which throws std::runtime_error("Library not initialized properly. Call BLS::Init()") (src/bls.cpp:81-85). After this PR those operational faults are indistinguishable from invalid signatures: callers see false and have no way to detect that the library was never initialized. Narrow the catch to the specific exception types thrown by element parsing (e.g. std::invalid_argument) and let std::runtime_error/other operational exceptions propagate so initialization bugs remain diagnosable.
source: ['codex']
🤖 Fix this 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 `src/schemes.cpp`:
- [SUGGESTION] lines 121-131: `catch (...)` in bytes overloads also swallows operational errors like missing BLS::Init()
The new `try { ... } catch (...) { return false; }` blocks across `CoreMPL::Verify`, `CoreMPL::AggregateVerify`, `BasicSchemeMPL::AggregateVerify`, `AugSchemeMPL::AggregateVerify`, `PopSchemeMPL::PopVerify`, and `PopSchemeMPL::FastAggregateVerify` (bytes overloads) catch *every* exception, not just malformed-input ones. `G1Element::FromBytes`/`G2Element::FromBytes` call `BLS::CheckRelicErrors()`, which throws `std::runtime_error("Library not initialized properly. Call BLS::Init()")` (`src/bls.cpp:81-85`). After this PR those operational faults are indistinguishable from invalid signatures: callers see `false` and have no way to detect that the library was never initialized. Narrow the catch to the specific exception types thrown by element parsing (e.g. `std::invalid_argument`) and let `std::runtime_error`/other operational exceptions propagate so initialization bugs remain diagnosable.
| const G2Element& signature, | ||
| const Bytes& message, | ||
| const bool fLegacy) { | ||
| for (const auto& pk : vecPublicKeys) { | ||
| if (!pk.IsValid() || pk == G1Element()) { | ||
| return false; | ||
| } | ||
| } | ||
| if (!signature.IsValid() || signature == G2Element()) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: VerifySecure no longer accepts the empty aggregate that AggregateSecure still produces
CoreMPL::AggregateSecure({}, {}, msg) returns G2Element() (identity) because CoreMPL::Aggregate over an empty vector yields identity. Previously VerifySecure({}, identity, msg) round-tripped to true through AggregateVerify({identity_pk}, {msg}, identity_sig) — VerifyAggregateSignatureArguments itself documents the empty-aggregate convention at src/schemes.cpp:69-71 (zero pubkeys + zero messages + identity signature is GOOD). The new identity-signature rejection at lines 246-247 (and the same code path at src/schemes.cpp:611-612 in AugSchemeMPL::VerifySecure) makes the same identity aggregate unverifiable, breaking the AggregateSecure/VerifySecure round-trip the public API previously supported. If empty secure aggregates are intentionally unsupported now, also reject vecPublicKeys.empty() in AggregateSecure so the contract is symmetric; otherwise the early identity-signature reject should allow the empty-pubkey case through.
source: ['codex']
🤖 Fix this 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 `src/schemes.cpp`:
- [SUGGESTION] lines 237-248: VerifySecure no longer accepts the empty aggregate that AggregateSecure still produces
`CoreMPL::AggregateSecure({}, {}, msg)` returns `G2Element()` (identity) because `CoreMPL::Aggregate` over an empty vector yields identity. Previously `VerifySecure({}, identity, msg)` round-tripped to true through `AggregateVerify({identity_pk}, {msg}, identity_sig)` — `VerifyAggregateSignatureArguments` itself documents the empty-aggregate convention at `src/schemes.cpp:69-71` (zero pubkeys + zero messages + identity signature is GOOD). The new identity-signature rejection at lines 246-247 (and the same code path at `src/schemes.cpp:611-612` in `AugSchemeMPL::VerifySecure`) makes the same identity aggregate unverifiable, breaking the `AggregateSecure`/`VerifySecure` round-trip the public API previously supported. If empty secure aggregates are intentionally unsupported now, also reject `vecPublicKeys.empty()` in `AggregateSecure` so the contract is symmetric; otherwise the early identity-signature reject should allow the empty-pubkey case through.
3 participants
Per draft-irtf-cfrg-bls-signature-05, KeyValidate (section 2.5) must reject the identity element and CoreVerify (section 2.7) must call KeyValidate before pairing.
Summary by CodeRabbit
New Features
Bug Fixes
Tests