fix[lang]: disallow some builtins in `pure` functions by tserg · Pull Request #3157 · vyperlang/vyper

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

What I did

How I did it

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

How to verify it

See test.

Commit message

this commit disallows some builtins from being called in `pure`
functions, specifically, `blockhash()` and `blobhash()`. it
accomplishes this by adding a `mutability` property to builtins, and a
case to `FunctionAnalyzer` to check the mutability property.

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 92.63%. Comparing base (1591a12) to head (0c0cdc1).
Report is 80 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3157   +/-   ##
=======================================
  Coverage   92.63%   92.63%           
=======================================
  Files         122      122           
  Lines       17518    17523    +5     
  Branches     2976     2977    +1     
=======================================
+ Hits        16228    16233    +5     
  Misses        892      892           
  Partials      398      398

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

vyper/builtins/_signatures.py

 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

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

charles-cooper

this lgtm. requesting a look from @cyberthirst

cyberthirst · 2025-03-19T12:27:25Z

I don't love the design, like we default to pure and then let this check self._check_call_mutability(func_type.mutability) trivially pass for some cases

I think that's pretty confusing when reading the code.

With Charles we discussed it would be better to compute this information at callsite as the arguments can affect the final mutability for the given node .. and then use it to check the mutability (so it wouldn't just trivially pass)

tserg · 2025-03-20T14:05:24Z

With Charles we discussed it would be better to compute this information at callsite as the arguments can affect the final mutability for the given node .. and then use it to check the mutability (so it wouldn't just trivially pass)

Does that mean this check should be specific to each builtin function, similar to how we check the mutability against the contract function's context for raw_call in build_IR?

cyberthirst · 2025-03-24T09:13:36Z

should we also fix this #3093 in the context of this PR?

cyberthirst · 2025-03-24T09:26:08Z

With Charles we discussed it would be better to compute this information at callsite as the arguments can affect the final mutability for the given node .. and then use it to check the mutability (so it wouldn't just trivially pass)

Does that mean this check should be specific to each builtin function, similar to how we check the mutability against the contract function's context for raw_call in build_IR?

The original thought was that the mutability would be callsite specific based on the given arguments (like raw_call can be static and delegate on different callsites).

But @charles-cooper suggested it might be unnecessary to pass the arguments to each function for evaluation because most of them don't rely on them, the mutability is statically set. So maybe we'd be ok with a raw_call specific check on callsite - unless you can think of other cases.

…blockhash

tserg · 2025-03-25T08:18:05Z

With Charles we discussed it would be better to compute this information at callsite as the arguments can affect the final mutability for the given node .. and then use it to check the mutability (so it wouldn't just trivially pass)

Does that mean this check should be specific to each builtin function, similar to how we check the mutability against the contract function's context for raw_call in build_IR?

The original thought was that the mutability would be callsite specific based on the given arguments (like raw_call can be static and delegate on different callsites).

But @charles-cooper suggested it might be unnecessary to pass the arguments to each function for evaluation because most of them don't rely on them, the mutability is statically set. So maybe we'd be ok with a raw_call specific check on callsite - unless you can think of other cases.

Yea raw_call looks like the odd one out where mutability depends on the arguments at callsite. Does that mean I should leave this PR as it is? If so, #3093 looks specific to raw_call so maybe it should be tackled in a separate PR?

charles-cooper · 2025-04-04T16:52:40Z

Yea raw_call looks like the odd one out where mutability depends on the arguments at callsite. Does that mean I should leave this PR as it is? If so, #3093 looks specific to raw_call so maybe it should be tackled in a separate PR?

yea, let's deal with the call-site analysis once we tackle raw_call

cyberthirst · 2025-04-07T13:21:28Z

@charles-cooper we have a couple of builtins that rely on context.check_is_not_constant - maybe we should reuse this machinery for these cases too? i think it's better to have these checks in the frontend

cyberthirst · 2025-04-07T16:05:00Z

vyper/builtins/_signatures.py

    _return_type: Optional[VyperType] = None
    _equality_attrs = ("_id",)
    _is_terminus = False
+    mutability: StateMutability = StateMutability.PURE


i think i'd like to see mutable default better .. it's a tradeoff .. we make a mistake and allow mutable function to be called in non-mutable context - or - we have the opposite default and risk some contracts not compiling

vyper/builtins/functions.py

    ZeroDivisionException,
 )
-from vyper.semantics.analysis.base import Modifiability
+from vyper.semantics.analysis.base import Modifiability, StateMutability


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

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 and others added 3 commits October 11, 2024 22:04

add blobhash

8610908

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

36d6ddb

…blockhash

Merge branch 'master' into fix/blockhash

115f9c4

charles-cooper requested a review from cyberthirst December 30, 2024 01:52

charles-cooper approved these changes Dec 30, 2024

View reviewed changes

cyberthirst modified the milestones: v0.3.8, v0.4.2 Mar 10, 2025

charles-cooper added the release - must release blocker label Mar 17, 2025

Merge branch 'master' into fix/blockhash

0bc74bd

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

c7b7378

…blockhash

cyberthirst reviewed Apr 7, 2025

View reviewed changes

cyberthirst approved these changes Apr 7, 2025

View reviewed changes

Merge branch 'master' into fix/blockhash

0c0cdc1

github-advanced-security bot found potential problems Apr 13, 2025

View reviewed changes

charles-cooper changed the title ~~fix[lang]: disallow blockhash in pure functions~~ fix[lang]: disallow blockhash() in pure functions Apr 14, 2025

charles-cooper changed the title ~~fix[lang]: disallow blockhash() in pure functions~~ fix[lang]: disallow some builtins in pure functions Apr 14, 2025

charles-cooper merged commit 79001f3 into vyperlang:master Apr 14, 2025
163 checks passed

cyberthirst mentioned this pull request Apr 25, 2025

refactor mutability checks for builtins #4602

Open

Uh oh!

Conversation

tserg commented Nov 24, 2022 • edited by charles-cooper Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What I did

How I did it

How to verify it

Commit message

Description for the changelog

Cute Animal Picture

Uh oh!

tserg commented Nov 24, 2022

Uh oh!

charles-cooper left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Nov 27, 2022 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tserg commented May 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charles-cooper commented Feb 21, 2024

Uh oh!

Check notice

Uh oh!

Uh oh!

cyberthirst commented Oct 11, 2024

Uh oh!

charles-cooper commented Oct 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charles-cooper left a comment

Choose a reason for hiding this comment

Uh oh!

cyberthirst commented Mar 19, 2025

Uh oh!

tserg commented Mar 20, 2025

Uh oh!

cyberthirst commented Mar 24, 2025

Uh oh!

cyberthirst commented Mar 24, 2025

Uh oh!

tserg commented Mar 25, 2025

Uh oh!

charles-cooper commented Apr 4, 2025

Uh oh!

cyberthirst commented Apr 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Check notice

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tserg commented Nov 24, 2022 •

edited by charles-cooper

Loading

codecov-commenter commented Nov 27, 2022 •

edited by codecov bot

Loading