Repository navigation
test(drive-abci): pin the check_tx consensus error for an IdentityUpdate adding malformed ECDSA/BLS key data - #5631
Conversation
…ate adding malformed ECDSA/BLS key data
An IdentityUpdate that adds an ECDSA_SECP256K1 or BLS12_381 key whose
data is not a public key used to get an internal error at check_tx:
the fee estimate hashes the added keys' data, which nothing had parsed
yet. Since check_tx verifies the proofs of possession of an
IdentityUpdate's added keys before the fee estimate, at every protocol
version, such data fails that check with a consensus error
(BasicECDSAError / BasicBLSError). No code change is needed; this pins
it.
The new test adds the issue's three shapes to the proofs of possession
test faults and runs each through Platform::check_tx at protocol
versions 1, 13, 14 and latest:
- an ECDSA key of 20 bytes;
- an ECDSA key of 33 bytes whose x coordinate is above the field prime;
- a BLS key of 47 bytes.
Test would have caught this in CI: ✖ before the fix, ✔ after. With
check_tx's added key proof verification disabled, each shape fails
with the internal error from the issue:
- 20-byte ECDSA: ParsingError("the key length is invalid: 20 Allowed
sizes: 33 or 65 bytes for ecdsa key");
- off-curve ECDSA: "unable to create pub key - key ECDSA error";
- 47-byte BLS: ParsingError("the key length is invalid: 47 Allowed
sizes: 48 bytes for bls key").
On v5.1-dev as is, each gets a consensus error.
Test-only: no behavior change, no protocol version impact.
Closes #5464
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 4 minutes. View limit details
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 |
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
|
✅ Final review complete — no blockers (commit c28d244) · triage: low |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The PR is a focused test-only addition that correctly covers malformed ECDSA and BLS key data at the check_tx admission boundary, with valid-transition regression coverage. No blocking correctness or architectural issues were found; the Rust workspace CI check passed. One diagnostic nitpick remains because the new loop does not identify which fault failed.
💬 1 nitpick(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
💬 Nitpick: Include the fault in shared check_tx assertion messages
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/mod.rs:4439-4448
The new test invokes assert_refused_at_check_tx for three faults from one loop, but the helper's failure messages only include the protocol version. A failure therefore reports, for example, only "at protocol version 15" and does not identify whether the hash-sized ECDSA, off-curve ECDSA, or short BLS case failed. Include the Debug-formatted fault in both assertions, matching the existing block-path diagnostic messages, so failures are immediately attributable.
assert_matches!(
errors.as_slice(),
[error] if fault.is_reported_by(error),
"at protocol version {protocol_version} with {fault:?}: {errors:?}"
);
assert_eq!(
check_tx(&platform, &update(&owner, None, 1, 1).await),
vec![],
"at protocol version {protocol_version} with {fault:?}"
);
source: glm-5.3-flash (phase1-reviewer: general, rust-quality)
Review provenance
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
lowbygpt-6.1-sol(effort low) — This is a small, self-contained Rust test-only change that adds malformed key fixtures and assertions around existing check_tx behavior without changing production logic. - Phase 1 reviewers:
glm-5.3-flash— general (completed, effort high); agentphase1-reviewer,glm-5.3-flash— rust-quality (completed, effort high); agentphase1-reviewer - Phase 1 model:
glm-5.3-flash— zai quota: 5h 96% left, weekly 94% left; passed overgemini-3.8-flash-high(antigravity quota lookup failed) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort medium); 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/execution/validation/state_transition/state_transitions/identity_update/mod.rs`:
- [NITPICK] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/mod.rs:4439-4448: Include the fault in shared check_tx assertion messages
The new test invokes assert_refused_at_check_tx for three faults from one loop, but the helper's failure messages only include the protocol version. A failure therefore reports, for example, only "at protocol version 15" and does not identify whether the hash-sized ECDSA, off-curve ECDSA, or short BLS case failed. Include the Debug-formatted fault in both assertions, matching the existing block-path diagnostic messages, so failures are immediately attributable.
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
assert_refused_at_check_tx runs for several faults, and a failure only named the protocol version. Both assertion messages now include the fault, as the block-path assertions do. Test diagnostics only: no behavior change, so no red to green run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The test-only change correctly adds coverage for malformed ECDSA and BLS key data at check_tx across the relevant protocol versions, while validating that the corresponding valid transition remains admissible. The malformed fixtures isolate invalid key data and assert the expected consensus error rather than an internal failure. Rust workspace tests passed; the remaining PR Hygiene check is policy-related and not a code failure.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
lowbygpt-6.1-sol(effort low) — This is a small, self-contained Rust test-only change that adds coverage for malformed key data without changing production behavior or protocol rules. - Phase 1 reviewers:
glm-5.3-flash— general (completed, effort high); agentphase1-reviewer,glm-5.3-flash— rust-quality (completed, effort high); agentphase1-reviewer - Phase 1 model:
glm-5.3-flash— zai quota: 5h 93% left, weekly 93% left; passed overgemini-3.8-flash-high(antigravity quota lookup failed) - 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 medium); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort medium); 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.
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
Issue being fixed or feature implemented
#5464: an IdentityUpdate that adds an
ECDSA_SECP256K1orBLS12_381key whose data is not a public key used to get an internal error at check_tx (code 13, logged at ERROR) instead of a consensus error. The fee estimate hashes the data of every added key (add_new_keys_to_identity), and nothing had parsed that data yet.The issue no longer reproduces on
v5.1-dev. #5494 made check_tx verify the proofs of possession of an IdentityUpdate's added keys on the first check, at every protocol version, before the fee estimate. Key data that is not a public key fails that verification with a consensus error (BasicECDSAError/BasicBLSError), so the fee estimate never sees it. #5494's own check_tx test covers a 48-byte BLS key that does not decode, but none of the shapes the issue names.Closes #5464
What was done?
Test only. The
proofs_of_possession_of_added_keystest module inidentity_update/mod.rsgets three newFaults, the shapes from the issue:HashSizedEcdsaKeyData: an ECDSA key of 20 bytes;OffCurveEcdsaKeyData: an ECDSA key of 33 bytes,0x02followed by an x coordinate above the field prime, so no curve point has it;ShortBlsKeyData: a BLS key of 47 bytes.Each ECDSA fault carries a well-formed proof signed by a real key, so only the key data fails. The BLS fault carries a 96-byte signature, as the existing BLS fault does.
The new test
should_refuse_at_check_tx_with_a_consensus_error_an_identity_update_adding_ecdsa_or_bls_key_data_that_is_not_a_public_keyruns each fault throughassert_refused_at_check_tx. For every fault and at protocol versions 1, 13, 14 and latest, that checks two things:Platform::check_txreturns exactly one consensus error of the expected kind, not an internal error;The new faults are not added to
FAULTS, which drives the block-path charging test; the issue is about admission only.Protocol-version impact
None. The change is test-only. The behavior it pins is check_tx admission, which is not consensus, and #5494 already applied it at every protocol version.
How Has This Been Tested?
Red → green. #5494's check_tx block is the fix, so the "before" state disables just that block: the
if let StateTransition::IdentityUpdate(...)incheck_tx_verification/v0/mod.rsis made never to match. With that block disabled, each shape fails with the internal error from the issue:Drive(Protocol(ParsingError("the key length is invalid: 20 Allowed sizes: 33 or 65 bytes for ecdsa key")))Drive(Protocol(Error(unable to create pub key - key ECDSA error)))Drive(Protocol(ParsingError("the key length is invalid: 47 Allowed sizes: 48 bytes for bls key")))On
v5.1-devas is, the test passes.Runs:
cargo test -p drive-abci --lib -- identity_update check_tx: 132 passed, 1 ignored.cargo test -p drive-abci --lib -- proofs_of_possession_of_added_keys: 16 passed.cargo clippy -p drive-abci --all-targets -- -D warningsand the same with--all-features: clean.cargo fmt --all: clean.Breaking Changes
None.
Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
PR Hygiene ·
c28d244/self-reviewedWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.