fix(eth): allow eth_call and eth_estimateGas from contract and non-existent addresses#13470
fix(eth): allow eth_call and eth_estimateGas from contract and non-existent addresses#13470nijoe1 wants to merge 15 commits intofilecoin-project:masterfrom
Conversation
…ailures This error type enables callers to distinguish sender validation failures from other errors and retry with skip-sender-validation when appropriate.
… simulation Add new VM interface method that skips sender validation checks, enabling simulation from contract addresses or non-existent addresses. This matches Geth's behavior for eth_call and eth_estimateGas. For LegacyVM, this bypasses actor existence, account type, nonce, and balance checks. For FVM, validation is handled at the state manager level.
…dress Implement skip sender validation for state manager calls: - CallWithGasSkipSenderValidation: Skip sender validation during gas estimation - ApplyOnStateWithGasSkipSenderValidation: Skip validation for eth_call - createSyntheticSenderActor: Create ephemeral EthAccount for non-existent senders - maybeModifySenderForSimulation: Modify EVM/Placeholder actors to EthAccount State modifications are isolated via buffered blockstore and discarded after simulation. This enables eth_call/eth_estimateGas from contracts or non-existent addresses, matching Geth's behavior.
…stimateGas Wire up skip sender validation throughout the gas estimation and eth_call paths: - EthEstimateGas falls back to skip-sender when sender validation fails - EthCall uses ApplyOnStateWithGasSkipSenderValidation for all calls - Add metrics: eth_call_count, eth_estimate_gas_count, eth_estimate_gas_skip_sender Error handling improved to preserve ErrExecutionReverted for JSON-RPC codec and distinguish sender validation errors from other failures.
Add integration tests covering all skip-sender-validation code paths: - TestEthCallSkipSender: 10 subtests (from contract, non-existent, EOA, nil, value without balance, and 5 revert types) - TestEthEstimateGasSkipSender: 8 subtests (from contract, non-existent, EOA, and 5 revert types with gas bounds validation) - TestSkipSenderStateIsolation: Verify synthetic actors don't persist Tests use dynamic selector computation and table-driven design.
…n failure The previous implementation always used ApplyOnStateWithGasSkipSenderValidation for EthCall, which broke the eth_conformance test because it affected the state handling for normal EOA accounts. This fix changes EthCall to: 1. First try the normal ApplyOnStateWithGas path (works for valid EOAs) 2. Fall back to ApplyOnStateWithGasSkipSenderValidation only when: - The normal path returns ErrSenderValidationFailed - The normal path returns SysErrSenderInvalid exit code (contracts) This maintains Geth compatibility for simulating from contracts/non-existent addresses while preserving correct behavior for normal accounts.
* Refactor eth_call and eth_estimateGas for better accuracy and code reuse - Unify gas estimation logic in gasutils to reduce duplication - Update eth_estimateGas fallback path to use ethGasSearch for improved accuracy - Ensure consistent handling of ErrSenderValidationFailed across API layers * Further refine VM simulation and gas estimation accuracy - Skip gas holder transfers and nonce increments in VM during simulations - Use ethGasSearch in skip-sender fallback for consistent estimation accuracy - Improve state isolation for synthetic actor simulations * Simplify eth_call refinements and revert invasive VM changes - Revert VM-level skip logic to maintain standard execution path - Implement 'simulation balance' in StateManager for synthetic actors - Simplify eth_estimateGas fallback to reduce complexity - Retain gasutils refactoring for better code reuse * Add regression tests for simulation balance and gas price scenarios - Verify eth_call from non-existent address succeeds with non-zero GasPrice - Verify eth_call succeeds with value transfers within simulation balance (100 FIL) - Verify eth_call still fails for value transfers exceeding simulation balance * Restore FromNonExistent test case and keep gas price regression cases * fix(eth): keep synthetic sender balance at zero * test(eth): add estimateGas non-existent sender gasPrice case * fix(eth): reject estimateGas with gas price on missing sender * revert(eth): keep estimateGas skip-sender fallback minimal * test(eth): expand skip-sender estimateGas/call coverage * test(eth): allow gasPrice eth_call for missing senders * fix:improves eth_call with value tests --------- Co-authored-by: nijoe1 <nick@fil.builders>
There was a problem hiding this comment.
Pull request overview
This pull request implements skip-sender-validation functionality for eth_call and eth_estimateGas to match Geth's behavior, enabling these calls to simulate transactions from contract addresses or non-existent addresses. This is essential for Ethereum development tools like Hardhat and Foundry.
Changes:
- Added skip-sender-validation paths in VM layer and state manager to bypass actor existence and account type checks
- Modified
eth_callandeth_estimateGasto fallback to skip-sender-validation when normal validation fails - Added telemetry metrics to track skip-sender-validation usage
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| api/api_errors.go | Added ErrSenderValidationFailed error type for distinguishing sender validation failures |
| chain/vm/vmi.go | Added ApplyMessageSkipSenderValidation method to VM interface |
| chain/vm/vm.go | Implemented skip-sender-validation in LegacyVM with synthetic actor creation |
| chain/vm/fvm.go | FVM delegates skip-sender-validation to state manager level |
| chain/vm/execution.go | Added executor wrapper for skip-sender-validation method |
| chain/stmgr/call.go | Implemented CallWithGasSkipSenderValidation and ApplyOnStateWithGasSkipSenderValidation with proper state tree actor creation |
| node/impl/gasutils/gasutils.go | Refactored gas estimation with unified internal implementation supporting skip-sender-validation |
| node/impl/eth/api.go | Added interface methods for skip-sender-validation state manager methods |
| node/impl/eth/gas.go | Modified EthEstimateGas and EthCall to fallback to skip-sender-validation on sender validation failures |
| metrics/metrics.go | Added metrics for eth_call and eth_estimateGas operations |
| itests/eth_skip_sender_test.go | Comprehensive tests for skip-sender-validation behavior with various sender types and error conditions |
Comments suppressed due to low confidence (1)
chain/vm/vm.go:590
- In the skipSenderValidation path, when a synthetic actor is created with zero balance (lines 510-513), the code still attempts to transfer gas cost from this actor at line 588. The transferToGasHolder method will fail with "not enough funds" error because it tries to deduct the gas cost from an actor with zero balance. This will cause all eth_call and eth_estimateGas operations from non-existent addresses to fail, defeating the purpose of this feature. The gas transfer should be skipped or handled differently when skipSenderValidation is true.
gasHolder := &types.Actor{Balance: types.NewInt(0)}
if err := vm.transferToGasHolder(msg.From, gasHolder, gascost); err != nil {
return nil, xerrors.Errorf("failed to withdraw gas funds: %w", err)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| gascost = types.BigMul(types.NewInt(uint64(msg.GasLimit)), msg.GasFeeCap) |
There was a problem hiding this comment.
The synthetic actor is created with zero balance, but gascost is calculated as msg.GasLimit * msg.GasFeeCap. When the subsequent code attempts to transfer this gas cost from the synthetic actor (which happens at line 588, outside this diff), it will fail because the actor has insufficient funds. Consider setting msg.GasFeeCap to zero when skipSenderValidation is true, or handle the gas transfer differently.
| } | |
| gascost = types.BigMul(types.NewInt(uint64(msg.GasLimit)), msg.GasFeeCap) | |
| } | |
| // For simulations, do not pre-charge gas from the synthetic zero-balance actor. | |
| // This avoids insufficient-funds failures when gas is later debited. | |
| gascost = types.NewInt(0) |
| func setupSkipSenderTest(t *testing.T) *skipSenderEnv { | ||
| ctx, cancel, client := kit.SetupFEVMTest(t) |
There was a problem hiding this comment.
The test uses SetupFEVMTest which only tests the FVM path (network version >= 16). The LegacyVM path (older network versions) is not tested, but the skip-sender-validation feature is implemented for both VM types. Consider adding tests that verify the behavior on older network versions using LegacyVM to ensure the feature works correctly in both paths.
|
Really quick skim-review to start with, will try and make time to go deeper. Overall this looks pretty good @nijoe1, great work!
|
This case is handled here where it modifies in memory the Contract.code to look like an EthAccount.code To check out if the tests are not sufficient in real world scenarios I cloned today the foc-devnet and connected my local lotus repo pointing to this PR code changes. Then using foundry I implemented the same set of tests as the ones defined here and all passed using the local lotus rpc on devnet where on calibration failed |
|
OK I think |
That's true. I added locally one test where And that fails when doing eth_call because we change contractA code Which is the best approach to test local changes made on Seems that I found an ugly workaround to do that where skip-sender-validation tests pass while implementing your above improvements but many other tests fail. That's why I am asking if there is a standard pattern to connect lotus with |
|
Sorry this is where it gets complicated. I was going to point you to https://github.com/filecoin-project/lotus/blob/master/documentation/misc/Builtin-actors_Development.md but there's a TODO at the bottom to document the things we need here. You need to clone ref-fvm, filecoin-ffi and do some messing around with Cargo.toml in filecoin-ffi to point to your local copy, or a branch on GitHub that you've modified, then you need to rebuild the CGO bindings in filecoin-ffi to make updates if you're changing the API (I'm assuming you're going to pass a flag down into ref-fvm that tells it to skip validation checks), then you can either symlink ffi into lotus or update the submodule to point to your branch. Then you'll need PRs against all 3 and we'll have to verify they work and review and merge them up the stack and back to here. It's a little messy but AI should be your friend in figuring out how to do the complicated bits like the horrible plumbing and CGO stuff in filecoin-ffi. Also look through git history to find examples of this. filecoin-project/filecoin-ffi#512 is pretty close and recent, plumbing a boolean option down into ref-fvm, see the linked ref-fvm PR there and also the corresponding change in lotus (somewhere). |
|
I made the workflow work and pass all the tests including fixes to all your above comments concerns! I might need some time to figure out how to cross-reference make PRs to each repo as the one that you shared! Thanks @rvagg |
|
@nijoe1 you'll probably have trouble getting CI happy when you do the separate PRs, it's possible, with Cargo.toml changes, but we can also deal with them individually and I could review by checking out your work and trying it for myself. Do your best and we'll work with it. |
eded1f7 to
c0d0c97
Compare
…from external filecoin-ffi
|
@rvagg I made the requested changes around the synthetic-actor and maybe-modify-actor with only one additional function into filecoin-ffi. |
Related Issues
Fixes compatibility with Geth's
eth_callandeth_estimateGasbehavior where callers can simulate transactions from contract addresses or non-existent addresses. This is required for tools like Hardhat, Foundry, and other Ethereum development frameworks that rely on this behavior for gas estimation and call simulation.Proposed Changes
API Layer:
ErrSenderValidationFailederror type to distinguish sender validation failures from other errorsVM Layer:
ApplyMessageSkipSenderValidationmethod to VM interfaceState Manager:
CallWithGasSkipSenderValidationfor gas estimation from any addressApplyOnStateWithGasSkipSenderValidationfor eth_call simulationcreateSyntheticSenderActorto create ephemeral EthAccount actors for non-existent sendersmaybeModifySenderForSimulationto temporarily modify EVM/Placeholder actors to EthAccount for simulationEth/Gas API:
EthEstimateGasnow falls back to skip-sender-validation when sender validation failsEthCalluses skip-sender-validation for all callsErrExecutionRevertederrors for proper JSON-RPC codec handlingeth_call_count,eth_estimate_gas_count,eth_estimate_gas_skip_senderKey behaviors:
Additional Info
This matches Geth's
SkipAccountChecksbehavior instate_transition.go. The implementation ensures:fixes: #12441 #12896
Checklist
Before you mark the PR ready for review, please make sure that: