-
Notifications
You must be signed in to change notification settings - Fork 89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
execute eth_estimateGas
and eth_createAccessList
on latest block
#3220
Conversation
eth_estimateGas
and eth_createAccessList
on latest block
Thanks Martin for PR and awesome detective work! |
This reverts commit b9185ed.
19889af
to
763eb47
Compare
@@ -182,7 +182,10 @@ impl Ethereum { | |||
.transport() | |||
.execute( | |||
"eth_createAccessList", | |||
vec![serde_json::to_value(&tx).unwrap()], | |||
vec![ | |||
serde_json::to_value(&tx).unwrap(), |
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.
Maybe instead of unwrap()
we can return an error (function returns Result
)?
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 would require a new error type and given that this unwrap never caused issues (all possible values are serializable) I prefer not to touch it.
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.
Did you test this somehow?
Btw, I thought it's known that simulating on block X on client means "on top of block X", while on tenderly it means "on top of X-1 block", so you actually always have to add 1 block to tenderly to achieve the same. So, I guess with this change we should always be 1:1 if we add that +1 to tenderly simulations.
Not so much. The e2e tests are still passing (including the forked ones) so all the nodes we care about support passing the
Overall the system we have is a bit racy. If the node becomes aware of a new block before out
Modulo race conditions with the block stream. I would love it if nodes simply returned the block they used when you do anything on the |
This would be enough for me. |
Ah true, forgot about it. We would have to implement ethereum/execution-apis#484 when ti lands. |
Description
For a while we had mysteriously failing transactions. What I mean is that we execute
eth_estimateGas
to see if a transaction would still work. If it fails we log all the data needed to re-simulate it on tenderly. But when you then actually re-simulate on tenderly the transaction would work.The solution unravels with this issue.
The spec for
eth_estimateGas
mentions 2 arguments: the tx and an identifier for a block. The block is supposed to be optional but geth had a bug for a very long time. When you don't pass a block argument it should be sent over the wire asnull
. But geth tried to deserializenull
which then caused these calls to fail.As a workaround the
web3
crate stopped serializing the block tag if it wasNone
. Eventually geth fixed this issue and implemented it such that a missing block tag would default topending
.Also a rule of thumb in node development is "if in doubt do whatever geth does" so reth also implemented the default to
pending
.Defaulting to
pending
is what can make these reverts unpredictable. When you simulate the tx onpending
the node takes the state of thelatest
block PLUS transactions that it currently has in its mempool. So whenever we log a reverting tx that works on tenderly it only failed in our node because some tx in the nodes mempool interfered with the solution.Interestingly there also a lot of cases where you can even simulate the tx on the next block. In those cases simulating on
pending
caused the driver completely erroneously since the problematic tx didn't even make it into the next block.example
log
Results in this simulation that even works 2 blocks past the block we logged.
Changes
Always estimate gas and create access lists on the
latest
block instead ofpending
.This should be the better default for 2 reasons:
pending
likely isn't accurate anyway.