Token network registry preconditions by rakanalh · Pull Request #4936 · raiden-network/raiden

rakanalh · 2019-09-20T09:49:44Z

Merge after #4876

Fixes: #4889
Fixes: #4888
Fixes: #4886
Fixes: #4884
Fixes #4882

Description

Please, describe what this PR does in detail:

If it's a bug fix, detail the root cause of the bug and how this PR fixes it.
If it's a new feature, describe the feature this PR is introducing and design decisions.
If it's a refactoring, describe why it is necessary. What are its pros and cons in respect to the previous code, and other possible design choices.

PR review check list

Quality check list that cannot be automatically verified.

Safety
- The changes respect the necessary conditions for safety (https://raiden-network-specification.readthedocs.io/en/latest/smart_contracts.html#protocol-values-constraints)
Code quality
- Error conditions are handled
- Exceptions are propagated to the correct parent greenlet
- Exceptions are correctly classified as recoverable or unrecoverable
Compatibility
- State changes are forward compatible
- Transport messages are backwards and forward compatible
Commits
- Have good messages
- Squashed unecessary commits
If it's a bug fix:
- Regression test for the bug
  - Properly covers the bug
  - If an integration test is used, it could not be written as a unit test
Documentation
- A new CLI flag was introduced, is there documentation that explains usage?
Specs
- If this is a protocol change, are the specs updated accordingly? If so, please link PR from the specs repo.
Is it a user facing feature/bug fix?
- Changelog entry has been added

hackaugusto · 2019-09-20T12:08:46Z

+            raise InvalidTokenAddress("The call to register a token at 0x00..00 will fail.")
+
+        token_proxy = self.blockchain_service.token(token_address)
        try:


Checklist of the preconditions:

canCreateTokenNetwork modified

require(token_to_token_networks[_token_address] == address(0x0));, done by already_registered = self.get_token_network

require(_token_address != address(0x0));, done above by if token_address == NULL_ADDRESS_BYTES:

require(_secret_registry != address(0x0));, checked implicitly, if the registry exist this value is not zero.

require(_deprecation_executor != address(0x0));, checked implicitly, if the registry exist this value is not zero.

require(_chain_id > 0);, checked implicitly, if the registry exist this value is not zero.

require(_settlement_timeout_min > 0);, checked implicitly, if the registry exist this value is not zero.

require(_settlement_timeout_max > _settlement_timeout_min);, checked implicitly, if the registry exist this value is greater than the min.

require(contractExists(_token_address));, checked implicitly, if the proxy exist the address was checked for code.

require(contractExists(_secret_registry));, checked implicitly, if the registry exist this value is greater than the min.

require(_token_network_deposit_limit > 0);

require(_token_network_deposit_limit >= _channel_participant_deposit_limit);

require(token.totalSupply() > 0);, done by token_supply = token_proxy.total_supply(block_identifier=block_identifier).

btw, the checks for:

require(_token_network_deposit_limit > 0); require(_token_network_deposit_limit >= _channel_participant_deposit_limit);

can be done above, together with the check for the NULL_ADDRESS_BYTES.

hackaugusto · 2019-09-20T12:23:04Z

+
+        token_proxy = self.blockchain_service.token(token_address)
        try:
+            token_supply = token_proxy.total_supply(block_identifier=block_identifier)


side note, can you try this out and see if the proxy will raise an exception instead of returning ""? The RPC call does return "", and back in the day when we used the JSONRPCClient to do these requests that was the value we got. However, I believe that web3 will actually check if the return type of the called function matches the value returned by the RPC call, and if the types don't match I believe it will raise an exception. In this case the total_supply ABI says the return type is an int, so I think that an exception will be raised here. You can easily check that by instantiating a Token proxy using the TokenNetworkRegistry address and calling the function.

Created an issue for this: #4945

- Follow the token network pruned blocks handling pattern - Added transaction / gas estimation failure checks

pirapira · 2019-09-23T09:49:11Z

+                    "The chain ID property for the TokenNetworkRegistry is invalid."
+                )
+
+            if chain_id != self.blockchain_service.network_id:


I think we should not check this. After a fork, there will be a fork with the original chain_id and another fork with a different chain_id. The choice is on the user.

That's not something we have to handle. Because when the user runs Raiden, it is ran on a specific chain ID. If there was a fork at some point and the user choose to use that new chain, Raiden has to be restarted to use that chain which will have a new ID.

We deploy the contract on chain_id == 1 and store chain_id == 1 in the contract

A fork happens into chain_ids 1 and 231.

A user tries to use the smart contract on chain 231.

In this case, the smart contract still remembers chain_id == 1 (that's the left hand side). But blockchain_service.network_id points to 231 because now the user is trying to use chain_id 231. So the check fails here. If the proxy allows, the transaction would succeed onchain because on-chain we don't check that the chain_id is a particular value.

I already know that the user runs Raiden on a specific chain ID, but I see the problem ^.

Isn't it also a problem that we store the chain id in the contract though? If there is a fork and both sides survive (let's say 1 and 231 per your example), the contracts would need to be redeployed in 231 chain.

If they are not then the on-chain chain_id would be wrong and as such all functions using it such as recoverAddressFromBalanceProof() would not work.

Currently, the onchain chain_id would be wrong. That's why no strict checks should be performed with it.

This point is kept track in #4956

pirapira · 2019-09-23T09:51:41Z

+                        "The chain ID property for the TokenNetworkRegistry is invalid."
+                    )
+
+                if chain_id != self.blockchain_service.network_id:


Here too, this check is nowhere in smart contracts. And I think Raiden should be usable on both chains after a fork.

This point is kept track in #4956

pirapira · 2019-09-23T12:24:34Z


-        # check preconditions
+        if token_network_deposit_limit <= 0:
+            raise InvalidTokenNetworkDepositLimit(


I heard that the proxies should not raise specific errors. This should be BrokenPreconditionError. #4878 (comment)

@pirapira No... it seems like the definition is still vague. This should not be a precondition error.

token_network_deposit_limit > 0 is a precondition. Here we're in a proxy and checking a precondition before estimating the gas. So the exception should be BrokenPreconditionError.

This point is kept track in #4957

codecov · 2019-09-23T13:54:13Z

Codecov Report

Merging #4936 into develop will decrease coverage by 0.4%.
The diff coverage is 42.3%.

@@             Coverage Diff             @@
##           develop    #4936      +/-   ##
===========================================
- Coverage    80.79%   80.39%   -0.41%     
===========================================
  Files          120      120              
  Lines        14514    14604      +90     
  Branches      2238     2265      +27     
===========================================
+ Hits         11727    11741      +14     
- Misses        2134     2193      +59     
- Partials       653      670      +17

Impacted Files	Coverage Δ
raiden/exceptions.py	`100% <100%> (ø)`	⬆️
raiden/app.py	`95% <100%> (+0.26%)`	⬆️
raiden/network/proxies/token_network_registry.py	`52.63% <40.47%> (-24.52%)`	⬇️
raiden/waiting.py	`81.71% <0%> (-4%)`	⬇️
raiden/utils/__init__.py	`82.69% <0%> (-0.97%)`	⬇️
raiden/tasks.py	`66.91% <0%> (-0.74%)`	⬇️
raiden/raiden_event_handler.py	`91.84% <0%> (-0.63%)`	⬇️
raiden/transfer/channel.py	`89% <0%> (-0.44%)`	⬇️
raiden/transfer/mediated_transfer/mediator.py	`90.25% <0%> (-0.2%)`	⬇️
raiden/network/pathfinding.py	`87.08% <0%> (-0.06%)`	⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0490408...1efc4b6. Read the comment docs.

pirapira · 2019-09-23T13:56:39Z

+class InvalidTokenNetworkDepositLimit(RaidenError):
+    """ Raised when an invalid token network deposit
+    limit is passed to the token network registry proxy.
+    """


This can happen only when the API didn't check enough conditions. This should crash the node with BrokenPreconditionError.

pirapira · 2019-09-23T13:57:07Z

+class InvalidChannelParticipantDepositLimit(RaidenError):
+    """ Raised when an invalid channel participant
+    deposit limit is passed to the token network registry proxy.
+    """


This can only happen when the API didn't check the condition. This situation should crash the node with BrokenPreconditionError.

auto-assign Bot requested a review from LefterisJP September 20, 2019 09:49

rakanalh added the dev: Work In Progress label Sep 20, 2019

hackaugusto reviewed Sep 20, 2019

View reviewed changes

pirapira mentioned this pull request Sep 23, 2019

Checks against chain_id == 0 in TokenNetworkRegistry #4887

Closed

13 tasks

rakanalh added 10 commits September 23, 2019 11:40

Add precondition checks for the token network registry

9edcc07

Refactor the token network registry

a02ea7e

- Follow the token network pruned blocks handling pattern - Added transaction / gas estimation failure checks

Remove string matching from exception in tests (error prone)

325970c

Fix gas limit keys

560d6ba

Check that chain id matches the one Raiden is running on

49b9c55

use gas_measurements.get to protect against typos

b5a0313

Introduce exceptions for invalid deposit limits

2b019ca

Remove redundant settlement timeout methods, use existing

434779c

Validate deposit limits and max token networks vs created

ace16c8

Fix token network registry tests

2861539

rakanalh mentioned this pull request Sep 23, 2019

Test that total_supply call returns "" vs raise an exception #4945

Open

2 tasks

pirapira suggested changes Sep 23, 2019

View reviewed changes

pirapira mentioned this pull request Sep 23, 2019

TokenNetworkRegistry should check the deprecation executor address before/after sending a tx #4884

Closed

rakanalh requested a review from hackaugusto September 23, 2019 09:57

rakanalh added dev: Please Review and removed dev: Work In Progress labels Sep 23, 2019

rakanalh removed the request for review from LefterisJP September 23, 2019 09:58

pirapira suggested changes Sep 23, 2019

View reviewed changes

pirapira mentioned this pull request Sep 23, 2019

TokenNetworkRegistry proxy checks against secret_registry_address zero #4883

Closed

13 tasks

Wait for the block at which token was deployed to be confirmed

1efc4b6

pirapira suggested changes Sep 23, 2019

View reviewed changes

hackaugusto approved these changes Sep 23, 2019

View reviewed changes

hackaugusto merged commit 5a0109a into raiden-network:develop Sep 23, 2019

rakanalh deleted the token-network-registry-preconditions branch September 23, 2019 14:02

This was referenced Sep 23, 2019

Remove checks that are not in the onchain contract #4956

Merged

Raise BrokenPreconditionError on invalid inputs #4957

Closed

Conversation

rakanalh commented Sep 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge after #4876

Description

PR review check list

Uh oh!

hackaugusto Sep 20, 2019 • edited by rakanalh Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Sep 23, 2019

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rakanalh commented Sep 20, 2019 •

edited

Loading

hackaugusto Sep 20, 2019 •

edited by rakanalh

Loading