Pr secrethash related#4150
Conversation
offerm
force-pushed
the
PR-secrethash-related
branch
from
5612600 to
8004530
Compare
Codecov Report
@@ Coverage Diff @@
## develop #4150 +/- ##
===========================================
+ Coverage 87.17% 87.22% +0.04%
===========================================
Files 102 102
Lines 12843 12977 +134
===========================================
+ Hits 11196 11319 +123
- Misses 1647 1658 +11
Continue to review full report at Codecov.
|
|
|
||
| if initiator_state.transfer_description.secret == EMPTY_SECRET: | ||
| transfer_description = initiator_state.transfer_description | ||
| payment_failed = EventPaymentSentFailed( |
There was a problem hiding this comment.
I don't think this event should be used here. We can only guarantee that a payment has failed after the lock has expired and we expect this event to mean that.
The handling is probably better done on the handle_secret_request on the message handler, and do the same thing as the handle_refund_transfer (as in, handle the special case when the secret is unknown).
There was a problem hiding this comment.
@hackaugusto So you suggest that I will just ignore this request and let the transfer expire?
Clearly I can do that but let's consider the app that made the payment (XUD). It can take some time until the transfer expires and in that time, XUD does not know what to do with the order in it order book.
XUD knows for sure that it never released the secret so there is no risk in getting back immediately and letting XUD knows that its peer is not configured right.
I added this handling as it was hard to find what is causing XUD transfer to fail. Keep in mind that this is a result of misconfiguration of the peer node and it is the sending that is suffering due to it.
The handling we added in handle_message_refundtransfer is not similar. Here, if the first route fails the transfer is blocked until expiration. However, on XUD level we are checking that there is a direct channel between the peers and that both peers are up, running, and connected to Raiden. So practically this situation of long payment can be avoided.
Let me know what you think.
There was a problem hiding this comment.
@offerm I believe Augusto's argument was against using EventPaymentSentFailed as the event to indicate that the payment actually failed. At this point in time where you emit this event, the lockedd amounts are still locked and the payment never really failed in the sense that the locks no longer exist. At this point, we could end up in a situation where the secret request failed because the XUD did not provided it (False returned by the resolve)... so we send this event here but after a while the same request is sent and the resolver provides the secret and the payment was a success. Do you see where this is going?
The alternative is as Augusto suggests, is to check this in the message handler to make sure that:
- This is an atomic swap transfer (via secret being unknown to the node)
- The resolver request fails
- Iff both conditions above happen, then we never dispatch the secret request to the state machine.
At least, this is how i am thinking about it right now. Maybe Augusto has a better approach to how this can be done in details.
There was a problem hiding this comment.
As @rakanalh described, IMO the payment is failed only after the lock has expired, this is important because the event is tied to change in the channel capacity. Here is some relevant discussion we had in the past: #3024 (comment)
You could add a new event as from the linked discussion. Since your application controls the secret, it knows the payment will never succeed.
Codecov Report
@@ Coverage Diff @@
## develop #4150 +/- ##
===========================================
+ Coverage 86.73% 87.19% +0.46%
===========================================
Files 102 102
Lines 13001 12975 -26
===========================================
+ Hits 11276 11314 +38
+ Misses 1725 1661 -64
Continue to review full report at Codecov.
|
If the target is asking for the secret but the initiator does not have it, emit and error. If secrethash provided with payment request and there is another inflight payment request with the same secrethash, emit an error
offerm
force-pushed
the
PR-secrethash-related
branch
from
b1b26f0 to
5368248
Compare
|
@rakanalh @hackaugusto need tests restart please. |
|
I understand the situation. Still, bases on my other change in this PR,
"the same request" can't be provided later as the secrethash is still
inflight (until expiration).
Check number 2 cant really be check on the initiator side since the
resolver is running on the target side.
Let me know if you prefer that i just left it to expire without an event to
the caller. The impact is only in case of bad configuration of the trage.
However, it impacts the initiator and creates a bad user experience.
…On Fri, 31 May 2019 at 16:20 Rakan Alhneiti ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In raiden/transfer/mediated_transfer/initiator.py
<#4150 (comment)>
:
> @@ -319,7 +319,19 @@ def handle_secretrequest(
elif not is_valid_secretrequest and is_message_from_target:
initiator_state.received_secret_request = True
- iteration = TransitionResult(initiator_state, list())
+
+ if initiator_state.transfer_description.secret == EMPTY_SECRET:
+ transfer_description = initiator_state.transfer_description
+ payment_failed = EventPaymentSentFailed(
@offerm <https://github.com/offerm> I believe Augusto's argument was
against using EventPaymentSentFailed as the event to indicate that the
payment actually failed. At this point in time where you emit this event,
the lockedd amounts are still locked and the payment never really failed in
the sense that the locks no longer exist. At this point, we could end up in
a situation where the secret request failed because the XUD did not
provided it (False returned by the resolve)... so we send this event here
but after a while the same request is sent and the resolver provides the
secret and the payment was a success. Do you see where this is going?
The alternative is as Augusto suggests, is to check this in the message
handler to make sure that:
1. This is an atomic swap transfer (via secret being unknown to the
node)
2. The resolver request fails
3. Iff both conditions above happen, then we never dispatch the secret
request to the state machine.
At least, this is how i am thinking about it right now. Maybe Augusto has
a better approach to how this can be done in details.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4150?email_source=notifications&email_token=ACKDI4R5YESMRPS47HF52U3PYEQ2NA5CNFSM4HP6IFNKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB2H4J6A#discussion_r289386036>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACKDI4R4WNPVHHXXKVZNTILPYEQ2NANCNFSM4HP6IFNA>
.
|
|
@rakanalh @hackaugusto Will you be OK to include in the EventPaymentSentFailed class a member like Let me know if this can pass. Offer |
|
Hey @offerm, @hackaugusto and I discussed this use case and we believe we have an alternative that requires some work on the XUD's end. In this case, the error you're trying to handle is a bit specific to the fact that Raiden does not know the secret iff the transfer currently in-flight is an atomic swap. We think that the best approach for this to be implemented is:
|
|
Hey @offerm, Is this PR something that you would like to pursue? |
|
Depend what you mean by "pursue". In out Raiden fork we are using this change. Without it you get wired situations or complicated logic. Let me know what can we do about it. Offer |
|
My meaning was whether this was still important to you and if you wanted to continue working on this PR according to our suggestions previously made. |
|
Please close or finish this PR until Thursday 20 of Feb. |
|
closing this for now |
5 participants
No description provided.