feat[lang]: nonreentrancy by default by charles-cooper · Pull Request #4563 · vyperlang/vyper

charles-cooper · 2025-04-06T14:48:23Z

What I did

How I did it

How to verify it

Commit message

to control the default nonreentrancy behavior on a module-by-module
basis. when the pragma is set to `on`, all the external functions and
getters in the file will be nonreentrant (excluding pure functions,
which can never be nonreentrant).

this is a step towards making nonreentrancy the default for the
language while providing a backwards-compatible upgrade path for users
in the 0.4.x series. in the future (probably starting in 0.5.0), the
default will be `pragma nonreentrancy on`, which will have to be opted
out of with the pragma.

Description for the changelog

Cute Animal Picture

codecov · 2025-04-06T15:12:03Z

Codecov Report

❌ Patch coverage is 91.37931% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.49%. Comparing base (756ff72) to head (15e51c8).
⚠️ Report is 61 commits behind head on master.

Files with missing lines	Patch %	Lines
vyper/ast/pre_parser.py	83.78%	5 Missing and 1 partial ⚠️
vyper/ast/nodes.py	77.77%	1 Missing and 1 partial ⚠️
vyper/compiler/settings.py	33.33%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4563      +/-   ##
==========================================
- Coverage   92.50%   92.49%   -0.01%     
==========================================
  Files         128      128              
  Lines       18432    18506      +74     
  Branches     3191     3208      +17     
==========================================
+ Hits        17050    17118      +68     
- Misses        941      944       +3     
- Partials      441      444       +3

☔ 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.

cyberthirst · 2025-04-07T16:58:55Z

as discussed offline, we should also process the pragma for imports

cyberthirst · 2025-04-07T17:02:54Z

this fails, imo we should only restrict the pragma to external functions

def test_nonreentrant_decorator(get_contract):
    code = """
# pragma nonreentrancy on

def foo():
    u: uint256 = 1

@external
def bar():
    self.foo()
    """
    c = get_contract(code)

    c.bar()

cyberthirst · 2025-04-08T06:32:37Z

# pragma nonreentrancy on

@external
@nonreentrant
def __default__():
    pass

vyper.exceptions.StructureException: used @nonreentrant decorator, but `#pragma nonreentrancy` is set

  contract "tests/custom/test.vy:5", line 5:0 
       4 @nonreentrant
  ---> 5 def __default__():
  -------^
       6     pass

which is problematic because __default__ is handled as reentrant by default

vyper/semantics/types/function.py

remove `parse_to_ast_with_settings`, and replace it with directly putting the settings parsed from the pragmas on the AST. this allows us to respect pragmas on a file-by-file basis if needed.

it's duplicated with vyper/ast/utils.py

…fault

cyberthirst · 2025-04-25T07:30:41Z

per Anatomist:

Invalid variable annotations such as `public(public(TYPE))` are accepted by the compiler due to missing duplication checks for the first two variable traits.

cyberthirst · 2025-04-25T07:40:41Z

Per Anatomist:

Additionally, the grammar definition in the experimental parser is less restrictive than the actual implementation. For example, immutable(transient(TYPE)) is allowed in the experimental parser but should not be allowed according to the actual implementation.

I don't think this is a huge deal but if we can bake semantics into the grammar while not sacrificing simplicity, we should do it.

nonreentrancy fixes

cyberthirst · 2025-04-25T09:09:07Z

Let's make sure the docs are up-to-date with the latest changes.

Including a comment from Anatomist. Didn't yet check whether it's relevant for the latest commit:

We recommend a few changes to the documentation:
• Since __default__ is no longer treated as a special case, the documentation should be updated accordingly.
• To clarify the purpose of the nonreentrant pragma, the documentation should specify that it applies only to non-pure external functions.
• Documentation for the reentrant modifier applied to global variables should be added.

cyberthirst · 2025-04-25T09:30:13Z

Per Anatomist:
Additionally, the grammar definition in the experimental parser is less restrictive than the actual implementation. For example, immutable(transient(TYPE)) is allowed in the experimental parser but should not be allowed according to the actual implementation.
I don't think this is a huge deal but if we can bake semantics into the grammar while not sacrificing simplicity, we should do it.

@trocher further mentions:

In grammar.lark, the grammar for constants is defined as:

constant: "constant" "(" type ")"
constant_private: ChainSecurity ":" constant
constant_with_getter: ChainSecurity ":" "public" "(" constant ")"
constant_def: (constant_private | constant_with_getter) "=" expr

However, this does not account for the reentrant decorator, which is a valid decorator for constant variables, since the below code is valid Vyper code, but fails to be parsed using the lark grammar:

a: public(reentrant(constant(uint256))) = 1

charles-cooper · 2025-04-25T09:50:28Z

Per Anatomist:
Additionally, the grammar definition in the experimental parser is less restrictive than the actual implementation. For example, immutable(transient(TYPE)) is allowed in the experimental parser but should not be allowed according to the actual implementation.
I don't think this is a huge deal but if we can bake semantics into the grammar while not sacrificing simplicity, we should do it.

i think it's fine to allow these kinds of things in the grammar, immutable(transient(TYPE)) is arguably a semantic error, not a syntactical error

cyberthirst · 2025-04-25T10:49:37Z

Per @trocher

In the case no nonreentrant functions are present in the contract, but only public state variables with the pragma nonreentrancy on, the _generate_layout_export_r() function will not output the slot for the key, although it is allocated with _allocate_layout_r().

The following contracts yield the given layout when running vyper -f layout file.vy, although the transient storage slot 0 is allocated and used by the public getter function for the a variable:

# pragma nonreentrancy on

a: public(constant(uint256)) = 1

b: transient(uint256)

{"transient_storage_layout": {"b": {"type": "uint256", "n_slots": 1, "slot": 1}}}

cyberthirst · 2025-04-25T10:54:09Z

Per @trocher

In the case no nonreentrant functions are present in the contract, but only public state variables with the pragma nonreentrancy on, the _generate_layout_export_r() function will not output the slot for the key, although it is allocated with _allocate_layout_r().

The following contracts yield the given layout when running vyper -f layout file.vy, although the transient storage slot 0 is allocated and used by the public getter function for the a variable:
# pragma nonreentrancy on

a: public(constant(uint256)) = 1

b: transient(uint256)
{"transient_storage_layout": {"b": {"type": "uint256", "n_slots": 1, "slot": 1}}}

This has further impact down the line because we test for round-trip when using overrides. Because of the missing key, contracts will stop compiling:

# pragma evm-version shanghai

a: public(uint256)

{"a": {"type": "uint256", "n_slots": 1, "slot": 0}, "$.nonreentrant_key":  {"type": "nonreentrant lock", "n_slots": 1, "slot": 20}}

vyper.exceptions.CompilerPanic: Computed storage layout does not match override file!
expected: {"a": {"type": "uint256", "n_slots": 1, "slot": 0}, "$.nonreentrant_key": {"type": "nonreentrant lock", "n_slots": 1, "slot": 20}}

got:
{"a": {"type": "uint256", "n_slots": 1, "slot": 0}}

This is an unhandled internal compiler error. Please create an issue on Github to notify the developers!
https://github.com/vyperlang/vyper/issues/new?template=bug.md

note that i used shanghai because for roundtrip we use storage

cyberthirst · 2025-04-26T08:28:00Z

per Anatomist:

Invalid variable annotations such as `public(public(TYPE))` are accepted by the compiler due to missing duplication checks for the first two variable traits.

lifted this into a separate issue: #4610

cyberthirst · 2025-04-26T08:44:54Z

@trocher further mentions:

In grammar.lark, the grammar for constants is defined as:
constant: "constant" "(" type ")"
constant_private: ChainSecurity ":" constant
constant_with_getter: ChainSecurity ":" "public" "(" constant ")"
constant_def: (constant_private | constant_with_getter) "=" expr
However, this does not account for the reentrant decorator, which is a valid decorator for constant variables, since the below code is valid Vyper code, but fails to be parsed using the lark grammar:
a: public(reentrant(constant(uint256))) = 1

lifted this to: #4611

cyberthirst · 2025-04-26T08:47:25Z

Per @trocher

In the case no nonreentrant functions are present in the contract, but only public state variables with the pragma nonreentrancy on, the _generate_layout_export_r() function will not output the slot for the key, although it is allocated with _allocate_layout_r().

The following contracts yield the given layout when running vyper -f layout file.vy, although the transient storage slot 0 is allocated and used by the public getter function for the a variable:
# pragma nonreentrancy on

a: public(constant(uint256)) = 1

b: transient(uint256)
{"transient_storage_layout": {"b": {"type": "uint256", "n_slots": 1, "slot": 1}}}

lifted this up to #4612

cyberthirst · 2025-04-28T11:12:18Z

docs/control-structures.rst

+The nonreentrant pragma
+-----------------------
+
+Beginning in 0.4.2, the ``#pragma nonreentrancy on`` pragma is available, and it enables nonreentrancy on all functions and public getters in the file. This is to prepare for a future release, probably in the 0.5.x series, where nonreentrant locks will be enabled by default language-wide.


on all external functions

we should document how it behaves for:

internal functions

pure external functions

view external functions

cyberthirst · 2025-04-28T11:17:09Z

docs/control-structures.rst

    # pragma nonreentrancy on

+    x: public(uint256)  # this has view-only reentrancy turned on
+    y: public(reentrant(uint256))  # this does not have view-only reentrancy


does not have?

cyberthirst · 2025-04-28T11:18:09Z

docs/control-structures.rst

    def __default__():
-        # the default function is reentrant by default, even when
-        # the nonreentrancy pragma is set
+        # this function is nonreentrant!


for make_a_call the comment says # this function is protected from re-entrancy, the inconsistency makes it a bit hard to read

cyberthirst · 2025-04-28T11:20:11Z

docs/control-structures.rst

 .. code-block:: vyper
    # pragma nonreentrancy on

+    x: public(uint256)  # this has view-only reentrancy turned on


getters are by default nonreentrant with the pragma on

…eentrancy-by-default improve nonreentrant pragma docs

charles-cooper added 3 commits April 6, 2025 15:57

wip nonreentrancy pragma

8f2cb41

update function.py

6af4284

lint

3874c05

charles-cooper added 5 commits April 7, 2025 15:13

limit the on/off keywords

6fbce15

handle default for fallback function

07142be

wip tests

0525826

fix tests

15b8726

fix lint

6665abf

cyberthirst reviewed Apr 8, 2025

View reviewed changes

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

charles-cooper added 10 commits April 10, 2025 12:09

refactor[parser]: put settings on Module AST node

3239722

remove `parse_to_ast_with_settings`, and replace it with directly putting the settings parsed from the pragmas on the AST. this allows us to respect pragmas on a file-by-file basis if needed.

remove dead code

266b1dc

it's duplicated with vyper/ast/utils.py

add settings to ast export

428abc7

add a test for file-specific settings

8969c67

Merge branch 'master' into refactor/module-settings

32f1b74

add a comment

1c6ef3f

Merge branch 'refactor/module-settings' into feat/nonreentrancy-by-de…

4b4f31d

…fault

scope nonreentrancy pragma to current file

a9a6837

refactor to have separate reentrant node

34917a9

add docs for nonreentrant pragma

655ec47

charles-cooper force-pushed the feat/nonreentrancy-by-default branch from 6516d49 to 655ec47 Compare April 11, 2025 06:50

cyberthirst and others added 6 commits April 11, 2025 09:16

Merge branch 'master' into feat/nonreentrancy-by-default

1f60586

add check for default function

d515ff5

test for nonreentrant internal functions

f9b1d3d

fix branches

e17e677

make reentrant the default on internal functions

80e1910

add nonreentrant pragma test for vyi

14edaaf

Merge pull request #79 from charles-cooper/nonreentrancy-pragma-fixes

feccaca

nonreentrancy fixes

cyberthirst mentioned this pull request Apr 26, 2025

state variables can have duplicate annotation #4610

Closed

cyberthirst mentioned this pull request Apr 26, 2025

grammar for annotations of constant state variables is too restrictive #4611

Open

cyberthirst mentioned this pull request Apr 26, 2025

layout export with -f layout doesn't include the reentrancy lock #4612

Closed

charles-cooper added 3 commits April 26, 2025 11:55

lint

fbabb37

change exception type, fix tests

bf7e859

update docs for default function and getters

8442a54

cyberthirst reviewed Apr 28, 2025

View reviewed changes

cyberthirst and others added 3 commits April 28, 2025 13:51

improve nonreentrant pragma docs

791bd67

Merge pull request #83 from cyberthirst/fork/charles-cooper/feat/nonr…

ef5b92c

…eentrancy-by-default improve nonreentrant pragma docs

clarifications for docs

98bda79

cyberthirst approved these changes Apr 28, 2025

View reviewed changes

charles-cooper added 2 commits April 28, 2025 14:53

Merge branch 'master' into feat/nonreentrancy-by-default

5ff86d2

Merge branch 'master' into feat/nonreentrancy-by-default

15e51c8

charles-cooper enabled auto-merge (squash) April 28, 2025 13:21

charles-cooper merged commit 59baa1b into vyperlang:master Apr 28, 2025
159 checks passed

cyberthirst mentioned this pull request Aug 1, 2025

VIP: make external entrypoints nonreentrant by default #4509

Closed

Uh oh!

Conversation

charles-cooper commented Apr 6, 2025 • edited 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!

codecov bot commented Apr 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cyberthirst commented Apr 7, 2025

Uh oh!

cyberthirst commented Apr 7, 2025

Uh oh!

cyberthirst commented Apr 8, 2025

Uh oh!

Uh oh!

cyberthirst commented Apr 25, 2025

Uh oh!

cyberthirst commented Apr 25, 2025

Uh oh!

cyberthirst commented Apr 25, 2025

Uh oh!

cyberthirst commented Apr 25, 2025

Uh oh!

charles-cooper commented Apr 25, 2025

Uh oh!

cyberthirst commented Apr 25, 2025

Uh oh!

cyberthirst commented Apr 25, 2025

Uh oh!

cyberthirst commented Apr 26, 2025

Uh oh!

cyberthirst commented Apr 26, 2025

Uh oh!

cyberthirst commented Apr 26, 2025

Uh oh!

cyberthirst Apr 28, 2025

Choose a reason for hiding this comment

Uh oh!

cyberthirst Apr 28, 2025

Choose a reason for hiding this comment

Uh oh!

cyberthirst Apr 28, 2025

Choose a reason for hiding this comment

Uh oh!

cyberthirst Apr 28, 2025

Choose a reason for hiding this comment

Uh oh!

cyberthirst Apr 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

charles-cooper commented Apr 6, 2025 •

edited

Loading

codecov bot commented Apr 6, 2025 •

edited

Loading