Skip to content

Commit

Permalink
Make PackageTemplate::merge_package fallible
Browse files Browse the repository at this point in the history 
This moves panics to a higher level, allows failures to be handled
gracefully in some cases, and supports more explicit testing without
using `#[should_panic]`.
  • Loading branch information
@wvanlint
wvanlint committed Sep 25, 2024
1 parent 8307ce0 commit a177511
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 22 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) {
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).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
33 changes: 14 additions & 19 deletions lightning/src/chain/package.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -828,18 +828,18 @@ impl PackageTemplate {
}
}
}
pub(crate) fn merge_package(&mut self, mut merge_from: PackageTemplate) {
pub(crate) fn merge_package(&mut self, mut merge_from: PackageTemplate) -> Result<(), PackageTemplate> {
if self.malleability == PackageMalleability::Untractable || merge_from.malleability == PackageMalleability::Untractable {
panic!("Merging template on untractable packages");
return Err(merge_from);
}
if !self.aggregable || !merge_from.aggregable {
panic!("Merging non aggregatable packages");
return Err(merge_from);
}
if let Some((_, lead_input)) = self.inputs.first() {
for (_, v) in merge_from.inputs.iter() {
if !lead_input.is_compatible(v) { panic!("Merging outputs from differing types !"); }
if !lead_input.is_compatible(v) { return Err(merge_from); }
}
} else { panic!("Merging template on an empty package"); }
} else { return Err(merge_from); }
for (k, v) in merge_from.inputs.drain(..) {
self.inputs.push((k, v));
}
Expand All @@ -851,6 +851,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 Down Expand Up @@ -1320,7 +1321,6 @@ mod tests {
}

#[test]
#[should_panic]
fn test_package_untractable_merge_to() {
let txid = Txid::from_str("c2d4449afa8d26140898dd54d3390b057ba2a5afcf03ba29d7dc0d8b9ffe966e").unwrap();
let secp_ctx = Secp256k1::new();
Expand All @@ -1329,11 +1329,10 @@ mod tests {

let mut untractable_package = PackageTemplate::build_package(txid, 0, revk_outp.clone(), 1000);
let malleable_package = PackageTemplate::build_package(txid, 1, htlc_outp.clone(), 1000);
untractable_package.merge_package(malleable_package);
assert!(untractable_package.merge_package(malleable_package).is_err());
}

#[test]
#[should_panic]
fn test_package_untractable_merge_from() {
let txid = Txid::from_str("c2d4449afa8d26140898dd54d3390b057ba2a5afcf03ba29d7dc0d8b9ffe966e").unwrap();
let secp_ctx = Secp256k1::new();
Expand All @@ -1342,11 +1341,10 @@ mod tests {

let mut malleable_package = PackageTemplate::build_package(txid, 0, htlc_outp.clone(), 1000);
let untractable_package = PackageTemplate::build_package(txid, 1, revk_outp.clone(), 1000);
malleable_package.merge_package(untractable_package);
assert!(malleable_package.merge_package(untractable_package).is_err());
}

#[test]
#[should_panic]
fn test_package_noaggregation_to() {
let txid = Txid::from_str("c2d4449afa8d26140898dd54d3390b057ba2a5afcf03ba29d7dc0d8b9ffe966e").unwrap();
let secp_ctx = Secp256k1::new();
Expand All @@ -1355,11 +1353,10 @@ mod tests {

let mut noaggregation_package = PackageTemplate::build_package(txid, 0, revk_outp_counterparty_balance.clone(), 1000);
let aggregation_package = PackageTemplate::build_package(txid, 1, revk_outp.clone(), 1000);
noaggregation_package.merge_package(aggregation_package);
assert!(noaggregation_package.merge_package(aggregation_package).is_err());
}

#[test]
#[should_panic]
fn test_package_noaggregation_from() {
let txid = Txid::from_str("c2d4449afa8d26140898dd54d3390b057ba2a5afcf03ba29d7dc0d8b9ffe966e").unwrap();
let secp_ctx = Secp256k1::new();
Expand All @@ -1368,11 +1365,10 @@ mod tests {

let mut aggregation_package = PackageTemplate::build_package(txid, 0, revk_outp.clone(), 1000);
let noaggregation_package = PackageTemplate::build_package(txid, 1, revk_outp_counterparty_balance.clone(), 1000);
aggregation_package.merge_package(noaggregation_package);
assert!(aggregation_package.merge_package(noaggregation_package).is_err());
}

#[test]
#[should_panic]
fn test_package_empty() {
let txid = Txid::from_str("c2d4449afa8d26140898dd54d3390b057ba2a5afcf03ba29d7dc0d8b9ffe966e").unwrap();
let secp_ctx = Secp256k1::new();
Expand All @@ -1381,11 +1377,10 @@ mod tests {
let mut empty_package = PackageTemplate::build_package(txid, 0, revk_outp.clone(), 1000);
empty_package.inputs = vec![];
let package = PackageTemplate::build_package(txid, 1, revk_outp.clone(), 1000);
empty_package.merge_package(package);
assert!(empty_package.merge_package(package).is_err());
}

#[test]
#[should_panic]
fn test_package_differing_categories() {
let txid = Txid::from_str("c2d4449afa8d26140898dd54d3390b057ba2a5afcf03ba29d7dc0d8b9ffe966e").unwrap();
let secp_ctx = Secp256k1::new();
Expand All @@ -1394,7 +1389,7 @@ mod tests {

let mut revoked_package = PackageTemplate::build_package(txid, 0, revk_outp, 1000);
let counterparty_package = PackageTemplate::build_package(txid, 1, counterparty_outp, 1000);
revoked_package.merge_package(counterparty_package);
assert!(revoked_package.merge_package(counterparty_package).is_err());
}

#[test]
Expand All @@ -1409,8 +1404,8 @@ mod tests {
let package_two = PackageTemplate::build_package(txid, 1, revk_outp_two, 1000);
let package_three = PackageTemplate::build_package(txid, 2, revk_outp_three, 1000);

package_one.merge_package(package_two);
package_one.merge_package(package_three);
assert!(package_one.merge_package(package_two).is_ok());
assert!(package_one.merge_package(package_three).is_ok());
assert_eq!(package_one.outpoints().len(), 3);

if let Some(split_package) = package_one.split_package(&BitcoinOutPoint { txid, vout: 1 }) {
Expand Down

0 comments on commit a177511

Please sign in to comment.