Use nonce ranges by czepluch · Pull Request #287 · raiden-network/raiden

czepluch · 2016-12-16T18:09:10Z

This PR adds nonce ranges to the netting channels. This makes sure that once #191 allows for channels to be reopened, the same transfers cannot be used again.

Tests for reopening a channel and for nonce ranges have also been added, but they are marked as xfail until #191 is done.

With this PR I believe that #39 can be closed

hackaugusto · 2016-12-19T10:13:58Z


+        # use nonce ranges
+        self.our_state.nonce = our_state.nonce * (external_state.opened_block * (2 ** 32))
+        self.partner_state.nonce = partner_state.nonce * (external_state.opened_block * (2 ** 32))


IMHO these statements should be part of ChannelEndState constructor, and then we can move the nonce initialization to the assetmanager.py

Do you have an argument for why this would be preferred?

The constructor is the place to initialize an object, without passing nonce in the constructor we end up with a object improperly initialized (eg. it will produce the wrong behavior)

hackaugusto · 2016-12-19T10:14:54Z

+
+@pytest.mark.xfail
+def test_reopen_channel(tester_state, tester_channelmanager, tester_channels, settle_timeout,
+                       netting_channel_abi):


Please, one argument per line :)

hackaugusto · 2016-12-19T10:18:21Z

Do we need to merge first #191 and then #287 or it doesnt matter?

czepluch · 2016-12-19T11:56:22Z

@hackaugusto It doesn't really do anything before #191 is merged, but this could be merged without #191. I believe I could get #191 ready pretty fast with a temporary fix until #232 is ready.

I will make the changes from your comments.

LefterisJP · 2016-12-19T14:06:08Z

+    address2 = tester.a2
+
+    # We need to close the channel before it can be deleted, to do so we need
+    # one transfer to call closeSingleTransfer(0


typo here: closeSingleTransfer(0 -> closeSingleTransfer()

LefterisJP · 2016-12-19T14:07:37Z

+    )
+    tester_state.mine(number_of_blocks=settle_timeout + 1)
+
+    # delete the channel needs to update the manager's state


delete -> deleting

LefterisJP · 2016-12-19T14:08:55Z

+
+    tester_state.mine(1)
+
+    # now a single new channel can be open


open -> opened

czepluch · 2016-12-19T17:18:27Z

Whoops. Introduced some errors with last commit. Looking into fixing them.

LefterisJP · 2016-12-20T08:36:11Z

        # 0 is used in the netting contract to represent the lack of a
        # transfer, so this value must start at 1
        if isinstance(get_block_number, int):
+            # if get_block_number == 0:


Please don't leave commented code inside a commit. Amend the commit and remove it.

Removed with latest commit. Please approve when checks are done.

hackaugusto · 2016-12-20T13:43:22Z

        our_state = ChannelEndState(
            channel_details['our_address'],
            channel_details['our_balance'],
+            self.raiden.chain.block_number,


I believe this should be the opened block instead of the current block.

This was already done when you reviewed this, no? At least it's done in my latest commit from before you made the comment.

hackaugusto · 2016-12-20T13:44:52Z

@@ -282,7 +282,7 @@ def __init__(self, participant_address, participant_balance):
        # sequential nonce, current value has not been used.
        # 0 is used in the netting contract to represent the lack of a
        # transfer, so this value must start at 1


we should update this comment.

, so this value must start at 1

to something like:

the nonce value must inside netting channel allowed range that is defined in terms of the opened block

czepluch · 2016-12-21T14:13:10Z

-                # do not call settle if already settled, the event polling
-                # might be lagging behind.
+                # do not call settle if already settled, the settle_event might
+                # not be set if the LogListener is lagging behind.


Vim fucked up somehow and did update the file correctly when fetching. So these lines are included in the commit, but I didn't write them.

Then amend the commit, delete them, and force push in your branch.

Cool. I didn't know the --amend command before now. The other times you've said amend the commit, I thought it was meant literally.

argh I see! That's why you had so many commits in other times. Good to see you know it now. Generally a lot of small commits is nice but not too many. If you just append something to a previous commit you should generally amend it and force push (only if it's your own feature branch ofcourse)

hackaugusto reviewed Dec 19, 2016

View reviewed changes

LefterisJP reviewed Dec 19, 2016

View reviewed changes

czepluch force-pushed the use_nonce_ranges branch from 662b619 to 84fc5c6 Compare December 19, 2016 15:36

czepluch added the dev: Please Review label Dec 19, 2016

czepluch added this to the MVP milestone Dec 19, 2016

LefterisJP suggested changes Dec 20, 2016

View reviewed changes

LefterisJP modified the milestones: Sprint 1, MVP Dec 20, 2016

czepluch force-pushed the use_nonce_ranges branch from 5f22faa to f7c99d3 Compare December 20, 2016 11:52

hackaugusto reviewed Dec 20, 2016

View reviewed changes

czepluch removed this from the Sprint 1 milestone Dec 20, 2016

konradkonrad mentioned this pull request Dec 20, 2016

Prevent BalanceProof replay attacks for different Registries and Ethereum networks #292

Closed

czepluch force-pushed the use_nonce_ranges branch from f7c99d3 to ead8731 Compare December 21, 2016 13:42

czepluch commented Dec 21, 2016

View reviewed changes

czepluch force-pushed the use_nonce_ranges branch from ead8731 to 02c5501 Compare December 21, 2016 14:24

czepluch self-assigned this Dec 21, 2016

czepluch added the dev: Ready To Merge label Dec 21, 2016

czepluch added 8 commits December 21, 2016 16:00

adding nonce ranges

ca86905

adding test for re opening of a channel plus nonce ranges

3c93da9

put each test method argument on its own line

2b46459

fixing typos

3e672dd

move nonce range statement to ChannelEndState constructor

9509080

handle int and instancemethod for block number

29f3d90

use opened block instead of current block

dacda6f

removing unused outcommented code

5603d6f

update comment describing the nonce range

6d6b4fd

LefterisJP force-pushed the use_nonce_ranges branch from 02c5501 to 6d6b4fd Compare December 21, 2016 15:01

LefterisJP approved these changes Dec 21, 2016

View reviewed changes

LefterisJP merged commit 93024fc into raiden-network:master Dec 21, 2016

This was referenced Dec 21, 2016

Harden messages against replay attacks #39

Closed

adding logic to allow to parties to open a new channel after settling #297

Merged

LefterisJP mentioned this pull request Jan 31, 2017

Get rid of the incremental nonce check in NettingChannelLibrary #365

Closed

3 tasks

hackaugusto removed dev: Please Review labels Feb 3, 2017

Conversation

czepluch commented Dec 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hackaugusto commented Dec 19, 2016

Uh oh!

czepluch commented Dec 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

czepluch commented Dec 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LefterisJP Dec 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LefterisJP Dec 21, 2016 •

edited

Loading