feat: use deterministic derivation path for platform address funding#512
feat: use deterministic derivation path for platform address funding#512pauldelucia wants to merge 1 commit intov1.0-devfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR introduces deterministic key derivation for platform address funding by adding DIP-17 support to the wallet's key derivation system. It replaces random key generation in asset lock transactions with seed-derived keys indexed through a new platform address funding key class, and adds tracking of the next available funding index. Changes
Sequence DiagramsequenceDiagram
participant FundTask as Fund Platform Address Task
participant Wallet as Wallet
participant AssetLock as Asset Lock Transaction
participant Derivation as Key Derivation (DIP-17)
FundTask->>Wallet: next_platform_address_funding_index(network)
Wallet->>Wallet: Scan watched addresses for key_class=2
Wallet-->>FundTask: Return next available index
FundTask->>AssetLock: generic_asset_lock_transaction(network, amount, ..., funding_index)
AssetLock->>Derivation: platform_address_funding_ecdsa_private_key(network, funding_index)
Derivation->>Derivation: Derive from wallet seed using DIP-17 path
Derivation-->>AssetLock: Return PrivateKey
AssetLock->>AssetLock: Compute public_key and asset_lock_address
AssetLock->>AssetLock: Create asset lock transaction
AssetLock-->>FundTask: Return (Transaction, PrivateKey, Address, ...)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 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 |
Replace random one-time keys with deterministic keys derived from wallet seed for platform address funding asset locks. Uses DIP-17 path structure with key_class=2 for funding: m/9'/coin_type'/17'/0'/2'/index This is a local workaround until dash-sdk adds native support for platform_address_funding_path(). Changes: - Add DerivationPathReference::PlatformAddressFunding variant - Add PLATFORM_KEY_CLASS_FUNDING constant (key_class=2) - Add platform_address_funding_ecdsa_private_key() method - Add next_platform_address_funding_index() to track used indices - Update generic_asset_lock_transaction() to use derived keys Co-Authored-By: Claude Opus 4.5 <[email protected]>
4a71c00 to
2fa3a34
Compare
| }; | ||
|
|
||
| let max_existing = self | ||
| .watched_addresses |
There was a problem hiding this comment.
When you have two computers with DET, there is no guarantee that watched_addresses will be the same on both. It means there is a potential index reuse weakness, leading to private key reuse.
I would rather derive the index from some identity metadata - I think we have identity index, don't we? So maybe we can use the identity index here instead?
can we just make this happen is dash-sdk? |
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean implementation of deterministic HD-derived keys for platform address funding. Derivation path follows DIP-17 conventions and concurrency is correctly handled within the wallet lock scope. However, the new funding path (key_class=2) is misclassified by is_platform_payment() which only checks the DIP-17 prefix without filtering on key_class.
Reviewed commit: 2fa3a34
🔴 1 blocking
1 additional finding
🔴 blocking: key_class=2 funding paths misclassified as Platform payment addresses
src/model/wallet/mod.rs (line 174)
is_platform_payment() only checks m/9'/coin_type'/17'/... and ignores the key_class component, so the new funding path m/9'/coin_type'/17'/0'/2'/index matches the payment predicate too. This leaks funding keys into payment-only flows:
platform_receive_address()and new-address index selectionplatform_addresses()listingget_platform_address_private_key()- Core refresh skips these Core P2PKH funding addresses as if they were Platform-only
The helper needs to require key_class == 0 for payments, or callers need to filter on DerivationPathReference::PlatformPayment instead of the broad DIP-17 prefix.
🤖 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 `src/model/wallet/mod.rs`:
- [BLOCKING] line 174: key_class=2 funding paths misclassified as Platform payment addresses
`is_platform_payment()` only checks `m/9'/coin_type'/17'/...` and ignores the `key_class` component, so the new funding path `m/9'/coin_type'/17'/0'/2'/index` matches the payment predicate too. This leaks funding keys into payment-only flows:
- `platform_receive_address()` and new-address index selection
- `platform_addresses()` listing
- `get_platform_address_private_key()`
- Core refresh skips these Core P2PKH funding addresses as if they were Platform-only
The helper needs to require `key_class == 0` for payments, or callers need to filter on `DerivationPathReference::PlatformPayment` instead of the broad DIP-17 prefix.
|
Closing — superseded by #790, which takes a more complete approach to the same problem (deterministic HD keys for asset lock transactions). PR #790 uses 🤖 Co-authored by Claudius the Magnificent AI Agent |
Replace random one-time keys with deterministic keys derived from wallet seed for platform address funding asset locks.
Uses DIP-17 path structure with key_class=2 for funding: m/9'/coin_type'/17'/0'/2'/index
This is a local workaround until dash-sdk adds native support for platform_address_funding_path().
Changes:
Summary by CodeRabbit
Security Improvements
Infrastructure Updates