Skip to content
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

use feerate instead of fee while sending a transaction #343

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

use feerate instead of fee while sending a transaction #343

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0b1819d
update spend_from_wallet to use feerate instead of fee
claddyy Jan 18, 2025
1d94e2c
Update fee if we have change output
claddyy Jan 19, 2025
dfd67f8
Check calculated vsize against vsize via API
claddyy Jan 19, 2025
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
4 changes: 3 additions & 1 deletion src/bin/taker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,8 +200,10 @@ fn main() -> Result<(), TakerError> {
let destination =
Destination::Address(Address::from_str(&address).unwrap().assume_checked());

let fee_rate = 3.0; // sats/vByte, Written as a temporary fix until issue #199 is solved.

let tx = taker.get_wallet_mut().spend_from_wallet(
fee,
fee_rate,
SendAmount::Amount(amount),
destination,
&coins_to_spend,
Expand Down
7 changes: 3 additions & 4 deletions src/maker/rpc/server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,16 +96,15 @@ fn handle_request(maker: &Arc<Maker>, socket: &mut TcpStream) -> Result<(), Make

let coins_to_send = maker.get_wallet().read()?.coin_select(amount + fee)?;

let fee_rate = 3.0; // sats/vByte, Written as a temporary fix until issue #199 is solved.

let tx = maker.get_wallet().write()?.spend_from_wallet(
fee,
fee_rate,
SendAmount::Amount(amount),
destination,
&coins_to_send,
)?;

let calculated_fee_rate = fee / (tx.weight());
log::info!("Calculated FeeRate : {:#}", calculated_fee_rate);

let txid = maker.get_wallet().read()?.send_tx(&tx)?;

RpcMsgResp::SendToAddressResp(txid.to_string())
Expand Down
109 changes: 75 additions & 34 deletions src/wallet/direct_send.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ use crate::wallet::api::UTXOSpendInfo;

use super::{error::WalletError, Wallet};

const P2PWPKH_WITNESS_SIZE: usize = 107;
const P2WSH_MULTISIG_2OF2_WITNESS_SIZE: usize = 222;

/// Represents options for specifying the amount to be sent in a transaction.
#[derive(Debug, Clone, PartialEq)]
pub enum SendAmount {
Expand Down Expand Up @@ -83,7 +86,7 @@ impl Wallet {
/// are held in a change address, if applicable.
pub fn spend_from_wallet(
&mut self,
fee: Amount,
fee_rate: f64,
send_amount: SendAmount,
destination: Destination,
coins_to_spend: &[(ListUnspentResultEntry, UTXOSpendInfo)],
Expand All @@ -102,38 +105,38 @@ impl Wallet {
};

let mut total_input_value = Amount::ZERO;
let mut total_witness_size = 0;
let mut valid_coins = Vec::new();

for (utxo_data, spend_info) in coins_to_spend {
// filter all contract and fidelity utxos.
if let UTXOSpendInfo::FidelityBondCoin { .. }
| UTXOSpendInfo::HashlockContract { .. }
| UTXOSpendInfo::TimelockContract { .. } = spend_info
{
log::warn!("Skipping Fidelity Bond or Contract UTXO.");
continue;
match spend_info {
UTXOSpendInfo::SeedCoin { .. } => {
total_witness_size += P2PWPKH_WITNESS_SIZE;
valid_coins.push((utxo_data, spend_info));
total_input_value += utxo_data.amount;
}
UTXOSpendInfo::SwapCoin { .. } => {
total_witness_size += P2WSH_MULTISIG_2OF2_WITNESS_SIZE;
valid_coins.push((utxo_data, spend_info));
total_input_value += utxo_data.amount;
}
UTXOSpendInfo::FidelityBondCoin { .. }
| UTXOSpendInfo::HashlockContract { .. }
| UTXOSpendInfo::TimelockContract { .. } => {
log::warn!("Skipping Fidelity Bond or Contract UTXO: {:?}", spend_info);
continue;
}
Comment on lines 111 to +128
Copy link

Choose a reason for hiding this comment

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

I think this logic can be moved to the call sites, or better, create wrapper APIs on top of a common send API. Weather a utxo is valid for spend should be determined at the call site.

For ex: if you wanna spend only fidelity bonds in API like spend_fildiety() you filter out the fidelity utxo, and then feed to the common spend_coins() API.

This will also allow us to use a common logic for all kind of spendings and use that everywhere, instead of current individual design of each spend functions. This refactor is much needed.

}
}

for (utxo_data, _) in &valid_coins {
tx.input.push(TxIn {
previous_output: OutPoint::new(utxo_data.txid, utxo_data.vout),
sequence: Sequence::ZERO,
witness: Witness::new(),
script_sig: ScriptBuf::new(),
});

total_input_value += utxo_data.amount;
}

if let SendAmount::Amount(a) = send_amount {
if a + fee > total_input_value {
return Err(WalletError::InsufficientFund {
available: total_input_value.to_btc(),
required: (a + fee).to_btc(),
});
}
}

log::info!("Total Input Amount: {} | Fees: {}", total_input_value, fee);

let dest_addr = match destination {
Destination::Wallet => self.get_next_internal_addresses(1)?[0].clone(),
Destination::Address(a) => {
Expand All @@ -154,31 +157,61 @@ impl Wallet {
}
};

let txout = {
let value = match send_amount {
SendAmount::Max => total_input_value - fee,
let txout = TxOut {
script_pubkey: dest_addr.script_pubkey(),
value: Amount::ZERO, //Temporary value
Copy link

Choose a reason for hiding this comment

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

This doesn't have to be 0. You can set it to the actual value needed here only.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm validating send_amount against fee parameters later, that's why I kept it 0, and the value is added later. Does keeping it this way makes more sense, or shall I change it?

Copy link

Choose a reason for hiding this comment

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

Change it, please. There is no ambiguity about the send amount. Just set the value here.

This approach makes sense for change amount where you don't know the value before hand.

Copy link

Choose a reason for hiding this comment

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

Ah I see, you need this to handle the max spend case. Cool then it can stay.

Copy link

Choose a reason for hiding this comment

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

On a second thought. Lets remove this SendAmount::Max thing. It's not working as expected. It will only create a non-change spend from with the given inputs. That doesn't mean thats the max wallet amount. The current design is not ideal too. Specifying Max and selected coin list in the input together is redundant and circular. Makes it almost unusable for the sweeping purpose.

Instead there should a be dedicated API called seep_wallet(dest_addr), which doesn't need any other input from the user, finds the total spendable utxos, create one tx with no change to sweep the whole wallet balance to dest-addr. This can be done in a followup PR. For now we can simplify the spend logic much more if we remove the Max enum.

Copy link

@mojoX911 mojoX911 Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

One good way of doing it would be to have a SpendType enum.

pun enum SpendType {
    Amount(u64),
    NoChange
}

When NoChange is given, it will create a change less tx, sending the whole total_input - fee to the dest_addr. This will be useful for fidelity, and contract spendings.

};

tx.output.push(txout);

let base_size = tx.base_size();
let mut vsize = (base_size * 4 + total_witness_size) / 4;
let fee = Amount::from_sat((fee_rate * vsize as f64).ceil() as u64);
log::info!("Total Input Amount: {} | Fees: {}", total_input_value, fee);

SendAmount::Amount(a) => a,
};
log::info!("Sending {} to {}.", value, dest_addr);
TxOut {
script_pubkey: dest_addr.script_pubkey(),
value,
if let SendAmount::Amount(a) = send_amount {
if a + fee > total_input_value {
return Err(WalletError::InsufficientFund {
available: total_input_value.to_btc(),
required: (a + fee).to_btc(),
});
}
}

let value = match send_amount {
SendAmount::Max => total_input_value - fee,
SendAmount::Amount(a) => a,
};

tx.output.push(txout);
tx.output[0].value = value;
log::info!("Sending {} to {}", value, dest_addr);

// Only include change if remaining > dust
if let SendAmount::Amount(amount) = send_amount {
mojoX911 marked this conversation as resolved.
Show resolved Hide resolved
let internal_spk = self.get_next_internal_addresses(1)?[0].script_pubkey();
let remaining = total_input_value - amount - fee;
if remaining > internal_spk.minimal_non_dust() {
let minimal_nondust = internal_spk.minimal_non_dust();
let mut remaining = total_input_value - amount - fee;
if remaining > minimal_nondust {
log::info!("Adding Change {}: {}", internal_spk, remaining);
tx.output.push(TxOut {
script_pubkey: internal_spk,
value: remaining,
});

let base_wchange = tx.base_size();
vsize = (base_wchange * 4 + total_witness_size) / 4;
let fee_wchange = Amount::from_sat((fee_rate * vsize as f64).ceil() as u64);

remaining = total_input_value - amount - fee_wchange;

if remaining > minimal_nondust {
tx.output[1].value = remaining;
}
log::info!(
"Adding change output with {} sats (fee: {})",
remaining,
fee_wchange
);
Comment on lines +207 to +214
Copy link

Choose a reason for hiding this comment

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

There is one edge case to consider here. because feewchange will be higher than the previous calculated fee, if the remaining is close to dust limit, this updated remaining value may be below dust after recalculation. Which will make the added change below dust limit.

This can be handled by creating an hypothetical remianing_with_change value first, and then check if that is less than dust. Only add change out if true, else carry on with the non-change calculation.

} else {
log::info!(
"Remaining change {} sats is below dust threshold. Skipping change output.",
Expand All @@ -191,6 +224,14 @@ impl Wallet {
&mut tx,
&mut coins_to_spend.iter().map(|(_, usi)| usi.clone()),
)?;

let signed_tx_vsize = tx.vsize();
assert_eq!(
signed_tx_vsize, vsize,
"Calculated vsize {} didn't match signed tx vsize {}",
signed_tx_vsize, vsize
);

log::debug!("Signed Transaction : {:?}", tx.raw_hex());
Ok(tx)
}
Expand Down
Loading