Fixed initiator fail events#3024
Conversation
|
|
||
| if lock_has_expired: | ||
| if initiator_state.received_secret_request: | ||
| reason = 'bad secret request message from target' |
There was a problem hiding this comment.
Handling this here means that a payment that failed because of an invalid secret request will take one block longer till it's cancelled!?
There was a problem hiding this comment.
Handling this here means that a payment that failed because of an invalid secret request will take one block longer till it's cancelled!?
It may be way more blocks, up to lock timeout, as the off-chain secret request can be sent at any point in time.
This change however makes the failure event consistent with the update in balance, since the byzantine node is the target, which is not necessarily the next hop, and we need to wait for the lock to expire before removing it.
There was a problem hiding this comment.
You're point makes sense, but on the other hand it might be nice to be able to inform the user as early as possible that the payment failed, even if the funds are still locked.
There was a problem hiding this comment.
Both things make sense. I really think we should target a consistent view of the state as much as possible, the system is hard enough to understand when things are consistent. However, it can be definitely useful to inform a user that a transfer will not be completely because of ByzantineBehavior, so my suggestion would have two different events, keep this one here, which exposes a consistent view, and add a new one to inform the user when something unexpected happened (like the target sending the wrong secret request).
If you agree with me, I think we should do that in another PR.
There was a problem hiding this comment.
If you agree with me, I think we should do that in another PR.
That sounds like a plan!
There was a problem hiding this comment.
That sounds like a plan!
So can one of you make an issue for this (so that we don't forget the other PR)?
If I understand correctly you mean to add an event that the payment will fail here ?
|
|
||
| if lock_has_expired: | ||
| if initiator_state.received_secret_request: | ||
| reason = 'bad secret request message from target' |
There was a problem hiding this comment.
That sounds like a plan!
So can one of you make an issue for this (so that we don't forget the other PR)?
If I understand correctly you mean to add an event that the payment will fail here ?
hackaugusto
force-pushed
the
fix_initiator_event
branch
from
4a89b2f to
cbf815e
Compare
hackaugusto
force-pushed
the
fix_initiator_event
branch
4 times, most recently
from
666c12e to
94cb24a
Compare
Codecov Report
@@ Coverage Diff @@
## master #3024 +/- ##
=========================================
Coverage ? 74.82%
=========================================
Files ? 94
Lines ? 12401
Branches ? 1735
=========================================
Hits ? 9279
Misses ? 2474
Partials ? 648
Continue to review full report at Codecov.
|
|
Rebasing and reviewing this. |
When a transfer fails because a lock expired the corresponding events must be emitted. This moves the event from the invalid secret request together with the lock expiration to avoid emitting the same event twice.
LefterisJP
force-pushed
the
fix_initiator_event
branch
from
060e5f1 to
01799f8
Compare
LefterisJP
left a comment
LefterisJP
left a comment
There was a problem hiding this comment.
Looks good to me. I fixed some minor problems after rebasing. We can merge as soon as all tests are green.
4 participants
When a transfer fails because a lock expired the corresponding events
must be emitted. This moves the event from the invalid secret request
together with the lock expiration to avoid emitting the same event
twice.