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.

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

commands: include unconfirmed change as coin selection candidates #873

Merged
Prev Previous commit
Next Next commit
spend: increase candidate weight to pay for ancestor
jp1ac4 committed Jan 25, 2024

Verified

This commit was signed with the committer’s verified signature.
jp1ac4 Michael Mallan
commit 94ef66c03a17221cc9cddb8d1d463a46f0b489c9
23 changes: 19 additions & 4 deletions src/commands/mod.rs
Original file line number Diff line number Diff line change
@@ -9,7 +9,7 @@ use crate::{
database::{Coin, DatabaseConnection, DatabaseInterface},
descriptors,
spend::{
create_spend, AddrInfo, CandidateCoin, CreateSpendRes, SpendCreationError,
create_spend, AddrInfo, AncestorInfo, CandidateCoin, CreateSpendRes, SpendCreationError,
SpendOutputAddress, SpendTxFees, TxGetter,
},
DaemonControl, VERSION,
@@ -179,6 +179,7 @@ fn coin_to_candidate(
coin: &Coin,
must_select: bool,
sequence: Option<bitcoin::Sequence>,
ancestor_info: Option<AncestorInfo>,
) -> CandidateCoin {
CandidateCoin {
outpoint: coin.outpoint,
@@ -187,6 +188,7 @@ fn coin_to_candidate(
is_change: coin.is_change,
must_select,
sequence,
ancestor_info,
}
}

@@ -466,7 +468,10 @@ impl DaemonControl {
.coins(&[CoinStatus::Confirmed], &[])
.into_values()
.map(|c| {
coin_to_candidate(&c, /*must_select=*/ false, /*sequence=*/ None)
coin_to_candidate(
&c, /*must_select=*/ false, /*sequence=*/ None,
/*ancestor_info=*/ None,
)
})
.collect()
} else {
@@ -483,7 +488,12 @@ impl DaemonControl {
}
coins
.into_values()
.map(|c| coin_to_candidate(&c, /*must_select=*/ true, /*sequence=*/ None))
.map(|c| {
coin_to_candidate(
&c, /*must_select=*/ true, /*sequence=*/ None,
/*ancestor_info=*/ None,
)
})
.collect()
};

@@ -795,7 +805,10 @@ impl DaemonControl {
let mut candidate_coins: Vec<CandidateCoin> = prev_coins
.values()
.map(|c| {
coin_to_candidate(c, /*must_select=*/ !is_cancel, /*sequence=*/ None)
coin_to_candidate(
c, /*must_select=*/ !is_cancel, /*sequence=*/ None,
/*ancestor_info=*/ None,
)
})
.collect();
let confirmed_cands: Vec<CandidateCoin> = db_conn
@@ -807,6 +820,7 @@ impl DaemonControl {
if !prev_coins.contains_key(&c.outpoint) {
Some(coin_to_candidate(
&c, /*must_select=*/ false, /*sequence=*/ None,
/*ancestor_info=*/ None,
))
} else {
None
@@ -996,6 +1010,7 @@ impl DaemonControl {
&c,
/*must_select=*/ true,
/*sequence=*/ Some(bitcoin::Sequence::from_height(timelock)),
/*ancestor_info=*/ None,
))
} else {
None
61 changes: 59 additions & 2 deletions src/spend.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::descriptors;
use crate::{bitcoin::MempoolEntry, descriptors};

use std::{collections::BTreeMap, convert::TryInto, fmt};

@@ -11,6 +11,7 @@ use miniscript::bitcoin::{
self,
absolute::{Height, LockTime},
bip32,
constants::WITNESS_SCALE_FACTOR,
psbt::{Input as PsbtIn, Output as PsbtOut, Psbt},
secp256k1,
};
@@ -154,6 +155,29 @@ fn sanity_check_psbt(
Ok(())
}

#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
pub struct AncestorInfo {
pub vsize: u32,
pub fee: u32,
}

impl From<MempoolEntry> for AncestorInfo {
fn from(entry: MempoolEntry) -> Self {
Self {
vsize: entry
.ancestor_vsize
.try_into()
.expect("vsize must fit in a u32"),
fee: entry
.fees
.ancestor
.to_sat()
.try_into()
.expect("fee in sats should fit in a u32"),
}
}
}

/// A candidate for coin selection when creating a transaction.
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
pub struct CandidateCoin {
@@ -169,6 +193,8 @@ pub struct CandidateCoin {
pub must_select: bool,
/// The nSequence field to set for an input spending this coin.
pub sequence: Option<bitcoin::Sequence>,
/// Information about in-mempool ancestors of the coin.
pub ancestor_info: Option<AncestorInfo>,
}

/// A coin selection result.
@@ -291,12 +317,43 @@ fn select_coins_for_spend(
base_weight = base_weight.saturating_sub(2);
}
let max_input_weight = TXIN_BASE_WEIGHT + max_sat_weight;
// Get feerate as u32 for calculation relating to ancestor below.
// We expect `feerate_vb` to be a positive integer, but take ceil()
// just in case to be sure we pay enough for ancestors.
let feerate_vb_u32 = feerate_vb.ceil() as u32;
Copy link
Member

Choose a reason for hiding this comment

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

It's preferable to explicitly panic if the feerate is outside the range of u32 than to potentially let an(other) insane value in.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's true, I could have manually checked the value, perhaps with feerate_vb.ceil() > u32::MAX as f32.
I was initially looking for something like try_into, but couldn't find anything for float to int.

Copy link
Member

Choose a reason for hiding this comment

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

Oh that's unfortunate that there is no try_into and we should implement those check manually.

let witness_factor: u32 = WITNESS_SCALE_FACTOR
.try_into()
.expect("scale factor must fit in u32");
Comment on lines +330 to +332
Copy link
Member

Choose a reason for hiding this comment

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

nit: On the other hand, since that's a constant an as cast here is fine i think.

let candidates: Vec<Candidate> = candidate_coins
.iter()
.map(|cand| Candidate {
input_count: 1,
value: cand.amount.to_sat(),
weight: max_input_weight,
weight: {
Copy link
Member

Choose a reason for hiding this comment

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

nit: i'd rather this calculation was done outside of the initialization of the Candidate struct. I find the code easier to follow if we compute everything we need and only then proceed to initialize the struct.

let extra = cand
.ancestor_info
.map(|info| {
// The implied ancestor vsize if the fee had been paid at our target feerate.
let ancestor_vsize_at_feerate: u32 = info
.fee
.checked_div(feerate_vb_u32)
.expect("feerate is greater than zero");
// If the actual ancestor vsize is bigger than the implied vsize, we will need to
// pay the difference in order for the combined feerate to be at the target value.
// We multiply the vsize by 4 to get the ancestor weight, which is an upper bound
// on its true weight (vsize*4 - 3 <= weight <= vsize*4), to ensure we pay enough.
// Note that if candidates share ancestors, we may add this difference more than
// once in the resulting transaction.
info.vsize
.saturating_sub(ancestor_vsize_at_feerate)
.checked_mul(witness_factor)
.expect("weight difference must fit in u32")
})
.unwrap_or(0);
Comment on lines +341 to +360
Copy link
Member

Choose a reason for hiding this comment

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

I stared at this for a while. Maybe we are thinking about this differently (which is interesting), but don't you think it's more obvious to just calculate the delta between the ancestor and target feerates instead?

diff --git a/src/spend.rs b/src/spend.rs
index d545375..72f3aeb 100644
--- a/src/spend.rs
+++ b/src/spend.rs
@@ -341,21 +341,22 @@ fn select_coins_for_spend(
                 let extra = cand
                     .ancestor_info
                     .map(|info| {
-                        // The implied ancestor vsize if the fee had been paid at our target feerate.
-                        let ancestor_vsize_at_feerate: u32 = info
+                        // The additional weight carried by this candidate is the size of its
+                        // unconfirmed ancestors minus the fees paid by this ancestor.
+                        // Calculate the delta between the ancestor's feerate and our target
+                        // feerate to compute the extra weight, if any.
+                        let ancestor_feerate = info
                             .fee
-                            .checked_div(feerate_vb_u32)
-                            .expect("feerate is greater than zero");
-                        // If the actual ancestor vsize is bigger than the implied vsize, we will need to
-                        // pay the difference in order for the combined feerate to be at the target value.
-                        // We multiply the vsize by 4 to get the ancestor weight, which is an upper bound
-                        // on its true weight (vsize*4 - 3 <= weight <= vsize*4), to ensure we pay enough.
-                        // Note that if candidates share ancestors, we may add this difference more than
-                        // once in the resulting transaction.
-                        info.vsize
-                            .saturating_sub(ancestor_vsize_at_feerate)
+                            .checked_div(info.vsize)
+                            .expect("Can't have a null vsize");
+                        let extra_feerate = feerate_vb_u32.saturating_sub(ancestor_feerate);
+                        let ancestor_weight = info
+                            .vsize
                             .checked_mul(witness_factor)
-                            .expect("weight difference must fit in u32")
+                            .expect("Weight must fit in a u32");
+                        extra_feerate
+                            .checked_mul(ancestor_weight)
+                            .expect("Must stay within the u32 bounds")
                     })
                     .unwrap_or(0);
                 // Store the extra weight for this candidate for use later on.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's true, I think it can also be thought of as the extra feerate for the ancestor component, which could be more intuitive.

Note that in your suggested change, extra_feerate.checked_mul(ancestor_weight) gives the extra fee that needs to be paid for the ancestor, and dividing by feerate_vb to get the extra weight still gives the same value of ancestor_size - (ancestor_fee / feerate).

Copy link
Member

Choose a reason for hiding this comment

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

Yes i didn't mean to change the semantic, just to make it clearer.

max_input_weight
.checked_add(extra)
.expect("effective weight must fit in u32")
},
is_segwit: true, // We only support receiving on Segwit scripts.
})
.collect();