Unclear handling of exceptions thrown in proxy's set_total_withdraw

[LefterisJP]

Problem Definition

While working on #4937 I noticed that the rest api calls the token network's set_total_withdraw() in an asynchronous way. Thus unlike for other proxy calls any exceptions thrown inside the proxy are not handled.

EDIT: Read the issue to figure out why this is actually not the case but also why this is still problematic. Keeping the description of what I initially thought to be a bug below so that the train of though of how we came here is preserved.

raiden/raiden/api/rest.py

Lines 1154 to 1165 in 6983c90

	try:
	self.raiden_api.set_total_channel_withdraw(
	registry_address,
	channel_state.token_address,
	channel_state.partner_state.address,
	total_withdraw,
	)
	except (NonexistingChannel, UnknownTokenAddress) as e:
	return api_error(errors=str(e), status_code=HTTPStatus.BAD_REQUEST)
	except (InsufficientFunds, WithdrawMismatch) as e:
	return api_error(errors=str(e), status_code=HTTPStatus.CONFLICT)

Here the rest api calls the python api's set_total_channel_withdraw

raiden/raiden/api/python.py

Lines 555 to 564 in 6983c90

	self.raiden.withdraw(
	canonical_identifier=channel_state.canonical_identifier, total_withdraw=total_withdraw
	)
	waiting.wait_for_withdraw_complete(
	raiden=self.raiden,
	canonical_identifier=channel_state.canonical_identifier,
	total_withdraw=total_withdraw,
	retry_timeout=retry_timeout,
	)

In there instead of calling the proxy directly what the code does is call raiden_service.withdraw()

raiden/raiden/raiden_service.py

Lines 1210 to 1217 in 6983c90

	def withdraw(
	self, canonical_identifier: CanonicalIdentifier, total_withdraw: WithdrawAmount
	) -> None:
	init_withdraw = ActionChannelWithdraw(
	canonical_identifier=canonical_identifier, total_withdraw=total_withdraw
	)
	self.handle_and_track_state_changes([init_withdraw])

Which as can be seen above creates an Action state change and adds it to the pending greenlets for processing.

raiden/raiden/transfer/channel.py

Lines 1607 to 1645 in 6983c90

	def send_withdraw_request(
	channel_state: NettingChannelState,
	total_withdraw: WithdrawAmount,
	block_number: BlockNumber,
	pseudo_random_generator: random.Random,
	) -> List[Event]:
	events: List[Event] = list()
	if get_status(channel_state) not in CHANNEL_STATES_PRIOR_TO_CLOSED:
	return events
	nonce = get_next_nonce(channel_state.our_state)
	expiration = get_safe_initial_expiration(
	block_number=block_number, reveal_timeout=channel_state.reveal_timeout
	)
	withdraw_state = PendingWithdrawState(
	total_withdraw=total_withdraw, nonce=nonce, expiration=expiration
	)
	channel_state.our_state.nonce = nonce
	channel_state.our_state.withdraws_pending[withdraw_state.total_withdraw] = withdraw_state
	withdraw_event = SendWithdrawRequest(
	canonical_identifier=CanonicalIdentifier(
	chain_identifier=channel_state.chain_id,
	token_network_address=channel_state.token_network_address,
	channel_identifier=channel_state.identifier,
	),
	recipient=channel_state.partner_state.address,
	message_identifier=message_identifier_from_prng(pseudo_random_generator),
	total_withdraw=withdraw_state.total_withdraw,
	participant=channel_state.our_state.address,
	nonce=channel_state.our_state.nonce,
	expiration=withdraw_state.expiration,
	)
	events.append(withdraw_event)
	return events

That results in processing the Action through the state machine and eventually generating a SendWithdrawRequest event as seen above.

This will send a WithdrawRequest to the partner .... wait for the confirmation ... process it ... and only during that processing will the proxy call be done. So the proxy's exceptions will not be seen.

Task

Figure out a way to do this properly and catch the exceptions.

raiden/raiden/raiden_event_handler.py

Lines 360 to 382 in 6983c90

	@staticmethod
	def handle_contract_send_channelwithdraw(
	raiden: "RaidenService", channel_withdraw_event: ContractSendChannelWithdraw
	) -> None:
	withdraw_confirmation_data = pack_withdraw(
	canonical_identifier=channel_withdraw_event.canonical_identifier,
	participant=raiden.address,
	total_withdraw=channel_withdraw_event.total_withdraw,
	expiration_block=channel_withdraw_event.expiration,
	)
	our_signature = raiden.signer.sign(data=withdraw_confirmation_data)
	channel_proxy = raiden.chain.payment_channel(
	canonical_identifier=channel_withdraw_event.canonical_identifier
	)
	channel_proxy.set_total_withdraw(
	total_withdraw=channel_withdraw_event.total_withdraw,
	expiration_block=channel_withdraw_event.expiration,
	participant_signature=our_signature,
	partner_signature=channel_withdraw_event.partner_signature,
	block_identifier=channel_withdraw_event.triggered_by_block_hash,
	)

Perhaps catch the exceptions in the handler of the ContractSendChannelWithdraw event?

[rakanalh]

Ahh, that is different than deposit in a way that the transaction will be invoked inside the event handler.
The event handler is invoked by the Raiden service here:

raiden/raiden/raiden_service.py

Lines 666 to 686 in 6983c90

	def _handle_event(self, chain_state: ChainState, raiden_event: RaidenEvent) -> None:
	assert isinstance(chain_state, ChainState)
	assert isinstance(raiden_event, RaidenEvent)
	try:
	self.raiden_event_handler.on_raiden_event(
	raiden=self, chain_state=chain_state, event=raiden_event
	)
	except RaidenRecoverableError as e:
	log.error(str(e))
	except InvalidDBData:
	raise
	except (RaidenUnrecoverableError, BrokenPreconditionError) as e:
	log_unrecoverable = (
	self.config["environment_type"] == Environment.PRODUCTION
	and not self.config["unrecoverable_error_should_crash"]
	)
	if log_unrecoverable:
	log.error(str(e))
	else:
	raise

So as you can see, the exception is actually being caught for all event handlers which trigger transaction calls.

[LefterisJP]

Hmmm but then why do you try to handle WithdrawMismatch which is a RecoverableError in the rest api call? This is misleading and led me down this rabbit hole.

Also the set_total_withdraw() proxy function raises two ValueError which are not caught.

raiden/raiden/network/proxies/token_network.py

Lines 1221 to 1225 in 6983c90

	if not isinstance(total_withdraw, int):
	raise ValueError("total_withdraw needs to be an integer number.")
	if total_withdraw <= 0:
	raise ValueError("total_withdraw should be larger than zero.")

These look like BrokenPreconditions to me.

[LefterisJP]

Also this difference in behaviour between function of the python API should be documented.

[LefterisJP]

Plus some tests to verify these exceptions are caught and handled properly should be written. We can turn the issue to that then. And not handle it as a bug.

[rakanalh]

Hmmm but then why do you try to handle WithdrawMismatch which is a RecoverableError in the rest api call? This is misleading and led me down this rabbit hole.

https://github.com/raiden-network/raiden/blob/6983c901ef9fe9d17985699b7c52cae3884b1345/raiden/api/python.py#L673-0L674

Because the python module basically raises the exception prior to calling the proxy. I agree that in this case it is confusing that both the proxy and the python API raise the same exception. The alternative was to introduce WithdrawMismatchBrokenPrecondition which leads us down to writing a specialized precondition for each case.

Also the set_total_withdraw() proxy function raises two ValueError which are not caught.
These look like BrokenPreconditions to me.

These are actually as you can see just type/value checks. Precondition errors are ones that are based on the data available on the blockchain at block_identifier. If we call the proxy with certain assumptions about the data and that turns out to be untrue, then we raise BrokenPreconditionError.

[rakanalh]

How about we close the issue and create another one for the tests?

[rakanalh]

Unclear handling of exceptions thrown in proxy's set_total_withdraw

I think the difference between RaidenError based exceptions & BrokenPreconditionError should be just put into an ADR or documentation page.

[LefterisJP]

These are actually as you can see just type checks. Precondition errors are ones that are based on the data available on the blockchain at block_identifier

I would like us to avoid using ValueError in the codebase. It's a very generic exception making it hard to catch and reason about.

Type checks like that are not needed. If a wrong type is given this is a programming error and it should be an assertion if anything. I would just remove them but you can make them into assertions too.

How about we close the issue and create another one for the tests?

Why? We can also use this one for the tests. I can edit the description. I like being able to see how we get lead to a problem.

In this case the complicated logic around withdraws and state machine and API.

[rakanalh]

Sure. ValueError was a legacy from the older implementation and i agree they could be turned into their own exceptions.

Type checks like that are not needed. If a wrong type is given this is a programming error and it should be an assertion if anything. I would just remove them but you can make them into assertions too.

Type checks exist all over the codebase & especially proxies. They're supposed to prevent invalid input at runtime. They are indeed programming errors and this how we deal with them. Using asserts will mean that any invalid user input could possibly crash the client which is undesired behaviour... think about it as not only types but also values.

[LefterisJP]

I think this code is not future-proof and is just waiting for bugs to happen. To make what I find very dangerous here clearer let us imagine the following scenario.

• I make a PR that adds new logic to the proxy's set_total_channel_withdraw()
• Inside that logic I add a new exception which just like in other places I want to catch from the rest API. We all look at how things are implemented around us in the module to implement something new.
• So looking at how exceptions are caught, I would just try to catch them like the rest of the Rest API leading to uncaught exceptions which would crash Raiden.
• If then the PR reviewer also thinks like me or has forgotten about that detail then we merge a bug in the codebase. Notice that even you who wrote the withdraw feature were convinced for some time (judging from your reaction in the chat) that this was a bug.

[LefterisJP]

Type checks exist all over the codebase & especially proxies. They're supposed to prevent invalid input at runtime. They are indeed programming errors and this how we deal with them. Using asserts will mean that any invalid user input could possibly crash the client which is undesired behaviour... think about it as not only types but also values.

Hmmm no no no that's not what I meant. We should definitely verify all user input. But if it's just a type check then this should either not exist, or it should be an assertion.

In short:

• Assertion should be used for things that should never happen. If that happens we are fucked. Something is really wrong ... probably programming error.
• All external input should be validated and if not valid should be rejected. If a direct user input is involved the user should also be informed as a result.
• Specific exceptions should be raised for specific problems like we do all over the codebase. Like things being invalid, out of range e.t.c.

For the specific example of the total_withdraw I would expect it to already have been validated in is_valid_withdraw_confirmation() of handle_receive_withdraw_confirmation().

I would expect external input coming from withdraw messages should be validated there and if not correct an EventInvalidReceivedWithdraw to be emitted.

At the moment this is not happening. So this is yet another thing that we should probably do.