[FastX adapter/verifier] Implement Move module initializers #337

awelc · 2022-02-01T23:12:07Z

This is the first step towards implementing task #90, that is putting together a skeleton of a testing framework for the adapter that will simplify testing of the new features as the code testing new functionality can be now (more) easily specified in the source files.

awelc · 2022-02-01T23:14:18Z

@sblackshear - I was wondering if this structure makes sense. In particular, I made the new tests part of "unit testing" even though they are using external resources (source files) - I think it should be OK but it would be good to get a second opinion before I create more of those.

sblackshear · 2022-02-02T00:30:31Z

@sblackshear - I was wondering if this structure makes sense. In particular, I made the new tests part of "unit testing" even though they are using external resources (source files) - I think it should be OK but it would be good to get a second opinion before I create more of those.

Yes, this looks great to me! We might considering calling the second src in (e.g.) fastx_programmability/adapter/src/unit_tests/src/simple_call/sources/M1.move something like data or test_data to emphasize that there aren't any Rust source files in there.

awelc · 2022-02-02T01:13:45Z

Yes, this looks great to me! We might considering calling the second src in (e.g.) fastx_programmability/adapter/src/unit_tests/src/simple_call/sources/M1.move something like data or test_data to emphasize that there aren't any Rust source files in there.

Good idea. I was wondering about it myself, and based on your suggestion renamed it to data (as test_data would arguably be a bit redundant I think).

awelc · 2022-02-05T04:13:02Z

I have pushed my first take at running module initializers even though it does not work 100% yet (though it passes all the tests).

The current implementation appears to run the code in the initializers but does not seem to be propagating all the effects correctly. In particular test_publish_init should result in an object being created as a result of running the initializer but assert_eq!(storage.created().len(), 1); will fail. Furthermore, even the package object does not seem to be making into the "created" set in these cases.

Even though this requires further investigation, I decided to push this commit so that @sblackshear and/or @lxfind can take a look. I went with @lxfind's suggestion on how to propagate effects across sessions , rather than going with what @sblackshear suggested that is to do everything within a single session. The main reason for this was that I was much further along the way towards multi-session approach and if this one is acceptable, then perhaps we can continue going this way.

Additionally, there are some minor refactorings that should probably happen here (e.g., to avoid some code duplication), but I did not want to go overboard with these considering that it's not even clear if this is the way to go.

sblackshear · 2022-02-05T15:17:13Z

fastx_programmability/adapter/src/adapter.rs

+    let function = match Function::new_from_name(module, INIT_FN_NAME) {
+        Some(v) => v,
+        None => return false,
+    };


I think should also check that function:

has no return value

has no type parameters

is private

We may also want to consider a bytecode verifier pass that checks this + checks that init is not called from anywhere.

First, I am sorry I was not more explicit about it, but this revision is definitely not final - I wanted to put out there just in case the route it takes is not the one we want (the alternative being the single-session approach with execute_function instead of execute_function_for_effects). If this one looks reasonable, then more work is certainly required, including fixing current deficiencies, verifier additions, more tests, etc.

With respect to this particular code fragment, I got so excited that I found code checking the type of the the TxContext parameter , now refactored to is_param_context (I wrote my own checks before...), that I missed the other conditions. Will add!

Is there a reason we make the check here, instead of adding a verifier to check that any function named init should follow this rule to avoid confusions?

The answer is that it may be seen as debatable to (for example) forbid all functions with the name init anywhere in the code.

As it is, we have flexibility to introduce checks in the verifier that are as strong or as weak as we want without affecting the execution path.

Indeed this is debatable. Here are my thoughts on why I think we should make it strict: It will make education a lot easier.
Imagine the two versions in our dev doc:
"init function is a reserved function and used to define module initializers. An init function must have a specific format otherwise an error will occur".

vs

"If you defined a function called init, and if it has the following 6 properties, it will be called during module initialization; otherwise it won't. And you won't be able to tell until you actually try to run it"

I think it can be dangerous to be too flexible here, since it's hard to reason about the code and can lead to mistakes and confusions.
The cost of not allowing a function named init is fairly minimum (we could also consider other names that's less common to avoid conflict too, such as __init__, or something along the line).
Happy to hear your thoughts and @sblackshear 's

@lxfind, I am with you there. In fact in my earlier discussions with @sblackshear I expressed the same preference. Still, we decided to do the separation for greater flexibility, and also to allow postponing implementation of the verifier pass until after the GDC deadline (can of course still be done before if we decide it so!).

Got it. In that case, can we file an issue to track it? (basically, for anything we want to come back on, either add a TODO or file an issue (preferred)

Done in #438.

sblackshear

This looks great for the most part! My only concerns of note are about the approach to updating TxContext and the gas_budget: 0 in the CLI.

sblackshear · 2022-02-10T17:23:06Z

fastpay/src/client.rs

@@ -383,6 +383,10 @@ enum ClientCommands {

        /// ID of the gas object for gas payment, in 20 bytes Hex string
        gas_object_id: ObjectID,
+
+        /// gas budget for running module initializers
+        #[structopt(default_value = "0")]


I think a default value of 0 will make any attempt to publish fail--something bigger might make sense?

Are there tests that exercise publishing from the CLI?

If no initializers are executed, then publishing with 0 gas budget will succeed - adapter_tests::test_simple_call (now) checks this particular case.

sblackshear · 2022-02-10T17:30:52Z

fastpay_core/src/authority/temporary_store.rs

@@ -182,16 +182,19 @@ impl Storage for AuthorityTemporaryStore {
    }

    fn read_object(&self, id: &ObjectID) -> Option<Object> {
-        match self.objects.get(id) {
+        match self.written.get(id) {


This is changing the behavior of this function from "read the state of the object before executing the tx" to "read the state of the object after executing the tx, but before committing to persistent storage". This may very well be ok, but CC @lxfind @gdanezis to double check on this. Also, I think we should add a comment explaining what this does (pre-existing issue, but will help avoid some future confusion).

Added @gdanezis as a reviewer (@lxfind already is one). This problem has also been reported in #394.

I wonder if we should return nothing if the object is in the deleted set. That being said, this only matters (for now) when running initializers and there is a limited amount of stuff that can be done in those as they are parameter-less, so perhaps it's not strictly necessary.

I think we need to clearly define (with comments) what the current new assumptions are and what could happen.
We know that we could have read after write now. But is it possible to have delete after write? Read after delete?
Once we define these assumptions, let's also assert them in the right place to make sure they are not violated.

Even the original set of assumptions (e.g., why we expected only either a write or an update to the same object but not both) are a bit unclear to me, so I was not sure what is needed here.

Perhaps we should go for the most general spec? That is:

An object can be only in either deleted or in written at any given time.

When reading an object:

check if it's deleted and, if so, return None

return its value in written if any

return its value in objects if any

return None

Though I am not sure if we even need 1 if we have 2.

Based on the current need (i.e. module initializers), I think we only have:

Read after write is OK

Nothing else is permitted (add assertions for read after delete, or delete after write, or write after delete)

sblackshear · 2022-02-10T17:33:04Z

fastpay_core/src/unit_tests/client_tests.rs

+    authority_clients: HashMap<AuthorityName, LocalAuthorityClient>,
+    committee: Committee,
+) -> ClientState<LocalAuthorityClient> {
+    let (admin, admin_key) = get_key_pair_from_bytes(&[


I don't have good understanding of the utilities available in the client tests, but the authority tests have get_key_pair(), which uses a seeded RNG to get an arbitrary (but deterministic) keypair. I think that is slightly cleaner than encoding the key bytes manually.

The thing is that the code being used here involves checks for a specific admin address and this address is both embedded in test source files and in the testing code (which I did not write - I just adapted testing code to work with module initializers).

sblackshear · 2022-02-10T18:04:35Z

fastx_programmability/adapter/src/adapter.rs

+            // (e.g., to reflect number of created objects) and use it
+            // to update the existing &mut TxContext instance.
+            let ctx_bytes = mutable_ref_values.pop().unwrap();
+            let updated_ctx: TxContext = bcs::from_bytes(ctx_bytes.as_slice()).unwrap();


One idea for how to do this without paying for this deserialization or introducing the possibility of an invalid update error:

Inside process_successful_execution, we will know the # of new objects created by this tx

We could pass the TxContext into there and do ctx.objects_created += num_objects_created.

Actually, ignore my suggestion--I don't think it's safe. The TxContext counter actually records the number of ID's created by the tx; the number of objects created can be smaller than this. A note that this update is only required on the publish code path + that we might want to move it there to avoid the overhead in call sometime in the future seems useful, though.

I was thinking on whether there is some way to compute the number of counter increments without reading the actual TxContext object but, as @sblackshear observed, I was not sure how safe/robust it would be.

I have, however put a conditional that takes the TxContext update path only when the helper function is executed during publishing (and not during "regular" Move call).

sblackshear · 2022-02-10T18:05:09Z

fastx_programmability/adapter/src/adapter.rs

+                Ok(ExecutionStatus::Failure { gas_used, error }) => exec_failure!(gas_used, *error),
+                Err(err) => exec_failure!(gas::MIN_MOVE_CALL_GAS, err),
+            };
+            // TODO: should we decrease gas budget on subsequent calls?


Yes--I think we can probably remove this comment (unless I misunderstand). It does seem like we should account for the possibility of current_gas_budget falling below 0 here, though?

Comment removed and I put an explicit overflow check to avoid panicking in case current_gas_budget < gas_used - the actual out-of-gas error will then be generated inside of the VM.

sblackshear · 2022-02-10T18:06:28Z

fastx_programmability/adapter/src/unit_tests/adapter_tests.rs

+        // try objects updated in temp memory first
+        match self.temporary.updated.get(id).cloned() {
+            Some(o) => Some(o),
+            // try objects creted in temp memory


Suggested change

// try objects creted in temp memory

// try objects created in temp memory

Done, thank you!

sblackshear · 2022-02-10T18:08:00Z

fastx_programmability/adapter/src/adapter.rs

+
+const INIT_FN_NAME: &IdentStr = ident_str!("init");
+
+pub fn module_with_init(module: &CompiledModule) -> bool {


Nit: I would find something like module_has_init slightly clearer.

…at no Rust code is involved

… module initializers

…vant tests

…entionally) fail due to insufficient gas

…rflow

sblackshear · 2022-02-10T23:35:54Z

fastpay/src/wallet_commands.rs

@@ -54,6 +54,10 @@ pub enum WalletCommands {
        /// ID of the gas object for gas payment, in 20 bytes Hex string
        #[structopt(long)]
        gas: ObjectID,
+
+        /// gas budget for running module initializers
+        #[structopt(default_value = "0")]


Still wondering about this--do we have any tests that try publishing from the wallet? I'd expect them to fail unless they explicitly pass a nonzero gas budget, but perhaps I'm wrong on that...

lxfind

I think we should add is_ok and is_err to ExecutionStatus to facilitate the checks.

lxfind · 2022-02-11T17:59:20Z

fastx_programmability/adapter/src/adapter.rs

+                let ctx_bytes = mutable_ref_values.pop().unwrap();
+                let updated_ctx: TxContext = bcs::from_bytes(ctx_bytes.as_slice()).unwrap();
+                if let Err(err) = ctx.update_state(updated_ctx) {
+                    exec_failure!(gas::MIN_MOVE_CALL_GAS, err);


We shouldn't return MIN_MOVE_CALL_GAS here. We have already finished VM execution, and hence any failure afterwards should return gas that's already used, in this case, gas_used.
But instead of fixing this here, see my other comment regarding how we should deal with tx context update.

I am going to put a quick fix here to avoid immediate problems.

lxfind · 2022-02-11T18:30:36Z

fastx_programmability/adapter/src/adapter.rs

+                // objects). We guard it with a flag to avoid
+                // serialization cost for non-publishing calls.
+                let ctx_bytes = mutable_ref_values.pop().unwrap();
+                let updated_ctx: TxContext = bcs::from_bytes(ctx_bytes.as_slice()).unwrap();


Instead of updating TxContext, it will be cleaner if you actually consume TxContext in the call, and return a new one. User code is unable to manipulate TxContext arbitrarily, so the one returned should always be valid.

@sblackshear suggested passing a mutable reference to the context, but I am not sure what the deeper motivation for that is.

Also, a solution with returning the context would involve changes on the Move repo side, wouldn't it? At this point we are re-using the calling functionality we already have and it does not support returning values. Unless you have some indirect way of returning in mind.

lxfind · 2022-02-11T19:02:05Z

fastx_programmability/adapter/src/adapter.rs

+            ) {
+                Ok(ExecutionStatus::Success { gas_used }) => gas_used,
+                Ok(ExecutionStatus::Failure { gas_used, error }) => exec_failure!(gas_used, *error),
+                Err(err) => exec_failure!(gas::MIN_MOVE_CALL_GAS, err),


This should not be MIN_MOVE_CALL_GAS. We need to charge based on what has been consumed so far.

lxfind · 2022-02-11T19:03:08Z

fastx_programmability/adapter/src/adapter.rs

+            if current_gas_budget > gas_used {
+                current_gas_budget -= gas_used;
+            } else {
+                current_gas_budget = 0;


I think there is a bug here. If the last run doesn't have sufficient gas, this will pass. But it should fail.

Actually, it's even more subtle, I think :-) Initially simply had current_gas_budget -= gas_used there and @sblackshear pointed out that we may need an additional check (if that subtraction failed, we'd panic). Now that I think about it, though, if current_gas_budget < gas_used then the call itself would fail (and throw an error) due to insufficient gas and we would never get to this subtraction. Does it make sense?

I see your point. Basically gas_used should be <= current_gas_budget by construction. In that case, we should make it a debug_assert?

lxfind · 2022-02-11T19:04:24Z

fastx_programmability/adapter/src/adapter.rs

    }

    // wrap the modules in an object, write it to the store
    let package_object = Object::new_package(modules, Authenticator::Address(sender), ctx.digest());
    state_view.write_object(package_object);

-    Ok(ExecutionStatus::Success)
+    let gas_cost = gas::calculate_module_publish_cost(&module_bytes);
+    let mut total_gas_used = gas_cost;


Now that we provide gas_budget with a Move Publish Transaction. For clarify, should that budget cover the cost of publishing itself too?

I think you are right, but it's more of a philosophical question. I will change it accordingly, but perhaps @sblackshear wants to weigh in.

Can you file an issue to decide on this?

Done in #440

lxfind · 2022-02-11T19:07:12Z

fastx_programmability/adapter/src/adapter.rs

+    let function = match Function::new_from_name(module, INIT_FN_NAME) {
+        Some(v) => v,
+        None => return false,
+    };


Is there a reason we make the check here, instead of adding a verifier to check that any function named init should follow this rule to avoid confusions?

lxfind · 2022-02-11T22:18:40Z

fastx_programmability/adapter/src/adapter.rs

    }

    // wrap the modules in an object, write it to the store
    let package_object = Object::new_package(modules, Authenticator::Address(sender), ctx.digest());
    state_view.write_object(package_object);

-    Ok(ExecutionStatus::Success)
+    let gas_cost = gas::calculate_module_publish_cost(&module_bytes);
+    let mut total_gas_used = gas_cost;


Can you file an issue to decide on this?

lxfind · 2022-02-11T22:18:51Z

fastx_programmability/adapter/src/adapter.rs

+            if current_gas_budget > gas_used {
+                current_gas_budget -= gas_used;
+            } else {
+                current_gas_budget = 0;


I see your point. Basically gas_used should be <= current_gas_budget by construction. In that case, we should make it a debug_assert?

lxfind · 2022-02-11T22:19:19Z

fastx_programmability/adapter/src/adapter.rs

+    let no_init_calls = modules_to_init.is_empty();
+    if !no_init_calls {


These two lines don't seem necessary.

no_init_calls is used in the code below.

hmmm interesting.
I think there is another bug here, imagine the following scenario:

There are init calls, and the gas_budget is sufficient to cover the init calls

But the gas_object doesn't have enough balance to cover total gas

In this case, we will eventually return Err with gas_used = gas_budget, which is wrong.

I think we really should have gas_budget to cover publish cost, it will make the logic a lot easier to reason about.

Got it! Before we make a decision on the gas_budget covering publish costs, a failure to deduct gas should result in total_gas_used being returned.

In fact, we should probably charge total_gas_used on both Move call and Move publish paths, as in both cases the actual work on the MoveVM side is already done.

for the move call order, we also check on the budget to make sure the gas object has efficient balance

Was this check actually implemented?

Never mind. I can now see it's checked by the authority in check_gas_requirement.

Let's include publish cost in the budget. I am fairly confident that's the right direction.

Sounds good. It would also simplify things if we had just one constant instead of these two: MIN_MOVE_PUBLISH_GAS and MIN_MOVE_CALL_GAS. Since at the point of gas amount verification in check_gas_requirement we don't know if publish operation will involve calls, we should probably conservatively assume that they would and check against MIN_MOVE_CALL_GAS and charge MIN_MOVE_CALL_GAS upon publishing failure, making MIN_MOVE_PUBLISH_GAS kind of redundant (they have currently the same value anyway).

What do you think? Perhaps MIN_MOVE_GENERIC_GAS

Yeah that's a good idea. Or just MIN_MOVE_GAS.

#436) * Added assertions representing current set of restrictions for accessing temporary stores * Gas-related fixes to the module initializers implementation * Do not apply gas budget to the actual publishing operation for now * Changes to assertions * Gas-related cleanup and fixes * Cleaned up gas handling during MoveVM operations * Replaced a macro with a function

* Final Demo Co-authored-by: Anastasios Kichidis <akihidis@gmail.com> * linter changes * fix python linter errors * fix: change cosmetic details - print which validators we connect to, - rust details of the last function, - mark the display code more clearly * sanitize seeder port input Co-authored-by: Anastasios Kichidis <akihidis@gmail.com> Co-authored-by: François Garillot <francois@garillot.net>

awelc requested a review from sblackshear February 1, 2022 23:12

awelc self-assigned this Feb 5, 2022

awelc requested a review from lxfind February 5, 2022 04:04

sblackshear reviewed Feb 5, 2022

View reviewed changes

huitseeker mentioned this pull request Feb 8, 2022

chore: start replacement FastNFT => Sui #395

Merged

awelc marked this pull request as ready for review February 10, 2022 06:34

sblackshear requested changes Feb 10, 2022

View reviewed changes

awelc force-pushed the aw/module-initializers branch from bebdfe4 to b79df47 Compare February 10, 2022 21:05

awelc added 5 commits February 10, 2022 13:08

Added adapter test that can use Move code from source files

ce41193

Renamed directory containing Move tests data to make it more clear th…

b7c085b

…at no Rust code is involved

First take at running module initializers

fda1c07

Fixes and changes to get all the current tests working in presence of…

c11f64f

… module initializers

Added additional restrictions on module initializer function and rele…

1d9f823

…vant tests

awelc force-pushed the aw/module-initializers branch from b79df47 to 1d9f823 Compare February 10, 2022 21:19

awelc added 5 commits February 10, 2022 13:29

Added non-zero gas budget to make sure that publish calls do not (int…

29f6f79

…entionally) fail due to insufficient gas

Make sure to test 0 gas budget when publishing without initializers

69f77fd

Update context modified in the Move VM only for publishing-related calls

56efb2f

Cosmetic changes

02347ec

Handle running out of gas gracefully rather than panicking due to ove…

d471a59

…rflow

awelc requested a review from gdanezis February 10, 2022 22:27

sblackshear approved these changes Feb 10, 2022

View reviewed changes

awelc merged commit d75b684 into main Feb 11, 2022

awelc deleted the aw/module-initializers branch February 11, 2022 05:12

oxade mentioned this pull request Feb 11, 2022

[adapter/authority] Return The Gas Used/Remaining In The Order Effects #250

Closed

huitseeker mentioned this pull request Feb 11, 2022

[bug] Bench error introduced in #337 #429

Closed

awelc mentioned this pull request Feb 11, 2022

[bench] Fixed accidentally reverted conditional #430

Closed

lxfind reviewed Feb 11, 2022

View reviewed changes

awelc mentioned this pull request Feb 11, 2022

[FastX adapter/verifier] Followup to the module initializers PR (#337) #436

Merged

lxfind reviewed Feb 11, 2022

View reviewed changes

awelc mentioned this pull request Feb 11, 2022

[verifier] Module initializer verification #438

Closed

awelc linked an issue Feb 17, 2022 that may be closed by this pull request

[fastx adapter/verifier] implement Move module initializers #90

Closed

gdanezis mentioned this pull request Mar 3, 2022

AuthorityTemporaryStore potential inconsistent access of object state #394

Closed

Daywalker99 mentioned this pull request Nov 10, 2022

[Snyk] Upgrade webextension-polyfill from 0.9.0 to 0.10.0 Daywalker99/sui#3

Open

snyk-bot mentioned this pull request Nov 20, 2022

[Snyk] Upgrade webextension-polyfill from 0.9.0 to 0.10.0 thuandm1/sui#3

Open

snyk-bot mentioned this pull request Apr 16, 2023

[Snyk] Upgrade webextension-polyfill from 0.9.0 to 0.10.0 mchern/sui#3

Open

	// try objects creted in temp memory
	// try objects created in temp memory


		const INIT_FN_NAME: &IdentStr = ident_str!("init");

		pub fn module_with_init(module: &CompiledModule) -> bool {

		let no_init_calls = modules_to_init.is_empty();
		if !no_init_calls {

[FastX adapter/verifier] Implement Move module initializers #337

[FastX adapter/verifier] Implement Move module initializers #337

Conversation

awelc commented Feb 1, 2022

awelc commented Feb 1, 2022

sblackshear commented Feb 2, 2022

awelc commented Feb 2, 2022

awelc commented Feb 5, 2022

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

sblackshear left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awelc Feb 10, 2022 • 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

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

Choose a reason for hiding this comment

lxfind 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awelc Feb 10, 2022 •

edited

Loading