Skip to content
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

Fix set_total_deposit failure after channel is closed #4685

Merged
merged 6 commits into from Aug 30, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 8 additions & 1 deletion raiden/network/proxies/token_network.py
Expand Up @@ -1402,11 +1402,18 @@ def _set_total_withdraw(
block_identifier=failed_at_blockhash,
)

if detail.state != ChannelState.OPENED:
if detail.state > ChannelState.OPENED:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does the comparison work?

https://docs.python.org/3/library/enum.html#comparisons says:

Ordered comparisons between enumeration values are not supported. Enum members are not integers (but see IntEnum below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ChannelState is an IntEnum so comparisons work.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was never big fan of comparing int values of enums as if you are not careful and add some new value later on in the wrong place it can cause really hard to find bugs. That is just my experience.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, I was looking at https://github.com/raiden-network/raiden/blob/develop/raiden/transfer/state.py#L70, which is a plain Enum. Is there a reason for having both or is it just for historic reasons?

msg = (
f"cannot call setTotalWithdraw on a channel that is not open. "
f"current_state={detail.state}"
)
raise RaidenRecoverableError(msg)

if detail.state < ChannelState.OPENED:
msg = (
f"cannot call setTotalWithdraw on a channel that does not exist. "
f"current_state={detail.state}"
)
raise RaidenUnrecoverableError(msg)

total_withdraw_done = our_details.withdrawn >= total_withdraw
Expand Down
79 changes: 79 additions & 0 deletions raiden/tests/integration/test_recovery.py
@@ -1,3 +1,4 @@
import gevent
import pytest

from raiden import waiting
Expand All @@ -16,6 +17,7 @@
transfer_and_assert_path,
)
from raiden.transfer import views
from raiden.transfer.events import ContractSendChannelWithdraw
from raiden.transfer.state import NODE_NETWORK_UNREACHABLE
from raiden.transfer.state_change import (
ContractReceiveChannelClosed,
Expand Down Expand Up @@ -235,3 +237,80 @@ def test_recovery_blockchain_events(raiden_network, token_addresses, network_wai
RANGE_ALL_STATE_CHANGES
)
assert search_for_item(restarted_state_changes, ContractReceiveChannelClosed, {})


@pytest.mark.parametrize("deposit", [2])
@pytest.mark.parametrize("number_of_nodes", [2])
def test_node_clears_pending_withdraw_transaction_after_channel_is_closed(
raiden_network, token_addresses, network_wait, number_of_nodes, retry_timeout
):
""" A test case related to https://github.com/raiden-network/raiden/issues/4639
Where a node sends a withdraw transaction, is stopped before the transaction is completed.
Meanwhile, the partner node closes the channel so when the stopped node is back up, it tries to
execute the pending withdraw transaction and fails because the channel was closed.
Expected behaviour: Channel closed state change should cancel a withdraw transaction.
Buggy behaviour: The channel closure isn't detected on recovery and
the on-chain transaction fails.
"""
app0, app1 = raiden_network
token_address = token_addresses[0]

# Prevent the withdraw transaction from being sent on-chain. This
# will keep the transaction in the pending list
send_channel_withdraw_event = app0.raiden.raiden_event_handler.hold(
ContractSendChannelWithdraw, {}
)

channel_state = views.get_channelstate_for(
chain_state=views.state_from_app(app0),
token_network_registry_address=app0.raiden.default_registry.address,
token_address=token_address,
partner_address=app1.raiden.address,
)
assert channel_state, "Channel does not exist"

app0.raiden.withdraw(canonical_identifier=channel_state.canonical_identifier, total_withdraw=1)

timeout = network_wait * number_of_nodes
with gevent.Timeout(seconds=timeout):
send_channel_withdraw_event.wait()

msg = "A withdraw transaction should be in the pending transactions list"
chain_state = views.state_from_app(app0)
assert search_for_item(
item_list=chain_state.pending_transactions,
item_type=ContractSendChannelWithdraw,
attributes={"total_withdraw": 1},
), msg

app0.raiden.stop()
app0.stop()

app1_api = RaidenAPI(app1.raiden)
app1_api.channel_close(
registry_address=app0.raiden.default_registry.address,
token_address=token_address,
partner_address=app0.raiden.address,
)

waiting.wait_for_close(
raiden=app1.raiden,
token_network_registry_address=app1.raiden.default_registry.address,
token_address=token_address,
channel_ids=[channel_state.identifier],
retry_timeout=retry_timeout,
)

app0.raiden.start()

chain_state = views.state_from_app(app0)

msg = "The withdraw transaction should have been invalidated on restart."
assert (
search_for_item(
item_list=chain_state.pending_transactions,
item_type=ContractSendChannelWithdraw,
attributes={"total_withdraw": 1},
)
is None
), msg
8 changes: 8 additions & 0 deletions raiden/transfer/events.py
Expand Up @@ -74,6 +74,14 @@ class ContractSendChannelWithdraw(ContractSendEvent):
expiration: BlockExpiration
partner_signature: Signature

@property
def channel_identifier(self) -> ChannelID:
return self.canonical_identifier.channel_identifier

@property
def token_network_address(self) -> TokenNetworkAddress:
return self.canonical_identifier.token_network_address


@dataclass(frozen=True)
class ContractSendChannelClose(ContractSendEvent):
Expand Down
10 changes: 10 additions & 0 deletions raiden/transfer/node.py
Expand Up @@ -14,6 +14,7 @@
ContractSendChannelClose,
ContractSendChannelSettle,
ContractSendChannelUpdateTransfer,
ContractSendChannelWithdraw,
ContractSendSecretReveal,
SendWithdrawRequest,
)
Expand Down Expand Up @@ -1104,6 +1105,15 @@ def is_transaction_invalidated(transaction: ContractSendEvent, state_change: Sta
if is_our_failed_update_transfer:
return True

is_our_failed_withdraw = (
isinstance(state_change, ContractReceiveChannelClosed)
and isinstance(transaction, ContractSendChannelWithdraw)
and state_change.token_network_address == transaction.token_network_address
and state_change.channel_identifier == transaction.channel_identifier
)
if is_our_failed_withdraw:
return True

return False


Expand Down
2 changes: 1 addition & 1 deletion raiden/waiting.py
Expand Up @@ -464,7 +464,7 @@ def wait_for_withdraw_complete(
total_withdraw: WithdrawAmount,
retry_timeout: float,
) -> None:
"""Wait until a transfer with a specific identifier and amount
"""Wait until a withdraw with a specific identifier and amount
is seen in the WAL.

Note:
Expand Down