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

Use STATICCALL for pure function calls. #2966

Merged
merged 3 commits into from Mar 6, 2018

Conversation

Projects
2 participants
@chriseth
Contributor

chriseth commented Sep 26, 2017

No description provided.

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Sep 26, 2017

Contributor

This still needs tests and requires an updated version of cpp-ethereum.

Contributor

chriseth commented Sep 26, 2017

This still needs tests and requires an updated version of cpp-ethereum.

Show outdated Hide outdated Changelog.md Outdated
Show outdated Hide outdated Changelog.md Outdated
@@ -1572,6 +1572,9 @@ void ExpressionCompiler::appendExternalFunctionCall(
bool returnSuccessCondition = funKind == FunctionType::Kind::BareCall || funKind == FunctionType::Kind::BareCallCode || funKind == FunctionType::Kind::BareDelegateCall;
bool isCallCode = funKind == FunctionType::Kind::BareCallCode || funKind == FunctionType::Kind::CallCode;
bool isDelegateCall = funKind == FunctionType::Kind::BareDelegateCall || funKind == FunctionType::Kind::DelegateCall;
bool useStaticCall =

This comment has been minimized.

@axic

axic Oct 16, 2017

Member

I think use suits this better, but the other two are named is so perhaps just go with isStaticCall or alternatively rename the other two to use?

@axic

axic Oct 16, 2017

Member

I think use suits this better, but the other two are named is so perhaps just go with isStaticCall or alternatively rename the other two to use?

This comment has been minimized.

@chriseth

chriseth Oct 16, 2017

Contributor

It's a different thing. I would call it isStaticCall out of the function type, but useStaticCall tells whether the compiler is also allowed to use the opcode due to pragmas.

@chriseth

chriseth Oct 16, 2017

Contributor

It's a different thing. I would call it isStaticCall out of the function type, but useStaticCall tells whether the compiler is also allowed to use the opcode due to pragmas.

@axic

Needs tests, changelog update and a rebase.

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Dec 12, 2017

Member

Still needs tests.

Member

axic commented Dec 12, 2017

Still needs tests.

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Jan 16, 2018

Contributor

Added tests and documentation. Should be ready to merge.

Contributor

chriseth commented Jan 16, 2018

Added tests and documentation. Should be ready to merge.

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Jan 16, 2018

Member

Perhaps we should have a target pragma specifying the chain (related #1117). The question there becomes wether we enforce the latest chain and allow to go back to the previous one or we stay on longer on the "old chain".

In example:

  • stay on Homestead and enable Byzantium via pragma target byztantium or pragma byzantium
  • stay on Byzantium and enable Homestead via pragma homestead
Member

axic commented Jan 16, 2018

Perhaps we should have a target pragma specifying the chain (related #1117). The question there becomes wether we enforce the latest chain and allow to go back to the previous one or we stay on longer on the "old chain".

In example:

  • stay on Homestead and enable Byzantium via pragma target byztantium or pragma byzantium
  • stay on Byzantium and enable Homestead via pragma homestead
@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Jan 16, 2018

Contributor

I would prefer a pragma homestead directive, or perhaps pragma target homestead

Contributor

chriseth commented Jan 16, 2018

I would prefer a pragma homestead directive, or perhaps pragma target homestead

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Jan 25, 2018

Contributor

Decision from discussion:

add a pragma target <x> pragma. The default is always "latest version of the EVM". Currently supported values are byzantium and homestead. Everything else is rejected.

homestead could also be used to force gas calculation for calls (which is currently still the default).

Contributor

chriseth commented Jan 25, 2018

Decision from discussion:

add a pragma target <x> pragma. The default is always "latest version of the EVM". Currently supported values are byzantium and homestead. Everything else is rejected.

homestead could also be used to force gas calculation for calls (which is currently still the default).

@axic axic added the nextrelease label Feb 14, 2018

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Feb 14, 2018

Member

Should use the new evmTarget from #1117.

Member

axic commented Feb 14, 2018

Should use the new evmTarget from #1117.

@chriseth chriseth added this to Planned for 0.4.21 in 0.4.21 Feb 16, 2018

@chriseth chriseth moved this from Planned for 0.4.21 to In Progress in 0.4.21 Feb 16, 2018

@chriseth chriseth moved this from In Progress to Under Review in 0.4.21 Feb 19, 2018

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Feb 19, 2018

Contributor

@axic this only requires the evm target check, or is something else missing?

Contributor

chriseth commented Feb 19, 2018

@axic this only requires the evm target check, or is something else missing?

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Mar 2, 2018

Contributor

Rebased. Now depends on #3569 and #3600 .

Contributor

chriseth commented Mar 2, 2018

Rebased. Now depends on #3569 and #3600 .

Show outdated Hide outdated Changelog.md Outdated
Show outdated Hide outdated docs/contracts.rst Outdated
@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Mar 2, 2018

Member

The static call changes look good.

Member

axic commented Mar 2, 2018

The static call changes look good.

@axic

axic approved these changes Mar 2, 2018

Apart from the changelog comment the staticcall changes looks fine. (Should of course merge the dependant PRs first.)

@@ -1607,6 +1607,10 @@ void ExpressionCompiler::appendExternalFunctionCall(
bool returnSuccessCondition = funKind == FunctionType::Kind::BareCall || funKind == FunctionType::Kind::BareCallCode || funKind == FunctionType::Kind::BareDelegateCall;
bool isCallCode = funKind == FunctionType::Kind::BareCallCode || funKind == FunctionType::Kind::CallCode;
bool isDelegateCall = funKind == FunctionType::Kind::BareDelegateCall || funKind == FunctionType::Kind::DelegateCall;
bool useStaticCall =
_functionType.stateMutability() <= StateMutability::View &&
m_context.experimentalFeatureActive(ExperimentalFeature::V050) &&

This comment has been minimized.

@axic

axic Mar 6, 2018

Member

Actually this contradicts #3600.

@axic

axic Mar 6, 2018

Member

Actually this contradicts #3600.

This comment has been minimized.

@axic

axic Mar 6, 2018

Member

Do we really need this line? We should just enable it based on the EMV version (which we also have as a condition below).

@axic

axic Mar 6, 2018

Member

Do we really need this line? We should just enable it based on the EMV version (which we also have as a condition below).

This comment has been minimized.

@chriseth

chriseth Mar 6, 2018

Contributor

We perform changes in codegen from time to time without requiring a breaking release. The condition for that is that the actual semantics visible from outside do not change. This is only the case if people do not perform invalid type conversions and if they use the most recent EVM version. I think it is fine to assume the latter, so we can just remove it. We should highlight that in the release notes.

@chriseth

chriseth Mar 6, 2018

Contributor

We perform changes in codegen from time to time without requiring a breaking release. The condition for that is that the actual semantics visible from outside do not change. This is only the case if people do not perform invalid type conversions and if they use the most recent EVM version. I think it is fine to assume the latter, so we can just remove it. We should highlight that in the release notes.

This comment has been minimized.

@chriseth

chriseth Mar 6, 2018

Contributor

Ok but actually arguing the other way around: If we remove this condition, then we actually assume that it is perfectly fine to use. This means it is not an experimental feature. In turn, we don't have to warn about using experimental 0.5.0.

@chriseth

chriseth Mar 6, 2018

Contributor

Ok but actually arguing the other way around: If we remove this condition, then we actually assume that it is perfectly fine to use. This means it is not an experimental feature. In turn, we don't have to warn about using experimental 0.5.0.

This comment has been minimized.

@axic

axic Mar 6, 2018

Member

Copying this from the channel for the record:

The argument is that we specifically state in the changelog that pragma experimental v0.5.0 is analysis only and won’t change the codegen phase.

The second argument is whether staticcall is experiemental or not and I’m leaning towards to say it isn’t experimental. That code should work, if it doesn’t that is a broken VM or a broken implementation in the codegen.

There is one case where this could cause an issue if the user disregards the warnings from the ViewPureChecker and does expected state modification in a view function and calls it externally in another contract.

Luckily, the ViewPureChecker reports a warning for these cases (https://github.com/ethereum/solidity/blob/develop/libsolidity/analysis/ViewPureChecker.cpp#L254) so it should only fail as a matter of programmer error.

Though it is kind of a breaking change because if they disregard the warning the output will not work.

@axic

axic Mar 6, 2018

Member

Copying this from the channel for the record:

The argument is that we specifically state in the changelog that pragma experimental v0.5.0 is analysis only and won’t change the codegen phase.

The second argument is whether staticcall is experiemental or not and I’m leaning towards to say it isn’t experimental. That code should work, if it doesn’t that is a broken VM or a broken implementation in the codegen.

There is one case where this could cause an issue if the user disregards the warnings from the ViewPureChecker and does expected state modification in a view function and calls it externally in another contract.

Luckily, the ViewPureChecker reports a warning for these cases (https://github.com/ethereum/solidity/blob/develop/libsolidity/analysis/ViewPureChecker.cpp#L254) so it should only fail as a matter of programmer error.

Though it is kind of a breaking change because if they disregard the warning the output will not work.

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Mar 6, 2018

Member

Rebased it.

Member

axic commented Mar 6, 2018

Rebased it.

@axic axic merged commit 14b12ae into develop Mar 6, 2018

8 checks passed

ci/circleci: build_emscripten Your tests passed on CircleCI!
Details
ci/circleci: build_x86 Your tests passed on CircleCI!
Details
ci/circleci: docs Your tests passed on CircleCI!
Details
ci/circleci: test_emscripten_external Your tests passed on CircleCI!
Details
ci/circleci: test_emscripten_solcjs Your tests passed on CircleCI!
Details
ci/circleci: test_x86 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

0.4.21 automation moved this from Under Review to Done Mar 6, 2018

@axic axic deleted the useStaticCall branch Mar 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment