removed duplicated call

[hackaugusto]

The check channel_exists_and_not_settled is done by
_new_channel_preconditions just before, this second call is unecessary.

[LefterisJP]

Hmm no this is a postcondition check now after gas_limit returned None. Perhaps the channel was opened between self._new_channel_preconditions(partner, settle_timeout, 'pending') and the estimate_gas() call.

[hackaugusto]

True, however this is not the pattern we agreed on.

Note it's also possible that a new block happens to appear after the channel_exists_and_not_settled and by cheer bad luck it has the new channel, these checks are best effort only, to avoid obviously unnecessary transactions, but the checks will always race with the blockchain.

It really doesn't make sense to do it twice, we should just define a pattern "first check, then estimate gas" or the other way around, and stick to it. The real result will only be known after the transaction is sent and we check the receipt. (Edit: If we do decide to do the checks after the estimate gas, then we should do all of them, and then it would not make sense to have it before and after estimate gas)

[hackaugusto]

Hummm, I think I see what you mean.

You're saying this check is part of the:

known_race, msg = check_blockchain_conditions(mined_block if transaction_mined else pending_block)

From the pattern above, correct? (I will go through the code again)

[LefterisJP]

True, however this is not the pattern we agreed on.

Hmm that's not how I read our discussion. Pasting the pseudocode snippet here:

nonblockchain_preconditions()
blockchain_preconditions(consistent_block)  # this will also function as a check of the logic that lead us to start the call in the first place
gas = gas_estimate(pending_block)
if gas is not None:
  txhash = send_transaction()
  result = poll(txhash)
if gas is None or result is Failure:
    transaction_mined = gas is not None
    known_race, msg = check_blockchain_conditions(mined_block if transaction_mined else pending_block)
    if known_race:
        raise RaidenRecoverableError('{} {}'.format('Transaction Failed' if transaction_mined else 'Transaction will fail', msg))
    else:
        raise RaidenUnrecoverableError("Sad face")  

You can see that there are blockchain and other precondition checks before estimate gas. And then blockchain checks again after failure (either estimate or transaction send failure).

Edit: If we do decide to do the checks after the estimate gas, then we should do all of them, and then it would not make sense to have it before and after estimate gas)

This is similar to what I had proposed before in our conversation here where I proposed to start with estimateGas and only if that fails check conditions.

To that you asked:

Why would you want to do the estimate gas before the preconditions checks? What does this achieve?

from which we went to the approach we agreed on.

---

But looking at the current version of proxies/token_network.sol it seems that new_netting_channel() is the only call in proxies which does not follow the pattern we agreed on above, so it definitely needs adjustment. The second (postconditions) checks should be under a combined estimateGas/transaction sent failure. Not duplicated in both cases.

So the code here:

raiden/raiden/network/proxies/token_network.py

Lines 228 to 238 in 032bdcf

	if not gas_limit:
	channel_exists = self.channel_exists_and_not_settled(
	participant1=self.node_address,
	participant2=partner,
	block_identifier='pending',
	)
	if channel_exists:
	raise DuplicatedChannelError('Duplicated channel')
	log.critical('Call to openChannel will fail', **log_details)
	raise RaidenUnrecoverableError('Call to openChannel will fail')

needs to be combined with the code here:

raiden/raiden/network/proxies/token_network.py

Lines 257 to 265 in 032bdcf

	if receipt_or_none:
	known_race, msg = self._new_channel_postconditions(
	partner=partner,
	block=receipt_or_none['blockNumber'],
	)
	if known_race:
	raise DuplicatedChannelError(msg)
	log.critical('new_netting_channel failed', **log_details)
	raise RaidenUnrecoverableError('creating new channel failed')

and here:

raiden/raiden/network/proxies/token_network.py

Lines 280 to 301 in 032bdcf

	self.proxy.jsonrpc_client.check_for_insufficient_eth(
	transaction_name='openChannel',
	transaction_executed=False,
	required_gas=GAS_REQUIRED_FOR_OPEN_CHANNEL,
	block_identifier='pending',
	)
	known_race, msg = self._new_channel_postconditions(
	partner=partner,
	block='pending',
	)
	if known_race:
	raise DuplicatedChannelError(msg)
	log.critical('new_netting_channel call will fail', **log_details)
	raise RaidenUnrecoverableError('Creating a new channel will fail')
	channel_identifier: ChannelID = self.detail_channel(
	participant1=self.node_address,
	participant2=partner,
	block_identifier='latest',
	).channel_identifier
	log_details['channel_identifier'] = str(channel_identifier)
	log.info('new_netting_channel successful', **log_details)

[hackaugusto]

yeah, you're right