feat: remove index tracking from commitment mask index #7373
Conversation
WalkthroughThis update removes all key index searching and updating logic from the key manager and transaction key manager modules. Several methods and their trait definitions for finding key indices, updating them, and retrieving spending key IDs are eliminated. Related tests are deleted, and the logic for output key recovery and next key generation is simplified, with special handling now only for the "PRE_MINE" branch. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TransactionKeyManager
Client->>TransactionKeyManager: get_next_key(branch)
alt branch == "PRE_MINE"
TransactionKeyManager->>TransactionKeyManager: Increment key index
TransactionKeyManager-->>Client: Return new key and ID
else branch != "PRE_MINE"
TransactionKeyManager->>TransactionKeyManager: Generate random key index
TransactionKeyManager-->>Client: Return new key and ID
end
sequenceDiagram
participant Client
participant TransactionKeyManager
Client->>TransactionKeyManager: try_output_key_recovery(commitment, encrypted_data, custom_recovery_key_id)
TransactionKeyManager->>TransactionKeyManager: Decrypt private key
TransactionKeyManager->>TransactionKeyManager: Import private key (as imported key ID)
TransactionKeyManager-->>Client: Return imported key ID, value, memo
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
base_layer/core/src/transactions/transaction_key_manager/inner.rs (1)
183-197: Consider documenting the indexing strategy.The shift from sequential to random index generation (except for PRE_MINE) is a significant architectural change. Consider adding a comment explaining why PRE_MINE uses sequential indexing while others use random indices.
pub async fn get_next_key(&self, branch: &str) -> Result<TariKeyAndId, KeyManagerServiceError> { let index = { match branch { + // PRE_MINE branch maintains sequential indexing for deterministic key generation PRE_MINE => { let mut km = self .key_managers .get(branch) .ok_or_else(|| self.unknown_key_branch_error("get_next_key", branch))? .write() .await; self.db.increment_key_index(branch)?; km.increment_key_index(1) }, + // Other branches use random indices to avoid index synchronization overhead _ => OsRng.next_u64(), } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
base_layer/core/src/transactions/transaction_key_manager/inner.rs(4 hunks)base_layer/core/src/transactions/transaction_key_manager/interface.rs(0 hunks)base_layer/core/src/transactions/transaction_key_manager/wrapper.rs(0 hunks)base_layer/key_manager/src/key_manager_service/handle.rs(0 hunks)base_layer/key_manager/src/key_manager_service/interface.rs(0 hunks)base_layer/key_manager/src/key_manager_service/service.rs(0 hunks)base_layer/wallet/tests/key_manager_service_tests/service.rs(0 hunks)base_layer/wallet/tests/transaction_service_tests/service.rs(1 hunks)
💤 Files with no reviewable changes (6)
- base_layer/wallet/tests/key_manager_service_tests/service.rs
- base_layer/core/src/transactions/transaction_key_manager/wrapper.rs
- base_layer/key_manager/src/key_manager_service/service.rs
- base_layer/key_manager/src/key_manager_service/handle.rs
- base_layer/core/src/transactions/transaction_key_manager/interface.rs
- base_layer/key_manager/src/key_manager_service/interface.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: ledger build tests
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: ci
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: cargo check with stable
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
🔇 Additional comments (3)
base_layer/wallet/tests/transaction_service_tests/service.rs (1)
1548-1548: LGTM! Test correctly adapted to API changes.The destructuring pattern change properly adapts to the updated
try_output_key_recoverymethod signature while maintaining the essential test functionality. The spending key ID is now intentionally ignored (with underscore prefix), which aligns with the PR objective to remove index tracking from the commitment mask index.base_layer/core/src/transactions/transaction_key_manager/inner.rs (2)
1543-1563: Excellent optimization for output recovery!The direct key import approach eliminates the expensive index search and update operations, which aligns perfectly with the PR's performance improvement goals. This is a much cleaner solution when the actual private key is already available from decryption.
183-197: Could not locate any explicit collision detection or duplicate‐index checks in the codebase. Since all non–PRE_MINE branches now use a random u64 for the key index (with no tracking in the DB or in memory), collisions—while astronomically unlikely—are neither checked nor guarded against. This change also makes it impossible to enumerate or replay‐derive non–PRE_MINE keys.Please confirm that:
- You accept relying on the statistical improbability of u64 collisions without explicit guards.
- There are no wallet recovery or auditing features that expect sequential, trackable indices for non–PRE_MINE branches.
- Key enumeration (e.g., for backups or key exports) is not required for these branches.
If any of the above is unacceptable, we should restore index tracking or introduce collision detection.
Description
Removes index tracking from commitment mask index
Motivation and Context
This used to be very important before we started using the key manager and we needed to keep track of more than one spend wallet on the same cipher seed. Trying to keep the indexes in sync causes massive performance issues accorss view wallets, offline singing etc. We can jut decryt the key and save it when we detect it.
Summary by CodeRabbit
Refactor
Bug Fixes
Tests
No user-facing features were added or removed.