Make payments not duplicatively fail/succeed on reload/reconnect#918
Conversation
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.
|
valentinewallace
left a comment
There was a problem hiding this comment.
Really nice usability improvement. Still reviewing but I think I just have one docs question
dd9dcad to
a2247b1
Compare
a2247b1 to
853838c
Compare
b45341e to
da33f5c
Compare
|
Squashed with no diff 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.
74dc019 to
864375e
Compare
|
Squashed and added one additional commit to address the fuzz failures this introduced. |
| 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. |
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.