Treat trying to deposit on a closed channel as recoverable error #4693

LefterisJP · 2019-08-30T11:05:28Z

Maybe fix #4451

Description

If a channel is not open, or is in a closing state (still open but
waiting on the closing transaction result) the set_total_deposit
function was throwing a ValueError.

That is a valid and recoverable race condition so this commit
introduces an appropriate error and handles it in all places where it
can be thrown.

From the stack trace shown in #4451 this seems to have been the problem but without logs it's hard to say with 100% certainty.

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
Is it a user facing feature/bug fix?
- Changelog entry has been added

Maybe fix raiden-network#4451 If a channel is not open, or is in a closing state (still open but waiting on the closing transaction result) the `set_total_deposit` function was throwing a `ValueError`. That is a valid and recoverable race condition so this commit introduces an appropriate error and handles it in all places where it can be thrown.

LefterisJP · 2019-08-30T11:07:10Z

raiden/api/rest.py

@@ -646,7 +647,7 @@ def open(
                return api_error(errors=str(e), status_code=HTTPStatus.PAYMENT_REQUIRED)
            except (NonexistingChannel, UnknownTokenAddress) as e:
                return api_error(errors=str(e), status_code=HTTPStatus.BAD_REQUEST)
-            except (DepositOverLimit, DepositMismatch) as e:
+            except (DepositOverLimit, DepositMismatch, ChannelNotOpenError) as e:


I am not sure about handling it here. Since this is right after opening in the same function I don't see how a race with the channel being closed can happen here.

But I opted to handle it here too just to be safe.

codecov · 2019-08-30T11:24:55Z

Codecov Report

Merging #4693 into develop will decrease coverage by 0.03%.
The diff coverage is 60%.

@@             Coverage Diff             @@
##           develop    #4693      +/-   ##
===========================================
- Coverage    80.86%   80.83%   -0.04%     
===========================================
  Files          118      118              
  Lines        14236    14226      -10     
  Branches      2197     2196       -1     
===========================================
- Hits         11512    11499      -13     
- Misses        2073     2089      +16     
+ Partials       651      638      -13

Impacted Files	Coverage Δ
raiden/connection_manager.py	`70.12% <ø> (ø)`	⬆️
raiden/api/python.py	`68.43% <100%> (+0.18%)`	⬆️
raiden/exceptions.py	`96.61% <100%> (+0.05%)`	⬆️
raiden/api/rest.py	`73% <33.33%> (-0.32%)`	⬇️
raiden/waiting.py	`81.54% <0%> (-4.17%)`	⬇️
raiden/network/proxies/token_network.py	`50.12% <0%> (-1.58%)`	⬇️
raiden/transfer/events.py	`97.12% <0%> (-0.12%)`	⬇️
raiden/network/transport/matrix/transport.py	`80.98% <0%> (+0.29%)`	⬆️
raiden/transfer/node.py	`94.75% <0%> (+0.5%)`	⬆️
... and 3 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 e8f0ac7...781ad64. Read the comment docs.

rakanalh

I am not confident this is the actual solution to the problem.

  File "raiden/connection_manager.py", line 322, in _join_partner
  File "raiden/api/python.py", line 610, in set_total_channel_deposit
ValueError: Channel is not in an open state.

are the relevant lines.

According to the stacktrace, the user was trying to "Join the network" by opening channel.

Looking at the code:

raiden/raiden/connection_manager.py

Line 307 in e8f0ac7

self.api.channel_open(self.registry_address, self.token_address, partner)

The call here should actually wait until the channel is actually opened.

raiden/raiden/api/python.py

Lines 439 to 445 in e8f0ac7

    
           waiting.wait_for_newchannel( 
        
               raiden=self.raiden, 
        
               token_network_registry_address=registry_address, 
        
               token_address=token_address, 
        
               partner_address=partner_address, 
        
               retry_timeout=retry_timeout, 
        
           )

Then directly after that, the deposit starts.

raiden/raiden/connection_manager.py

Lines 317 to 323 in e8f0ac7

    
           try: 
        
               self.api.set_total_channel_deposit( 
        
                   registry_address=self.registry_address, 
        
                   token_address=self.token_address, 
        
                   partner_address=partner, 
        
                   total_deposit=total_deposit, 
        
               )

It is plausible that the partner closed the channel directly after it was created but i would say this is highly unlikely given the description in the issue and the verbal communication we got from @pirapira.

I would try to reproduce the issue before rushing into a solution. @pirapira can you help with that?

rakanalh · 2019-08-30T12:40:43Z

raiden/exceptions.py

@@ -189,6 +189,10 @@ class DuplicatedChannelError(RaidenRecoverableError):
    """Raised if someone tries to create a channel that already exists."""


+class ChannelNotOpenError(RaidenRecoverableError):


Nitpick: I would generalize this to something like InvalidChannelState

Hmmm perhaps unexpected channel state? Invalid would mean that it's something which we can't even parse.

LefterisJP · 2019-08-30T12:46:04Z

It is plausible that the partner closed the channel directly after it was created but i would say this is highly unlikely given the description in the issue and the verbal communication we got from @pirapira.

I also think this is highly unlikely but under the circumstances and lack of logs this is all I can say about #4451 at this point.

The potential race condition is there though and I think that replacing the ValueError with something better and handling this as a recoverable error just like it should be is a good move.

pirapira · 2019-08-30T12:48:46Z

If the issue depended on partner's behavior, maybe I need to reproduce the issue with the person on the other side of the channel. I try to find logs. How is a typical log file named?

LefterisJP · 2019-08-30T12:49:36Z

It looks like this: raiden-debug_2019-08-05T13:01:28.996726.log

pirapira · 2019-08-30T12:50:14Z

/home/yoichi/Downloads/home/pirate/Documents/Raiden/Binaries/Wizard/raiden-debug_2019-07-25T09:40:33.055353.log

pirapira · 2019-08-30T12:54:21Z

Here it is https://gist.github.com/pirapira/210c4b883134d6f42f1c7a36faa6e0bb

hackaugusto · 2019-09-03T10:12:02Z

I'm approving because a review from me was requested. I did not check the original issue to know if this PR fixes it, but the changes look fine to me.

Augusto approved and as discussed the issue may be solved from this.

LefterisJP added 2 commits August 30, 2019 12:22

Add test to see that the channel not open error is raised

6308029

LefterisJP commented Aug 30, 2019

View reviewed changes

LefterisJP added the dev: Please Review label Aug 30, 2019

rakanalh self-requested a review August 30, 2019 12:25

rakanalh previously requested changes Aug 30, 2019

View reviewed changes

ChannelNotOpenError -> UnexpectedChannelState

781ad64

LefterisJP force-pushed the workon_4451 branch from b242764 to 781ad64 Compare August 30, 2019 14:14

Dominik1999 requested a review from hackaugusto September 3, 2019 08:35

hackaugusto approved these changes Sep 3, 2019

View reviewed changes

LefterisJP merged commit 33a6f52 into raiden-network:develop Sep 3, 2019

LefterisJP deleted the workon_4451 branch September 3, 2019 10:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Treat trying to deposit on a closed channel as recoverable error #4693

Treat trying to deposit on a closed channel as recoverable error #4693

LefterisJP commented Aug 30, 2019

LefterisJP Aug 30, 2019

codecov bot commented Aug 30, 2019 •

edited

Loading

rakanalh left a comment

rakanalh Aug 30, 2019

LefterisJP Aug 30, 2019

LefterisJP commented Aug 30, 2019

pirapira commented Aug 30, 2019

LefterisJP commented Aug 30, 2019

pirapira commented Aug 30, 2019

pirapira commented Aug 30, 2019

hackaugusto commented Sep 3, 2019

	waiting.wait_for_newchannel(
	raiden=self.raiden,
	token_network_registry_address=registry_address,
	token_address=token_address,
	partner_address=partner_address,
	retry_timeout=retry_timeout,
	)

	try:
	self.api.set_total_channel_deposit(
	registry_address=self.registry_address,
	token_address=self.token_address,
	partner_address=partner,
	total_deposit=total_deposit,
	)

		@@ -189,6 +189,10 @@ class DuplicatedChannelError(RaidenRecoverableError):
		"""Raised if someone tries to create a channel that already exists."""


		class ChannelNotOpenError(RaidenRecoverableError):

Treat trying to deposit on a closed channel as recoverable error #4693

Treat trying to deposit on a closed channel as recoverable error #4693

Conversation

LefterisJP commented Aug 30, 2019

Description

PR review check list

LefterisJP Aug 30, 2019

Choose a reason for hiding this comment

codecov bot commented Aug 30, 2019 • edited Loading

Codecov Report

rakanalh left a comment

Choose a reason for hiding this comment

rakanalh Aug 30, 2019

Choose a reason for hiding this comment

LefterisJP Aug 30, 2019

Choose a reason for hiding this comment

LefterisJP commented Aug 30, 2019

pirapira commented Aug 30, 2019

LefterisJP commented Aug 30, 2019

pirapira commented Aug 30, 2019

pirapira commented Aug 30, 2019

hackaugusto commented Sep 3, 2019

codecov bot commented Aug 30, 2019 •

edited

Loading