Skip to content

Commit

Permalink
Merge pull request #3429 from andrei-21/feature/channelmonitor-goodies
Browse files Browse the repository at this point in the history 
Clean channelmonitor.rs code
  • Loading branch information
@TheBlueMatt
TheBlueMatt authored Dec 4, 2024
2 parents 1386bef + 8ebda06 commit 726dd5c
Showing 1 changed file with 53 additions and 65 deletions.
118 changes: 53 additions & 65 deletions lightning/src/chain/channelmonitor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ pub struct ChannelMonitorUpdate {

/// LDK prior to 0.1 used this constant as the [`ChannelMonitorUpdate::update_id`] for any
/// [`ChannelMonitorUpdate`]s which were generated after the channel was closed.
const LEGACY_CLOSED_CHANNEL_UPDATE_ID: u64 = core::u64::MAX;
const LEGACY_CLOSED_CHANNEL_UPDATE_ID: u64 = u64::MAX;

impl Writeable for ChannelMonitorUpdate {
fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
Expand Down Expand Up @@ -285,7 +285,7 @@ impl_writeable_tlv_based!(HolderSignedTx, {
(0, txid, required),
// Note that this is filled in with data from OnchainTxHandler if it's missing.
// For HolderSignedTx objects serialized with 0.0.100+, this should be filled in.
(1, to_self_value_sat, (default_value, u64::max_value())),
(1, to_self_value_sat, (default_value, u64::MAX)),
(2, revocation_key, required),
(4, a_htlc_key, required),
(6, b_htlc_key, required),
Expand All @@ -298,7 +298,7 @@ impl_writeable_tlv_based!(HolderSignedTx, {
impl HolderSignedTx {
fn non_dust_htlcs(&self) -> Vec<HTLCOutputInCommitment> {
self.htlc_outputs.iter().filter_map(|(htlc, _, _)| {
if let Some(_) = htlc.transaction_output_index {
if htlc.transaction_output_index.is_some() {
Some(htlc.clone())
} else {
None
Expand All @@ -319,7 +319,7 @@ struct CounterpartyCommitmentParameters {

impl Writeable for CounterpartyCommitmentParameters {
fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
w.write_all(&(0 as u64).to_be_bytes())?;
w.write_all(&0u64.to_be_bytes())?;
write_tlv_fields!(w, {
(0, self.counterparty_delayed_payment_base_key, required),
(2, self.counterparty_htlc_base_key, required),
Expand All @@ -334,7 +334,7 @@ impl Readable for CounterpartyCommitmentParameters {
// Versions prior to 0.0.100 had some per-HTLC state stored here, which is no longer
// used. Read it for compatibility.
let per_htlc_len: u64 = Readable::read(r)?;
for _ in 0..per_htlc_len {
for _ in 0..per_htlc_len {
let _txid: Txid = Readable::read(r)?;
let htlcs_count: u64 = Readable::read(r)?;
for _ in 0..htlcs_count {
Expand Down Expand Up @@ -791,13 +791,13 @@ struct IrrevocablyResolvedHTLC {
payment_preimage: Option<PaymentPreimage>,
}

// In LDK versions prior to 0.0.111 commitment_tx_output_idx was not Option-al and
// IrrevocablyResolvedHTLC objects only existed for non-dust HTLCs. This was a bug, but to maintain
// backwards compatibility we must ensure we always write out a commitment_tx_output_idx field,
// using `u32::max_value()` as a sentinal to indicate the HTLC was dust.
/// In LDK versions prior to 0.0.111 commitment_tx_output_idx was not Option-al and
/// IrrevocablyResolvedHTLC objects only existed for non-dust HTLCs. This was a bug, but to maintain
/// backwards compatibility we must ensure we always write out a commitment_tx_output_idx field,
/// using [`u32::MAX`] as a sentinal to indicate the HTLC was dust.
impl Writeable for IrrevocablyResolvedHTLC {
fn write<W: Writer>(&self, writer: &mut W) -> Result<(), io::Error> {
let mapped_commitment_tx_output_idx = self.commitment_tx_output_idx.unwrap_or(u32::max_value());
let mapped_commitment_tx_output_idx = self.commitment_tx_output_idx.unwrap_or(u32::MAX);
write_tlv_fields!(writer, {
(0, mapped_commitment_tx_output_idx, required),
(1, self.resolving_txid, option),
Expand All @@ -821,7 +821,7 @@ impl Readable for IrrevocablyResolvedHTLC {
(3, resolving_tx, option),
});
Ok(Self {
commitment_tx_output_idx: if mapped_commitment_tx_output_idx == u32::max_value() { None } else { Some(mapped_commitment_tx_output_idx) },
commitment_tx_output_idx: if mapped_commitment_tx_output_idx == u32::MAX { None } else { Some(mapped_commitment_tx_output_idx) },
resolving_txid,
payment_preimage,
resolving_tx,
Expand Down Expand Up @@ -1581,7 +1581,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
filter.register_tx(&lock.get_funding_txo().0.txid, &lock.get_funding_txo().1);
for (txid, outputs) in lock.get_outputs_to_watch().iter() {
for (index, script_pubkey) in outputs.iter() {
assert!(*index <= u16::max_value() as u32);
assert!(*index <= u16::MAX as u32);
let outpoint = OutPoint { txid: *txid, index: *index as u16 };
log_trace!(logger, "Registering outpoint {} with the filter for monitoring spends", outpoint);
filter.register_output(WatchedOutput {
Expand Down Expand Up @@ -2002,18 +2002,16 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
let current_height = self.current_best_block().height;
let mut inner = self.inner.lock().unwrap();

if is_all_funds_claimed {
if !inner.funding_spend_seen {
debug_assert!(false, "We should see funding spend by the time a monitor clears out");
is_all_funds_claimed = false;
}
if is_all_funds_claimed && !inner.funding_spend_seen {
debug_assert!(false, "We should see funding spend by the time a monitor clears out");
is_all_funds_claimed = false;
}

const BLOCKS_THRESHOLD: u32 = 4032; // ~four weeks
match (inner.balances_empty_height, is_all_funds_claimed) {
(Some(balances_empty_height), true) => {
// Claimed all funds, check if reached the blocks threshold.
return current_height >= balances_empty_height + BLOCKS_THRESHOLD;
current_height >= balances_empty_height + BLOCKS_THRESHOLD
},
(Some(_), false) => {
// previously assumed we claimed all funds, but we have new funds to claim.
Expand Down Expand Up @@ -2065,8 +2063,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
holder_commitment: bool, counterparty_revoked_commitment: bool,
confirmed_txid: Option<Txid>
) -> Option<Balance> {
let htlc_commitment_tx_output_idx =
if let Some(v) = htlc.transaction_output_index { v } else { return None; };
let htlc_commitment_tx_output_idx = htlc.transaction_output_index?;

let mut htlc_spend_txid_opt = None;
let mut htlc_spend_tx_opt = None;
Expand Down Expand Up @@ -2116,14 +2113,14 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
}
}
let htlc_resolved = self.htlcs_resolved_on_chain.iter()
.find(|v| if v.commitment_tx_output_idx == Some(htlc_commitment_tx_output_idx) {
.any(|v| if v.commitment_tx_output_idx == Some(htlc_commitment_tx_output_idx) {
debug_assert!(htlc_spend_txid_opt.is_none());
htlc_spend_txid_opt = v.resolving_txid.as_ref();
debug_assert!(htlc_spend_tx_opt.is_none());
htlc_spend_tx_opt = v.resolving_tx.as_ref();
true
} else { false });
debug_assert!(holder_timeout_spend_pending.is_some() as u8 + htlc_spend_pending.is_some() as u8 + htlc_resolved.is_some() as u8 <= 1);
debug_assert!(holder_timeout_spend_pending.is_some() as u8 + htlc_spend_pending.is_some() as u8 + htlc_resolved as u8 <= 1);

let htlc_commitment_outpoint = BitcoinOutPoint::new(confirmed_txid.unwrap(), htlc_commitment_tx_output_idx);
let htlc_output_to_spend =
Expand Down Expand Up @@ -2154,31 +2151,31 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
confirmation_height: conf_thresh,
source: BalanceSource::Htlc,
});
} else if htlc_resolved.is_some() && !htlc_output_spend_pending {
} else if htlc_resolved && !htlc_output_spend_pending {
// Funding transaction spends should be fully confirmed by the time any
// HTLC transactions are resolved, unless we're talking about a holder
// commitment tx, whose resolution is delayed until the CSV timeout is
// reached, even though HTLCs may be resolved after only
// ANTI_REORG_DELAY confirmations.
debug_assert!(holder_commitment || self.funding_spend_confirmed.is_some());
} else if counterparty_revoked_commitment {
let htlc_output_claim_pending = self.onchain_events_awaiting_threshold_conf.iter().find_map(|event| {
let htlc_output_claim_pending = self.onchain_events_awaiting_threshold_conf.iter().any(|event| {
if let OnchainEvent::MaturingOutput {
descriptor: SpendableOutputDescriptor::StaticOutput { .. }
} = &event.event {
if event.transaction.as_ref().map(|tx| tx.input.iter().any(|inp| {
event.transaction.as_ref().map(|tx| tx.input.iter().any(|inp| {
if let Some(htlc_spend_txid) = htlc_spend_txid_opt {
tx.compute_txid() == *htlc_spend_txid || inp.previous_output.txid == *htlc_spend_txid
} else {
Some(inp.previous_output.txid) == confirmed_txid &&
inp.previous_output.vout == htlc_commitment_tx_output_idx
}
})).unwrap_or(false) {
Some(())
} else { None }
} else { None }
})).unwrap_or(false)
} else {
false
}
});
if htlc_output_claim_pending.is_some() {
if htlc_output_claim_pending {
// We already push `Balance`s onto the `res` list for every
// `StaticOutput` in a `MaturingOutput` in the revoked
// counterparty commitment transaction case generally, so don't
Expand Down Expand Up @@ -2239,7 +2236,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
payment_preimage: *payment_preimage,
});
}
} else if htlc_resolved.is_none() {
} else if !htlc_resolved {
return Some(Balance::MaybePreimageClaimableHTLC {
amount_satoshis: htlc.amount_msat / 1000,
expiry_height: htlc.cltv_expiry,
Expand Down Expand Up @@ -3191,10 +3188,8 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
// confirmed (even with 1 confirmation) as it'll be rejected as
// duplicate/conflicting.
let detected_funding_spend = self.funding_spend_confirmed.is_some() ||
self.onchain_events_awaiting_threshold_conf.iter().find(|event| match event.event {
OnchainEvent::FundingSpendConfirmation { .. } => true,
_ => false,
}).is_some();
self.onchain_events_awaiting_threshold_conf.iter().any(
|event| matches!(event.event, OnchainEvent::FundingSpendConfirmation { .. }));
if detected_funding_spend {
log_trace!(logger, "Avoiding commitment broadcast, already detected confirmed spend onchain");
continue;
Expand Down Expand Up @@ -3268,7 +3263,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
// If we've detected a counterparty commitment tx on chain, we must include it in the set
// of outputs to watch for spends of, otherwise we're likely to lose user funds. Because
// its trivial to do, double-check that here.
for (txid, _) in self.counterparty_commitment_txn_on_chain.iter() {
for txid in self.counterparty_commitment_txn_on_chain.keys() {
self.outputs_to_watch.get(txid).expect("Counterparty commitment txn which have been broadcast should have outputs registered");
}
&self.outputs_to_watch
Expand Down Expand Up @@ -4118,16 +4113,10 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
}

// Find which on-chain events have reached their confirmation threshold.
let onchain_events_awaiting_threshold_conf =
self.onchain_events_awaiting_threshold_conf.drain(..).collect::<Vec<_>>();
let mut onchain_events_reaching_threshold_conf = Vec::new();
for entry in onchain_events_awaiting_threshold_conf {
if entry.has_reached_confirmation_threshold(&self.best_block) {
onchain_events_reaching_threshold_conf.push(entry);
} else {
self.onchain_events_awaiting_threshold_conf.push(entry);
}
}
let (onchain_events_reaching_threshold_conf, onchain_events_awaiting_threshold_conf): (Vec<_>, Vec<_>) =
self.onchain_events_awaiting_threshold_conf.drain(..).partition(
|entry| entry.has_reached_confirmation_threshold(&self.best_block));
self.onchain_events_awaiting_threshold_conf = onchain_events_awaiting_threshold_conf;

// Used to check for duplicate HTLC resolutions.
#[cfg(debug_assertions)]
Expand All @@ -4142,19 +4131,19 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
let mut matured_htlcs = Vec::new();

// Produce actionable events from on-chain events having reached their threshold.
for entry in onchain_events_reaching_threshold_conf.drain(..) {
for entry in onchain_events_reaching_threshold_conf {
match entry.event {
OnchainEvent::HTLCUpdate { ref source, payment_hash, htlc_value_satoshis, commitment_tx_output_idx } => {
OnchainEvent::HTLCUpdate { source, payment_hash, htlc_value_satoshis, commitment_tx_output_idx } => {
// Check for duplicate HTLC resolutions.
#[cfg(debug_assertions)]
{
debug_assert!(
unmatured_htlcs.iter().find(|&htlc| htlc == &source).is_none(),
!unmatured_htlcs.contains(&&source),
"An unmature HTLC transaction conflicts with a maturing one; failed to \
call either transaction_unconfirmed for the conflicting transaction \
or block_disconnected for a block containing it.");
debug_assert!(
matured_htlcs.iter().find(|&htlc| htlc == source).is_none(),
!matured_htlcs.contains(&source),
"A matured HTLC transaction conflicts with a maturing one; failed to \
call either transaction_unconfirmed for the conflicting transaction \
or block_disconnected for a block containing it.");
Expand All @@ -4166,7 +4155,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
self.pending_monitor_events.push(MonitorEvent::HTLCEvent(HTLCUpdate {
payment_hash,
payment_preimage: None,
source: source.clone(),
source,
htlc_value_satoshis,
}));
self.htlcs_resolved_on_chain.push(IrrevocablyResolvedHTLC {
Expand Down Expand Up @@ -4211,7 +4200,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
});
#[cfg(test)]
{
// If we see a transaction for which we registered outputs previously,
// If we see a transaction for which we registered outputs previously,
// make sure the registered scriptpubkey at the expected index match
// the actual transaction output one. We failed this case before #653.
for tx in &txn_matched {
Expand Down Expand Up @@ -4596,7 +4585,8 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
height,
block_hash: Some(*block_hash),
event: OnchainEvent::HTLCUpdate {
source, payment_hash,
source,
payment_hash,
htlc_value_satoshis: Some(amount_msat / 1000),
commitment_tx_output_idx: Some(input.previous_output.vout),
},
Expand Down Expand Up @@ -4808,7 +4798,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
for _ in 0..htlcs_count {
htlcs.push((read_htlc_in_commitment!(), <Option<HTLCSource> as Readable>::read(reader)?.map(|o: HTLCSource| Box::new(o))));
}
if let Some(_) = counterparty_claimable_outpoints.insert(txid, htlcs) {
if counterparty_claimable_outpoints.insert(txid, htlcs).is_some() {
return Err(DecodeError::InvalidValue);
}
}
Expand All @@ -4818,7 +4808,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
for _ in 0..counterparty_commitment_txn_on_chain_len {
let txid: Txid = Readable::read(reader)?;
let commitment_number = <U48 as Readable>::read(reader)?.0;
if let Some(_) = counterparty_commitment_txn_on_chain.insert(txid, commitment_number) {
if counterparty_commitment_txn_on_chain.insert(txid, commitment_number).is_some() {
return Err(DecodeError::InvalidValue);
}
}
Expand All @@ -4828,17 +4818,15 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
for _ in 0..counterparty_hash_commitment_number_len {
let payment_hash: PaymentHash = Readable::read(reader)?;
let commitment_number = <U48 as Readable>::read(reader)?.0;
if let Some(_) = counterparty_hash_commitment_number.insert(payment_hash, commitment_number) {
if counterparty_hash_commitment_number.insert(payment_hash, commitment_number).is_some() {
return Err(DecodeError::InvalidValue);
}
}

let mut prev_holder_signed_commitment_tx: Option<HolderSignedTx> =
match <u8 as Readable>::read(reader)? {
0 => None,
1 => {
Some(Readable::read(reader)?)
},
1 => Some(Readable::read(reader)?),
_ => return Err(DecodeError::InvalidValue),
};
let mut current_holder_commitment_tx: HolderSignedTx = Readable::read(reader)?;
Expand All @@ -4851,7 +4839,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
for _ in 0..payment_preimages_len {
let preimage: PaymentPreimage = Readable::read(reader)?;
let hash = PaymentHash(Sha256::hash(&preimage.0[..]).to_byte_array());
if let Some(_) = payment_preimages.insert(hash, (preimage, Vec::new())) {
if payment_preimages.insert(hash, (preimage, Vec::new())).is_some() {
return Err(DecodeError::InvalidValue);
}
}
Expand Down Expand Up @@ -4895,7 +4883,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
for _ in 0..outputs_len {
outputs.push((Readable::read(reader)?, Readable::read(reader)?));
}
if let Some(_) = outputs_to_watch.insert(txid, outputs) {
if outputs_to_watch.insert(txid, outputs).is_some() {
return Err(DecodeError::InvalidValue);
}
}
Expand All @@ -4909,15 +4897,15 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
if let Some(prev_commitment_tx) = prev_holder_signed_commitment_tx.as_mut() {
let prev_holder_value = onchain_tx_handler.get_prev_holder_commitment_to_self_value();
if prev_holder_value.is_none() { return Err(DecodeError::InvalidValue); }
if prev_commitment_tx.to_self_value_sat == u64::max_value() {
if prev_commitment_tx.to_self_value_sat == u64::MAX {
prev_commitment_tx.to_self_value_sat = prev_holder_value.unwrap();
} else if prev_commitment_tx.to_self_value_sat != prev_holder_value.unwrap() {
return Err(DecodeError::InvalidValue);
}
}

let cur_holder_value = onchain_tx_handler.get_cur_holder_commitment_to_self_value();
if current_holder_commitment_tx.to_self_value_sat == u64::max_value() {
if current_holder_commitment_tx.to_self_value_sat == u64::MAX {
current_holder_commitment_tx.to_self_value_sat = cur_holder_value;
} else if current_holder_commitment_tx.to_self_value_sat != cur_holder_value {
return Err(DecodeError::InvalidValue);
Expand Down Expand Up @@ -5259,7 +5247,7 @@ mod tests {
delayed_payment_basepoint: DelayedPaymentBasepoint::from(PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[47; 32]).unwrap())),
htlc_basepoint: HtlcBasepoint::from(PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[48; 32]).unwrap()))
};
let funding_outpoint = OutPoint { txid: Txid::all_zeros(), index: u16::max_value() };
let funding_outpoint = OutPoint { txid: Txid::all_zeros(), index: u16::MAX };
let channel_id = ChannelId::v1_from_funding_outpoint(funding_outpoint);
let channel_parameters = ChannelTransactionParameters {
holder_pubkeys: keys.holder_channel_pubkeys.clone(),
Expand Down Expand Up @@ -5511,7 +5499,7 @@ mod tests {
delayed_payment_basepoint: DelayedPaymentBasepoint::from(PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[47; 32]).unwrap())),
htlc_basepoint: HtlcBasepoint::from(PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[48; 32]).unwrap())),
};
let funding_outpoint = OutPoint { txid: Txid::all_zeros(), index: u16::max_value() };
let funding_outpoint = OutPoint { txid: Txid::all_zeros(), index: u16::MAX };
let channel_id = ChannelId::v1_from_funding_outpoint(funding_outpoint);
let channel_parameters = ChannelTransactionParameters {
holder_pubkeys: keys.holder_channel_pubkeys.clone(),
Expand Down

0 comments on commit 726dd5c

Please sign in to comment.