[fastx dist sys] Connect vm adapter to handle-confirmation logic #62

gdanezis · 2021-12-15T11:22:58Z

This PR finishes issue #8 to support transactions with multiple inputs and outputs, and a connection to the real vm adapter.
Specifically:

We make the object store an immutable ref, and use interior mutability (Arc/Mutex) to allow concurrent access and multiple owners.
We create a local execution context / store 'AuthorityTemporaryStore' to provide objects to the execution, and collect result. We then process these results in 'handle_confirmation_order' to create new locks and objects.
Update the adapter to use correct sequence number (issue [fastx adapter] new Move VM API that accepts objects as input #30 )

gdanezis · 2021-12-15T11:25:03Z

fastpay_core/src/authority.rs

    type Error = FastPayError;

    fn get_resource(
        &self,
        address: &AccountAddress,
        struct_tag: &StructTag,
    ) -> Result<Option<Vec<u8>>, Self::Error> {
-        match self.objects.get(address) {
+        match self.authority_state.objects.lock().unwrap().get(address) {


Help! @sblackshear

Am I correct to think here either (1) a resource is listed in the input objects or (2) the resource is read-only. If this is the case, I will modify the read here to read from the 'AuthorityTemporaryStore' objects first, and then the global store (but check the read_only).

Same for the module lookup, but modules are always read-only so there this is invariant is trivial.

Good question. Actually, get_resource is only called when the Move VM touches Move global state, which we don't allow in fastX (https://github.com/MystenLabs/fastnft/blob/main/fastx_programmability/verifier/src/global_storage_access_verifier.rs is the mechanism for preventing this). So I think raising an Err corresponding to an internal invariant violation is probably the thing to do for now?

In the future, if we allow dynamic accesses of read-only objects, I think we would want to support it exactly as you suggest.

sblackshear

Excited for this!

fastpay_core/src/authority.rs

sblackshear · 2021-12-15T14:55:34Z

fastpay_core/src/authority.rs

+        }
+
+        // If it exists remove it
+        if let Some(removed) = self.objects.remove(id) {


I think raising an internal invariant violation if we try to remove something nonexistent would make sense

Right now I panic if it does not exist.

Where is the panic? I think this will silently fall through to 467 if id does not exist in objects (which is the case I'm concerned about).

fastx_programmability/adapter/src/adapter.rs

fastpay_core/src/authority.rs

sblackshear · 2021-12-15T21:22:30Z

fastpay_core/src/authority.rs

+        }
+
+        // If it exists remove it
+        if let Some(removed) = self.objects.remove(id) {


Where is the panic? I think this will silently fall through to 467 if id does not exist in objects (which is the case I'm concerned about).

fastpay_core/src/authority.rs

sblackshear · 2021-12-15T21:28:54Z

fastpay_core/src/authority.rs

+            // mutate objects if they are &mut so they cannot be read-only.
+            panic!("Internal invariant violation: Mutating a read-only object.")
+        }
+        self.written.push(object.to_object_reference());


I think checking no duplicates (either here or at into_inner) is a useful sanity check.

I will stick this behind a test conditional config.

Done: check for duplicate write, duplicate delete, duplicates across write & delete, and the number of writes+delete is equal to the mutable inputs.

sblackshear · 2021-12-15T21:34:37Z

fastpay_core/src/authority.rs

@@ -460,26 +481,102 @@ impl AuthorityState {
    }
 }

-impl Storage for AuthorityState {
+pub struct AuthorityTemporaryStore<'a> {
+    pub authority_state: &'a AuthorityState,


I would be in favor of removing the pub from each of these fields + exposing getters for any that we want to allow folks to read. There are invariants about the relationship between fields like objects and deleted that are enforced by the functions exposed, but could be violated by someone accessing these fields directly.

Please have a look at #[readonly::make]: https://github.com/dtolnay/readonly

George: None of them have to be pub, so all good.

Totally, done. When I build a struct i start with pub members, otherwise the type system complains they are not used, and as a result I forget them as pub. My bad.

huitseeker

One main Q on read-only violations as a non-recoverable error.

fastpay_core/src/authority.rs

huitseeker · 2021-12-15T23:28:26Z

fastpay_core/src/authority.rs

@@ -460,26 +481,102 @@ impl AuthorityState {
    }
 }

-impl Storage for AuthorityState {
+pub struct AuthorityTemporaryStore<'a> {
+    pub authority_state: &'a AuthorityState,


Please have a look at #[readonly::make]: https://github.com/dtolnay/readonly

George: None of them have to be pub, so all good.

huitseeker · 2021-12-15T23:48:34Z

fastpay_core/src/authority.rs

+");
+                Ok(Some(m.contents.clone()))
+            }
+            other => unimplemented!(


Is it possible that a #[non_exhaustive] attribute is missing somewhere in Data?

Here @sblackshear may have a view, as he designed the 'Data' abstraction?

Did not know about this feature, very cool! Yes, would make sense to add this on Data

huitseeker · 2021-12-15T23:53:11Z

fastpay_core/src/authority.rs

+        if object.is_read_only() {
+            // This is an internal invariant violation. Move only allows us to
+            // mutate objects if they are &mut so they cannot be read-only.
+            panic!("Internal invariant violation: Mutating a read-only object.")
+        }


What's the value in making this (and other places) an irrecoverable panic versus returning a Result, or some other indication of success / failure of the operation? It seems that writing to a read-only object might be a common mistake?

Right now we are restricted in terms of interface to what the movevm expects from a Storage I believe, so I have to follow the signatures that return no Result. At the same time I would rather check some of these invariants.

Note these are internal invariants, if the move adaptor is implemented correctly they should never trigger. Let me stick them behind a test comp flag to only use when refactoring, but not in prod -- another reason to not change the signatures.

This concern for an object being or not being read-only is everywhere in the code at this stage. I'd be comfortable with a debug_assert, which tells me this should never fire outside egregious misuse, or asking the move VM to change its APIs, whichever suits you best.

Although this is an internal invariant violation, I think should eventually be a Result so we can propagate it up to someone that can log it.

This code is inside Storage::write_object(&self, Object), which is a FastNFT trait under our control. I think that API should indeed be changed to use Result everywhere, but perhaps in a separate PR. This won't cause any problems for Move--it will just need to handle the Result inside the adapter.

Ah ok, I was confused about the provenance of Storage. Lets indeed make it fire results, but I also think we should not be spending runtime resources checking for errors in our code, again and again.

sblackshear · 2021-12-16T15:46:24Z

fastpay_core/src/authority.rs

    fn delete_object(&mut self, id: &ObjectID) {
-        self.objects.remove(id);
+        // Check it is not read-only
+        #[cfg(test)] // Movevm should ensure this


@huitseeker: thoughts about cfg(test) vs debug_assert vs other options for assertions we want to fire everywhere except in release builds?

assertions we want to fire everywhere except in release builds?

Not #[cfg(test)], that only fires in testing.
debug_assert is made for this.

Ok, thanks @sblackshear & @huitseeker for all the feedback:

I have now moved the checks to a separate function.

I have used the HashSet pattern to keep code short.

I have improved the check to ensure all mutable objects are either written or deleted.

As a result I caught a bug (we were not updating gas object in module upload) and fixed it.

huitseeker

Hopefully I've left comments that can improve at least the quadratic duplicate checks.

huitseeker · 2021-12-16T19:15:07Z

fastpay_core/src/authority.rs

+            for (i, write_ref) in self.written.iter().enumerate() {
+                for (j, write_ref2) in self.written.iter().enumerate() {
+                    if i != j && write_ref == write_ref2 {
+                        panic!("Invariant violation: duplicate writing.");
+                    }
+                }


This is :

debug_assert!({ let mut used = HashSet::new(); self.written.all(move |elt| used.insert(elt)) })

Fixed, see above

huitseeker · 2021-12-16T19:17:30Z

fastpay_core/src/authority.rs

+            for (k, del_ref) in self.deleted.iter().enumerate() {
+                for (l, del_ref2) in self.deleted.iter().enumerate() {
+                    if k != l && del_ref == del_ref2 {
+                        panic!("Invariant violation: duplicate deletion.");
+                    }
+                }
+            }


See above for refactoring a uniqueness check to linear time with a HashSet.

Fixed, see above

huitseeker · 2021-12-16T19:18:54Z

fastpay_core/src/authority.rs

+                for del_ref in &self.deleted {
+                    if write_ref == del_ref {
+                        panic!("Invariant violation: both writing and deleting same object.");
+                    }
+                }
+            }


You could just have a list of distinct object refs and a bitmap or compressed bitmap for the deleted ones, with the rest being written.

But if you happen to like this two-vectors approach, ideally, you'd be able to reuse the HashSets you're building above and below to make sure that before inserting something in the second HashSet, you're checking for non-presence in the prior one.

I kept the two vector approach but refactored the test using HashSets. Fixed, see above.

huitseeker · 2021-12-16T19:20:46Z

fastpay_core/src/authority.rs

+            // Check a number of invariants. A correct vm calling the struct should ensure these
+            // but to help with testing and refactoring we check again here.


Please export this invariant-checking to a function.

Yep, now in a separate function. Fixed, see above

huitseeker · 2021-12-16T19:21:10Z

fastpay_core/src/authority.rs

+        Vec<ObjectRef>,
+        Vec<ObjectRef>,
+    ) {
+        #[cfg(test)]


This only fires in testing, I suspect you mean debug_assert.

Thanks for this -- now all behind debug_assert

gdanezis · 2021-12-17T14:01:18Z

Hey @huitseeker - do a check and then we can land this?

huitseeker

Nit: Ah, ok! I see better why you were looking at a config flag now.

The PR is OK in its current state, but here's what I think you were trying to do: using #[cfg(debug_assertions)]

You could make your checking function check_invariants:

not use debug_assert itself,
but return a boolean and an optional message instead
then in the main function you would:

#[cfg(debug_assertions)]
{
    let (ok, msg) = check_invariants();
    debug_assert!(ok, "{}", msg);
}

The advantage being that absolutely nothing runs if you're not in debug mode, not even the function call.

gdanezis · 2021-12-17T15:47:32Z

Nit: Ah, ok! I see better why you were looking at a config flag now.

The PR is OK in its current state, but here's what I think you were trying to do: using #[cfg(debug_assertions)]

You could make your checking function check_invariants:

not use debug_assert itself,

but return a boolean and an optional message instead

then in the main function you would:
#[cfg(debug_assertions)]
{
    let (ok, msg) = check_invariants();
    debug_assert!(ok, "{}", msg);
}
The advantage being that absolutely nothing runs if you're not in debug mode, not even the function call.

Nice one. I used a simpler variant of the above to avoid the call. (I suspect the compiler already avoided the call, but to be safe).

gdanezis commented Dec 15, 2021

View reviewed changes

gdanezis marked this pull request as draft December 15, 2021 11:29

sblackshear reviewed Dec 15, 2021

View reviewed changes

gdanezis commented Dec 15, 2021

View reviewed changes

fastx_programmability/adapter/src/adapter.rs Show resolved Hide resolved

gdanezis marked this pull request as ready for review December 15, 2021 15:43

gdanezis requested a review from sblackshear December 15, 2021 15:44

gdanezis self-assigned this Dec 15, 2021

gdanezis mentioned this pull request Dec 15, 2021

[programmability] processing logic for module publishing #59

Merged

sblackshear reviewed Dec 15, 2021

View reviewed changes

huitseeker reviewed Dec 16, 2021

View reviewed changes

gdanezis force-pushed the localcontext branch from 15eb9f9 to a818221 Compare December 16, 2021 13:15

sblackshear reviewed Dec 16, 2021

View reviewed changes

sblackshear approved these changes Dec 16, 2021

View reviewed changes

huitseeker suggested changes Dec 16, 2021

View reviewed changes

George Danezis added 8 commits December 17, 2021 11:21

Mutex protect object store, and make temp store in authority

b32c645

Connect vm output to dist sys logic

0db2310

Updated adpater to use correct sequence numbers (issue #30)

58f1258

Revised based on feedback (Thanks @sam)

6e3d801

Handle read_only objects correctly in handle_order

214db20

Intergrated review comments (thanks @sam & @francois)

61a54b4

Rebased, and fixed tests

c0e9850

Improved checks and tests; fixed gas update bug

bec75c2

gdanezis force-pushed the localcontext branch from 54e633b to bec75c2 Compare December 17, 2021 11:22

Make fmt happy

5fc8bfd

huitseeker approved these changes Dec 17, 2021

View reviewed changes

Eliminate function call if not debug

f29e559

gdanezis merged commit 2d3275f into main Dec 17, 2021

gdanezis deleted the localcontext branch December 17, 2021 15:55

huitseeker mentioned this pull request Dec 17, 2021

fix: fix dead code warning in release mode #70

Merged

huitseeker mentioned this pull request Jan 4, 2022

Add gas logic for move call and module publish #119

Merged

		// Check a number of invariants. A correct vm calling the struct should ensure these
		// but to help with testing and refactoring we check again here.

[fastx dist sys] Connect vm adapter to handle-confirmation logic #62

[fastx dist sys] Connect vm adapter to handle-confirmation logic #62

Conversation

gdanezis commented Dec 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sblackshear Dec 15, 2021 • edited Loading

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

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

huitseeker Dec 15, 2021 • edited by gdanezis Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huitseeker left a comment • edited Loading

Choose a reason for hiding this comment

huitseeker Dec 15, 2021 • edited by gdanezis 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

gdanezis Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sblackshear Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huitseeker Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huitseeker left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huitseeker Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huitseeker Dec 16, 2021 • 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

gdanezis commented Dec 17, 2021

huitseeker left a comment

Choose a reason for hiding this comment

gdanezis commented Dec 17, 2021

gdanezis commented Dec 15, 2021 •

edited

Loading

sblackshear Dec 15, 2021 •

edited

Loading

huitseeker Dec 15, 2021 •

edited by gdanezis

Loading

huitseeker left a comment •

edited

Loading

huitseeker Dec 15, 2021 •

edited by gdanezis

Loading

gdanezis Dec 16, 2021 •

edited

Loading

sblackshear Dec 16, 2021 •

edited

Loading

huitseeker Dec 16, 2021 •

edited

Loading

huitseeker left a comment •

edited

Loading

huitseeker Dec 16, 2021 •

edited

Loading

huitseeker Dec 16, 2021 •

edited

Loading