fix(drive-abci): correct DECRYPTION bounds branch + bill grovedb reads in bounds validation#3697
fix(drive-abci): correct DECRYPTION bounds branch + bill grovedb reads in bounds validation#3697QuantumExplorer wants to merge 5 commits into
Conversation
…ype bounds validation `validate_identity_public_key_contract_bounds` v0 consulted `document_type.requires_identity_encryption_bounded_key()` in the `Purpose::DECRYPTION` arm of `ContractBounds::SingleContractDocumentType`. That branch should consult `requires_identity_decryption_bounded_key()` — asymmetric document-type configs were either wrongly rejected or wrongly accepted depending on which side was set. v1 is a structural copy of v0 with that single call corrected, gated by `validate_identity_public_key_contract_bounds: 1` inside the existing `DRIVE_ABCI_VALIDATION_VERSIONS_V8` (which is referenced only by `PLATFORM_V12`, the in-development 3.1.0 release). Pre-v12 chain replay continues to route through v0 verbatim. Adds a differential test pinning both the legacy v0 behavior and the v1 fix on a doc type that sets `requiresIdentityDecryptionBoundedKey` but not `requiresIdentityEncryptionBoundedKey` — the missing coverage hole that originally let this slip through. Supersedes #2996 (Paul DeLucia's original fix, which couldn't be merged because its v12/v8 scaffolding conflicted with #3017). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a v1 identity public-key contract-bounds validator (fixes DECRYPTION check), wires billing-aware contract and key fetches into Drive and identity update flow, adds differential and billing tests, exposes system-contract cache lookup and contract-fetch outcome, and enables the v1 flag in platform v8. ChangesIdentity Public Key Contract-Bounds Validation v1
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3697 +/- ##
============================================
- Coverage 87.33% 87.32% -0.02%
============================================
Files 2584 2587 +3
Lines 315653 316276 +623
============================================
+ Hits 275679 276176 +497
- Misses 39974 40100 +126
🚀 New features to boost your workflow:
|
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "e109e6e509fdda6f054b95a5e531a0bd1466fe7a0c0d6fcf6f8f70da1ebb119e"
)Xcode manual integration:
|
…ion (audit N6/N7) `validate_identity_public_key_contract_bounds` had an explicit `//todo: we should add to the execution context the cost of fetching contracts` on v0 and discarded every grovedb read (contract fetches + unique-key lookups). Under paid-error semantics, an identity_update that failed bounds validation under-charged the user for the validation work that ran. Audit entries N6 / N7 in `docs/paid-error-fee-audit.md` (PR #3670), tracked by issue #3673. Folded into v1 of the bounds validator (already gated at PROTOCOL_VERSION_12 via DRIVE_ABCI_VALIDATION_VERSIONS_V8 by the previous commit). v0 stays byte-identical for chain replay. - `rs-drive`: new `pub fn fetch_identity_keys_with_costs` (mirrors the existing `fetch_identity_balance_with_costs` precedent — takes `&Epoch`, returns `(T, FeeResult)`). - `rs-drive-abci` v1: switched contract fetches to `get_contract_with_fetch_info_and_fee` and identity-key lookups to the new `fetch_identity_keys_with_costs`, pushing each returned `FeeResult` into `execution_context` via `ValidationOperation::PrecalculatedOperation`. Also refactored the body to factor out the unique-key check into a single helper (was duplicated across four bounds × purpose combinations). - Dispatcher now takes `epoch: &Epoch`. v0 ignores it (preserved). - `identity_update::validate_state_v0` passes `platform.state.last_committed_block_epoch_ref()` to the dispatcher. New test `v1_bills_contract_fetch_and_unique_key_lookup` asserts the execution context receives at least two `PrecalculatedOperation` entries with non-zero processing fees after v1 runs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/state/v0/mod.rs (1)
100-107:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe new billed validation reads still never escape this method.
This call now records
PrecalculatedOperations intostate_transition_execution_context, but the context created at Line 46 is still only a local and is discarded on every return path. So the v1 contract-fetch / key-lookup fees proven in the direct validator tests are not propagated through the real identity-update validation flow, and this path still undercharges those reads.
🧹 Nitpick comments (1)
packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_identity_public_key_contract_bounds/mod.rs (1)
75-76: ⚡ Quick winExercise the dispatcher entrypoint in at least one differential test.
These tests call
validate_identity_public_keys_contract_bounds_v0/_v1directly, so they will not catch a regression in the new1 =>route or theepochforwarding insidevalidate_identity_public_keys_contract_bounds(). One assertion through the dispatcher would cover the integration this PR is actually wiring up.Also applies to: 164-223, 262-270
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_identity_public_key_contract_bounds/mod.rs` around lines 75 - 76, Add a differential test that calls the dispatcher function validate_identity_public_keys_contract_bounds (not the _v0/_v1 helpers) to ensure the 1 => route and epoch forwarding are exercised; update or add a test case that sends inputs which select version 1 so the dispatcher hits the `1 =>` arm and verify outputs/side-effects match calling validate_identity_public_keys_contract_bounds_v1 with the forwarded epoch, mirroring an existing v0 test pattern to assert integration rather than only unit behavior of validate_identity_public_keys_contract_bounds_v0/_v1.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_identity_public_key_contract_bounds/mod.rs`:
- Around line 75-76: Add a differential test that calls the dispatcher function
validate_identity_public_keys_contract_bounds (not the _v0/_v1 helpers) to
ensure the 1 => route and epoch forwarding are exercised; update or add a test
case that sends inputs which select version 1 so the dispatcher hits the `1 =>`
arm and verify outputs/side-effects match calling
validate_identity_public_keys_contract_bounds_v1 with the forwarded epoch,
mirroring an existing v0 test pattern to assert integration rather than only
unit behavior of validate_identity_public_keys_contract_bounds_v0/_v1.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f60645c-ea25-4767-acc2-7339fa3043d4
📒 Files selected for processing (5)
packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_identity_public_key_contract_bounds/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/common/validate_identity_public_key_contract_bounds/v1/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/state/v0/mod.rspackages/rs-drive/src/drive/identity/key/fetch/fetch_identity_keys/mod.rspackages/rs-drive/src/drive/identity/key/fetch/fetch_identity_keys/v0/mod.rs
QuantumExplorer
changed the title
…cts free
Bounds validation, and other validators that look up a bound contract, only
need to bill grovedb reads when the target is a user contract. System
contracts (DPNS, Dashpay, Withdrawals, MasternodeRewards, TokenHistory,
KeywordSearch) live in `Drive.cache.system_data_contracts` as an in-memory
`ArcSwap`; serving them touches no storage, so no fee should be charged.
This commit:
- `rs-drive`: new versioned `Drive::get_system_or_user_contract_with_fee`
that returns a `FetchedContract` enum:
* `FetchedContract::System(Arc<DataContract>)` — served from the
in-memory cache, no fee.
* `FetchedContract::User { fetch_info, fee }` — fetched from grovedb,
`fee` covers the read.
Gated by a new `DriveContractGetMethodVersions::get_system_or_user_contract_with_fee`
field (initialised to 0 in V1/V2/V3 since this is the first impl).
- `rs-drive`: `SystemDataContracts::find_by_id(id)` lookup over the six
cached system-contract IDs.
- `rs-drive-abci` v1 of bounds validation now uses the new method instead
of `get_contract_with_fetch_info_and_fee`. Only the `User` arm pushes
a `PrecalculatedOperation` into the execution context; `System` is free.
New test `v1_does_not_bill_contract_fetch_for_system_contract` exercises
DPNS via `ContractBounds::SingleContract` and asserts the execution
context receives zero billing entries (vs. the user-contract case which
emits ≥ 2).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/rs-drive/src/drive/contract/get_fetch/get_system_or_user_contract_with_fee/v0/mod.rs`:
- Around line 37-38: The match arm returning `(None, _) => Ok(None)` in the
result of `get_contract_with_fetch_info_and_fee` lets fee values get dropped;
update pattern handling so that when fetch_info is None but fee is Some(fee) you
return a user-miss variant carrying the fee (e.g.,
`FetchedContract::UserNotFound { fee }`) instead of Ok(None), and keep the
existing `(Some(fetch_info), Some(fee)) => Ok(Some(FetchedContract::User {
fetch_info, fee }))` case; adjust callers that consume `FetchedContract` to
always bill the returned fee for user-path reads (including not-found) so
DataContractNotPresentError paths receive a PrecalculatedOperation with the fee.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2dfd2034-5cdf-414b-b310-79efc3521bf1
📒 Files selected for processing (11)
packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_identity_public_key_contract_bounds/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/common/validate_identity_public_key_contract_bounds/v1/mod.rspackages/rs-drive/src/cache/system_contracts.rspackages/rs-drive/src/drive/contract/get_fetch/get_system_or_user_contract_with_fee/mod.rspackages/rs-drive/src/drive/contract/get_fetch/get_system_or_user_contract_with_fee/v0/mod.rspackages/rs-drive/src/drive/contract/get_fetch/mod.rspackages/rs-drive/src/drive/contract/mod.rspackages/rs-platform-version/src/version/drive_versions/drive_contract_method_versions/mod.rspackages/rs-platform-version/src/version/drive_versions/drive_contract_method_versions/v1.rspackages/rs-platform-version/src/version/drive_versions/drive_contract_method_versions/v2.rspackages/rs-platform-version/src/version/drive_versions/drive_contract_method_versions/v3.rs
`get_system_or_user_contract_with_fee_v0` was returning `Ok(None)` for the
not-found case, dropping the fee from the failed grovedb probe on the
floor. The probe itself still ran and still cost something.
Reshaped the return type:
- Removed the outer `Option<FetchedContract>`.
- Renamed `FetchedContract` → `ContractFetchOutcome` and added a
`NotFound { fee: FeeResult }` variant alongside `System(...)` and
`User { fetch_info, fee }`.
- `ContractFetchOutcome::fee()` now returns `Some(&fee)` for both `User`
and `NotFound` (and `None` only for `System`, which never touches
storage). `contract()` returns `Option<&DataContract>` — `None` only
for `NotFound`.
Updated v1 of bounds validation accordingly: it bills `outcome.fee()`
unconditionally, then checks `outcome.contract()` for the not-present
path. The new flow naturally bills the probe even for missing contracts.
New test `v1_bills_lookup_when_contract_not_found` exercises a random
non-system contract id and asserts the execution context receives
exactly one `PrecalculatedOperation` with a non-zero processing fee,
plus the expected `DataContractNotPresentError`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Thanks @coderabbitai. Triaging the two findings from this review: Major (outside-diff) — "new billed validation reads still never escape this method." Accurate, and broader than this PR. Nitpick — "exercise the dispatcher entrypoint." Adding a dispatcher-level test in the next commit on this branch. |
|
Major (dropped-local execution_context): Understood. The billing wiring in this PR is correct in isolation; the outer-context plumbing to make those fees actually reach the user's bill is properly scoped to Nitpick (dispatcher-level test): Acknowledged — looking forward to seeing it in the next commit. I'll go ahead and resolve the major comment so the review doesn't show it as open noise against this PR. ✏️ Learnings added
✅ Actions performedComments resolved. Approval is disabled; enable |
…s CodeRabbit nitpick) Prior tests called validate_identity_public_keys_contract_bounds_v0 / _v1 directly and would not catch a regression in the v1 dispatcher arm or the new `epoch` forwarding. Adds `dispatcher_routes_to_v1_at_latest_platform_version`, which: - Sanity-checks that PlatformVersion::latest() selects v1 of the bounds-validator field (test premise; fails loudly if the version field moves). - Calls the public `validate_identity_public_keys_contract_bounds` dispatcher against the asymmetric-decryption-bounds contract used by the v0/v1 differential test, and asserts the result is valid — proving the dispatcher routed to v1 (v0 would return DataContractBoundsNotPresentError here per the existing pinned-v0 test). - Asserts the execution context received ≥ 1 PrecalculatedOperation entry, proving the epoch parameter is forwarded through to v1 (v0 wouldn't bill). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The DECRYPTION correctness fix is implemented and correctly gated to PROTOCOL_VERSION_12 via the V8 version table bump. However, the committed v1 is otherwise byte-identical to v0: the fee-accounting work the PR description and title prominently advertise (FetchedContract enum, get_system_or_user_contract_with_fee, fetch_identity_keys_with_costs, SystemDataContracts::find_by_id, check_unique_bound_key helper, dispatcher epoch: &Epoch) is entirely absent from the diff, as are the two billing tests the description claims were added. PR description must be narrowed to match the actual scope, or the missing work must land before merge.
🔴 1 blocking | 🟡 2 suggestion(s) | 💬 1 nitpick(s)
4 additional finding(s)
blocking: v1 advertised as adding grovedb-read billing and system-contract waiver, but the committed code ships neither
packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_identity_public_key_contract_bounds/v1/mod.rs (line 57)
The PR title (+ bill grovedb reads in bounds validation) and description claim three coupled fixes activate together at PROTOCOL_VERSION_12: the DECRYPTION correctness fix, fee accounting for contract/key reads (audit entries N6/N7), and a system-contract waiver. Only the first is present in the diff. v1 is byte-identical to v0 except for the one-line requires_identity_decryption_bounded_key() accessor change:
- line 62 still binds
_execution_context(unused) - line 65 still carries the verbatim
//todo: we should add to the execution context the cost of fetching contracts - lines 71 and 189 still call the non-billing
get_contract_with_fetch_info(_, false, _, _) - lines 115, 158, 238, and 274 still call
fetch_identity_keys(...)instead of a cost-tracking variant
None of the described new APIs are introduced: there is no FetchedContract enum, no Drive::get_system_or_user_contract_with_fee, no fetch_identity_keys_with_costs, no SystemDataContracts::find_by_id, no check_unique_bound_key helper, and no epoch: &Epoch parameter on the dispatcher. The description also asserts four tests were added; only the two DECRYPTION pinning tests exist — the claimed v1_bills_contract_fetch_and_unique_key_lookup and v1_does_not_bill_contract_fetch_for_system_contract are absent.
Because DRIVE_ABCI_VALIDATION_VERSIONS_V8.validate_identity_public_key_contract_bounds is bumped 0→1 (referenced only by PLATFORM_V12), the version gate at v12 activates only the DECRYPTION correctness fix. Note this does not make under-billing worse than v0 — v0 had the same TODO and the same unbilled reads — but the PR represents itself as closing that audit gap and does not. Either land the billing/waiver changes the description promises, or narrow the PR title/description to the correctness fix and track N6/N7 separately so reviewers and downstream consumers aren't misled into believing the audit items are closed.
suggestion: v1 regression coverage only exercises the one branch that was changed
packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_identity_public_key_contract_bounds/mod.rs (line 55)
v1 was authored by copy-pasting v0 and editing exactly one accessor on the DECRYPTION/SingleContractDocumentType branch. The new tests in mod.rs::tests (lines 123-214) exercise only that single branch. Nothing exercises v1 on the other branches that were duplicated verbatim: SingleContract/ENCRYPTION, SingleContract/DECRYPTION, SingleContractDocumentType/ENCRYPTION, the contract-not-present cases, the unknown-document-type case, or the invalid-purpose case. The existing v0 tests still invoke validate_identity_public_keys_contract_bounds_v0 directly and do not run v1 at all.
Additionally, the dispatcher in mod.rs is never invoked — the tests bypass it and call v0/v1 directly — so there is no assertion that PLATFORM_V12 actually routes to v1. The fact that this PR's own correctness fix is a one-character copy-paste oversight makes parameterizing the v0 suite over both versions (or duplicating it for v1) a quality gate rather than a polish step: it would catch any unintended typo in v1's copy and pin the v0↔v1 contract at the dispatcher level.
suggestion: Four-way duplicated unique-key conflict check should be factored into a helper
packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_identity_public_key_contract_bounds/v1/mod.rs (line 100)
The same block — build IdentityKeysRequest with ContractBoundKey or ContractDocumentTypeBoundKey, call drive.fetch_identity_keys::<OptionalSingleIdentityPublicKeyOutcome>(...), then construct IdentityPublicKeyAlreadyExistsForUniqueContractBoundsError — appears four times in v1 (SingleContract/ENCRYPTION at 105-121, SingleContract/DECRYPTION at 148-164, SingleContractDocumentType/ENCRYPTION at 232-250, SingleContractDocumentType/DECRYPTION at 268-286). The PR description claims this was refactored into a check_unique_bound_key helper. Whether or not that helper lands, the duplication is a real defect-multiplier: the original DECRYPTION bug being fixed here is itself a symptom of the same asymmetry — someone updated one of two near-identical branches and missed the symmetry. Extracting a helper that takes (KeyRequestType, ...) and returns Result<SimpleConsensusValidationResult, Error> would collapse all four arms to a few lines each and make this class of copy-paste bug mechanically impossible.
nitpick: Module doc comment accurately describes the committed code but contradicts the PR title and description
packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_identity_public_key_contract_bounds/v1/mod.rs (line 1)
The doc comment at lines 1-3 says v1 only fixes the DECRYPTION branch and intentionally keeps the rest of the file structurally identical to v0. That is an accurate, audit-friendly summary of the committed diff. The discrepancy is with the PR title (+ bill grovedb reads in bounds validation) and description (three-fix scope including fee accounting and a system-contract waiver). Resolve before merge so the doc comment and the PR framing agree on the actual scope.
🤖 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.
- [BLOCKING] In `packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_identity_public_key_contract_bounds/v1/mod.rs`:57-76: v1 advertised as adding grovedb-read billing and system-contract waiver, but the committed code ships neither
The PR title (`+ bill grovedb reads in bounds validation`) and description claim three coupled fixes activate together at PROTOCOL_VERSION_12: the DECRYPTION correctness fix, fee accounting for contract/key reads (audit entries N6/N7), and a system-contract waiver. Only the first is present in the diff. v1 is byte-identical to v0 except for the one-line `requires_identity_decryption_bounded_key()` accessor change:
- line 62 still binds `_execution_context` (unused)
- line 65 still carries the verbatim `//todo: we should add to the execution context the cost of fetching contracts`
- lines 71 and 189 still call the non-billing `get_contract_with_fetch_info(_, false, _, _)`
- lines 115, 158, 238, and 274 still call `fetch_identity_keys(...)` instead of a cost-tracking variant
None of the described new APIs are introduced: there is no `FetchedContract` enum, no `Drive::get_system_or_user_contract_with_fee`, no `fetch_identity_keys_with_costs`, no `SystemDataContracts::find_by_id`, no `check_unique_bound_key` helper, and no `epoch: &Epoch` parameter on the dispatcher. The description also asserts four tests were added; only the two DECRYPTION pinning tests exist — the claimed `v1_bills_contract_fetch_and_unique_key_lookup` and `v1_does_not_bill_contract_fetch_for_system_contract` are absent.
Because `DRIVE_ABCI_VALIDATION_VERSIONS_V8.validate_identity_public_key_contract_bounds` is bumped 0→1 (referenced only by `PLATFORM_V12`), the version gate at v12 activates only the DECRYPTION correctness fix. Note this does not make under-billing worse than v0 — v0 had the same TODO and the same unbilled reads — but the PR represents itself as closing that audit gap and does not. Either land the billing/waiver changes the description promises, or narrow the PR title/description to the correctness fix and track N6/N7 separately so reviewers and downstream consumers aren't misled into believing the audit items are closed.
- [SUGGESTION] In `packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_identity_public_key_contract_bounds/mod.rs`:55-215: v1 regression coverage only exercises the one branch that was changed
v1 was authored by copy-pasting v0 and editing exactly one accessor on the DECRYPTION/SingleContractDocumentType branch. The new tests in `mod.rs::tests` (lines 123-214) exercise only that single branch. Nothing exercises v1 on the other branches that were duplicated verbatim: SingleContract/ENCRYPTION, SingleContract/DECRYPTION, SingleContractDocumentType/ENCRYPTION, the contract-not-present cases, the unknown-document-type case, or the invalid-purpose case. The existing v0 tests still invoke `validate_identity_public_keys_contract_bounds_v0` directly and do not run v1 at all.
Additionally, the dispatcher in `mod.rs` is never invoked — the tests bypass it and call v0/v1 directly — so there is no assertion that PLATFORM_V12 actually routes to v1. The fact that this PR's own correctness fix is a one-character copy-paste oversight makes parameterizing the v0 suite over both versions (or duplicating it for v1) a quality gate rather than a polish step: it would catch any unintended typo in v1's copy and pin the v0↔v1 contract at the dispatcher level.
- [SUGGESTION] In `packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_identity_public_key_contract_bounds/v1/mod.rs`:100-286: Four-way duplicated unique-key conflict check should be factored into a helper
The same block — build `IdentityKeysRequest` with `ContractBoundKey` or `ContractDocumentTypeBoundKey`, call `drive.fetch_identity_keys::<OptionalSingleIdentityPublicKeyOutcome>(...)`, then construct `IdentityPublicKeyAlreadyExistsForUniqueContractBoundsError` — appears four times in v1 (SingleContract/ENCRYPTION at 105-121, SingleContract/DECRYPTION at 148-164, SingleContractDocumentType/ENCRYPTION at 232-250, SingleContractDocumentType/DECRYPTION at 268-286). The PR description claims this was refactored into a `check_unique_bound_key` helper. Whether or not that helper lands, the duplication is a real defect-multiplier: the original DECRYPTION bug being fixed here is itself a symptom of the same asymmetry — someone updated one of two near-identical branches and missed the symmetry. Extracting a helper that takes `(KeyRequestType, ...)` and returns `Result<SimpleConsensusValidationResult, Error>` would collapse all four arms to a few lines each and make this class of copy-paste bug mechanically impossible.
- [NITPICK] In `packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_identity_public_key_contract_bounds/v1/mod.rs`:1-3: Module doc comment accurately describes the committed code but contradicts the PR title and description
The doc comment at lines 1-3 says v1 only fixes the DECRYPTION branch and intentionally keeps the rest of the file structurally identical to v0. That is an accurate, audit-friendly summary of the committed diff. The discrepancy is with the PR title (`+ bill grovedb reads in bounds validation`) and description (three-fix scope including fee accounting and a system-contract waiver). Resolve before merge so the doc comment and the PR framing agree on the actual scope.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Codex flagged a real blocking issue: IdentityUpdateTransition::validate_state ignores the caller's execution context (_execution_context param at identity_update/mod.rs:99) and calls validate_state_v0 without forwarding it; validate_state_v0 then creates a fresh local context (state/v0/mod.rs:46-47) into which the new v1 bounds-validation PrecalculatedOperations are pushed and silently discarded on return. The pattern is divergent from batch/mod.rs:207,234, which correctly threads the outer context. The PR thus advertises a billing fix for audit N6/N7 that does not actually reach the processor's state_transition_execution_context used to build the ExecutionEvent (processor/v0/mod.rs:329,350), reopening the paid-error DoS gap for identity_update. Remaining findings are low-severity test-coverage and defense-in-depth observations.
🔴 1 blocking | 🟡 2 suggestion(s) | 💬 1 nitpick(s)
4 additional finding(s)
blocking: identity_update bounds billing is dropped: `validate_state` ignores the outer execution context and `validate_state_v0` creates a throwaway local one
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/mod.rs (line 92)
IdentityUpdateTransition::validate_state declares _execution_context: &mut StateTransitionExecutionContext (note the leading underscore) at identity_update/mod.rs:99 and dispatches to self.validate_state_v0(platform, tx, platform_version) at line 110 without passing the context. Inside validate_state_v0 (state/v0/mod.rs:46-47), a fresh state_transition_execution_context is constructed and threaded into every inner validator, including the new validate_identity_public_keys_contract_bounds(..., &mut state_transition_execution_context, ...) call at lines 99-110. That local context is dropped when validate_state_v0 returns, so the PrecalculatedOperation entries the v1 bounds validator pushes for get_contract_with_fetch_info_and_fee and fetch_identity_keys_with_costs never reach the outer state_transition_execution_context that processor/v0/mod.rs:329 passes in and processor/v0/mod.rs:350 hands to ExecutionEvent::create_from_state_transition_action.
Net effect: the PR's claimed fix for audit items N6/N7 does not bind in the real execution path for identity_update. The v1 dispatcher routing test in validate_identity_public_key_contract_bounds/mod.rs:445-513 only proves the validator pushes billing into the context it was handed — it does not exercise the processor pipeline, where the billing is silently dropped. As a consequence, a identity_update state transition that passes nonce/signature checks and then triggers contract-bounds grovedb reads (contract fetches and uniqueness key lookups) is still effectively un-billed for those reads, restoring the paid-error DoS leverage this PR is meant to close.
The surrounding pattern is already correct in batch/mod.rs:207,234, which propagates the caller's execution_context through to validate_state_v0. The fix is to drop the underscore on the trait parameter and forward it the same way (and adjust validate_state_v0's signature to accept and use the caller's context instead of constructing a fresh one).
suggestion: v1 branches still lack differential coverage for the matrix that produced this PR's bug
packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_identity_public_key_contract_bounds/mod.rs (line 66)
The new tests cover the previously-buggy DECRYPTION + SingleContractDocumentType branch, the system-contract free-fetch path, the not-found billed path, and the dispatcher route. They do not exercise:
SingleContract+ ENCRYPTION +Uniquehappy path (OK throughcheck_unique_bound_key).SingleContract+ DECRYPTION +Uniquehappy path.SingleContractDocumentType+ ENCRYPTION +Unique(symmetric to the v0 bug being fixed).- Uniqueness-conflict path producing
IdentityPublicKeyAlreadyExistsForUniqueContractBoundsError. InvalidDocumentTypeError(unknowndocument_type_name) andInvalidKeyPurposeForContractBoundsError(e.g.Purpose::AUTHENTICATIONagainstSingleContract).
The bug this PR fixes is itself a copy-paste asymmetry between the two DECRYPTION branches. A single parametrized suite walking the (bound_variant × purpose × requirements) matrix would mechanically prevent the next such regression. Helper extraction collapsed the duplication that motivated the original concern, so this is no longer a structural risk — just a future-proofing gap.
nitpick: `v1_does_not_bill_contract_fetch_for_system_contract` conflates 'no contract-fetch fee' with 'no fees at all'
packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_identity_public_key_contract_bounds/mod.rs (line 234)
The test asserts billed.is_empty() for an ENCRYPTION key bound to DPNS with ContractBounds::SingleContract. This relies on the implicit premise that DPNS configures neither requires_identity_encryption_bounded_key nor requires_identity_decryption_bounded_key, so execution short-circuits via DataContractBoundsNotPresentError before the uniqueness lookup runs. If a future DPNS schema (or another system contract used in this test) sets an encryption bound, the uniqueness branch fires, fetch_identity_keys_with_costs runs against grovedb, a PrecalculatedOperation is appended, and the test breaks for a reason unrelated to the system-contract-fetch waiver it is meant to pin. The test's comment already names the dependency. A more robust assertion would isolate the contract-fetch slot from the key-lookup slot (e.g., snapshot operation count immediately before vs. after the system-contract resolution path, or filter on a contract-fetch-specific marker), keeping the regression-protection scope narrow.
suggestion: `SystemDataContracts::find_by_id` ignores `active_since_protocol_version` — latent consensus footgun
packages/rs-drive/src/cache/system_contracts.rs (line 181)
SystemDataContracts::find_by_id returns the cached Arc<DataContract> for any matching system-contract ID without consulting ActiveSystemDataContract::active_since_protocol_version. Today this is safe because (a) the only caller, get_system_or_user_contract_with_fee_v0, is only reached through validate_identity_public_keys_contract_bounds_v1, which is gated to PROTOCOL_VERSION_12, and (b) all currently-cached system contracts activate at versions ≤ 9.
This becomes a consensus risk if a future protocol version (a) adds a system contract whose activation is later than the bounds-validator version, or (b) reuses find_by_id from a code path that runs before the contract is active. In that scenario the helper would short-circuit the grovedb lookup for an ID that is not yet present in storage, returning a contract object that diverges from what grovedb-based code paths see on other validators — a consensus-divergence shape. Threading the current PlatformVersion (or ProtocolVersion) through the lookup and filtering on active_since_protocol_version would make the safety invariant local to the function signature and self-evident to future callers. Not blocking for v12, but worth hardening before the next system contract lands.
🤖 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.
- [BLOCKING] In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/mod.rs`:92-117: identity_update bounds billing is dropped: `validate_state` ignores the outer execution context and `validate_state_v0` creates a throwaway local one
`IdentityUpdateTransition::validate_state` declares `_execution_context: &mut StateTransitionExecutionContext` (note the leading underscore) at identity_update/mod.rs:99 and dispatches to `self.validate_state_v0(platform, tx, platform_version)` at line 110 without passing the context. Inside `validate_state_v0` (state/v0/mod.rs:46-47), a fresh `state_transition_execution_context` is constructed and threaded into every inner validator, including the new `validate_identity_public_keys_contract_bounds(..., &mut state_transition_execution_context, ...)` call at lines 99-110. That local context is dropped when `validate_state_v0` returns, so the `PrecalculatedOperation` entries the v1 bounds validator pushes for `get_contract_with_fetch_info_and_fee` and `fetch_identity_keys_with_costs` never reach the outer `state_transition_execution_context` that processor/v0/mod.rs:329 passes in and processor/v0/mod.rs:350 hands to `ExecutionEvent::create_from_state_transition_action`.
Net effect: the PR's claimed fix for audit items N6/N7 does not bind in the real execution path for `identity_update`. The v1 dispatcher routing test in `validate_identity_public_key_contract_bounds/mod.rs:445-513` only proves the validator pushes billing into the context it was handed — it does not exercise the processor pipeline, where the billing is silently dropped. As a consequence, a `identity_update` state transition that passes nonce/signature checks and then triggers contract-bounds grovedb reads (contract fetches and uniqueness key lookups) is still effectively un-billed for those reads, restoring the paid-error DoS leverage this PR is meant to close.
The surrounding pattern is already correct in `batch/mod.rs:207,234`, which propagates the caller's `execution_context` through to `validate_state_v0`. The fix is to drop the underscore on the trait parameter and forward it the same way (and adjust `validate_state_v0`'s signature to accept and use the caller's context instead of constructing a fresh one).
- [SUGGESTION] In `packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_identity_public_key_contract_bounds/mod.rs`:66-514: v1 branches still lack differential coverage for the matrix that produced this PR's bug
The new tests cover the previously-buggy DECRYPTION + SingleContractDocumentType branch, the system-contract free-fetch path, the not-found billed path, and the dispatcher route. They do not exercise:
- `SingleContract` + ENCRYPTION + `Unique` happy path (OK through `check_unique_bound_key`).
- `SingleContract` + DECRYPTION + `Unique` happy path.
- `SingleContractDocumentType` + ENCRYPTION + `Unique` (symmetric to the v0 bug being fixed).
- Uniqueness-conflict path producing `IdentityPublicKeyAlreadyExistsForUniqueContractBoundsError`.
- `InvalidDocumentTypeError` (unknown `document_type_name`) and `InvalidKeyPurposeForContractBoundsError` (e.g. `Purpose::AUTHENTICATION` against `SingleContract`).
The bug this PR fixes is itself a copy-paste asymmetry between the two `DECRYPTION` branches. A single parametrized suite walking the `(bound_variant × purpose × requirements)` matrix would mechanically prevent the next such regression. Helper extraction collapsed the duplication that motivated the original concern, so this is no longer a structural risk — just a future-proofing gap.
- [NITPICK] In `packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_identity_public_key_contract_bounds/mod.rs`:234-299: `v1_does_not_bill_contract_fetch_for_system_contract` conflates 'no contract-fetch fee' with 'no fees at all'
The test asserts `billed.is_empty()` for an `ENCRYPTION` key bound to DPNS with `ContractBounds::SingleContract`. This relies on the implicit premise that DPNS configures neither `requires_identity_encryption_bounded_key` nor `requires_identity_decryption_bounded_key`, so execution short-circuits via `DataContractBoundsNotPresentError` before the uniqueness lookup runs. If a future DPNS schema (or another system contract used in this test) sets an encryption bound, the uniqueness branch fires, `fetch_identity_keys_with_costs` runs against grovedb, a `PrecalculatedOperation` is appended, and the test breaks for a reason unrelated to the system-contract-fetch waiver it is meant to pin. The test's comment already names the dependency. A more robust assertion would isolate the contract-fetch slot from the key-lookup slot (e.g., snapshot operation count immediately before vs. after the system-contract resolution path, or filter on a contract-fetch-specific marker), keeping the regression-protection scope narrow.
- [SUGGESTION] In `packages/rs-drive/src/cache/system_contracts.rs`:181-200: `SystemDataContracts::find_by_id` ignores `active_since_protocol_version` — latent consensus footgun
`SystemDataContracts::find_by_id` returns the cached `Arc<DataContract>` for any matching system-contract ID without consulting `ActiveSystemDataContract::active_since_protocol_version`. Today this is safe because (a) the only caller, `get_system_or_user_contract_with_fee_v0`, is only reached through `validate_identity_public_keys_contract_bounds_v1`, which is gated to PROTOCOL_VERSION_12, and (b) all currently-cached system contracts activate at versions ≤ 9.
This becomes a consensus risk if a future protocol version (a) adds a system contract whose activation is later than the bounds-validator version, or (b) reuses `find_by_id` from a code path that runs before the contract is active. In that scenario the helper would short-circuit the grovedb lookup for an ID that is not yet present in storage, returning a contract object that diverges from what grovedb-based code paths see on other validators — a consensus-divergence shape. Threading the current `PlatformVersion` (or `ProtocolVersion`) through the lookup and filtering on `active_since_protocol_version` would make the safety invariant local to the function signature and self-evident to future callers. Not blocking for v12, but worth hardening before the next system contract lands.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
3 participants
Issue being fixed or feature implemented
Three related fixes to
validate_identity_public_key_contract_bounds, all gated together at PROTOCOL_VERSION_12 via the same V8 validation-table bump.1. Correctness — DECRYPTION branch reads wrong bound. Supersedes #2996. v0 consulted
document_type.requires_identity_encryption_bounded_key()in thePurpose::DECRYPTIONarm ofContractBounds::SingleContractDocumentType. That arm should consultrequires_identity_decryption_bounded_key().2. Fee accounting — discarded grovedb reads (audit N6/N7). v0 had an explicit
//todo: we should add to the execution context the cost of fetching contracts. Every contract fetch and unique-key lookup inside the function ran without billing. Under the paid-error semantics introduced by #3616, anidentity_updatethat failed bounds validation under-charged the user for the validation work performed. Captured indocs/paid-error-fee-audit.md(PR #3670) as entries N6 / N7; tracked as a follow-up by #3673.3. System contracts are fee-free. System contracts (DPNS, Dashpay, Withdrawals, MasternodeRewards, TokenHistory, KeywordSearch) live in
Drive.cache.system_data_contractsas an in-memoryArcSwap. Serving them touches no storage, so users shouldn't be charged when their key is bound to a system contract.What was done?
Correctness fix
v1/mod.rscallsrequires_identity_decryption_bounded_key()in theDECRYPTIONarm ofSingleContractDocumentType.Fee accounting + system-contract waiver
Drive::get_system_or_user_contract_with_feereturning aFetchedContractenum:FetchedContract::System(Arc<DataContract>)— served from the in-memory cache, no fee.FetchedContract::User { fetch_info, fee }— fetched from grovedb,feecovers the read.DriveContractGetMethodVersions::get_system_or_user_contract_with_feefield (set to 0 in V1/V2/V3 since this is the first impl).pub fn fetch_identity_keys_with_costsonDrive, mirroring the existingfetch_identity_balance_with_costsprecedent — takes&Epoch, returns(T, FeeResult).SystemDataContracts::find_by_id(id)lookup over the six cached system-contract IDs.v1uses both: short-circuits to the system-contract cache when the bound points to one (free); falls back to the user-contract path otherwise (billed). Unique-key lookups go throughfetch_identity_keys_with_costsand are always billed.check_unique_bound_keyhelper (was duplicated 4×).Dispatcher / caller
epoch: &Epoch. v0 explicitly ignores it (let _ = epoch;) — v0 stays byte-identical, no change to chain replay for protocol versions ≤ 11.identity_update::validate_state_v0passesplatform.state.last_committed_block_epoch_ref()through.Version gating
DRIVE_ABCI_VALIDATION_VERSIONS_V8.validate_identity_public_key_contract_bounds: 0 → 1. V8 is referenced only byPLATFORM_V12, so:How Has This Been Tested?
Four tests in the dispatcher's
testsmod (which has visibility into both v0 and v1):v0_wrongly_rejects_decryption_key_when_only_decryption_bounds_set— pins v0's legacy (wrong) behavior for chain replay safety.v1_accepts_decryption_key_when_decryption_bounds_present— asserts v1's correctness fix.v1_bills_contract_fetch_and_unique_key_lookup— for a user contract, the execution context receives ≥ 2PrecalculatedOperationentries with non-zero total processing fee.v1_does_not_bill_contract_fetch_for_system_contract— for a bound pointing at DPNS, the execution context receives zero billing entries.cargo fmt --all,cargo check, andcargo clippy --lib --tests -- -D warningsall clean on bothdriveanddrive-abci.Breaking Changes
None at the chain level. v0 behavior is preserved verbatim for protocol versions ≤ 11. Corrected behavior + fee accounting + system-contract waiver activate together at PROTOCOL_VERSION_12.
API-level:
Drivegains two new public methods (get_system_or_user_contract_with_fee,fetch_identity_keys_with_costs) and one re-exported enum (FetchedContract). The internal validator's dispatcher signature gains anepoch: &Epochparameter; the only caller in-tree has been updated.Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests