-
Notifications
You must be signed in to change notification settings - Fork 353
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Delay RAA-after-next processing until PaymentSent is are handled
In 0ad1f4c we fixed a nasty bug where a failure to persist a `ChannelManager` faster than a `ChannelMonitor` could result in the loss of a `PaymentSent` event, eventually resulting in a `PaymentFailed` instead! As noted in that commit, there's still some risk, though its been substantially reduced - if we receive an `update_fulfill_htlc` message for an outbound payment, and persist the initial removal `ChannelMonitorUpdate`, then respond with our own `commitment_signed` + `revoke_and_ack`, followed by receiving our peer's final `revoke_and_ack`, and then persist the `ChannelMonitorUpdate` generated from that, all prior to completing a `ChannelManager` persistence, we'll still forget the HTLC and eventually trigger a `PaymentFailed` rather than the correct `PaymentSent`. Here we fully fix the issue by delaying the final `ChannelMonitorUpdate` persistence until the `PaymentSent` event has been processed and document the fact that a spurious `PaymentFailed` event can still be generated for a sent payment. The original fix in 0ad1f4c is still incredibly useful here, allowing us to avoid blocking the first `ChannelMonitorUpdate` until the event processing completes, as this would cause us to add event-processing delay in our general commitment update latency. Instead, we ultimately race the user handling the `PaymentSent` event with how long it takes our `revoke_and_ack` + `commitment_signed` to make it to our counterparty and receive the response `revoke_and_ack`. This should give the user plenty of time to handle the event before we need to make progress. Sadly, because we change our `ChannelMonitorUpdate` semantics, this change requires a number of test changes, avoiding checking for a post-RAA `ChannelMonitorUpdate` until after we process a `PaymentSent` event. Note that this does not apply to payments we learned the preimage for on-chain - ensuring `PaymentSent` events from such resolutions will be addressed in a future PR. Thus, tests which resolve payments on-chain switch to a direct call to the `expect_payment_sent` function with the claim-expected flag unset.
- Loading branch information
1 parent
91707fb
commit 6cb3b5e
Showing
14 changed files
with
229 additions
and
101 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.