From bfb02ad7c1a50910a445bf89f583d54f0533661c Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Sun, 12 Jul 2026 17:38:10 +0700 Subject: [PATCH 1/2] fix(key-wallet): return the fee the tx actually pays from build/build_signed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TransactionBuilder::build_unsigned()/build_signed() returned a fee computed from the encoded size of the built transaction. When the builder drops a dust change remainder (0 < change <= 546 duffs), those duffs go to miners, so the real fee is the size fee plus the dust — the returned value under-reported by up to 546 duffs exactly when the fee got larger. Compute the returned fee as sum(selected input values) - sum(output values) instead, which is what the transaction actually pays in every case (normal change, exact change, dust drop, and drain). Remove the now-unused encoded_size helper and its error paths, and update the doc comments. Fixes #871 Co-Authored-By: Claude Fable 5 --- .../transaction_builder.rs | 76 ++++++++++--------- 1 file changed, 41 insertions(+), 35 deletions(-) diff --git a/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs b/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs index 2ed56efb3..219694a9e 100644 --- a/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs +++ b/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs @@ -12,7 +12,6 @@ use core::fmt; use dashcore::blockdata::script::{Builder, PushBytes, ScriptBuf}; use dashcore::blockdata::transaction::special_transaction::TransactionPayload; use dashcore::blockdata::transaction::{OutPoint, Transaction}; -use dashcore::consensus::Encodable; use dashcore::sighash::{EcdsaSighashType, LegacySighash, SighashCache}; use dashcore::Address; use dashcore::{TxIn, TxOut}; @@ -25,15 +24,6 @@ use std::cmp::Ordering; /// bytes/signed input) and be rejected by the network const MAX_STANDARD_TX_INPUTS: usize = 500; -/// Consensus-encoded byte length of `tx`, used to compute the fee from the real -/// serialized size. Surfaces an encode error instead of panicking so the caller -/// can release any reservations it took rather than stranding the inputs. -fn encoded_size(tx: &Transaction) -> Result { - let mut bytes = Vec::new(); - tx.consensus_encode(&mut bytes) - .map_err(|err| BuilderError::InvalidData(format!("failed to encode transaction: {}", err))) -} - /// Calculate varint size for a given number fn varint_size(n: usize) -> usize { match n { @@ -432,28 +422,25 @@ impl TransactionBuilder { } } + /// Build the unsigned transaction. The returned fee is the fee the + /// transaction actually pays: Σ(selected input values) − Σ(output values). + /// This can exceed the size-based fee target when a dust change remainder + /// (≤ 546 duffs) is dropped and left to miners. pub fn build_unsigned(self) -> Result<(Transaction, u64), BuilderError> { - let fee_rate = self.fee_rate; - let reservations = self.reservations.clone(); - let (tx, inputs) = self.assemble_unsigned()?; - let fee = match encoded_size(&tx) { - Ok(size) => fee_rate.calculate_fee(size), - Err(err) => { - if let Some(reservations) = &reservations { - reservations.release(inputs.iter().map(|utxo| &utxo.outpoint)); - } - return Err(err); - } - }; + let total_input: u64 = inputs.iter().map(|utxo| utxo.value()).sum(); + let total_output: u64 = tx.output.iter().map(|out| out.value).sum(); - Ok((tx, fee)) + Ok((tx, total_input.saturating_sub(total_output))) } /// Build and sign the transaction. The `path_resolver` maps each input /// address to the derivation path the signer should use for that input. - /// The returned fee is computed from the encoded size of the signed tx. + /// The returned fee is the fee the transaction actually pays: + /// Σ(selected input values) − Σ(output values). This can exceed the + /// size-based fee target when a dust change remainder (≤ 546 duffs) is + /// dropped and left to miners. pub async fn build_signed( self, signer: &S, @@ -463,10 +450,10 @@ impl TransactionBuilder { S: TransactionSigner + ?Sized + Sync, P: Fn(Address) -> Option + Send, { - let fee_rate = self.fee_rate; let reservations = self.reservations.clone(); let (tx, inputs) = self.assemble_unsigned()?; + let total_input: u64 = inputs.iter().map(|utxo| utxo.value()).sum(); // Signing never reaches the network for a local key, but an external // signer can fail. A failed sign means the reserved inputs are still // spendable, so release them now instead of stranding the funds until @@ -482,17 +469,9 @@ impl TransactionBuilder { } }; - let fee = match encoded_size(&tx) { - Ok(size) => fee_rate.calculate_fee(size), - Err(err) => { - if let Some(reservations) = &reservations { - reservations.release(reserved.iter()); - } - return Err(err); - } - }; + let total_output: u64 = tx.output.iter().map(|out| out.value).sum(); - Ok((tx, fee)) + Ok((tx, total_input.saturating_sub(total_output))) } } @@ -852,6 +831,33 @@ mod tests { assert_eq!(tx.output[0].value, 150000); } + /// When the change remainder is dust (≤ 546 duffs) the builder drops it and + /// those duffs go to miners. The returned fee must be what the transaction + /// actually pays (Σ inputs − Σ outputs), not the smaller size-based target. + #[test] + fn test_dropped_dust_change_counts_toward_returned_fee() { + // 150000 to recipient + 226 size fee + 300 dust remainder + let utxos = vec![Utxo::dummy(0, 150526, 100, false, true)]; + + let recipient_address = Address::dummy(Network::Testnet, 0); + let change_address = Address::dummy(Network::Testnet, 0); + + let (tx, fee) = TransactionBuilder::new() + .set_current_height(200) + .set_selection_strategy(SelectionStrategy::SmallestFirst) + .set_fee_rate(FeeRate::normal()) + .set_change_address(change_address) + .add_inputs(utxos) + .add_output(&recipient_address, 150000) + .build_unsigned() + .unwrap(); + + assert_eq!(tx.output.len(), 1, "dust change must be dropped"); + let total_output: u64 = tx.output.iter().map(|o| o.value).sum(); + assert_eq!(fee, 150526 - total_output, "fee must equal inputs minus outputs"); + assert_eq!(fee, 526, "fee must include the 300-duff dropped dust remainder"); + } + #[test] fn test_special_payload_size_calculations() { // Test that special payload sizes are calculated correctly From 2e4c1c40ccbbc933d69dc5587ed52b5098c37a53 Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Sun, 12 Jul 2026 17:51:32 +0700 Subject: [PATCH 2/2] =?UTF-8?q?test(key-wallet):=20pin=20asset-lock=20fee?= =?UTF-8?q?=20semantics=20=E2=80=94=20locked=20credits=20are=20not=20fee?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An asset lock burns the locked amount into an on-chain OP_RETURN output mirroring the payload's credit outputs, so it is part of the output sum and inputs - outputs yields the miner fee only. Add a regression test proving the returned fee excludes the locked credits. Co-Authored-By: Claude Fable 5 --- .../transaction_builder.rs | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs b/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs index 219694a9e..b2afbefe7 100644 --- a/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs +++ b/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs @@ -858,6 +858,42 @@ mod tests { assert_eq!(fee, 526, "fee must include the 300-duff dropped dust remainder"); } + /// An asset lock burns the locked amount into an on-chain OP_RETURN output + /// mirroring the payload's credit outputs. That output is part of + /// Σ(outputs), so the returned fee must be the miner fee only — the locked + /// credits must never be counted as fee. + #[test] + fn test_asset_lock_fee_excludes_locked_credits() { + let utxos = vec![Utxo::dummy(0, 1_000_000, 100, false, true)]; + let change_address = Address::dummy(Network::Testnet, 0); + + let asset_lock_payload = AssetLockPayload { + version: 1, + credit_outputs: vec![TxOut { + value: 100_000, + script_pubkey: ScriptBuf::new(), + }], + }; + + let (tx, fee) = TransactionBuilder::new() + .set_current_height(200) + .set_fee_rate(FeeRate::normal()) + .set_change_address(change_address) + .set_special_payload(TransactionPayload::AssetLockPayloadType(asset_lock_payload)) + .add_inputs(utxos) + .build_unsigned() + .unwrap(); + + assert_eq!(tx.output.len(), 2, "OP_RETURN burn output + change"); + let total_output: u64 = tx.output.iter().map(|o| o.value).sum(); + assert_eq!(fee, 1_000_000 - total_output, "fee must equal inputs minus outputs"); + assert!( + fee < 1_000, + "fee must be the miner fee only, not include the 100k locked credits, got {}", + fee + ); + } + #[test] fn test_special_payload_size_calculations() { // Test that special payload sizes are calculated correctly