Do not add extcodesize check when revertStrings is set to debug

[shermike]

If revertStrings option is set to debug or higher, then the compiler always adds the extcodesize check even if the function has a return value.

I believe it's a bug because it's unnatural for the compiler to change its logic just because a debug option is provided.

[github-actions]

Thank you for your contribution to the Solidity compiler! A team member will follow up shortly.

If you haven't read our contributing guidelines and our review checklist before, please do it now, this makes the reviewing process and accepting your contribution smoother.

If you have any questions or need our help, feel free to post them in the PR or talk to us directly on the #solidity-dev channel on Matrix.

[nikola-matic]

This isn't a bug - it was implemented as part of #8178 in order to provide revert strings for internal reverts as a way to provide additional debug info.

I believe it's a bug because it's unnatural for the compiler to change its logic just because a debug option is provided.

Compilers do that all the time, especially with regards to optimizations, or lack thereof in case of debug builds. That's why you're not supposed to deploy code generated using a debug build (gas costs aside), or in general use debug builds for anything other than debugging (rule of thumb for all languages, not just Solidity).

[cameel]

To expand on what @nikola-matic said, the fact that the compiler skips EXTCODESIZE check in some situations is just an implementation detail. We do that when we know that calling an EOA will not significantly change contract's behavior. This is the case e.g. when we expect the call to return something, because an EOA can never return anything and the check against non-zero size of retundata cannot pass.

The problem is that enabling verbose revert reasons makes these two reverts distinguishable. To be able to return accurate information, we have to keep it. Otherwise the behavior would not be consistent between calls that return something and ones that do not.

[shermike]

I'm not sure I understood your point correctly. Let me clarify my concerns.

What if the callee is not EOA but custom precompile and we call it as a regular contract (I understand it is a controversial approach but still)? In that case we have just different contract logic: without a verbose revert, everything works fine; but with verbose revert - it fails due to extcodecheck.

Even if the address is an EOA, calling Contract(eoa).someFunc() will fail differently depending on the revert settings: without verbose revert, it fails during result decoding; with verbose revert, it fails during the extcodecheck.

[cameel]

without verbose revert, it fails during result decoding; with verbose revert, it fails during the extcodecheck.

It fails at a different point, but the observable end result (aside from gas) is the same - a revert without a message.

What if the callee is not EOA but custom precompile

Ok, I see your point. True, when you enable verbose reverts this check prevents high-level calls to precompiles. That's unfortunate, though the fact that external calls even work with precompiles was not really intended in the first place. I don't think this is enough of a reason to change this behavior in a general case. You can always work around it with a low-level call after all.

Still, the EXTCODESIZE check comes up often for various reasons and we even considered dropping it for consistency with EOF (which would have made it impossible to implement). In the end it's more of a sanity check than something absolutely essential. Not sure we should go that far, but we're open to discussing it. I think that at the very least it would not be a bad idea to provide a mechanism to opt out, e.g.:

contract.someFunc{checked: false}()

I'd recommend opening a separate issue or forum thread for that topic if you want to pursue it though.

[shermike]

It fails at a different point, but the observable end result (aside from gas) is the same - a revert without a message.

AFAIU the result with revertStrings=debug will be a revert message Target contract does not contain code instead of something from decoding routine (like Decode result failed) if this PR is applied.

I'd recommend opening a separate issue or forum thread for that topic if you want to pursue it though.

Initially, I was going to open an issue, but then decided to create a PR right away since I was confident it was a bug :)

Anyway, thanks for the explanation!