-
Notifications
You must be signed in to change notification settings - Fork 65
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
core: fix missing call trace for create/create2 w/ early return #1706
Conversation
traces_.senders.insert(state.msg->recipient); | ||
traces_.recipients.insert(contract_address); | ||
} else if (op_code == evmc_opcode::OP_CREATE2) { | ||
SILKWORM_ASSERT(stack_height >= 4); |
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.
Is on_instruction_start
called after evmone has verified stack_height >= 4
& init_code_offset < state.memory.size()
? If that's not the case, we shouldn't use SILKWORM_ASSERT
, because that will simply terminate the entire program. (SILKWORM_ASSERT
should only be used for asserting invariants already established by some other code.)
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.
AFAIK evmone
does not check such conditions, but I'm waiting for @chfast on this. Surely, I agree that the current solution is far from ideal. I thought about the available options and no one seemed really satisfying:
- we could easily check for both conditions and do nothing, but this would mean that if something changes (e.g. invariant break) we silently skip the call trace registration. This is not good because we would obtain a silent difference in call traces, which would resurface as a difficult-to-debug flaw at the API level
- as a mitigation, we could go with option 1. reinforced by an error log, but currently we have logging support only starting from
infra
package, not incore
package. Not sure if this specific case is worth the effort to make logging compatible witheWASM
compilation - we cannot use exceptions because we are in
core
package - we cannot return any error code because
on_instruction_start
isvoid
(I think this design is correct, I don't see any value in returning error codes from tracers intoevmone
) SILKWORM_ASSERT
has the drawback you underline, but at least it makes the invariant break visible even if "in the hard way"
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.
I'd simply do nothing & return successfully in case the arguments are invalid and we can't recover CREATE2 address. Smart contract code is external/outside our control and we should handle such situations gracefully. What happens in Erigon's tracing in the case of invalid arguments?
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.
I'd simply do nothing & return successfully in case the arguments are invalid and we can't recover CREATE2 address. Smart contract code is external/outside our control and we should handle such situations gracefully.
Fair enough. I completely agree with graceful handling for unexpected conditions, so I will adopt option 1.
What happens in Erigon's tracing in the case of invalid arguments?
Unfortunately, generally speaking the answer to this question is not unique because failure conditions (e.g. out-of-gas or insufficient balance) are treated differently in contract calls and contract creations: some of them do trigger the tracers, other ones do not without a clear motivation. Probably some legacy from existing go-evm
behaviour and OE tracing compatibility may be in place here.
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.
Yes, this is called before any conditions are checked in evmone. This can be changed but I'm also not sure where to place it exactly. E.g. if this is placed after stack requirements are checked the tracer would not be called.
However, we can try to extend on_execution_end
to also contain the last instruction or code position. The on_execution_end
already contains the error code the execution ended with.
@chfast might be of interest to you |
More unit tests
This PR fixes another incompatibility between Silkworm and Erigon database content related to call traces.
Specifically, the discrepancy happened on mainnet at block 1305821, where transaction 0 hits the fallback function of contract 0xFDf3D6c72F8aF59Fd1D7Fa65D1C12f89f12F9394. After some interactions with another address, such a contract executes a
CREATE
opcode to deploy another contract: this operation "silently succeeds" during one early check before the code deployment, i.e. in this specific case the deploying contract has insufficient balance to cover the expected endowment for the new contract.When an early return happens (positive or negative, it doesn't matter) during preliminary checks performed by the
evmone
, no invocation ofevmone::Host::call
hook takes place, hence no proper execution context gets activated for theCREATE
operation. This in turns means that any registered EVM tracer does not receive any notification on itson_execution_start
callback, as it is the case instead when aCREATE
operation is effectively executed (independently from its final result).So, the only way to intercept any
CREATE
operation executing an early return for the purpose of tracing its sender and recipient is then by theon_instruction_start
callback.All the above considerations can be applied to the
CREATE2
operation as well.