Skip to content

Commit

Permalink
Fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
@wvanlint
wvanlint committed Sep 13, 2024
1 parent e9919a3 commit c9b6f6d
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 65 deletions.
9 changes: 6 additions & 3 deletions lightning/src/chain/onchaintx.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -772,8 +772,11 @@ impl<ChannelSigner: EcdsaChannelSigner> OnchainTxHandler<ChannelSigner> {
for j in 0..i {
if requests[i].can_merge_with(&requests[j], cur_height) {
let merge = requests.remove(i);
requests[j].merge_package(merge);
break;
if let Err(rejected) = requests[j].merge_package(merge, cur_height) {
requests.insert(i, rejected);
} else {
break;
}
}
}
}
Expand Down Expand Up @@ -1101,7 +1104,7 @@ impl<ChannelSigner: EcdsaChannelSigner> OnchainTxHandler<ChannelSigner> {
OnchainEvent::ContentiousOutpoint { package } => {
if let Some(pending_claim) = self.claimable_outpoints.get(package.outpoints()[0]) {
if let Some(request) = self.pending_claim_requests.get_mut(&pending_claim.0) {
request.merge_package(package);
assert!(request.merge_package(package, height).is_ok());
// Using a HashMap guarantee us than if we have multiple outpoints getting
// resurrected only one bump claim tx is going to be broadcast
bump_candidates.insert(pending_claim.clone(), request.clone());
Expand Down
108 changes: 46 additions & 62 deletions lightning/src/chain/package.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -662,33 +662,24 @@ impl PackageSolvingData {
_ => { panic!("API Error!"); }
}
}
fn absolute_tx_timelock(&self, current_height: u32) -> u32 {
// We use `current_height` as our default locktime to discourage fee sniping and because
// transactions with it always propagate.
let absolute_timelock = match self {
PackageSolvingData::RevokedOutput(_) => current_height,
PackageSolvingData::RevokedHTLCOutput(_) => current_height,
PackageSolvingData::CounterpartyOfferedHTLCOutput(_) => current_height,
PackageSolvingData::CounterpartyReceivedHTLCOutput(ref outp) => cmp::max(outp.htlc.cltv_expiry, current_height),
// HTLC timeout/success transactions rely on a fixed timelock due to the counterparty's
// signature.
PackageSolvingData::HolderHTLCOutput(ref outp) => {
if outp.preimage.is_some() {
debug_assert_eq!(outp.cltv_expiry, 0);
}
outp.cltv_expiry
},
PackageSolvingData::HolderFundingOutput(_) => current_height,
};
absolute_timelock
/// Some output types are locked with CHECKLOCKTIMEVERIFY and the spending transaction must
/// have a minimum locktime, which is returned here.
fn minimum_locktime(&self) -> Option<u32> {
match self {
PackageSolvingData::CounterpartyReceivedHTLCOutput(ref outp) => Some(outp.htlc.cltv_expiry),
_ => None,
}
}
/// Some output types are pre-signed in such a way that the spending transaction must have an
/// exact locktime.
/// This returns that locktime for such outputs.
fn signed_locktime(&self) -> Option<u32> {
match self {
PackageSolvingData::HolderHTLCOutput(_) => {
Some(self.absolute_tx_timelock(0))
PackageSolvingData::HolderHTLCOutput(ref outp) => {
if outp.preimage.is_some() {
debug_assert_eq!(outp.cltv_expiry, 0);
}
Some(outp.cltv_expiry)
},
_ => None,
}
Expand All @@ -704,7 +695,7 @@ impl PackageSolvingData {
PackageSolvingData::RevokedOutput(RevokedOutput { .. }) =>
PackageMalleability::Malleable(AggregationCluster::Unpinnable),
PackageSolvingData::RevokedHTLCOutput(..) =>
PackageMalleability::Malleable(AggregationCluster::Pinnable),
PackageMalleability::Malleable(AggregationCluster::Unpinnable),
PackageSolvingData::CounterpartyOfferedHTLCOutput(..) =>
PackageMalleability::Malleable(AggregationCluster::Unpinnable),
PackageSolvingData::CounterpartyReceivedHTLCOutput(..) =>
Expand Down Expand Up @@ -803,8 +794,8 @@ impl PackageTemplate {
// First check the types of the inputs and only merge if they are possibly claiming
// at the same time.
// This shouldn't ever happen, but if we do end up with packages trying to claim
// funds from two different commitment transactions (which cannot possibly be
// on-chain at the same time) we definitely shouldn't merge them.
// funds from two different commitment transactions for the same channel,
// (which cannot possibly be on-chain at the same time) we definitely shouldn't merge them.
#[cfg(debug_assertions)] {
for i in 0..self.inputs.len() {
for j in 0..i {
Expand All @@ -822,30 +813,34 @@ impl PackageTemplate {
return false;
}

// First, check if the two packages have compatible locktimes.
// We only want to aggregate claims if they have the same locktime.
// Check if the packages have signed locktimes. If they do, we only want to aggregate
// packages with the same, signed locktime.
if self.signed_locktime() != other.signed_locktime() {
return false;
}
// Check if the two packages have compatible minimum locktimes.
if self.package_locktime(cur_height) != other.package_locktime(cur_height) {
return false;
}

// Now check that we only merge packages if we they are both unpinnable or both
// pinnable
let self_pinnable = self_cluster == AggregationCluster::Pinnable ||
self.counterparty_spendable_height() >= cur_height;
self.counterparty_spendable_height() <= cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE;
let other_pinnable = other_cluster == AggregationCluster::Pinnable ||
other.counterparty_spendable_height() >= cur_height;
other.counterparty_spendable_height() <= cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE;
if self_pinnable && other_pinnable {
return true;
}

let self_unpinnable = self_cluster == AggregationCluster::Unpinnable &&
self.counterparty_spendable_height() < cur_height - COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE;
self.counterparty_spendable_height() > cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE;
let other_unpinnable = self_cluster == AggregationCluster::Unpinnable &&
other.counterparty_spendable_height() < cur_height - COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE;
other.counterparty_spendable_height() > cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE;
if self_unpinnable && other_unpinnable {
return true;
}
self_cluster == other_cluster && self.counterparty_spendable_height() == other.counterparty_spendable_height()
false
},
}
}
Expand Down Expand Up @@ -909,8 +904,10 @@ impl PackageTemplate {
}
}
}
pub(crate) fn merge_package(&mut self, mut merge_from: PackageTemplate) {
assert!(self.can_merge_with(&merge_from, 0));
pub(crate) fn merge_package(&mut self, mut merge_from: PackageTemplate, cur_height: u32) -> Result<(), PackageTemplate> {
if !self.can_merge_with(&merge_from, cur_height) {
return Err(merge_from);
}
for (k, v) in merge_from.inputs.drain(..) {
self.inputs.push((k, v));
}
Expand All @@ -922,6 +919,7 @@ impl PackageTemplate {
self.feerate_previous = merge_from.feerate_previous;
}
self.height_timer = cmp::min(self.height_timer, merge_from.height_timer);
Ok(())
}
/// Gets the amount of all outptus being spent by this package, only valid for malleable
/// packages.
Expand All @@ -932,36 +930,22 @@ impl PackageTemplate {
}
amounts
}
fn signed_locktime(&self) -> Option<u32> {
let signed_locktime = self.inputs.iter().find_map(|(_, outp)| outp.signed_locktime());
#[cfg(debug_assertions)]
for (_, outp) in self.inputs.iter() {
debug_assert!(outp.signed_locktime().is_none() || outp.signed_locktime() == signed_locktime);
}
signed_locktime
}
pub(crate) fn package_locktime(&self, current_height: u32) -> u32 {
let locktime = self.inputs.iter().map(|(_, outp)| outp.absolute_tx_timelock(current_height))
.max().expect("There must always be at least one output to spend in a PackageTemplate");

// If we ever try to aggregate a `HolderHTLCOutput`s with another output type, we'll likely
// end up with an incorrect transaction locktime since the counterparty has included it in
// its HTLC signature. This should never happen unless we decide to aggregate outputs across
// different channel commitments.
#[cfg(debug_assertions)] {
if self.inputs.iter().any(|(_, outp)|
if let PackageSolvingData::HolderHTLCOutput(outp) = outp {
outp.preimage.is_some()
} else {
false
}
) {
debug_assert_eq!(locktime, 0);
};
for timeout_htlc_expiry in self.inputs.iter().filter_map(|(_, outp)|
if let PackageSolvingData::HolderHTLCOutput(outp) = outp {
if outp.preimage.is_none() {
Some(outp.cltv_expiry)
} else { None }
} else { None }
) {
debug_assert_eq!(locktime, timeout_htlc_expiry);
}
let minimum_locktime = self.inputs.iter().filter_map(|(_, outp)| outp.minimum_locktime()).max().unwrap_or(0);
if let Some(signed_locktime) = self.signed_locktime() {
assert!(signed_locktime >= minimum_locktime);
signed_locktime
} else {
core::cmp::max(current_height, minimum_locktime)
}

locktime
}
pub(crate) fn package_weight(&self, destination_script: &Script) -> u64 {
let mut inputs_weight = 0;
Expand Down Expand Up @@ -1220,7 +1204,7 @@ where
F::Target: FeeEstimator,
{
// If old feerate inferior to actual one given back by Fee Estimator, use it to compute new fee...
let (new_fee, new_feerate) = if let Some((new_fee, new_feerate)) =
let (new_fee, new_feerate) = if let Some((new_fee, new_feerate)) =
compute_fee_from_spent_amounts(input_amounts, predicted_weight, conf_target, fee_estimator, logger)
{
match feerate_strategy {
Expand Down

0 comments on commit c9b6f6d

Please sign in to comment.