Skip to content

Conversation

@matkt
Copy link
Contributor

@matkt matkt commented May 5, 2020

Signed-off-by: Karim TAAM karim.t2am@gmail.com

PR description

Fix cases where the gas estimate is too low and causes the transaction to fail

Implementation of a strategy that will estimate the number of gas needed by a transaction

1 - Estimate the number of gas used by a transaction
2 - Estimate the number of gas necessary to add for each sub call present in a transaction (65/64^depth)
3 - Add the minimum gas required for operations that need it (SSTORE) https://eips.ethereum.org/EIPS/eip-1706

This has been tested with the test network (dev) and the Clique network.

matkt added 4 commits May 5, 2020 14:14
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
…exceeds-gas-limit

Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
…-gas-limit

Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
@matkt matkt marked this pull request as ready for review May 5, 2020 13:46
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
@RatanRSur
Copy link
Contributor

RatanRSur commented May 5, 2020

Is the current version failing to give a high enough estimate in a case other than this?

See eth_call parameters, expect that all properties are optional. If no gas limit is specified geth uses the block gas limit from the pending block as an upper bound. As a result the returned estimate might not be enough to executed the call/transaction when the amount of gas is higher than the pending block gas limit.

https://github.com/ethereum/wiki/wiki/JSON-RPC#eth_estimategas

@matkt
Copy link
Contributor Author

matkt commented May 5, 2020

Is the current version failing to give a high enough estimate in a case other than this?

See eth_call parameters, expect that all properties are optional. If no gas limit is specified geth uses the block gas limit from the pending block as an upper bound. As a result the returned estimate might not be enough to executed the call/transaction when the amount of gas is higher than the pending block gas limit.

https://github.com/ethereum/wiki/wiki/JSON-RPC#eth_estimategas

My quick reading

Yes in some cases the estimate was not good despite a sufficient block gas limit. A good test is present and allows you to see the different behavior with Geth (https://github.com/cianx/besu-test). For example in the case of resetting the balance to 0. In the test presented above the block gas limit is "0x1fffffffffffff"

Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
matkt added 8 commits May 7, 2020 16:43
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
…s-exceeds-gas-limit

Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
…s-exceeds-gas-limit

Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
@matkt matkt force-pushed the feature/besu-696-fix-intrinsic-gas-exceeds-gas-limit branch from 0bd6c4d to 3c70ed0 Compare May 11, 2020 08:24
Copy link
Contributor

@AbdelStark AbdelStark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some changes need to be addressed.

Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
…besu-696-fix-intrinsic-gas-exceeds-gas-limit

Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
@matkt matkt merged commit 310e0b7 into hyperledger:master May 12, 2020
shemnon pushed a commit to shemnon/besu that referenced this pull request May 12, 2020
Fix cases where the gas estimate is too low and causes the transaction to fail

Implementation of a strategy that will estimate the number of gas needed by a transaction

1 - Estimate the number of gas used by a transaction
2 - Estimate the number of gas necessary to add for each sub call present in a transaction (65/64^depth)
3 - Add the minimum gas required for operations that need it (SSTORE) 

Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>
@matkt matkt deleted the feature/besu-696-fix-intrinsic-gas-exceeds-gas-limit branch May 14, 2020 09:04
shemnon pushed a commit to shemnon/besu that referenced this pull request May 23, 2020
Fix cases where the gas estimate is too low and causes the transaction to fail

Implementation of a strategy that will estimate the number of gas needed by a transaction

1 - Estimate the number of gas used by a transaction
2 - Estimate the number of gas necessary to add for each sub call present in a transaction (65/64^depth)
3 - Add the minimum gas required for operations that need it (SSTORE) 

Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants