Skip to content

fix(wallet): use deterministic HD key for generic asset lock transactions#790

Draft
lklimek wants to merge 3 commits intov1.0-devfrom
fix/deterministic-asset-lock-keys
Draft

fix(wallet): use deterministic HD key for generic asset lock transactions#790
lklimek wants to merge 3 commits intov1.0-devfrom
fix/deterministic-asset-lock-keys

Conversation

@lklimek
Copy link
Copy Markdown
Contributor

@lklimek lklimek commented Mar 24, 2026

Summary

Replaces the randomly generated private key in generic_asset_lock_transaction() with a deterministic HD-derived key, preventing permanent fund loss when Platform rejects a state transition after Core has already broadcast the asset lock transaction.

User Story

Imagine you are mining on a local network and accumulate 500 tDASH across many small coinbase UTXOs. You try to fund a Platform address with 400 tDASH. The Core transaction broadcasts successfully (your balance drops), but Platform rejects the state transition with "Tx too large". With the old code, the private key for the asset lock was randomly generated and held only in memory — once the error propagated, the key was lost and your funds were unrecoverable. With this fix, the key is derived from your wallet seed and can always be re-derived for retry or recovery.

Changes

  • New derivation path m/9'/coin_type'/15'/index' for generic asset lock funding (sub-feature 15 avoids collision with 5=identity, 17=platform payment)
  • generic_asset_lock_ecdsa_private_key() derives the key from the wallet seed and registers the address in known_addresses (same pattern as identity_top_up_ecdsa_private_key)
  • next_generic_funding_index() scans known addresses for the next unused index
  • Bootstrap pre-registers 8 generic funding addresses at wallet load, so asset lock recovery can find them
  • OsRng removed from generic_asset_lock_transaction() — no more random keys

Related

Test plan

  • Fund a Platform address from wallet UTXOs — verify the asset lock transaction succeeds end-to-end
  • Check that the asset lock address is registered in the wallet's known addresses after funding
  • Restart the app — verify the generic funding addresses are bootstrapped (8 addresses under m/9'/coin_type'/15'/*')
  • Simulate a Platform rejection after Core broadcast — verify the asset lock is recoverable via "Search for Asset Locks" (the key can be re-derived)
  • Fund Platform address multiple times — verify each uses a different funding index (monotonically increasing)
  • Verify identity registration and top-up flows are unchanged

🤖 Co-authored by Claudius the Magnificent AI Agent

…ions

Replace randomly generated OsRng key in generic_asset_lock_transaction()
with a deterministic HD-derived key at m/9'/coin_type'/15'/index'. This
ensures asset lock keys are always recoverable from the wallet seed,
preventing permanent fund loss when Platform rejects a state transition
after Core has already broadcast the asset lock transaction.

Co-Authored-By: Claude Opus 4.6 <[email protected]>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 24, 2026

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: abc4e862-4e5c-487c-a548-63872c9c6182

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/deterministic-asset-lock-keys

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Replace incorrect feature 15' with DIP-9-compliant Feature 5' sub-feature
paths for asset lock key derivation. Introduce AssetLockUsage enum that
maps each usage to its DIP-9 sub-feature index (1'=Registration,
2'=TopUp, 3'=Invitation, 4'=PlatformAddressFunding, 5'=ShieldedTopUp).

Path structure changes from m/9'/ct'/15'/index' (4 components) to
m/9'/ct'/5'/sub_feature'/index' (5 components). Bootstrap now pre-derives
addresses for both PlatformAddressFunding and ShieldedTopUp usages.

Add TODOs for pre-existing registration/top-up path mismatches that
would require a migration strategy to fix.

Co-Authored-By: Claude Opus 4.6 <[email protected]>
@lklimek
Copy link
Copy Markdown
Contributor Author

lklimek commented Mar 24, 2026

Note: Shield-from-asset-lock DB persistence gap

This PR improves key recovery for asset locks (preventing fund loss from lost private keys), but it doesn't address a separate issue flagged in PR #786 review: the shield_from_asset_lock() path broadcasts the asset lock transaction directly via core_client.send_raw_transaction() without persisting the asset lock to the database first.

The existing identity-funding flow uses broadcast_and_commit_asset_lock() which stores the asset lock in the DB before broadcast and removes it on failure. The shield-from-asset-lock path skips this, so if the app crashes after Core broadcast but before the shielded state transition completes, there's no DB record for the standard recovery path to reuse. Funds aren't permanently lost (recover_asset_locks() can find unspent locks on-chain via list_unspent), but the automatic recovery guarantee is weaker.

For parity, consider also:

  1. Persisting the asset lock to the database before broadcast
  2. Rolling back the DB entry on broadcast failure
  3. Allowing recovery via "Search for Asset Locks"

🤖 Co-authored by Claudius the Magnificent AI Agent

Replace direct broadcast_raw_transaction() calls in
create_registration_asset_lock and create_top_up_asset_lock with
broadcast_and_commit_asset_lock() to ensure DB persistence before
broadcast and UTXO cleanup after. Prevents recovery gaps if app
crashes after Core broadcast but before state transition completes.

All asset lock broadcast paths now go through the single centralized
broadcast_and_commit_asset_lock() function.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants