From 633be238fd829309861913c9b1e24aebfaa787d1 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Wed, 15 Nov 2023 13:27:27 -0500 Subject: [PATCH] Fix skimmed fee ser in Channel Previously, if some holding cell HTLCs have skimmed fees present and some don't, we would fail to deserialize a Channel. See added test coverage. --- lightning/src/ln/channel.rs | 33 +++++++++++---------------- pending_changelog/skimmed_fee_ser.txt | 4 ++++ 2 files changed, 17 insertions(+), 20 deletions(-) create mode 100644 pending_changelog/skimmed_fee_ser.txt diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 9455a761..f01454bd 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -7152,7 +7152,7 @@ impl Writeable for Channel where SP::Target: SignerProvider { let mut pending_outbound_blinding_points: Vec> = Vec::new(); (self.context.pending_outbound_htlcs.len() as u64).write(writer)?; - for (idx, htlc) in self.context.pending_outbound_htlcs.iter().enumerate() { + for htlc in self.context.pending_outbound_htlcs.iter() { htlc.htlc_id.write(writer)?; htlc.amount_msat.write(writer)?; htlc.cltv_expiry.write(writer)?; @@ -7188,21 +7188,14 @@ impl Writeable for Channel where SP::Target: SignerProvider { reason.write(writer)?; } } - if let Some(skimmed_fee) = htlc.skimmed_fee_msat { - if pending_outbound_skimmed_fees.is_empty() { - for _ in 0..idx { pending_outbound_skimmed_fees.push(None); } - } - pending_outbound_skimmed_fees.push(Some(skimmed_fee)); - } else if !pending_outbound_skimmed_fees.is_empty() { - pending_outbound_skimmed_fees.push(None); - } + pending_outbound_skimmed_fees.push(htlc.skimmed_fee_msat); pending_outbound_blinding_points.push(htlc.blinding_point); } let mut holding_cell_skimmed_fees: Vec> = Vec::new(); let mut holding_cell_blinding_points: Vec> = Vec::new(); (self.context.holding_cell_htlc_updates.len() as u64).write(writer)?; - for (idx, update) in self.context.holding_cell_htlc_updates.iter().enumerate() { + for update in self.context.holding_cell_htlc_updates.iter() { match update { &HTLCUpdateAwaitingACK::AddHTLC { ref amount_msat, ref cltv_expiry, ref payment_hash, ref source, ref onion_routing_packet, @@ -7215,13 +7208,7 @@ impl Writeable for Channel where SP::Target: SignerProvider { source.write(writer)?; onion_routing_packet.write(writer)?; - if let Some(skimmed_fee) = skimmed_fee_msat { - if holding_cell_skimmed_fees.is_empty() { - for _ in 0..idx { holding_cell_skimmed_fees.push(None); } - } - holding_cell_skimmed_fees.push(Some(skimmed_fee)); - } else if !holding_cell_skimmed_fees.is_empty() { holding_cell_skimmed_fees.push(None); } - + holding_cell_skimmed_fees.push(skimmed_fee_msat); holding_cell_blinding_points.push(blinding_point); }, &HTLCUpdateAwaitingACK::ClaimHTLC { ref payment_preimage, ref htlc_id } => { @@ -8474,8 +8461,8 @@ mod tests { } #[test] - fn blinding_point_ser() { - // Ensure that channel blinding points are (de)serialized properly. + fn blinding_point_skimmed_fee_ser() { + // Ensure that channel blinding points and skimmed fees are (de)serialized properly. let feeest = LowerBoundedFeeEstimator::new(&TestFeeEstimator{fee_est: 15000}); let secp_ctx = Secp256k1::new(); let seed = [42; 32]; @@ -8516,6 +8503,9 @@ mod tests { if idx % 2 == 0 { htlc.blinding_point = Some(test_utils::pubkey(42 + idx as u8)); } + if idx % 3 == 0 { + htlc.skimmed_fee_msat = Some(1); + } } chan.context.pending_outbound_htlcs = pending_outbound_htlcs.clone(); @@ -8545,8 +8535,11 @@ mod tests { holding_cell_htlc_updates.push(dummy_holding_cell_claim_htlc.clone()); } else { let mut dummy_add = dummy_holding_cell_add_htlc.clone(); - if let HTLCUpdateAwaitingACK::AddHTLC { ref mut blinding_point, .. } = &mut dummy_add { + if let HTLCUpdateAwaitingACK::AddHTLC { + ref mut blinding_point, ref mut skimmed_fee_msat, .. + } = &mut dummy_add { *blinding_point = Some(test_utils::pubkey(42 + i)); + *skimmed_fee_msat = Some(42); } else { panic!() } holding_cell_htlc_updates.push(dummy_add); } diff --git a/pending_changelog/skimmed_fee_ser.txt b/pending_changelog/skimmed_fee_ser.txt new file mode 100644 index 00000000..d3e68746 --- /dev/null +++ b/pending_changelog/skimmed_fee_ser.txt @@ -0,0 +1,4 @@ +## Bug fixes + +* In LDK versions 0.0.116 through 0.0.118, in rare cases where skimmed fees are present on shutdown + the `ChannelManager` may fail to deserialize on startup. -- 2.30.2