From 036b20bc086e310eb711aef3f7e6b1afa22628dd Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Wed, 31 May 2023 10:56:54 -0400 Subject: [PATCH 1/4] fix: metadata journal can rollback incorrectly call register_update when node has already been updated in this "frame" note this does not change any observed functionality in the compiler because writes to the metadata journal inside for loops only ever happen to be written once, but it prevents a bug in case we ever add mutating writes inside of the same metadata journal frame. chainsec june review 5.3 --- vyper/ast/metadata.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/vyper/ast/metadata.py b/vyper/ast/metadata.py index 30e06e0016..0a419c3732 100644 --- a/vyper/ast/metadata.py +++ b/vyper/ast/metadata.py @@ -17,8 +17,11 @@ def __init__(self): self._node_updates: list[dict[tuple[int, str, Any], NodeMetadata]] = [] def register_update(self, metadata, k): + KEY = (id(metadata), k) + if KEY in self._node_updates[-1]: + return prev = metadata.get(k, self._NOT_FOUND) - self._node_updates[-1][(id(metadata), k)] = (metadata, prev) + self._node_updates[-1][KEY] = (metadata, prev) @contextlib.contextmanager def enter(self): From f9daf1ce00a0d0c2d587c1fa580d8884ffbeed5f Mon Sep 17 00:00:00 2001 From: tserg <8017125+tserg@users.noreply.github.com> Date: Tue, 5 Sep 2023 11:54:53 +0800 Subject: [PATCH 2/4] add test --- tests/ast/test_metadata_journal.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 tests/ast/test_metadata_journal.py diff --git a/tests/ast/test_metadata_journal.py b/tests/ast/test_metadata_journal.py new file mode 100644 index 0000000000..f20331fde9 --- /dev/null +++ b/tests/ast/test_metadata_journal.py @@ -0,0 +1,21 @@ +from vyper.ast.metadata import NodeMetadata +from vyper.exceptions import VyperException + + +def test_metadata_journal(): + m = NodeMetadata() + + m["x"] = 1 + try: + with m.enter_typechecker_speculation(): + m["x"] = 2 + m["x"] = 3 + + assert m["x"] == 3 + raise VyperException("dummy exception") + + except VyperException: + pass + + # rollback upon exception + assert m["x"] == 1 From 2e1261892d9f9716806d081c396b7c1d46adc80d Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Tue, 5 Sep 2023 17:52:00 -0400 Subject: [PATCH 3/4] add more metadata journal tests --- tests/ast/test_metadata_journal.py | 63 +++++++++++++++++++++++++++++- 1 file changed, 62 insertions(+), 1 deletion(-) diff --git a/tests/ast/test_metadata_journal.py b/tests/ast/test_metadata_journal.py index f20331fde9..34830409fc 100644 --- a/tests/ast/test_metadata_journal.py +++ b/tests/ast/test_metadata_journal.py @@ -2,7 +2,23 @@ from vyper.exceptions import VyperException -def test_metadata_journal(): +def test_metadata_journal_basic(): + m = NodeMetadata() + + m["x"] = 1 + assert m["x"] == 1 + + +def test_metadata_journal_commit(): + m = NodeMetadata() + + with m.enter_typechecker_speculation(): + m["x"] = 1 + + assert m["x"] == 1 + + +def test_metadata_journal_exception(): m = NodeMetadata() m["x"] = 1 @@ -19,3 +35,48 @@ def test_metadata_journal(): # rollback upon exception assert m["x"] == 1 + + +def test_metadata_journal_rollback_inner(): + m = NodeMetadata() + + m["x"] = 1 + with m.enter_typechecker_speculation(): + m["x"] = 2 + + try: + with m.enter_typechecker_speculation(): + m["x"] = 3 + m["x"] = 4 # test multiple writes + + assert m["x"] == 4 + raise VyperException("dummy exception") + + except VyperException: + pass + + assert m["x"] == 2 + + +def test_metadata_journal_rollback_outer(): + m = NodeMetadata() + + m["x"] = 1 + try: + with m.enter_typechecker_speculation(): + m["x"] = 2 + + with m.enter_typechecker_speculation(): + m["x"] = 3 + m["x"] = 4 # test multiple writes + + assert m["x"] == 4 + + m["x"] = 5 + + raise VyperException("dummy exception") + + except VyperException: + pass + + assert m["x"] == 1 From fcdafa9601380407f6710f5d4b2e33a10fa287c2 Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Tue, 5 Sep 2023 18:00:12 -0400 Subject: [PATCH 4/4] commit message: this commit fixes an issue where multiple writes inside of a checkpoint lead to journal corruption on rollback. in particular, it ensures a call to `register_update()` when the metadata dict has already been updated inside of a given checkpoint. note this does not change any observed functionality in the compiler because writes to the metadata journal inside for loops only ever happen to be written once, but it prevents a bug in case we ever add multiple writes inside of the same checkpoint. chainsec june review 5.3