fix(drive): store the current-key alias for bound encryption/decryption keys under the purpose subtree - #4832
Conversation
…ultipleReferenceToLatest Registers a MEDIUM ENCRYPTION key with SingleContract bounds against a contract whose config opts in with requiresIdentityEncryptionBoundedKey = 2. The existing test only covers the Unique (0) mode. This one fails on v4.2-dev: Drive inserts the current-key sibling reference beside the purpose subtree instead of inside it, grovedb reports a missing reference, and the transition ends as an InternalError. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…on keys under the purpose subtree An IdentityUpdate adding an ENCRYPTION or DECRYPTION key bound to a contract whose config opts in with requiresIdentityEncryptionBoundedKey / requiresIdentityDecryptionBoundedKey = 2 (MultipleReferenceToLatest) failed inside Drive with a grovedb MissingReference and was dropped as an InternalError. The current-key alias is a sibling reference at the empty key, but for non-AUTHENTICATION purposes v1 wrote it one level above the purpose subtree that holds the key id it names, so it could not resolve. The AUTHENTICATION and document-type branches, and every reader (CurrentKeyOfKindRequest, identities_contract_keys_query), already used the purpose subtree. Both v1 methods now use the purpose subtree for every purpose. AllKeysOfKindRequest skips the empty key for every purpose so listings never repeat the alias. v1 is fixed in place: the old path never produced a committed block on any network (the batch always failed and the transition was removed from the proposal), so replay is unaffected and no migration is needed; v0 is untouched. Comments and the v14 changelog describe the real layout. The regression test now covers both purposes, both key-kind queries, and the refresh path on disable; a drive-level test covers the fetch and getIdentitiesContractKeys reads, the fee estimate, and grovedb consistency. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 14 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: dashpay/platform/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (8)
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 |
PR HygieneState: waiting-bots · commit
Self-review is an author attestation that you have read the diff: This check passes when the policy is satisfied; the repository decides whether merging requires it. |
|
⛔ Final review complete — 2 blocking finding(s) (commit 6999335) · triage: normal · Phase 2 only (queue backlog) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
The alias relocation matches the existing current-key readers, but newly enabled atomic key rotations can retain the disabled key as current, and all-keys queries now omit Unique-mode keys. The writer changes also alter shipped protocol-14 behavior without a new activation boundary. Independent source inspection confirms these three issues; git diff --check passed, but the Rust tests were not independently rerun.
🔴 2 blocking | 🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
normalbygpt-6-astra(effort low) — The behavioral changes are modest alias-path and query-range fixes with substantial regression tests, but their interaction with key disabling, storage modes, and protocol-version compatibility warrants ordinary review rather than qualifying as a large or intricate critical-surface change. - Phase 1 reviewers: not run (skipped for throughput: 21 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort high); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort high); 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/src/drive/identity/contract_info/keys/refresh_potential_contract_info_key_references/v1/mod.rs`:
- [BLOCKING] packages/rs-drive/src/drive/identity/contract_info/keys/refresh_potential_contract_info_key_references/v1/mod.rs:212-216: Coalesce alias insertion and refresh during bound-key rotation
An IdentityUpdate that registers a replacement bound ENCRYPTION or DECRYPTION key and disables the previous current key queues registration before revocation. With the corrected paths, registration inserts the replacement alias and revocation then queues an untrusted refresh for exactly the same slot. The coalescer still recognizes only AUTHENTICATION, so GroveDB's batch map keeps the later refresh instead of the insertion. That refresh reads the existing on-disk reference, leaving the disabled key current; enabling batching consistency verification instead rejects the duplicate operations. Although document-type bounds already had this problem, this PR newly exposes it for contract-level MultipleReferenceToLatest keys. Extend alias coalescing through a versioned batch-application path so insertion wins over refresh for both newly enabled purposes, and add same-update registration-plus-revocation tests with consistency verification both enabled and disabled.
In `packages/rs-drive/src/drive/identity/contract_info/keys/add_potential_contract_info_for_contract_bounded_key/v1/mod.rs`:
- [BLOCKING] packages/rs-drive/src/drive/identity/contract_info/keys/add_potential_contract_info_for_contract_bounded_key/v1/mod.rs:307-312: Activate the storage repair through a new protocol version
Both alias writers change their existing v1 implementations while the protocol-14 method table still selects generation 1. CHANGELOG.md explicitly records protocol 14 as shipped in 4.2.0-beta.2, and the versioning book requires shipped behavior to remain frozen. The fact that the broken path never committed state establishes that a migration may be unnecessary, but not that old and patched nodes agree: a patched proposer can successfully include this IdentityUpdate, whereas an unpatched validator encounters MissingReference and process_proposal rejects proposals containing failed transitions. Preserve the existing v1 insertion and refresh implementations, introduce new generations, and activate them only at the next unreleased protocol boundary. Changing protocol 14's selectors alone would not resolve that disagreement.
In `packages/rs-drive/src/drive/identity/key/fetch/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/drive/identity/key/fetch/mod.rs:958-961: Preserve Unique-mode keys in all-keys queries
The empty-key entry is not always a duplicate alias. For Unique-bound ENCRYPTION and DECRYPTION keys, both the v0 and v1 insertion implementations store the only reference at vec![], without a key-ID entry. Replacing RangeFull with RangeAfter therefore changes AllKeysOfKindRequest from returning that key to returning no keys. The document-type branch below has the same regression, and into_path_query has no PlatformVersion parameter, so selecting protocol 13 does not preserve the previous behavior. These listing requests are not currently consensus or gRPC callers, which limits the severity, but the public Drive fetch API still loses supported results. Preserve Unique-mode results while removing MultipleReferenceToLatest duplicates, retain historical query behavior through version selection wherever semantics differ, and cover both storage modes and both bounds types. Redirecting callers to CurrentKeyOfKindRequest in documentation does not preserve the existing API behavior.
| let sibling_path = identity_contract_info_group_path_key_purpose_vec( | ||
| &identity_id, | ||
| &root_id, | ||
| purpose, | ||
| ); |
There was a problem hiding this comment.
🔴 Blocking: Coalesce alias insertion and refresh during bound-key rotation
An IdentityUpdate that registers a replacement bound ENCRYPTION or DECRYPTION key and disables the previous current key queues registration before revocation. With the corrected paths, registration inserts the replacement alias and revocation then queues an untrusted refresh for exactly the same slot. The coalescer still recognizes only AUTHENTICATION, so GroveDB's batch map keeps the later refresh instead of the insertion. That refresh reads the existing on-disk reference, leaving the disabled key current; enabling batching consistency verification instead rejects the duplicate operations. Although document-type bounds already had this problem, this PR newly exposes it for contract-level MultipleReferenceToLatest keys. Extend alias coalescing through a versioned batch-application path so insertion wins over refresh for both newly enabled purposes, and add same-update registration-plus-revocation tests with consistency verification both enabled and disabled.
source: gpt-6-astra (phase2-reviewer: general)
| let sibling_ref_type_path = SiblingReference(key_id_bytes); | ||
| let sibling_path = if purpose == Purpose::AUTHENTICATION { | ||
| // A bound authentication key's current-key reference belongs beside | ||
| // its key IDs, under the purpose subtree. Keep legacy paths frozen. | ||
| identity_contract_info_group_path_key_purpose_vec( | ||
| &identity_id, | ||
| &root_id, | ||
| purpose, | ||
| ) | ||
| } else { | ||
| identity_contract_info_group_keys_path_vec(&identity_id, &root_id) | ||
| }; | ||
| let sibling_path = identity_contract_info_group_path_key_purpose_vec( | ||
| &identity_id, | ||
| &root_id, | ||
| purpose, | ||
| ); |
There was a problem hiding this comment.
🔴 Blocking: Activate the storage repair through a new protocol version
Both alias writers change their existing v1 implementations while the protocol-14 method table still selects generation 1. CHANGELOG.md explicitly records protocol 14 as shipped in 4.2.0-beta.2, and the versioning book requires shipped behavior to remain frozen. The fact that the broken path never committed state establishes that a migration may be unnecessary, but not that old and patched nodes agree: a patched proposer can successfully include this IdentityUpdate, whereas an unpatched validator encounters MissingReference and process_proposal rejects proposals containing failed transitions. Preserve the existing v1 insertion and refresh implementations, introduce new generations, and activate them only at the next unreleased protocol boundary. Changing protocol 14's selectors alone would not resolve that disagreement.
source: gpt-6-astra (phase2-reviewer: architecture-layering)
| AllKeysOfKindRequest => { | ||
| if purpose == Purpose::AUTHENTICATION { | ||
| // Bound authentication keys keep their current-key alias at the | ||
| // empty key of the purpose subtree; listing must not repeat it. | ||
| Query::new_single_query_item(QueryItem::RangeAfter(vec![]..)) | ||
| } else { | ||
| Query::new_single_query_item(QueryItem::RangeFull(RangeFull)) | ||
| } | ||
| // Every purpose keeps its current-key alias at the empty key of the | ||
| // purpose subtree; listing must not repeat it. | ||
| Query::new_single_query_item(QueryItem::RangeAfter(vec![]..)) |
There was a problem hiding this comment.
🟡 Suggestion: Preserve Unique-mode keys in all-keys queries
The empty-key entry is not always a duplicate alias. For Unique-bound ENCRYPTION and DECRYPTION keys, both the v0 and v1 insertion implementations store the only reference at vec![], without a key-ID entry. Replacing RangeFull with RangeAfter therefore changes AllKeysOfKindRequest from returning that key to returning no keys. The document-type branch below has the same regression, and into_path_query has no PlatformVersion parameter, so selecting protocol 13 does not preserve the previous behavior. These listing requests are not currently consensus or gRPC callers, which limits the severity, but the public Drive fetch API still loses supported results. Preserve Unique-mode results while removing MultipleReferenceToLatest duplicates, retain historical query behavior through version selection wherever semantics differ, and cover both storage modes and both bounds types. Redirecting callers to CurrentKeyOfKindRequest in documentation does not preserve the existing API behavior.
source: gpt-6-astra (phase2-reviewer: general)
github-actions
Bot
added
the
bot-review-skipped
|
Superseded by #4843: same commits and head ( 🤖 Posted autonomously by Claude on behalf of pasta. |
2 participants
Issue being fixed or feature implemented
An
IdentityUpdatethat adds an ENCRYPTION or DECRYPTION key withContractBounds::SingleContractfails inside Drive when the bound contract's config setsrequiresIdentityEncryptionBoundedKey/requiresIdentityDecryptionBoundedKeyto2(StorageKeyRequirements::MultipleReferenceToLatest). Validation passes, the apply hits a grovedbMissingReference, and the transition ends as anInternalError: no nonce bump, no fee, the identity unchanged, and the client's wait times out with no consensus error to show.2is the value the DashPay system contract uses and the value every contract published from the JS/wasm SDK gets by default, so this is the common case. TheUnique(0) andMultiple(1) modes work, and document-type-level bounds (SingleContractDocumentType) work in every mode.Not a protocol-14 regression: the same drop reproduces on testnet today (drive 4.1.0, protocol 13, apply v0), and the lines at fault are unchanged since the feature landed in #1358 (2023). Contract-level bound ENCRYPTION/DECRYPTION keys under opt-in
2have never worked on any network. Yappr hit this in March 2026 and worked around it by registering unbound keys.Cause.
add_potential_contract_info_for_contract_bounded_keyinserts the per-key reference under the purpose subtree, then forMultipleReferenceToLatestadds a "current key" alias as aSiblingReference(key_id)at the empty key. For non-AUTHENTICATION purposes the alias was written one level up, in the keys subtree (identity_contract_info_group_keys_path_vec) instead of the purpose subtree (identity_contract_info_group_path_key_purpose_vec). A sibling reference resolves within its own path, so the alias pointed at.../keys/<key_id>, which does not exist, and grovedb refused the batch. The AUTHENTICATION branch (#4780) and the document-type branch already used the purpose subtree, and the query side (IdentityKeysRequest::into_path_query,ContractBoundKey(_, _, CurrentKeyOfKindRequest)) has always read the alias from the purpose subtree's empty key.refresh_potential_contract_info_key_referencesmirrored the same wrong path.What was done?
add_potential_contract_info_for_contract_bounded_key/v1: the current-key alias goes under the purpose subtree for every purpose, matching the document-type branch and the query.refresh_potential_contract_info_key_references/v1: same path for the refresh written when such a key is disabled.identity/key/fetch/mod.rs:AllKeysOfKindRequestskips the empty-key alias (RangeAfter([]..)) for every purpose, not only AUTHENTICATION, so listing bound ENCRYPTION/DECRYPTION keys no longer returns the newest key twice. Docs updated.contract_info/keys/mod.rs, the low-level batch apply,drive_identity_method_versions/v2.rsandv14.rsupdated to describe the real layout.Versioning.
v1of both methods is fixed in place; nov2, no method-table bump.v1is selected only by protocol 14, and the oldv1path never produced committed state on any network: the batch always failed,prepare_proposalremoved the transition, andprocess_proposalrejects blocks carrying anInternalError. Replay under old and newv1is therefore identical, and no migration is needed.v0(protocols ≤ 13) is untouched. If maintainers prefer av2plus a table bump for protocol 14, that is a mechanical change to this PR.Audit of other alias readers/writers. Estimation helpers already account for the alias at the purpose level (the keys level now matches its
AllSubtreesestimate). The proof verifier andverify_identities_contract_keysreuseidentities_contract_keys_query, which reads the purpose subtree's empty key; no change.AllKeysOfKindRequeston contract-bound requests is not reachable from consensus or gRPC (tests only), so the listing change has no on-chain effect.Not changed, noted for reviewers. Two same-purpose mode-2 keys bound to the same contract in one
IdentityUpdatewould both try to insert the alias (pre-existing; the coalescer only handles AUTHENTICATION).identities_contract_document_type_keys_queryusescontract_idinstead ofgroup_idin its subquery path (pre-existing).How Has This Been Tested?
Regression test
test_identity_update_adding_contract_bound_key_multiple_reference_to_latest(drive-abci): the existing contract-bound key test with the contract's opt-in flipped from0to2. Confirmed failing before the fix with theMissingReferenceerror above. After the fix it registers an ENCRYPTION and a DECRYPTION key in one update with grovedb consistency verification on, asserts the current-key and all-keys queries per purpose, then disables the ENCRYPTION key to exercise the refresh path and re-verifies the tree.New drive test
test_fetch_contract_bound_encryption_and_decryption_keys_with_multiple_reference_to_latest: current key, all keys,fetch_identities_contract_keys, fee estimate ≥ actual, grovedb verify.On-network reproduction before the fix: devnet
moutai(4.2.0-beta.2, protocol 14) and testnet (drive 4.1.0, protocol 13) both drop aSingleContract-bound ENCRYPTION key against a contract opting in with2(wait times out, nonce and balance unchanged, key absent), while opt-in0/1, an unbound control key, and aSingleContractDocumentTypebound key against DashPaycontactRequestare all accepted.Not run: the full workspace suite, strategy tests, a post-fix retest on a live network.
Breaking Changes
None. Nothing ever landed at the old alias path on any network, so state layouts of existing chains are unaffected; only transitions that previously failed now succeed.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code