[move][test_scenario] Fix bug in `test_scenario` where we weren't returning allocated tickets #17991

tzakian · 2024-05-30T05:59:30Z

Description

Fixes a bug where we weren't returning allocated receiving tickets in a transaction at the end of the transaction. This fixes the bug, and adds tests to make sure allocated tickets are properly handled at the end of a test scenario transaction.

Test plan

Added new tests to test the failing behavior and made sure they pass.

vercel · 2024-05-30T05:59:35Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
sui-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 13, 2024 9:05pm

3 Skipped Deployments

Name	Status	Preview	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview	Sep 13, 2024 9:05pm
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview	Sep 13, 2024 9:05pm
sui-typescript-docs	⬜️ Ignored (Inspect)	Visit Preview	Sep 13, 2024 9:05pm

amnn

I think I understand the core change, but would appreciate some more context on the broader changes!

amnn · 2024-07-16T10:59:28Z

crates/sui-framework/packages/sui-framework/sources/test/test_scenario.move

            sender,
-            scenario.txn_number,
+            test_random_tx_hash,


Could you share some context on how the (pseudo-)random tx digest generation fits into the overall fix?

(Might be worth adding some of that context as a code comment around this change).

Happy to add a code comment.

Basically:

Object IDs are generated from the transaction context based on the transaction hash + number of generated IDs thus far in the transaction.

In test_scenario we always generate the transaction context with the same initial state of for a given sender.

This means that each Move unit test using the test scenario will generate the same sequence of object IDs when calling new_object.

The object runtime (and storage) is shared across all Move unit tests, and Move unit tests are executed in parallel.

This set of things leads to situations where multiple Move unit tests using test scenario can read/write to the shared object runtime/storage with the same object ID and different values in parallel leading to race conditions on the state of the object runtime.

By seeding the tx digest pseudo-randomly whenever we start a transaction we make it basically impossible to run into this type of "multiple objects with the same ID being read/written in parallel to the shared runtime" in Move unit tests.

Got it -- thanks for explaining! Happy with this for now, but it kind of seems like the right long-term solution is to use a different scenario object per test or something (to graduate this from "basically impossible" to "actually impossible"), how difficult would something like that be?

crates/sui-framework/packages/sui-framework/tests/test_scenario_tests.move

amnn · 2024-07-16T11:03:38Z

crates/sui-framework/packages/sui-framework/tests/test_scenario_tests.move

+            assert!(parent.value == 10, EValueMismatch);
+            assert!(obj2.value == 20, EValueMismatch);


Can we use clever errors for this?

Suggested change

assert!(parent.value == 10, EValueMismatch);

assert!(obj2.value == 20, EValueMismatch);

assert!(parent.value == 10);

assert!(obj2.value == 20);

I noticed that in another place you were able to get rid of the abort code but not in this file -- is it just a change waiting to be pushed?

ah darn forgot to add these changes. Will update!

amnn · 2024-07-16T11:04:48Z

crates/sui-framework/packages/sui-framework/tests/test_scenario_tests.move

+    // Make sure that we properly handle the case where we receive an object
+    // when dealing with allocated tickets.


nit: doc comment? Also could you describe what happens when we don't properly handle that case, for posterity?

crates/sui-framework/packages/sui-framework/tests/test_scenario_tests.move

sui-execution/latest/sui-move-natives/src/test_scenario.rs

crates/sui-framework/packages/sui-framework/tests/test_scenario_tests.move

tzakian · 2024-07-16T22:15:11Z

crates/sui-framework/packages/sui-framework/tests/random_tests.move

@@ -51,7 +51,8 @@ module sui::random_tests {
            scenario.ctx(),
        );
        scenario.next_tx(@0x0);
-        let gen1 = random_state.new_generator(scenario.ctx());
+        let mut ctx = tx_context::new_from_hint(@0x0, 1, 0, 0, 0);


Note the changes here: it was previously using the fact that the object IDs derived from the context were stable which is no longer the case so we manually stabilize them here.

I don't fully understand, what happens if this isn't stable?

tzakian · 2024-07-16T22:16:01Z

crates/sui-framework/packages/sui-framework/tests/random_tests.move

@@ -442,8 +456,10 @@ module sui::random_tests {
        let _output = gen.generate_u128_in_range(0, 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF);
        let mut i = 0;
        while (i < 50) {
-            let min = gen.generate_u128();
-            let max = min + (gen.generate_u64() as u128);
+            let (min, max) = (gen.generate_u128(), gen.generate_u128());


Fixed logic here since this was incorrect and subject to overflow (and the tests were in fact failing for this).

amnn

Looks good! Some questions from me, and perhaps some final clean-ups on abort codes to get pushed still, but this is good to go, thanks @tzakian !

amnn · 2024-07-17T11:57:36Z

crates/sui-framework/packages/sui-framework/tests/random_tests.move

@@ -103,6 +106,9 @@ module sui::random_tests {
            scenario.ctx(),
        );

+        // Deterministically set tx context so the RNG seeded by the ctx below is deterministic.


Here, we're kind of hoping that someone doesn't create an object using this ctx in later code, right? Because if they did, we could be in that bad situation where two tests run in parallel and write different contents for the same object.

FWIW, I preferred the explicit use of the ctx you get from new_from_hint instead of overwriting the scenario's context for that reason.

amnn · 2024-07-17T11:59:06Z

crates/sui-framework/packages/sui-framework/tests/test_scenario_tests.move

+            assert!(parent.value == 10, EValueMismatch);
+            assert!(obj2.value == 20, EValueMismatch);


I noticed that in another place you were able to get rid of the abort code but not in this file -- is it just a change waiting to be pushed?

tnowacki · 2024-07-22T21:00:52Z

crates/sui-framework/packages/sui-framework/sources/test/test_scenario.move

+        // Randomly seed the transaction context's transaction hash to avoid
+        // object ID collisions in parallel Move tests.
+        //


I'd remove this comment, since it won't be true long term.

I would just summarize here that the transaction digest is randomized to better help simulate real execution environments. Not sure we need to say more than that?

I think this block of comments would be a lot better placed in some rust file if possible

tnowacki · 2024-07-22T21:03:41Z

crates/sui-framework/packages/sui-framework/tests/random_tests.move

@@ -51,7 +51,8 @@ module sui::random_tests {
            scenario.ctx(),
        );
        scenario.next_tx(@0x0);
-        let gen1 = random_state.new_generator(scenario.ctx());
+        let mut ctx = tx_context::new_from_hint(@0x0, 1, 0, 0, 0);


I don't fully understand, what happens if this isn't stable?

tnowacki · 2024-07-22T21:04:11Z

crates/sui-framework/packages/sui-framework/tests/random_tests.move

+        *scenario.ctx() = tx_context::new_from_hint(@0x0, 1, 0, 0, 0);
+


I am afraid

When the Rng is generated, it takes the source of randomness that is shared between transactions in a checkpoint (Random) and mixes it with its own transaction digest (by generating a new address and then using that), so if the digest is not stable in this case, you'll get different random numbers each time.

My fear was from writing to scenario.ctx(), not the hard coding of the context itself

tzakian · 2024-07-23T19:59:29Z

Just FYI @amnn and @tnowacki -- took a look at using thread local storage for the object store, and that solved the issues very nicely, and without the need to randomly seed the test scenario transaction context 🎉

tnowacki · 2024-07-24T18:59:14Z

sui-execution/latest/sui-move-natives/src/test_scenario.rs

-pub struct InMemoryTestStore(pub &'static RwLock<InMemoryStorage>);
+pub struct InMemoryTestStore(pub &'static LocalKey<RefCell<InMemoryStorage>>);


Approving to unblock, but this still feels like a crime.

…urning allocated tickets.

…urning allocated tickets (#17991) ## Description Fixes a bug where we weren't returning allocated receiving tickets in a transaction at the end of the transaction. This fixes the bug, and adds tests to make sure allocated tickets are properly handled at the end of a test scenario transaction. ## Test plan Added new tests to test the failing behavior and made sure they pass.

[move][test_scenario] Fix bug in test_scenario where we weren't returning allocated tickets (#17991) MystenLabs/sui#17991

tzakian requested review from amnn, tnowacki and a team May 30, 2024 05:59

tzakian force-pushed the tzakian/tto-test-scenario-fix branch from add2a37 to 131c6c7 Compare July 16, 2024 02:30

vercel bot deployed to Preview – sui-docs July 16, 2024 02:31 View deployment

amnn reviewed Jul 16, 2024

View reviewed changes

tzakian force-pushed the tzakian/tto-test-scenario-fix branch from 131c6c7 to 18a359e Compare July 16, 2024 22:13

tzakian commented Jul 16, 2024

View reviewed changes

vercel bot deployed to Preview – sui-docs July 16, 2024 22:15 View deployment

tzakian commented Jul 16, 2024

View reviewed changes

amnn approved these changes Jul 17, 2024

View reviewed changes

tzakian force-pushed the tzakian/tto-test-scenario-fix branch from 18a359e to ab6928a Compare July 18, 2024 22:33

vercel bot deployed to Preview – sui-docs July 18, 2024 22:34 View deployment

tnowacki reviewed Jul 22, 2024

View reviewed changes

tzakian force-pushed the tzakian/tto-test-scenario-fix branch from ab6928a to 09ca36e Compare July 23, 2024 19:54

vercel bot deployed to Preview – sui-docs July 23, 2024 19:56 View deployment

tzakian force-pushed the tzakian/tto-test-scenario-fix branch from 09ca36e to 4e2cf6e Compare July 23, 2024 19:57

vercel bot deployed to Preview – sui-docs July 23, 2024 19:59 View deployment

tzakian force-pushed the tzakian/tto-test-scenario-fix branch from 4e2cf6e to ff47f40 Compare July 23, 2024 20:11

vercel bot deployed to Preview – sui-docs July 23, 2024 20:12 View deployment

tzakian force-pushed the tzakian/tto-test-scenario-fix branch from ff47f40 to 4028c93 Compare July 24, 2024 18:52

tzakian enabled auto-merge (squash) July 24, 2024 18:52

vercel bot deployed to Preview – sui-docs July 24, 2024 18:56 View deployment

tnowacki approved these changes Jul 24, 2024

View reviewed changes

tnowacki reviewed Jul 24, 2024

View reviewed changes

tzakian added 3 commits September 13, 2024 13:52

[move][test_scenario] Fix bug in test_scenario where we weren't ret…

49a4f64

…urning allocated tickets.

Additional fixes for TTO in test scenario

737fd2c

fixup! Additional fixes for TTO in test scenario

3ed4b47

tzakian added 2 commits September 13, 2024 13:52

fixup! fixup! Additional fixes for TTO in test scenario

2e74335

fixup! fixup! fixup! Additional fixes for TTO in test scenario

a430508

tzakian force-pushed the tzakian/tto-test-scenario-fix branch from 4028c93 to a430508 Compare September 13, 2024 21:04

vercel bot deployed to Preview – sui-docs September 13, 2024 21:05 View deployment

tzakian merged commit 2de2e03 into main Sep 13, 2024
48 checks passed

tzakian deleted the tzakian/tto-test-scenario-fix branch September 13, 2024 21:27

valeriyr added a commit to iotaledger/iota that referenced this pull request Feb 3, 2025

fix: added missed changed from the upstream PR #17991

f07075d

[move][test_scenario] Fix bug in test_scenario where we weren't returning allocated tickets (#17991) MystenLabs/sui#17991

valeriyr added a commit to iotaledger/iota that referenced this pull request Feb 3, 2025

fix: added missed changed from the upstream PR #17991

32acb20

[move][test_scenario] Fix bug in test_scenario where we weren't returning allocated tickets (#17991) MystenLabs/sui#17991

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[move][test_scenario] Fix bug in `test_scenario` where we weren't returning allocated tickets #17991

[move][test_scenario] Fix bug in `test_scenario` where we weren't returning allocated tickets #17991

tzakian commented May 30, 2024

vercel bot commented May 30, 2024 •

edited

Loading

amnn left a comment

amnn Jul 16, 2024

amnn Jul 16, 2024

tzakian Jul 16, 2024

amnn Jul 17, 2024

amnn Jul 16, 2024

amnn Jul 17, 2024

tzakian Jul 17, 2024

amnn Jul 16, 2024

tzakian Jul 16, 2024

tnowacki Jul 22, 2024

tzakian Jul 16, 2024

amnn left a comment

amnn Jul 17, 2024

amnn Jul 17, 2024

tnowacki Jul 22, 2024

tnowacki Jul 22, 2024

tnowacki Jul 22, 2024

amnn Jul 23, 2024

tnowacki Jul 23, 2024

tzakian commented Jul 23, 2024

tnowacki Jul 24, 2024

		assert!(parent.value == 10, EValueMismatch);
		assert!(obj2.value == 20, EValueMismatch);

		// Make sure that we properly handle the case where we receive an object
		// when dealing with allocated tickets.

		*scenario.ctx() = tx_context::new_from_hint(@0x0, 1, 0, 0, 0);

		pub struct InMemoryTestStore(pub &'static RwLock<InMemoryStorage>);
		pub struct InMemoryTestStore(pub &'static LocalKey<RefCell<InMemoryStorage>>);

[move][test_scenario] Fix bug in test_scenario where we weren't returning allocated tickets #17991

[move][test_scenario] Fix bug in test_scenario where we weren't returning allocated tickets #17991

Conversation

tzakian commented May 30, 2024

Description

Test plan

vercel bot commented May 30, 2024 • edited Loading

amnn 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

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

amnn 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tzakian commented Jul 23, 2024

Choose a reason for hiding this comment

[move][test_scenario] Fix bug in `test_scenario` where we weren't returning allocated tickets #17991

[move][test_scenario] Fix bug in `test_scenario` where we weren't returning allocated tickets #17991

vercel bot commented May 30, 2024 •

edited

Loading