fix: duplicate identity keys created in multiple wallets#289
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review all |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1205 🏁 Script executed: Length of output: 4476 🏁 Script executed: Length of output: 15483 @lklimek I've completed a review of PR #289, which addresses the issue of duplicate identity keys being created when multiple wallets are loaded. Core Issue & FixThe main problem appears to be in the identity key generation process where a hardcoded index ( - wallet.identity_authentication_ecdsa_private_key(app_context.network, 0, 0, Some(&app_context))
+ wallet.identity_authentication_ecdsa_private_key(app_context.network, identity_id_number, 0, Some(&app_context))This change is applied consistently to all four key derivation calls, ensuring that each wallet's identity keys are properly isolated. Additional Improvements
Overall AssessmentThis PR looks good and directly addresses the reported issue. The changes are focused and include appropriate logging to help with debugging. The database migration ensures that existing users will have their data corrected automatically. The PR is ready to be merged once all CI checks pass. |
There was a problem hiding this comment.
Pull Request Overview
This PR centralizes wallet update logic in the UI and ensures identity private keys are regenerated when switching wallets, fixes secure seed erasure in the wallet model, and adds database tracing, version bump, and network-string fixes to prevent duplicate identity keys across wallets.
- Refactor AddNewIdentityScreen: introduce
update_wallet,ensure_correct_identity_keys, andupdate_identity_keyto streamline identity key handling - Securely erase wallet seed in
WalletSeed’sDrop, add doc foridentity_registration_ecdsa_private_key - Instrument database loading with
tracing::trace!, bump DB version to 8 with migration, fix devnet network name handling
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ui/identities/add_new_identity_screen/mod.rs | Centralize wallet selection and identity key regeneration logic |
| src/model/wallet/mod.rs | Update WalletSeed drop cleanup and add identity key method doc |
| src/database/wallet.rs | Add tracing::trace! logs for wallet loading steps |
| src/database/initialization.rs | Bump DEFAULT_DB_VERSION to 8 and add identity network fix-up |
| src/database/identities.rs | Log saving identity without wallet |
| src/context.rs | Simplify Devnet network string from devnet: to devnet |
Comments suppressed due to low confidence (3)
src/model/wallet/mod.rs:234
- The original
close()call ensured all cleanup was done; now onlyOpenvariants zeroize the seed. Verify that no sensitive data remains in other variants and thatclose()isn’t required to perform additional cleanup steps.
if let WalletSeed::Open(open_seed) = self {
src/database/initialization.rs:7
- The DB version has been bumped to 8—ensure any external documentation or migration guides are updated to reflect this new version.
pub const DEFAULT_DB_VERSION: u16 = 8;
src/context.rs:225
- Switching from
devnet:<name>to a fixeddevnetrepresentation may break existing configurations or tooling. Verify that all consumers of this value have been updated accordingly.
Network::Devnet => "devnet".to_string(),
| )?; | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
this stops us from being able to use multiple devnets
There was a problem hiding this comment.
As discussed, we drop multi-devnet approach.
There was a problem hiding this comment.
this only updates one table... there are many tables that have a network.
There was a problem hiding this comment.
fixed, please double-check if I didn't miss sth
…fix/duplicate-keys
When we load more than 1 wallet into Dash Evo Tool, identity keys in second one sometimes are the same as corresponding identities in first one.
Core issue was that private keys were not correctly regenerated when switching to another wallet. This has been fixed. Wallet update logic was refactored to have one central place where it happens.