Split test channel setup in two steps by hackaugusto · Pull Request #1392 · raiden-network/raiden

hackaugusto · 2018-05-02T04:49:39Z

First the apps which will have a channel are choosen, then the channel
is open and a deposit is made.

The split was such that each channel can be open and initialized by a
separated greenlet, and the routine to wait for the channel was extended
to also wait for the deposit.

coveralls · 2018-05-02T05:35:37Z

Coverage increased (+2.8%) to 75.464% when pulling 9867ccc on hackaugusto:split_channel_setup into 138c6a9 on raiden-network:master.

hackaugusto · 2018-05-03T01:54:25Z

I had to add the waiting for the balance in the network setup, otherwise the tests executed without the --blockchain-cache flag failed.

While fixing the waiting routine I made it possible to run the channel setup in parallel, to speed up the fixture setup, that exposed a bug in the nonce management in the JSONRPCClient. I removed some of the code from the rpc client to find the bug and simplified the logic, I believe the nonce update logic is useless, but I didn't want to remove it in this PR.

palango · 2018-05-03T13:03:04Z

-                # resets, force an update.
-                self.nonce_update_interval = query_time - self.nonce_update_interval
-                needs_update = True
+        # Python's 2.7 time is not monotonic and it's affected by clock resets,


there is now a monotonic clock in py 3.5 and above. Should we switch to that while you're at it?
https://docs.python.org/3/library/time.html#time.monotonic

what are the semantics of this clock? it should be walltime and not cpu cycles

Not exactly sure, just found this comment the last time I was working on the client. This is the PEP: https://www.python.org/dev/peps/pep-0418/#time-monotonic

palango · 2018-05-07T11:44:19Z

I'm reviewing this one.

palango

Really nice PR. Can you go a bit more in detail on why the nonce management is useless and create an follow-up issue for it.

palango · 2018-05-07T13:20:28Z


 @pytest.mark.parametrize('number_of_tokens', [0])
 @pytest.mark.parametrize('number_of_nodes', [1])
+@pytest.mark.parametrize('channels_per_node', [0])


Are these parametrisations used for the fixtures as well? I alsways thought it was the other way around.

the parametrize decorator will overwrite the default value of the fixture using whatever is inside the list.

https://docs.pytest.org/en/latest/fixture.html#parametrizing-fixtures

First the apps which will have a channel are choosen, then the channel is open and a deposit is made. The split was such that each channel can be open and initialized by a separated greenlet, and the routine to wait for the channel was extended to also wait for the deposit.

Simplified the send_transaction by removing the sender argument, since the private key is always provided.

Different transactions must use different nonces, otherwise a transactions may overwite each other or fail to be accepted. To avoid reusing the nonce this state is internal to the JSONRPCClient and must be protected from concurrent access. Using the same nonce will try to overwrite an existing transaction in the pool, this feature is not used at the moment in raiden.

Raiden must wait until all the sent transactions are registered as pending, therefor the condition should include the latest used nonce:: nonce <= self.nonce_current_value

There was no need for the retry to be in a decorator, since it depends on the JSONRPCClient implementation and it's used in a single place, therefor the decorator was removed and the logic inlined in the decorated funtion. The name call was clashing with the ContractProxy.call, to make the intent clearer the method JSONRPCClient.call was renamed

hackaugusto · 2018-05-07T14:50:26Z

Can you go a bit more in detail on why the nonce management is useless and create an follow-up issue for it.

Either we assume that we share the privatekey or not:

if we don't share it, then the nonce update is useless: The running Raiden node is the only one sending transactions with that key, therefor the only node changing the nonce, so the local nonce is always valid.
- assuming the ethereum node puts transactions in pending faster than Raiden restarts.
if we do share, then there will be races where two nodes send the different transactions with the same nonce, only updating the nonce without fixing and resending the transactions is not sufficient. At the best case scenario, the retry logic would also fix the transaction, but then either:
- the two nodes deadlock each other and waste ether, because one will try to overwrite the other transaction and needs to pay more for the gas
- the node which sent the rejected transaction accepts that it lost the race and sends a new transaction with a fresher nonce. This is IMO just too annoying, the effort is not worth and would be better to just /not/ use local signing.

hackaugusto added the dev: Please Review label May 2, 2018

hackaugusto force-pushed the split_channel_setup branch 2 times, most recently from cbbbb95 to 62e9e59 Compare May 2, 2018 05:09

hackaugusto changed the title ~~Split channel setup in two steps~~ Split test channel setup in two steps May 2, 2018

hackaugusto force-pushed the split_channel_setup branch 11 times, most recently from 628b230 to 902f727 Compare May 2, 2018 21:28

hackaugusto force-pushed the split_channel_setup branch from 2f2358b to 775bbba Compare May 3, 2018 02:52

palango reviewed May 3, 2018

View reviewed changes

palango approved these changes May 7, 2018

View reviewed changes

hackaugusto added 8 commits May 7, 2018 11:41

removed sender argument form send_transaction

0c0131d

Simplified the send_transaction by removing the sender argument, since the private key is always provided.

removed unused method eth_sendTransaction

03e2a92

removed unused argument from deploy_solidity_contract

1318a8b

bugfix: nonce queried from server

81f09a9

Raiden must wait until all the sent transactions are registered as pending, therefor the condition should include the latest used nonce:: nonce <= self.nonce_current_value

simplified nonce logic

3f69d43

hackaugusto force-pushed the split_channel_setup branch from 775bbba to 9867ccc Compare May 7, 2018 14:42

hackaugusto merged commit cd9d77e into raiden-network:master May 7, 2018

hackaugusto deleted the split_channel_setup branch May 7, 2018 15:21

LefterisJP mentioned this pull request May 7, 2018

Warn about using same private key from other applications and remove nonce management #1400

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split test channel setup in two steps#1392

Split test channel setup in two steps#1392
hackaugusto merged 8 commits into
raiden-network:masterfrom
hackaugusto:split_channel_setup

hackaugusto commented May 2, 2018

Uh oh!

coveralls commented May 2, 2018 •

edited

Loading

Uh oh!

hackaugusto commented May 3, 2018 •

edited

Loading

Uh oh!

palango May 3, 2018

Uh oh!

hackaugusto May 3, 2018

Uh oh!

palango May 3, 2018

Uh oh!

palango commented May 7, 2018

Uh oh!

palango left a comment

Uh oh!

palango May 7, 2018

Uh oh!

hackaugusto May 7, 2018

Uh oh!

hackaugusto commented May 7, 2018 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

hackaugusto commented May 2, 2018

Uh oh!

coveralls commented May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hackaugusto commented May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

palango May 3, 2018

Choose a reason for hiding this comment

Uh oh!

hackaugusto May 3, 2018

Choose a reason for hiding this comment

Uh oh!

palango May 3, 2018

Choose a reason for hiding this comment

Uh oh!

palango commented May 7, 2018

Uh oh!

palango left a comment

Choose a reason for hiding this comment

Uh oh!

palango May 7, 2018

Choose a reason for hiding this comment

Uh oh!

hackaugusto May 7, 2018

Choose a reason for hiding this comment

Uh oh!

hackaugusto commented May 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented May 2, 2018 •

edited

Loading

hackaugusto commented May 3, 2018 •

edited

Loading

hackaugusto commented May 7, 2018 •

edited

Loading