search

0xPolygonHermez / zkevm-node

Go implementation of a node that operates the Polygon zkEVM Network
Other
531 stars 685 forks source link

opt estimate gas #3607

Closed chengzhinei closed 4 months ago

chengzhinei commented 5 months ago

Closes #.

What does this PR do?

We found that this interface will cost a long time(8 second) to estimate gas some time, and I found that the logic is diffrerent with go-ethereum, zkevm is missing this part and this may cause more execution times in the following loop. Each additional execution takes more time.

Benchmark result: Before this change an estimate use 9 executions and took 5.06 seconds. After adding this pr it takes 3 executions and takes 1.74 seconds up1LEnWD74

Reviewers

Main reviewers:

Codeowner reviewers:

cla-bot[bot] commented 5 months ago

We require contributors/corporates @KamiD, @chengzhinei to read our Contributor License Agreement, please check the Individual CLA document/Corporate CLA document

cla-bot[bot] commented 5 months ago

We require contributors/corporates @KamiD, @chengzhinei to read our Contributor License Agreement, please check the Individual CLA document/Corporate CLA document

cla-bot[bot] commented 4 months ago

We require contributors/corporates @KamiD, @chengzhinei to read our Contributor License Agreement, please check the Individual CLA document/Corporate CLA document

cla-bot[bot] commented 4 months ago

We require contributors/corporates @KamiD, @chengzhinei to read our Contributor License Agreement, please check the Individual CLA document/Corporate CLA document

cla-bot[bot] commented 4 months ago

We require contributors/corporates @KamiD, @chengzhinei to read our Contributor License Agreement, please check the Individual CLA document/Corporate CLA document

cla-bot[bot] commented 4 months ago

We require contributors/corporates @KamiD, @chengzhinei to read our Contributor License Agreement, please check the Individual CLA document/Corporate CLA document

cla-bot[bot] commented 4 months ago

We require contributors/corporates @KamiD, @chengzhinei to read our Contributor License Agreement, please check the Individual CLA document/Corporate CLA document

tclemos commented 4 months ago

Hey @chengzhinei, thanks for the contribution. Looks like one of the tests is failing due to the lack of the gasRefunded in the formula. Also, linter is complaining about the constant magical numbers.

Would you mind reviewing these points? Let me know if you need help.

chengzhinei commented 4 months ago

Hey @chengzhinei, thanks for the contribution. Looks like one of the tests is failing due to the lack of the gasRefunded in the formula. Also, linter is complaining about the constant magical numbers.

Would you mind reviewing these points? Let me know if you need help.

Thanks for your reply! Let me see and handle it~ Hope to solve it early and I'll keep you informed, thanks!

cla-bot[bot] commented 4 months ago

We require contributors/corporates @KamiD, @chengzhinei to read our Contributor License Agreement, please check the Individual CLA document/Corporate CLA document

chengzhinei commented 4 months ago

Hey @chengzhinei, thanks for the contribution. Looks like one of the tests is failing due to the lack of the gasRefunded in the formula. Also, linter is complaining about the constant magical numbers.

Would you mind reviewing these points? Let me know if you need help.

Hey @tclemos , I have fixed the lint check. Besides, the gasRefund has been added, but it seems that the test still can't work well. Now only the test "estimate_gas_OOC_poseidon" has the problem: I have confirmed that this error is happend when the gas limit was modified to be optimisticGasLimit, and it revert at the first time to execute the tx. It seems the value "(gasUsed + gasRefund + 2300)*64/63" is still can't afford the fee, if it is possible the execution response of gasUsed and gasRefund is not very accurate？ image

tclemos commented 4 months ago

I realized something similar while investigating the failing tests. I'll examine it more closely and see if @krlosMata can help me understand what's happening in this case.

chengzhinei commented 4 months ago

I realized something similar while investigating the failing tests. I'll examine it more closely and see if @krlosMata can help me understand what's happening in this case.

I realized that the exec response has a value "GasLeft", maybe this value should be added too?

tclemos commented 4 months ago

@chengzhinei I've found the issue. Estimating the gas of a TX that would return an out-of-counter error was breaking the estimation because the gasUsed and gasRefunded were returned as 0, and the result was not considered Failed. This caused the optimistic gas formula to compute a shallow gas amount, which caused the error reported by the test.

While fixing it, I took advantage of refactoring and replacing the multiple return values with a struct, but in order to do that, I had to create my own PR, so I've migrated your changes into my PR too.

I think we can close this PR and follow with this one: https://github.com/0xPolygonHermez/zkevm-node/issues/3652

wdty?

chengzhinei commented 4 months ago

@chengzhinei I've found the issue. Estimating the gas of a TX that would return an out-of-counter error was breaking the estimation because the gasUsed and gasRefunded were returned as 0, and the result was not considered Failed. This caused the optimistic gas formula to compute a shallow gas amount, which caused the error reported by the test.

While fixing it, I took advantage of refactoring and replacing the multiple return values with a struct, but in order to do that, I had to create my own PR, so I've migrated your changes into my PR too.

I think we can close this PR and follow with this one: #3652

wdty?

That's great! This PR can be closed then, and thanks again!

tclemos commented 4 months ago

replaced by this PR: https://github.com/0xPolygonHermez/zkevm-node/pull/3653