Skip to content

Commit 0fb5ca6

Browse files
tankyleotankyleo
committed
[wip] Put a 10_000vByte cap on HolderHTLCOutput 0FC package templates
Otherwise, we could potentially hit the max 10_000vB size limit on V3 transactions (BIP 431 rule 4) if we aggregate enough HTLC-claims together.
1 parent 3aa9187 commit 0fb5ca6

File tree

3 files changed

+79
-51
lines changed

3 files changed

+79
-51
lines changed

‎lightning/src/chain/channelmonitor.rs‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3816,13 +3816,14 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
38163816

38173817
// If the channel is force closed, try to claim the output from this preimage.
38183818
// First check if a counterparty commitment transaction has been broadcasted:
3819+
let is_0fc_channel = self.channel_type_features().supports_anchor_zero_fee_commitments();
38193820
macro_rules! claim_htlcs {
38203821
($commitment_number: expr, $txid: expr, $htlcs: expr) => {
38213822
let (htlc_claim_reqs, _) = self.get_counterparty_output_claim_info(funding_spent, $commitment_number, $txid, None, $htlcs, confirmed_spend_height);
38223823
let conf_target = self.closure_conf_target();
38233824
self.onchain_tx_handler.update_claims_view_from_requests(
38243825
htlc_claim_reqs, self.best_block.height, self.best_block.height, broadcaster,
3825-
conf_target, &self.destination_script, fee_estimator, logger,
3826+
conf_target, &self.destination_script, fee_estimator, is_0fc_channel, logger,
38263827
);
38273828
}
38283829
}
@@ -3874,9 +3875,10 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
38743875
funding_spent, holder_commitment_tx, self.best_block.height,
38753876
);
38763877
let conf_target = self.closure_conf_target();
3878+
let is_0fc_channel = self.channel_type_features().supports_anchor_zero_fee_commitments();
38773879
self.onchain_tx_handler.update_claims_view_from_requests(
38783880
claim_reqs, self.best_block.height, self.best_block.height, broadcaster,
3879-
conf_target, &self.destination_script, fee_estimator, logger,
3881+
conf_target, &self.destination_script, fee_estimator, is_0fc_channel, logger,
38803882
);
38813883
}
38823884
}
@@ -3955,9 +3957,10 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
39553957
};
39563958
let (claimable_outpoints, _) = self.generate_claimable_outpoints_and_watch_outputs(Some(reason));
39573959
let conf_target = self.closure_conf_target();
3960+
let is_0fc_channel = self.channel_type_features().supports_anchor_zero_fee_commitments();
39583961
self.onchain_tx_handler.update_claims_view_from_requests(
39593962
claimable_outpoints, self.best_block.height, self.best_block.height, broadcaster,
3960-
conf_target, &self.destination_script, fee_estimator, logger,
3963+
conf_target, &self.destination_script, fee_estimator, is_0fc_channel, logger,
39613964
);
39623965
}
39633966

@@ -5196,8 +5199,9 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
51965199
log_trace!(logger, "Best block re-orged, replaced with new block {} at height {}", block_hash, height);
51975200
self.onchain_events_awaiting_threshold_conf.retain(|ref entry| entry.height <= height);
51985201
let conf_target = self.closure_conf_target();
5202+
let is_0fc_channel = self.channel_type_features().supports_anchor_zero_fee_commitments();
51995203
self.onchain_tx_handler.blocks_disconnected(
5200-
height, &broadcaster, conf_target, &self.destination_script, fee_estimator, logger,
5204+
height, &broadcaster, conf_target, &self.destination_script, fee_estimator, is_0fc_channel, logger,
52015205
);
52025206
Vec::new()
52035207
} else { Vec::new() }
@@ -5659,9 +5663,10 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
56595663
}
56605664

56615665
let conf_target = self.closure_conf_target();
5666+
let is_0fc_channel = self.channel_type_features().supports_anchor_zero_fee_commitments();
56625667
self.onchain_tx_handler.update_claims_view_from_requests(
56635668
claimable_outpoints, conf_height, self.best_block.height, broadcaster, conf_target,
5664-
&self.destination_script, fee_estimator, logger,
5669+
&self.destination_script, fee_estimator, is_0fc_channel, logger,
56655670
);
56665671
self.onchain_tx_handler.update_claims_view_from_matched_txn(
56675672
&txn_matched, conf_height, conf_hash, self.best_block.height, broadcaster, conf_target,
@@ -5727,8 +5732,9 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
57275732

57285733
let bounded_fee_estimator = LowerBoundedFeeEstimator::new(fee_estimator);
57295734
let conf_target = self.closure_conf_target();
5735+
let is_0fc_channel = self.channel_type_features().supports_anchor_zero_fee_commitments();
57305736
self.onchain_tx_handler.blocks_disconnected(
5731-
new_height, &broadcaster, conf_target, &self.destination_script, &bounded_fee_estimator, logger
5737+
new_height, &broadcaster, conf_target, &self.destination_script, &bounded_fee_estimator, is_0fc_channel, logger
57325738
);
57335739

57345740
// Only attempt to broadcast the new commitment after the `block_disconnected` call above so that
@@ -5788,8 +5794,9 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
57885794
}
57895795

57905796
let conf_target = self.closure_conf_target();
5797+
let is_0fc_channel = self.channel_type_features().supports_anchor_zero_fee_commitments();
57915798
self.onchain_tx_handler.transaction_unconfirmed(
5792-
txid, &broadcaster, conf_target, &self.destination_script, fee_estimator, logger
5799+
txid, &broadcaster, conf_target, &self.destination_script, fee_estimator, is_0fc_channel, logger
57935800
);
57945801

57955802
// Only attempt to broadcast the new commitment after the `transaction_unconfirmed` call above so

‎lightning/src/chain/onchaintx.rs‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -765,7 +765,7 @@ impl<ChannelSigner: EcdsaChannelSigner> OnchainTxHandler<ChannelSigner> {
765765
pub(super) fn update_claims_view_from_requests<B: Deref, F: Deref, L: Logger>(
766766
&mut self, mut requests: Vec<PackageTemplate>, conf_height: u32, cur_height: u32,
767767
broadcaster: &B, conf_target: ConfirmationTarget, destination_script: &Script,
768-
fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L
768+
fee_estimator: &LowerBoundedFeeEstimator<F>, is_0fc_channel: bool, logger: &L
769769
) where
770770
B::Target: BroadcasterInterface,
771771
F::Target: FeeEstimator,
@@ -807,9 +807,9 @@ impl<ChannelSigner: EcdsaChannelSigner> OnchainTxHandler<ChannelSigner> {
807807
// Then try to maximally aggregate `requests`.
808808
for i in (1..requests.len()).rev() {
809809
for j in 0..i {
810-
if requests[i].can_merge_with(&requests[j], cur_height) {
810+
if requests[i].can_merge_with(&requests[j], cur_height, is_0fc_channel) {
811811
let merge = requests.remove(i);
812-
if let Err(rejected) = requests[j].merge_package(merge, cur_height) {
812+
if let Err(rejected) = requests[j].merge_package(merge, cur_height, is_0fc_channel) {
813813
debug_assert!(false, "Merging package should not be rejected after verifying can_merge_with.");
814814
requests.insert(i, rejected);
815815
} else {
@@ -1123,6 +1123,7 @@ impl<ChannelSigner: EcdsaChannelSigner> OnchainTxHandler<ChannelSigner> {
11231123
conf_target: ConfirmationTarget,
11241124
destination_script: &Script,
11251125
fee_estimator: &LowerBoundedFeeEstimator<F>,
1126+
is_0fc_channel: bool,
11261127
logger: &L,
11271128
) where
11281129
B::Target: BroadcasterInterface,
@@ -1138,15 +1139,15 @@ impl<ChannelSigner: EcdsaChannelSigner> OnchainTxHandler<ChannelSigner> {
11381139

11391140
if let Some(height) = height {
11401141
self.blocks_disconnected(
1141-
height - 1, broadcaster, conf_target, destination_script, fee_estimator, logger,
1142+
height - 1, broadcaster, conf_target, destination_script, fee_estimator, is_0fc_channel, logger,
11421143
);
11431144
}
11441145
}
11451146

11461147
#[rustfmt::skip]
11471148
pub(super) fn blocks_disconnected<B: Deref, F: Deref, L: Logger>(
11481149
&mut self, new_best_height: u32, broadcaster: &B, conf_target: ConfirmationTarget,
1149-
destination_script: &Script, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L,
1150+
destination_script: &Script, fee_estimator: &LowerBoundedFeeEstimator<F>, is_0fc_channel: bool, logger: &L,
11501151
)
11511152
where B::Target: BroadcasterInterface,
11521153
F::Target: FeeEstimator,
@@ -1169,7 +1170,8 @@ impl<ChannelSigner: EcdsaChannelSigner> OnchainTxHandler<ChannelSigner> {
11691170

11701171
if let Some(pending_claim) = self.claimable_outpoints.get(package.outpoints()[0]) {
11711172
if let Some(request) = self.pending_claim_requests.get_mut(&pending_claim.0) {
1172-
assert!(request.merge_package(package, new_best_height + 1).is_ok());
1173+
// TODO: double check this assert holds here for V3 HTLC transactions
1174+
assert!(request.merge_package(package, new_best_height + 1, is_0fc_channel).is_ok());
11731175
// Using a HashMap guarantee us than if we have multiple outpoints getting
11741176
// resurrected only one bump claim tx is going to be broadcast
11751177
bump_candidates.insert(pending_claim.clone(), request.clone());
@@ -1426,6 +1428,7 @@ mod tests {
14261428
ConfirmationTarget::UrgentOnChainSweep,
14271429
&destination_script,
14281430
&fee_estimator,
1431+
false,
14291432
&logger,
14301433
);
14311434

@@ -1450,6 +1453,7 @@ mod tests {
14501453
ConfirmationTarget::UrgentOnChainSweep,
14511454
&destination_script,
14521455
&fee_estimator,
1456+
false,
14531457
&logger,
14541458
);
14551459

‎lightning/src/chain/package.rs‎

Lines changed: 55 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1183,8 +1183,11 @@ impl PartialEq for PackageTemplate {
11831183
}
11841184

11851185
impl PackageTemplate {
1186+
fn weight(&self) -> u64 {
1187+
self.inputs.iter().map(|(_, solving_data)| solving_data.weight()).sum::<usize>() as u64
1188+
}
11861189
#[rustfmt::skip]
1187-
pub(crate) fn can_merge_with(&self, other: &PackageTemplate, cur_height: u32) -> bool {
1190+
pub(crate) fn can_merge_with(&self, other: &PackageTemplate, cur_height: u32, is_0fc_channel: bool) -> bool {
11881191
match (self.malleability, other.malleability) {
11891192
(PackageMalleability::Untractable, _) => false,
11901193
(_, PackageMalleability::Untractable) => false,
@@ -1235,16 +1238,30 @@ impl PackageTemplate {
12351238
self.counterparty_spendable_height <= cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE;
12361239
let other_pinnable = other_cluster == AggregationCluster::Pinnable ||
12371240
other.counterparty_spendable_height <= cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE;
1238-
if self_pinnable && other_pinnable {
1239-
return true;
1240-
}
12411241

12421242
let self_unpinnable = self_cluster == AggregationCluster::Unpinnable &&
12431243
self.counterparty_spendable_height > cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE;
12441244
let other_unpinnable = other_cluster == AggregationCluster::Unpinnable &&
12451245
other.counterparty_spendable_height > cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE;
1246-
if self_unpinnable && other_unpinnable {
1247-
return true;
1246+
1247+
if self_pinnable && other_pinnable || self_unpinnable && other_unpinnable {
1248+
if is_0fc_channel && self.inputs.iter().any(|(_, solving_data)| matches!(solving_data, PackageSolvingData::HolderHTLCOutput(_))) {
1249+
// MUST be true, otherwise we are aggregating V2 tx claims with V3 tx claims
1250+
debug_assert!(self.inputs.iter().all(|(_, solving_data)| matches!(solving_data, PackageSolvingData::HolderHTLCOutput(_) )));
1251+
debug_assert!(other.inputs.iter().all(|(_, solving_data)| matches!(solving_data, PackageSolvingData::HolderHTLCOutput(_) )));
1252+
// See rust-bitcoin to_vbytes_ceil
1253+
let self_vbytes = (self.weight() + 3) / 4; // This is the weight of the witnesses alone, we need to add more here
1254+
let other_vbytes = (other.weight() + 3) / 4;
1255+
// What is a good offset to use here to leave room for the user-provided input-output pair?
1256+
// How much validation to do at coin-selection time in bump_transaction mod ?
1257+
// Just warn users in the docs not to use some really heavy witnesses to fee-bump their transactions?
1258+
// A 1-input-1-output p2wpkh-input p2wpkh-input transaction is 109.25vB.
1259+
if self_vbytes + other_vbytes < 10_000 - 200 {
1260+
return true;
1261+
}
1262+
} else {
1263+
return true;
1264+
}
12481265
}
12491266
false
12501267
},
@@ -1310,9 +1327,9 @@ impl PackageTemplate {
13101327
}
13111328
}
13121329
pub(crate) fn merge_package(
1313-
&mut self, mut merge_from: PackageTemplate, cur_height: u32,
1330+
&mut self, mut merge_from: PackageTemplate, cur_height: u32, is_0fc_channel: bool,
13141331
) -> Result<(), PackageTemplate> {
1315-
if !self.can_merge_with(&merge_from, cur_height) {
1332+
if !self.can_merge_with(&merge_from, cur_height, is_0fc_channel) {
13161333
return Err(merge_from);
13171334
}
13181335
for (k, v) in merge_from.inputs.drain(..) {
@@ -1985,11 +2002,11 @@ mod tests {
19852002
let mut untractable_package = PackageTemplate::build_package(fake_txid(1), 0, funding_outp.clone(), 0);
19862003
let mut malleable_package = PackageTemplate::build_package(fake_txid(2), 0, htlc_outp.clone(), 1100);
19872004

1988-
assert!(!untractable_package.can_merge_with(&malleable_package, 1000));
1989-
assert!(untractable_package.merge_package(malleable_package.clone(), 1000).is_err());
2005+
assert!(!untractable_package.can_merge_with(&malleable_package, 1000, false));
2006+
assert!(untractable_package.merge_package(malleable_package.clone(), 1000, false).is_err());
19902007

1991-
assert!(!malleable_package.can_merge_with(&untractable_package, 1000));
1992-
assert!(malleable_package.merge_package(untractable_package.clone(), 1000).is_err());
2008+
assert!(!malleable_package.can_merge_with(&untractable_package, 1000, false));
2009+
assert!(malleable_package.merge_package(untractable_package.clone(), 1000, false).is_err());
19932010
}
19942011

19952012
#[test]
@@ -2000,8 +2017,8 @@ mod tests {
20002017
let mut empty_package = PackageTemplate::build_package(fake_txid(1), 0, revk_outp.clone(), 0);
20012018
empty_package.inputs = vec![];
20022019
let mut package = PackageTemplate::build_package(fake_txid(1), 1, revk_outp.clone(), 1100);
2003-
assert!(empty_package.merge_package(package.clone(), 1000).is_err());
2004-
assert!(package.merge_package(empty_package.clone(), 1000).is_err());
2020+
assert!(empty_package.merge_package(package.clone(), 1000, false).is_err());
2021+
assert!(package.merge_package(empty_package.clone(), 1000, false).is_err());
20052022
}
20062023

20072024
#[test]
@@ -2017,15 +2034,15 @@ mod tests {
20172034
let mut offered_htlc_2_package = PackageTemplate::build_package(fake_txid(1), 1, offered_htlc_2.clone(), 0);
20182035
let mut accepted_htlc_package = PackageTemplate::build_package(fake_txid(1), 2, accepted_htlc.clone(), 1001);
20192036

2020-
assert!(!offered_htlc_2_package.can_merge_with(&offered_htlc_1_package, 1000));
2021-
assert!(offered_htlc_2_package.merge_package(offered_htlc_1_package.clone(), 1000).is_err());
2022-
assert!(!offered_htlc_1_package.can_merge_with(&offered_htlc_2_package, 1000));
2023-
assert!(offered_htlc_1_package.merge_package(offered_htlc_2_package.clone(), 1000).is_err());
2037+
assert!(!offered_htlc_2_package.can_merge_with(&offered_htlc_1_package, 1000, false));
2038+
assert!(offered_htlc_2_package.merge_package(offered_htlc_1_package.clone(), 1000, false).is_err());
2039+
assert!(!offered_htlc_1_package.can_merge_with(&offered_htlc_2_package, 1000, false));
2040+
assert!(offered_htlc_1_package.merge_package(offered_htlc_2_package.clone(), 1000, false).is_err());
20242041

2025-
assert!(!accepted_htlc_package.can_merge_with(&offered_htlc_1_package, 1000));
2026-
assert!(accepted_htlc_package.merge_package(offered_htlc_1_package.clone(), 1000).is_err());
2027-
assert!(!offered_htlc_1_package.can_merge_with(&accepted_htlc_package, 1000));
2028-
assert!(offered_htlc_1_package.merge_package(accepted_htlc_package.clone(), 1000).is_err());
2042+
assert!(!accepted_htlc_package.can_merge_with(&offered_htlc_1_package, 1000, false));
2043+
assert!(accepted_htlc_package.merge_package(offered_htlc_1_package.clone(), 1000, false).is_err());
2044+
assert!(!offered_htlc_1_package.can_merge_with(&accepted_htlc_package, 1000, false));
2045+
assert!(offered_htlc_1_package.merge_package(accepted_htlc_package.clone(), 1000, false).is_err());
20292046
}
20302047

20312048
#[test]
@@ -2043,15 +2060,15 @@ mod tests {
20432060
let future_outp_1_package = PackageTemplate::build_package(fake_txid(1), 2, future_outp_1.clone(), 0);
20442061
let future_outp_2_package = PackageTemplate::build_package(fake_txid(1), 3, future_outp_2.clone(), 0);
20452062

2046-
assert!(old_outp_1_package.can_merge_with(&old_outp_2_package, 1000));
2047-
assert!(old_outp_2_package.can_merge_with(&old_outp_1_package, 1000));
2048-
assert!(old_outp_1_package.clone().merge_package(old_outp_2_package.clone(), 1000).is_ok());
2049-
assert!(old_outp_2_package.clone().merge_package(old_outp_1_package.clone(), 1000).is_ok());
2063+
assert!(old_outp_1_package.can_merge_with(&old_outp_2_package, 1000, false));
2064+
assert!(old_outp_2_package.can_merge_with(&old_outp_1_package, 1000, false));
2065+
assert!(old_outp_1_package.clone().merge_package(old_outp_2_package.clone(), 1000, false).is_ok());
2066+
assert!(old_outp_2_package.clone().merge_package(old_outp_1_package.clone(), 1000, false).is_ok());
20502067

2051-
assert!(!future_outp_1_package.can_merge_with(&future_outp_2_package, 1000));
2052-
assert!(!future_outp_2_package.can_merge_with(&future_outp_1_package, 1000));
2053-
assert!(future_outp_1_package.clone().merge_package(future_outp_2_package.clone(), 1000).is_err());
2054-
assert!(future_outp_2_package.clone().merge_package(future_outp_1_package.clone(), 1000).is_err());
2068+
assert!(!future_outp_1_package.can_merge_with(&future_outp_2_package, 1000, false));
2069+
assert!(!future_outp_2_package.can_merge_with(&future_outp_1_package, 1000, false));
2070+
assert!(future_outp_1_package.clone().merge_package(future_outp_2_package.clone(), 1000, false).is_err());
2071+
assert!(future_outp_2_package.clone().merge_package(future_outp_1_package.clone(), 1000, false).is_err());
20552072
}
20562073

20572074
#[test]
@@ -2087,10 +2104,10 @@ mod tests {
20872104
for i in 0..clusters[a].len() {
20882105
for j in 0..clusters[b].len() {
20892106
if a != b {
2090-
assert!(!clusters[a][i].can_merge_with(clusters[b][j], 1000));
2107+
assert!(!clusters[a][i].can_merge_with(clusters[b][j], 1000, false));
20912108
} else {
20922109
if i != j {
2093-
assert!(clusters[a][i].can_merge_with(clusters[b][j], 1000));
2110+
assert!(clusters[a][i].can_merge_with(clusters[b][j], 1000, false));
20942111
}
20952112
}
20962113
}
@@ -2104,9 +2121,9 @@ mod tests {
21042121
];
21052122
for i in (1..packages.len()).rev() {
21062123
for j in 0..i {
2107-
if packages[i].can_merge_with(&packages[j], 1000) {
2124+
if packages[i].can_merge_with(&packages[j], 1000, false) {
21082125
let merge = packages.remove(i);
2109-
assert!(packages[j].merge_package(merge, 1000).is_ok());
2126+
assert!(packages[j].merge_package(merge, 1000, false).is_ok());
21102127
}
21112128
}
21122129
}
@@ -2122,8 +2139,8 @@ mod tests {
21222139
let counterparty_received_htlc = dumb_counterparty_received_output!(1_000_000, 900, ChannelTypeFeatures::only_static_remote_key());
21232140
let counterparty_received_htlc_package = PackageTemplate::build_package(fake_txid(2), 0, counterparty_received_htlc.clone(), 0);
21242141

2125-
assert!(!offered_htlc_package.can_merge_with(&counterparty_received_htlc_package, 1000));
2126-
assert!(offered_htlc_package.merge_package(counterparty_received_htlc_package.clone(), 1000).is_err());
2142+
assert!(!offered_htlc_package.can_merge_with(&counterparty_received_htlc_package, 1000, false));
2143+
assert!(offered_htlc_package.merge_package(counterparty_received_htlc_package.clone(), 1000, false).is_err());
21272144
}
21282145

21292146
#[test]
@@ -2137,8 +2154,8 @@ mod tests {
21372154
let package_two = PackageTemplate::build_package(fake_txid(1), 1, revk_outp_two, 1100);
21382155
let package_three = PackageTemplate::build_package(fake_txid(1), 2, revk_outp_three, 1100);
21392156

2140-
assert!(package_one.merge_package(package_two, 1000).is_ok());
2141-
assert!(package_one.merge_package(package_three, 1000).is_ok());
2157+
assert!(package_one.merge_package(package_two, 1000, false).is_ok());
2158+
assert!(package_one.merge_package(package_three, 1000, false).is_ok());
21422159
assert_eq!(package_one.outpoints().len(), 3);
21432160

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

0 commit comments

Comments
 (0)