Skip to content

Clean channelmonitor.rs code #3429

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 4, 2024
Merged

Clean channelmonitor.rs code #3429

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
553e6c7
Clean channelmonitor.rs code
andrei-21 Nov 27, 2024
8ebda06
Turn `IrrevocablyResolvedHTLC` comment into a Rust comment
andrei-21 Dec 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 53 additions & 65 deletions lightning/src/chain/channelmonitor.rs
View file
Open in desktop
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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is gonna conflict with #3413.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean here? I only dropped core:: prefix, you are not touching this line in your PR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, I had my PRs confused. The one I was thinking of already landed lol

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(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds an assumption that onchain_events_awaiting_threshold_conf is sorted by when events will reach their confirmation threshold. That may be the case in practice (its not entirely clear to me from the code), but I'm not super happy with adding that assumption here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, how? partition() does not make such an assumption:

partition() returns a pair, all of the elements for which it returned true, and all of the elements for which it returned false.

https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.partition

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's a terrible name for that function lol.

|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
Loading