Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions packages/rs-platform-wallet/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ dashcore = { workspace = true }
# Standard dependencies
thiserror = "1.0"
async-trait = "0.1"
tracing = "0.1"

# Collections
indexmap = "2.0"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
//! Gap-limit identity discovery for wallet sync
//!
//! This module implements DashSync-style gap-limit scanning for identities
//! during wallet sync. It derives consecutive authentication keys from the
//! wallet's BIP32 tree and queries Platform to find registered identities.

use super::key_derivation::derive_identity_auth_key_hash;
use super::PlatformWalletInfo;
use crate::error::PlatformWalletError;
use dpp::identity::accessors::IdentityGettersV0;
use dpp::prelude::Identifier;
use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface;

impl PlatformWalletInfo {
/// Discover identities by scanning consecutive identity indices with a gap limit
///
/// Starting from `start_index`, derives ECDSA authentication keys for consecutive
/// identity indices and queries Platform for registered identities. Scanning stops
/// when `gap_limit` consecutive indices yield no identity.
///
/// This mirrors the DashSync gap-limit approach: keep scanning until N consecutive
/// misses, then stop.
///
/// # Arguments
///
/// * `wallet` - The wallet to derive authentication keys from
/// * `start_index` - The first identity index to check
/// * `gap_limit` - Number of consecutive misses before stopping (typically 5)
///
/// # Returns
///
/// Returns the list of newly discovered identity IDs
pub async fn discover_identities(
&mut self,
wallet: &key_wallet::Wallet,
start_index: u32,
gap_limit: u32,
) -> Result<Vec<Identifier>, PlatformWalletError> {
use dash_sdk::platform::types::identity::PublicKeyHash;
use dash_sdk::platform::Fetch;

let sdk = self
.identity_manager()
.sdk
.as_ref()
.ok_or_else(|| {
PlatformWalletError::InvalidIdentityData(
"SDK not configured in identity manager".to_string(),
)
})?
.clone();

let network = self.network();
let mut discovered = Vec::new();
let mut consecutive_misses = 0u32;
let mut identity_index = start_index;

while consecutive_misses < gap_limit {
// Derive the authentication key hash for this identity index (key_index 0)
let key_hash_array = derive_identity_auth_key_hash(wallet, network, identity_index, 0)?;

// Query Platform for an identity registered with this key hash
match dpp::identity::Identity::fetch(&sdk, PublicKeyHash(key_hash_array)).await {
Ok(Some(identity)) => {
let identity_id = identity.id();

// Add to manager if not already present
if self.identity_manager().identity(&identity_id).is_none() {
self.identity_manager_mut().add_identity(identity)?;
discovered.push(identity_id);
}
consecutive_misses = 0;
}
Ok(None) => {
consecutive_misses += 1;
}
Err(e) => {
tracing::warn!(
identity_index,
"Failed to query identity by public key hash: {}",
e
);
// Don't increment consecutive_misses: a query failure is not
// a confirmed empty slot and should not consume the gap budget.
}
Comment on lines +77 to +85
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don't spend the gap budget on query failures.

A Platform/SDK error is not a confirmed empty slot. Incrementing consecutive_misses here can stop the scan early and skip identities later in the range. Either bubble the error up, or log/retry without counting it as a miss.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/rs-platform-wallet/src/platform_wallet_info/identity_discovery.rs`
around lines 82 - 89, The Err(e) arm currently increments consecutive_misses
(variable consecutive_misses) which treats query failures as empty slots;
instead do not count errors as misses—either propagate the error out from the
current function (replace the Err(e) branch with an early return/return Err(e)
or use the ? operator) or log and retry without modifying consecutive_misses;
keep the tracing::warn call but remove the consecutive_misses += 1 so query
failures don't stop the scan prematurely.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 33b3da2 — removed the consecutive_misses += 1 from the error branch. Query failures now just log a warning without consuming gap budget.

}

identity_index += 1;
}
Comment on lines +58 to +89
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking: Unbounded loop when Platform queries consistently fail

The while consecutive_misses < gap_limit loop only terminates when gap_limit consecutive Ok(None) results are seen. In the Err(e) branch, the code intentionally does not increment consecutive_misses (the comment explains why), but identity_index advances unconditionally. If Platform is unreachable, every iteration hits Err, consecutive_misses stays at 0, and the loop runs until identity_index reaches 2^31 (~2 billion iterations) where ChildNumber::from_hardened_idx finally fails. In practice this is an effective livelock that will hang wallet sync for hours or days during a network outage. Add a cap on consecutive errors (e.g., bail after 3-5 consecutive failures) or a total iteration limit.

source: ['claude-general', 'codex-general', 'claude-rust-quality', 'codex-rust-quality']

🤖 Fix this 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-platform-wallet/src/platform_wallet_info/identity_discovery.rs`:
- [BLOCKING] lines 58-89: Unbounded loop when Platform queries consistently fail
  The `while consecutive_misses < gap_limit` loop only terminates when `gap_limit` consecutive `Ok(None)` results are seen. In the `Err(e)` branch, the code intentionally does not increment `consecutive_misses` (the comment explains why), but `identity_index` advances unconditionally. If Platform is unreachable, every iteration hits `Err`, `consecutive_misses` stays at 0, and the loop runs until `identity_index` reaches 2^31 (~2 billion iterations) where `ChildNumber::from_hardened_idx` finally fails. In practice this is an effective livelock that will hang wallet sync for hours or days during a network outage. Add a cap on consecutive errors (e.g., bail after 3-5 consecutive failures) or a total iteration limit.

Comment on lines +58 to +89
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking: Unbounded loop when Platform queries consistently fail

The while consecutive_misses < gap_limit loop only terminates when gap_limit consecutive Ok(None) results are seen. In the Err(e) branch, the code intentionally does not increment consecutive_misses (the comment explains why), but identity_index advances unconditionally. If Platform is unreachable, every iteration hits Err, consecutive_misses stays at 0, and the loop runs until identity_index reaches 2^31 (~2 billion iterations) where ChildNumber::from_hardened_idx finally fails. In practice this is an effective livelock that will hang wallet sync for hours or days during a network outage. Add a cap on consecutive errors (e.g., bail after 3-5 consecutive failures) or a total iteration limit.

source: ['claude-general', 'codex-general', 'claude-rust-quality', 'codex-rust-quality']

🤖 Fix this 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-platform-wallet/src/platform_wallet_info/identity_discovery.rs`:
- [BLOCKING] lines 58-89: Unbounded loop when Platform queries consistently fail
  The `while consecutive_misses < gap_limit` loop only terminates when `gap_limit` consecutive `Ok(None)` results are seen. In the `Err(e)` branch, the code intentionally does not increment `consecutive_misses` (the comment explains why), but `identity_index` advances unconditionally. If Platform is unreachable, every iteration hits `Err`, `consecutive_misses` stays at 0, and the loop runs until `identity_index` reaches 2^31 (~2 billion iterations) where `ChildNumber::from_hardened_idx` finally fails. In practice this is an effective livelock that will hang wallet sync for hours or days during a network outage. Add a cap on consecutive errors (e.g., bail after 3-5 consecutive failures) or a total iteration limit.


Ok(discovered)
}

/// Discover identities and fetch their DashPay contact requests
///
/// Calls [`discover_identities`] then fetches sent and received contact requests
/// for each discovered identity, storing them in the identity manager.
///
/// # Arguments
///
/// * `wallet` - The wallet to derive authentication keys from
/// * `start_index` - The first identity index to check
/// * `gap_limit` - Number of consecutive misses before stopping (typically 5)
///
/// # Returns
///
/// Returns the list of newly discovered identity IDs
pub async fn discover_identities_with_contacts(
&mut self,
wallet: &key_wallet::Wallet,
start_index: u32,
gap_limit: u32,
) -> Result<Vec<Identifier>, PlatformWalletError> {
let discovered = self
.discover_identities(wallet, start_index, gap_limit)
.await?;

if discovered.is_empty() {
return Ok(discovered);
}

let sdk = self
.identity_manager()
.sdk
.as_ref()
.ok_or_else(|| {
PlatformWalletError::InvalidIdentityData(
"SDK not configured in identity manager".to_string(),
)
})?
.clone();

for identity_id in &discovered {
// Get the identity from the manager to pass to the SDK
let identity = match self.identity_manager().identity(identity_id) {
Some(id) => id.clone(),
None => continue,
};

if let Err(e) = self
.fetch_and_store_contact_requests(&sdk, &identity, identity_id)
.await
{
tracing::warn!(
%identity_id,
"Failed to fetch contact requests during discovery: {}",
e
);
}
}

Ok(discovered)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
//! Shared key derivation utilities for identity authentication keys
//!
//! This module provides helper functions used by both the matured transactions
//! processor and the identity discovery scanner.

use crate::error::PlatformWalletError;
use key_wallet::Network;

/// Derive the 20-byte RIPEMD160(SHA256) hash of the public key at the given
/// identity authentication path.
///
/// Path format: `base_path / identity_index' / key_index'`
/// where `base_path` is `m/9'/COIN_TYPE'/5'/0'` (mainnet or testnet).
///
/// # Arguments
///
/// * `wallet` - The wallet to derive keys from
/// * `network` - Network to select the correct coin type
/// * `identity_index` - The identity index (hardened)
/// * `key_index` - The key index within that identity (hardened)
///
/// # Returns
///
/// Returns the 20-byte public key hash suitable for Platform identity lookup.
pub(crate) fn derive_identity_auth_key_hash(
wallet: &key_wallet::Wallet,
network: Network,
identity_index: u32,
key_index: u32,
) -> Result<[u8; 20], PlatformWalletError> {
use dashcore::secp256k1::Secp256k1;
use dpp::util::hash::ripemd160_sha256;
use key_wallet::bip32::{ChildNumber, DerivationPath, ExtendedPubKey};
use key_wallet::dip9::{
IDENTITY_AUTHENTICATION_PATH_MAINNET, IDENTITY_AUTHENTICATION_PATH_TESTNET,
};

let base_path = match network {
Network::Dash => IDENTITY_AUTHENTICATION_PATH_MAINNET,
Network::Testnet => IDENTITY_AUTHENTICATION_PATH_TESTNET,
_ => {
return Err(PlatformWalletError::InvalidIdentityData(
"Unsupported network for identity derivation".to_string(),
));
}
};

let mut full_path = DerivationPath::from(base_path);
full_path = full_path.extend([
ChildNumber::from_hardened_idx(identity_index).map_err(|e| {
PlatformWalletError::InvalidIdentityData(format!("Invalid identity index: {}", e))
})?,
ChildNumber::from_hardened_idx(key_index).map_err(|e| {
PlatformWalletError::InvalidIdentityData(format!("Invalid key index: {}", e))
})?,
]);

let auth_key = wallet
.derive_extended_private_key(&full_path)
.map_err(|e| {
PlatformWalletError::InvalidIdentityData(format!(
"Failed to derive authentication key: {}",
e
))
})?;

let secp = Secp256k1::new();
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Nitpick: Secp256k1 context allocated per call in a loop

Secp256k1::new() allocates a ~750 KB signing+verification context. In discover_identities, this is called once per iteration via derive_identity_auth_key_hash. Since each iteration also does a network round-trip to Platform, the allocation overhead is negligible in practice. Still, accepting a &Secp256k1 parameter or using Secp256k1::verification_only() (~350 KB) would be cleaner if this function is ever used in tighter loops.

source: ['claude-rust-quality']

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Nitpick: Secp256k1 context allocated per call in a loop

Secp256k1::new() allocates a ~750 KB signing+verification context. In discover_identities, this is called once per iteration via derive_identity_auth_key_hash. Since each iteration also does a network round-trip to Platform, the allocation overhead is negligible in practice. Still, accepting a &Secp256k1 parameter or using Secp256k1::verification_only() (~350 KB) would be cleaner if this function is ever used in tighter loops.

source: ['claude-rust-quality']

let public_key = ExtendedPubKey::from_priv(&secp, &auth_key);
let public_key_bytes = public_key.public_key.serialize();
let key_hash = ripemd160_sha256(&public_key_bytes);

let mut key_hash_array = [0u8; 20];
key_hash_array.copy_from_slice(&key_hash);

Ok(key_hash_array)
}
Loading
Loading