Skip to content

Commit 04c46a5

Browse files
TheBlueMattTheBlueMatt
committed
Stop passing current height to PackageTemplate::build_package
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.
1 parent 3ea6449 commit 04c46a5

File tree

2 files changed

+44
-47
lines changed

2 files changed

+44
-47
lines changed

‎lightning/src/chain/channelmonitor.rs

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3002,7 +3002,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
30023002
// Assume that the broadcasted commitment transaction confirmed in the current best
30033003
// block. Even if not, its a reasonable metric for the bump criteria on the HTLC
30043004
// transactions.
3005-
let (claim_reqs, _) = self.get_broadcasted_holder_claims(&holder_commitment_tx, self.best_block.height);
3005+
let (claim_reqs, _) = self.get_broadcasted_holder_claims(&holder_commitment_tx);
30063006
let conf_target = self.closure_conf_target();
30073007
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);
30083008
}
@@ -3018,7 +3018,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
30183018
let commitment_package = PackageTemplate::build_package(
30193019
self.funding_info.0.txid.clone(), self.funding_info.0.index as u32,
30203020
PackageSolvingData::HolderFundingOutput(funding_outp),
3021-
self.best_block.height, self.best_block.height
3021+
self.best_block.height,
30223022
);
30233023
let mut claimable_outpoints = vec![commitment_package];
30243024
let event = MonitorEvent::HolderForceClosedWithInfo {
@@ -3041,7 +3041,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
30413041
// assuming it gets confirmed in the next block. Sadly, we have code which considers
30423042
// "not yet confirmed" things as discardable, so we cannot do that here.
30433043
let (mut new_outpoints, _) = self.get_broadcasted_holder_claims(
3044-
&self.current_holder_commitment_tx, self.best_block.height
3044+
&self.current_holder_commitment_tx,
30453045
);
30463046
let unsigned_commitment_tx = self.onchain_tx_handler.get_unsigned_holder_commitment_tx();
30473047
let new_outputs = self.get_broadcasted_holder_watch_outputs(
@@ -3459,7 +3459,11 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
34593459
for (idx, outp) in tx.output.iter().enumerate() {
34603460
if outp.script_pubkey == revokeable_p2wsh {
34613461
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());
3462-
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);
3462+
let justice_package = PackageTemplate::build_package(
3463+
commitment_txid, idx as u32,
3464+
PackageSolvingData::RevokedOutput(revk_outp),
3465+
height + self.counterparty_commitment_params.on_counterparty_tx_csv as u32,
3466+
);
34633467
claimable_outpoints.push(justice_package);
34643468
to_counterparty_output_info =
34653469
Some((idx.try_into().expect("Txn can't have more than 2^32 outputs"), outp.value));
@@ -3476,7 +3480,12 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
34763480
return (claimable_outpoints, to_counterparty_output_info);
34773481
}
34783482
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);
3479-
let justice_package = PackageTemplate::build_package(commitment_txid, transaction_output_index, PackageSolvingData::RevokedHTLCOutput(revk_htlc_outp), htlc.cltv_expiry, height);
3483+
let justice_package = PackageTemplate::build_package(
3484+
commitment_txid,
3485+
transaction_output_index,
3486+
PackageSolvingData::RevokedHTLCOutput(revk_htlc_outp),
3487+
htlc.cltv_expiry,
3488+
);
34803489
claimable_outpoints.push(justice_package);
34813490
}
34823491
}
@@ -3598,7 +3607,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
35983607
self.counterparty_commitment_params.counterparty_htlc_base_key,
35993608
htlc.clone(), self.onchain_tx_handler.channel_type_features().clone()))
36003609
};
3601-
let counterparty_package = PackageTemplate::build_package(commitment_txid, transaction_output_index, counterparty_htlc_outp, htlc.cltv_expiry, 0);
3610+
let counterparty_package = PackageTemplate::build_package(commitment_txid, transaction_output_index, counterparty_htlc_outp, htlc.cltv_expiry);
36023611
claimable_outpoints.push(counterparty_package);
36033612
}
36043613
}
@@ -3642,7 +3651,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
36423651
);
36433652
let justice_package = PackageTemplate::build_package(
36443653
htlc_txid, idx as u32, PackageSolvingData::RevokedOutput(revk_outp),
3645-
height + self.counterparty_commitment_params.on_counterparty_tx_csv as u32, height
3654+
height + self.counterparty_commitment_params.on_counterparty_tx_csv as u32,
36463655
);
36473656
claimable_outpoints.push(justice_package);
36483657
if outputs_to_watch.is_none() {
@@ -3657,7 +3666,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
36573666
// Returns (1) `PackageTemplate`s that can be given to the OnchainTxHandler, so that the handler can
36583667
// broadcast transactions claiming holder HTLC commitment outputs and (2) a holder revokable
36593668
// script so we can detect whether a holder transaction has been seen on-chain.
3660-
fn get_broadcasted_holder_claims(&self, holder_tx: &HolderSignedTx, conf_height: u32) -> (Vec<PackageTemplate>, Option<(ScriptBuf, PublicKey, RevocationKey)>) {
3669+
fn get_broadcasted_holder_claims(&self, holder_tx: &HolderSignedTx) -> (Vec<PackageTemplate>, Option<(ScriptBuf, PublicKey, RevocationKey)>) {
36613670
let mut claim_requests = Vec::with_capacity(holder_tx.htlc_outputs.len());
36623671

36633672
let redeemscript = chan_utils::get_revokeable_redeemscript(&holder_tx.revocation_key, self.on_holder_tx_csv, &holder_tx.delayed_payment_key);
@@ -3685,7 +3694,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
36853694
let htlc_package = PackageTemplate::build_package(
36863695
holder_tx.txid, transaction_output_index,
36873696
PackageSolvingData::HolderHTLCOutput(htlc_output),
3688-
htlc.cltv_expiry, conf_height
3697+
htlc.cltv_expiry,
36893698
);
36903699
claim_requests.push(htlc_package);
36913700
}
@@ -3728,7 +3737,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
37283737
if self.current_holder_commitment_tx.txid == commitment_txid {
37293738
is_holder_tx = true;
37303739
log_info!(logger, "Got broadcast of latest holder commitment tx {}, searching for available HTLCs to claim", commitment_txid);
3731-
let res = self.get_broadcasted_holder_claims(&self.current_holder_commitment_tx, height);
3740+
let res = self.get_broadcasted_holder_claims(&self.current_holder_commitment_tx);
37323741
let mut to_watch = self.get_broadcasted_holder_watch_outputs(&self.current_holder_commitment_tx, tx);
37333742
append_onchain_update!(res, to_watch);
37343743
fail_unbroadcast_htlcs!(self, "latest holder", commitment_txid, tx, height,
@@ -3738,7 +3747,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
37383747
if holder_tx.txid == commitment_txid {
37393748
is_holder_tx = true;
37403749
log_info!(logger, "Got broadcast of previous holder commitment tx {}, searching for available HTLCs to claim", commitment_txid);
3741-
let res = self.get_broadcasted_holder_claims(holder_tx, height);
3750+
let res = self.get_broadcasted_holder_claims(holder_tx);
37423751
let mut to_watch = self.get_broadcasted_holder_watch_outputs(holder_tx, tx);
37433752
append_onchain_update!(res, to_watch);
37443753
fail_unbroadcast_htlcs!(self, "previous holder", commitment_txid, tx, height, block_hash,

‎lightning/src/chain/package.rs

Lines changed: 24 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1029,7 +1029,7 @@ impl PackageTemplate {
10291029
}).is_some()
10301030
}
10311031

1032-
pub (crate) fn build_package(txid: Txid, vout: u32, input_solving_data: PackageSolvingData, soonest_conf_deadline: u32, first_bump: u32) -> Self {
1032+
pub (crate) fn build_package(txid: Txid, vout: u32, input_solving_data: PackageSolvingData, soonest_conf_deadline: u32) -> Self {
10331033
let (malleability, aggregable) = PackageSolvingData::map_output_type_flags(&input_solving_data);
10341034
let inputs = vec![(BitcoinOutPoint { txid, vout }, input_solving_data)];
10351035
PackageTemplate {
@@ -1038,7 +1038,7 @@ impl PackageTemplate {
10381038
soonest_conf_deadline,
10391039
aggregable,
10401040
feerate_previous: 0,
1041-
height_timer: first_bump,
1041+
height_timer: 0,
10421042
}
10431043
}
10441044
}
@@ -1253,18 +1253,6 @@ mod tests {
12531253
}
12541254
}
12551255

1256-
#[test]
1257-
#[should_panic]
1258-
fn test_package_differing_heights() {
1259-
let txid = Txid::from_str("c2d4449afa8d26140898dd54d3390b057ba2a5afcf03ba29d7dc0d8b9ffe966e").unwrap();
1260-
let secp_ctx = Secp256k1::new();
1261-
let revk_outp = dumb_revk_output!(secp_ctx, false);
1262-
1263-
let mut package_one_hundred = PackageTemplate::build_package(txid, 0, revk_outp.clone(), 1000, 100);
1264-
let package_two_hundred = PackageTemplate::build_package(txid, 1, revk_outp.clone(), 1000, 200);
1265-
package_one_hundred.merge_package(package_two_hundred);
1266-
}
1267-
12681256
#[test]
12691257
#[should_panic]
12701258
fn test_package_untractable_merge_to() {
@@ -1273,8 +1261,8 @@ mod tests {
12731261
let revk_outp = dumb_revk_output!(secp_ctx, false);
12741262
let htlc_outp = dumb_htlc_output!();
12751263

1276-
let mut untractable_package = PackageTemplate::build_package(txid, 0, revk_outp.clone(), 1000, 100);
1277-
let malleable_package = PackageTemplate::build_package(txid, 1, htlc_outp.clone(), 1000, 100);
1264+
let mut untractable_package = PackageTemplate::build_package(txid, 0, revk_outp.clone(), 1000);
1265+
let malleable_package = PackageTemplate::build_package(txid, 1, htlc_outp.clone(), 1000);
12781266
untractable_package.merge_package(malleable_package);
12791267
}
12801268

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

1289-
let mut malleable_package = PackageTemplate::build_package(txid, 0, htlc_outp.clone(), 1000, 100);
1290-
let untractable_package = PackageTemplate::build_package(txid, 1, revk_outp.clone(), 1000, 100);
1277+
let mut malleable_package = PackageTemplate::build_package(txid, 0, htlc_outp.clone(), 1000);
1278+
let untractable_package = PackageTemplate::build_package(txid, 1, revk_outp.clone(), 1000);
12911279
malleable_package.merge_package(untractable_package);
12921280
}
12931281

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

1302-
let mut noaggregation_package = PackageTemplate::build_package(txid, 0, revk_outp_counterparty_balance.clone(), 1000, 100);
1303-
let aggregation_package = PackageTemplate::build_package(txid, 1, revk_outp.clone(), 1000, 100);
1290+
let mut noaggregation_package = PackageTemplate::build_package(txid, 0, revk_outp_counterparty_balance.clone(), 1000);
1291+
let aggregation_package = PackageTemplate::build_package(txid, 1, revk_outp.clone(), 1000);
13041292
noaggregation_package.merge_package(aggregation_package);
13051293
}
13061294

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

1315-
let mut aggregation_package = PackageTemplate::build_package(txid, 0, revk_outp.clone(), 1000, 100);
1316-
let noaggregation_package = PackageTemplate::build_package(txid, 1, revk_outp_counterparty_balance.clone(), 1000, 100);
1303+
let mut aggregation_package = PackageTemplate::build_package(txid, 0, revk_outp.clone(), 1000);
1304+
let noaggregation_package = PackageTemplate::build_package(txid, 1, revk_outp_counterparty_balance.clone(), 1000);
13171305
aggregation_package.merge_package(noaggregation_package);
13181306
}
13191307

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

1327-
let mut empty_package = PackageTemplate::build_package(txid, 0, revk_outp.clone(), 1000, 100);
1315+
let mut empty_package = PackageTemplate::build_package(txid, 0, revk_outp.clone(), 1000);
13281316
empty_package.inputs = vec![];
1329-
let package = PackageTemplate::build_package(txid, 1, revk_outp.clone(), 1000, 100);
1317+
let package = PackageTemplate::build_package(txid, 1, revk_outp.clone(), 1000);
13301318
empty_package.merge_package(package);
13311319
}
13321320

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

1341-
let mut revoked_package = PackageTemplate::build_package(txid, 0, revk_outp, 1000, 100);
1342-
let counterparty_package = PackageTemplate::build_package(txid, 1, counterparty_outp, 1000, 100);
1329+
let mut revoked_package = PackageTemplate::build_package(txid, 0, revk_outp, 1000);
1330+
let counterparty_package = PackageTemplate::build_package(txid, 1, counterparty_outp, 1000);
13431331
revoked_package.merge_package(counterparty_package);
13441332
}
13451333

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

1354-
let mut package_one = PackageTemplate::build_package(txid, 0, revk_outp_one, 1000, 100);
1355-
let package_two = PackageTemplate::build_package(txid, 1, revk_outp_two, 1000, 100);
1356-
let package_three = PackageTemplate::build_package(txid, 2, revk_outp_three, 1000, 100);
1342+
let mut package_one = PackageTemplate::build_package(txid, 0, revk_outp_one, 1000);
1343+
let package_two = PackageTemplate::build_package(txid, 1, revk_outp_two, 1000);
1344+
let package_three = PackageTemplate::build_package(txid, 2, revk_outp_three, 1000);
13571345

13581346
package_one.merge_package(package_two);
13591347
package_one.merge_package(package_three);
@@ -1375,7 +1363,7 @@ mod tests {
13751363
let txid = Txid::from_str("c2d4449afa8d26140898dd54d3390b057ba2a5afcf03ba29d7dc0d8b9ffe966e").unwrap();
13761364
let htlc_outp_one = dumb_htlc_output!();
13771365

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

1389-
let mut package = PackageTemplate::build_package(txid, 0, revk_outp, 1000, 100);
1390-
assert_eq!(package.timer(), 100);
1377+
let mut package = PackageTemplate::build_package(txid, 0, revk_outp, 1000);
1378+
assert_eq!(package.timer(), 0);
13911379
package.set_timer(101);
13921380
assert_eq!(package.timer(), 101);
13931381
}
@@ -1398,7 +1386,7 @@ mod tests {
13981386
let secp_ctx = Secp256k1::new();
13991387
let counterparty_outp = dumb_counterparty_output!(secp_ctx, 1_000_000, ChannelTypeFeatures::only_static_remote_key());
14001388

1401-
let package = PackageTemplate::build_package(txid, 0, counterparty_outp, 1000, 100);
1389+
let package = PackageTemplate::build_package(txid, 0, counterparty_outp, 1000);
14021390
assert_eq!(package.package_amount(), 1000);
14031391
}
14041392

@@ -1412,22 +1400,22 @@ mod tests {
14121400

14131401
{
14141402
let revk_outp = dumb_revk_output!(secp_ctx, false);
1415-
let package = PackageTemplate::build_package(txid, 0, revk_outp, 0, 100);
1403+
let package = PackageTemplate::build_package(txid, 0, revk_outp, 0);
14161404
assert_eq!(package.package_weight(&ScriptBuf::new()), weight_sans_output + WEIGHT_REVOKED_OUTPUT);
14171405
}
14181406

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

14271415
{
14281416
for channel_type_features in [ChannelTypeFeatures::only_static_remote_key(), ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies()].iter() {
14291417
let counterparty_outp = dumb_counterparty_offered_output!(secp_ctx, 1_000_000, channel_type_features.clone());
1430-
let package = PackageTemplate::build_package(txid, 0, counterparty_outp, 1000, 100);
1418+
let package = PackageTemplate::build_package(txid, 0, counterparty_outp, 1000);
14311419
assert_eq!(package.package_weight(&ScriptBuf::new()), weight_sans_output + weight_offered_htlc(channel_type_features));
14321420
}
14331421
}

0 commit comments

Comments
 (0)