fix[lang]: disallow `blockhash` in `pure` functions #3157

tserg · 2022-11-24T08:25:23Z

What I did

Fix #3141.

How I did it

Check for Name nodes with blockhash in local validation of pure functions.

How to verify it

See test.

Commit message

fix: disallow blockhash in pure functions

Description for the changelog

Disallow blockhash in pure functions

Cute Animal Picture

tserg · 2022-11-24T09:44:17Z

An alternative and perhaps more scalable approach is to add a _reads_environment property to BuiltinFunction, which is set to True for BlockHash(). We can then define the set of builtin functions that reads from the chain by checking for this property (e.g. ENVIRONMENT_BUILTIN_FUNCTIONS). However, I encountered issues with circular imports if I define ENVIRONMENT_BUILTIN_FUNCTIONS in vyper/builtin_functions/functions.py and import it in vyper/semantics/validation/local.py.

charles-cooper

yea, i think the clearer and more maintainable approach would be to set the mutability of the builtin correctly

codecov-commenter · 2022-11-27T06:33:04Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.38%. Comparing base (4595938) to head (e2e9864).

❗ Current head e2e9864 differs from pull request most recent head b1517b7. Consider uploading reports for the commit b1517b7 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3157      +/-   ##
==========================================
+ Coverage   86.34%   86.38%   +0.03%     
==========================================
  Files          92       92              
  Lines       14028    14026       -2     
  Branches     3083     3081       -2     
==========================================
+ Hits        12113    12116       +3     
+ Misses       1487     1483       -4     
+ Partials      428      427       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

charles-cooper · 2022-11-27T06:35:40Z

vyper/semantics/validation/local.py

+            from vyper.builtin_functions.functions import BUILTIN_FUNCTIONS
+            builtin_fns = fn_node.get_descendants(vy_ast.Name, {"id": set(BUILTIN_FUNCTIONS)})
+
+            node_list.extend(builtin_fns)  # type: ignore


why do we need to do this?

To add builtin function calls to the list of nodes to check. Or should the check for builtin functions be performed elsewhere?

oh for some reason i thought the mutability check was performed under visit_Call. i guess this solution is ok for now; i want to get in #2974 first though

…blockhash

vyper/builtins/_signatures.py

vyper/builtins/functions.py

vyper/semantics/analysis/local.py

tserg · 2023-05-18T07:38:09Z

I have changed the approach to one that also fixes #3425.

charles-cooper · 2023-05-18T11:54:28Z

vyper/semantics/analysis/local.py

+            # Add all calls except structs
+            filtered_call_nodes = [
+                c.func
+                for c in fn_node.get_descendants(vy_ast.Call)
+                if not (len(c.args) == 1 and isinstance(c.args[0], vy_ast.Dict))
+            ]
+            node_list.extend(filtered_call_nodes)  # type: ignore
+
            for node in node_list:
                t = node._metadata.get("type")
-                if isinstance(t, ContractFunctionT) and t.mutability == StateMutability.PURE:
+                # skip structs and interface instantiation
+                if isinstance(t, StructT) or is_type_t(t, InterfaceT):
+                    continue
+
+                # skip ContractFunctionT and BuiltinFunction with mutability set to pure
+                if hasattr(t, "mutability") and t.mutability == StateMutability.PURE:


i think the ast analysis is still not the way to go. i think probably a better approach is to add a mutability to all ExprInfos, and then during local.py we can check at every single expression (and maybe statement too) whether expr_info.mutability > self.func.mutability.

…blockhash

charles-cooper · 2024-02-21T12:55:02Z

this is looking better. i'm still hesitant though, because i tried this myself and realized that the approach doesn't work for raw_call -- raw_call requires a call site in order to figure out whether it's a staticcall or not. but maybe we can just fenagle in some extra logic into raw_call (maybe in its fetch_call_return implementation)

…blockhash

charles-cooper · 2024-03-18T14:05:29Z

vyper/builtins/functions.py

@@ -1061,6 +1061,25 @@ def fetch_call_return(self, node):

        kwargz = {i.arg: i.value for i in node.keywords}

+        delegate_call = kwargz.get("is_delegate_call")


what if the user passes is_delegate_call=False?

charles-cooper · 2024-03-18T14:06:07Z

vyper/builtins/functions.py

+        if (delegate_call or static_call) and value is not None:
+            raise ArgumentException("value= may not be passed for static or delegate calls!")
+
+        fn_node = node.get_ancestor(vy_ast.FunctionDef)


note for refactoring: ideally we should pass this in a context object

should I add a comment here?

tests/functional/codegen/features/decorators/test_pure.py

charles-cooper · 2024-03-25T13:48:00Z

tests/functional/codegen/calling_convention/test_external_contract_calls.py

@@ -900,25 +900,29 @@ def set_lucky(arg1: address, arg2: int128):
    print("Successfully executed an external contract call state change")


-def test_constant_external_contract_call_cannot_change_state():
-    c = """
+@pytest.mark.parametrize("state_modifying_decorator", ("payable", "nonpayable"))


Suggested change

@pytest.mark.parametrize("state_modifying_decorator", ("payable", "nonpayable"))

@pytest.mark.parametrize("modifying", ("payable", "nonpayable"))

charles-cooper · 2024-03-25T13:48:17Z

tests/functional/codegen/calling_convention/test_external_contract_calls.py

-def test_constant_external_contract_call_cannot_change_state():
-    c = """
+@pytest.mark.parametrize("state_modifying_decorator", ("payable", "nonpayable"))
+@pytest.mark.parametrize("non_modifying_decorator", ("pure", "view"))


Suggested change

@pytest.mark.parametrize("non_modifying_decorator", ("pure", "view"))

@pytest.mark.parametrize("constant", ("pure", "view"))

charles-cooper · 2024-03-25T13:48:22Z

tests/functional/codegen/calling_convention/test_external_contract_calls.py

-def test_constant_external_contract_call_cannot_change_state():
-    c = """
+@pytest.mark.parametrize("state_modifying_decorator", ("payable", "nonpayable"))
+@pytest.mark.parametrize("non_modifying_decorator", ("pure", "view"))


Suggested change

@pytest.mark.parametrize("non_modifying_decorator", ("pure", "view"))

@pytest.mark.parametrize("constant", ("pure", "view"))

vyper/builtins/functions.py

charles-cooper · 2024-03-25T13:56:35Z

vyper/builtins/functions.py

@@ -1061,6 +1061,26 @@ def fetch_call_return(self, node):

        kwargz = {i.arg: i.value for i in node.keywords}


maybe get i.get_folded_value().value here

charles-cooper · 2024-03-25T13:59:21Z

let's revert the changes to raw_call here because they seem a bit out of scope; we can revisit in another PR.

…ix/blockhash

vyper/semantics/analysis/local.py

…blockhash

vyper/builtins/_signatures.py

@@ -6,7 +6,7 @@
 from vyper.codegen.expr import Expr
 from vyper.codegen.ir_node import IRnode
 from vyper.exceptions import CompilerPanic, TypeMismatch, UnfoldableNode
-from vyper.semantics.analysis.base import Modifiability
+from vyper.semantics.analysis.base import Modifiability, StateMutability


vyper/builtins/functions.py

@@ -53,7 +53,7 @@
    UnfoldableNode,
    ZeroDivisionException,
 )
-from vyper.semantics.analysis.base import Modifiability, VarInfo
+from vyper.semantics.analysis.base import Modifiability, StateMutability, VarInfo


cyberthirst · 2024-10-11T08:53:47Z

should we do the same for blobhash?

charles-cooper · 2024-10-11T11:45:39Z

should we do the same for blobhash?

Yes

tserg · 2024-10-11T14:04:39Z

tests/functional/codegen/calling_convention/test_external_contract_calls.py

Should I revert the changes in this file?

probably not. do any of them fail? it increases test coverage, right?

probably not. do any of them fail? it increases test coverage, right?

no, they pass, and probably increases test coverage. I will leave this file as it is then.

…blockhash

tserg added 3 commits November 24, 2022 16:20

add blockhash to local validation

90747ef

add test

393dfed

fix mypy lint

3d303c7

tserg marked this pull request as ready for review November 24, 2022 09:44

fubuloubu approved these changes Nov 24, 2022

View reviewed changes

charles-cooper requested changes Nov 26, 2022

View reviewed changes

use StateMutability

439bb78

charles-cooper reviewed Nov 27, 2022

View reviewed changes

tserg added 4 commits November 27, 2022 14:36

fix lint

5fadb3b

Merge branch 'master' of https://github.com/vyperlang/vyper into fix/…

d976d3f

…blockhash

fix merge conflict

aa82067

fix import

cccd707

charles-cooper added this to the v0.3.8 milestone May 14, 2023

tserg added 3 commits May 18, 2023 13:15

Merge branch 'master' of https://github.com/vyperlang/vyper into fix/…

6f193e4

…blockhash

clean up imports

a0e80ad

undo blank line

b3e4b62

github-advanced-security bot found potential problems May 18, 2023

View reviewed changes

vyper/builtins/_signatures.py Fixed Show fixed Hide fixed

vyper/builtins/functions.py Fixed Show fixed Hide fixed

vyper/semantics/analysis/local.py Fixed Show fixed Hide fixed

vyper/semantics/analysis/local.py Fixed Show fixed Hide fixed

tserg added 5 commits May 18, 2023 13:38

filter for nonpure builtins from namespace

79e1674

fetch all call nodes

f6f2e91

clean up

a5b1924

undo blank line

b754a1b

add interface test

eff8438

fix lint

035a91b

charles-cooper reviewed May 18, 2023

View reviewed changes

tserg added 2 commits February 21, 2024 18:22

Merge branch 'master' of https://github.com/vyperlang/vyper into fix/…

9410901

…blockhash

revert merge conflict

f6c95b2

tserg added 2 commits March 8, 2024 18:04

Merge branch 'master' of https://github.com/vyperlang/vyper into fix/…

7ae3018

…blockhash

fix test

c3f2bf6

charles-cooper reviewed Mar 18, 2024

View reviewed changes

tests/functional/codegen/features/decorators/test_pure.py Outdated Show resolved Hide resolved

charles-cooper reviewed Mar 18, 2024

View reviewed changes

tests/functional/codegen/features/decorators/test_pure.py Outdated Show resolved Hide resolved

tserg added 2 commits March 19, 2024 23:23

modify tests

3931761

add default value

1895549

charles-cooper reviewed Mar 25, 2024

View reviewed changes

vyper/builtins/functions.py Outdated Show resolved Hide resolved

charles-cooper reviewed Mar 25, 2024

View reviewed changes

vyper/builtins/functions.py Outdated Show resolved Hide resolved

charles-cooper reviewed Mar 25, 2024

View reviewed changes

charles-cooper changed the title ~~fix: disallow blockhash in pure functions~~ fix[lang]: disallow blockhash in pure functions Mar 25, 2024

Merge branch 'master' into fix/blockhash

9e52ecc

tserg added 4 commits March 26, 2024 15:55

apply bts suggestion

dad2c46

revert raw call changes

ebfe003

fix lint

4709936

Merge branch 'fix/blockhash' of https://github.com/tserg/vyper into f…

e2e9864

…ix/blockhash

charles-cooper reviewed Mar 26, 2024

View reviewed changes

vyper/semantics/analysis/local.py Show resolved Hide resolved

tserg added 2 commits March 27, 2024 17:04

Merge branch 'master' of https://github.com/vyperlang/vyper into fix/…

b1517b7

…blockhash

Merge branch 'master' of https://github.com/vyperlang/vyper into fix/…

d9e32c7

…blockhash

github-advanced-security bot found potential problems Oct 11, 2024

View reviewed changes

tserg commented Oct 11, 2024

View reviewed changes

tserg added 2 commits October 11, 2024 22:04

add blobhash

8610908

Merge branch 'master' of https://github.com/vyperlang/vyper into fix/…

36d6ddb

…blockhash

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix[lang]: disallow `blockhash` in `pure` functions #3157

fix[lang]: disallow `blockhash` in `pure` functions #3157

tserg commented Nov 24, 2022

tserg commented Nov 24, 2022

charles-cooper left a comment

codecov-commenter commented Nov 27, 2022 •

edited

Loading

charles-cooper Nov 27, 2022

tserg Nov 27, 2022

charles-cooper Nov 27, 2022

tserg commented May 18, 2023

charles-cooper May 18, 2023

charles-cooper commented Feb 21, 2024

charles-cooper Mar 18, 2024

charles-cooper Mar 18, 2024

tserg Mar 19, 2024

charles-cooper Mar 25, 2024

charles-cooper Mar 25, 2024

charles-cooper Mar 25, 2024

charles-cooper Mar 25, 2024

charles-cooper Mar 25, 2024

charles-cooper commented Mar 25, 2024

cyberthirst commented Oct 11, 2024

charles-cooper commented Oct 11, 2024

tserg Oct 11, 2024

charles-cooper Oct 11, 2024

tserg Oct 12, 2024

		@@ -1061,6 +1061,25 @@ def fetch_call_return(self, node):

		kwargz = {i.arg: i.value for i in node.keywords}

		delegate_call = kwargz.get("is_delegate_call")

	@pytest.mark.parametrize("state_modifying_decorator", ("payable", "nonpayable"))
	@pytest.mark.parametrize("modifying", ("payable", "nonpayable"))

	@pytest.mark.parametrize("non_modifying_decorator", ("pure", "view"))
	@pytest.mark.parametrize("constant", ("pure", "view"))

		@@ -1061,6 +1061,26 @@ def fetch_call_return(self, node):

		kwargz = {i.arg: i.value for i in node.keywords}

fix[lang]: disallow blockhash in pure functions #3157

Are you sure you want to change the base?

fix[lang]: disallow blockhash in pure functions #3157

Conversation

tserg commented Nov 24, 2022

What I did

How I did it

How to verify it

Commit message

Description for the changelog

Cute Animal Picture

tserg commented Nov 24, 2022

charles-cooper left a comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 27, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tserg commented May 18, 2023

Choose a reason for hiding this comment

charles-cooper commented Feb 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charles-cooper commented Mar 25, 2024

cyberthirst commented Oct 11, 2024

charles-cooper commented Oct 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fix[lang]: disallow `blockhash` in `pure` functions #3157

fix[lang]: disallow `blockhash` in `pure` functions #3157

codecov-commenter commented Nov 27, 2022 •

edited

Loading