[fastx adapter] making move execution work end-to-end #88

sblackshear · 2021-12-23T20:14:30Z

Using the new VM API in diem/diem@346301f to make the Move-powered execution work as intended. At a high level:

The adapter takes some FastX objects, some pure values (e.g., addresses), and a Move function to call as input as input
It takes extracts a Move value from each object and type-checks the objects against the function signature
It invokes the function on the Move values and pure values
It processes the effects produced from executing the function to update the input objects, as well as updating the store with freshly created and deleted objects.

This is intricate logic that will need much more testing in time. But wanted to get this functionality out as early as possible to unblock the client workstream, which will need to send MoveCall orders and observe their effects.

huitseeker

This looks pretty darn amazing to me, but I'm a bit of a Move noob, so I'll just comment.

This introduces 8 new TODOs not covered by a subsequent PR: should we introduce an umbrella issue to track them? "Leftovers in adapter logic", perhaps?

fastpay_core/src/authority.rs

fastx_programmability/adapter/src/adapter.rs

huitseeker · 2021-12-28T16:23:27Z

fastx_programmability/adapter/src/adapter.rs

+    for (mut obj, new_contents) in mutable_refs {
+        match &mut obj.data {
+            Data::Move(m) => m.contents = new_contents,
+            _ => unreachable!(),


Is this unreachable or is this an API invariant violation? Should a panic here involve a message?

Also, consider using pre, perhaps.

It is unreachable--we checked earlier than mutable_refs only had Move objects. Will change to panic with an appropriate message.

pre looks very interesting/useful. But I am trying to weigh the risk/reward of adding this as a dep in a system that's currently pretty dependency-minimal--thoughts on that?

pre is:

pretty darn safe, because it's itself small, it disappears at release and is dependency-minimal,

really useful when you're committed to using a specific set of preconditions that should be enforced by callers, rather than a one-off.

So it may be just early (in this PR), if you do have a battery invariants in mind, or pre may not be for you if you don't.

huitseeker · 2021-12-28T16:30:33Z

fastx_programmability/adapter/src/adapter.rs

+    }
+    // any object left in `by_value_objects` is an input passed by value that was not transferred or frozen.
+    // this means that either the object was (1) deleted from the FastX system altogether, or
+    // (2) wrapped inside another object that is in the FastX object pool


Remind me: what's the logic for deleting wrapped objects in this case?

A wrapped object was either:

Passed in via by_value_objects and not subsequently transferred

Created by the transaction and subsequently transferred or deleted

In case (1), the wrapped object will be present in by_value_objects and thus deleted from the object pool here.

In case (2), the wrapped object was never in the object pool, so there's no need to delete it.

Got it, thanks!

fastx_programmability/adapter/src/adapter.rs

huitseeker · 2021-12-28T16:33:18Z

fastx_programmability/adapter/src/adapter.rs

+        }
+    }
+    debug_assert!(
+        by_value_objects.len() + mutable_ref_objects.len() + num_immutable_objects == num_objects


huitseeker · 2021-12-28T16:40:57Z

fastx_programmability/framework/sources/ObjectBasics.move

@@ -0,0 +1,30 @@
+/// Test CTURD object basics (create, transfer, update, read, delete)


💩 ?
(nit : consider a different acronym)

Lol, this was the most memorable one I could think of

CRUD is a well-known acronym in databases, so that CRUDT, TCRUD would ring a bell.

huitseeker · 2021-12-28T16:41:51Z

fastx_types/src/base_types.rs

@@ -118,6 +142,11 @@ impl TransactionDigest {
            SequenceNumber::new(),
        )
    }
+
+    // for testing


nit: #[cfg(test)] ?

Would do this, but then it can't be used outside of the base_types crate (unless there is a way to allow this?)

True, the only way to allow this is with a feature, which dependent crates would have to activate when appropriate. Sigh.

Probably makes sense for us to do this eventually because we have several functions that look like this... but in a future PR

fastx_types/src/object.rs

sblackshear · 2021-12-28T19:56:21Z

This introduces 8 new TODOs not covered by a subsequent PR: should we introduce an umbrella issue to track them? "Leftovers in adapter logic", perhaps?

Good call + will do!

Using the new VM API in diem/diem@346301f to make the Move-powered execution work as intended. At a high level: - The adapter takes some FastX objects, some pure values (e.g., addresses), and a Move function to call as input as input - It takes extracts a Move value from each object and type-checks the objects against the function signature - It invokes the function on the Move values and pure values - It processes the effects produced from executing the function to update the input objects, as well as updating the store with freshly created and deleted objects. This is intricate logic that will need much more testing in time. But wanted to get this functionality out as early as possible to unblock the client workstream, which will need to send `MoveCall` orders and observe their effects.

huitseeker

One Q: where (would|does)a the sequence number management that happens during the creation of an object from an unwrap live?

sblackshear · 2021-12-28T23:57:42Z

fastx_programmability/adapter/src/adapter.rs

+                        old_object.next_sequence_number = sequence_number;
+                        old_object
+                    } else {
+                        Object::new_move(move_obj, recipient, SequenceNumber::new())


One Q: where (would|does)a the sequence number management that happens during the creation of an object from an unwrap live?

@huitseeker : this is it--same code path as object creation.

Worth noting that this is (as @gdanezis pointed out in the preso this morning) completely unsafe if sequence numbers are used for replay prevention (as they are today ...).

But it is fine in the (future, proposed world) where keys in the order_lock map are (ObjectId, TxDigest) (or a commitment to this pair).

#98 for much more detail on this.

sblackshear · 2021-12-29T06:15:52Z

I'm going to go ahead and land this because there are a few outstanding PR's that are tough to review by virtue of being based on it. Happy to address any/all other comments async!

sblackshear requested review from huitseeker, lxfind, patrickkuo, gdanezis and oxade December 23, 2021 20:14

sblackshear force-pushed the use_new_move_vm_api branch from 2e70390 to 68f99f8 Compare December 23, 2021 20:32

sblackshear changed the title ~~[RFC][fastx adapter] making move execution work end-to-end~~ [fastx adapter] making move execution work end-to-end Dec 23, 2021

sblackshear force-pushed the use_new_move_vm_api branch 4 times, most recently from 24b17d1 to 0b46af2 Compare December 26, 2021 18:03

sblackshear mentioned this pull request Dec 27, 2021

[fastx types] 32 byte authenticator #94

Merged

huitseeker reviewed Dec 28, 2021

View reviewed changes

sblackshear force-pushed the use_new_move_vm_api branch from 2a2e158 to 84b86b0 Compare December 28, 2021 21:14

sblackshear force-pushed the use_new_move_vm_api branch from 84b86b0 to 1d83bd5 Compare December 28, 2021 21:25

huitseeker reviewed Dec 28, 2021

View reviewed changes

sblackshear commented Dec 28, 2021

View reviewed changes

sblackshear merged commit 3a3575a into MystenLabs:main Dec 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fastx adapter] making move execution work end-to-end #88

[fastx adapter] making move execution work end-to-end #88

sblackshear commented Dec 23, 2021

huitseeker left a comment

huitseeker Dec 28, 2021

sblackshear Dec 28, 2021

huitseeker Dec 28, 2021

huitseeker Dec 28, 2021

sblackshear Dec 28, 2021

huitseeker Dec 28, 2021

huitseeker Dec 28, 2021

huitseeker Dec 28, 2021

sblackshear Dec 28, 2021

huitseeker Dec 28, 2021

huitseeker Dec 28, 2021

sblackshear Dec 28, 2021

huitseeker Dec 28, 2021

sblackshear Dec 28, 2021

sblackshear commented Dec 28, 2021

huitseeker left a comment

sblackshear Dec 28, 2021

sblackshear Dec 29, 2021

sblackshear Dec 29, 2021

sblackshear commented Dec 29, 2021

		@@ -0,0 +1,30 @@
		/// Test CTURD object basics (create, transfer, update, read, delete)

[fastx adapter] making move execution work end-to-end #88

[fastx adapter] making move execution work end-to-end #88

Conversation

sblackshear commented Dec 23, 2021

huitseeker 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

sblackshear commented Dec 28, 2021

huitseeker 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

sblackshear commented Dec 29, 2021