Repository navigation
refactor: share rust-dashcore BLS backend through dpp::bls - #5320
Merged
lklimek merged 13 commits intoOct 7, 2026
Merged
Conversation
Record signatures, secure aggregates, scalar boundary behavior, key generation and persisted validator bytes before replacing the BLS implementation. All eight compatibility tests pass on the original backend. Co-authored-by: Codex <noreply@openai.com>
Preserve historical scalar reduction, infinity parsing, signature bytes and validator serialization with the previously committed compatibility vectors. Adapt repository consumers and replace the blst git patch with registry 0.3.17. Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (61)
💤 Files with no reviewable changes (2)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe pull request replaces the BLS backend API in DPP and migrates Platform, wallet, proof-verifier, and signer code to the new types. It adds Serde support and compatibility fixtures and tests for BLS operations and validator storage. ChangesBLS Backend and API
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~50 minutes Change: Refactor Suggested reviewers: Merge Risk: ⚪ Minimal · up to The historical quorum-state read path remains compatible, and no concrete issue currently prevents merging after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❓ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 55.03% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 149 functions across 50 files. (9 skipped: 4 unsupported, 5 over the file limit.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
Collaborator
|
✅ Final review complete — no blockers (commit 0fabd55) · triage: critical |
…ions Reserve legacy enum tags while rejecting their payloads at deserialization. Require v4.0.0 to commit a block before opening older databases. Remove legacy readers and the default production dependency on bls-signatures; retain the dependency only for tests and explicit mocks. Test legacy rejection, supported storage tags, persisted-state round trips, and the existing serialization hash using a pre-removal fixture. Co-Authored-By: Codex <noreply@openai.com>
Move the signature implementation under bls and export PublicKey, SecretKey, Signature and BlsError from dpp::bls. Update all repository consumers and remove the former dpp::bls_signatures path. Co-Authored-By: Codex <noreply@openai.com>
lklimek
changed the title
Move BLS Serde support under bls/serde.rs. Share key parsing between Deserialize and field adapters while retaining tuple decoding for bincode and deserialize_any for buffered tagged fields. Deprecate the old adapter re-export and retain its optional-key path. Co-Authored-By: Codex <noreply@openai.com>
Restore Platform-state V0 and quorum-storage V0/V1 readers and their production dependency. Keep their removal separate from the backend migration so the compatibility policy can be discussed independently. Adapt restored imports to dpp::bls. Co-Authored-By: Codex <noreply@openai.com>
4 of 7 tasks
lklimek
changed the title
lklimek
marked this pull request as ready for review
October 7, 2026 11:18
Contributor
Author
|
/self-reviewed |
thepastaclaw
reviewed
Oct 7, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
No blocking issue was substantiated; three non-blocking suggestions remain concerning breaking-change metadata, repeated curve-point decoding, and legacy-conversion fixture coverage. Verification was static only, with no builds, tests, or benchmarks run. The supplied CI snapshot had policy checks pending and runner/title checks queued, so successful Rust validation was not independently confirmed.
🟡 3 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, cross-cutting diff replaces cryptographic signing, verification and key handling in packages/rs-dpp/src/bls/bls_signatures.rs and native_bls.rs and changes BLS serialization in bls/serde.rs, directly modifying critical surfaces despite the intended compatibility. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-dpp/src/core_types/validator/v0/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/core_types/validator/v0/mod.rs:29: Classify the BLS API replacement as breaking release metadata
This public field now uses a different nominal key type, and the PR also removes the public dpp::bls_signatures re-export. Existing external Rust callers therefore require source changes even though wire encodings and consensus behavior remain compatible. The PR description says that ! is reserved for consensus changes, but CLAUDE.md and the checked-in PR template require it for breaking changes generally. The pinned conventional-changelog-dash preset also recognizes ! and BREAKING CHANGE notes without a consensus-only restriction. Add the breaking-change marker to the PR title, ensure the eventual release commit carries breaking-change metadata, and retain the Rust migration instructions in release notes. This API change does not require a PlatformVersion activation.
In `packages/rs-dpp/tests/bls_compatibility.rs`:
- [SUGGESTION] packages/rs-dpp/tests/bls_compatibility.rs:108-115: Assert DPP's legacy-key conversion against the frozen vectors
The frozen legacy_public_key assertion calls dash-pkc's to_scheme directly rather than the new DPP PublicKey::to_legacy_bytes method used by the wallet locator. A regression in the DPP method can therefore leave this compatibility assertion green. The locator tests do not close that gap: they check that the encodings differ and build their matching fixtures through the same helper. Add an assertion through the DPP method alongside the upstream assertion so the frozen historical bytes cover the actual migrated consumer boundary.
In `packages/rs-dpp/src/bls/bls_signatures.rs`:
- [SUGGESTION] packages/rs-dpp/src/bls/bls_signatures.rs:178-180: Retain validated points instead of decoding them again during verification
PublicKey::try_from and Signature::from_compressed already decode and subgroup-check non-infinity inputs, but discard the backend points. These lines repeat that decoding and validation on every verification, and the pinned IETF backend subsequently calls blst verification with both signature and public-key validation enabled. Unlike the previous decoded-point representation, this adds repeated curve work to NativeBlsModule's verification path; the InstantLock loop also re-decodes the same signature for each candidate quorum. Retain validated backend points with explicit handling for historically accepted infinity, or benchmark and document the CPU-versus-memory tradeoff before retaining the compressed-only representation. Preserve the current admission checks and verification-time infinity rejection.
Cover DPP legacy-key conversion with frozen vectors and preserve infinity rejection across basic and aggregate verification. Document the Rust API migration in the changelog. Co-Authored-By: Codex <noreply@openai.com>
Keep PublicKey opaque while representing infinity and validated backend points with a private enum. Preserve encodings and validation behavior. Co-Authored-By: Codex <noreply@openai.com>
thepastaclaw
reviewed
Oct 7, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static inspection of the complete diff at 1d729f9 found no blocking defect and confirmed that the prior redundant-decoding and legacy-conversion coverage findings are fixed. One in-scope suggestion remains: directly exercise the migrated historical V0 quorum-storage conversion with frozen bytes. No builds or tests were run; the supplied CI snapshot has runner selection queued and policy/title checks pending or queued, so completed runtime validation is not established by this review.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate diff replaces signing, verification, aggregation and key handling in packages/rs-dpp/src/bls/bls_signatures.rs and consolidates key serialization in packages/rs-dpp/src/bls/serde.rs, directly changing cryptographic implementation despite intending identical acceptance rules. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/for_saving_v0.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/for_saving_v0.rs:117-120: Exercise the retained V0 storage conversion with frozen bytes
This historical reader now converts a C++ `bls_signatures::PublicKey` through the new DPP parser. The frozen storage test decodes DPP `ValidatorV0`/`ValidatorSetV0` records and standalone DPP key Serde, but never decodes a `SignatureVerificationQuorumSetForSaving::V0` record and executes this cross-library conversion. Current runtime-to-storage conversion emits V2, so ordinary current-format round trips do not exercise this reader either. Compatibility between the two constructors remains an important invariant because the existing `expect` would turn a regression into a restoration panic.
Add a frozen pre-migration V0 quorum-storage fixture with nonempty current and previous quorum lists, decode it through the production storage enum, convert it into the runtime quorum set, and assert the restored key bytes, hashes, and indexes. This tests the boundary changed by this migration; it is a coverage suggestion, not evidence of an incompatibility or a request to redesign the historical reader.
Decode pre-migration V0 bytes through the production storage enum and verify current and previous quorum keys, hashes, indexes, configuration and heights. Include fixture provenance and a reproducible historical generator. Co-Authored-By: Codex <noreply@openai.com>
thepastaclaw
approved these changes
Oct 7, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The complete diff contains no confirmed in-scope defects in the shared BLS API, migrated consumers, or retained historical storage readers. The three prior coverage/performance suggestions are addressed; the unconditional breaking-marker request is withdrawn under the repository's consensus-only convention. This is a static assessment: no builds or tests were run, and the supplied CI snapshot still has build-runner selection queued and policy checks pending.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, cross-cutting diff changes cryptographic key parsing, signing, verification and aggregation in packages/rs-dpp/src/bls/bls_signatures.rs and serialized key handling in packages/rs-dpp/src/bls/serde.rs, directly affecting critical surfaces despite intending compatibility. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
shumkov
approved these changes
Oct 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Basic explanation
What this does: Moves Platform signing and signature verification to the same BLS backend as rust-dashcore and consolidates the Rust API and Serde support under
dpp::bls.Value: Removes the
blsful → blstrs_plusdependency chain and the workspace-wide Git patch forblst.Risks: Cryptographic compatibility is covered by frozen vectors and differential checks; no consensus-rule change is intended. Rust callers must update BLS type imports. Retaining decoded public keys and signatures increases their in-memory size while avoiding repeated decoding during verification. Historical database readers remain supported; their proposed removal is isolated in stacked draft #5323 for discussion.
Issue being fixed or feature implemented
Stacked on #5307, targeting
chore/rust-dashcore-1112-v5.1.The previous BLS dependency chain pinned
blst = 0.3.12, requiring a Git patch for WASM aggregate verification. This change shares rust-dashcore's pinneddash-pkcbackend and uses registryblst 0.3.17.What was done?
PublicKey,SecretKey,SignatureandBlsErrorunderdpp::bls; move the implementation intopackages/rs-dpp/src/bls/and update repository consumers. Removedpp::bls_signatures.blsful,blstrs_plusand the rootblstpatch. Usedash-pkcrevisione6402ced257c370a586ade9840ebbf545a4b7926, matching rust-dashcore. Rand versions are unchanged.Serialize/Deserializeand field adapters inbls/serde.rs, sharing the key visitor. BinaryDeserializeusesdeserialize_tuple(48, ...); field adapters retaindeserialize_anyfor buffered tagged data. Keepserialization::dashcore::bls_pubkeyand itsoptionadapter as a deprecated compatibility re-export.bls-signaturesdependency used for storage compatibility and test fixtures.PublicKeyremains an opaque struct backed by a privateInfinity/Validatedenum. Backend verification checks remain enabled.7c73e983b4c3cd9b5e2ead34bbe0360646b7b4d9. Decode through the production storage enum and restore both current and previous quorum lists, asserting keys, hashes, indexes, configuration and height metadata. Include fixture provenance, checksum and reproduction source.In-place changes to shipped generations
Signature-processing and validator methods receive type/accessor substitutions while retaining digests, protocol dispatch and cryptographic acceptance rules. Historical local storage formats remain readable. Supported serialized bytes, existing stored-state hashes and consensus rules are preserved; Serde implementation changes are covered by frozen byte fixtures and tagged-value/bincode tests.
How Has This Been Tested?
Historical quorum storage: all 40
drive-abciquorum-module tests passed, including the frozen V0 restoration test;drive-abciClippy passed with--all-targets --all-features --locked --offline -- --no-deps -D warnings. The 350-byte fixture was generated successfully by the pre-migration code in a separate checkout.Eight DPP frozen compatibility tests passed on the final implementation, including legacy-key conversion through DPP, Basic signatures, secure aggregation, scalar boundaries, malformed/subgroup points, infinity rejection and historical validator storage. The expanded suite also passed before changing the point representation.
The private-public-key-enum refactor passed the eight compatibility tests, 14 DPP BLS/Serde/core-type tests, and DPP all-target/all-feature Clippy with the same flags below.
Before the private-enum refactor, 259 targeted tests passed across DPP BLS/Serde/core types/signing, simple-signer, proof verification, drive-abci platform types and ChainLocks, and wallet masternode lookup/provider updates.
Before the private-enum refactor, Clippy passed for
dpp,simple-signer,drive-proof-verifier,drive-abciandplatform-walletwith--all-targets --all-features --locked --offline -- --no-deps -D warnings.Formatting of the changed Rust files and whitespace checks passed.
Limitations: No full workspace runtime suite, WASM/mobile validation, live-network replay or benchmarks were run for the retained-point implementation. Backend subgroup checks during verification remain enabled; no measured speedup is claimed. Cargo emits the existing DPP manifest warning that
default-featuresis ignored for the inheriteddashcoredependency.Breaking Changes
dpp::bls::{PublicKey, SecretKey, Signature, BlsError}. Generic blsful types and schemes are no longer exposed. UsePublicKey::to_bytes(),SecretKey::from_be_bytes()returningOption, and Basicsign(message)/Signature::from_compressed(). Key Display/Debug output uses compressed encodings.serialization::dashcore::bls_pubkeypath remains as a deprecated re-export ofbls::serde, includingoption.!according to the repository's explicit consensus-breaking convention; the Rust source incompatibility is documented here and inCHANGELOG.md.Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit