-
Notifications
You must be signed in to change notification settings - Fork 379
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
Use nonce ranges #287
Use nonce ranges #287
Conversation
@@ -534,6 +534,10 @@ def __init__(self, our_state, partner_state, external_state, | |||
self.settle_timeout = settle_timeout | |||
self.external_state = external_state | |||
|
|||
# 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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO these statements should be part of ChannelEndState constructor, and then we can move the nonce initialization to the assetmanager.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have an argument for why this would be preferred?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
|
||
@pytest.mark.xfail | ||
def test_reopen_channel(tester_state, tester_channelmanager, tester_channels, settle_timeout, | ||
netting_channel_abi): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, one argument per line :)
@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. |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo here: closeSingleTransfer(0
-> closeSingleTransfer()
) | ||
tester_state.mine(number_of_blocks=settle_timeout + 1) | ||
|
||
# delete the channel needs to update the manager's state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete -> deleting
|
||
tester_state.mine(1) | ||
|
||
# now a single new channel can be open |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
open -> opened
662b619
to
84fc5c6
Compare
Whoops. Introduced some errors with last commit. Looking into fixing them. |
@@ -283,8 +283,14 @@ def __init__(self, participant_address, participant_balance, get_block_number): | |||
# 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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't leave commented code inside a commit. Amend the commit and remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed with latest commit. Please approve when checks are done.
5f22faa
to
f7c99d3
Compare
@@ -119,10 +119,12 @@ def register_channel(self, netting_channel, reveal_timeout): | |||
our_state = ChannelEndState( | |||
channel_details['our_address'], | |||
channel_details['our_balance'], | |||
self.raiden.chain.block_number, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this should be the opened block instead of the current block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was already done when you reviewed this, no? At least it's done in my latest commit from before you made the comment.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
f7c99d3
to
ead8731
Compare
# 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then amend the commit, delete them, and force push in your branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
ead8731
to
02c5501
Compare
02c5501
to
6d6b4fd
Compare
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