Withdraw expiry#4289
Merged
Merged
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #4289 +/- ##
===========================================
- Coverage 80.21% 80.16% -0.05%
===========================================
Files 102 102
Lines 13618 13807 +189
Branches 2111 2148 +37
===========================================
+ Hits 10923 11069 +146
- Misses 2089 2126 +37
- Partials 606 612 +6
Continue to review full report at Codecov.
|
hackaugusto
suggested changes
Jun 26, 2019
Contributor
hackaugusto
left a comment
hackaugusto
left a comment
There was a problem hiding this comment.
I have to do a second pass on this ones. This first review was more on a structural level, thinking about the protocol and edge cases is pending.
hackaugusto
previously requested changes
Jul 2, 2019
Contributor
hackaugusto
left a comment
hackaugusto
left a comment
There was a problem hiding this comment.
The signature does not contain the expiration and the tests are passing, this tells me the raiden-contracts are not supporting this expiration functionality. This must not be merged until that is fixed
Note that this is safe provided that in python >= 3.6, dictionary items are sorted by insertion order. Which means that we can rely on the fact that the withdraws added will be iterated in the order in which they were added similar to a list.
expiration_block in setTotalWithdraw. The proper implementation is coming in #4195.
- The most important change is to consider an expired message *as VALID*. Without this change, when nodes go offline queues will get out-of-sync. The reason is that a response is expected to clear the node's queue. Consider the following case: 1. Alice sends a WithdrawRequest to Bob. 2. Bob goes offline. 3. Enough blocks passes to make Alice's request expired. 4. 1. Bob cames back online and receives Alice's request. 5. At this point the request is expired, if Bob reject the request Alice's queue will never move forward, this makes the channel unusable. - Moved signature validation of the withdraw messages to a separate function that only depends on the state change. This forces the signature to be validated against the message contents only, and don't leave margin to mix up the signature validation with the channel state. This will prevent hiding errors were the signature is generated from the correct values but the message itself is populated with invalid values. Note: Ideally the unecessary fields should be removed. - Instead of handling the withdraw amount at every call site, switched to using partner_total_withdraw and our_total_withdraw, which enforces the withdraw to be the maximum of the onchain and offchain values. - Fixed is_valid_action_withdraw overflow check, it should use the requested withdraw amount from the action, and not the current withdraw amount from our_state. - Moved the signature errors last. This is a cosmetic change, but it should make debugging easier. This was more important when the signature was checked against the expected values of the channel, because there if any of the values was incorrect the signature would also be incorrect, but it would not be known which specific field was invalid. By leaving the signature check last, we make sure to have the most sepecific error message possible, which is the best for understand the error. - Added the validation checks for the missing fields.
Because an expired withdraw is not considered invalid the processed can be sent on the `is_valid` branch. This also avoids sending transaction that are expected to fail, namely sending an expirable transactions too close to the expiry block.
Consider the following scenarion: 1. Alice sends to Bob a WithdrawRequest 2. Bob goes offline 3. The WithdrawRequest expires, and Alice sends the WithdrawExpired 4. Bob comes back online 5. Bob processes the WithdrawRequest and sends the WithdrawConfirmed 6. Bob processes the WithdrawExpired and sends the Processsed With the previous code Alice would not send a Processed for the WithdrawConfirmed, which would make the channel unusable for Bob. This happened because Alice removed the WithdrawRequest from the withdraws mapping, and once it received the WithdrawConfirmed messages it was considered invalid.
ulope
approved these changes
Jul 8, 2019
Collaborator
ulope
left a comment
ulope
left a comment
There was a problem hiding this comment.
As requested in chat, approving with only a cursory glance.
Two nitpicks below.
| The withdraw has expired if the current block exceeds | ||
| the withdraw's expiration + confirmation blocks. | ||
| """ | ||
| return block_number >= expiration_threshold |
Collaborator
There was a problem hiding this comment.
The doc string doesn't match the code. 'Exceeds' suggests > instead of >=.
Contributor
Author
There was a problem hiding this comment.
Maybe "exceeds" should be replaced with "reaches and goes beyond the withdraw expiration".
But the operator is correct, once the expiration block is reached, the withdraw should be considered expired.
| return channel_total_withdraw <= UINT256_MAX | ||
|
|
||
|
|
||
| def is_valid_withdraw_request_signature( |
Collaborator
There was a problem hiding this comment.
The next three functions are (apart from parameter names and types) completely identical. That seems like a nice source for future bugs.
Contributor
Author
There was a problem hiding this comment.
We plan on a follow up PR to address some of the nitpicks we noticed here and there.
I think @hackaugusto had a goal when he did this. Was it because we can change the signature parameter for each of those messages?
rakanalh
dismissed
hackaugusto’s stale review
I paired with Augusto and resolved all his comments.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #4195 .
TODO
total_withdrawto hold the value on-chainwithdrawsshould only contain the list of pending withdraw states