op-e2e: Fix bug in TestWithdrawals #5931
Conversation
|
✅ Deploy Preview for opstack-docs canceled.
|
|
hmm, How is the difference in SuggestedGasTip between erigon/geth showing up here? |
|
Maybe the Erigon comment was more confusing than it was helpful. To be clear, the bug is that the fee computation is using the gas usage from L1, with the gas pricing from an L2 tx. That is the bug this PR fixes. The test currently passes because it is using a Geth for both L1 and L2 nodes, which returns the same SuggestedGasTip (1 Gwei) when there is no tip history to infer from. Because the SuggestedGasTip is the same for both L1 and L2, the actual GasTip for the L2 transaction ends up being the same as the GasTip for the L1 transaction. Therefore the test passes even though we're mixing up the L1 and L2 GasTips. When using Erigon for the L2 nodes, the SuggestedGasTip returns a different suggested gas tip (0 Gwei). So, now the GasTip on the L1 txes (1 Gwei) is not the same as the GasTip on the L2 tx (0 Gwei), so the test fails. Erigon is not breaking anything, it's just the test's assertion that has the bug. Personally, I'm not entirely sure why |
The existing TestWithdrawals test attempts to assert that the total fees of the prove and finalize transactions are accounted for in the L1 balance. But, it re-uses the `calcGasFees` function and passes in the `GasTipCap` and the `GasFeeCap` from the L2 transaction, not from the actual L1 transactions. In the event that the gas fee differs on the L2 from the L1, then the test will fail. To keep the diff small, this change simply multiplies the GasUsed and `EffectiveGasPrice` from the receipts. This could be moved into a helper function like `calcGasFees`. Using the `EffectiveGasPrice` seems just as good or better than relying on the parameters of the original transaction, and could be similarly utilized higher in the test for the L2 fees. If we really want to get the parameters from the tx, then `ProveAndFinalizeWithdrawal` would need to be modified to return the txes.
jyellick
force-pushed
the
fix-withdrawals-test
branch
from
d048c7d
to
769fe00
Compare
There was a problem hiding this comment.
Funnily enough, hit this with the latest op-geth as well because it changes the fee estimation for OP networks so L1 and L2 wind up returning different GasTips (#5938).
The test is confirming that the L1 contract transfers the correct amount of ETH so I agree its reasonable to trust the EffectiveGasPrice from the receipt rather than having to calculate from the transaction.
Thanks.
|
This PR has been added to the merge queue, and will be merged soon. |
|
Hey @jyellick, this pull request failed to merge and has been dequeued from the merge train. If you believe your PR failed in the merge train because of a flaky test, requeue it by commenting with |
The existing TestWithdrawals test attempts to assert that the total fees of the prove and finalize transactions are accounted for in the L1 balance. But, it re-uses the
calcGasFeesfunction and passes in theGasTipCapand theGasFeeCapfrom the L2 transaction, not from the actual L1 transactions. In the event that the gas fee differs on the L2 from the L1, then the test will fail.To keep the diff small, this change simply multiplies the GasUsed and
EffectiveGasPricefrom the receipts. This could be moved into a helper function likecalcGasFees. Using theEffectiveGasPriceseems just as good or better than relying on the parameters of the original transaction, and could be similarly utilized higher in the test for the L2 fees. If we really want to get the parameters from the tx, thenProveAndFinalizeWithdrawalwould need to be modified to return the txes.FWIW, I encountered this when using Erigon for the L2 nodes, which compute the SuggestedGasTip slightly differently from Geth resulting in a discrepancy between the L1 and L2 gas tips, but this could conceivably cause flakiness even when using Geth.