Enhance/cleanup unconfirmed execution #375

oxade · 2022-02-06T02:30:53Z

This PR converts this to work with the new refactoring.

Removed the ability to execute transactions without confirmation and instead all executions go through the same function execute_transaction
This allows us consolidate the critical areas where we might have to lock/unlock orders.
Additionally propagated the OrderInfoResponse for executions which was being discarded.
Updated test cases to use pending-orders
Also ported multi-insert and multi-remove over to Mysten infra, so no need to have them here.

How it works:

An order ord's objects can be locked or unlocked by the order if all of ord's input objects are already locked by ord or by no other order.

There are two phases to completing a transaction:
TX Order without confirmation: this locks the objects owned by ord if they are available for locking. However in case of an authority error the objects are unlocked.
If the order is successfully executed, the objects remain locked until confirmation.

Confirmation of TX order:
Here the objects are unlocked if tx is successful or if tx fails with errors having no side effects.
If the objects remain locked after this step, there is a critical error.

Next steps:

Clarify which errors from authorities should or should not lead to unlocking orders
Make pending_order table test & + test & unlock thread-safe

oxade · 2022-02-06T02:32:08Z

@lxfind please take a look to make sure this PR fits in the refactor flow.

fastpay_core/src/authority_aggregator.rs

oxade · 2022-02-06T02:39:51Z

fastpay_core/src/client.rs

+        // Which errors should be unlock on?
+        // TODO: define which errors we must not unlock from
+        // https://github.com/MystenLabs/fastnft/issues/346
+        if result.is_err() || with_confirmation {


I feel there are situations where an authority operation fails and we would rather keep the order locked.
How can we for sure determine which errors should unlock orders?

I have two questions about this:

If we keep the order locked, when would we unlock them?

Could you also write down a concrete scenario where things could go wrong if we always unlock here?

Up to the caller. They can re-submit and order. Or can call try_complete_pending

Situations where the true state of the order is not defined. Example quorum logic failure, network/comms failure, set_order_lock failure here https://github.com/MystenLabs/fastnft/blob/main/fastpay_core/src/authority/authority_store.rs#L289

These are just guesses of course. I could be wrong.

lxfind · 2022-02-07T03:46:40Z

Thanks for putting up the PR.
I did give this flag idea a thought, and I chose to not add this flag to merge the two code paths for the following reasons:

We don't know if transfer_to_fastx_unsafe_unconfirmed is actually useful/needed.
Even if (1) is necessary, so far, there is only one caller that's calling execute_transaction_without_confirmation_unsafe. I am not aware of any new callers being added.
So we could say that execute_transaction_without_confirmation_unsafe is an edge case at best. Adding a flag like this makes the code a lot more complex. Such complexity is not worth it for an edge case (this PR vs 2 lines of code in the original impl).
Of course, if we are convinced that this is not an edge case and we will be adding more callers to execute_transaction_without_confirmation_unsafe, I would totally agree that a unification of the code paths would be a good idea.

Let me what you think.

Also, @gdanezis Could you confirm how transfer_to_fastx_unsafe_unconfirmed would be used? And whether it would be a common need for the client to call into execute_transaction_without_confirmation_unsafe?

fastpay_core/src/authority_aggregator.rs

lxfind · 2022-02-07T05:43:12Z

fastpay_core/src/client.rs

+        // Which errors should be unlock on?
+        // TODO: define which errors we must not unlock from
+        // https://github.com/MystenLabs/fastnft/issues/346
+        if result.is_err() || with_confirmation {


I have two questions about this:

If we keep the order locked, when would we unlock them?

Could you also write down a concrete scenario where things could go wrong if we always unlock here?

lxfind · 2022-02-07T05:46:22Z

fastpay_core/src/client.rs

+        if !self.can_lock_or_unlock(order)? {
+            return Err(FastPayError::ConcurrentTransactionError);
+        }


Duplicates?

lxfind · 2022-02-07T05:47:28Z

fastpay_core/src/client.rs

-            &self.store.pending_orders,
-            order.input_objects().iter().map(|e| e.object_id()),
-        )
+        if !self.can_lock_or_unlock(order)? {


Curious, what happens if we fail to unlock here due to can_lock_or_unlock returns Err? Could the objects be locked forever?

May want to add a TODO here. I think we need to handle the failure of can_lock_or_unlock eventually.

oxade · 2022-02-07T13:45:47Z

Thanks for putting up the PR. I did give this flag idea a thought, and I chose to not add this flag to merge the two code paths for the following reasons:

We don't know if transfer_to_fastx_unsafe_unconfirmed is actually useful/needed.

Even if (1) is necessary, so far, there is only one caller that's calling execute_transaction_without_confirmation_unsafe. I am not aware of any new callers being added.
So we could say that execute_transaction_without_confirmation_unsafe is an edge case at best. Adding a flag like this makes the code a lot more complex. Such complexity is not worth it for an edge case (this PR vs 2 lines of code in the original impl).
Of course, if we are convinced that this is not an edge case and we will be adding more callers to execute_transaction_without_confirmation_unsafe, I would totally agree that a unification of the code paths would be a good idea.

Let me what you think.

Also, @gdanezis Could you confirm how transfer_to_fastx_unsafe_unconfirmed would be used? And whether it would be a common need for the client to call into execute_transaction_without_confirmation_unsafe?

Right. I want to get rid of the unconfirmed path too for the reasons you mentioned, and more. But while we have it, I'm striving to reduce ways it can be misused.

fastpay_core/src/authority_aggregator.rs

fastpay_core/src/client.rs

lxfind · 2022-02-07T22:47:54Z

fastpay_core/src/client.rs

-            &self.store.pending_orders,
-            order.input_objects().iter().map(|e| e.object_id()),
-        )
+        if !self.can_lock_or_unlock(order)? {


May want to add a TODO here. I think we need to handle the failure of can_lock_or_unlock eventually.

fastpay_core/src/client.rs

lxfind · 2022-02-08T00:09:22Z

fastpay_core/src/authority_aggregator.rs

+            .iter()
+            .map(|vote| {
+                (
+                    vote.signed_order.as_ref().unwrap().authority,
+                    vote.signed_order.as_ref().unwrap().signature,
+                )
+            })
+            .collect::<Vec<_>>();
+
+        let certificate = CertifiedOrder { order, signatures };


Curious why do we have to do this here instead of keeping the old logic (match returns the pairs)?

The responses from the old code will always be empty because it's the output of the catch-up confirmation steps, which tx_order does not do anymore. It's a bit misleading.

lxfind

Approving but some of the code will go away in latter PRs from @gdanezis (may want to take a look at his PR to make sure we are porting changes from this PR)

oxade · 2022-02-08T00:38:44Z

Approving but some of the code will go away in latter PRs from @gdanezis (may want to take a look at his PR to make sure we are porting changes from this PR)

Agreed. Many of the concepts here are based on logic that will go away soon, however I'm not sure of the timelines, so I'm hoping these incremental steps can keep us moving at least.

9f662f1f6d7711c6545519c8b85996d515d3f104 removed a proptest non-regression file that exercises #375. We reinstate it and un-ignore the test.

Fixes #375

9f662f1f6d7711c6545519c8b85996d515d3f104 removed a proptest non-regression file that exercises MystenLabs#375. We reinstate it and un-ignore the test.

Fixes MystenLabs#375

oxade added 2 commits February 5, 2022 21:10

Unify tx execution and cleanup storage fns

8ee1c91

Clippy

5f90762

oxade requested a review from lxfind February 6, 2022 02:30

oxade commented Feb 6, 2022

View reviewed changes

fastpay_core/src/authority_aggregator.rs Show resolved Hide resolved

oxade requested review from sblackshear, gdanezis, arun-koshy, huitseeker and patrickkuo February 6, 2022 02:34

oxade commented Feb 6, 2022

View reviewed changes

lxfind reviewed Feb 7, 2022

View reviewed changes

oxade added 5 commits February 7, 2022 15:33

Removed uncomfirmed tx path

77d6820

Removed uncomfirmed tx path

0f648ba

synced to main

808cbaa

synced to main

f75a1aa

synced to main

959ba65

lxfind reviewed Feb 7, 2022

View reviewed changes

oxade added 2 commits February 7, 2022 18:24

Added more comments

2e04a66

Revert fn defintion

ac68223

lxfind reviewed Feb 8, 2022

View reviewed changes

lxfind approved these changes Feb 8, 2022

View reviewed changes

oxade merged commit 82f1283 into main Feb 8, 2022

oxade deleted the enhance/cleanup-unconfirmed-execution branch February 8, 2022 00:38

lxfind mentioned this pull request Feb 8, 2022

[fastx client] Correct generic processing of orders and certificates. (Part 1) #378

Merged

This was referenced Feb 8, 2022

[client] Implement safe pending-order check to prevent equivocation and deadlocks #335

Closed

Enhanced pending orders logic #352

Closed

mwtian pushed a commit that referenced this pull request Sep 12, 2022

revert(dag): reinstate proptest regression checks

a152f52

9f662f1f6d7711c6545519c8b85996d515d3f104 removed a proptest non-regression file that exercises #375. We reinstate it and un-ignore the test.

mwtian pushed a commit that referenced this pull request Sep 12, 2022

fix: relax bound checked in probabilistic check

348b199

Fixes #375

mwtian pushed a commit to mwtian/sui that referenced this pull request Sep 29, 2022

revert(dag): reinstate proptest regression checks

5ca065d

9f662f1f6d7711c6545519c8b85996d515d3f104 removed a proptest non-regression file that exercises MystenLabs#375. We reinstate it and un-ignore the test.

mwtian pushed a commit to mwtian/sui that referenced this pull request Sep 29, 2022

fix: relax bound checked in probabilistic check

40c3bfb

Fixes MystenLabs#375

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 Feb 4, 2023

[Snyk] Upgrade webextension-polyfill from 0.9.0 to 0.10.0 DOGECOIN87/sui-new#4

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance/cleanup unconfirmed execution #375

Enhance/cleanup unconfirmed execution #375

oxade commented Feb 6, 2022 •

edited

Loading

oxade commented Feb 6, 2022

oxade Feb 6, 2022

lxfind Feb 7, 2022

oxade Feb 7, 2022

lxfind commented Feb 7, 2022

lxfind Feb 7, 2022

lxfind Feb 7, 2022

lxfind Feb 7, 2022

lxfind Feb 7, 2022

oxade commented Feb 7, 2022

lxfind Feb 7, 2022

lxfind Feb 8, 2022

oxade Feb 8, 2022

lxfind left a comment

oxade commented Feb 8, 2022

Enhance/cleanup unconfirmed execution #375

Enhance/cleanup unconfirmed execution #375

Conversation

oxade commented Feb 6, 2022 • edited Loading

How it works:

oxade commented Feb 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lxfind commented Feb 7, 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

oxade commented Feb 7, 2022

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

oxade commented Feb 8, 2022

oxade commented Feb 6, 2022 •

edited

Loading