Block the mon update removing a preimage until upstream mon writes #2169

TheBlueMatt · 2023-04-07T01:10:29Z

When we forward a payment and receive an update_fulfill_htlc
message from the downstream channel, we immediately claim the HTLC
on the upstream channel, before even doing a commitment_signed
dance on the downstream channel. This implies that our
ChannelMonitorUpdates "go out" in the right order - first we
ensure we'll get our money by writing the preimage down, then we
write the update that resolves giving money on the downstream node.

This is safe as long as ChannelMonitorUpdates complete in the
order in which they are generated, but of course looking forward we
want to support asynchronous updates, which may complete in any
order.

Thus, here, we enforce the correct ordering by blocking the
downstream ChannelMonitorUpdate until the upstream one completes.
Like the PaymentSent event handling we do so only for the
revoke_and_ack ChannelMonitorUpdate, ensuring the
preimage-containing upstream update has a full RTT to complete
before we actually manage to slow anything down.

This completes the work started in #2167 and depends on (and older version of) #2111 and #2112. This completes much of the work required for fully async ChannelMonitorUpdates. It introduces #2168, though I may try to fix that prior to marking ready, but its not super critical as even after this PR we do not "officially support" async ChannelMonitorUpdates.

codecov-commenter · 2023-04-07T03:58:28Z

Codecov Report

Patch coverage: 98.24% and project coverage change: +1.77% 🎉

Comparison is base (8de8861) 90.45% compared to head (34c20b0) 92.23%.
Report is 4 commits behind head on main.

❗ Current head 34c20b0 differs from pull request most recent head 5d9c602. Consider uploading reports for the commit 5d9c602 to get more accurate results

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2169      +/-   ##
==========================================
+ Coverage   90.45%   92.23%   +1.77%     
==========================================
  Files         112      111       -1     
  Lines       58566    69661   +11095     
  Branches    58566    69661   +11095     
==========================================
+ Hits        52977    64253   +11276     
+ Misses       5589     5408     -181

Files Changed	Coverage Δ
lightning/src/util/test_utils.rs	`73.61% <ø> (+4.90%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`91.43% <93.33%> (+2.52%)`	⬆️
lightning/src/ln/channelmanager.rs	`91.85% <97.91%> (+5.14%)`	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	`98.76% <99.14%> (+0.09%)`	⬆️
lightning/src/chain/channelmonitor.rs	`94.59% <100.00%> (-0.06%)`	⬇️
lightning/src/ln/chan_utils.rs	`95.03% <100.00%> (ø)`
lightning/src/ln/functional_tests.rs	`99.19% <100.00%> (+1.09%)`	⬆️
lightning/src/ln/payment_tests.rs	`98.64% <100.00%> (+1.18%)`	⬆️
lightning/src/ln/shutdown_tests.rs	`99.50% <100.00%> (+0.97%)`	⬆️

... and 36 files with indirect coverage changes

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

valentinewallace · 2023-05-23T20:04:48Z

Could you fix the build? thanks!

TheBlueMatt · 2023-06-20T02:24:42Z

Rebased on #2362

TheBlueMatt · 2023-08-27T20:39:18Z

This should be good to go now. There's some followups that will be required for 117, but they're noted in the comments for now.

TheBlueMatt · 2023-09-10T16:49:12Z

Rebased.

jkczyz

Feel free to squash, IMO.

dunxen · 2023-09-11T10:52:13Z

Generally LGTM. Will have another look after squash

TheBlueMatt · 2023-09-11T17:39:21Z

Squashed without further changes.

When we need to rebroadcast a `commitment_signed` on reconnect in response to a previous update (ie not one which contains any updates) we previously hacked in support for it by passing a `-1` for the number of expected update_add_htlcs. This is a mess, and with the introduction of `ReconnectArgs` we can now clean it up easily with a new bool.

TheBlueMatt · 2023-09-12T16:03:45Z

Rebased without changes.

jkczyz

Generally looks good outside some clarification in the test comments.

lightning/src/ln/chanmon_update_fail_tests.rs

jkczyz · 2023-09-12T19:01:57Z

LGTM

When we forward a payment and receive an `update_fulfill_htlc` message from the downstream channel, we immediately claim the HTLC on the upstream channel, before even doing a `commitment_signed` dance on the downstream channel. This implies that our `ChannelMonitorUpdate`s "go out" in the right order - first we ensure we'll get our money by writing the preimage down, then we write the update that resolves giving money on the downstream node. This is safe as long as `ChannelMonitorUpdate`s complete in the order in which they are generated, but of course looking forward we want to support asynchronous updates, which may complete in any order. Thus, here, we enforce the correct ordering by blocking the downstream `ChannelMonitorUpdate` until the upstream one completes. Like the `PaymentSent` event handling we do so only for the `revoke_and_ack` `ChannelMonitorUpdate`, ensuring the preimage-containing upstream update has a full RTT to complete before we actually manage to slow anything down.

`expect_payment_forwarded` takes a bool to indicate that the inbound channel on which we received a forwarded payment has been closed, but then ignores it in favor of looking at the fee in the event. While this is generally correct, in cases where we process an event after a channel was closed, which was generated before a channel closed this is incorrect. Instead, we examine the bool we already passed and use that.

Because some of these tests require connecting blocks without calling `get_and_clear_pending_msg_events`, we need to split up the block connection utilities to only optionally call sanity-checks.

Also allowing us to pass the event manually.

This adds a test for monitor update actions being completed on startup if a monitor update completed "while we were shut down" (or, really, the manager didn't get persisted after the update completed).

TheBlueMatt · 2023-09-12T19:03:24Z

Squashed without further changes.

lightning/src/ln/functional_tests.rs

TheBlueMatt added the blocked on dependent pr label Apr 7, 2023

TheBlueMatt force-pushed the 2023-03-monitor-e-monitor branch from 304d811 to c382bfd Compare April 16, 2023 01:07

TheBlueMatt force-pushed the 2023-03-monitor-e-monitor branch 3 times, most recently from 70684b1 to f17550b Compare May 4, 2023 23:14

TheBlueMatt mentioned this pull request May 10, 2023

Add infra to block ChannelMonitorUpdates on forwarded claims #2167

Merged

TheBlueMatt force-pushed the 2023-03-monitor-e-monitor branch from f17550b to 428d162 Compare May 12, 2023 00:10

TheBlueMatt force-pushed the 2023-03-monitor-e-monitor branch from 428d162 to 720d840 Compare May 24, 2023 04:15

wpaulino self-requested a review May 24, 2023 17:52

TheBlueMatt force-pushed the 2023-03-monitor-e-monitor branch from 720d840 to e8cec7f Compare May 31, 2023 22:57

TheBlueMatt force-pushed the 2023-03-monitor-e-monitor branch 2 times, most recently from b30d299 to a1517aa Compare June 20, 2023 02:24

TheBlueMatt added this to the 0.0.117 milestone Jun 20, 2023

TheBlueMatt mentioned this pull request Jun 21, 2023

Move in-flight ChannelMonitorUpdates to ChannelManager #2362

Merged

TheBlueMatt force-pushed the 2023-03-monitor-e-monitor branch 2 times, most recently from a78d620 to e590021 Compare June 27, 2023 20:12

TheBlueMatt force-pushed the 2023-03-monitor-e-monitor branch from e590021 to 8b53997 Compare July 5, 2023 16:57

This was referenced Jul 5, 2023

Handle pre-startup and closed-channel monitor update completion actions #2391

Merged

Handle Channel force-close with pending blocked monitors blocking another channel #2168

Closed

ConorOkus mentioned this pull request Jul 5, 2023

Async Support #2394

Closed

7 tasks

TheBlueMatt force-pushed the 2023-03-monitor-e-monitor branch from 8b53997 to 6dd2155 Compare July 11, 2023 19:57

TheBlueMatt force-pushed the 2023-03-monitor-e-monitor branch from 6dd2155 to b247241 Compare July 24, 2023 22:32

valentinewallace mentioned this pull request Jul 28, 2023

Delay RAA-after-next processing until PaymentSent is are handled #2112

Merged

TheBlueMatt force-pushed the 2023-03-monitor-e-monitor branch from b247241 to 98dbe43 Compare August 21, 2023 19:22

TheBlueMatt marked this pull request as ready for review August 27, 2023 20:38

TheBlueMatt force-pushed the 2023-03-monitor-e-monitor branch from 98dbe43 to 183e904 Compare August 27, 2023 20:38

TheBlueMatt force-pushed the 2023-03-monitor-e-monitor branch from c54b7b3 to d48e624 Compare September 7, 2023 23:00

TheBlueMatt mentioned this pull request Sep 8, 2023

Drop the ChannelMonitorUpdateStatus::PermanentFailure variant #2562

Merged

TheBlueMatt force-pushed the 2023-03-monitor-e-monitor branch from d48e624 to 17bc1ee Compare September 10, 2023 16:48

jkczyz reviewed Sep 11, 2023

View reviewed changes

TheBlueMatt force-pushed the 2023-03-monitor-e-monitor branch from 17bc1ee to ca130b9 Compare September 11, 2023 17:39

wpaulino removed their request for review September 11, 2023 19:15

TheBlueMatt added 2 commits September 12, 2023 16:03

Fix various unused warnings in test and regular builds

4835b16

TheBlueMatt force-pushed the 2023-03-monitor-e-monitor branch from ca130b9 to 42536d2 Compare September 12, 2023 16:03

TheBlueMatt mentioned this pull request Sep 12, 2023

Housekeeping: fix some warning and docs #2568

Merged

TheBlueMatt force-pushed the 2023-03-monitor-e-monitor branch from 42536d2 to 2754eb2 Compare September 12, 2023 16:14

jkczyz reviewed Sep 12, 2023

View reviewed changes

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

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

TheBlueMatt force-pushed the 2023-03-monitor-e-monitor branch from 2754eb2 to 5d9c602 Compare September 12, 2023 18:34

jkczyz reviewed Sep 12, 2023

View reviewed changes

lightning/src/ln/chanmon_update_fail_tests.rs Show resolved Hide resolved

TheBlueMatt force-pushed the 2023-03-monitor-e-monitor branch from 5d9c602 to 5b0ebdb Compare September 12, 2023 18:54

TheBlueMatt added 5 commits September 12, 2023 19:03

Update tests to test re-claiming of forwarded HTLCs on startup

0d8b096

Because some of these tests require connecting blocks without calling `get_and_clear_pending_msg_events`, we need to split up the block connection utilities to only optionally call sanity-checks.

Split expect_payment_forwarded into a function called by macro

6c3029d

Also allowing us to pass the event manually.

Test monitor update completion actions on pre-startup completion

9f3e127

This adds a test for monitor update actions being completed on startup if a monitor update completed "while we were shut down" (or, really, the manager didn't get persisted after the update completed).

TheBlueMatt force-pushed the 2023-03-monitor-e-monitor branch from 5b0ebdb to 9f3e127 Compare September 12, 2023 19:03

jkczyz approved these changes Sep 12, 2023

View reviewed changes

valentinewallace approved these changes Sep 12, 2023

View reviewed changes

lightning/src/ln/functional_tests.rs Show resolved Hide resolved

TheBlueMatt merged commit 3dcdb14 into lightningdevkit:main Sep 12, 2023

Block the mon update removing a preimage until upstream mon writes #2169

Block the mon update removing a preimage until upstream mon writes #2169

Uh oh!

Conversation

TheBlueMatt commented Apr 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Apr 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

valentinewallace commented May 23, 2023

Uh oh!

TheBlueMatt commented Jun 20, 2023

Uh oh!

TheBlueMatt commented Aug 27, 2023

Uh oh!

TheBlueMatt commented Sep 10, 2023

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

dunxen commented Sep 11, 2023

Uh oh!

TheBlueMatt commented Sep 11, 2023

Uh oh!

TheBlueMatt commented Sep 12, 2023

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jkczyz commented Sep 12, 2023

Uh oh!

TheBlueMatt commented Sep 12, 2023

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

TheBlueMatt commented Apr 7, 2023 •

edited

Loading

codecov-commenter commented Apr 7, 2023 •

edited

Loading