fix(evmstate/test): stabilize trace tx tests with deterministic ERC20 transfer recipient #2418
Conversation
… transfer recipient
📝 WalkthroughWalkthroughFixed non-deterministic gas values in ERC20 transfer trace tests by introducing deterministic recipient logic in test utilities and updating corresponding expected gas values in test assertions. Updated changelog and release notes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧬 Code graph analysis (1)x/evm/evmtest/tx.go (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
🔇 Additional comments (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the non-determinism in TestTraceTx by using a fixed recipient address for ERC-20 transfers. The explanation provided in the code comments and PR description is excellent and clearly outlines the problem and the solution. The changes are well-implemented and correctly adjust the expected gas values in the tests. I have one minor suggestion to improve code maintainability by replacing a magic number with a named constant.
| var ( | ||
| // Use a deterministic recipient to avoid nondeterministic intrinsic gas from calldata | ||
| // (non-zero vs zero byte costs). A fixed address ensures stable gas/gasUsed in traces. | ||
| // | ||
| // This address, "0x0000000000000000000000000000000000010f2C", has only 3 | ||
| // non-zero bytes in its 20-byte payload (01, 0f, 2c). This means we expect | ||
| // it to have lower intrinsic gas than a "typical" random address that has | ||
| // ~20 non-zero bytes. Lower by (20 - 3) * 12 = 204 gas units, based on | ||
| // EIP-2028 (Istanbul upgrade). | ||
| // | ||
| // Istanbul reduced the calldata charge to 4 gas | ||
| // per zero byte and 16 per non-zero byte, which is why changing the address | ||
| // bytes moves intrinsic gas in steps of 12. | ||
| fixedRecipient = gethcommon.BigToAddress(big.NewInt(69_420)) | ||
| ) |
There was a problem hiding this comment.
To improve readability and maintainability, consider defining 69_420 as a named constant. This avoids using a magic number directly in the function body. The var block for a single variable is also slightly unconventional. This suggestion refactors the block to use a const.
| var ( | |
| // Use a deterministic recipient to avoid nondeterministic intrinsic gas from calldata | |
| // (non-zero vs zero byte costs). A fixed address ensures stable gas/gasUsed in traces. | |
| // | |
| // This address, "0x0000000000000000000000000000000000010f2C", has only 3 | |
| // non-zero bytes in its 20-byte payload (01, 0f, 2c). This means we expect | |
| // it to have lower intrinsic gas than a "typical" random address that has | |
| // ~20 non-zero bytes. Lower by (20 - 3) * 12 = 204 gas units, based on | |
| // EIP-2028 (Istanbul upgrade). | |
| // | |
| // Istanbul reduced the calldata charge to 4 gas | |
| // per zero byte and 16 per non-zero byte, which is why changing the address | |
| // bytes moves intrinsic gas in steps of 12. | |
| fixedRecipient = gethcommon.BigToAddress(big.NewInt(69_420)) | |
| ) | |
| const fixedRecipientInt = 69_420 | |
| // Use a deterministic recipient to avoid nondeterministic intrinsic gas from calldata | |
| // (non-zero vs zero byte costs). A fixed address ensures stable gas/gasUsed in traces. | |
| // | |
| // This address, "0x0000000000000000000000000000000000010f2C", has only 3 | |
| // non-zero bytes in its 20-byte payload (01, 0f, 2c). This means we expect | |
| // it to have lower intrinsic gas than a "typical" random address that has | |
| // ~20 non-zero bytes. Lower by (20 - 3) * 12 = 204 gas units, based on | |
| // EIP-2028 (Istanbul upgrade). | |
| // | |
| // Istanbul reduced the calldata charge to 4 gas | |
| // per zero byte and 16 per non-zero byte, which is why changing the address | |
| // bytes moves intrinsic gas in steps of 12. | |
| fixedRecipient := gethcommon.BigToAddress(big.NewInt(fixedRecipientInt)) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2418 +/- ##
=======================================
Coverage 59.23% 59.24%
=======================================
Files 359 359
Lines 24190 24193 +3
=======================================
+ Hits 14329 14332 +3
Misses 8635 8635
Partials 1226 1226
🚀 New features to boost your workflow:
|
Stabilize TestTraceTx by fixing ERC-20 transfer recipient to deterministic address
Problem and root cause
TestTraceTxfailed intermittently ongasandgasUsedassertions for the ERC-20 transfer case, with variations in steps of 12 gas.Intrinsic gas charges depend on calldata byte values. Since the Istanbul upgrade (EIP-2028), each zero byte costs 4 gas and each non-zero byte costs 16 gas.
When the recipient address was randomly generated, the number of non-zero bytes in the 20-byte address changed between runs, altering intrinsic gas in increments of 12 and producing nondeterministic results.
Fix
Use a fixed recipient address (
0x0000000000000000000000000000000000010f2C, created withgethcommon.BigToAddress(big.NewInt(69_420))) when building the ERC-20 transfer calldata.This ensures the non-zero/zero byte pattern in calldata remains constant, stabilizing intrinsic gas and making
gasandgasUsedconsistent across runs.Why this works
Changes
DeployAndExecuteERC20Transferto use a fixed recipient address.gasandgasUsedin test cases to reflect the deterministic intrinsic gas.Verification
gas.References