Skip to content
Merged
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
409 changes: 139 additions & 270 deletions Cargo.lock

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ qrcode = "0.14.1"
nix = { version = "0.31.1", features = ["signal"] }
eframe = { version = "0.33.3", features = ["persistence", "wgpu"] }
base64 = "0.22.1"
dash-sdk = { git = "https://github.com/dashpay/platform", rev = "54048b9352c5c8361f4cbfaa6a188dd3244e00c4", features = [
dash-sdk = { git = "https://github.com/dashpay/platform", rev = "9e00c8b894c45ee25df35ae694570c61ff007788", features = [
"core_key_wallet",
"core_key_wallet_manager",
"core_bincode",
Expand All @@ -28,7 +28,7 @@ dash-sdk = { git = "https://github.com/dashpay/platform", rev = "54048b9352c5c83
"core_spv",
"shielded",
] }
rs-sdk-trusted-context-provider = { git = "https://github.com/dashpay/platform", rev = "54048b9352c5c8361f4cbfaa6a188dd3244e00c4" }
rs-sdk-trusted-context-provider = { git = "https://github.com/dashpay/platform", rev = "9e00c8b894c45ee25df35ae694570c61ff007788" }
zip32 = "0.2.0"
grovestark = { git = "https://www.github.com/dashpay/grovestark", rev = "5b9e289cca54c79b1305d5f4f40bf1148f1eb0e3" }
rayon = "1.8"
Expand Down
3 changes: 3 additions & 0 deletions src/backend_task/contract.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,9 @@ impl AppContext {
// Fetch the contract description from the Search Contract
let search_contract = &self.keyword_search_contract;
let document_query = DocumentQuery {
select: Default::default(),
group_by: vec![],
having: vec![],
data_contract: search_contract.clone(),
document_type_name: "fullDescription".to_string(),
limit: 1,
Expand Down
123 changes: 64 additions & 59 deletions src/backend_task/core/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ use dash_sdk::dpp::dashcore::{
};
use dash_sdk::dpp::fee::Credits;
use dash_sdk::dpp::key_wallet::Network as WalletNetwork;
use dash_sdk::dpp::key_wallet::account::ECDSAAddressDerivation;
use dash_sdk::dpp::key_wallet::managed_account::managed_account_trait::ManagedAccountTrait;
use dash_sdk::dpp::key_wallet::wallet::managed_wallet_info::ManagedWalletInfo;
use dash_sdk::dpp::key_wallet::wallet::managed_wallet_info::coin_selection::SelectionStrategy;
use dash_sdk::dpp::key_wallet::wallet::managed_wallet_info::fee::FeeRate;
Expand Down Expand Up @@ -704,68 +704,81 @@ impl AppContext {
let mut scale_factor = 1.0f64;
let mut attempted_fallback = false;

// Get UTXOs and change address from the wallet account
let (utxos, change_index) = {
let managed_info =
wm.get_wallet_info(wallet_id)
// The account xpub is needed by the managed account to derive the next
// change address (key-wallet 0.43 derives addresses directly rather than
// exposing a bare next-change index).
let account_xpub = {
let wallet =
wm.get_wallet(wallet_id)
.ok_or_else(|| TaskError::WalletPaymentFailed {
detail: "Wallet info unavailable".to_string(),
detail: "Wallet object not found".to_string(),
})?;
let account = managed_info
.accounts()
let wallet_account = wallet
.accounts
.standard_bip44_accounts
.get(&DEFAULT_BIP44_ACCOUNT_INDEX)
.ok_or_else(|| TaskError::WalletPaymentFailed {
detail: "BIP44 wallet account missing".to_string(),
})?;
wallet_account.account_xpub
};

// Get UTXOs and the next (unused) change address from the managed account.
// `add_to_state = false` does not advance the address pool, but key-wallet
// 0.43's `next_change_address` still bumps the managed account's monitor
// revision unconditionally. There is no read-only "peek next change address"
// on ManagedCoreFundsAccount, so this fee-estimation peek takes a write lock
// and nudges the monitor revision. That only signals the mempool bloom filter
// it may be stale (a one-time rebuild), so it is acceptable here.
// TODO(v3.1-bump): switch to a non-mutating change-address peek if key-wallet
// exposes one upstream.
let (utxos, change_addr) = {
let managed_info = wm.get_wallet_info_mut(wallet_id).ok_or_else(|| {
TaskError::WalletPaymentFailed {
detail: "Wallet info unavailable".to_string(),
}
})?;
let account = managed_info
.accounts_mut()
.standard_bip44_accounts
.get_mut(&DEFAULT_BIP44_ACCOUNT_INDEX)
.ok_or_else(|| TaskError::WalletPaymentFailed {
detail: "BIP44 account missing".to_string(),
})?;

let utxos: Vec<_> = account.utxos.values().cloned().collect();
let change_index = account.get_next_change_address_index().unwrap_or(0);
(utxos, change_index)
let change_addr = account
.next_change_address(Some(&account_xpub), false)
.map_err(|e| TaskError::WalletPaymentFailed {
detail: format!("Failed to derive change address: {e}"),
})?;
Comment on lines +750 to +754

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🟡 Suggestion: Consider add_to_state=true now that the API exposes the choice — current false preserves a latent issue

The new next_change_address(account_xpub, add_to_state) makes the address-pool registration explicit. With add_to_state=false, the address-pool path in key-wallet generates a new change address (when no pre-generated unused change address exists) without inserting it into addresses / address_index / script_pubkey_index (address_pool.rs:507-516). Because DET's SPV registration walks account.all_addresses() (src/context/wallet_lifecycle.rs:518), and that returns only entries actually in self.addresses, a freshly-derived change address used in a send is not added to the SPV watch set.

Important context for the author and reviewers: this is NOT a fresh regression. The prior code (wallet_account.derive_change_address(change_index)) was an immutable & borrow and did not register either. The new API simply makes the trade-off visible. Two observations:

  1. The comment correctly notes parity with the prior behavior, and the PR description marks this for smoke testing. If smoke runs confirm that subsequent UTXO scans recover the change output (because the wallet records its own outgoing tx outputs), the existing false is defensible.
  2. If the smoke test reveals the change output is not picked up by SPV until a manual recovery/rescan, the simplest fix is add_to_state=true. Re-using the same change address across retries inside the loop would still work (the address-pool returns the same unused entry on subsequent calls until it's marked used).

At minimum, the outcome of this smoke check should be documented in the PR before merge, since this site sits right on the boundary the new API was designed to clarify.

source: ['claude', 'codex']

Comment on lines +750 to +754

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🟡 Suggestion: Change-address peek with add_to_state=false leaves newly derived change unregistered

Carried forward from the prior review — still valid. This site still calls next_change_address(Some(&account_xpub), false). In key-wallet 0.43, next_change_address delegates to address_pool::next_unused(..., add_to_state) which will derive a brand-new change address whenever the internal pool has no pre-generated unused entry. With add_to_state = false, that fresh address is returned but not inserted into the account's internal address pool, so the transaction can pay change to an address the wallet has never reserved/registered for monitoring. The wallet can then miss tracking its own change output until some later rescan or unrelated address-generation step discovers it. The new TODO comment correctly explains why the existing true path has an unwanted monitor-revision side effect, but does not address this correctness risk — and because this PR migrates DET onto the new explicit add_to_state API, closing this hole is part of the migration surface. Either flip to true and accept the bloom-filter rebuild, or take a different non-mutating peek that still registers the address before use.

Suggested change
let change_addr = account
.next_change_address(Some(&account_xpub), false)
.map_err(|e| TaskError::WalletPaymentFailed {
detail: format!("Failed to derive change address: {e}"),
})?;
let change_addr = account
.next_change_address(Some(&account_xpub), true)
.map_err(|e| TaskError::WalletPaymentFailed {
detail: format!("Failed to derive change address: {e}"),
})?;

source: ['codex']

(utxos, change_addr)
Comment on lines 749 to +755

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🟡 Suggestion: Change-address derived with add_to_state=false is used as the real change output, leaving it unregistered for monitoring

Carried forward from the prior review and re-validated at d510bcd. account.next_change_address(Some(&account_xpub), false) is called at line 750-751, and the returned address is then passed to builder.set_change_address(change_addr.clone()) at line 771 and broadcast as the actual change output — this is not just a fee-estimation peek. In key-wallet 0.43, next_change_address forwards add_to_state to address_pool::next_unused; when no pre-generated unused internal address remains, the fallback generate_address_at_index(next_index, key_source, add_to_state) only inserts the new address into the managed account's pool when add_to_state is true. The new comment at lines 726-734 acknowledges the pool is not advanced and argues the monitor-revision bump is acceptable for bloom-filter rebuild reasons, but a bloom rebuild does not help if the address is not in the wallet's tracked set; the monitoring concern is unresolved. Two consecutive sends before SPV sees the first change tx can also derive the same change index and reuse it. The TODO(v3.1-bump) tag is appropriate, but as long as the runtime behavior persists this stays a correctness/privacy risk.

Suggested change
let utxos: Vec<_> = account.utxos.values().cloned().collect();
let change_index = account.get_next_change_address_index().unwrap_or(0);
(utxos, change_index)
let change_addr = account
.next_change_address(Some(&account_xpub), false)
.map_err(|e| TaskError::WalletPaymentFailed {
detail: format!("Failed to derive change address: {e}"),
})?;
(utxos, change_addr)
let utxos: Vec<_> = account.utxos.values().cloned().collect();
let change_addr = account
.next_change_address(Some(&account_xpub), true)
.map_err(|e| TaskError::WalletPaymentFailed {
detail: format!("Failed to derive change address: {e}"),
})?;
(utxos, change_addr)

source: ['claude', 'codex']

};

let wallet = wm
.get_wallet(wallet_id)
.ok_or_else(|| TaskError::WalletPaymentFailed {
detail: "Wallet object not found".to_string(),
})?;
let wallet_account = wallet
.accounts
.standard_bip44_accounts
.get(&DEFAULT_BIP44_ACCOUNT_INDEX)
.ok_or_else(|| TaskError::WalletPaymentFailed {
detail: "BIP44 wallet account missing".to_string(),
})?;
let change_addr = wallet_account
.derive_change_address(change_index)
.map_err(|e| TaskError::WalletPaymentFailed {
detail: format!("Failed to derive change address: {e}"),
})?;

loop {
let scaled_recipients: Vec<(Address, u64)> = recipients
.iter()
.map(|(addr, amt)| (addr.clone(), (*amt as f64 * scale_factor) as u64))
.collect();

let build_result = (|| -> Result<Transaction, BuilderError> {
// build_unsigned requires input >= output + fee and returns
// InsufficientFunds otherwise; the old build() let change shrink and
// could return a tx that underpaid the fee. The fallback loop below
// now drives off that earlier (correct) InsufficientFunds.
let build_result: Result<Transaction, BuilderError> = {
let mut builder = TransactionBuilder::new()
.set_fee_rate(FeeRate::normal())
.set_change_address(change_addr.clone());
.set_change_address(change_addr.clone())
.set_selection_strategy(SelectionStrategy::LargestFirst)
.set_current_height(current_height)
.add_inputs(utxos.iter().cloned());

for (addr, amt) in &scaled_recipients {
builder = builder.add_output(addr, *amt)?;
builder = builder.add_output(addr, *amt);
}

builder = builder.select_inputs(
&utxos,
SelectionStrategy::LargestFirst,
current_height,
|_| None, // No private keys for unsigned tx
)?;

builder.build()
})();
builder.build_unsigned().map(|(tx, _fee)| tx)
};

match build_result {
Ok(tx) => return Ok(tx),
Expand Down Expand Up @@ -871,30 +884,22 @@ impl AppContext {
// Build the transaction using TransactionBuilder
let mut builder = TransactionBuilder::new()
.set_fee_rate(FeeRate::normal())
.set_change_address(change_address.clone());
.set_change_address(change_address.clone())
.set_selection_strategy(SelectionStrategy::OptimalConsolidation)
.set_current_height(current_height)
.add_inputs(all_utxos);

for (address, amount) in recipients {
builder = builder
.add_output(&address, amount)
.map_err(|e: BuilderError| WalletError::TransactionBuild(e.to_string()))?;
builder = builder.add_output(&address, amount);
}

builder = builder
.select_inputs(
&all_utxos,
SelectionStrategy::OptimalConsolidation,
current_height,
|_| None, // No private keys for unsigned transaction
)
// TODO(RUST-002): String-based error classification — see #660
.map_err(|e: BuilderError| match e.to_string() {
msg if msg.contains("Insufficient") => WalletError::InsufficientFunds,
msg => WalletError::TransactionBuild(msg),
})?;

builder
.build()
.map_err(|e: BuilderError| WalletError::TransactionBuild(e.to_string()))
.build_unsigned()
.map(|(tx, _fee)| tx)
.map_err(|e: BuilderError| match e {
BuilderError::InsufficientFunds { .. } => WalletError::InsufficientFunds,
other => WalletError::TransactionBuild(other.to_string()),
})
}

fn sign_spv_transaction(
Expand Down
3 changes: 3 additions & 0 deletions src/backend_task/dashpay/contact_requests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -538,6 +538,9 @@ async fn resolve_username_to_identity(

// Use the cached DPNS contract from AppContext instead of fetching from network
let domain_query = DocumentQuery {
select: Default::default(),
group_by: vec![],
having: vec![],
data_contract: app_context.dpns_contract.clone(),
document_type_name: "domain".to_string(),
where_clauses: vec![
Expand Down
9 changes: 9 additions & 0 deletions src/backend_task/document.rs
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,9 @@ impl AppContext {
) => {
// First fetch the document to transfer
let document_query = DocumentQuery {
select: Default::default(),
group_by: vec![],
having: vec![],
data_contract: data_contract.clone(),
document_type_name: document_type.name().to_string(),
where_clauses: vec![],
Expand Down Expand Up @@ -325,6 +328,9 @@ impl AppContext {
) => {
// First fetch the document to purchase
let document_query = DocumentQuery {
select: Default::default(),
group_by: vec![],
having: vec![],
data_contract: data_contract.clone(),
document_type_name: document_type.name().to_string(),
where_clauses: vec![],
Expand Down Expand Up @@ -383,6 +389,9 @@ impl AppContext {
) => {
// First fetch the document to set price on
let document_query = DocumentQuery {
select: Default::default(),
group_by: vec![],
having: vec![],
data_contract: data_contract.clone(),
document_type_name: document_type.name().to_string(),
where_clauses: vec![],
Expand Down
1 change: 1 addition & 0 deletions src/backend_task/identity/add_key_to_identity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ impl AppContext {
sdk.version(),
None,
)
.await
.map_err(|e| TaskError::IdentityUpdateTransitionError {
source_error: Box::new(SdkError::Protocol(e)),
})?;
Expand Down
3 changes: 3 additions & 0 deletions src/backend_task/identity/discover_identities.rs
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,9 @@ impl AppContext {
use dash_sdk::platform::{Document, DocumentQuery, FetchMany};

let query = DocumentQuery {
select: Default::default(),
group_by: vec![],
having: vec![],
data_contract: self.dpns_contract.clone(),
document_type_name: "domain".to_string(),
where_clauses: vec![WhereClause {
Expand Down
3 changes: 3 additions & 0 deletions src/backend_task/identity/load_identity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,9 @@ impl AppContext {

// Fetch DPNS names using SDK
let dpns_names_document_query = DocumentQuery {
select: Default::default(),
group_by: vec![],
having: vec![],
data_contract: self.dpns_contract.clone(),
document_type_name: "domain".to_string(),
where_clauses: vec![WhereClause {
Expand Down
6 changes: 6 additions & 0 deletions src/backend_task/identity/load_identity_by_dpns_name.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ impl AppContext {

// Query the DPNS contract for the domain document
let domain_query = DocumentQuery {
select: Default::default(),
group_by: vec![],
having: vec![],
data_contract: self.dpns_contract.clone(),
document_type_name: "domain".to_string(),
where_clauses: vec![
Expand Down Expand Up @@ -73,6 +76,9 @@ impl AppContext {

// Fetch all DPNS names owned by this identity
let dpns_names_document_query = DocumentQuery {
select: Default::default(),
group_by: vec![],
having: vec![],
data_contract: self.dpns_contract.clone(),
document_type_name: "domain".to_string(),
where_clauses: vec![WhereClause {
Expand Down
3 changes: 3 additions & 0 deletions src/backend_task/identity/load_identity_from_wallet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,9 @@ impl AppContext {
let identity_id = identity.id();

let dpns_names_document_query = DocumentQuery {
select: Default::default(),
group_by: vec![],
having: vec![],
data_contract: self.dpns_contract.clone(),
document_type_name: "domain".to_string(),
where_clauses: vec![WhereClause {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ impl AppContext {
let identity_id = qualified_identity.identity.id();

let dpns_names_document_query = DocumentQuery {
select: Default::default(),
group_by: vec![],
having: vec![],
data_contract: self.dpns_contract.clone(),
document_type_name: "domain".to_string(),
where_clauses: vec![WhereClause {
Expand Down
3 changes: 3 additions & 0 deletions src/backend_task/identity/register_dpns_name.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,9 @@ impl AppContext {
.await?;

let dpns_names_document_query = DocumentQuery {
select: Default::default(),
group_by: vec![],
having: vec![],
data_contract: self.dpns_contract.clone(),
document_type_name: "domain".to_string(),
where_clauses: vec![WhereClause {
Expand Down
48 changes: 28 additions & 20 deletions src/backend_task/identity/register_identity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -463,7 +463,7 @@ impl AppContext {
qualified_identity: QualifiedIdentity,
) -> Result<Identity, TaskError> {
match identity
.put_to_platform_and_wait_for_response(
.put_to_platform_and_wait_for_response_with_private_key(
sdk,
asset_lock_proof.clone(),
asset_lock_proof_private_key,
Expand All @@ -475,38 +475,46 @@ impl AppContext {
Ok(updated_identity) => Ok(updated_identity),
Err(e) => {
if matches!(e, Error::Protocol(ProtocolError::UnknownVersionError(_))) {
identity
.put_to_platform_and_wait_for_response(
let retry_result = identity
.put_to_platform_and_wait_for_response_with_private_key(
sdk,
asset_lock_proof.clone(),
asset_lock_proof_private_key,
&qualified_identity,
None,
)
.await
.map_err(|retry_err| {
let logged = self.log_drive_proof_error(retry_err, RequestType::BroadcastStateTransition);
.await;

match retry_result {
Ok(updated_identity) => Ok(updated_identity),
Err(retry_err) => {
let logged = self.log_drive_proof_error(
retry_err,
RequestType::BroadcastStateTransition,
);
// If the logged variant is ProofError, return it directly;
// otherwise log the reconstructed transition for debugging.
if matches!(logged, TaskError::ProofError { .. }) {
return logged;
}
if let Ok(transition) = IdentityCreateTransition::try_from_identity_with_signer(
identity,
asset_lock_proof,
asset_lock_proof_private_key.inner.as_ref(),
&qualified_identity,
&NativeBlsModule,
0,
self.platform_version(),
) {
if !matches!(logged, TaskError::ProofError { .. })
&& let Ok(transition) =
IdentityCreateTransition::try_from_identity_with_signer_and_private_key(
identity,
asset_lock_proof,
asset_lock_proof_private_key.inner.as_ref(),
&qualified_identity,
&NativeBlsModule,
0,
self.platform_version(),
)
.await
{
tracing::debug!(
"Register identity retry failed; reconstructed transition: {:?}",
transition
);
}
logged
})
Err(logged)
}
}
} else {
Err(self.log_drive_proof_error(e, RequestType::BroadcastStateTransition))
}
Expand Down
Loading
Loading