Skip to content

Don't delete payment task when receiving invalid secret request#3017

Merged
hackaugusto merged 4 commits into
raiden-network:masterfrom
palango:fix-3001
Nov 14, 2018
Merged

Don't delete payment task when receiving invalid secret request#3017
hackaugusto merged 4 commits into
raiden-network:masterfrom
palango:fix-3001

Conversation

@palango
Copy link
Copy Markdown
Contributor

@palango palango commented Nov 13, 2018

Deleting the payment task will lead to problems when unlocking locks related to the task.

@hackaugusto Can you have a look and see if this goes in the right direction? I'll work on a test in the meantime.

Fixes #3001

Copy link
Copy Markdown
Contributor

@hackaugusto hackaugusto left a comment

Choose a reason for hiding this comment

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

The fix looks correct to me.

The secret request is a message which can be cleared with a delivered, which is sent by the transport layer, so even if the first delivered failed to be delivered and the message was processed things are fine, the second message will not be processed but the delivered will be sent, clearing the target's queue.

Also, this is preserving the semantics of ignoring subsequent messages from the target which are invalid.

Important note: The message could be invalid for multiple reasons, the one that we want to prevent here is a target which is not following the protocol. However the definition of invalid will change, currently it required constant expiration and amount, which will most likely change to account for fees and other complications for mediated transfers.

@hackaugusto
Copy link
Copy Markdown
Contributor

Side note: I would not rely on github for documentation, I would either use the comments along side the code itself or the commit messages.

Copy link
Copy Markdown
Contributor

@hackaugusto hackaugusto left a comment

Choose a reason for hiding this comment

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

Could you please add a regression test?

Copy link
Copy Markdown
Contributor

@LefterisJP LefterisJP left a comment

Choose a reason for hiding this comment

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

lgtm

Comment thread raiden/tests/unit/transfer/mediated_transfer/test_initiatorstate.py Outdated
[no ci integration]
Copy link
Copy Markdown
Contributor

@hackaugusto hackaugusto left a comment

Choose a reason for hiding this comment

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

Approving, there are some changes that needs to be done on top of this (event emitting), but I will open a new PR for this.

@hackaugusto hackaugusto merged commit 13590b0 into raiden-network:master Nov 14, 2018
@palango palango deleted the fix-3001 branch November 14, 2018 09:15
@palango
Copy link
Copy Markdown
Contributor Author

palango commented Nov 14, 2018

there are some changes that needs to be done on top of this (event emitting)

What are these things?

@hackaugusto
Copy link
Copy Markdown
Contributor

What are these things?

#3024

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants