You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
primitives%fix!: check operator keys under the payload's BLS scheme - #129
The provider payload checks in dash-primitives refuse an operator key only when it is null. The reference implementation v24.0.0 reads the key under the scheme the payload uses: legacy for a version 1 ProRegTx or ProUpRegTx, and basic otherwise. It then refuses the key unless it is a valid key that encodes back to the same bytes. So a key that one scheme reads as valid and the other doesn't passes the check here under either version.
#88 split this check off until dash-pkc could read keys under both schemes as the reference implementation does. #101 added that, so this pull request brings the check back.
Additional Information
Description
Notes
Reference implementation
Corpus
Read the operator key under the payload's scheme
is_valid_operator_key takes the payload version. It uses the legacy scheme at version 1 and the basic scheme otherwise, and requires a key that is not null and is BlsPkBytes::is_canonical under that scheme. ProUpSharedRegTx always uses the basic scheme.
The ProRegTx, ProUpRegTx and ProUpSharedRegTx checks return ProTxInvalid::NullKey for an invalid operator key, the reason they already give for a null one, at the same place in the check.
The ProRegTx, ProUpRegTx and ProUpSharedRegTx checks now refuse operator keys they accepted before: any non-null key that isn't canonical under the payload's scheme fails with ProTxInvalid::NullKey.
How Has This Been Tested?
operator_key, for each of the three payloads at version 2 or later, over chain and regtest fixtures: the fixture's key and a key that only the basic scheme reads pass; a null key, an off-curve key, an off-subgroup key, the point at infinity, and the key with the top bit of its first byte cleared each fail with ProTxInvalid::NullKey.
operator_key_legacy, for ProRegTx and ProUpRegTx at version 1: a valid key, an off-curve key, an off-subgroup key, and that key with its top bit cleared pass; a null key, the point at infinity, and a key that only the basic scheme reads fail.
Every case in both tests also checks the key at the same point in each check as the reference implementation. A refused key still fails with ProTxInvalid::NullKey when the next check would fail too: the operator reward, the payouts or the signature count. The check before it still fails first: the mode or the version.
The fixed keys are the first input of their class in dash-pkc's bls_read.json5, which gives the reference implementation's read of each input under both schemes.
The dash-primitives and workspace suites pass with all features and with default features.
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 tests
I have made corresponding changes to the documentation
I did not use agentic assistance (i.e. LLMs, AI) except as disclosed and can answer questions,
comments and concerns without said assistance. I assume authorship of the code submitted.
I have assigned this pull request to a milestone (for repository code-owners and collaborators only)
Agent disclosure
Code, commit messages and this description were generated with Claude Code using Claude Opus 5.5, running autonomously as an agent on behalf of the author.
The checks read the operator key under the payload's scheme, legacy
for a version 1 ProRegTx or ProUpRegTx and basic otherwise, and refuse
an invalid key as null, as the reference implementation does.
No actionable comments were generated in the recent review.
Summary
Summary by CodeRabbit
Bug Fixes
Operator keys are now checked for valid encoding and curve membership when registering or updating provider transactions.
Key validation follows the rules for the transaction version, improving compatibility with both legacy and current key formats.
Invalid operator keys and null voting keys are reported consistently.
Validation errors are returned in a consistent order when multiple registration or update fields are invalid.
Operator key validation now checks key canonicality under the applicable BLS scheme. ProRegTx and ProUpRegTx select the scheme based on transaction version. ProUpSharedRegTx validates keys with the IETF BLS scheme. Tests cover key encodings and the order of key errors relative to other validation errors.
Suggested reviewers:thepastaclaw
Merge Risk: Minimal · up to 75dab
The operator-key checks have no identified merge-blocking issue. Merge after normal checks.
Pre-merge checks: 4
Check name
Status
Explanation
Title check
The title clearly summarizes the main change: scheme-aware validation of operator keys.
Description check
The description directly explains the operator-key validation changes, affected payloads, reference behavior, breaking change, and tests.
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Reviewing files that changed from the base of the PR and between 90952f8 and 75dab82.
Files selected for processing: 4
pkgs/primitives/src/provider/proregtx.rs
pkgs/primitives/src/provider/proupregtx.rs
pkgs/primitives/src/provider/proupsharedregtx.rs
pkgs/primitives/src/provider/sig.rs
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
Additional comments: 1
pkgs/primitives/src/provider/proregtx.rs: 1
342-344: Trivial · Maintainability & Code Quality · Low value
Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.
Invalid operator keys return the same error as null keys.
An off-curve or non-canonical operator key now returns ProTxInvalid::NullKey. Its message is bad-protx-key-null. The enum already has OperatorKeyMismatch (bad-protx-operator-pubkey). The PR states that NullKey is the intended result, so this is acceptable. Callers cannot tell a null key from an invalid key.
Reviewing files that changed from the base of the PR and between 90952f8 and bfff0a4.
Files selected for processing: 4
pkgs/primitives/src/provider/proregtx.rs
pkgs/primitives/src/provider/proupregtx.rs
pkgs/primitives/src/provider/proupsharedregtx.rs
pkgs/primitives/src/provider/sig.rs
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
Final validation — Phase 1 + Phase 2
Verified the changes at bfff0a4 against the pinned Dash Core implementation, existing BLS canonicality contracts, and corpus-backed tests; no correctness or security defects were found. One non-blocking suggestion remains about expressing the shared-registrar payload's fixed BLS scheme directly. This was a static review; current CI reports successful applicable build, test, lint, and cross-platform checks, with automated review still pending.
🟡 1 suggestion(s)
Approved: nothing here blocks the merge. The non-blocking findings are optional: fix them forward, in this PR or a follow-up, without waiting for another review round.
Triage: normal by gpt-6.1-sol (effort low) — The diff changes cryptographic key acceptance through a small scheme-selecting helper and three validation call sites, with most additions devoted to tests, so it is not large or intricate enough for critical.
These findings are from an automated code review (PastaClaw). How to work through them:
- Verify each finding against the code; fix only what is real, as narrowly as the finding.
- If a finding is wrong, decline it on its thread in one or two sentences; the bot answers once and stops.
- To defer one, link a tracking issue on its thread (or reply asking the bot to file one).
- Suggestions don't block the merge, but most are worth fixing in this PR; judge each on its merits.
- Keep the PR's scope; don't grow it to satisfy hardening suggestions.
- Carried items were raised in an earlier round: read the thread first, and don't re-fix what you already fixed, declined or deferred there. Disputed and deferred items are already handled and are not listed.
In `pkgs/primitives/src/provider/proupsharedregtx.rs`:
- [SUGGESTION] pkgs/primitives/src/provider/proupsharedregtx.rs:84-86: Express the shared registrar's fixed BLS scheme directly
This payload has an independent u16 version whose current value is 1, and its operator key always uses the basic scheme. Passing ProTxVersion::BasicBls to a helper documented as taking the payload version makes that argument serve two different roles: an actual payload version for ProRegTx/ProUpRegTx, but a scheme selector here. The call is correct today and the comment explains it, so this is a non-blocking API-design suggestion rather than a consensus defect. Factor the non-null/canonicality predicate into a helper generic over BlsScheme, and let the version-based helper dispatch to it; this caller can then select BlsScIetf directly without depending on an unrelated payload-version type.
The reason will be displayed to describe this comment to others. Learn more.
✅ Resolved at 75dab82f; see the replies below.
🟡 Suggestion: Express the shared registrar's fixed BLS scheme directly
This payload has an independent u16 version whose current value is 1, and its operator key always uses the basic scheme. Passing ProTxVersion::BasicBls to a helper documented as taking the payload version makes that argument serve two different roles: an actual payload version for ProRegTx/ProUpRegTx, but a scheme selector here. The call is correct today and the comment explains it, so this is a non-blocking API-design suggestion rather than a consensus defect. Factor the non-null/canonicality predicate into a helper generic over BlsScheme, and let the version-based helper dispatch to it; this caller can then select BlsScIetf directly without depending on an unrelated payload-version type.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality)
The reason will be displayed to describe this comment to others. Learn more.
Done in 75dab82. is_valid_bls_key::<S> checks that a key is not null and canonical under a scheme type, and is_valid_operator_key picks the scheme from the payload version and calls it, the same split as verify_bls::<S> and verify_operator. ProUpSharedRegTx now calls is_valid_bls_key::<BlsScIetf> directly, so ProTxVersion is no longer imported there. Behaviour and tests are unchanged.
🤖 Posted autonomously by Claude on behalf of pasta.
The reason will be displayed to describe this comment to others. Learn more.
Resolved (re-reviewed at 75dab82f): Confirmed your change in 75dab82: ProUpSharedRegTx calls is_valid_bls_key:: directly and no longer imports ProTxVersion. The version-based helper delegates to the same generic predicate, preserving the non-null and scheme-specific canonicality checks.
`is_valid_bls_key` checks a key under a scheme type, and
`is_valid_operator_key` picks that scheme from the payload version.
ProUpSharedRegTx passes the basic scheme instead of a payload version.
The reason will be displayed to describe this comment to others. Learn more.
Re-review — Final validation — Phase 1 + Phase 2
At head 75dab82, the scheme-typed refactor preserves Core-compatible operator-key validation and existing error precedence; no new actionable defects were found. Validation was static only, including comparison with pinned Dash Core source and reference-derived corpus inputs. Current CI shows lint, stable-platform builds, cross-builds, and completed MSRV checks passing; matrix.os and matrix.target are cancelled runs with no failed steps, while dash-num and dash-p2p-core patch-coverage checks remain pending.
Triage: low by gpt-6.1-sol (effort low) — The production change is small and contained, selecting an existing BLS canonicality check by payload version at three validation sites, while most of the diff adds tests rather than intricate cryptographic logic.
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.
The reason will be displayed to describe this comment to others. Learn more.
These are cryptographic tests, dash-primitives isn't the place for bespoke cryptographic checks, especially since it's expressed through whatever dash-pkc exposes in its public API, making this a check that dash-pkc should have, though likely, it already does in greater detail due to access to private APIs.
The reason will be displayed to describe this comment to others. Learn more.
Since the operator pubkey is present in multiple structs it may be valuable to wrap them in a struct, what we did in primitives::sig to differentiate between multiple signature payloads and make version-based is_valid checks an inherent function.
Would be worthwhile to add a commit that folds in verify_operator as well
This branch has not been deployed
No deployments
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
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
thepastaclaw
left a comment
Motivation
The provider payload checks in
dash-primitivesrefuse an operator key only when it is null. The reference implementation v24.0.0 reads the key under the scheme the payload uses: legacy for a version 1 ProRegTx or ProUpRegTx, and basic otherwise. It then refuses the key unless it is a valid key that encodes back to the same bytes. So a key that one scheme reads as valid and the other doesn't passes the check here under either version.#88 split this check off until
dash-pkccould read keys under both schemes as the reference implementation does. #101 added that, so this pull request brings the check back.Additional Information
is_valid_operator_keytakes the payload version. It uses the legacy scheme at version 1 and the basic scheme otherwise, and requires a key that is not null and isBlsPkBytes::is_canonicalunder that scheme. ProUpSharedRegTx always uses the basic scheme.bls_read.json5(dash-pkc), unchangedProTxInvalid::NullKeyfor an invalid operator key, the reason they already give for a null one, at the same place in the check.Breaking Changes
The ProRegTx, ProUpRegTx and ProUpSharedRegTx checks now refuse operator keys they accepted before: any non-null key that isn't canonical under the payload's scheme fails with
ProTxInvalid::NullKey.How Has This Been Tested?
operator_key, for each of the three payloads at version 2 or later, over chain and regtest fixtures: the fixture's key and a key that only the basic scheme reads pass; a null key, an off-curve key, an off-subgroup key, the point at infinity, and the key with the top bit of its first byte cleared each fail withProTxInvalid::NullKey.operator_key_legacy, for ProRegTx and ProUpRegTx at version 1: a valid key, an off-curve key, an off-subgroup key, and that key with its top bit cleared pass; a null key, the point at infinity, and a key that only the basic scheme reads fail.ProTxInvalid::NullKeywhen the next check would fail too: the operator reward, the payouts or the signature count. The check before it still fails first: the mode or the version.dash-pkc'sbls_read.json5, which gives the reference implementation's read of each input under both schemes.dash-primitivesand workspace suites pass with all features and with default features.Checklist
comments and concerns without said assistance. I assume authorship of the code submitted.
Agent disclosure
Code, commit messages and this description were generated with Claude Code using Claude Opus 5.5, running autonomously as an agent on behalf of the author.
🤖 Generated with Claude Code