Skip to content

Pr secrethash related #4150

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
offerm wants to merge 1 commit into from
Closed

Pr secrethash related #4150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions raiden/api/python.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -755,8 +755,11 @@ def transfer_async(
if secret is not None and not isinstance(secret, typing.T_Secret):
raise InvalidSecret("secret is not valid.")

if secrethash is not None and not isinstance(secrethash, typing.T_SecretHash):
raise InvalidSecretHash("secrethash is not valid.")
if secrethash is not None:
if not isinstance(secrethash, typing.T_SecretHash):
raise InvalidSecretHash("secrethash is not valid.")
if current_state.payment_mapping.secrethashes_to_task.get(secrethash):
raise InvalidSecretHash("task with the same secrethash is still pending")

log.debug(
"Initiating transfer",
Expand Down
14 changes: 13 additions & 1 deletion raiden/transfer/mediated_transfer/initiator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Copy link
Copy Markdown
Contributor

@hackaugusto hackaugusto May 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Contributor Author

@offerm offerm May 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Contributor

@rakanalh rakanalh May 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Contributor

@hackaugusto hackaugusto Jun 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

payment_network_address=transfer_description.payment_network_address,
token_network_address=transfer_description.token_network_address,
identifier=transfer_description.payment_identifier,
target=transfer_description.target,
reason="Target requested the secret but secret is unknown to the Initiator",
)
iteration = TransitionResult(initiator_state, [payment_failed])
else:
iteration = TransitionResult(initiator_state, list())

else:
iteration = TransitionResult(initiator_state, list())
Expand Down