[fastx client] Better Error Handling & Messaging - Part 1 #256

patrickkuo · 2022-01-24T14:47:06Z

This PR address issue #187

This is part one of error handling improvement, covers client apis excluding move related apis.

Replaced bail! and ensures with FastPayError objects
improved some error display
added more informations to errors (e.g. object_id)

patrickkuo · 2022-01-24T19:13:22Z

fastx_types/src/error.rs

@@ -171,3 +178,15 @@ impl std::convert::From<PartialVMError> for FastPayError {
        }
    }
 }
+
+// Assuming all errors are FastPayError, wrap all other errors in FastPayError::UnknownError
+impl From<anyhow::Error> for FastPayError {


is this ok? @huitseeker

This is to allow method with return type Result<_,FastPayError> to use ? on methods returning Result<_, anyhow::Error>

It is coercing a hue range of errors to a perhaps not meaningful FastPayError::UnknownError. Can you point me to a place (or better, places) where this is used?

it is used in broadcast_and_execute line 486, the method returns FastPayError and inline 561 handle.stop().await? returns anyhow::Error.

Thinking more about this, It's probably better to change the method signature back to return anyhow::Error and wrap the FastPayError.

huitseeker · 2022-01-25T15:44:19Z

fastx_types/src/error.rs

+        match e.downcast_ref::<FastPayError>() {
+            Some(e) => e.clone(),


This will never fire, look instead at the chain fn on anyhow::Error if you want to do this sort of thing.

huitseeker · 2022-01-25T15:47:52Z

fastx_types/src/error.rs

@@ -171,3 +178,15 @@ impl std::convert::From<PartialVMError> for FastPayError {
        }
    }
 }
+
+// Assuming all errors are FastPayError, wrap all other errors in FastPayError::UnknownError
+impl From<anyhow::Error> for FastPayError {


It is coercing a hue range of errors to a perhaps not meaningful FastPayError::UnknownError. Can you point me to a place (or better, places) where this is used?

patrickkuo · 2022-01-25T19:33:03Z

I am working on refactoring the client, to make object_transfer and move_call / publish to share the same cert synchonisation code path, there are a lot of duplicated code at the moment. Will continue error handling improvement part 2 after the refactoring.

* bug fix: execute_transfer will not be able to execute twice if previous request failed, causing `pending_transfer` to stick.

…ransfer_object * Added From<anyhow::Error> for FastPayError * some refactoring

* new test for receive_object * new error for incorrect recipient

* check certificate order signature before receive_object

gdanezis

Nice one! A few places where we can add more or less info to the errors.

gdanezis · 2022-01-26T15:00:10Z

fastpay_core/src/authority.rs

            fp_ensure!(
                object.digest() == object_digest,
-                FastPayError::InvalidObjectDigest
+                FastPayError::InvalidObjectDigest { object_id }


Should we indicate what the expected digest was here?

gdanezis · 2022-01-26T15:02:55Z

fastpay_core/src/client.rs

-                        return Err(FastPayError::FailedToCommunicateWithQuorum {
-                            err: err.to_string(),
+                        return Err(FastPayError::QuorumNotReachedError {
+                            num_of_authorities: number_of_authorities,


Here you seem to indicate the number of authorities in relation to the quorum. We often present protocol as having 3f+1 authorities and needed 2f+1 votes to get a quorum. However, in our actual protocol we have stake (with diff authorities having different stake) and expect 2/3 +1 of stake to get a quorum. So the number of authorities, wihout reference to their identity or stake is not really informative.

gdanezis · 2022-01-26T15:03:11Z

fastpay_core/src/client.rs

-        Err(FastPayError::FailedToCommunicateWithQuorum {
-            err: "multiple errors".to_string(),
+        Err(FastPayError::QuorumNotReachedError {
+            num_of_authorities: number_of_authorities,


Same as above ...

gdanezis · 2022-01-26T15:04:40Z

fastpay_core/src/client.rs

+            FastPayError::UnexpectedSequenceNumber {
+                object_id: *order.object_id(),
+                expected_sequence: self.next_sequence_number(order.object_id())?,
+                received_sequence: order.sequence_number()


We should probably be consistent to either report what was expected, what was provided or both. I think that what was expected is the right approach, since what was provided should be known.

patrickkuo added this to the Milestone 3 milestone Jan 24, 2022

patrickkuo self-assigned this Jan 24, 2022

patrickkuo force-pushed the pat/client-error-handling branch from ffa6375 to d3f8412 Compare January 24, 2022 18:46

patrickkuo commented Jan 24, 2022

View reviewed changes

patrickkuo force-pushed the pat/client-error-handling branch from 5a8479b to 2892de7 Compare January 25, 2022 13:32

huitseeker reviewed Jan 25, 2022

View reviewed changes

patrickkuo requested review from sblackshear, huitseeker, gdanezis, arun-koshy, awelc, lxfind and oxade January 25, 2022 19:31

patrickkuo marked this pull request as ready for review January 25, 2022 19:33

patrickkuo force-pushed the pat/client-error-handling branch from 3be5a54 to e09e762 Compare January 26, 2022 00:45

patrickkuo added 8 commits January 26, 2022 13:07

communicate_with_quorum returns FastPayError instead of str anyhow error

cbf12f9

* add to test case

ac37455

* bug fix: execute_transfer will not be able to execute twice if previous request failed, causing `pending_transfer` to stick.

* added test for concurrent transfer and wrong sequence number from t…

33cb8a3

…ransfer_object * Added From<anyhow::Error> for FastPayError * some refactoring

fixup after rebase

5878be3

* added more info to ObjectNotFoundError

1f71b77

* new test for receive_object * new error for incorrect recipient

* added test for receiving tempered certificate

29043b6

* check certificate order signature before receive_object

* remove unknownError and imp From<anyhow::Error> for FastPayError

12552d1

* added more information for InvalidObjectDigest

87955be

patrickkuo force-pushed the pat/client-error-handling branch from e09e762 to 87955be Compare January 26, 2022 13:07

gdanezis approved these changes Jan 26, 2022

View reviewed changes

address PR comments

9ca309f

patrickkuo changed the title ~~[fastx client] Better Error Handling & Messaging~~ [fastx client] Better Error Handling & Messaging - Part 1 Jan 26, 2022

bug fixes

4ad31c3

patrickkuo merged commit ada53c7 into main Jan 26, 2022

huitseeker deleted the pat/client-error-handling branch February 2, 2022 13:05

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

update bench default max concurrent requests (#256)

92f8b55

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

update bench default max concurrent requests (MystenLabs#256)

5849ad9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fastx client] Better Error Handling & Messaging - Part 1 #256

[fastx client] Better Error Handling & Messaging - Part 1 #256

patrickkuo commented Jan 24, 2022 •

edited

Loading

patrickkuo Jan 24, 2022

patrickkuo Jan 24, 2022

huitseeker Jan 25, 2022

patrickkuo Jan 25, 2022

huitseeker Jan 25, 2022

huitseeker Jan 25, 2022

patrickkuo commented Jan 25, 2022

gdanezis left a comment •

edited

Loading

gdanezis Jan 26, 2022

gdanezis Jan 26, 2022

gdanezis Jan 26, 2022

gdanezis Jan 26, 2022

		match e.downcast_ref::<FastPayError>() {
		Some(e) => e.clone(),

[fastx client] Better Error Handling & Messaging - Part 1 #256

[fastx client] Better Error Handling & Messaging - Part 1 #256

Conversation

patrickkuo commented Jan 24, 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

patrickkuo commented Jan 25, 2022

gdanezis 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickkuo commented Jan 24, 2022 •

edited

Loading

gdanezis left a comment •

edited

Loading