feat(dpp)!: dashpay payment detection keys and stealth derivation - #4272
feat(dpp)!: dashpay payment detection keys and stealth derivation#4272QuantumExplorer wants to merge 15 commits into
Conversation
… cache A read-only query thread fetches contracts from committed state with no transaction and populates the global cache from what it read. If such a thread reads a contract, is descheduled while block execution rewrites that contract, and performs its insert only after the block cache is promoted to the global cache, the unconditional insert clobbered the newer contract with the stale one. Block execution would then serialize documents against a different contract than a node whose cache was cold — the same consensus divergence the v13 DPNS refresh closes, reopened through a millisecond-scale scheduling window that exists on every contract rewrite, and that an attacker could widen at the predictable activation height by flooding document queries. Contract versions increase strictly monotonically (the data contract update transition enforces new == old + 1, and the v13 DPNS rewrite goes 1 -> 2), so an insert carrying a lower version than the cached entry is always a delayed writer racing a newer copy in, never fresh information. The insert now skips in that case, atomically per key via moka's compute API so the version comparison and the write have no window between them. Same-version inserts still overwrite, which the cache-hit fee-calculation path relies on. This closes the race for the ordinary contract-update path as well, where the batch finalization task evicts the superseded contract during block execution and the same delayed query insert could land after promotion. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Implements the transparent-rail (Core + Platform address) half of DIP-33: profile payment address fields, the two payment key purposes, and the client-side stealth one-time address derivation. DashPay contract v2 (protocol version 14): * profile gains optional corePaymentAddress (Base58 string) and platformPaymentAddress (21-byte DIP-18 storage form), positions 5-6. * Mirrors the DPNS-v2-at-PV13 mechanics: schema/v2, v2 loader, a new system_data_contract_versions v3 (dashpay: 2), and a transition_to_version_14 first-block migration that re-applies the DashPay contract. Reuses the SystemDataContract::ALL cache refresh, so the migration inherits the monotonic data-contract-cache guard that protects the DPNS v13 activation. Identity key purposes PAYMENT_SCAN (7) and PAYMENT_SPEND (8): * ECDSA_SECP256K1, non-signing, no contract bounds, at most one active key of each purpose per identity. Not searchable (no per-purpose key reference tree), found by fetching the identity's keys. * Gated at protocol version 14 via validate_identity_public_keys_structure v1 (in-transition rules) plus a validate_payment_key_uniqueness common state check wired into the identity update add path. Two new consensus errors: InvalidKeyPurposeKeyType (10535), TooManyPublicKeysOfPurpose (10536). Mirrored in wasm-dpp / wasm-dpp2. Stealth derivation: * platform-encryption::stealth — pure secp256k1 curve math (shared point, rail-domain-separated one-time tweak, one-time pub/secret keys) with DIP-33 known-answer test vectors. * platform-wallet stealth module — DIP-9 feature 33' key derivation and Core P2PKH / Platform one-time destination construction (payer, receiver, spender). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR adds DashPay v2 schemas, DIP-33 payment purposes and stealth payment derivation, payment-key validation, protocol v14 migration behavior, and monotonic contract-cache updates. ChangesDashPay v2 and payment-purpose contracts
DIP-33 stealth derivation
Payment-key validation
Protocol v14 and cache migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/dashpay-tips-jar #4272 +/- ##
=========================================================
- Coverage 87.63% 87.06% -0.58%
=========================================================
Files 2670 2645 -25
Lines 339591 337926 -1665
=========================================================
- Hits 297604 294211 -3393
- Misses 41987 43715 +1728
🚀 New features to boost your workflow:
|
QuantumExplorer
changed the title
|
⛔ Blockers found — Opus deferred (commit 79154a2) |
The PAYMENT_SCAN/PAYMENT_SPEND variants carry underscores, which trips clippy's non_camel_case_types lint (single-word all-caps variants do not). Matches the allow already on the rs-dpp Purpose enum. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ress key_wallet::Address has no pubkey_hash(); extract the P2PKH hash via script_pubkey().p2pkh_public_key_hash_bytes() as elsewhere in the crate. Only the test target used it, so lib check/clippy did not catch it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tests The pattern was enforced at consensus (verified: documents carrying a non-Base58 value were rejected with a JsonSchemaError), but it could not prove the checksum or network byte, so clients had to fully validate the address regardless. What it did do was make every random document generator using FillIfNotRequired emit schema-invalid dashpay profiles, breaking 16 unrelated document tests — a permanent tax on anyone generating a profile fixture. The byteArray platformPaymentAddress field broke nothing, which is the asymmetry that surfaced this. Consensus now constrains the length only; validating the address proper is explicitly a client responsibility (DIP-33 updated to match). Rebaselined for the larger PV14 dashpay contract, all latest-version only — historical protocol-version pins are untouched so chain replay stays bit-for-bit reproducible: * check_tx contract create/update processing fees * deterministic root hash after contract insertion * happy-path replace and delete fee baselines * expected profile document string representations, which now include the two new optional fields Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CI fixes + one design change worth a lookThree CI rounds fixed. The third surfaced something I think is a genuine improvement, so flagging it explicitly rather than burying it in a commit. Dropped the Base58
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/rs-platform-encryption/src/lib.rs (1)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe intra-doc link points at a private module.
Line 26 declares
mod stealth;withoutpub. An intra-doc link to a private item does not resolve in generated documentation, and rustdoc emits aprivate_intra_doc_linkswarning. Either make the module public or drop the link.📝 Option A: make the module public
-mod stealth; +pub mod stealth;📝 Option B: keep the module private and remove the link
-//! - [`stealth`] — DIP-33 stealth one-time key derivation. +//! - `stealth` — DIP-33 stealth one-time key derivation (re-exported at the crate root).🤖 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-platform-encryption/src/lib.rs` at line 13, Resolve the broken intra-doc link to stealth in the crate-level documentation by either making the stealth module public at its declaration or removing the [`stealth`] entry from the documentation list; preserve the intended visibility of the module and ensure rustdoc no longer reports a private_intra_doc_links warning.packages/rs-platform-wallet/src/wallet/identity/crypto/stealth.rs (1)
161-172: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winEach stealth entry point builds its own
Secp256k1context. The root cause is that the module creates the context instead of accepting one.Secp256k1::new()allocates and initializes the full pre-computation table, which costs far more than the one or two curve operations that follow it.recognize_one_time_destinationruns once per candidate notification and per output index during wallet scanning, so this sits on a hot path. Add asecp: &Secp256k1<C>parameter to the three public functions, or use the crate-globalsecp256k1::SECP256K1.
packages/rs-platform-wallet/src/wallet/identity/crypto/stealth.rs#L161-L172: removelet secp = Secp256k1::new();fromderive_one_time_destinationand take the context from the caller.packages/rs-platform-wallet/src/wallet/identity/crypto/stealth.rs#L200-L200: removelet secp = Secp256k1::new();fromrecognize_one_time_destinationand take the context from the caller. This is the scanning path and benefits most.packages/rs-platform-wallet/src/wallet/identity/crypto/stealth.rs#L225-L225: removelet secp = Secp256k1::new();fromderive_one_time_secret_key. Onlystealth_shared_pointneeds the context;one_time_secret_keyperforms scalar addition and needs none.🤖 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-platform-wallet/src/wallet/identity/crypto/stealth.rs` around lines 161 - 172, The stealth entry points independently initialize expensive Secp256k1 contexts; update derive_one_time_destination, recognize_one_time_destination, and derive_one_time_secret_key in packages/rs-platform-wallet/src/wallet/identity/crypto/stealth.rs at lines 161-172, 200-200, and 225-225 to reuse a caller-provided context or the crate-global SECP256K1. Remove each Secp256k1::new() allocation, thread the context through the affected calls, and ensure derive_one_time_secret_key passes it only to stealth_shared_point while leaving one_time_secret_key context-free.packages/rs-platform-encryption/src/stealth.rs (1)
121-155: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider making the modular reduction branch-free, or removing it.
ge_orderreturns on the first differing byte andreduce_mod_ordertakes a data-dependent branch. The input is SHA256 of the shared secret point, so the control flow depends on secret-derived data. The leak is weak because SHA256 is one-way, but a cryptographic primitive crate is the wrong place to accept a variable-time scalar path.Two options:
- Make the subtraction unconditional and select the result with a constant-time mask.
- Drop the reduction.
Scalar::from_be_bytesalready rejects values at or above the order. Treat that rejection like the existing zero case and require a fresh ephemeral key. The probability of either event is about 2^-128.Option 2 removes
reduce_mod_order,ge_order, andCURVE_ORDER_BEentirely. Confirm first that the DIP-33 specification defines the tweak as a reduction and not as a rejection, because the two differ observably on out-of-range digests.🤖 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-platform-encryption/src/stealth.rs` around lines 121 - 155, Verify whether DIP-33 requires reducing the SHA256-derived tweak modulo the curve order or rejecting out-of-range values. If rejection is permitted, remove reduce_mod_order, ge_order, and CURVE_ORDER_BE, and treat Scalar::from_be_bytes failure like the existing zero-tweak case by generating a fresh ephemeral key; otherwise replace the data-dependent comparison and subtraction in those helpers with constant-time unconditional subtraction and masked selection.packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_payment_key_uniqueness/v0/mod.rs (1)
59-69: 🚀 Performance & Scalability | 🔵 TrivialUnbounded key fetch scales with total historical keys, not the current transition.
The
AllKeysfetch has nolimit. Total key count for an identity is unbounded over its lifetime, sincemax_public_keys_in_creationonly caps keys added per transition, not the total accumulated over many updates. Consider monitoring identities with unusually large key counts, or revisit pagination if this becomes a hot path under high key churn.🤖 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_payment_key_uniqueness/v0/mod.rs` around lines 59 - 69, Review the key lookup in the payment-key uniqueness validation flow and avoid relying on an unbounded AllKeys fetch for identities with large historical key counts. Update the logic around IdentityKeysRequest and fetch_identity_keys to use an appropriate bounded or paginated strategy while still checking all keys relevant to the current transition.
🤖 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-dpp/src/errors/consensus/basic/basic_error.rs`:
- Around line 604-613: Move InvalidKeyPurposeKeyTypeError and
TooManyPublicKeysOfPurposeError out of their current position in BasicError and
append both variants after the existing tail variant
TokenPricingScheduleEmptyError, preserving their transparent error annotations
and the order of all existing variants.
In `@packages/rs-platform-encryption/src/stealth.rs`:
- Around line 167-175: Verify whether the expected DIP-33 vector constants and
outputs used by the known-answer test were copied from the published DIP-33
specification. If so, cite the exact DIP-33 section in the comment near B_SCAN,
B_SPEND, and R_EPHEMERAL; otherwise, reword the interop claim to describe the
test as a locally generated regression lock and note a follow-up to add
published vectors.
In `@packages/wasm-dpp2/src/enums/keys/purpose.rs`:
- Around line 83-84: Update the PurposeLike numeric conversion near the
PurposeWasm mappings to validate that the input is finite, integral, and within
the supported purpose range before casting to u8. Reject fractional values and
non-negative overflow values instead of allowing truncation or wrapping, while
preserving valid mappings such as 7 to PAYMENT_SCAN and 8 to PAYMENT_SPEND.
---
Nitpick comments:
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_payment_key_uniqueness/v0/mod.rs`:
- Around line 59-69: Review the key lookup in the payment-key uniqueness
validation flow and avoid relying on an unbounded AllKeys fetch for identities
with large historical key counts. Update the logic around IdentityKeysRequest
and fetch_identity_keys to use an appropriate bounded or paginated strategy
while still checking all keys relevant to the current transition.
In `@packages/rs-platform-encryption/src/lib.rs`:
- Line 13: Resolve the broken intra-doc link to stealth in the crate-level
documentation by either making the stealth module public at its declaration or
removing the [`stealth`] entry from the documentation list; preserve the
intended visibility of the module and ensure rustdoc no longer reports a
private_intra_doc_links warning.
In `@packages/rs-platform-encryption/src/stealth.rs`:
- Around line 121-155: Verify whether DIP-33 requires reducing the
SHA256-derived tweak modulo the curve order or rejecting out-of-range values. If
rejection is permitted, remove reduce_mod_order, ge_order, and CURVE_ORDER_BE,
and treat Scalar::from_be_bytes failure like the existing zero-tweak case by
generating a fresh ephemeral key; otherwise replace the data-dependent
comparison and subtraction in those helpers with constant-time unconditional
subtraction and masked selection.
In `@packages/rs-platform-wallet/src/wallet/identity/crypto/stealth.rs`:
- Around line 161-172: The stealth entry points independently initialize
expensive Secp256k1 contexts; update derive_one_time_destination,
recognize_one_time_destination, and derive_one_time_secret_key in
packages/rs-platform-wallet/src/wallet/identity/crypto/stealth.rs at lines
161-172, 200-200, and 225-225 to reuse a caller-provided context or the
crate-global SECP256K1. Remove each Secp256k1::new() allocation, thread the
context through the affected calls, and ensure derive_one_time_secret_key passes
it only to stealth_shared_point while leaving one_time_secret_key context-free.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c7e5d2fe-3388-47f1-b8fc-6fa3b5aab1c1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (48)
packages/dashpay-contract/schema/v2/dashpay.schema.jsonpackages/dashpay-contract/src/lib.rspackages/dashpay-contract/src/v2/mod.rspackages/rs-dpp/src/errors/consensus/basic/basic_error.rspackages/rs-dpp/src/errors/consensus/basic/identity/invalid_key_purpose_key_type_error.rspackages/rs-dpp/src/errors/consensus/basic/identity/mod.rspackages/rs-dpp/src/errors/consensus/basic/identity/too_many_public_keys_of_purpose_error.rspackages/rs-dpp/src/errors/consensus/codes.rspackages/rs-dpp/src/identity/identity_public_key/purpose.rspackages/rs-dpp/src/state_transition/state_transitions/identity/public_key_in_creation/methods/validate_identity_public_keys_structure/mod.rspackages/rs-dpp/src/state_transition/state_transitions/identity/public_key_in_creation/methods/validate_identity_public_keys_structure/v1/mod.rspackages/rs-drive-abci/src/execution/check_tx/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/common/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/common/validate_payment_key_uniqueness/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/common/validate_payment_key_uniqueness/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/deletion.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/replacement.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/state/v0/mod.rspackages/rs-drive/src/cache/data_contract.rspackages/rs-drive/src/cache/system_contracts.rspackages/rs-drive/src/drive/contract/refresh_cache/mod.rspackages/rs-drive/src/drive/identity/estimation_costs/for_purpose_in_key_reference_tree/v0/mod.rspackages/rs-drive/tests/deterministic_root_hash.rspackages/rs-platform-encryption/Cargo.tomlpackages/rs-platform-encryption/src/error.rspackages/rs-platform-encryption/src/lib.rspackages/rs-platform-encryption/src/stealth.rspackages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_method_versions/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_method_versions/v2.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v1.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v2.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v3.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v4.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v5.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v6.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v7.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v8.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v9.rspackages/rs-platform-version/src/version/system_data_contract_versions/mod.rspackages/rs-platform-version/src/version/system_data_contract_versions/v3.rspackages/rs-platform-version/src/version/v14.rspackages/rs-platform-wallet/src/wallet/identity/crypto/mod.rspackages/rs-platform-wallet/src/wallet/identity/crypto/stealth.rspackages/wasm-dpp/src/errors/consensus/consensus_error.rspackages/wasm-dpp/src/identity/identity_public_key/purpose.rspackages/wasm-dpp2/src/enums/keys/purpose.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The DIP-33 implementation is generally well-structured and version-dispatched, but three blocking issues remain: positional BasicError compatibility is broken, noncanonical 65-byte payment keys are accepted, and Swift rewrites the new key-purpose discriminants during persistence. Additional WASM and Kotlin boundary issues, retryable error flattening, and missing state-validation coverage should also be addressed.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking | 🟡 5 suggestion(s)
2 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-dpp/src/errors/consensus/basic/basic_error.rs`:
- [BLOCKING] packages/rs-dpp/src/errors/consensus/basic/basic_error.rs:607-611: Append the new BasicError variants to preserve wire discriminants
BasicError derives bincode Encode/Decode and PlatformSerialize/PlatformDeserialize, so its enum discriminants are positional. Inserting these variants before StateTransitionNotActiveError shifts every existing variant through TokenPricingScheduleEmptyError, causing previously encoded errors to decode as the wrong variant or fail. This directly violates the ordering invariant documented at the enum tail. Move both new variants after TokenPricingScheduleEmptyError without changing the order of any existing variant, and add a compatibility test that pins a pre-existing variant from after the current insertion point.
In `packages/rs-dpp/src/state_transition/state_transitions/identity/public_key_in_creation/methods/validate_identity_public_keys_structure/v1/mod.rs`:
- [BLOCKING] packages/rs-dpp/src/state_transition/state_transitions/identity/public_key_in_creation/methods/validate_identity_public_keys_structure/v1/mod.rs:148-161: Require compressed 33-byte encodings for payment keys
This check constrains payment keys to ECDSA_SECP256K1 but does not constrain their encoding. DPP's ECDSA public-key parser explicitly accepts both 33-byte compressed and 65-byte uncompressed SEC1 keys, and the generic added-key signature verifier passes either representation to PublicKey::from_slice. A signed transition can therefore register a 65-byte PAYMENT_SCAN or PAYMENT_SPEND key even though DIP-33 specifies compressed SEC1 points and this validator's own comment requires the full compressed key. Besides producing noncanonical persisted state, compressed and uncompressed encodings of the same point also evade the in-transition duplicate check, which compares raw bytes. Reject payment-purpose keys unless data().len() is exactly 33 and add a regression test using a valid 65-byte key.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:1798-1804: Preserve payment-key purposes across the Swift FFI boundary
Rust emits each DPP purpose as its raw u8 value, so the new PAYMENT_SCAN and PAYMENT_SPEND keys reach Swift as 7 and 8. Swift's KeyPurpose enum still ends at 6, and this fallback silently persists either new value as authentication. The cold-start restore path later sends that stored raw value back to Rust, changing the key's meaning, while ManagedIdentity.getPublicKeys() rejects the same values as unknown. Add paymentScan/paymentSpend cases with raw values 7 and 8, update exhaustive switches, and replace this semantic fallback with an unsupported-discriminant error rather than coercing an unknown purpose to authentication.
In `packages/wasm-dpp2/src/enums/keys/purpose.rs`:
- [SUGGESTION] packages/wasm-dpp2/src/enums/keys/purpose.rs:65-66: Accept the payment-purpose strings emitted by the WASM getter
The IdentityPublicKey purpose getter emits PAYMENT_SCAN and PAYMENT_SPEND. The setter lowercases those strings to payment_scan and payment_spend, but this parser accepts only paymentscan and paymentspend. Consequently, assigning key.purpose = key.purpose or copying a key through its exposed properties fails only for the new variants. Accept the underscore forms as aliases so getter values round-trip through constructors and setters.
- [SUGGESTION] packages/wasm-dpp2/src/enums/keys/purpose.rs:74-84: Reject fractional numeric payment-purpose values
PurposeLike's TypeScript union is not a runtime constraint. Casting an arbitrary JavaScript number directly to u8 truncates fractional values, so 7.5 is accepted as PAYMENT_SCAN and 8.5 as PAYMENT_SPEND. Validate that the number is finite, integral, and within the supported 0 through 8 range before casting. Rust's float-to-integer cast saturates rather than wraps, so positive overflow currently becomes 255 and is rejected; the newly introduced defect is specifically the acceptance of fractional values that truncate to 7 or 8.
In `packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_payment_key_uniqueness/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_payment_key_uniqueness/v0/mod.rs:37-84: Cover payment-key uniqueness against persisted identity state
This helper implements the consensus-critical against-state half of the one-active-key invariant, including the state-sensitive rotation exception, but rs-drive-abci contains no PAYMENT_SCAN or PAYMENT_SPEND tests. Add tests proving that an add-only update conflicts with an existing active key, disabling that exact key while adding a replacement succeeds, disabled historical keys do not block replacement, and scan/spend purposes remain independent. A pipeline-level test should also pin rejection before protocol version 14 and acceptance at version 14.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityUpdates.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityUpdates.kt:27-35: Expose the new DPP key purposes in Kotlin
The generic Kotlin identity registration and update codecs serialize KeyPurpose.ffiValue, but this mirror stops at OWNER=6. Kotlin callers therefore cannot construct PAYMENT_SCAN=7 or PAYMENT_SPEND=8 rows even though the Rust decoder now supports them. Add both enum cases and update the example application's exhaustive when expressions and purpose-selection logic so Android callers can use the protocol-version-14 feature.
In `packages/rs-platform-wallet/src/wallet/identity/crypto/stealth.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/crypto/stealth.rs:171-229: Preserve retryable stealth crypto errors
The lower crypto layer exposes ZeroStealthTweak as a recoverable condition that requires the payer to choose a fresh ephemeral key, but every stealth operation converts CryptoError into InvalidIdentityData(String). Callers cannot distinguish the retry condition without parsing display text. Add a typed PlatformWalletError source variant for platform_encryption::CryptoError, or a dedicated zero-tweak variant, and propagate it with ?. Errors constructing or deriving the DIP-9 path should likewise use the existing KeyDerivation category rather than InvalidIdentityData.
| #[error(transparent)] | ||
| InvalidKeyPurposeKeyTypeError(InvalidKeyPurposeKeyTypeError), | ||
|
|
||
| #[error(transparent)] | ||
| TooManyPublicKeysOfPurposeError(TooManyPublicKeysOfPurposeError), |
There was a problem hiding this comment.
🔴 Blocking: Append the new BasicError variants to preserve wire discriminants
BasicError derives bincode Encode/Decode and PlatformSerialize/PlatformDeserialize, so its enum discriminants are positional. Inserting these variants before StateTransitionNotActiveError shifts every existing variant through TokenPricingScheduleEmptyError, causing previously encoded errors to decode as the wrong variant or fail. This directly violates the ordering invariant documented at the enum tail. Move both new variants after TokenPricingScheduleEmptyError without changing the order of any existing variant, and add a compatibility test that pins a pre-existing variant from after the current insertion point.
source: ['codex']
There was a problem hiding this comment.
Resolved in b2b51fb — Append the new BasicError variants to preserve wire discriminants no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| if let Some(invalid_type_key) = identity_public_keys_with_witness.iter().find(|key| { | ||
| Purpose::payment_purposes().contains(&key.purpose()) | ||
| && key.key_type() != KeyType::ECDSA_SECP256K1 | ||
| }) { | ||
| return Ok(SimpleConsensusValidationResult::new_with_error( | ||
| BasicError::InvalidKeyPurposeKeyTypeError(InvalidKeyPurposeKeyTypeError::new( | ||
| invalid_type_key.id(), | ||
| invalid_type_key.purpose(), | ||
| invalid_type_key.key_type(), | ||
| vec![KeyType::ECDSA_SECP256K1], | ||
| )) | ||
| .into(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Require compressed 33-byte encodings for payment keys
This check constrains payment keys to ECDSA_SECP256K1 but does not constrain their encoding. DPP's ECDSA public-key parser explicitly accepts both 33-byte compressed and 65-byte uncompressed SEC1 keys, and the generic added-key signature verifier passes either representation to PublicKey::from_slice. A signed transition can therefore register a 65-byte PAYMENT_SCAN or PAYMENT_SPEND key even though DIP-33 specifies compressed SEC1 points and this validator's own comment requires the full compressed key. Besides producing noncanonical persisted state, compressed and uncompressed encodings of the same point also evade the in-transition duplicate check, which compares raw bytes. Reject payment-purpose keys unless data().len() is exactly 33 and add a regression test using a valid 65-byte key.
source: ['codex']
There was a problem hiding this comment.
Resolved in b2b51fb — Require compressed 33-byte encodings for payment keys no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| "paymentscan" => Ok(PurposeWasm::PAYMENT_SCAN), | ||
| "paymentspend" => Ok(PurposeWasm::PAYMENT_SPEND), |
There was a problem hiding this comment.
🟡 Suggestion: Accept the payment-purpose strings emitted by the WASM getter
The IdentityPublicKey purpose getter emits PAYMENT_SCAN and PAYMENT_SPEND. The setter lowercases those strings to payment_scan and payment_spend, but this parser accepts only paymentscan and paymentspend. Consequently, assigning key.purpose = key.purpose or copying a key through its exposed properties fails only for the new variants. Accept the underscore forms as aliases so getter values round-trip through constructors and setters.
| "paymentscan" => Ok(PurposeWasm::PAYMENT_SCAN), | |
| "paymentspend" => Ok(PurposeWasm::PAYMENT_SPEND), | |
| "paymentscan" | "payment_scan" => Ok(PurposeWasm::PAYMENT_SCAN), | |
| "paymentspend" | "payment_spend" => Ok(PurposeWasm::PAYMENT_SPEND), |
source: ['codex']
There was a problem hiding this comment.
Resolved in b2b51fb — Accept the payment-purpose strings emitted by the WASM getter no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| pub(super) fn validate_payment_key_uniqueness_in_state_v0( | ||
| identity_id: Identifier, | ||
| public_keys_being_added: &[IdentityPublicKeyInCreation], | ||
| public_key_ids_to_disable: &[KeyID], | ||
| drive: &Drive, | ||
| _execution_context: &mut StateTransitionExecutionContext, | ||
| transaction: TransactionArg, | ||
| platform_version: &PlatformVersion, | ||
| ) -> Result<SimpleConsensusValidationResult, Error> { | ||
| let payment_purposes_being_added: Vec<Purpose> = Purpose::payment_purposes() | ||
| .into_iter() | ||
| .filter(|purpose| { | ||
| public_keys_being_added | ||
| .iter() | ||
| .any(|key| key.purpose() == *purpose) | ||
| }) | ||
| .collect(); | ||
|
|
||
| if payment_purposes_being_added.is_empty() { | ||
| return Ok(SimpleConsensusValidationResult::new()); | ||
| } | ||
|
|
||
| let identity_key_request = IdentityKeysRequest { | ||
| identity_id: identity_id.to_buffer(), | ||
| request_type: KeyRequestType::AllKeys, | ||
| limit: None, | ||
| offset: None, | ||
| }; | ||
| let existing_keys = drive.fetch_identity_keys::<KeyIDIdentityPublicKeyPairBTreeMap>( | ||
| identity_key_request, | ||
| transaction, | ||
| platform_version, | ||
| )?; | ||
|
|
||
| for purpose in payment_purposes_being_added { | ||
| let conflicting_active_key_exists = existing_keys.values().any(|key| { | ||
| key.purpose() == purpose | ||
| && key.disabled_at().is_none() | ||
| && !public_key_ids_to_disable.contains(&key.id()) | ||
| }); | ||
| if conflicting_active_key_exists { | ||
| return Ok(SimpleConsensusValidationResult::new_with_error( | ||
| TooManyPublicKeysOfPurposeError::new(purpose, 1).into(), | ||
| )); | ||
| } | ||
| } | ||
|
|
||
| Ok(SimpleConsensusValidationResult::new()) |
There was a problem hiding this comment.
🟡 Suggestion: Cover payment-key uniqueness against persisted identity state
This helper implements the consensus-critical against-state half of the one-active-key invariant, including the state-sensitive rotation exception, but rs-drive-abci contains no PAYMENT_SCAN or PAYMENT_SPEND tests. Add tests proving that an add-only update conflicts with an existing active key, disabling that exact key while adding a replacement succeeds, disabled historical keys do not block replacement, and scan/spend purposes remain independent. A pipeline-level test should also pin rejection before protocol version 14 and acceptance at version 14.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Cover payment-key uniqueness against persisted identity state no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| 4 => Ok(PurposeWasm::SYSTEM), | ||
| 5 => Ok(PurposeWasm::VOTING), | ||
| 6 => Ok(PurposeWasm::OWNER), | ||
| 7 => Ok(PurposeWasm::PAYMENT_SCAN), | ||
| 8 => Ok(PurposeWasm::PAYMENT_SPEND), |
There was a problem hiding this comment.
🟡 Suggestion: Reject fractional numeric payment-purpose values
PurposeLike's TypeScript union is not a runtime constraint. Casting an arbitrary JavaScript number directly to u8 truncates fractional values, so 7.5 is accepted as PAYMENT_SCAN and 8.5 as PAYMENT_SPEND. Validate that the number is finite, integral, and within the supported 0 through 8 range before casting. Rust's float-to-integer cast saturates rather than wraps, so positive overflow currently becomes 255 and is rejected; the newly introduced defect is specifically the acceptance of fractional values that truncate to 7 or 8.
source: ['coderabbit']
There was a problem hiding this comment.
Resolved in b2b51fb — Reject fractional numeric payment-purpose values no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| let shared = stealth_shared_point(&secp, ephemeral_secret, scan_public) | ||
| .map_err(|e| PlatformWalletError::InvalidIdentityData(e.to_string()))?; | ||
| let one_time_public = one_time_public_key( | ||
| &secp, | ||
| spend_public, | ||
| &shared, | ||
| &ephemeral_public, | ||
| rail.into(), | ||
| n, | ||
| ) | ||
| .map_err(|e| PlatformWalletError::InvalidIdentityData(e.to_string()))?; | ||
| Ok(( | ||
| destination_from_public_key(&one_time_public, rail, network), | ||
| ephemeral_public, | ||
| )) | ||
| } | ||
|
|
||
| /// Receiver / watch service: re-derive the one-time destination for output `n` | ||
| /// given the payer's published `R`, the recipient's scan secret, and the | ||
| /// recipient's spend public key. Compare the result against the referenced | ||
| /// on-chain output. Requires no spend secret. | ||
| pub fn recognize_one_time_destination( | ||
| scan_secret: &SecretKey, | ||
| spend_public: &PublicKey, | ||
| ephemeral_public: &PublicKey, | ||
| rail: PaymentRail, | ||
| n: u32, | ||
| network: Network, | ||
| ) -> Result<OneTimeDestination, PlatformWalletError> { | ||
| let secp = Secp256k1::new(); | ||
| let shared = stealth_shared_point(&secp, scan_secret, ephemeral_public) | ||
| .map_err(|e| PlatformWalletError::InvalidIdentityData(e.to_string()))?; | ||
| let one_time_public = one_time_public_key( | ||
| &secp, | ||
| spend_public, | ||
| &shared, | ||
| ephemeral_public, | ||
| rail.into(), | ||
| n, | ||
| ) | ||
| .map_err(|e| PlatformWalletError::InvalidIdentityData(e.to_string()))?; | ||
| Ok(destination_from_public_key(&one_time_public, rail, network)) | ||
| } | ||
|
|
||
| /// Spender: derive the one-time secret key controlling output `n`, given the | ||
| /// payer's published `R`, the recipient's scan secret, and the recipient's | ||
| /// spend secret. | ||
| pub fn derive_one_time_secret_key( | ||
| scan_secret: &SecretKey, | ||
| spend_secret: &SecretKey, | ||
| ephemeral_public: &PublicKey, | ||
| rail: PaymentRail, | ||
| n: u32, | ||
| ) -> Result<SecretKey, PlatformWalletError> { | ||
| let secp = Secp256k1::new(); | ||
| let shared = stealth_shared_point(&secp, scan_secret, ephemeral_public) | ||
| .map_err(|e| PlatformWalletError::InvalidIdentityData(e.to_string()))?; | ||
| one_time_secret_key(spend_secret, &shared, ephemeral_public, rail.into(), n) | ||
| .map_err(|e| PlatformWalletError::InvalidIdentityData(e.to_string())) |
There was a problem hiding this comment.
🟡 Suggestion: Preserve retryable stealth crypto errors
The lower crypto layer exposes ZeroStealthTweak as a recoverable condition that requires the payer to choose a fresh ephemeral key, but every stealth operation converts CryptoError into InvalidIdentityData(String). Callers cannot distinguish the retry condition without parsing display text. Add a typed PlatformWalletError source variant for platform_encryption::CryptoError, or a dedicated zero-tweak variant, and propagate it with ?. Errors constructing or deriving the DIP-9 path should likewise use the existing KeyDerivation category rather than InvalidIdentityData.
source: ['codex']
There was a problem hiding this comment.
Resolved in b2b51fb — Preserve retryable stealth crypto errors no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
…nt keys, typed purposes across bindings - move InvalidKeyPurposeKeyTypeError/TooManyPublicKeysOfPurposeError to the BasicError tail (positional bincode discriminants) and pin discriminants in tests - require payment keys to be exactly 33-byte compressed SEC1 points (a 65-byte uncompressed encoding of the same point also evaded the raw-bytes duplicate check) - Swift: add paymentScan/paymentSpend to KeyPurpose; skip-and-warn on unknown discriminants instead of coercing to authentication - Kotlin: add PAYMENT_SCAN/PAYMENT_SPEND to the KeyPurpose mirror - wasm-dpp2: accept payment_scan/payment_spend string aliases; reject non-integral numeric purpose values - platform-wallet: preserve retryable stealth crypto errors via a typed StealthCrypto variant; derivation errors use KeyDerivation - drive-abci: state tests for payment-key uniqueness (conflict, rotation, disabled-key replacement, purpose independence, PV13 rejection) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Same serialization as platformPaymentAddress: variant byte (0x00 P2PKH / 0x01 P2SH) + 20-byte HASH160. The Base58Check string bought nothing at consensus (checksum unprovable there), while the byte form is exact-size validated, smaller, symmetric with the platform field, and carries no network byte — the network follows the Platform chain, so a mismatch is unrepresentable. Matches the updated DIP-33 text. Rebaselines: PV14 fees (check_tx, replace, delete) and deterministic root hash. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the review in b2b51fb + 07c5630. Blockers
Suggestions
Also in this push (07c5630): Verified locally: dpp 3893 tests, drive-abci full suite, drive, platform-wallet/encryption, dashpay-contract all green; workspace clippy (all-features) clean; Kotlin |
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/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- Around line 1798-1807: In the identity-key persistence flow around the
existing fetch/update logic, validate purpose, securityLevel, and keyType before
fetching or branching on an existing row. For supported discriminants, update
existing rows with the validated enum raw values as well as inserting new rows;
skip unsupported entries without coercion. Add coverage for payment purposes 7
and 8, including updates to already-persisted keys.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7abb592e-7465-4dfa-b1df-26f2869ec658
📒 Files selected for processing (16)
packages/dashpay-contract/schema/v2/dashpay.schema.jsonpackages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/identity/AddIdentityKeyScreen.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityUpdates.ktpackages/rs-dpp/src/address_funds/witness_verification_operations.rspackages/rs-dpp/src/errors/consensus/basic/basic_error.rspackages/rs-dpp/src/state_transition/state_transitions/identity/public_key_in_creation/methods/validate_identity_public_keys_structure/v1/mod.rspackages/rs-drive-abci/src/execution/check_tx/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/deletion.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/replacement.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/mod.rspackages/rs-drive/tests/deterministic_root_hash.rspackages/rs-platform-wallet/src/error.rspackages/rs-platform-wallet/src/wallet/identity/crypto/stealth.rspackages/swift-sdk/Sources/SwiftDashSDK/DPP/DPPIdentity.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swiftpackages/wasm-dpp2/src/enums/keys/purpose.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/rs-drive-abci/src/execution/check_tx/v0/mod.rs
- packages/dashpay-contract/schema/v2/dashpay.schema.json
- packages/rs-drive/tests/deterministic_root_hash.rs
- packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/replacement.rs
- packages/wasm-dpp2/src/enums/keys/purpose.rs
- packages/rs-dpp/src/state_transition/state_transitions/identity/public_key_in_creation/methods/validate_identity_public_keys_structure/v1/mod.rs
- packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/deletion.rs
DashPay contract v2 (protocol version 14): the profile document gains two optional public payment fields — corePaymentAddress (position 5) and platformPaymentAddress (position 6), both the 21-byte storage form (variant byte 0x00 P2PKH / 0x01 P2SH + 20-byte HASH160). Payments to them are intentionally public and linkable to the profile (per DIP-33, the public tier); no derivation-source publishing. Follows the DPNS-v2 versioning pattern: schema/v2 + system_data_contract_versions v3 (dashpay: 2) at PV14, with the contract reload on the first block after the protocol change. Rebaselines PV14 fees (check_tx, replace, delete), profile fixture strings, and the deterministic root hash. Extracted from the larger payment-addresses branch; payment detection key purposes and stealth derivation follow in a separate PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
changed the title
|
PR split: the public tips tier (dashpay contract v2 + |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The payment-key model and stealth derivation are generally well structured, but three consensus blockers remain: creation paths accept contract-bounded payment keys that later fail internally, the uniqueness scan discards its query costs, and the new state rule modifies the existing identity-update v0 implementation. The Swift legacy C bridge also cannot decode the new purposes, and several public-API and activation-test improvements remain advisable.
Source: reviewer backends gpt-5.6-sol (general, security-auditor, rust-quality, ffi-engineer); final verifier backend gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking | 🟡 4 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-dpp/src/state_transition/state_transitions/identity/public_key_in_creation/methods/validate_identity_public_keys_structure/v1/mod.rs`:
- [BLOCKING] packages/rs-dpp/src/state_transition/state_transitions/identity/public_key_in_creation/methods/validate_identity_public_keys_structure/v1/mod.rs:150-182: Reject contract bounds on payment keys during structure validation
The payment-key checks enforce key type and compressed encoding but do not reject `contract_bounds`. Identity updates eventually call `validate_identity_public_keys_contract_bounds`, but the identity-create, identity-create-from-addresses, and identity-create-from-shielded-pool state paths do not run that validation. A creation transition can therefore pass consensus validation with a `PAYMENT_SCAN` or `PAYMENT_SPEND` key whose bounds are present. During identity insertion, `add_potential_contract_info_for_contract_bounded_key_v0` rejects every purpose other than `ENCRYPTION` or `DECRYPTION` with the internal `IdentityKeyBoundsError("purpose not available for key bounds")`. Reject payment-purpose keys with bounds in this shared structure validator using `InvalidKeyPurposeForContractBoundsError`, and add creation-path regression coverage so user-controlled input produces a deterministic consensus error instead of an execution failure.
In `packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_payment_key_uniqueness/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_payment_key_uniqueness/v0/mod.rs:37-69: Meter the all-keys lookup used by payment-key uniqueness validation
Every update adding a payment key performs an `AllKeys` query over the identity's complete key history. `fetch_identity_keys` builds GroveDB cost operations and then discards them, while this helper deliberately ignores `_execution_context`, so the potentially large read and deserialization work is never charged. Repeated payment-key rotations can therefore force validators to rescan a large identity cheaply. Use `fetch_identity_keys_with_costs`, pass the committed epoch, and add the resulting fee to the actual processor-owned context as `ValidationOperation::PrecalculatedOperation`. The identity-update implementation currently creates a separate local context while ignoring the outer context passed by the processor, so that context must also be threaded through; otherwise the operation will still be absent from the resulting `ExecutionEvent`.
In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/state/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/state/v0/mod.rs:124-139: Introduce a new identity-update state-validation version
This inserts the payment-key uniqueness rule directly into `validate_state_v0`, while every Drive-ABCI validation table still selects identity-update state version 0. Consensus-critical version implementations must remain behaviorally immutable. Keep v0 unchanged, add a `state/v1` implementation containing the new rule and the correctly threaded execution context, extend the dispatcher to versions 0 and 1, and select version 1 only for protocol version 14. Tests should explicitly show that version 13 continues through v0 and version 14 uses v1.
In `packages/rs-sdk-ffi/src/identity/create_from_components.rs`:
- [SUGGESTION] packages/rs-sdk-ffi/src/identity/create_from_components.rs:111-125: Synchronize the legacy Rust C API with the new Swift purpose values
Swift now exposes `paymentScan = 7` and `paymentSpend = 8`, and `identityToHandle` forwards those raw values through `DashSDKPublicKeyData.purpose`. This decoder only accepts purposes 0 through 6, so converting a `DPPIdentity` containing either new key into a native identity handle fails with `Invalid key purpose`; document and token operations that call `identityToHandle` consequently cannot use such identities. The standalone decoder in `packages/rs-sdk-ffi/src/identity/keys.rs:222-235` is also stale. Use the existing `Purpose::try_from` conversion, or explicitly accept 7 and 8 in both C entry points, so the Swift mirror and Rust boundary share the same discriminants.
In `packages/rs-platform-wallet/src/wallet/identity/crypto/stealth.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/crypto/stealth.rs:51-56: Represent the closed payment key classes with an enum
DIP-33 defines the scan, spend, and notification-out key classes, but the public derivation functions accept any `u32`. Values outside 0 through 2 still produce valid hardened BIP32 paths, so callers can derive nonstandard keys without receiving an error. Represent the role as a `PaymentKeyClass` enum and convert it to the hardened index internally. This makes invalid roles unrepresentable and gives callers an exhaustive API if another role is introduced later.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/crypto/stealth.rs:157-225: Reuse the secp256k1 context during destination scanning
Each payer, receiver, and spender entry point constructs a fresh `Secp256k1` context. `recognize_one_time_destination` is intended to run repeatedly while scanning candidate notifications and output counters, making repeated context initialization unnecessary work on the hottest path. Accept a caller-owned context, provide a scanner object that owns one, or use the crate's shared context, then pass it through to the lower functions that already accept a borrowed context.
In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/mod.rs:2010-2229: Pin the activation test to protocol version 14
The acceptance test uses `PlatformVersion::latest()`, so it will silently move to later protocol versions and stop protecting the version-14 table. Select `PlatformVersion::get(14)` explicitly. The version-13 test should also assert the expected paid `InvalidIdentityPublicKeySecurityLevelError` rather than merely checking that execution was unsuccessful; the current assertion passes for unrelated signature, nonce, serialization, balance, or internal failures and therefore does not prove the activation boundary.
| if let Some(invalid_type_key) = identity_public_keys_with_witness.iter().find(|key| { | ||
| Purpose::payment_purposes().contains(&key.purpose()) | ||
| && key.key_type() != KeyType::ECDSA_SECP256K1 | ||
| }) { | ||
| return Ok(SimpleConsensusValidationResult::new_with_error( | ||
| BasicError::InvalidKeyPurposeKeyTypeError(InvalidKeyPurposeKeyTypeError::new( | ||
| invalid_type_key.id(), | ||
| invalid_type_key.purpose(), | ||
| invalid_type_key.key_type(), | ||
| vec![KeyType::ECDSA_SECP256K1], | ||
| )) | ||
| .into(), | ||
| )); | ||
| } | ||
|
|
||
| // DIP-33 publishes payment keys as compressed SEC1 points, but DPP's ECDSA | ||
| // parser also accepts 65-byte uncompressed keys — and the two encodings of | ||
| // the same point would evade the raw-bytes duplicate check above | ||
| if let Some(invalid_size_key) = identity_public_keys_with_witness.iter().find(|key| { | ||
| Purpose::payment_purposes().contains(&key.purpose()) && key.data().len() != 33 | ||
| }) { | ||
| return Ok(SimpleConsensusValidationResult::new_with_error( | ||
| BasicError::InvalidIdentityPublicKeyDataError( | ||
| InvalidIdentityPublicKeyDataError::new( | ||
| invalid_size_key.id(), | ||
| PublicKeyValidationError::new( | ||
| "payment keys must be 33-byte compressed SEC1 points", | ||
| ), | ||
| ), | ||
| ) | ||
| .into(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Reject contract bounds on payment keys during structure validation
The payment-key checks enforce key type and compressed encoding but do not reject contract_bounds. Identity updates eventually call validate_identity_public_keys_contract_bounds, but the identity-create, identity-create-from-addresses, and identity-create-from-shielded-pool state paths do not run that validation. A creation transition can therefore pass consensus validation with a PAYMENT_SCAN or PAYMENT_SPEND key whose bounds are present. During identity insertion, add_potential_contract_info_for_contract_bounded_key_v0 rejects every purpose other than ENCRYPTION or DECRYPTION with the internal IdentityKeyBoundsError("purpose not available for key bounds"). Reject payment-purpose keys with bounds in this shared structure validator using InvalidKeyPurposeForContractBoundsError, and add creation-path regression coverage so user-controlled input produces a deterministic consensus error instead of an execution failure.
source: ['codex']
| pub(super) fn validate_payment_key_uniqueness_in_state_v0( | ||
| identity_id: Identifier, | ||
| public_keys_being_added: &[IdentityPublicKeyInCreation], | ||
| public_key_ids_to_disable: &[KeyID], | ||
| drive: &Drive, | ||
| _execution_context: &mut StateTransitionExecutionContext, | ||
| transaction: TransactionArg, | ||
| platform_version: &PlatformVersion, | ||
| ) -> Result<SimpleConsensusValidationResult, Error> { | ||
| let payment_purposes_being_added: Vec<Purpose> = Purpose::payment_purposes() | ||
| .into_iter() | ||
| .filter(|purpose| { | ||
| public_keys_being_added | ||
| .iter() | ||
| .any(|key| key.purpose() == *purpose) | ||
| }) | ||
| .collect(); | ||
|
|
||
| if payment_purposes_being_added.is_empty() { | ||
| return Ok(SimpleConsensusValidationResult::new()); | ||
| } | ||
|
|
||
| let identity_key_request = IdentityKeysRequest { | ||
| identity_id: identity_id.to_buffer(), | ||
| request_type: KeyRequestType::AllKeys, | ||
| limit: None, | ||
| offset: None, | ||
| }; | ||
| let existing_keys = drive.fetch_identity_keys::<KeyIDIdentityPublicKeyPairBTreeMap>( | ||
| identity_key_request, | ||
| transaction, | ||
| platform_version, | ||
| )?; |
There was a problem hiding this comment.
🔴 Blocking: Meter the all-keys lookup used by payment-key uniqueness validation
Every update adding a payment key performs an AllKeys query over the identity's complete key history. fetch_identity_keys builds GroveDB cost operations and then discards them, while this helper deliberately ignores _execution_context, so the potentially large read and deserialization work is never charged. Repeated payment-key rotations can therefore force validators to rescan a large identity cheaply. Use fetch_identity_keys_with_costs, pass the committed epoch, and add the resulting fee to the actual processor-owned context as ValidationOperation::PrecalculatedOperation. The identity-update implementation currently creates a separate local context while ignoring the outer context passed by the processor, so that context must also be threaded through; otherwise the operation will still be absent from the resulting ExecutionEvent.
source: ['codex']
| // DIP-33 payment keys: the identity may end up with at most one active | ||
| // key of each payment purpose. This must run even when no keys are | ||
| // being disabled (a pure add-only update), unlike the master key | ||
| // uniqueness check below. | ||
| validation_result.add_errors( | ||
| validate_payment_key_uniqueness_in_state( | ||
| self.identity_id(), | ||
| self.public_keys_to_add(), | ||
| self.public_key_ids_to_disable(), | ||
| drive, | ||
| &mut state_transition_execution_context, | ||
| tx, | ||
| platform_version, | ||
| )? | ||
| .errors, | ||
| ); |
There was a problem hiding this comment.
🔴 Blocking: Introduce a new identity-update state-validation version
This inserts the payment-key uniqueness rule directly into validate_state_v0, while every Drive-ABCI validation table still selects identity-update state version 0. Consensus-critical version implementations must remain behaviorally immutable. Keep v0 unchanged, add a state/v1 implementation containing the new rule and the correctly threaded execution context, extend the dispatcher to versions 0 and 1, and select version 1 only for protocol version 14. Tests should explicitly show that version 13 continues through v0 and version 14 uses v1.
source: ['codex']
| /// `key_class'` for the scan key. | ||
| pub const KEY_CLASS_SCAN: u32 = 0; | ||
| /// `key_class'` for the spend key. | ||
| pub const KEY_CLASS_SPEND: u32 = 1; | ||
| /// `key_class'` for the notification out-key. | ||
| pub const KEY_CLASS_NOTIF_OUT: u32 = 2; |
There was a problem hiding this comment.
🟡 Suggestion: Represent the closed payment key classes with an enum
DIP-33 defines the scan, spend, and notification-out key classes, but the public derivation functions accept any u32. Values outside 0 through 2 still produce valid hardened BIP32 paths, so callers can derive nonstandard keys without receiving an error. Represent the role as a PaymentKeyClass enum and convert it to the hardened index internally. This makes invalid roles unrepresentable and gives callers an exhaustive API if another role is introduced later.
source: ['codex']
| pub fn derive_one_time_destination( | ||
| ephemeral_secret: &SecretKey, | ||
| scan_public: &PublicKey, | ||
| spend_public: &PublicKey, | ||
| rail: PaymentRail, | ||
| n: u32, | ||
| network: Network, | ||
| ) -> Result<(OneTimeDestination, PublicKey), PlatformWalletError> { | ||
| let secp = Secp256k1::new(); | ||
| let ephemeral_public = PublicKey::from_secret_key(&secp, ephemeral_secret); | ||
| let shared = stealth_shared_point(&secp, ephemeral_secret, scan_public)?; | ||
| let one_time_public = one_time_public_key( | ||
| &secp, | ||
| spend_public, | ||
| &shared, | ||
| &ephemeral_public, | ||
| rail.into(), | ||
| n, | ||
| )?; | ||
| Ok(( | ||
| destination_from_public_key(&one_time_public, rail, network), | ||
| ephemeral_public, | ||
| )) | ||
| } | ||
|
|
||
| /// Receiver / watch service: re-derive the one-time destination for output `n` | ||
| /// given the payer's published `R`, the recipient's scan secret, and the | ||
| /// recipient's spend public key. Compare the result against the referenced | ||
| /// on-chain output. Requires no spend secret. | ||
| pub fn recognize_one_time_destination( | ||
| scan_secret: &SecretKey, | ||
| spend_public: &PublicKey, | ||
| ephemeral_public: &PublicKey, | ||
| rail: PaymentRail, | ||
| n: u32, | ||
| network: Network, | ||
| ) -> Result<OneTimeDestination, PlatformWalletError> { | ||
| let secp = Secp256k1::new(); | ||
| let shared = stealth_shared_point(&secp, scan_secret, ephemeral_public)?; | ||
| let one_time_public = one_time_public_key( | ||
| &secp, | ||
| spend_public, | ||
| &shared, | ||
| ephemeral_public, | ||
| rail.into(), | ||
| n, | ||
| )?; | ||
| Ok(destination_from_public_key(&one_time_public, rail, network)) | ||
| } | ||
|
|
||
| /// Spender: derive the one-time secret key controlling output `n`, given the | ||
| /// payer's published `R`, the recipient's scan secret, and the recipient's | ||
| /// spend secret. | ||
| pub fn derive_one_time_secret_key( | ||
| scan_secret: &SecretKey, | ||
| spend_secret: &SecretKey, | ||
| ephemeral_public: &PublicKey, | ||
| rail: PaymentRail, | ||
| n: u32, | ||
| ) -> Result<SecretKey, PlatformWalletError> { | ||
| let secp = Secp256k1::new(); | ||
| let shared = stealth_shared_point(&secp, scan_secret, ephemeral_public)?; | ||
| Ok(one_time_secret_key( | ||
| spend_secret, | ||
| &shared, | ||
| ephemeral_public, | ||
| rail.into(), | ||
| n, | ||
| )?) |
There was a problem hiding this comment.
🟡 Suggestion: Reuse the secp256k1 context during destination scanning
Each payer, receiver, and spender entry point constructs a fresh Secp256k1 context. recognize_one_time_destination is intended to run repeatedly while scanning candidate notifications and output counters, making repeated context initialization unnecessary work on the hottest path. Accept a caller-owned context, provide a scanner object that owns one, or use the crate's shared context, then pass it through to the lower functions that already accept a borrowed context.
source: ['codex']
| let platform_version = PlatformVersion::latest(); | ||
| let mut platform = TestPlatformBuilder::new() | ||
| .build_with_mock_rpc() | ||
| .set_genesis_state(); | ||
| let (identity, signer, _, master_key) = | ||
| setup_identity_return_master_key(&mut platform, 958, dash_to_credits!(0.1)); | ||
|
|
||
| let results = process_update( | ||
| &platform, | ||
| identity.id(), | ||
| &signer, | ||
| &master_key, | ||
| 1, | ||
| 1, | ||
| &[(10, Purpose::PAYMENT_SCAN, 4001)], | ||
| vec![], | ||
| platform_version, | ||
| ) | ||
| .await; | ||
| assert_matches!( | ||
| results.as_slice(), | ||
| [StateTransitionExecutionResult::SuccessfulExecution { .. }] | ||
| ); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_adding_payment_key_conflicts_with_existing_active_key() { | ||
| let platform_version = PlatformVersion::latest(); | ||
| let mut platform = TestPlatformBuilder::new() | ||
| .build_with_mock_rpc() | ||
| .set_genesis_state(); | ||
| let (identity, signer, _, master_key) = | ||
| setup_identity_return_master_key(&mut platform, 958, dash_to_credits!(0.1)); | ||
| install_payment_key( | ||
| &platform, | ||
| identity.id(), | ||
| 10, | ||
| Purpose::PAYMENT_SCAN, | ||
| 4002, | ||
| platform_version, | ||
| ); | ||
|
|
||
| let results = process_update( | ||
| &platform, | ||
| identity.id(), | ||
| &signer, | ||
| &master_key, | ||
| 1, | ||
| 1, | ||
| &[(11, Purpose::PAYMENT_SCAN, 4003)], | ||
| vec![], | ||
| platform_version, | ||
| ) | ||
| .await; | ||
| assert_matches!( | ||
| results.as_slice(), | ||
| [StateTransitionExecutionResult::PaidConsensusError { | ||
| error: ConsensusError::BasicError(BasicError::TooManyPublicKeysOfPurposeError( | ||
| _ | ||
| )), | ||
| .. | ||
| }] | ||
| ); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_rotating_payment_key_by_disabling_in_same_transition_is_valid() { | ||
| let platform_version = PlatformVersion::latest(); | ||
| let mut platform = TestPlatformBuilder::new() | ||
| .build_with_mock_rpc() | ||
| .set_genesis_state(); | ||
| let (identity, signer, _, master_key) = | ||
| setup_identity_return_master_key(&mut platform, 958, dash_to_credits!(0.1)); | ||
| let old_key = install_payment_key( | ||
| &platform, | ||
| identity.id(), | ||
| 10, | ||
| Purpose::PAYMENT_SCAN, | ||
| 4004, | ||
| platform_version, | ||
| ); | ||
|
|
||
| let results = process_update( | ||
| &platform, | ||
| identity.id(), | ||
| &signer, | ||
| &master_key, | ||
| 1, | ||
| 1, | ||
| &[(11, Purpose::PAYMENT_SCAN, 4005)], | ||
| vec![old_key.id()], | ||
| platform_version, | ||
| ) | ||
| .await; | ||
| assert_matches!( | ||
| results.as_slice(), | ||
| [StateTransitionExecutionResult::SuccessfulExecution { .. }] | ||
| ); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_previously_disabled_payment_key_does_not_block_replacement() { | ||
| let platform_version = PlatformVersion::latest(); | ||
| let mut platform = TestPlatformBuilder::new() | ||
| .build_with_mock_rpc() | ||
| .set_genesis_state(); | ||
| let (identity, signer, _, master_key) = | ||
| setup_identity_return_master_key(&mut platform, 958, dash_to_credits!(0.1)); | ||
| let old_key = install_payment_key( | ||
| &platform, | ||
| identity.id(), | ||
| 10, | ||
| Purpose::PAYMENT_SCAN, | ||
| 4006, | ||
| platform_version, | ||
| ); | ||
|
|
||
| // first update: disable the old key only | ||
| let results = process_update( | ||
| &platform, | ||
| identity.id(), | ||
| &signer, | ||
| &master_key, | ||
| 1, | ||
| 1, | ||
| &[], | ||
| vec![old_key.id()], | ||
| platform_version, | ||
| ) | ||
| .await; | ||
| assert_matches!( | ||
| results.as_slice(), | ||
| [StateTransitionExecutionResult::SuccessfulExecution { .. }] | ||
| ); | ||
|
|
||
| // second update: the disabled historical key must not block adding | ||
| // a replacement | ||
| let results = process_update( | ||
| &platform, | ||
| identity.id(), | ||
| &signer, | ||
| &master_key, | ||
| 2, | ||
| 2, | ||
| &[(11, Purpose::PAYMENT_SCAN, 4007)], | ||
| vec![], | ||
| platform_version, | ||
| ) | ||
| .await; | ||
| assert_matches!( | ||
| results.as_slice(), | ||
| [StateTransitionExecutionResult::SuccessfulExecution { .. }] | ||
| ); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_scan_and_spend_purposes_are_independent() { | ||
| let platform_version = PlatformVersion::latest(); | ||
| let mut platform = TestPlatformBuilder::new() | ||
| .build_with_mock_rpc() | ||
| .set_genesis_state(); | ||
| let (identity, signer, _, master_key) = | ||
| setup_identity_return_master_key(&mut platform, 958, dash_to_credits!(0.1)); | ||
| install_payment_key( | ||
| &platform, | ||
| identity.id(), | ||
| 10, | ||
| Purpose::PAYMENT_SCAN, | ||
| 4008, | ||
| platform_version, | ||
| ); | ||
|
|
||
| let results = process_update( | ||
| &platform, | ||
| identity.id(), | ||
| &signer, | ||
| &master_key, | ||
| 1, | ||
| 1, | ||
| &[(11, Purpose::PAYMENT_SPEND, 4009)], | ||
| vec![], | ||
| platform_version, | ||
| ) | ||
| .await; | ||
| assert_matches!( | ||
| results.as_slice(), | ||
| [StateTransitionExecutionResult::SuccessfulExecution { .. }] | ||
| ); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_payment_keys_rejected_before_protocol_version_14() { | ||
| let platform_version = PlatformVersion::get(13).expect("expected version 13"); | ||
| let mut platform = TestPlatformBuilder::new() | ||
| .with_initial_protocol_version(13) | ||
| .build_with_mock_rpc() | ||
| .set_genesis_state(); | ||
| let (identity, signer, _, master_key) = | ||
| setup_identity_return_master_key(&mut platform, 958, dash_to_credits!(0.1)); | ||
|
|
||
| let results = process_update( | ||
| &platform, | ||
| identity.id(), | ||
| &signer, | ||
| &master_key, | ||
| 1, | ||
| 1, | ||
| &[(10, Purpose::PAYMENT_SCAN, 4010)], | ||
| vec![], | ||
| platform_version, | ||
| ) | ||
| .await; | ||
| assert!( | ||
| !matches!( | ||
| results.as_slice(), | ||
| [StateTransitionExecutionResult::SuccessfulExecution { .. }] | ||
| ), | ||
| "payment keys must be rejected before protocol version 14, got: {:?}", | ||
| results | ||
| ); |
There was a problem hiding this comment.
🟡 Suggestion: Pin the activation test to protocol version 14
The acceptance test uses PlatformVersion::latest(), so it will silently move to later protocol versions and stop protecting the version-14 table. Select PlatformVersion::get(14) explicitly. The version-13 test should also assert the expected paid InvalidIdentityPublicKeySecurityLevelError rather than merely checking that execution was unsuccessful; the current assertion passes for unrelated signature, nonce, serialization, balance, or internal failures and therefore does not prove the activation boundary.
source: ['codex']
…er, cover the v13-v14 boundary Address review: the 21-byte schema constraint alone accepted values whose leading byte is not a supported address type. A new data trigger (bindings list v2, PV14) rejects profile creates/replaces whose corePaymentAddress or platformPaymentAddress does not start with 0x00 (P2PKH) or 0x01 (P2SH) — unlike a Base58Check string, the storage form has no checksum, so this check makes accepted values fully decodable. - profile fixtures now carry valid type bytes (guarded to schema versions that have the fields, so protocol-version-11 variants are untouched) - acceptance/rejection matrix test for both fields across leading bytes - boundary test drives the v13→v14 upgrade through the production perform_events_on_first_block_of_protocol_change dispatcher with a stored v1 profile and warmed contract cache, asserting both fields absent before, present after, and the legacy profile still readable - pre-upgrade assertion now also covers platformPaymentAddress - rebaselines for the schema description change (check_tx fees, root hash) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…/dashpay-payment-addresses
…xed test name Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…/dashpay-payment-addresses
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…/dashpay-payment-addresses
|
Parked (converted to draft), not abandoned. The non-public payment tier has been re-scoped: the public tips tier (#4380, merged) plus shielded-pool payments cover the product need, so the notified/stealth tier this PR implements — The deciding argument: any payment-time Platform artifact (the payment notification that stealth discovery depends on) is timing-correlatable with the settlement transaction by a global observer, which defeats the unlinkability goal. The clean alternatives are either chain-native scanning (a BIP352-style DAPI tweak index — real new infrastructure) or simply routing private payments through the shielded pool, which already exists and hides the amount as well. We chose the pool. Everything here remains reviewable and revivable: the pastaclaw round-1 findings are all addressed, the branch stays, and the published detection keys were deliberately made BIP352-shaped so a future scanning-based tier can adopt them unchanged. DIP-33 (dashpay/dips#188) is being redrafted to the reduced scope with this design recorded as considered-and-deferred. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The payment-purpose model and stealth derivation are generally coherent, but three consensus blockers remain: creation paths accept contract-bounded payment keys that later fail internally, the uniqueness scan is unmetered, and the new uniqueness behavior was added directly to identity-update state-validation v0. Four API, FFI, performance, and activation-test suggestions also remain valid; Opus review is deferred until the Codex blocker gate clears.
Source: reviewer backend gpt-5.6-sol; final verifier backend gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
6 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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-sdk-ffi/src/identity/create_from_components.rs`:
- [SUGGESTION] packages/rs-sdk-ffi/src/identity/create_from_components.rs:111-125: Synchronize the legacy Rust C API with the new Swift purpose values
Swift now exposes `paymentScan = 7` and `paymentSpend = 8`, and `identityToHandle` forwards every public key's raw purpose through `DashSDKPublicKeyData`. This decoder still accepts only values 0 through 6, so converting a `DPPIdentity` containing a payment key into a native identity handle fails with `Invalid key purpose`. Document and token operations using this bridge consequently cannot use such identities. The separate decoder in `packages/rs-sdk-ffi/src/identity/keys.rs` is also stale. Use the canonical `Purpose::try_from` conversion, or explicitly accept discriminants 7 and 8 at both C entry points.
In `packages/rs-dpp/src/state_transition/state_transitions/identity/public_key_in_creation/methods/validate_identity_public_keys_structure/v1/mod.rs`:
- [BLOCKING] packages/rs-dpp/src/state_transition/state_transitions/identity/public_key_in_creation/methods/validate_identity_public_keys_structure/v1/mod.rs:150-182: Reject contract bounds on payment keys during structure validation
(existing thread: https://github.com/dashpay/platform/pull/4272#discussion_r3766545358)
The payment-purpose checks enforce key type and compressed encoding but do not reject `contract_bounds`, despite the PR defining payment keys as unbounded. Identity updates later call `validate_identity_public_keys_contract_bounds`, but identity-create, identity-create-from-addresses, and identity-create-from-shielded-pool do not perform that validation. Those creation transitions can therefore pass consensus validation with a bounded `PAYMENT_SCAN` or `PAYMENT_SPEND` key and reach Drive insertion, where `add_potential_contract_info_for_contract_bounded_key_v0` rejects every purpose other than encryption or decryption with the internal `IdentityKeyBoundsError("purpose not available for key bounds")`. Reject bounded payment keys here with `InvalidKeyPurposeForContractBoundsError`, and add creation-pipeline coverage so malformed user input produces a deterministic consensus error rather than an internal execution failure.
In `packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_payment_key_uniqueness/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_payment_key_uniqueness/v0/mod.rs:37-69: Meter the all-keys lookup used by payment-key uniqueness validation
(existing thread: https://github.com/dashpay/platform/pull/4272#discussion_r3766545363)
Every update adding a payment key performs an unbounded `AllKeys` query over the identity's complete key history. `fetch_identity_keys` executes the GroveDB query and collects its low-level operations, but then discards those operations, while this helper explicitly ignores `_execution_context`. Repeated payment-key rotations can therefore force validators to read and deserialize a growing key history without charging for that work. Use `fetch_identity_keys_with_costs` with the last committed epoch and record its `FeeResult` as `ValidationOperation::PrecalculatedOperation`. The identity-update state implementation must also receive and use the processor-owned execution context instead of its private local context; otherwise the new charge will not be included in the resulting `ExecutionEvent`.
In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/state/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/state/v0/mod.rs:124-139: Introduce a new identity-update state-validation version
(existing thread: https://github.com/dashpay/platform/pull/4272#discussion_r3766545366)
The payment-key uniqueness rule was inserted directly into `validate_state_v0`. Both the protocol-13 and protocol-14 Drive-ABCI validation tables still select identity-update state version 0, so the PR changes an existing consensus implementation in place. Restore v0 to its prior behavior, add a `state/v1` implementation containing the uniqueness rule and correctly threaded execution context, extend the dispatcher to support versions 0 and 1, and select version 1 only in the protocol-14 validation table. Add dispatch coverage showing that protocol 13 retains v0 while protocol 14 activates v1.
In `packages/rs-platform-wallet/src/wallet/identity/crypto/stealth.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/crypto/stealth.rs:51-123: Represent the closed payment key classes with an enum
(existing thread: https://github.com/dashpay/platform/pull/4272#discussion_r3766545370)
The module defines the three DIP-33 key classes—scan, spend, and notification out—as constants, but both public derivation APIs accept an unrestricted `u32`. Any other hardened value therefore produces a valid-looking path for a nonstandard key class instead of being rejected. Introduce a `PaymentKeyClass` enum and convert its variants to hardened indices internally. That makes invalid roles unrepresentable and gives callers exhaustive handling if another standardized class is added later.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/crypto/stealth.rs:157-225: Reuse the secp256k1 context during destination scanning
(existing thread: https://github.com/dashpay/platform/pull/4272#discussion_r3766545373)
Each payer, receiver, and spender entry point constructs and randomizes a fresh `Secp256k1` context. `recognize_one_time_destination` is intended to run repeatedly while scanning candidate notifications and output counters, so this repeatedly allocates and initializes a context on the hottest path. Accept a caller-owned context, provide a scanner that owns one, or use an available shared context, then pass it into the lower functions that already borrow a context.
In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/mod.rs:2008-2229: Pin the activation test to protocol version 14
(existing thread: https://github.com/dashpay/platform/pull/4272#discussion_r3766545380)
The protocol-14 acceptance tests use `PlatformVersion::latest()`, so they will silently move to later protocol versions and stop protecting the version-14 dispatch table. Select `PlatformVersion::get(14)` explicitly. The protocol-13 test also asserts only that execution was not successful; it should match the expected paid `InvalidIdentityPublicKeySecurityLevelError`, because unrelated signature, nonce, serialization, balance, or internal failures currently satisfy the assertion without proving the activation boundary.
2 participants
Issue being fixed or feature implemented
Second slice of DIP-33 (dashpay/dips#188), stacked on #4380 (the public tips tier — review that first). This PR adds the notified tier's key material and math: identity payment detection keys and stealth one-time address derivation for the Core and Platform transparent rails.
What was done?
PAYMENT_SCAN = 7/PAYMENT_SPEND = 8(protocol version 14): ECDSA_SECP256K1 only, exactly 33-byte compressed SEC1 points, non-signing, no contract bounds, not searchable, at most one active key per purpose per identity.InvalidKeyPurposeKeyTypeError(10535) andTooManyPublicKeysOfPurposeError(10536), appended at the BasicError tail with discriminant-pinning tests.platform-encryption::stealth(pure curve ops, KAT vectors matching DIP-33) and the platform-wallet module (DIP-9 feature 33' derivation; payer/receiver/spender roles; rail-domain-separated one-time destinations for Core P2PKH and Platform addresses; typed retryable errors).KeyPurpose(with unknown-discriminant skip instead of coercion), KotlinKeyPurpose.How Has This Been Tested?
:sdk:compileDebugKotlinpasses; Swift covered by CI.Breaking Changes
Consensus-breaking at protocol version 14 (new key purposes accepted by structure validation). Pre-release; no migration concerns.
Checklist:
For repository code-owners and collaborators only