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.

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

leaving token network doesn't work #1879

Closed
pcppcp opened this Issue Jul 18, 2018 · 4 comments

Comments

Projects
None yet
4 participants
@pcppcp
Contributor

pcppcp commented Jul 18, 2018

curl -X DELETE localhost:5001/api/1/connections/0x0f114A1E9Db192502E7856309cc899952b3db1ED will return [], but no channels are closed.

WebUI returns success for this action.

@pcppcp pcppcp added ui API labels Jul 18, 2018

@LefterisJP

This comment has been minimized.

Collaborator

LefterisJP commented Jul 18, 2018

@pcppcp Are you sure that no channels are closed after some time. The webui returns success for the action as it's an asynchronous call.

@pcppcp

This comment has been minimized.

Contributor

pcppcp commented Jul 18, 2018

hmm, I'll try to reproduce it. Maybe it's related to number of open channels, but it didn't work for both webui and cli.

@palango palango added this to the Red Eyes milestone Jul 19, 2018

@rakanalh rakanalh self-assigned this Jul 23, 2018

@rakanalh rakanalh added the P1 label Jul 23, 2018

@rakanalh

This comment has been minimized.

Contributor

rakanalh commented Jul 26, 2018

Debugging this lead me to check both views.get_channelstate_for_receiving and views.get_channelstate_open. The former will return an empty list (according to my reproduction of the issue) while the latter would return the list of open channels. Going back to the APIs, we have a parameter: only_receiving_channels which defaults to True. Meaning that we will only be triggering leaving a channels for receiving while other open channels can still be open.

One part is a bit confusiong which is that the leave method of the ConnectionManager is documented with:

    Note: By default we're just discarding all channels for which we haven't
    received anything.  This potentially leaves deposits locked in channels after
    `closing`. This is "safe" from an accounting point of view (deposits
    can not be lost), but may still be undesirable from a liquidity point
    of view (deposits will only be freed after manually closing or after
    the partner closed the channel).

    If only_receiving is False then we close and settle all channels
    irrespective of them having received transfers or not.

The confusing part here is that views.get_channelstate_for_receiving returns the list of channels from which we actually received transfers. So the first sentence of the leave's method comment contradicts the logic implemented.

Calling the leave API:

http --timeout 1000 DELETE http://localhost:5003/api/1/connections/0x0f114A1E9Db192502E7856309cc899952b3db1ED only_receiving_channels=false

Successfully triggered all channels to close.

Suggested solutions:

  1. Make only_receiving_channels false by default
  2. Do nothing and explicitly document the choice that the user has to make. However, we have to take into consideration that the UI doesn't prompt the user for selecting which channels to close when leaving the network, which brings us to the first point.

@LefterisJP @hackaugusto please advise.

@LefterisJP

This comment has been minimized.

Collaborator

LefterisJP commented Jul 26, 2018

@rakanalh hahaha nice find. So basically the function should have been doing what the comment says it does. But as you noticed, due to using views.get_channelstate_for_receiving it is doing the exact opposite. That is definitely a bug.

But let's take a step back and explain the reasoning here. In the past we could not recover from an irregular shutdown. Now with the WAL we can and we do so.

Instead what we guaranteed was that you safely use Raiden and shut it down regularly. Shutting down regurarly would cause you to close all channels and only after closing and settling would the shutdown be succesfull. To make that faster while keeping the safety guarantee we added this "only_receiving" flag to do what the comment describes. But it should have been only_not_received instead.

Now that we are done with the history lesson ... I could go as far as saying we don't really need that argument anymore. So I would remove that logic completely, especially considering it's not exposed from the web UI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment