Skip to content

Fixed initiator fail events #3024

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

Merged
LefterisJP merged 5 commits into from
Jan 29, 2019
Merged

Fixed initiator fail events #3024

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
59ceb03
Fixed initiator fail events
hackaugusto Nov 14, 2018
14985a1
pr review
hackaugusto Nov 14, 2018
f36ac1a
fixed use before assignment and unit test
hackaugusto Jan 28, 2019
fa45048
fixed fuzz tests
hackaugusto Jan 28, 2019
01799f8
typo, check reason string in tests and post rebase fixes
LefterisJP Jan 29, 2019
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
1 change: 0 additions & 1 deletion raiden/tests/fuzz/test_state_changes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,6 @@ def _invalid_authentic_secret_request(self, previous, action):
if action.secrethash in self.processed_secret_requests or self._is_removed(previous):
assert not result.events
else:
assert event_types_match(result.events, EventPaymentSentFailed)
self.processed_secret_requests.add(action.secrethash)

def _unauthentic_secret_request(self, action):
Expand Down
10 changes: 6 additions & 4 deletions raiden/tests/unit/transfer/mediated_transfer/test_initiatorstate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ def test_state_wait_secretrequest_valid():
setup.block_number,
)

assert len(iteration2.events) == 0
assert not iteration2.events


def test_state_wait_secretrequest_invalid_amount():
Expand All @@ -333,8 +333,8 @@ def test_state_wait_secretrequest_invalid_amount():
setup.block_number,
)

assert len(iteration.events) == 1
assert isinstance(iteration.events[0], EventPaymentSentFailed)
msg = 'The payment event now is emitted when the lock expires'
assert search_for_item(iteration.events, EventPaymentSentFailed, {}) is None, msg
assert iteration.new_state.initiator.received_secret_request is True

state_change_2 = ReceiveSecretRequest(
Expand Down Expand Up @@ -955,13 +955,15 @@ def test_initiator_lock_expired():
})
assert lock_expired is not None

assert search_for_item(iteration.events, EventUnlockFailed, {})

# Since the lock expired make sure we also get the payment sent failed event
payment_failed = search_for_item(iteration.events, EventPaymentSentFailed, {
'payment_network_identifier': channel1.payment_network_identifier,
'token_network_identifier': channel1.token_network_identifier,
'identifier': UNIT_TRANSFER_IDENTIFIER,
'target': transfer.target,
'reason': "transfer's lock has expired",
'reason': 'lock expired',
})
assert payment_failed is not None

Expand Down
38 changes: 23 additions & 15 deletions raiden/transfer/mediated_transfer/initiator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from raiden.transfer.events import EventPaymentSentFailed, EventPaymentSentSuccess
from raiden.transfer.mediated_transfer.events import (
CHANNEL_IDENTIFIER_GLOBAL_QUEUE,
EventUnlockFailed,
EventUnlockSuccess,
SendLockedTransfer,
SendSecretReveal,
Expand Down Expand Up @@ -50,7 +51,8 @@ def events_for_unlock_lock(
secret: Secret,
secrethash: SecretHash,
pseudo_random_generator: random.Random,
):
) -> List[Event]:
""" Unlocks the lock offchain, and emits the events for the successful payment. """
# next hop learned the secret, unlock the token locally and send the
# lock claim message to next hop
transfer_description = initiator_state.transfer_description
Expand Down Expand Up @@ -86,6 +88,9 @@ def handle_block(
channel_state: NettingChannelState,
pseudo_random_generator: random.Random,
) -> TransitionResult:
""" Checks if the lock has expired, and if it has sends a remove expired
lock and emits the failing events.
"""
secrethash = initiator_state.transfer.lock.secrethash
locked_lock = channel_state.our_state.secrethashes_to_lockedlocks.get(secrethash)

Expand Down Expand Up @@ -119,20 +124,31 @@ def handle_block(
)
events.extend(expired_lock_events)

if initiator_state.received_secret_request:
reason = 'bad secret request message from target'
Copy link
Copy Markdown
Contributor

@palango palango Nov 14, 2018

Choose a reason for hiding this comment

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

Handling this here means that a payment that failed because of an invalid secret request will take one block longer till it's cancelled!?

Copy link
Copy Markdown
Contributor Author

@hackaugusto hackaugusto Nov 14, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@palango palango Nov 14, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@hackaugusto hackaugusto Nov 14, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@palango palango Nov 14, 2018

Choose a reason for hiding this comment

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

If you agree with me, I think we should do that in another PR.

That sounds like a plan!

Copy link
Copy Markdown
Contributor

@LefterisJP LefterisJP Nov 14, 2018

Choose a reason for hiding this comment

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

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 ?

Copy link
Copy Markdown
Contributor

@palango palango Nov 16, 2018

Choose a reason for hiding this comment

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

Added #3044

else:
reason = 'lock expired'

transfer_description = initiator_state.transfer_description
payment_identifier = transfer_description.payment_identifier
# TODO: When we introduce multiple transfers per payment this needs to be
# reconsidered. As we would want to try other routes once a route
# has failed, and a transfer failing does not mean the entire payment
# would have to fail.
# Related issue: https://github.com/raiden-network/raiden/issues/2329
transfer_failed = EventPaymentSentFailed(
payment_failed = EventPaymentSentFailed(
payment_network_identifier=transfer_description.payment_network_identifier,
token_network_identifier=transfer_description.token_network_identifier,
identifier=transfer_description.payment_identifier,
identifier=payment_identifier,
target=transfer_description.target,
reason="transfer's lock has expired",
reason=reason,
)
events.append(transfer_failed)
unlock_failed = EventUnlockFailed(
identifier=payment_identifier,
secrethash=initiator_state.transfer_description.secrethash,
reason=reason,
)

lock_exists = channel.lock_exists_in_either_channel_side(
channel_state=channel_state,
secrethash=secrethash,
Expand All @@ -143,7 +159,7 @@ def handle_block(
# task around to wait for the LockExpired messages to sync.
# Check https://github.com/raiden-network/raiden/issues/3183
initiator_state if lock_exists else None,
events,
events + [payment_failed, unlock_failed],
)
else:
return TransitionResult(initiator_state, events)
Expand Down Expand Up @@ -333,16 +349,8 @@ def handle_secretrequest(
iteration = TransitionResult(initiator_state, [revealsecret])

elif not is_valid_secretrequest and is_message_from_target:
cancel = EventPaymentSentFailed(
payment_network_identifier=channel_state.payment_network_identifier,
token_network_identifier=channel_state.token_network_identifier,
identifier=initiator_state.transfer_description.payment_identifier,
target=initiator_state.transfer_description.target,
reason='bad secret request message from target',
)

initiator_state.received_secret_request = True
iteration = TransitionResult(initiator_state, [cancel])
iteration = TransitionResult(initiator_state, list())

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