Improve clarity of errors for QuorumDriverError type and it's usage i…

…n RPCs (MystenLabs#19258)

## Description 
The CLI and SDK rely on the QuorumDriverError type to handle errors from
the Quorum driver. This PR improves the clarity of errors for the
QuorumDriverError type and its usage in RPCs.
- a published transaction which has a conflict now provides the
conflicting transaction
- invalid user signature simplified 
- better formatting for object double used
- rely on display when possible

## Test plan 
Augment the current RPC error tests surrounding
`From<QuorumDriverError>` trait implementations to ensure that the error
messages are clear and informative.

## Release notes
- [ ] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] Indexer: 
- [ ] JSON-RPC:
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:
- [ ] REST API:

crates/sui-core/src/quorum_driver/mod.rs

@@ -354,8 +354,7 @@ where
                 );
                 Err(Some(QuorumDriverError::ObjectsDoubleUsed {
                     conflicting_txes: conflicting_tx_digests,
-                    retried_tx: None,
-                    retried_tx_success: None,
+                    retried_tx_status: None,
                 }))
             }
 
@@ -450,8 +449,7 @@ where
                 );
                 Err(Some(QuorumDriverError::ObjectsDoubleUsed {
                     conflicting_txes: conflicting_tx_digests,
-                    retried_tx: None,
-                    retried_tx_success: None,
+                    retried_tx_status: None,
                 }))
             }
             Ok(success) => {
@@ -468,8 +466,7 @@ where
                 }
                 Err(Some(QuorumDriverError::ObjectsDoubleUsed {
                     conflicting_txes: conflicting_tx_digests,
-                    retried_tx: Some(conflicting_tx_digest),
-                    retried_tx_success: Some(success),
+                    retried_tx_status: Some((conflicting_tx_digest, success)),
                 }))
             }
         }

crates/sui-core/src/quorum_driver/tests.rs

@@ -296,12 +296,10 @@ async fn test_quorum_driver_object_locked() -> Result<(), anyhow::Error> {
     // double spend and this is a fatal error.
     if let Err(QuorumDriverError::ObjectsDoubleUsed {
         conflicting_txes,
-        retried_tx,
-        retried_tx_success,
+        retried_tx_status,
     }) = res
     {
-        assert_eq!(retried_tx, None);
-        assert_eq!(retried_tx_success, None);
+        assert_eq!(retried_tx_status, None);
         assert_eq!(conflicting_txes.len(), 1);
         assert_eq!(conflicting_txes.iter().next().unwrap().0, tx.digest());
     } else {
@@ -338,12 +336,10 @@ async fn test_quorum_driver_object_locked() -> Result<(), anyhow::Error> {
     // Aggregator gets three bad responses, and tries tx, which should succeed.
     if let Err(QuorumDriverError::ObjectsDoubleUsed {
         conflicting_txes,
-        retried_tx,
-        retried_tx_success,
+        retried_tx_status,
     }) = res
     {
-        assert_eq!(retried_tx, Some(*tx.digest()));
-        assert_eq!(retried_tx_success, Some(true));
+        assert_eq!(retried_tx_status, Some((*tx.digest(), true)));
         assert_eq!(conflicting_txes.len(), 1);
         assert_eq!(conflicting_txes.iter().next().unwrap().0, tx.digest());
     } else {
@@ -403,12 +399,10 @@ async fn test_quorum_driver_object_locked() -> Result<(), anyhow::Error> {
 
     if let Err(QuorumDriverError::ObjectsDoubleUsed {
         conflicting_txes,
-        retried_tx,
-        retried_tx_success,
+        retried_tx_status,
     }) = res
     {
-        assert_eq!(retried_tx, None);
-        assert_eq!(retried_tx_success, None);
+        assert_eq!(retried_tx_status, None);
         assert_eq!(conflicting_txes.len(), 2);
         let tx_stake = conflicting_txes.get(tx.digest()).unwrap().1;
         assert!(tx_stake == 2500 || tx_stake == 5000);
@@ -444,12 +438,10 @@ async fn test_quorum_driver_object_locked() -> Result<(), anyhow::Error> {
 
     if let Err(QuorumDriverError::ObjectsDoubleUsed {
         conflicting_txes,
-        retried_tx,
-        retried_tx_success,
+        retried_tx_status,
     }) = res
     {
-        assert_eq!(retried_tx, None);
-        assert_eq!(retried_tx_success, None);
+        assert_eq!(retried_tx_status, None);
         assert_eq!(conflicting_txes.len(), 1);
         assert_eq!(conflicting_txes.get(tx.digest()).unwrap().1, 5000);
     } else {
@@ -515,12 +507,10 @@ async fn test_quorum_driver_object_locked() -> Result<(), anyhow::Error> {
 
     if let Err(QuorumDriverError::ObjectsDoubleUsed {
         conflicting_txes,
-        retried_tx,
-        retried_tx_success,
+        retried_tx_status,
     }) = res
     {
-        assert_eq!(retried_tx, None);
-        assert_eq!(retried_tx_success, None);
+        assert_eq!(retried_tx_status, None);
         assert!(conflicting_txes.len() == 3 || conflicting_txes.len() == 2);
         assert!(conflicting_txes
             .iter()

crates/sui-json-rpc/src/error.rs

@@ -1,6 +1,8 @@
 // Copyright (c) Mysten Labs, Inc.
 // SPDX-License-Identifier: Apache-2.0
 
+use crate::authority_state::StateReadError;
+use crate::name_service::NameServiceError;
 use fastcrypto::error::FastCryptoError;
 use hyper::header::InvalidHeaderValue;
 use itertools::Itertools;
@@ -9,14 +11,12 @@ use jsonrpsee::types::error::{CallError, INTERNAL_ERROR_CODE};
 use jsonrpsee::types::ErrorObject;
 use std::collections::BTreeMap;
 use sui_json_rpc_api::{TRANSACTION_EXECUTION_CLIENT_ERROR_CODE, TRANSIENT_ERROR_CODE};
+use sui_types::committee::{QUORUM_THRESHOLD, TOTAL_VOTING_POWER};
 use sui_types::error::{SuiError, SuiObjectResponseError, UserInputError};
 use sui_types::quorum_driver_types::QuorumDriverError;
 use thiserror::Error;
 use tokio::task::JoinError;
 
-use crate::authority_state::StateReadError;
-use crate::name_service::NameServiceError;
-
 pub type RpcInterimResult<T = ()> = Result<T, Error>;
 
 #[derive(Debug, Error)]
@@ -133,17 +133,9 @@ impl From<Error> for RpcError {
             Error::QuorumDriverError(err) => {
                 match err {
                     QuorumDriverError::InvalidUserSignature(err) => {
-                        let inner_error_str = match err {
-                            // TODO(wlmyng): update SuiError display trait to render UserInputError with display
-                            SuiError::UserInputError { error } => error.to_string(),
-                            _ => err.to_string(),
-                        };
-
-                        let error_message = format!("Invalid user signature: {inner_error_str}");
-
                         let error_object = ErrorObject::owned(
                             TRANSACTION_EXECUTION_CLIENT_ERROR_CODE,
-                            error_message,
+                            format!("Invalid user signature: {err}"),
                             None::<()>,
                         );
                         RpcError::Call(CallError::Custom(error_object))
@@ -164,14 +156,44 @@ impl From<Error> for RpcError {
                     }
                     QuorumDriverError::ObjectsDoubleUsed {
                         conflicting_txes,
-                        retried_tx,
-                        retried_tx_success,
+                        retried_tx_status,
                     } => {
+                        let weights: Vec<u64> =
+                            conflicting_txes.values().map(|(_, stake)| *stake).collect();
+                        let remaining: u64 = TOTAL_VOTING_POWER - weights.iter().sum::<u64>();
+
+                        // better version of above
+                        let reason = if weights.iter().all(|w| remaining + w < QUORUM_THRESHOLD) {
+                            "equivocated until the next epoch"
+                        } else {
+                            "reserved for another transaction"
+                        };
+
+                        let retried_info = match retried_tx_status {
+                            Some((digest, success)) => {
+                                format!(
+                                    "Retried transaction {} ({}) because it was able to gather the necessary votes.",
+                                    digest, if success { "succeeded" } else { "failed" }
+                                )
+                            }
+                            None => "".to_string(),
+                        };
+
                         let error_message = format!(
-                        "Failed to sign transaction by a quorum of validators because of locked objects. Retried a conflicting transaction {:?}, success: {:?}",
-                        retried_tx,
-                        retried_tx_success
-                    );
+                            "Failed to sign transaction by a quorum of validators because one or more of its objects is {}. {} Other transactions locking these objects:\n{}",
+                            reason,
+                            retried_info,
+                            conflicting_txes
+                                .iter()
+                                .sorted_by(|(_, (_, a)), (_, (_, b))| b.cmp(a))
+                                .map(|(digest, (_, stake))| format!(
+                                    "- {} (stake {}.{})",
+                                    digest,
+                                    stake / 100,
+                                    stake % 100,
+                                ))
+                                .join("\n"),
+                        );
 
                         let new_map = conflicting_txes
                             .into_iter()
@@ -224,8 +246,13 @@ impl From<Error> for RpcError {
                             "NonRecoverableTransactionError should have at least one non-retryable error"
                         );
 
-                        let error_list = new_errors.join(", ");
-                        let error_msg = format!("Transaction execution failed due to issues with transaction inputs, please review the errors and try again: {}.", error_list);
+                        let mut error_list = vec![];
+
+                        for err in new_errors.iter() {
+                            error_list.push(format!("- {}", err));
+                        }
+
+                        let error_msg = format!("Transaction execution failed due to issues with transaction inputs, please review the errors and try again:\n{}", error_list.join("\n"));
 
                         let error_object = ErrorObject::owned(
                             TRANSACTION_EXECUTION_CLIENT_ERROR_CODE,
@@ -382,27 +409,75 @@ mod tests {
                 TransactionDigest,
                 (Vec<(AuthorityName, ObjectRef)>, StakeUnit),
             > = BTreeMap::new();
-            let tx_digest = TransactionDigest::default();
+            let tx_digest = TransactionDigest::from([1; 32]);
             let object_ref = test_object_ref();
-            let stake_unit: StakeUnit = 10;
+
+            // 4vJ9JU1bJJE96FWSJKvHsmmFADCg4gpZQff4P3bkLKi has enough stake to escape equivocation
+            let stake_unit: StakeUnit = 8000;
             let authority_name = AuthorityPublicKeyBytes([0; AuthorityPublicKey::LENGTH]);
             conflicting_txes.insert(tx_digest, (vec![(authority_name, object_ref)], stake_unit));
 
+            // 8qbHbw2BbbTHBW1sbeqakYXVKRQM8Ne7pLK7m6CVfeR stake below quorum threshold
+            let tx_digest = TransactionDigest::from([2; 32]);
+            let stake_unit: StakeUnit = 500;
+            let authority_name = AuthorityPublicKeyBytes([1; AuthorityPublicKey::LENGTH]);
+            conflicting_txes.insert(tx_digest, (vec![(authority_name, object_ref)], stake_unit));
+
+            let quorum_driver_error = QuorumDriverError::ObjectsDoubleUsed {
+                conflicting_txes,
+                retried_tx_status: Some((TransactionDigest::from([1; 32]), true)),
+            };
+
+            let rpc_error: RpcError = Error::QuorumDriverError(quorum_driver_error).into();
+
+            let error_object: ErrorObjectOwned = rpc_error.into();
+            let expected_code = expect!["-32002"];
+            expected_code.assert_eq(&error_object.code().to_string());
+            let expected_message = expect!["Failed to sign transaction by a quorum of validators because one or more of its objects is reserved for another transaction. Retried transaction 4vJ9JU1bJJE96FWSJKvHsmmFADCg4gpZQff4P3bkLKi (succeeded) because it was able to gather the necessary votes. Other transactions locking these objects:\n- 4vJ9JU1bJJE96FWSJKvHsmmFADCg4gpZQff4P3bkLKi (stake 80.0)\n- 8qbHbw2BbbTHBW1sbeqakYXVKRQM8Ne7pLK7m6CVfeR (stake 5.0)"];
+            expected_message.assert_eq(error_object.message());
+            let expected_data = expect![[
+                r#"{"4vJ9JU1bJJE96FWSJKvHsmmFADCg4gpZQff4P3bkLKi":[["0x0000000000000000000000000000000000000000000000000000000000000000",0,"11111111111111111111111111111111"]],"8qbHbw2BbbTHBW1sbeqakYXVKRQM8Ne7pLK7m6CVfeR":[["0x0000000000000000000000000000000000000000000000000000000000000000",0,"11111111111111111111111111111111"]]}"#
+            ]];
+            let actual_data = error_object.data().unwrap().to_string();
+            expected_data.assert_eq(&actual_data);
+        }
+
+        #[test]
+        fn test_objects_double_used_equivocated() {
+            use sui_types::crypto::VerifyingKey;
+            let mut conflicting_txes: BTreeMap<
+                TransactionDigest,
+                (Vec<(AuthorityName, ObjectRef)>, StakeUnit),
+            > = BTreeMap::new();
+            let tx_digest = TransactionDigest::from([1; 32]);
+            let object_ref = test_object_ref();
+
+            // 4vJ9JU1bJJE96FWSJKvHsmmFADCg4gpZQff4P3bkLKi has lower stake at 10
+            let stake_unit: StakeUnit = 4000;
+            let authority_name = AuthorityPublicKeyBytes([0; AuthorityPublicKey::LENGTH]);
+            conflicting_txes.insert(tx_digest, (vec![(authority_name, object_ref)], stake_unit));
+
+            // 8qbHbw2BbbTHBW1sbeqakYXVKRQM8Ne7pLK7m6CVfeR is a higher stake and should be first in the list
+            let tx_digest = TransactionDigest::from([2; 32]);
+            let stake_unit: StakeUnit = 5000;
+            let authority_name = AuthorityPublicKeyBytes([1; AuthorityPublicKey::LENGTH]);
+            conflicting_txes.insert(tx_digest, (vec![(authority_name, object_ref)], stake_unit));
+
             let quorum_driver_error = QuorumDriverError::ObjectsDoubleUsed {
                 conflicting_txes,
-                retried_tx: Some(TransactionDigest::default()),
-                retried_tx_success: Some(true),
+                // below threshold
+                retried_tx_status: None,
             };
 
             let rpc_error: RpcError = Error::QuorumDriverError(quorum_driver_error).into();
 
             let error_object: ErrorObjectOwned = rpc_error.into();
             let expected_code = expect!["-32002"];
             expected_code.assert_eq(&error_object.code().to_string());
-            let expected_message = expect!["Failed to sign transaction by a quorum of validators because of locked objects. Retried a conflicting transaction Some(TransactionDigest(11111111111111111111111111111111)), success: Some(true)"];
+            let expected_message = expect!["Failed to sign transaction by a quorum of validators because one or more of its objects is equivocated until the next epoch.  Other transactions locking these objects:\n- 8qbHbw2BbbTHBW1sbeqakYXVKRQM8Ne7pLK7m6CVfeR (stake 50.0)\n- 4vJ9JU1bJJE96FWSJKvHsmmFADCg4gpZQff4P3bkLKi (stake 40.0)"];
             expected_message.assert_eq(error_object.message());
             let expected_data = expect![[
-                r#"{"11111111111111111111111111111111":[["0x0000000000000000000000000000000000000000000000000000000000000000",0,"11111111111111111111111111111111"]]}"#
+                r#"{"4vJ9JU1bJJE96FWSJKvHsmmFADCg4gpZQff4P3bkLKi":[["0x0000000000000000000000000000000000000000000000000000000000000000",0,"11111111111111111111111111111111"]],"8qbHbw2BbbTHBW1sbeqakYXVKRQM8Ne7pLK7m6CVfeR":[["0x0000000000000000000000000000000000000000000000000000000000000000",0,"11111111111111111111111111111111"]]}"#
             ]];
             let actual_data = error_object.data().unwrap().to_string();
             expected_data.assert_eq(&actual_data);
@@ -441,7 +516,7 @@ mod tests {
             let expected_code = expect!["-32002"];
             expected_code.assert_eq(&error_object.code().to_string());
             let expected_message =
-                expect!["Transaction execution failed due to issues with transaction inputs, please review the errors and try again: Balance of gas object 10 is lower than the needed amount: 100, Object (0x0000000000000000000000000000000000000000000000000000000000000000, SequenceNumber(0), o#11111111111111111111111111111111) is not available for consumption, its current version: SequenceNumber(10)."];
+                expect!["Transaction execution failed due to issues with transaction inputs, please review the errors and try again:\n- Balance of gas object 10 is lower than the needed amount: 100\n- Object ID 0x0000000000000000000000000000000000000000000000000000000000000000 Version 0x0 Digest 11111111111111111111111111111111 is not available for consumption, current version: 0xa"];
             expected_message.assert_eq(error_object.message());
         }
 
@@ -473,7 +548,7 @@ mod tests {
             let expected_code = expect!["-32002"];
             expected_code.assert_eq(&error_object.code().to_string());
             let expected_message =
-                expect!["Transaction execution failed due to issues with transaction inputs, please review the errors and try again: Could not find the referenced object 0x0000000000000000000000000000000000000000000000000000000000000000 at version None."];
+                expect!["Transaction execution failed due to issues with transaction inputs, please review the errors and try again:\n- Could not find the referenced object 0x0000000000000000000000000000000000000000000000000000000000000000 at version None"];
             expected_message.assert_eq(error_object.message());
         }
 

crates/sui-rest-api/src/error.rs

@@ -70,8 +70,7 @@ impl From<sui_types::quorum_driver_types::QuorumDriverError> for RestError {
             }
             ObjectsDoubleUsed {
                 conflicting_txes,
-                retried_tx,
-                retried_tx_success,
+                retried_tx_status,
             } => {
                 let new_map = conflicting_txes
                     .into_iter()
@@ -85,8 +84,8 @@ impl From<sui_types::quorum_driver_types::QuorumDriverError> for RestError {
 
                 let message = format!(
                         "Failed to sign transaction by a quorum of validators because of locked objects. Retried a conflicting transaction {:?}, success: {:?}. Conflicting Transactions:\n{:#?}",
-                        retried_tx,
-                        retried_tx_success,
+                        retried_tx_status.map(|(tx, _)| tx),
+                        retried_tx_status.map(|(_, success)| success),
                         new_map,
                     );
 

crates/sui-types/src/error.rs

@@ -98,7 +98,10 @@ pub enum UserInputError {
         object_id: ObjectID,
         version: Option<SequenceNumber>,
     },
-    #[error("Object {provided_obj_ref:?} is not available for consumption, its current version: {current_version:?}")]
+    #[error(
+        "Object ID {} Version {} Digest {} is not available for consumption, current version: {current_version}",
+        .provided_obj_ref.0, .provided_obj_ref.1, .provided_obj_ref.2
+    )]
     ObjectVersionUnavailableForConsumption {
         provided_obj_ref: ObjectRef,
         current_version: SequenceNumber,

crates/sui-types/src/quorum_driver_types.rs

@@ -31,20 +31,19 @@ pub const NON_RECOVERABLE_ERROR_MSG: &str =
 /// Every invariant needs detailed documents to instruct client handling.
 #[derive(Eq, PartialEq, Clone, Debug, Serialize, Deserialize, Error, Hash, AsRefStr)]
 pub enum QuorumDriverError {
-    #[error("QuorumDriver internal error: {0:?}.")]
+    #[error("QuorumDriver internal error: {0}.")]
     QuorumDriverInternalError(SuiError),
-    #[error("Invalid user signature: {0:?}.")]
+    #[error("Invalid user signature: {0}.")]
     InvalidUserSignature(SuiError),
     #[error(
         "Failed to sign transaction by a quorum of validators because of locked objects: {:?}, retried a conflicting transaction {:?}, success: {:?}",
         conflicting_txes,
-        retried_tx,
-        retried_tx_success
+        .retried_tx_status.map(|(tx, success)| tx),
+        .retried_tx_status.map(|(tx, success)| success),
     )]
     ObjectsDoubleUsed {
         conflicting_txes: BTreeMap<TransactionDigest, (Vec<(AuthorityName, ObjectRef)>, StakeUnit)>,
-        retried_tx: Option<TransactionDigest>,
-        retried_tx_success: Option<bool>,
+        retried_tx_status: Option<(TransactionDigest, bool)>,
     },
     #[error("Transaction timed out before reaching finality")]
     TimeoutBeforeFinality,