solidity icon indicating copy to clipboard operation
solidity copied to clipboard

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

Open mattaereal opened this issue 11 months ago • 7 comments

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.

mattaereal avatar Mar 11 '24 17:03 mattaereal

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.

github-actions[bot] avatar Mar 11 '24 17:03 github-actions[bot]

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.

tinchoabbate avatar Mar 14 '24 12:03 tinchoabbate

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

mattaereal avatar Mar 15 '24 19:03 mattaereal

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 avatar Mar 15 '24 21:03 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.

cameel avatar Mar 15 '24 21:03 cameel

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

mattaereal avatar Mar 18 '24 17:03 mattaereal

I'll rebase and squash the commits down to one when I get to my computer (currently traveling).

mattaereal avatar Aug 22 '24 19:08 mattaereal