fix: reject identity elements in deserialization and key generation by UdjinM6 · Pull Request #7193 · dashpay/dash

UdjinM6 · 2026-03-03T09:04:11Z

Issue being fixed or feature implemented

Identity elements are mathematically valid curve points but have no legitimate use in the protocol.

What was done?

Reject BLS identity elements (point at infinity for G1/G2) at the deserialization boundary in SetBytes(). Also reject zero private keys in MakeNewKey(). Identity elements would not pass further validation anyway, reject them early.

How Has This Been Tested?

Run tests

Breaking Changes

n/a

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation
I have assigned this pull request to a milestone

Reject BLS identity elements (point at infinity for G1/G2) at the deserialization boundary in SetBytes(). Also reject zero private keys in MakeNewKey(), though these would not pass further validation. Identity elements are mathematically valid curve points but have no legitimate use in the protocol. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

github-actions · 2026-03-03T09:04:49Z

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

coderabbitai · 2026-03-03T09:07:05Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2e6a225 and 42b707b.

📒 Files selected for processing (2)

src/bls/bls.cpp
src/bls/bls.h

Walkthrough

These changes add validation guards to BLS key handling. In CBLSSecretKey::MakeNewKey, the code now retries key generation if the resulting PrivateKey equals a default-constructed instance, ensuring the key is valid before marking it as such. In CBLSWrapper::SetBytes, deserialization now checks if the resulting object is default-constructed and, if so, resets the wrapper and returns early without setting fValid status, preventing invalid states from being treated as valid.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'fix: reject identity elements in deserialization and key generation' directly and precisely summarizes the main changes: rejecting identity elements in both SetBytes() deserialization and MakeNewKey() key generation.
Description check	✅ Passed	The description is clearly related to the changeset, explaining the issue (identity elements lack legitimate protocol use), the solution (early rejection at deserialization and key generation boundaries), and rationale (these would fail further validation anyway).

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

thepastaclaw

Concept ACK — rejecting identity elements (point at infinity for G1/G2) and zero private keys early is solid defense-in-depth. The existing all_of(vecBytes, 0) check doesn't catch these since the BLS12-381 infinity point serializes as 0xc0 || 00...00, not all-zeros.

A few thoughts:

Tests: Would be great to have tests that verify identity elements are actually rejected by SetBytes(). Something like constructing the known serialized form of the G1/G2 identity (0xc0 + zeros), calling SetBytes(), and asserting !IsValid(). Same for verifying MakeNewKey() can't produce a zero key (though that's probabilistically near-impossible to test directly).
Minor nit: In the SetBytes identity check, Reset() already replaces *this with a default-constructed instance (which nullifies cachedHash), so the cachedHash.SetNull() call right after is redundant — though it matches the pattern at the bottom of the function and doesn't hurt clarity.

PastaPastaPasta

utACK 42b707b

knst

LGTM 42b707b

knst · 2026-03-03T14:12:32Z

src/bls/bls.cpp

        GetStrongRandBytes({buf, sizeof(buf)});
        try {
            impl = bls::PrivateKey::FromBytes(bls::Bytes(reinterpret_cast<const uint8_t*>(buf), SerSize));
+            if (impl == bls::PrivateKey()) {


nit: I don't think that's quite possible situation even, should it still be here?

…and key generation 42b707b fix: reject identity elements in deserialization and key generation (UdjinM6) Pull request description: ## Issue being fixed or feature implemented Identity elements are mathematically valid curve points but have no legitimate use in the protocol. ## What was done? Reject BLS identity elements (point at infinity for G1/G2) at the deserialization boundary in SetBytes(). Also reject zero private keys in MakeNewKey(). Identity elements would not pass further validation anyway, reject them early. ## How Has This Been Tested? Run tests ## Breaking Changes n/a ## Checklist: - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone ACKs for top commit: PastaPastaPasta: utACK 42b707b Tree-SHA512: 047b098fd56b5da07099fde9b03ada7dd4b42698f47cdc84d3c855c11b0122d46a74765fcaaad5d73465abd0d19605445c9e4b6ab6182cf2b318bfe695d2ef0a

240a95f Merge #7193: fix: reject identity elements in deserialization and key generation (pasta) 444cbf2 Merge #7191: fix(qt): reseat quorum labels when new types are inserted (pasta) Pull request description: ## Backport Cherry-picks of #7191 and #7193 into `v23.1.x`. ### Included - #7193 — `fix: reject identity elements in deserialization and key generation` (42b707b) - #7191 — `fix(qt): reseat quorum labels when new types are inserted` (641b3ea) ACKs for top commit: UdjinM6: utACK 240a95f Tree-SHA512: dc270139159974b3bdab9177a7215de201d4deecc7883d47e35174fe7073c3508e61313e99e554b5a646ceb179202c96eb170e53dc9fc01177349aaa840acf2b

8d5936d chore: add #7191 and #7193 to v23.1.1 release notes (PastaClaw) 9f3662b chore: v23.1.1 release preparation (PastaClaw) 5dbfa98 chore: v23.1.1 release preparation (PastaClaw) Pull request description: ## v23.1.1 Release Preparation Version bump and release notes for v23.1.1. ### Changes - **Version bump:** `_CLIENT_VERSION_BUILD` 0 → 1 in `configure.ac` - **Release notes:** Written for all 12 backported PRs, following v23.0.2 format - **Archived:** v23.1.0 release notes to `doc/release-notes/dash/release-notes-23.1.0.md` - **Flatpak metainfo:** Added v23.1.0 and v23.1.1 release entries ### Notes - This PR should be merged **after** #7182 (the backport PR) lands on `v23.1.x` - `chainparams.cpp` updates (`nMinimumChainWork`, `defaultAssumeValid`) are not included — those should be done closer to tag time with fresh block data - Translations and manpages regeneration may need separate commits depending on process ACKs for top commit: UdjinM6: utACK 8d5936d Tree-SHA512: 9a4dca9635ee2bdecf224be928ce1e952c13256171a6e075793fefa074c1a1ee8b149a5b9853abe00ddb423bbecc7f5d5631d311a77c37ff6df5bca19c1c067f

UdjinM6 added this to the 23.1.1 milestone Mar 3, 2026

UdjinM6 added the backport-candidate-23.1.x label Mar 3, 2026

UdjinM6 requested review from PastaPastaPasta and knst March 3, 2026 09:04

thepastaclaw mentioned this pull request Mar 3, 2026

[GH] activity: dashpay/dash#7193 — review comment from UdjinM6 thepastaclaw/tracker#437

Closed

thepastaclaw reviewed Mar 3, 2026

View reviewed changes

PastaPastaPasta approved these changes Mar 3, 2026

View reviewed changes

knst approved these changes Mar 3, 2026

View reviewed changes

PastaPastaPasta merged commit 11c2565 into dashpay:develop Mar 3, 2026
78 of 84 checks passed

This was referenced Mar 3, 2026

[GH] activity: dashpay/dash#7193 — review comment from UdjinM6 thepastaclaw/tracker#456

Closed

backport: cherry-pick #7191 and #7193 into v23.1.x #7195

Merged

thepastaclaw added a commit to thepastaclaw/dash that referenced this pull request Mar 3, 2026

chore: add dashpay#7191 and dashpay#7193 to v23.1.1 release notes

f8927d1

thepastaclaw added a commit to thepastaclaw/dash that referenced this pull request Mar 5, 2026

chore: add dashpay#7191 and dashpay#7193 to v23.1.1 release notes

8d5936d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: reject identity elements in deserialization and key generation#7193

fix: reject identity elements in deserialization and key generation#7193
PastaPastaPasta merged 1 commit intodashpay:developfrom
UdjinM6:fix/bls-reject-identity-elements

UdjinM6 commented Mar 3, 2026

Uh oh!

github-actions bot commented Mar 3, 2026

Uh oh!

coderabbitai bot commented Mar 3, 2026

❌ Failed checks (1 warning)

Uh oh!

thepastaclaw left a comment

Uh oh!

PastaPastaPasta left a comment

Uh oh!

knst left a comment

Uh oh!

knst Mar 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

UdjinM6 commented Mar 3, 2026

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

github-actions bot commented Mar 3, 2026

✅ No Merge Conflicts Detected

Uh oh!

coderabbitai bot commented Mar 3, 2026

Walkthrough

Estimated code review effort

❌ Failed checks (1 warning)

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

knst Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants