Skip to content

Commit

Permalink
Stop passing current height to PackageTemplate::build_package
Browse files Browse the repository at this point in the history 
Now that we don't store the confirmation height of the inputs
being spent, passing the current height to
`PackageTemplate::build_package` is useless - we only use it to set
the height at which we should next bump the fee, but we just want
it to be "next block", so we might as well use `0` and avoid the
extra argument. Further, in one case we were already passing `0`,
so passing the argument is just confusing as we can't rely on it
being set.

Note that this does remove an assertion that we never merge
packages that were crated at different heights, and in the future
we may wish to do that (as there's no specific reason not to), but
we do not currently change the behavior.
  • Loading branch information
@TheBlueMatt
TheBlueMatt committed Sep 6, 2024
1 parent 3ea6449 commit 04c46a5
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 47 deletions.
31 changes: 20 additions & 11 deletions lightning/src/chain/channelmonitor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3002,7 +3002,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
// Assume that the broadcasted commitment transaction confirmed in the current best
// block. Even if not, its a reasonable metric for the bump criteria on the HTLC
// transactions.
let (claim_reqs, _) = self.get_broadcasted_holder_claims(&holder_commitment_tx, self.best_block.height);
let (claim_reqs, _) = self.get_broadcasted_holder_claims(&holder_commitment_tx);
let conf_target = self.closure_conf_target();
self.onchain_tx_handler.update_claims_view_from_requests(claim_reqs, self.best_block.height, self.best_block.height, broadcaster, conf_target, fee_estimator, logger);
}
Expand All @@ -3018,7 +3018,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
let commitment_package = PackageTemplate::build_package(
self.funding_info.0.txid.clone(), self.funding_info.0.index as u32,
PackageSolvingData::HolderFundingOutput(funding_outp),
self.best_block.height, self.best_block.height
self.best_block.height,
);
let mut claimable_outpoints = vec![commitment_package];
let event = MonitorEvent::HolderForceClosedWithInfo {
Expand All @@ -3041,7 +3041,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
// assuming it gets confirmed in the next block. Sadly, we have code which considers
// "not yet confirmed" things as discardable, so we cannot do that here.
let (mut new_outpoints, _) = self.get_broadcasted_holder_claims(
&self.current_holder_commitment_tx, self.best_block.height
&self.current_holder_commitment_tx,
);
let unsigned_commitment_tx = self.onchain_tx_handler.get_unsigned_holder_commitment_tx();
let new_outputs = self.get_broadcasted_holder_watch_outputs(
Expand Down Expand Up @@ -3459,7 +3459,11 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
for (idx, outp) in tx.output.iter().enumerate() {
if outp.script_pubkey == revokeable_p2wsh {
let revk_outp = RevokedOutput::build(per_commitment_point, self.counterparty_commitment_params.counterparty_delayed_payment_base_key, self.counterparty_commitment_params.counterparty_htlc_base_key, per_commitment_key, outp.value, self.counterparty_commitment_params.on_counterparty_tx_csv, self.onchain_tx_handler.channel_type_features().supports_anchors_zero_fee_htlc_tx());
let justice_package = PackageTemplate::build_package(commitment_txid, idx as u32, PackageSolvingData::RevokedOutput(revk_outp), height + self.counterparty_commitment_params.on_counterparty_tx_csv as u32, height);
let justice_package = PackageTemplate::build_package(
commitment_txid, idx as u32,
PackageSolvingData::RevokedOutput(revk_outp),
height + self.counterparty_commitment_params.on_counterparty_tx_csv as u32,
);
claimable_outpoints.push(justice_package);
to_counterparty_output_info =
Some((idx.try_into().expect("Txn can't have more than 2^32 outputs"), outp.value));
Expand All @@ -3476,7 +3480,12 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
return (claimable_outpoints, to_counterparty_output_info);
}
let revk_htlc_outp = RevokedHTLCOutput::build(per_commitment_point, self.counterparty_commitment_params.counterparty_delayed_payment_base_key, self.counterparty_commitment_params.counterparty_htlc_base_key, per_commitment_key, htlc.amount_msat / 1000, htlc.clone(), &self.onchain_tx_handler.channel_transaction_parameters.channel_type_features);
let justice_package = PackageTemplate::build_package(commitment_txid, transaction_output_index, PackageSolvingData::RevokedHTLCOutput(revk_htlc_outp), htlc.cltv_expiry, height);
let justice_package = PackageTemplate::build_package(
commitment_txid,
transaction_output_index,
PackageSolvingData::RevokedHTLCOutput(revk_htlc_outp),
htlc.cltv_expiry,
);
claimable_outpoints.push(justice_package);
}
}
Expand Down Expand Up @@ -3598,7 +3607,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
self.counterparty_commitment_params.counterparty_htlc_base_key,
htlc.clone(), self.onchain_tx_handler.channel_type_features().clone()))
};
let counterparty_package = PackageTemplate::build_package(commitment_txid, transaction_output_index, counterparty_htlc_outp, htlc.cltv_expiry, 0);
let counterparty_package = PackageTemplate::build_package(commitment_txid, transaction_output_index, counterparty_htlc_outp, htlc.cltv_expiry);
claimable_outpoints.push(counterparty_package);
}
}
Expand Down Expand Up @@ -3642,7 +3651,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
);
let justice_package = PackageTemplate::build_package(
htlc_txid, idx as u32, PackageSolvingData::RevokedOutput(revk_outp),
height + self.counterparty_commitment_params.on_counterparty_tx_csv as u32, height
height + self.counterparty_commitment_params.on_counterparty_tx_csv as u32,
);
claimable_outpoints.push(justice_package);
if outputs_to_watch.is_none() {
Expand All @@ -3657,7 +3666,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
// Returns (1) `PackageTemplate`s that can be given to the OnchainTxHandler, so that the handler can
// broadcast transactions claiming holder HTLC commitment outputs and (2) a holder revokable
// script so we can detect whether a holder transaction has been seen on-chain.
fn get_broadcasted_holder_claims(&self, holder_tx: &HolderSignedTx, conf_height: u32) -> (Vec<PackageTemplate>, Option<(ScriptBuf, PublicKey, RevocationKey)>) {
fn get_broadcasted_holder_claims(&self, holder_tx: &HolderSignedTx) -> (Vec<PackageTemplate>, Option<(ScriptBuf, PublicKey, RevocationKey)>) {
let mut claim_requests = Vec::with_capacity(holder_tx.htlc_outputs.len());

let redeemscript = chan_utils::get_revokeable_redeemscript(&holder_tx.revocation_key, self.on_holder_tx_csv, &holder_tx.delayed_payment_key);
Expand Down Expand Up @@ -3685,7 +3694,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
let htlc_package = PackageTemplate::build_package(
holder_tx.txid, transaction_output_index,
PackageSolvingData::HolderHTLCOutput(htlc_output),
htlc.cltv_expiry, conf_height
htlc.cltv_expiry,
);
claim_requests.push(htlc_package);
}
Expand Down Expand Up @@ -3728,7 +3737,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
if self.current_holder_commitment_tx.txid == commitment_txid {
is_holder_tx = true;
log_info!(logger, "Got broadcast of latest holder commitment tx {}, searching for available HTLCs to claim", commitment_txid);
let res = self.get_broadcasted_holder_claims(&self.current_holder_commitment_tx, height);
let res = self.get_broadcasted_holder_claims(&self.current_holder_commitment_tx);
let mut to_watch = self.get_broadcasted_holder_watch_outputs(&self.current_holder_commitment_tx, tx);
append_onchain_update!(res, to_watch);
fail_unbroadcast_htlcs!(self, "latest holder", commitment_txid, tx, height,
Expand All @@ -3738,7 +3747,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
if holder_tx.txid == commitment_txid {
is_holder_tx = true;
log_info!(logger, "Got broadcast of previous holder commitment tx {}, searching for available HTLCs to claim", commitment_txid);
let res = self.get_broadcasted_holder_claims(holder_tx, height);
let res = self.get_broadcasted_holder_claims(holder_tx);
let mut to_watch = self.get_broadcasted_holder_watch_outputs(holder_tx, tx);
append_onchain_update!(res, to_watch);
fail_unbroadcast_htlcs!(self, "previous holder", commitment_txid, tx, height, block_hash,
Expand Down
60 changes: 24 additions & 36 deletions lightning/src/chain/package.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1029,7 +1029,7 @@ impl PackageTemplate {
}).is_some()
}

pub (crate) fn build_package(txid: Txid, vout: u32, input_solving_data: PackageSolvingData, soonest_conf_deadline: u32, first_bump: u32) -> Self {
pub (crate) fn build_package(txid: Txid, vout: u32, input_solving_data: PackageSolvingData, soonest_conf_deadline: u32) -> Self {
let (malleability, aggregable) = PackageSolvingData::map_output_type_flags(&input_solving_data);
let inputs = vec![(BitcoinOutPoint { txid, vout }, input_solving_data)];
PackageTemplate {
Expand All @@ -1038,7 +1038,7 @@ impl PackageTemplate {
soonest_conf_deadline,
aggregable,
feerate_previous: 0,
height_timer: first_bump,
height_timer: 0,
}
}
}
Expand Down Expand Up @@ -1253,18 +1253,6 @@ mod tests {
}
}

#[test]
#[should_panic]
fn test_package_differing_heights() {
let txid = Txid::from_str("c2d4449afa8d26140898dd54d3390b057ba2a5afcf03ba29d7dc0d8b9ffe966e").unwrap();
let secp_ctx = Secp256k1::new();
let revk_outp = dumb_revk_output!(secp_ctx, false);

let mut package_one_hundred = PackageTemplate::build_package(txid, 0, revk_outp.clone(), 1000, 100);
let package_two_hundred = PackageTemplate::build_package(txid, 1, revk_outp.clone(), 1000, 200);
package_one_hundred.merge_package(package_two_hundred);
}

#[test]
#[should_panic]
fn test_package_untractable_merge_to() {
Expand All @@ -1273,8 +1261,8 @@ mod tests {
let revk_outp = dumb_revk_output!(secp_ctx, false);
let htlc_outp = dumb_htlc_output!();

let mut untractable_package = PackageTemplate::build_package(txid, 0, revk_outp.clone(), 1000, 100);
let malleable_package = PackageTemplate::build_package(txid, 1, htlc_outp.clone(), 1000, 100);
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);
}

Expand All @@ -1286,8 +1274,8 @@ mod tests {
let htlc_outp = dumb_htlc_output!();
let revk_outp = dumb_revk_output!(secp_ctx, false);

let mut malleable_package = PackageTemplate::build_package(txid, 0, htlc_outp.clone(), 1000, 100);
let untractable_package = PackageTemplate::build_package(txid, 1, revk_outp.clone(), 1000, 100);
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);
}

Expand All @@ -1299,8 +1287,8 @@ mod tests {
let revk_outp = dumb_revk_output!(secp_ctx, false);
let revk_outp_counterparty_balance = dumb_revk_output!(secp_ctx, true);

let mut noaggregation_package = PackageTemplate::build_package(txid, 0, revk_outp_counterparty_balance.clone(), 1000, 100);
let aggregation_package = PackageTemplate::build_package(txid, 1, revk_outp.clone(), 1000, 100);
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);
}

Expand All @@ -1312,8 +1300,8 @@ mod tests {
let revk_outp = dumb_revk_output!(secp_ctx, false);
let revk_outp_counterparty_balance = dumb_revk_output!(secp_ctx, true);

let mut aggregation_package = PackageTemplate::build_package(txid, 0, revk_outp.clone(), 1000, 100);
let noaggregation_package = PackageTemplate::build_package(txid, 1, revk_outp_counterparty_balance.clone(), 1000, 100);
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);
}

Expand All @@ -1324,9 +1312,9 @@ mod tests {
let secp_ctx = Secp256k1::new();
let revk_outp = dumb_revk_output!(secp_ctx, false);

let mut empty_package = PackageTemplate::build_package(txid, 0, revk_outp.clone(), 1000, 100);
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, 100);
let package = PackageTemplate::build_package(txid, 1, revk_outp.clone(), 1000);
empty_package.merge_package(package);
}

Expand All @@ -1338,8 +1326,8 @@ mod tests {
let revk_outp = dumb_revk_output!(secp_ctx, false);
let counterparty_outp = dumb_counterparty_output!(secp_ctx, 0, ChannelTypeFeatures::only_static_remote_key());

let mut revoked_package = PackageTemplate::build_package(txid, 0, revk_outp, 1000, 100);
let counterparty_package = PackageTemplate::build_package(txid, 1, counterparty_outp, 1000, 100);
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);
}

Expand All @@ -1351,9 +1339,9 @@ mod tests {
let revk_outp_two = dumb_revk_output!(secp_ctx, false);
let revk_outp_three = dumb_revk_output!(secp_ctx, false);

let mut package_one = PackageTemplate::build_package(txid, 0, revk_outp_one, 1000, 100);
let package_two = PackageTemplate::build_package(txid, 1, revk_outp_two, 1000, 100);
let package_three = PackageTemplate::build_package(txid, 2, revk_outp_three, 1000, 100);
let mut package_one = PackageTemplate::build_package(txid, 0, revk_outp_one, 1000);
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);
Expand All @@ -1375,7 +1363,7 @@ mod tests {
let txid = Txid::from_str("c2d4449afa8d26140898dd54d3390b057ba2a5afcf03ba29d7dc0d8b9ffe966e").unwrap();
let htlc_outp_one = dumb_htlc_output!();

let mut package_one = PackageTemplate::build_package(txid, 0, htlc_outp_one, 1000, 100);
let mut package_one = PackageTemplate::build_package(txid, 0, htlc_outp_one, 1000);
let ret_split = package_one.split_package(&BitcoinOutPoint { txid, vout: 0});
assert!(ret_split.is_none());
}
Expand All @@ -1386,8 +1374,8 @@ mod tests {
let secp_ctx = Secp256k1::new();
let revk_outp = dumb_revk_output!(secp_ctx, false);

let mut package = PackageTemplate::build_package(txid, 0, revk_outp, 1000, 100);
assert_eq!(package.timer(), 100);
let mut package = PackageTemplate::build_package(txid, 0, revk_outp, 1000);
assert_eq!(package.timer(), 0);
package.set_timer(101);
assert_eq!(package.timer(), 101);
}
Expand All @@ -1398,7 +1386,7 @@ mod tests {
let secp_ctx = Secp256k1::new();
let counterparty_outp = dumb_counterparty_output!(secp_ctx, 1_000_000, ChannelTypeFeatures::only_static_remote_key());

let package = PackageTemplate::build_package(txid, 0, counterparty_outp, 1000, 100);
let package = PackageTemplate::build_package(txid, 0, counterparty_outp, 1000);
assert_eq!(package.package_amount(), 1000);
}

Expand All @@ -1412,22 +1400,22 @@ mod tests {

{
let revk_outp = dumb_revk_output!(secp_ctx, false);
let package = PackageTemplate::build_package(txid, 0, revk_outp, 0, 100);
let package = PackageTemplate::build_package(txid, 0, revk_outp, 0);
assert_eq!(package.package_weight(&ScriptBuf::new()), weight_sans_output + WEIGHT_REVOKED_OUTPUT);
}

{
for channel_type_features in [ChannelTypeFeatures::only_static_remote_key(), ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies()].iter() {
let counterparty_outp = dumb_counterparty_output!(secp_ctx, 1_000_000, channel_type_features.clone());
let package = PackageTemplate::build_package(txid, 0, counterparty_outp, 1000, 100);
let package = PackageTemplate::build_package(txid, 0, counterparty_outp, 1000);
assert_eq!(package.package_weight(&ScriptBuf::new()), weight_sans_output + weight_received_htlc(channel_type_features));
}
}

{
for channel_type_features in [ChannelTypeFeatures::only_static_remote_key(), ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies()].iter() {
let counterparty_outp = dumb_counterparty_offered_output!(secp_ctx, 1_000_000, channel_type_features.clone());
let package = PackageTemplate::build_package(txid, 0, counterparty_outp, 1000, 100);
let package = PackageTemplate::build_package(txid, 0, counterparty_outp, 1000);
assert_eq!(package.package_weight(&ScriptBuf::new()), weight_sans_output + weight_offered_htlc(channel_type_features));
}
}
Expand Down

0 comments on commit 04c46a5

Please sign in to comment.