Try to handle estimateGas predicting that a call will fail #2988
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
LefterisJP
force-pushed
the
when_estimate_gas_returns_none_call_will_fail
branch
from
096afe7
to
05fc391
Compare
palango
reviewed
Nov 9, 2018
palango
reviewed
Nov 9, 2018
LefterisJP
force-pushed
the
when_estimate_gas_returns_none_call_will_fail
branch
3 times, most recently
from
df0d533
to
8776c8f
Compare
palango
previously approved these changes
Nov 9, 2018
hackaugusto
reviewed
Nov 9, 2018
Merged
|
Quick note: I added a comment in another PR which is relate to this one: #2623 (review) |
palango
dismissed
their stale review
we need to figure out the waiting thingy
hackaugusto
reviewed
Jan 4, 2019
LefterisJP
force-pushed
the
when_estimate_gas_returns_none_call_will_fail
branch
from
8776c8f
to
d400484
Compare
|
Rebased on latest master, fixed conflicts. Now going to look at the PR comments and test failures. |
hackaugusto
reviewed
Jan 10, 2019
LefterisJP
force-pushed
the
when_estimate_gas_returns_none_call_will_fail
branch
3 times, most recently
from
493b847
to
13007e9
Compare
LefterisJP
added a commit
to LefterisJP/raiden
that referenced
this pull request
Jan 16, 2019
As discussed in Github [here](raiden-network#2988 (comment)) we are now using the following order for gas estimation and call() in the smart contract proxy transactions: ```python 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("Bad error -- we don't know why") ```
|
This PR is first steps towards a model such as: #3268 |
LefterisJP
force-pushed
the
when_estimate_gas_returns_none_call_will_fail
branch
from
13007e9
to
fc25fa5
Compare
LefterisJP
added a commit
to LefterisJP/raiden
that referenced
this pull request
Jan 16, 2019
As discussed in Github [here](raiden-network#2988 (comment)) we are now using the following order for gas estimation and call() in the smart contract proxy transactions: ```python 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("Bad error -- we don't know why") ```
LefterisJP
force-pushed
the
when_estimate_gas_returns_none_call_will_fail
branch
2 times, most recently
from
86c595d
to
59bf2bd
Compare
|
Note that with this approach of calling This is no longer the case. We can't detect this anymore and we now just return an As an alternative to allow detecting this we could still do something like below for each proxy transaction where estimateGas fails: gas_limit = estimate_gas()
...
if not gas_limit:
if balanceOf(self.node.address) < GAS_REQUIRED_FOR_TX * gas_price:
raise InsufficientFunds()I can't see any problem with having the above added so I will probably add it in the morning. |
LefterisJP
added a commit
to LefterisJP/raiden
that referenced
this pull request
Jan 22, 2019
As discussed in Github [here](raiden-network#2988 (comment)) we are now using the following order for gas estimation and call() in the smart contract proxy transactions: ```python 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("Bad error -- we don't know why") ```
Agreed, but the purpose of this PR is not to increase readability but optimize in the case of estimateGas failing so that we don't change the transaction. I hope to continue with #3268 which should help with code readability and organization for the proxies. |
LefterisJP
force-pushed
the
when_estimate_gas_returns_none_call_will_fail
branch
from
e88c5ef
to
5e255b0
Compare
Codecov Report
@@ Coverage Diff @@
## master #2988 +/- ##
=========================================
Coverage ? 74.75%
=========================================
Files ? 94
Lines ? 12338
Branches ? 1729
=========================================
Hits ? 9223
Misses ? 2474
Partials ? 641
Continue to review full report at Codecov.
|
hackaugusto
approved these changes
Jan 23, 2019
After the changes introduced by raiden-network#2623 we again rely on gas estimation for the gas limit to provide to an onchain call which is great since it reduces our overall ETH balance requirements. But we don't take into account the fact that if estimateGas returns None it's because the transaction will fail, either due to our account not having enough funds or because the call will just revert somewhere down the line due to the contract logic (e.g. calling settle before closing). With that in mind we can now detect those bad cases a bit earlier and handle them without sending an onchain transaction. This is what this PR is trying to achieve.
- Better logging messages - kwargs - Swap mistaken errors messages on why approve call failed
This is temporary and should be reverted in favour of the web3.py implementation as soon as they include it in a release.
As discussed in Github [here](raiden-network#2988 (comment)) we are now using the following order for gas estimation and call() in the smart contract proxy transactions: ```python 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("Bad error -- we don't know why") ```
If estimate gas fails, then introduce an extra check which will determine if the failure came due to our address not having sufficient ETH to pay for gas
LefterisJP
force-pushed
the
when_estimate_gas_returns_none_call_will_fail
branch
from
73a6c99
to
9eaa6c3
Compare
|
Rebased on master to avoid the secret revealed flakiness. |
hackaugusto
pushed a commit
that referenced
this pull request
Jan 23, 2019
As discussed in Github [here](#2988 (comment)) we are now using the following order for gas estimation and call() in the smart contract proxy transactions: ```python 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("Bad error -- we don't know why") ```
hackaugusto
pushed a commit
to hackaugusto/raiden
that referenced
this pull request
Jan 25, 2019
As discussed in Github [here](raiden-network#2988 (comment)) we are now using the following order for gas estimation and call() in the smart contract proxy transactions: ```python 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("Bad error -- we don't know why") ```
hackaugusto
pushed a commit
to hackaugusto/raiden
that referenced
this pull request
Jan 25, 2019
As discussed in Github [here](raiden-network#2988 (comment)) we are now using the following order for gas estimation and call() in the smart contract proxy transactions: ```python 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("Bad error -- we don't know why") ```
hackaugusto
pushed a commit
to hackaugusto/raiden
that referenced
this pull request
Jan 25, 2019
As discussed in Github [here](raiden-network#2988 (comment)) we are now using the following order for gas estimation and call() in the smart contract proxy transactions: ```python 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("Bad error -- we don't know why") ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After the changes introduced by #2623 we again rely on gas estimation for the gas limit to provide to an onchain call which is great since it reduces our overall ETH balance requirements.
But we don't take into account the fact that if estimateGas returns
Noneit'sbecause the transaction will fail, either due to our account not having enough
funds or because the call will just revert somewhere down the line due to the
contract logic (e.g. calling settle before closing).
With that in mind we can now detect those bad cases a bit earlier and handle
them without sending an onchain transaction. This is what this PR is trying to achieve.
Admittedly all those cases are bad ones. Since all the checks should have stopped us from getting into that part of the code in the first place, unless we expected that sending an onchain transaction will succeed.