Add PersistClaimInfo and ClaimInfoRequest events #3067

G8XSU · 2024-05-16T00:30:25Z

Based on #3057

Introduce two new events i.e. PersistClaimInfo and ClaimInfoRequest.

PersistClaimInfo: Indicates that [ClaimInfo] should be durably persisted.

ClaimInfoRequest: Used to request [ClaimInfo] for a specific counterparty commitment transaction.

This PR is still in progress as event writing changes are pending and is not in mergeable state but the api and interface can still be reviewed. Mostly ignore docs, they should be legible but need some more work.

As part of #3049

codecov-commenter · 2024-05-16T00:37:44Z

Codecov Report

Attention: Patch coverage is 0% with 19 lines in your changes missing coverage. Please review.

Project coverage is 89.70%. Comparing base (50d21b7) to head (7fad4ee).
Report is 357 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/events/mod.rs	0.00%	19 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3067      +/-   ##
==========================================
- Coverage   89.75%   89.70%   -0.06%     
==========================================
  Files         122      122              
  Lines      101791   101810      +19     
  Branches   101791   101810      +19     
==========================================
- Hits        91367    91327      -40     
- Misses       7743     7795      +52     
- Partials     2681     2688       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lightning/src/events/mod.rs

TheBlueMatt

Makes sense to me. We'll definitely want some code that uses this so we can check that ClaimMetadata has the right contents before we land.

lightning/src/events/mod.rs

TheBlueMatt

Feel free to squash fixups, but we shouldn't land this as-is without cfg-tags, but honestly IMO we should just land this as a part of a later PR that handles these events.

lightning/src/events/mod.rs

TheBlueMatt · 2024-08-06T18:26:25Z

lightning/src/events/mod.rs

+	///
+	/// 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.


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.

lightning/src/events/mod.rs

TheBlueMatt · 2024-08-06T18:30:29Z

Also, please include at least a bit of detail in the commit messages.

TheBlueMatt · 2024-08-06T18:48:44Z

lightning/src/events/mod.rs

+	///
+	/// 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.


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)?

lightning/src/events/mod.rs

tnull · 2024-08-07T10:09:15Z

lightning/src/events/mod.rs

+	/// [`ClaimMetadata`] from this event.
+	///
+	/// [`ChainMonitor::provide_claim_info`]: crate::chain::chainmonitor::ChainMonitor::provide_claim_info
+	/// [`PersistClaimInfo`]: Event::PersistClaimInfo


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

tnull · 2024-08-07T10:14:23Z

lightning/src/events/mod.rs

+	/// [`ClaimInfoRequest`]: Event::ClaimInfoRequest
+	PersistClaimInfo {
+		/// The [`OutPoint`] identifying the channel monitor with which this claim is associated.
+		monitor_id: transaction::OutPoint,


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?

tnull · 2024-08-07T10:18:13Z

lightning/src/events/mod.rs

+		/// 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,


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?

tnull · 2024-08-07T10:21:06Z

lightning/src/events/mod.rs

+	///
+	/// [`ChainMonitor::provide_claim_info`]: crate::chain::chainmonitor::ChainMonitor::provide_claim_info
+	/// [`PersistClaimInfo`]: Event::PersistClaimInfo
+	ClaimInfoRequest {


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?

lightning/src/events/mod.rs

tnull · 2024-08-08T12:41:31Z

This needs a rebase now.

G8XSU · 2024-08-28T20:07:37Z

Merging this into #3106 as per feedback.
Addressed most of the comments here, some might be unanswered.
Will move some questions to 3106, and close the rest here.

tnull · 2024-08-29T07:19:21Z

Merging this into #3106 as per feedback.

Should this PR get closed then?

G8XSU · 2024-08-29T07:21:27Z

Yes i will close this after answering some comments or moving them to new pr.

G8XSU commented May 16, 2024

View reviewed changes

lightning/src/events/mod.rs Outdated Show resolved Hide resolved

G8XSU force-pushed the 2024-05-08-claimable-persist-3049-events branch 4 times, most recently from 2c9e408 to a34b67e Compare May 17, 2024 19:31

G8XSU mentioned this pull request May 17, 2024

[Memory] Persist counterparty_claimable_outpoints out of channel_monitor #3049

Open

9 tasks

TheBlueMatt reviewed May 19, 2024

View reviewed changes

lightning/src/events/mod.rs Show resolved Hide resolved

lightning/src/events/mod.rs Outdated Show resolved Hide resolved

lightning/src/events/mod.rs Outdated Show resolved Hide resolved

G8XSU mentioned this pull request Jun 6, 2024

Persist counterparty_claimable_outpoints out of channel_monitor #3106

Open

Add PersistClaimInfo and ClaimInfoRequest events

8a56137

G8XSU force-pushed the 2024-05-08-claimable-persist-3049-events branch from a34b67e to 8a56137 Compare July 30, 2024 23:16

G8XSU changed the title ~~[WIP] Add PersistClaimInfo and ClaimInfoRequest events~~ Add PersistClaimInfo and ClaimInfoRequest events Jul 30, 2024

G8XSU marked this pull request as ready for review July 30, 2024 23:20

G8XSU mentioned this pull request Aug 1, 2024

Add functions to handle ClaimInfoRequest and PersistClaimInfo events. #3217

Closed

G8XSU added 3 commits August 2, 2024 16:40

Use impl_writeable macro

0e8d297

Add doc about Failure Behavior and Persistence

060db20

fix doc to not use used to

7fad4ee

TheBlueMatt reviewed Aug 6, 2024

View reviewed changes

tnull reviewed Aug 7, 2024

View reviewed changes

G8XSU closed this Sep 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PersistClaimInfo and ClaimInfoRequest events #3067

Add PersistClaimInfo and ClaimInfoRequest events #3067

G8XSU commented May 16, 2024 •

edited

Loading

codecov-commenter commented May 16, 2024 •

edited by codecov bot

Loading

TheBlueMatt left a comment

TheBlueMatt left a comment

TheBlueMatt Aug 6, 2024

TheBlueMatt commented Aug 6, 2024

TheBlueMatt Aug 6, 2024

tnull Aug 7, 2024

tnull Aug 7, 2024

tnull Aug 7, 2024

tnull Aug 7, 2024

tnull commented Aug 8, 2024

G8XSU commented Aug 28, 2024

tnull commented Aug 29, 2024

G8XSU commented Aug 29, 2024

	/// 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.

Add PersistClaimInfo and ClaimInfoRequest events #3067

Add PersistClaimInfo and ClaimInfoRequest events #3067

Conversation

G8XSU commented May 16, 2024 • edited Loading

codecov-commenter commented May 16, 2024 • edited by codecov bot Loading

Codecov Report

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt Aug 6, 2024

Choose a reason for hiding this comment

TheBlueMatt commented Aug 6, 2024

TheBlueMatt Aug 6, 2024

Choose a reason for hiding this comment

tnull Aug 7, 2024

Choose a reason for hiding this comment

tnull Aug 7, 2024

Choose a reason for hiding this comment

tnull Aug 7, 2024

Choose a reason for hiding this comment

tnull Aug 7, 2024

Choose a reason for hiding this comment

tnull commented Aug 8, 2024

G8XSU commented Aug 28, 2024

tnull commented Aug 29, 2024

G8XSU commented Aug 29, 2024

G8XSU commented May 16, 2024 •

edited

Loading

codecov-commenter commented May 16, 2024 •

edited by codecov bot

Loading