Document the possibility of high-level external calls to precompiled contracts

[mattaereal]

Hi! I'm trying to add a bit more clarity about something that has been somewhat recently documented but wasn't as easy as I expected to understand from the docs.

A few weeks ago, while checking out the code of Blast (L2), we learned that you can do external calls over precompiled contracts without failing and were wondering how that could be possible. After debugging the compiler and, as the suggestion states, from v0.8.10, the extcodesize check is removed from external calls if there's return data to decode, since it would fail anyway at the time of decoding.

We thought this behavior should be noted on different occasions, particularly while working with precompiled contracts (we thought it wouldn't work). So, without modifying much and reusing what was already written (in the docs and in the commit message), I added a few references and included a note that was outside a callout inside of another.

[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.

[cameel]

Thanks for your interest in improving the docs!

But well, this isn't an exception made on purpose for precompiles. Looking back at the PR that introduced it, this is technically an unwanted change in behavior, but a pretty obscure one and not harmful so it was not considered worth addressing or documenting in detail (see e.g. #12205 (comment)). Before 0.8.10 a high-level external call to a precompiled contract would revert due to that check, but it did not really matter, since they're pretty low-level themselves and we expect people to use low-level calls for that.

And TBH these details seem way too obscure to put them in "Introduction to Smart Contracts". That spot in control-structures.rst would be more appropriate. You could just add a sentence there saying that the consequence of this is that it makes high-level external calls to precompiles possible.

TBH, I'm not sure we should document it though. If we document it, we commit to officially supporting it and to me it seems more like a grey area of undefined behavior that's not guaranteed. Though if you connect the dots based on the current docs, you can already conclude that it means that high-level calls to precompiles should work so maybe it does not matter - we already stepped into that one.

[tinchoabbate]

Hi there! Adding my perspective to the discussion.

since they're pretty low-level themselves and we expect people to use low-level calls for that.

AFAIK, that's not happening in reality. Some people might be using low-level calls as you expect, but there're others using this behavior of the compiler. For example, to perform high-level calls to custom precompiles in L2s.

TBH, I'm not sure we should document it though. If we document it, we commit to officially supporting it and to me it seems more like a grey area of undefined behavior that's not guaranteed.

By documenting it we'd describe the actual behavior of the compiler. I mean, it already works like this. So what's the point in hiding it? If the team doesn't want to officially support it, then the compiler's code should be changed, right? If it's a bug it should be fixed, if it's a feature it should be documented clearly. I find it hard to see a gray area.

Though if you connect the dots based on the current docs, you can already conclude that it means that high-level calls to precompiles should work so maybe it does not matter - we already stepped into that one.

Documenting it does matter. It removes uncertainty from developers using the compiler and from us security folks who dive into these kind of details of Solidity. I'd rather have it documented explicitly than somehow becoming an obscure behavior of the compiler that developers can run into unexpectedly. It's best to have a clear documentation that people can rely on instead of on unsafe assumptions.

[mattaereal]

Weird. The tests that are failing aren't related to the documentation.

[cameel]

I discussed this with @ekpyron and we're fine with this being documented after all. Like I said, the feature is mostly unintended, but we don't see any good reason to change this behavior in the future so we can just as well explain how it works, as long as it's done in a way that won't be more confusing than helpful (it's a pretty obscure corner case after all).

[cameel]

I still think that "Introduction to Smart Contracts" is not the right place for this kind of detail. It would be much better off added to the note about precompiles that we alredy have under "External Function Calls". It would be enough to have a link to that section here.

Also, it should make a clear distinction between low-level and high-level external calls. I actually found it pretty confusing without that distinction when I was reading it for the first time.

[mattaereal]

I'm on it. Though I'm not on my setup so I'll be doing it via web browser.

[cameel]

I mean, it already works like this. So what's the point in hiding it?

The point is that some details fall into the category of unspecified behavior. A spec does not always prescribe compiler's behavior in minute detail and things that are not in the spec are subject to change between versions, implementations, etc. For us the docs are the closest thing to a spec that we have. So we sometimes intentionally don't describe every detail, because we don't want users to rely on it.

In this case we decided we're actually fine making this a part of the spec, but the point still stands in general. I mean, would be best to have both - a spec and a detailed description of how our implementation works, but we're a bit resource constrained at the moment so the best we can manage are the docs in the current form.

[cameel]

Weird. The tests that are failing aren't related to the documentation.

You can ignore them. This is a documentation change and those tests can't possibly be affected by it.

Our external tests are notoriously flaky unfortunately. They're just test suites of several real projects written in Solidity that we use to verify backwards-compatibility of changes to the compiler and for benchmarking. Unfortunately the JS ecosystem is a dependency hell and even when it's not a dependency issue, those test suites themselves are not always deterministic so they break randomly all the time. They will probably succeed if you rerun them, but they're not required to merge the PR anyway.

[mattaereal]

I think I fulfilled what you requested @cameel.

1. Removed the warning on precompiled contracts from introductory since it's pretty obscure for an introduction and may change in the future (idk).
2. Added a note on control-structure file in the section of external calls.
3. Specifically mentioned high-level calls