Gas estimation#2623
Merged
Merged
Conversation
hackaugusto
force-pushed
the
gas_estimation
branch
5 times, most recently
from
2c6562f to
89267fa
Compare
hackaugusto
force-pushed
the
gas_estimation
branch
3 times, most recently
from
d0e3e45 to
bbebc20
Compare
palango
previously requested changes
Sep 28, 2018
Contributor
palango
left a comment
palango
left a comment
There was a problem hiding this comment.
Left some comments and questions
palango
reviewed
Sep 28, 2018
LefterisJP
reviewed
Sep 30, 2018
Merged
hackaugusto
force-pushed
the
gas_estimation
branch
2 times, most recently
from
5c85c79 to
f2f8c18
Compare
This was referenced Oct 29, 2018
Contributor
|
I'll rebase this now |
Integration tests are flaky because the current code base is using a percentage of the block gas limit as the transaction startgas (cached for 600 seconds). This is a problem when there are too few transactions and the gas limit decreases by more than 20%, making the transaction sent invalid and consectuvily the ethereum node rejects it with a 'exceeds block gas limit' message. For the version v0.4.0 of the smart contract these functions have a predictable gas profile and can use hard coded constats: - EndpointRegistry:registerEndpoint - TokenNetworkRegistry:createERC20TokenNetwork - TokenNetwork:closeChannel - TokenNetwork:updateNonClosingBalanceProof - SecretRegistry:registerSecret - SecretRegistry:registerSecretBatch For these functions we call external smart contracts that can have a variable gas usage, therefore must be estimated. - TokenNetwork:openChannel -> uses token.balanceOf - TokenNetwork:setTotalDeposit -> uses token.balanceOf and token.transferFrom - TokenNetwork:setTotalWithdraw -> uses token.transfer - TokenNetwork:settleChannel -> uses token.transfer - TokenNetwork:unlock -> uses token.transfer - TokenNetwork:cooperativeSettle -> uses token.transfer The proxies were updated to use a gas estimation when it makes sense, similar to what was introduced by 454671a. The estimate_and_transact function was broken by the byzantium release and fixed with the max gas limit by 1f18640, which was removed from the code base by f1d055c and implicitely changed the code to use the block gas limit.
palango
force-pushed
the
gas_estimation
branch
2 times, most recently
from
cf465e7 to
7f20a9b
Compare
palango
force-pushed
the
gas_estimation
branch
from
7f20a9b to
7d18985
Compare
konradkonrad
reviewed
Nov 6, 2018
konradkonrad
reviewed
Nov 6, 2018
konradkonrad
reviewed
Nov 6, 2018
konradkonrad
reviewed
Nov 6, 2018
konradkonrad
reviewed
Nov 6, 2018
konradkonrad
reviewed
Nov 6, 2018
konradkonrad
reviewed
Nov 6, 2018
konradkonrad
suggested changes
Nov 6, 2018
palango
force-pushed
the
gas_estimation
branch
from
9102848 to
27c9c97
Compare
konradkonrad
approved these changes
Nov 7, 2018
Contributor
konradkonrad
left a comment
konradkonrad
left a comment
There was a problem hiding this comment.
thanks, that looks good!
LefterisJP
added a commit
to LefterisJP/raiden
that referenced
this pull request
Nov 8, 2018
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.
LefterisJP
added a commit
to LefterisJP/raiden
that referenced
this pull request
Nov 9, 2018
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.
LefterisJP
added a commit
to LefterisJP/raiden
that referenced
this pull request
Nov 9, 2018
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.
hackaugusto
commented
Nov 9, 2018
| def safe_gas_limit(*estimates) -> int: | ||
| """ Calculates a safe gas limit for a number of estimates. | ||
|
|
||
| Even though it's not documented, it does happen that estimate_gas returns `None`. |
Contributor
Author
There was a problem hiding this comment.
This won't make sense anymore once #2988 is merged, perhaps we should remove it?
Contributor
Author
There was a problem hiding this comment.
I wonder if only returns None os failure scenarios.
LefterisJP
added a commit
to LefterisJP/raiden
that referenced
this pull request
Jan 8, 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.
LefterisJP
added a commit
to LefterisJP/raiden
that referenced
this pull request
Jan 14, 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.
LefterisJP
added a commit
to LefterisJP/raiden
that referenced
this pull request
Jan 16, 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.
LefterisJP
added a commit
to LefterisJP/raiden
that referenced
this pull request
Jan 16, 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.
LefterisJP
added a commit
to LefterisJP/raiden
that referenced
this pull request
Jan 22, 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.
LefterisJP
added a commit
to LefterisJP/raiden
that referenced
this pull request
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.
hackaugusto
pushed a commit
that referenced
this pull request
Jan 23, 2019
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 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.
hackaugusto
pushed a commit
to hackaugusto/raiden
that referenced
this pull request
Jan 25, 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.
hackaugusto
pushed a commit
to hackaugusto/raiden
that referenced
this pull request
Jan 25, 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.
hackaugusto
pushed a commit
to hackaugusto/raiden
that referenced
this pull request
Jan 25, 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.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Integration tests are flaky because the current code base is using a
percentage of the block gas limit as the transaction startgas. This is a
problem when there are too few transactions and the gas limit decreases
by more than 20%, making the transaction sent invalid and consectuvily
the ethereum node rejects it with a 'exceeds block gas limit' message.
For the version v0.4.0 of the smart contract these functions have a
predictable gas profile and can use hard coded constats:
For these functions we call external smart contracts that can have a
variable gas usage, therefore must be estimated.
token.transferFrom
The proxies were updated to use a gas estimation when it makes sense,
similar to what was introduced by 454671a.
The estimate_and_transact function was broken by the byzantium release
and fixed with the max gas limit by 1f18640,
which was removed from the code base by f1d055c
and implicitely changed the code to use the block gas limit.
Fixes #2921