Don't pause events for chainsync persistence #2957

G8XSU · 2024-03-21T03:35:57Z

This change was introduced in 5c2ff2cb30ef1639c80b275eea209a289dd91b77 (#1108)
and the main reason mentioned is to avoid duplicate payment events.

Duplicate payment events should be expected by clients and handled in an idempotent manner.

Removing this holding up of events simplifies the logic and makes it easier to do #2647 in which we will stop persisting channelmonitors for every block update.

codecov-commenter · 2024-03-21T05:00:36Z

Codecov Report

Attention: Patch coverage is 98.66667% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 89.35%. Comparing base (9a438ee) to head (4d5de1f).
Report is 41 commits behind head on main.

Files	Patch %	Lines
lightning/src/chain/chainmonitor.rs	97.43%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2957      +/-   ##
==========================================
+ Coverage   89.13%   89.35%   +0.21%     
==========================================
  Files         118      118              
  Lines       97678    98933    +1255     
  Branches    97678    98933    +1255     
==========================================
+ Hits        87066    88401    +1335     
+ Misses       8370     8267     -103     
- Partials     2242     2265      +23

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

tnull · 2024-03-21T09:12:15Z

Duplicate payment events should be expected by clients and handled in an idempotent manner.

I'm not sure if we document that assumption anywhere so far? Could you clarify if this would also pertain inbound payments and PaymentForwarded events? Given that these don't expose a PaymentId I'm not sure we provide the user a good way to handle spurious events in and idempotent manner?

G8XSU · 2024-03-21T17:50:58Z

Could you clarify if this would also pertain inbound payments and PaymentForwarded events? Given that these don't expose a PaymentId I'm not sure we provide the user a good way to handle spurious events in and idempotent manner?

Synced offline, due to current event architecture, we could always get duplicate events after an event is handled and channel-manager isn't immediately persisted.
Inbound payments afaiu, should have payment hash but PaymentForwarded don't have any such identifier.
We can separately consider adding a hash of htlc forwarded in PaymentForwarded

G8XSU · 2024-03-21T17:51:31Z

Where should we fail when persistence fails?

Update: We no longer fail in release_pending_events for this, hence it should not be a concern anymore.

lightning/src/ln/payment_tests.rs

TheBlueMatt

The change itself here looks good, but there's a ton of other changes in ChainMonitor that we should unwind now. I'm pretty sure we can entirely drop UpdateOrigin now (and just use the off-chain monitor update ID), stop tracking any pending monitor updates that came from on-chain, etc.

lightning/src/chain/chainmonitor.rs

TheBlueMatt

Basically LGTM!

lightning/src/chain/chainmonitor.rs

TheBlueMatt · 2024-04-12T18:37:43Z

Needs rebase, also I think the top commit should be squashed down into the places that create the warnings.

TheBlueMatt · 2024-04-15T20:36:07Z

So one thing that came up while I was working with this API in the fuzzer is that its really awkward that the update_id in the ChannelMonitorUpdate is a different type from the update_id passed to the persister (and provided via list_pending_monitor_updates). I'm not sure users are gonna hit that all that often, but it would be really nice if they were the same type (one way or another).

G8XSU · 2024-04-15T21:31:17Z

Discussed couple of things offline,

Will remove concept of MonitorUpdateId altogether, will use u64 everywhere.
Will remove update_id from pub api of persist interface, i.e. both persist_new_channel and update_persisted_channel
Doc to notify channel_monitor_updated using monitor.getlatestUpdateId or update.update_id.
and to not notify when an update is absent in update_persisted_channel

TheBlueMatt

CI is failing and lots of docs are stale.

lightning/src/chain/chainmonitor.rs

We used to wait on ChannelMonitor persistence to avoid duplicate payment events. But this can still happen in cases where ChannelMonitor handed the event to ChannelManager and we did not persist ChannelManager after event handling. It is expected to receive payment duplicate events and clients should handle these events in an idempotent manner. Removing this hold-up of events simplifies the logic and makes it easier to not persist ChannelMonitors on every block connect.

We only used to store last_chain_persist_height to release events held for more than LATENCY_GRACE_PERIOD_BLOCKS due to pending monitor update with UpdateOrigin::ChainSync. Since we no longer pause events for ChainSync persistence, we no longer need to store last_chain_persist_height.

We no longer need to track them since we no longer hold events for pending MonitorUpdates resulting from ChainSync.

MonitorUpdateId was an opaque abstraction for id's generated by UpdateOrigin:Offchain and UpdateOrigin::ChainSync monitor updates. It was mainly needed to map calls made to ChainMonitor::channel_monitor_updated. We no longer track UpdateOrigin::ChainSync MonitorUpdates and can directly use ChannelMonitor::get_latest_update_id() for tracking UpdateOrigin::Offchain monitor updates.

It was used earlier for generating unique MonitorUpdateId for UpdateOrigin::ChainSync monitor updates.

TheBlueMatt

One comment that can be a followup.

lightning/src/util/test_utils.rs

lightning/src/chain/chainmonitor.rs

G8XSU · 2024-05-06T18:09:11Z

Created a followup PR, I can merge that into this, if needed.

Followups to #2957

G8XSU force-pushed the pause-events branch 2 times, most recently from f674099 to 5ea38da Compare March 21, 2024 04:52

TheBlueMatt previously approved these changes Mar 21, 2024

View reviewed changes

lightning/src/ln/payment_tests.rs Outdated Show resolved Hide resolved

G8XSU dismissed TheBlueMatt’s stale review via 74eab55 March 21, 2024 20:34

G8XSU force-pushed the pause-events branch from 74eab55 to 0a3d19f Compare March 21, 2024 23:49

TheBlueMatt reviewed Mar 22, 2024

View reviewed changes

G8XSU mentioned this pull request Mar 25, 2024

Optimize ChannelMonitor persistence on block connections. #2966

Merged

4 tasks

tnull mentioned this pull request Mar 27, 2024

Include a PaymentContext in PaymentPurpose #2970

Merged

TheBlueMatt reviewed Mar 27, 2024

View reviewed changes

lightning/src/chain/chainmonitor.rs Outdated Show resolved Hide resolved

lightning/src/chain/chainmonitor.rs Outdated Show resolved Hide resolved

G8XSU requested a review from TheBlueMatt April 12, 2024 06:51

TheBlueMatt reviewed Apr 12, 2024

View reviewed changes

lightning/src/chain/chainmonitor.rs Outdated Show resolved Hide resolved

lightning/src/chain/chainmonitor.rs Outdated Show resolved Hide resolved

TheBlueMatt added this to the 0.0.124 milestone Apr 12, 2024

G8XSU force-pushed the pause-events branch 6 times, most recently from 1d42927 to e1d5921 Compare April 25, 2024 19:17

TheBlueMatt reviewed Apr 26, 2024

View reviewed changes

lightning/src/chain/chainmonitor.rs Outdated Show resolved Hide resolved

G8XSU force-pushed the pause-events branch 2 times, most recently from 8a10e83 to 45de321 Compare April 26, 2024 22:59

Stop tracking MonitorUpdates from ChainSync in pending_monitor_updates

7de602a

We no longer need to track them since we no longer hold events for pending MonitorUpdates resulting from ChainSync.

G8XSU force-pushed the pause-events branch 2 times, most recently from b72e897 to bd15c45 Compare April 27, 2024 00:21

G8XSU added 2 commits April 26, 2024 20:28

Remove ChainMonitor::sync_persistence_id

4d5de1f

It was used earlier for generating unique MonitorUpdateId for UpdateOrigin::ChainSync monitor updates.

G8XSU force-pushed the pause-events branch from bd15c45 to 4d5de1f Compare April 27, 2024 03:29

G8XSU requested a review from TheBlueMatt April 27, 2024 03:29

TheBlueMatt approved these changes May 6, 2024

View reviewed changes

lightning/src/util/test_utils.rs Show resolved Hide resolved

lightning/src/util/test_utils.rs Show resolved Hide resolved

TheBlueMatt added the Seeking Code Review label May 6, 2024

wpaulino approved these changes May 6, 2024

View reviewed changes

lightning/src/chain/chainmonitor.rs Show resolved Hide resolved

lightning/src/chain/chainmonitor.rs Show resolved Hide resolved

G8XSU added a commit to G8XSU/rust-lightning that referenced this pull request May 6, 2024

Minor Doc fixes after lightningdevkit#2957

4facc5a

G8XSU mentioned this pull request May 6, 2024

Followups to #2957 #3041

Merged

TheBlueMatt merged commit b8d4ac1 into lightningdevkit:main May 6, 2024
15 of 16 checks passed

G8XSU added a commit to G8XSU/rust-lightning that referenced this pull request May 6, 2024

Minor Doc fixes after lightningdevkit#2957

a5933ae

TheBlueMatt added a commit that referenced this pull request May 9, 2024

Merge pull request #3041 from G8XSU/followup-2957

33b9ba5

Followups to #2957

Psycho-Pirate pushed a commit to Psycho-Pirate/rust-lightning that referenced this pull request May 29, 2024

Minor Doc fixes after lightningdevkit#2957

dbf8069

tnull mentioned this pull request Jul 4, 2024

force close is not always broadcasting latest TX #3155

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't pause events for chainsync persistence #2957

Don't pause events for chainsync persistence #2957

G8XSU commented Mar 21, 2024 •

edited

Loading

codecov-commenter commented Mar 21, 2024 •

edited

Loading

tnull commented Mar 21, 2024

G8XSU commented Mar 21, 2024

G8XSU commented Mar 21, 2024

TheBlueMatt left a comment

TheBlueMatt left a comment

TheBlueMatt commented Apr 12, 2024

TheBlueMatt commented Apr 15, 2024

G8XSU commented Apr 15, 2024 •

edited

Loading

TheBlueMatt left a comment

TheBlueMatt left a comment

G8XSU commented May 6, 2024

Don't pause events for chainsync persistence #2957

Don't pause events for chainsync persistence #2957

Conversation

G8XSU commented Mar 21, 2024 • edited Loading

codecov-commenter commented Mar 21, 2024 • edited Loading

Codecov Report

tnull commented Mar 21, 2024

G8XSU commented Mar 21, 2024

G8XSU commented Mar 21, 2024

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Apr 12, 2024

TheBlueMatt commented Apr 15, 2024

G8XSU commented Apr 15, 2024 • edited Loading

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

G8XSU commented May 6, 2024

G8XSU commented Mar 21, 2024 •

edited

Loading

codecov-commenter commented Mar 21, 2024 •

edited

Loading

G8XSU commented Apr 15, 2024 •

edited

Loading