Skip to content

Use STATICCALL for pure function calls. #2966

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.

Sign up for GitHub

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

Merged
axic merged 3 commits into from
Mar 6, 2018
Merged

Use STATICCALL for pure function calls. #2966

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b467116
Use STATICCALL for pure function calls if EVM version supports it and…
chriseth Sep 26, 2017
a3593df
Test for using staticcall for view and pure functions.
chriseth Jan 15, 2018
3057aee
Document STATICCALL usage in experimental 0.5.0.
chriseth Jan 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@

Features:
* C99/C++-style scoping rules (instead of JavaScript function scoping) take effect as experimental v0.5.0 feature.
* Code Generator: Assert that ``k != 0`` for ``molmod(a, b, k)`` and ``addmod(a, b, k)`` as experimental 0.5.0 feature.
* Code Generator: Assert that ``k != 0`` for ``mulmod(a, b, k)`` and ``addmod(a, b, k)`` as experimental 0.5.0 feature.
* Code Generator: Do not retain any gas in calls (except if EVM version is set to homestead).
* Code Generator: Use ``STATICCALL`` opcode for calling ``view`` and ``pure`` functions as experimenal 0.5.0 feature.
* Interface: Provide ability to select target EVM version (homestead or byzantium, with byzantium being the default).
* Standard JSON: Reject badly formatted invalid JSON inputs.
* Type Checker: Disallow uninitialized storage pointers as experimental 0.5.0 feature.
Expand Down
19 changes: 19 additions & 0 deletions docs/contracts.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,6 +472,13 @@ The following statements are considered modifying the state:
.. note::
Getter methods are marked ``view``.

.. note::
If invalid explicit type conversions are used, state modifications are possible
even though a ``view`` function was called.
You can switch the compiler to use ``STATICCALL`` when calling such functions and thus
prevent modifications to the state on the level of the EVM by adding
``pragma experimental "v0.5.0";``

.. warning::
The compiler does not enforce yet that a ``view`` method is not modifying state. It raises a warning though.

Expand Down Expand Up @@ -502,6 +509,18 @@ In addition to the list of state modifying statements explained above, the follo
}
}

.. note::
If invalid explicit type conversions are used, state modifications are possible
even though a ``pure`` function was called.
You can switch the compiler to use ``STATICCALL`` when calling such functions and thus
prevent modifications to the state on the level of the EVM by adding
``pragma experimental "v0.5.0";``

.. warning::
It is not possible to prevent functions from reading the state at the level
of the EVM, it is only possible to prevent them from writing to the state
(i.e. only ``view`` can be enforced at the EVM level, ``pure`` can not).

.. warning::
Before version 0.4.17 the compiler didn't enforce that ``pure`` is not reading the state.

Expand Down
4 changes: 2 additions & 2 deletions libsolidity/ast/ExperimentalFeatures.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ namespace solidity

enum class ExperimentalFeature
{
SMTChecker,
ABIEncoderV2, // new ABI encoder that makes use of JULIA
SMTChecker,
V050, // v0.5.0 breaking changes
Test,
TestOnlyAnalysis
Expand All @@ -45,8 +45,8 @@ static const std::map<ExperimentalFeature, bool> ExperimentalFeatureOnlyAnalysis

static const std::map<std::string, ExperimentalFeature> ExperimentalFeatureNames =
{
{ "SMTChecker", ExperimentalFeature::SMTChecker },
{ "ABIEncoderV2", ExperimentalFeature::ABIEncoderV2 },
{ "SMTChecker", ExperimentalFeature::SMTChecker },
{ "v0.5.0", ExperimentalFeature::V050 },
{ "__test", ExperimentalFeature::Test },
{ "__testOnlyAnalysis", ExperimentalFeature::TestOnlyAnalysis },
Expand Down
9 changes: 9 additions & 0 deletions libsolidity/codegen/ExpressionCompiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1610,6 +1610,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 =
Copy link
Contributor

@axic axic Oct 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@chriseth chriseth Oct 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

_functionType.stateMutability() <= StateMutability::View &&
m_context.experimentalFeatureActive(ExperimentalFeature::V050) &&
Copy link
Contributor

@axic axic Mar 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this contradicts #3600.

Copy link
Contributor

@axic axic Mar 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@chriseth chriseth Mar 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@chriseth chriseth Mar 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@axic axic Mar 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

m_context.evmVersion().hasStaticCall();

unsigned retSize = 0;
if (returnSuccessCondition)
Expand Down Expand Up @@ -1738,6 +1742,8 @@ void ExpressionCompiler::appendExternalFunctionCall(
// [value,] addr, gas (stack top)
if (isDelegateCall)
solAssert(!_functionType.valueSet(), "Value set for delegatecall");
else if (useStaticCall)
solAssert(!_functionType.valueSet(), "Value set for staticcall");
else if (_functionType.valueSet())
m_context << dupInstruction(m_context.baseToCurrentStackOffset(valueStackPos));
else
Expand Down Expand Up @@ -1769,10 +1775,13 @@ void ExpressionCompiler::appendExternalFunctionCall(
gasNeededByCaller += eth::GasCosts::callNewAccountGas; // we never know
m_context << gasNeededByCaller << Instruction::GAS << Instruction::SUB;
}
// Order is important here, STATICCALL might overlap with DELEGATECALL.
if (isDelegateCall)
m_context << Instruction::DELEGATECALL;
else if (isCallCode)
m_context << Instruction::CALLCODE;
else if (useStaticCall)
m_context << Instruction::STATICCALL;
else
m_context << Instruction::CALL;

Expand Down
60 changes: 56 additions & 4 deletions test/libsolidity/SolidityEndToEndTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,20 @@
* Unit tests for the solidity expression compiler, testing the behaviour of the code.
*/

#include <test/libsolidity/SolidityExecutionFramework.h>

#include <test/TestHelper.h>

#include <libsolidity/interface/Exceptions.h>
#include <libsolidity/interface/EVMVersion.h>

#include <libevmasm/Assembly.h>

#include <boost/test/unit_test.hpp>

#include <functional>
#include <string>
#include <tuple>
#include <boost/test/unit_test.hpp>
#include <libevmasm/Assembly.h>
#include <libsolidity/interface/Exceptions.h>
#include <test/libsolidity/SolidityExecutionFramework.h>

using namespace std;
using namespace std::placeholders;
Expand Down Expand Up @@ -10778,6 +10785,51 @@ BOOST_AUTO_TEST_CASE(snark)
BOOST_CHECK(callContractFunction("verifyTx()") == encodeArgs(true));
}

BOOST_AUTO_TEST_CASE(staticcall_for_view_and_pure)
{
char const* sourceCode = R"(
pragma experimental "v0.5.0";
contract C {
uint x;
function f() public returns (uint) {
x = 3;
return 1;
}
}
interface CView {
function f() view external returns (uint);
}
interface CPure {
function f() pure external returns (uint);
}
contract D {
function f() public returns (uint) {
return (new C()).f();
}
function fview() public returns (uint) {
return (CView(new C())).f();
}
function fpure() public returns (uint) {
return (CPure(new C())).f();
}
}
)";
compileAndRun(sourceCode, 0, "D");
// This should work (called via CALL)
ABI_CHECK(callContractFunction("f()"), encodeArgs(1));
if (dev::test::Options::get().evmVersion().hasStaticCall())
{
// These should throw (called via STATICCALL)
ABI_CHECK(callContractFunction("fview()"), encodeArgs());
ABI_CHECK(callContractFunction("fpure()"), encodeArgs());
}
else
{
ABI_CHECK(callContractFunction("fview()"), encodeArgs(1));
ABI_CHECK(callContractFunction("fpure()"), encodeArgs(1));
}
}

BOOST_AUTO_TEST_SUITE_END()

}
Expand Down