Skip to content
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.

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

Add PersistClaimInfo and ClaimInfoRequest events #3067

Closed
Changes from 1 commit
Commits
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
164 changes: 161 additions & 3 deletions lightning/src/events/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,16 +35,19 @@ use crate::util::errors::APIError;
use crate::util::ser::{BigSize, FixedLengthReader, Writeable, Writer, MaybeReadable, Readable, RequiredWrapper, UpgradableRequired, WithoutLength};
use crate::util::string::UntrustedString;

use bitcoin::{Transaction, OutPoint};
use bitcoin::{Transaction, OutPoint, Txid, BlockHash};
use bitcoin::blockdata::locktime::absolute::LockTime;
use bitcoin::blockdata::script::ScriptBuf;
use bitcoin::hashes::Hash;
use bitcoin::hashes::sha256::Hash as Sha256;
use bitcoin::secp256k1::PublicKey;
use bitcoin::transaction::Version;
use crate::io;
use crate::io::Read;
use core::time::Duration;
use core::ops::Deref;
use crate::ln::chan_utils::HTLCOutputInCommitment;
use crate::ln::msgs::DecodeError;
use crate::sync::Arc;

#[allow(unused_imports)]
Expand Down Expand Up @@ -954,9 +957,9 @@ pub enum Event {
/// If this is `Some`, then the corresponding channel should be avoided when the payment is
/// retried. May be `None` for older [`Event`] serializations.
short_channel_id: Option<u64>,
#[cfg(test)]
#[cfg(test)]
error_code: Option<u16>,
#[cfg(test)]
#[cfg(test)]
error_data: Option<Vec<u8>>,
},
/// Indicates that a probe payment we sent returned successful, i.e., only failed at the destination.
Expand Down Expand Up @@ -1331,6 +1334,60 @@ pub enum Event {
/// Destination of the HTLC that failed to be processed.
failed_next_destination: HTLCDestination,
},
/// Used to request [`ClaimInfo`] for a specific counterparty commitment transaction.
///
/// This event is generated when there is a need for [`ClaimInfo`] that was previously stored
/// using [`PersistClaimInfo`] for the specified counterparty commitment transaction.
/// This event can be safely ignored if [`PersistClaimInfo`] event is being ignored.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This event can never be ignored, cause if it is generated PersistClaimInfo is being handled.

Suggested change
/// This event can be safely ignored if [`PersistClaimInfo`] event is being ignored.
/// This event will never be generated if the [`PersistClaimInfo`] event is being ignored.

///
/// The response to this event should be handled by calling
/// [`ChainMonitor::provide_claim_info`] with the [`ClaimInfo`] that was previously stored and
/// [`ClaimMetadata`] from this event.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably have some kind of advice for what users should do if they don't find the claim info on disk - I assume they should probably treat it as if they definitely just lost funds (panic or whatever)?

///
/// [`ChainMonitor::provide_claim_info`]: crate::chain::chainmonitor::ChainMonitor::provide_claim_info
/// [`PersistClaimInfo`]: Event::PersistClaimInfo
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: No need to add this reference if in the same module. Then again, we may want to move the new structs to ChainMonitor?

ClaimInfoRequest {
Copy link
Contributor

Choose a reason for hiding this comment

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

The names of other event types usually either reflect that something happened (PaymentClaimed/PaymentFailed) or that something needs to be done (DiscardFunding, ConnectionNeeded). ClaimInfoRequest (and to an extend also PersistClaimInfo) doesn't make it easy to infer whether it's simply informational or if the user needs to take action. Could this be improved?

/// The [`OutPoint`] identifying the channel monitor with which this claim is associated.
monitor_id: transaction::OutPoint,
/// The transaction identifier for which [`ClaimInfo`] is requested.
claim_key: Txid,
/// Additional metadata that must be supplied in the call to [`ChainMonitor::provide_claim_info`].
///
/// [`ChainMonitor::provide_claim_info`]: crate::chain::chainmonitor::ChainMonitor::provide_claim_info
claim_metadata: ClaimMetadata,
Copy link
Contributor

Choose a reason for hiding this comment

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

ClaimInfo and ClaimMetadata are very similar and very generic names. Can we make them a bit more specific, i.e., reflect their purpose a bit better?

},

/// Indicates that [`ClaimInfo`] should be durably persisted.
///
/// This event is used to persist information regarding a claim, necessary to generate revocation
/// transactions. Persisted [`ClaimInfo`] can be requested at a later time using [`ClaimInfoRequest`]
/// when we need to check for counterparty spend transactions.
/// It is recommended to store [`ClaimInfo`] against a combination key of `monitor_id` and
/// `claim_key` for effective retrieval after a [`ClaimInfoRequest`].
/// After successfully persisting the claim information, [`ChainMonitor::claim_info_persisted`]
/// must be called to notify the system that the data has been successfully stored.
///
/// Calling [`ChainMonitor::claim_info_persisted`] results in the removal of the [`ClaimInfo`] from both
/// the in-memory and on-disk versions of the [`ChannelMonitor`]. This action reduces the memory
/// footprint of the [`ChannelMonitor`], optimizing both process memory and disk usage.
/// This event can be safely ignored if such optimization is not desired;
/// however, if ignored, the [`ClaimInfo`] will continue to be stored both in-memory and on-disk
/// for [`ChannelMonitor`].
///
/// [`ChainMonitor::claim_info_persisted`]: crate::chain::chainmonitor::ChainMonitor::claim_info_persisted
/// [`ChannelMonitor`]: crate::chain::channelmonitor::ChannelMonitor
/// [`ClaimInfoRequest`]: Event::ClaimInfoRequest
PersistClaimInfo {
/// The [`OutPoint`] identifying the channel monitor with which this claim is associated.
monitor_id: transaction::OutPoint,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be called funding_txo? Do we already now how this will work in the context of splicing? If this is indeed a monitor_id, should we use an opaque MonitorId type that would allow us to change its semantics internally in the future? If not, can we make this a bitcoin::OutPoint to avoid mixing the two different OutPoint types across our public API?

/// The transaction identifier against which [`ClaimInfo`] is to be persisted.
claim_key: Txid,
/// Claim related information necessary to generate revocation transactions, that must be durably
/// persisted before calling [`ChainMonitor::claim_info_persisted`].
///
/// [`ChainMonitor::claim_info_persisted`]: crate::chain::chainmonitor::ChainMonitor::claim_info_persisted
claim_info: ClaimInfo,
},
/// Indicates that a transaction originating from LDK needs to have its fee bumped. This event
/// requires confirmed external funds to be readily available to spend.
///
Expand Down Expand Up @@ -1378,6 +1435,75 @@ pub enum Event {
}
}

/// Metadata associated for generating a claim when broadcast of commitment transaction is detected.
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct ClaimMetadata {
/// The [`BlockHash`] of the block containing the related commitment transaction.
pub(crate) block_hash: BlockHash,
/// The counterparty commitment transaction for which a claim might be necessary.
pub(crate) tx: Transaction,
/// The height of the block in which the commitment transaction is included.
pub(crate) height: u32,
}

/// Represents detailed information about HTLCs in a commitment transaction that can be claimed.
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct ClaimInfo {
/// A list of HTLC outputs for the commitment transaction that are eligible for claiming.
pub(crate) htlcs: Vec<HTLCOutputInCommitment>,
}

impl Writeable for ClaimInfo {
fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
write_tlv_fields!(w, {
(1, self.htlcs, required_vec),
});
Ok(())
}
}

impl Readable for ClaimInfo {
fn read<R: Read>(reader: &mut R) -> Result<Self, DecodeError> {
_init_and_read_len_prefixed_tlv_fields!(reader, {
(1, htlcs, required_vec),
});

Ok(ClaimInfo {
htlcs: htlcs,
})
}
}

impl Writeable for ClaimMetadata {
fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
write_tlv_fields!(w, {
(1, self.block_hash, required),
(3, self.tx, required),
(5, self.height, required),
});
Ok(())
}
}

impl Readable for ClaimMetadata {
fn read<R: Read>(reader: &mut R) -> Result<Self, DecodeError> {
let mut block_hash = RequiredWrapper(None);
let mut tx = RequiredWrapper(None);
let mut height = RequiredWrapper(None);
read_tlv_fields!(reader, {
(1, block_hash, required),
(3, tx, required),
(5, height, required),
});

Ok(ClaimMetadata {
block_hash: block_hash.0.unwrap(),
tx: tx.0.unwrap(),
height: height.0.unwrap(),
})
}
}

impl Writeable for Event {
fn write<W: Writer>(&self, writer: &mut W) -> Result<(), io::Error> {
match self {
Expand Down Expand Up @@ -1672,6 +1798,18 @@ impl Writeable for Event {
(8, former_temporary_channel_id, required),
});
},
&Event::ClaimInfoRequest { ref monitor_id, ref claim_key, ref claim_metadata } => {
45u8.write(writer)?;
write_tlv_fields!(writer, {
(0, monitor_id, required),
(2, claim_key, required),
(4, claim_metadata, required),
});
}
&Event::PersistClaimInfo { .. } => {
47u8.write(writer)?;
// We don't write `PersistClaimInfo` because it is ok if we lost it, and it may be replayed.
},
// Note that, going forward, all new events must only write data inside of
// `write_tlv_fields`. Versions 0.0.101+ will ignore odd-numbered events that write
// data via `write_tlv_fields`.
Expand Down Expand Up @@ -2146,6 +2284,26 @@ impl MaybeReadable for Event {
former_temporary_channel_id: former_temporary_channel_id.0.unwrap(),
}))
},
45u8 => {
let mut f = || {
let mut monitor_id = RequiredWrapper(None);
let mut claim_key = RequiredWrapper(None);
let mut claim_metadata = RequiredWrapper(None);
read_tlv_fields!(reader, {
(0, monitor_id, required),
(2, claim_key, required),
(4, claim_metadata, required),
});

Ok(Some(Event::ClaimInfoRequest {
monitor_id: monitor_id.0.unwrap(),
claim_key: claim_key.0.unwrap(),
claim_metadata: claim_metadata.0.unwrap(),
}))
};
f()
},
47u8 => Ok(None),
// Versions prior to 0.0.100 did not ignore odd types, instead returning InvalidValue.
// Version 0.0.100 failed to properly ignore odd types, possibly resulting in corrupt
// reads.
Expand Down
Loading