Model smart contract function calls as commands

[hackaugusto]

Abstract

To support Overwrite in-flight transactions in-flight transactions #2801, Transaction prioritization #2802, and Transaction batching #4063 it's necessary an object to represent transactions, much like the ContractSend* state changes, but that can be manipulated by a transaction manager.

Additionally, the current implementation of the proxies are cumbersome, as of 1466689 the token network proxy has 1238 lines, and it's increasing.

Motivation

This is a proposal to enable the issues and simplify the current code base, by representing each possible transaction as a command instance.

Specification

I have a non working prototype bellow, which should be enough to understand the design I have in mind.

from abc import ABC, abstractmethod


class TXIdentifier(typing.Namedtuple):
    txhash: TXHash
    # Nonce if local signing is used, an unique value per rpcclient otherwise
    # (necessary to increase the gasprice of a transaction)
    cookie: Cookie


class Transaction(abc.ABC):
    def __init__(
            self,
            rpclient: RPCClient,
            contract: Address,
            function_selector: FunctionSelector,
            data: bytes
    ):
        self.rpcclient = rpclient
        self.contract = contract
        self.function_selector = function_selector
        self.data = data

    @abstractmethod
    def approximated_startgas(self):
        """ Estimated startgas based on local tests.

        This could be done by using py-evm, or by approximation formulas
        based on the smart contract tests.
        """
        pass

    @abstractmethod
    def validate_preconditions(self, blockhash: BlockHash):
        """ Raises if sending this transaction will result in an execution
        failure at the given `blockhash`.

        raises:
            UnrecoverableError: If the required preconditions have never been
                met to successfully execute the transaction.
            RecoverableError: If the required preconditions have been
                invalidated and sending the transaction would fail.
        """
        pass

    @absctractmehod
    def validate_after_failure(self, blockspec: BlockSpec):
        """ Raises an exception describing the reason for the transaction
        failure.

        raises:
            RecoverableError: If this transaction raced and lost with another
                transaction, which invalidated the required preconditions after the
                given block.
        """
        pass

    def send(
            self,
            blockhash: BlockHash,
            gasprice: GasPrice,
            nonce: Nonce = None,
    ):
        self.validate_preconditions(blockhash)
        startgas = self.rpcclient.gasestimate()
        if startgas is not None:
            self.gasprice = gasprice
            self.txidentifier: TXIdentifier = self.rpclient.send(startgas, gasprice)
        else:
            self.validate_after_failure('latest')
            if self.client.balance() < self.approximated_startgas():
                raise UnrecoverableError('insufficient balance')
            raise UnrecoverableError('unexpected error or insufficient balance')

    def increase_gasprice(self, gasprice: GasPrice):
        """ Try to resend this this transaction with a higher gasprice to
        increase the likelyhood of it being mined.
        """
        assert self.txidentifier is not None
        assert self.gasprice and self.gasprice < gasprice
        self.gasprice = gasprice
        self.rpclient.send(startgas, gasprice, txidentifier=self.txidentifier)

    def replace(self, blockhash: BlockHash, other: Transaction) -> TXHash:
        """ Try to replace `other` with this transaction.

        `other` maybe a transaction for the exact same
        """
        assert other.gasprice
        self.send(
            blockhash=blockhash,
            gasprice=other.gasprice + 1,
            nonce=other.nonce,
        )


class TransactionManager:
    def __init__(self, blockhash: BlockHash):
        self.heap = list()
        self.blockhash
        self.gasprice = self.estimate_gasprice()

    def block_callback(self):
        """ When a new block is mined, upgrade local state and check if a
        transaction has been mined.

        This should be installed as a callback to the AlarmTask.
        """
        self.update()  # updated confirmed blockhash and gasprice estimate
        self.poll_transactions()  # check if a pending transaction has been mined

    def poll_transactions(self):
        if self.pending:
            result = self.rpcclient.poll(self.pending)

            if isinstance(result, RPCFailure):
                # This needs additional handling, since the RPCFailure could be that we
                # tried to replace a transaction, but it failed (the previous transaction got
                # mined first).
                raise result.exception

            if isinstance(result, TransactionFailure):
                self.pending.validate_after_failure()
                raise UnrecoverableError('unexpected error')

            if isinstance(result, Success):
                heap.pop(self.heap, self.pending)
                self.maybe_send_next()

            elif isinstance(result, Pending) and self.has_to_increase_gas(self.pending):
                # gasprice updated by demand on new blocks
                self.pending.increase_gasprice(self.gasprice)

    def add_transaction(self, transaction: Transaction):
        heap.push(self.heap, transaction)

        if self.pending is None:
            self.pending = transaction
            transaction.send(self.blockhash, self.gasprice)
        elif higher_priority(transaction, self.pending):
            self.pending = transaction.replace(self.blockhash, self.pending)

    def maybe_send_next(self):
        if self.pending is None:
            self.pending = heap.peek(self.heap)
            self.pending.send(self.blockhash, self.gasprice)


class SetTotalDepositTransaction(Transaction):
    def __init__(
            self,
            channel_identifier: ChannelID,
            total_deposit: TokenAmount,
            our_address: Address,
            partner: Address,
    ):
        if not isinstance(total_deposit, int):
            raise ValueError('total_deposit needs to be an integral number.')

        self.channel_identifier = channel_identifier
        self.total_deposit = total_deposit
        self.our_address = our_address
        self.partner = partner

    def validate_preconditions(self, blockhash: BlockHash):
        onchannel_identifier = self.proxy.get_channel_identifier()

        if onchain_channel_identifier != self.channel_identifier:
            raise ChannelOutdatedError(
                'Current channel identifier is outdated. '
                f'current={self.channel_identifier}, '
                f'new={onchain_channel_identifier}',
            )

        previous_total_deposit = self.proxy.participant_deposit()

        if total_deposit < previous_total_deposit:
            msg = (
                f'Current total deposit ({previous_total_deposit}) is already larger '
                f'than the requested total deposit amount ({total_deposit})'
            )
            log.info('setTotalDeposit failed', reason=msg, **log_details)
            raise DepositMismatch(msg)

        current_balance = self.proxy.balance_of(self.our_address)
        if current_balance < amount_to_deposit:
            msg = (
                f'new_total_deposit - previous_total_deposit =  {amount_to_deposit} can not '
                f'be larger than the available balance {current_balance}, '
                f'for token at address {pex(token_address)}'
            )
            log.info('setTotalDeposit failed', reason=msg, **log_details)
            raise DepositMismatch(msg)

    def validate_after_failure(self, blockspec: BlockSpec):
        if token.allowance(self.node_address, self.address) < amount_to_deposit:
            log_msg = (
                'The allowance is insufficient, '
                'check concurrent deposits for the same token network '
                'but different proxies.'
            )
        elif token.balance_of(self.node_address) < amount_to_deposit:
            log_msg = "The address doesn't have enough funds"
        elif latest_deposit < total_deposit:
            log_msg = 'The tokens were not transferred'
        else:
            log_msg = 'unknown'

        log.critical('setTotalDeposit failed', reason=log_msg, **log_details)

        if channel_state in (ChannelState.NONEXISTENT, ChannelState.REMOVED):
            raise RaidenUnrecoverableError(
                f'Channel between participant {participant1} '
                f'and {participant2} does not exist',
            )
        elif channel_state == ChannelState.SETTLED:
            raise RaidenUnrecoverableError(
                'Deposit is not possible due to channel being settled',
            )
        elif channel_state == ChannelState.CLOSED:
            raise RaidenRecoverableError(
                'Channel is already closed',
            )
        elif participant_details.our_details.deposit < deposit_amount:
            raise RaidenUnrecoverableError(
                'Deposit amount did not increase after deposit transaction',
            )

Pros

• The design helps to think about error handling, by following a single pattern like the one described here
• Improved polling, since the poll is done on the alarm task callback and not on a tight loop with an arbitrary timeout.
• Allows the for transaction reordering and resending

Cons

While I was writing the above mock code, I realized this design will work only for one transaction at the time, so the deposit above has to be done in a single transaction, for the approve and setTotalDeposit.

Alternatively the design could be extended to allow for ordering of transactions.

Backwards Compatibility

This does not change any wire or database data. However, as state in cons it may require raiden-network/raiden-contracts#400 to be finalized, which will required upgrades to the smart contracts.

[hackaugusto]

First refinement, only changed methods are shown:

class Transaction(abc.ABC):
    # removed validate_preconditions and validate_after_failure

    @abstractmethod
    def preconditions(self, blockhash: BlockHash):
        """ Raises if the required preconditions for the given transaction have
        never been met.

        Used for things like, interacting with an address which was never a
        smart contract, or trying to call a function before it's preconditions
        have been met.

        Note:
            This does not raise if the preconditions were met in the past,
            but got invalidated.

        raises:
            UnrecoverableError: If the required preconditions have never been
                met to successfully execute the transaction.
        """
        pass

    # recoverable errors should not raise exceptions. This was done previously
    # because of control flow, to avoid refactoring return type of proxies
    # methods
    @absctractmehod
    def is_valid(self, blockspec: BlockSpec) -> Tuple[bool, Optional[Reason]]:
        """ Checks if the transaction can be executed, or if got invalidated
        meanwhile.

        This is called on a transaction which satisfied it's preconditions, so
        at some point it was valid to execute the transaction, this function
        checks if that is still the case.

        If the transaction can be executed a tuple `(True, None)` is returned,
        otherwise a tuple `(False, reason)` is returned, where `reason` is an
        estimation of what invalidated the transaction. If the reason for the
        failure is not detected, `(False, None)` is returned.
        """
        pass

    def send(
            self,
            blockhash: BlockHash,
            gasprice: GasPrice,
            nonce: Nonce = None,
    ):
        # Shouldn't wait until the transaction is queue by the manager to check
        # it's preconditions. Doing the check here would move the error reporting
        # temporarily and spatially, making debugging unnecessarily harder

        startgas = self.rpcclient.gasestimate()
        if startgas is not None:
            self.gasprice = gasprice
            self.txidentifier: TXIdentifier = self.rpclient.send(startgas, gasprice)
        else:
            self.validate_after_failure('latest')
            if self.client.balance() < self.approximated_startgas():
                raise UnrecoverableError('insufficient balance')
            raise UnrecoverableError('unexpected error or insufficient balance')


class TransactionManager:
    def poll_transactions(self):
        if self.pending:
            result = self.rpcclient.poll(self.pending)

            if isinstance(result, RPCFailure):
                # This needs additional handling, since the RPCFailure could be that we
                # tried to replace a transaction, but it failed (the previous transaction got
                # mined first).
                raise result.exception

            if isinstance(result, TransactionFailure):
                valid, reason = self.pending.is_valid()

                if valid:
                    raise UnrecoverableError('valid transaction failed to execute')
                else:
                    if reason is None:
                        raise UnrecoverableError('unexpected error')
                    log(reason)

            if isinstance(result, Success):
                heap.pop(self.heap, self.pending)
                self.pending = None
                self.maybe_send_next()

            elif isinstance(result, Pending) and self.has_to_increase_gas(self.pending):
                # gasprice updated by demand on new blocks
                self.pending.increase_gasprice(self.gasprice)

    def add_transaction(self, transaction: Transaction):
        transaction.preconditions()  # Raises if invalid, this will quit the program

        valid, reason = transaction.is_valid()

        if valid:
            heap.push(self.heap, transaction)

            if self.pending is None:
                self.pending = transaction
                transaction.send(self.blockhash, self.gasprice)
            elif higher_priority(transaction, self.pending):
                self.pending = transaction.replace(self.blockhash, self.pending)
        else:
            if reason is None:
                raise UnrecoverableError('unexpected error')
            log(reason)

    def maybe_send_next(self):
        while self.pending is None and self.heap:
            transaction = heap.peek(self.heap)
            valid, reason = self.pending.is_valid()

            if valid:
                self.pending = transaction
                transaction.send(self.blockhash, self.gasprice)
            else:
                if reason is None:
                    raise UnrecoverableError('unexpected error')
                heap.pop(self.heap)
                log(reason)

[hackaugusto]

related: #2194

[hackaugusto]

related Improve transaction error handling #2719 , this provides a model to implement is_valid methods.

[hackaugusto]

Two notes:

• TransactionManager.poll_transactions may not be the best idea. The rationale was to only mine for the transaction at points in which it could have executed or not, reducing the number of rpc calls. However, the transaction execution is not the only important thing done by the polling, we also need to check if the transaction is in the transaction pool.
• This is missing a way to return errors. Perhaps we should extend the token idea to also identify the transaction, and allow the caller to use this token to retrieve errors.

[LefterisJP]

From our discussion about Raiden planning on 28/01/2019 we concluded that we should push this for after the blockchain consistent view issue.

Reasoning: It can still be done after the consistent view with ease, it's not really a priority and implementing it may take a bit of time and as such should not be started right now.

[hackaugusto]

Note: with the work done on the consistent blockchain view we changed the definition of the preconditions method. Instead of:

    @abstractmethod
    def preconditions(self, blockhash: BlockHash):
        """ Raises if the required preconditions for the given transaction have
        never been met.

        Used for things like, interacting with an address which was never a
        smart contract, or trying to call a function before it's preconditions
        have been met.

        Note:
            This does not raise if the preconditions were met in the past,
            but got invalidated.

        raises:
            UnrecoverableError: If the required preconditions have never been
                met to successfully execute the transaction.
        """
        pass

This will now:

    @abstractmethod
    def preconditions(self, triggered_by_block_hash: BlockHash):
        """ Raises if the preconditions are not satisfied at the given
        `triggered_by_block_hash`.

        Used to check if there are any logic bugs in the business logic
        which resulted in an impossible ContractSend*.

        Note:
            This does not care if the preconditions got invalidated on the
            latest block.

        raises:
            UnrecoverableError: If the preconditions are not satisfied.
        """
        pass

The previous definition tried to handle logic errors and unnecessary transactions, while the new one only handles logic errors. Invalidated transactions are now handled exclusively by the estimate_gas call, and race conditions by the status of the transaction receipt.

Ref: #3505 (comment)

[hackaugusto]

While writing #4063 I realized that TransactionManager.maybe_send_next has to be a bit smarter. If there is a transaction in the queue which has an expiration and high priority, we not only have to consider the time it takes for that transaction to be mined (reveal_timeout), but also the time it takes for the in-flight transaction to be mined.

Example: If there are two transactions in the queue, open_and_deposit and register_secret. The first does not have an expiration while the later expires at block n, the current block is c. Consider the following cases:

1. c <= (n +1 -2*reveal_timeout), it's safe to send open_and_deposit, because it will be mined before register_secret has to be sent.
2. (n +1 -2*reveal_timeout) < c < (n -1 -reveal_timeout), In this case, it's not safe to send any non-prioritize transaction, so open_and_deposit can not be sent. If that transaction would be sent, it may take for both open_and_deposit and secret_reveal exactly reveal_timeout blocks to be mined, making the secret_register happen 1 block after its expiration.
3. c == n -1 -reveal_timeout This is the block at which reveal_secret has to be sent for safety.
4. c > n -1 -reveal_timeout This means we are inside the danger zone and the transaction may not be mined in time.

The above considers that reveal_timeout is the number of blocks required for a transaction to be mined under congestion, so sending both transactions at the same time would not fix case 2. I have not considered what would happen if the open_and_deposit was overwritten by secret_register

[hackaugusto]

These issues are related:

Specialize the block data type #4489
Allow parallel channel opening #4625
Handle race conditions on estimate gas #3890
Consider using meta transactions to overcome the gas price problem #4783
Batch secret registration in the client #4063
Synchronize transaction receipt polling #4932
Proxies for trusted SCs should only perform precondition checks if the gas estimation fails #5013

I thought a bit about this, and this is an interesting approach: https://gist.github.com/hackaugusto/088c31ba95c74355989f5a42b9b0a6e0

[hackaugusto]

The design described here should probably be revised and take advantage of the optimization from Proxies for trusted SCs should only perform precondition checks if the gas estimation fails #5013