Skip to content

fix: apply BIP-69 sorting to asset lock credit outputs #622

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
QuantumExplorer wants to merge 1 commit into
base: v0.42-dev
Choose a base branch
from
Draft

fix: apply BIP-69 sorting to asset lock credit outputs #622

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 13 additions & 3 deletions key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,8 @@ impl ManagedWalletInfo {
let fee = tx_builder_with_inputs.calculate_fee();
let fee_with_extra = tx_builder_with_inputs.calculate_fee_with_extra_output();

let transaction = tx_builder_with_inputs.build_asset_lock(credit_outputs)?;
let (transaction, sorted_indices) =
tx_builder_with_inputs.build_asset_lock(credit_outputs)?;

let actual_fee = if transaction.output.len() > outputs_count_before {
fee_with_extra
Expand All @@ -250,7 +251,9 @@ impl ManagedWalletInfo {
};

// Transaction built successfully — now derive keys.
let mut keys = Vec::with_capacity(credit_output_fundings.len());
// Keys are derived in the original funding order, then reordered
// to match the BIP-69 sorted credit output positions in the payload.
let mut original_keys = Vec::with_capacity(credit_output_fundings.len());
for funding in &credit_output_fundings {
let funding_key_account = resolve_funding_account(
&mut self.accounts,
Expand All @@ -260,7 +263,14 @@ impl ManagedWalletInfo {
let key = funding_key_account
.next_private_key(root_xpriv, network)
.map_err(|e| AssetLockError::KeyDerivation(e.to_string()))?;
keys.push(key);
original_keys.push(key);
}

// Reorder keys to match sorted output positions:
// sorted_indices[i] = original index of the output now at position i
let mut keys = vec![[0u8; 32]; original_keys.len()];
for (sorted_pos, &orig_idx) in sorted_indices.iter().enumerate() {
keys[sorted_pos] = original_keys[orig_idx];
}

Ok(AssetLockResult {
Expand Down
29 changes: 25 additions & 4 deletions key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -629,13 +629,34 @@ impl TransactionBuilder {

/// Build an Asset Lock Transaction
///
/// Used to lock Dash for use in Platform (creates Platform credits)
pub fn build_asset_lock(self, credit_outputs: Vec<TxOut>) -> Result<Transaction, BuilderError> {
/// Used to lock Dash for use in Platform (creates Platform credits).
/// Credit outputs in the payload are sorted by BIP-69 (amount ascending,
/// then scriptPubKey lexicographically) for deterministic ordering.
/// Returns `(Transaction, sorted_indices)` where `sorted_indices[i]` is
/// the original index of the credit output now at position `i` in the payload.
pub fn build_asset_lock(
self,
credit_outputs: Vec<TxOut>,
) -> Result<(Transaction, Vec<usize>), BuilderError> {
// Track original indices so callers can re-map keys
let mut indexed: Vec<(usize, TxOut)> = credit_outputs.into_iter().enumerate().collect();

// BIP-69: sort by amount ascending, then scriptPubKey lexicographically
indexed.sort_by(|(_, a), (_, b)| match a.value.cmp(&b.value) {
std::cmp::Ordering::Equal => a.script_pubkey.as_bytes().cmp(b.script_pubkey.as_bytes()),
other => other,
});

let sorted_indices: Vec<usize> = indexed.iter().map(|(orig, _)| *orig).collect();
let sorted_outputs: Vec<TxOut> = indexed.into_iter().map(|(_, out)| out).collect();

let payload = AssetLockPayload {
version: 0,
credit_outputs,
credit_outputs: sorted_outputs,
};
self.set_special_payload(TransactionPayload::AssetLockPayloadType(payload)).build()
let tx =
self.set_special_payload(TransactionPayload::AssetLockPayloadType(payload)).build()?;
Ok((tx, sorted_indices))
Comment on lines +637 to +659
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 2, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

This still hides asset-lock payload bytes from select_inputs().

special_payload is only created here, but key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs, Lines 227-245, call select_inputs() and calculate_fee*() before invoking build_asset_lock(). Because the builder only counts payload bytes once self.special_payload is set, asset-lock builds can still underselect near the margin and return a fee that excludes the payload. Please expose the sorted payload/indices before selection, or add a preparatory API that lets callers install the payload earlier.

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

In `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs` around
lines 637 - 659, The build_asset_lock() currently sets self.special_payload only
at the final build step, so callers like asset_lock_builder::select_inputs() and
calculate_fee*() never see the payload bytes and can underselect; fix this by
exposing the sorted AssetLockPayload and indices prior to input selection—either
(A) add a preparatory method on the builder (e.g.,
prepare_asset_lock_payload(&mut self, credit_outputs: Vec<TxOut>) ->
Result<Vec<usize>, BuilderError>) that performs the BIP-69 sorting, installs
self.special_payload (or otherwise stores the payload) and returns
sorted_indices so select_inputs()/calculate_fee*() can include payload size, or
(B) change build_asset_lock() to factor out the sorting into a public helper
(e.g., sort_asset_lock_outputs(credit_outputs) -> (AssetLockPayload,
Vec<usize>)) that asset_lock_builder can call before select_inputs(); update
callers in asset_lock_builder.rs (the code around lines 227-245) to call the new
preparatory API so payload bytes are counted during fee/input selection.

}

/// Estimate transaction size in bytes
Expand Down
Loading