fix[lang]: stateless modules should not be initialized #4347

sandbubbles · 2024-11-04T11:33:04Z

What I did

Forbid initializes on stateless modules as defined in #4050.

How I did it

Define modules with storage variables, transient variables or with uses and initializes as stateful.
When using initializes on stateless module, raise exception.

How to verify it

Commit message

Commit message for the final, squashed PR. (Optional, but reviewers will appreciate it! Please see our commit message style guide for what we would ideally like to see in a commit message.)

Description for the changelog

Cute Animal Picture

charles-cooper · 2024-11-08T12:59:43Z

vyper/semantics/analysis/utils.py

+def is_stateless(module: vy_ast.Module):
+    """
+    Determine whether a module is stateless by examining its top-level declarations.
+    A module has state if it contains storage variables, transient variables, or
+    immutables, or if it includes a "uses" or "initializes" declaration.
+    """
+    for i in module.body:
+        if isinstance(i, (vy_ast.InitializesDecl, vy_ast.UsesDecl)):
+            return False
+        if isinstance(i, vy_ast.VariableDecl) and not i.is_constant:
+            return False
+        if isinstance(i, vy_ast.FunctionDef) and i.name == "__init__":
+            return False
+    return True


i think this belongs better on ModuleT

Okay, moved it.

charles-cooper · 2024-11-08T13:00:05Z

vyper/semantics/analysis/module.py

@@ -409,6 +410,10 @@ def visit_InitializesDecl(self, node):
        module_info = get_expr_info(module_ref).module_info
        if module_info is None:
            raise StructureException("Not a module!", module_ref)
+        if is_stateless(module_info.module_node):


maybe should we also have this restriction on UsesDecl -- @cyberthirst wdyt?

sorry, missed the ping

i think that makes sense - uses indicates that we're accessing the used module's state - and thus the module should actually have state

charles-cooper

this lgtm. paging @cyberthirst to take a look as well

cyberthirst · 2024-12-03T15:27:08Z

tests/functional/codegen/features/test_transient.py

@@ -570,3 +570,15 @@ def foo() -> (uint256[3], uint256, uint256, uint256):

    c = get_contract(main, input_bundle=input_bundle)
    assert c.foo() == ([1, 2, 3], 1, 2, 42)
+
+
+def test_transient_is_state(make_input_bundle):


let's please add a comment here that we're testing the initialization and how it relates to state

cyberthirst · 2024-12-03T15:31:09Z

tests/functional/codegen/modules/test_nonreentrant.py

@@ -1,5 +1,7 @@
 def test_export_nonreentrant(make_input_bundle, get_contract, tx_failed):
    lib1 = """
+phony: uint32


this feels off, see my comment at is_stateless

cyberthirst · 2024-12-03T15:36:02Z

vyper/semantics/types/module.py

@@ -559,3 +559,19 @@ def immutable_section_bytes(self):
    @cached_property
    def interface(self):
        return InterfaceT.from_ModuleT(self)
+
+    @cached_property
+    def is_stateless(self):


i think this definition omits reentrancy locks - for cancun those are created inside transient which we define as state. the locks aren't implemented as variables so this check would miss them

to further support the statement above, consider following:

#main.vy import lib1 #initializes: lib1 exports: lib1.foo

#lib1.vy @external @nonreentrant def foo() -> uint256: return 5

which yields:

vyper.exceptions.ImmutableViolation: Cannot access `lib1` state! note that use of the `@nonreentrant` decorator is also considered state access contract "tests/custom/test.vy:5", line 5:9 4 ---> 5 exports: lib1.foo ----------------^ 6 (hint: add `uses: lib1` or `initializes: lib1` as a top-level statement to your contract)

vyper/semantics/types/module.py

cyberthirst · 2024-12-03T15:55:08Z

vyper/semantics/types/module.py

+        transient variables, or immutables, or if it includes a "uses" or
+        "initializes" declaration.
+        """
+        if len(self.initializes_decls) > 0 or len(self.uses_decls) > 0:


should we really check for uses_decl? uses refer to another's module state. or maybe i'm just debating the definition of "statlessness" - if it's about not accessing any state (in the sense of purity) then this code's ok

codecov · 2024-12-07T09:15:00Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.91%. Comparing base (194d60a) to head (ef0c49d).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4347      +/-   ##
==========================================
- Coverage   91.86%   90.91%   -0.95%     
==========================================
  Files         119      119              
  Lines       16640    16654      +14     
  Branches     2801     2807       +6     
==========================================
- Hits        15286    15141     -145     
- Misses        929     1057     +128     
- Partials      425      456      +31

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

sandbubbles · 2024-12-11T09:42:10Z

This should be good for another review.

charles-cooper · 2024-12-11T23:42:21Z

vyper/semantics/types/module.py

@@ -559,3 +559,24 @@ def immutable_section_bytes(self):
    @cached_property
    def interface(self):
        return InterfaceT.from_ModuleT(self)
+
+    @cached_property
+    def is_not_initializable(self):


maybe we can flip the logic and rename to is_initializable ?

charles-cooper · 2024-12-12T14:31:37Z

vyper/semantics/types/module.py

+        for fun in self.functions.values():
+            if fun.nonreentrant:


convention: ContractFunctionT is usually named fn_t in loops (or other places with small scope)

Suggested change

for fun in self.functions.values():

if fun.nonreentrant:

for fn_t in self.functions.values():

if fn_t.nonreentrant:

actually maybe this should be a loop over exposed functions (per @cyberthirst )?

I think that would skip internal functions with nonreentrancy locks - is that intentional?

no, not intentional

hmm, maybe the exposed_functions thing is a red herring -- we don't care about exposed_functions per se, we care about state in the recursion, but that is handled by checking if there are any initializes decls.

yup, i think as you said - we dont need to recurse in the function as the state of the imported modules is already handled. So the self.functions.values() is okay?

yes i think so, unless @cyberthirst thinks otherwise.

charles-cooper · 2024-12-12T15:01:42Z

tests/unit/compiler/test_abi.py

@@ -361,6 +363,8 @@ def __init__():
 def test_event_export_from_function_export(make_input_bundle):
    # test events used in exported functions are exported
    lib1 = """
+phony: uint32


maybe we shouldn't add phony here -- the code change is telling us to remove initializes: lib1

sandbubbles · 2024-12-30T11:38:17Z

I've marked the uses declarations as initializable - but as discussed offline - we might have false negatives and allow compilation of some modules we didn't intend.

In the tests, I removed the initializes and phony variables, since none of the tests required initializes.

sandbubbles added 11 commits November 3, 2024 11:45

forbid initialization of a stateless module

1b1ac45

fix tests so they do not initialize a stateless module

3a9e29d

lint

5527244

set immutables as stateless

3c695da

add tests

5000efc

fix stateless test

e3d7582

make immutables state

c92e1ad

fix test

cf91784

test transient in cancun

1ab6433

make init function as state

28c9be7

remove one wrong test

0c5f645

sandbubbles assigned charles-cooper Nov 7, 2024

charles-cooper reviewed Nov 8, 2024

View reviewed changes

refactor is_stateless into ModuleT method

47efc8e

sandbubbles marked this pull request as ready for review November 9, 2024 10:19

charles-cooper added 2 commits November 20, 2024 10:21

simplify is_stateless

5fe6c84

promote some properties to cached properties

fececb7

charles-cooper approved these changes Nov 20, 2024

View reviewed changes

charles-cooper requested a review from cyberthirst November 20, 2024 16:51

cyberthirst reviewed Dec 3, 2024

View reviewed changes

vyper/semantics/types/module.py Show resolved Hide resolved

cyberthirst reviewed Dec 3, 2024

View reviewed changes

sandbubbles added 5 commits December 5, 2024 10:41

add comment to explain a test

4cc01bb

rename function to is_initializable

ba19783

remove redundant storage variables

b668f41

make modules with nonreentrant initializable

6c7a30f

test that nonreentrant function use state

948d047

sandbubbles added 4 commits December 5, 2024 11:31

update comment

3dbbeb6

remove uses decl from initializable

1f4a2d6

rename the function so the boolean matches

be5aa0b

refactor comment

7b2f38b

sandbubbles requested a review from charles-cooper December 11, 2024 08:42

cyberthirst added the release - must release blocker label Dec 11, 2024

charles-cooper reviewed Dec 11, 2024

View reviewed changes

change is_not_initializable to is_initializable

bb4944f

sandbubbles requested a review from charles-cooper December 12, 2024 14:00

charles-cooper reviewed Dec 12, 2024

View reviewed changes

sandbubbles added 3 commits December 15, 2024 11:45

rename variable

b41bc28

remove unnecessary phonies and initializes

2bcba89

clean up tests

7f2f452

charles-cooper removed the release - must release blocker label Dec 17, 2024

charles-cooper added this to the v0.4.1 milestone Dec 17, 2024

sandbubbles added 5 commits December 30, 2024 11:08

remove phony variables to fix tests with "uses"

f64109f

modify test to expect a successful compilation

bf7307c

mark uses declaration initializable

a075979

Merge branch 'master' into fix/stateless-modules-can-be-initialized

b67ab3f

add comments to tests

ef0c49d

sandbubbles requested review from charles-cooper and cyberthirst December 30, 2024 11:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix[lang]: stateless modules should not be initialized #4347

fix[lang]: stateless modules should not be initialized #4347

sandbubbles commented Nov 4, 2024

charles-cooper Nov 8, 2024

sandbubbles Nov 9, 2024

charles-cooper Nov 8, 2024

cyberthirst Dec 3, 2024

charles-cooper left a comment

cyberthirst Dec 3, 2024

cyberthirst Dec 3, 2024

cyberthirst Dec 3, 2024

cyberthirst Dec 3, 2024

codecov bot commented Dec 7, 2024 •

edited

Loading

sandbubbles commented Dec 11, 2024

charles-cooper Dec 11, 2024

charles-cooper Dec 12, 2024

charles-cooper Dec 12, 2024

charles-cooper Dec 12, 2024 •

edited

Loading

sandbubbles Dec 12, 2024

charles-cooper Dec 12, 2024

charles-cooper Dec 12, 2024

sandbubbles Dec 12, 2024

charles-cooper Dec 12, 2024

charles-cooper Dec 12, 2024

sandbubbles commented Dec 30, 2024

fix[lang]: stateless modules should not be initialized #4347

Are you sure you want to change the base?

fix[lang]: stateless modules should not be initialized #4347

Conversation

sandbubbles commented Nov 4, 2024

What I did

How I did it

How to verify it

Commit message

Description for the changelog

Cute Animal Picture

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 left a 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

codecov bot commented Dec 7, 2024 • edited Loading

Codecov Report

sandbubbles commented Dec 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charles-cooper Dec 12, 2024 • edited Loading

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

sandbubbles commented Dec 30, 2024

codecov bot commented Dec 7, 2024 •

edited

Loading

charles-cooper Dec 12, 2024 •

edited

Loading