Make payments not duplicatively fail/succeed on reload/reconnect #918
Conversation
This file contains hidden or 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
Codecov Report
@@ Coverage Diff @@
## main #918 +/- ##
==========================================
- Coverage 90.45% 90.26% -0.20%
==========================================
Files 59 59
Lines 29785 30569 +784
==========================================
+ Hits 26941 27592 +651
- Misses 2844 2977 +133
Continue to review full report at Codecov.
|
TheBlueMatt
force-pushed
the
2021-05-dup-claims
branch
from
dd9dcad to
a2247b1
Compare
TheBlueMatt
force-pushed
the
2021-05-dup-claims
branch
from
a2247b1 to
853838c
Compare
valentinewallace
approved these changes
May 13, 2021
TheBlueMatt
force-pushed
the
2021-05-dup-claims
branch
2 times, most recently
from
b45341e to
da33f5c
Compare
|
Squashed with no diff from |
jkczyz
approved these changes
May 20, 2021
TheBlueMatt
force-pushed
the
2021-05-dup-claims
branch
2 times, most recently
from
dbaf591 to
74dc019
Compare
|
Rebased on latest upstream and added trivial cleanups to address Jeff's comments above. Would like a once-over from one of the existing reviewers. |
LGTM |
We currently generate duplicative PaymentFailed/PaymentSent events in two cases: a) If we receive a update_fulfill_htlc message, followed by a disconnect, then a resend of the same update_fulfill_htlc message, we will generate a PaymentSent event for each message. b) When a Channel is closed, any outbound HTLCs which were relayed through it are simply dropped when the Channel is. From there, the ChannelManager relies on the ChannelMonitor having a copy of the relevant fail-/claim-back data and processes the HTLC fail/claim when the ChannelMonitor tells it to. If, due to an on-chain event, an HTLC is failed/claimed, and then we serialize the ChannelManager, but do not re-serialize the relevant ChannelMonitor, we may end up getting a duplicative event. In order to provide the expected consistency, we add explicit tracking of pending outbound payments using their unique session_priv field which is generated when the payment is sent. Then, before generating PaymentFailed/PaymentSent events, we check that the session_priv for the payment is still pending. Thix fixes lightningdevkit#209.
TheBlueMatt
force-pushed
the
2021-05-dup-claims
branch
from
74dc019 to
864375e
Compare
|
Squashed and added one additional commit to address the fuzz failures this introduced. |
jkczyz
reviewed
May 20, 2021
| @@ -179,7 +179,7 @@ impl KeysInterface for KeyProvider { | |||
| SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 6, self.node_id]).unwrap(), | |||
| SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 7, self.node_id]).unwrap(), | |||
| SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 8, self.node_id]).unwrap(), | |||
| [id, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 9, self.node_id], | |||
| [id as u8, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 9, self.node_id], | |||
There was a problem hiding this comment.
I take it we don't need to use four bytes here because we won't exhaust the one-byte space?
There was a problem hiding this comment.
Yea, we only ever create up to two of them per peer.
|
ACK 864375e Tested locally. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We currently generate duplicative PaymentFailed/PaymentSent events
in two cases:
a) If we receive a update_fulfill_htlc message, followed by a
disconnect, then a resend of the same update_fulfill_htlc
message, we will generate a PaymentSent event for each message.
b) When a Channel is closed, any outbound HTLCs which were relayed
through it are simply dropped when the Channel is. From there,
the ChannelManager relies on the ChannelMonitor having a copy of
the relevant fail-/claim-back data and processes the HTLC
fail/claim when the ChannelMonitor tells it to.
If, due to an on-chain event, an HTLC is failed/claimed, and
then we serialize the ChannelManager, but do not re-serialize
the relevant ChannelMonitor, we may end up getting a duplicative
event.
In order to provide the expected consistency, we add explicit
tracking of pending outbound payments using their unique
session_priv field which is generated when the payment is sent.
Then, before generating PaymentFailed/PaymentSent events, we check
that the session_priv for the payment is still pending.
Thix fixes #209.