-
Notifications
You must be signed in to change notification settings - Fork 680
Conversation
Hey, just found about your work on rewriting gas estimation via https://medium.com/truffle-suite/ethereum-gas-exactimation-1158a996eb8c This is interesting as I was investigating the same recently as I thought every client behaved like ganache, in that they were simply executing the call once with the gas specified and returned the gasUsed minus the gas refund. For that reason I was proposing a new estimation algorithm that paired with a new opcode could provide accurate estimation without binary search. see ethereum/EIPs#2075 I cancelled the Pull request since its motivation and rationale were wrong: current mainnet client are actually estimating the gas required correctly. But the solution presented would reduce by 20 times the amount of computation required for an accurate estimation (since it would not need a binary search). I was planning to create a new PR with that sole objective in mind. Now, I am intrigued by your solution. while it would deal with the extra gas required for success (due to 1/64 rules), it would still require a binary search to deal with smart contract that have code like
What my new proposal proposes is a new opcode (like explained in ethereum/EIPs#2075 But since we would need backward compatibility, we would still require binary search for contract deployed before that. In other words, the optimization could only affect new contract and, new contract that do not use the new requireGas opcode would have their gas estimation broken. I would love to get this kind of optimisation implemented, so let me know if you need more info. I would be happy to collaborate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a photo of me reviewing this PR. I hope you like it.
\\\\\\\
\\\\\\\\\\\\
\\\\\\\\\\\\\\\
-----------,-| |C> // )\\\\|
,','| / || ,'/////|
---------,',' | (, || /////
|| | \\ ||||//''''|
|| | ||||||| _|
|| |______ `````\____/ \
|| | ,| _/_____/ \
|| ,' ,' | / |
||,' ,' | | \ |
_________|/ ,' | / | |
_____________,' ,',_____| | | |
| ,',' | | | |
| ,',' ____|_____/ / |
| ,',' __/ | / |
_____________|',' ///_/-------------/ |
|===========,'
Also, changes requested. :joy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we should invert the steps.success()
return value?
lib/utils/gasEstimation.js
Outdated
@@ -185,7 +170,7 @@ const stepTracker = () => { | |||
allOps.push(info); | |||
}, | |||
isSVT: (index) => svt.includes(index), | |||
success: () => !allOps.length || isTerminator(allOps[allOps.length - 1].opcode.name), | |||
success: () => !allOps.length || !isTerminator(allOps[allOps.length - 1].opcode.name), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels backwards to me. I feel like the result should be negated here (as well as the steps.success
check above)
@wighawag Thanks for reaching out and sorry for the late reply! We definitely appreciate your interest and are always open to contributions and collaborations in the pursuit of free and open source software! You're right that this can still fail when the code branches on a condition like With regard to next steps, we will be modifying the algorithm slightly to listen for the Let us know what you think and please feel free to improve this implementation! Also, @davidmurdoch and I will be co-presenting a workshop at TruffleCon 2019 on this algorithm, gas estimation in general, and techniques for gas optimization. If you're interested, it will be one of the best places to connect and collaborate with other like minded devs! https://www.trufflesuite.com/trufflecon2019 |
Hi @nicholasjpaterno thanks for the reply.
This is not true, with meta transaction or any other call that need to ensure the gas available is sufficient before making a call, the gasleft check is required. See https://github.com/gnosis/safe-contracts/blob/5a8b07326faf30c644361c9d690d57dbeb838698/contracts/GnosisSafe.sol#L101 It is important that the estimation algorithm supports such use cases.
That would work but it would be better if we could add a specific gascheck opcode contracts could use so so that even in these cases (contract that use gas checks) the optimization can run. See my comments above. Anyway it is good to see you are working in that direction. I won't be able to attend truffle conf unfortunately. |
* WIP, testing code * WIP testing code * WIP new algorithm * WIP disregard * WIP fix logic, not final * WIP fix summation algo, not final * WIP improve gas accounting * WIP not stable * WIP - fixed base summation bug * WIP Tests running; SVT -> WIP * Rename gasLimit * WIP; composite context fix * Remove testing code from blockchain_double * Fix deployment costs, recognize EIP150 opcodes * SVT May not be necessary anymore * Cleanup gas estimation, remove old code * FIX SVT - STABLE * Minor cleanup * Finalize gas estimation Cleanup all unneccessary code. * Fix SVT amount withheld * Increase recursive DELEGATECALL depth * Fix SVT * Fix bug in SVT calculation * Cleanup gasEstimation * Fix gas estimation, small refactor
@wighawag if the gasLeft checks whether the call has more gas than it needs to run, this will fail. Remember, the initial estimate allows the transaction to use all the gas available in the block, which would enable us to investigate beyond any I agree though, an explicit operation to check for gas would make things easy, as I could just investigate the state at that time, however for backwards compatibility it would never obviate the need for binary searching in these cases. I originally attempted to perform some static analysis with the |
Yes it will but this case is definitely a case of "tricking gas estimation algorithms" as you would put it. After all the binary search will also fails there.
I know that, but such initial estimate will under estimate for contract which have code like
As I mentioned for contract that use such This means that such contracts will get the optimization while other will fall back on the binary search, preserving backward compatibility. |
@wighawag it could actually bit a bit better than the worse case you've described. We can check for the In any case, I would like to keep this extra level of estimation certainty on our radar. Would you mind creating a separate issue for it so if people start running into gas estimation issues related to |
@wighawag Definitely agree!
I agree though that a simple safety mechanism providing a lower bound for calls is an important use case. With that said, it would be relatively easy to calculate the correct gas cost whenever its in a form like
True! Although, I think this would be the best case. And, with the optimization above I think I can correctly estimate in most cases. Worst case of course is having to binary search. |
Yes sorry, meant to say best case. And as @davidmurdoch mentioned with your gas opcode check the best case could be one call (assuming the smart contract is not using the gas opcode). But for the use case I had in mind (meta transaction), the gas opcode would be used to compute the relayer's reward, even if the check of gas could use the hypothetical new opcode
If it is in that particular form, yes but not if it was something like require(gasComputedEarlier > X) it would be more complicated. And if the algorithm is not full proof, it can't be used as it would break backward compatibility to assume it works in all case.
I can put forward a failing test case for ganache as I encountered the issue while I was testing some contract behavior. Just need to extract them out. |
Block limit agnostic EIP150 gas estimation algorithm.