From cd6dfa41d32b92383e4b05ec4dbeac23941e1c15 Mon Sep 17 00:00:00 2001 From: Jort Date: Thu, 12 Sep 2024 18:16:04 -0700 Subject: [PATCH] Improve clarity of errors for QuorumDriverError type and it's usage in RPCs (#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` 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 | 9 +- crates/sui-core/src/quorum_driver/tests.rs | 30 ++--- crates/sui-json-rpc/src/error.rs | 131 +++++++++++++++----- crates/sui-rest-api/src/error.rs | 7 +- crates/sui-types/src/error.rs | 5 +- crates/sui-types/src/quorum_driver_types.rs | 11 +- 6 files changed, 128 insertions(+), 65 deletions(-) diff --git a/crates/sui-core/src/quorum_driver/mod.rs b/crates/sui-core/src/quorum_driver/mod.rs index 94ffdfdbf6771..69923bac8c26e 100644 --- a/crates/sui-core/src/quorum_driver/mod.rs +++ b/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)), })) } } diff --git a/crates/sui-core/src/quorum_driver/tests.rs b/crates/sui-core/src/quorum_driver/tests.rs index 049fc9d0a8c9e..3c721d4667ef8 100644 --- a/crates/sui-core/src/quorum_driver/tests.rs +++ b/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() diff --git a/crates/sui-json-rpc/src/error.rs b/crates/sui-json-rpc/src/error.rs index da735fdc7fbae..f56d0cde9ebf4 100644 --- a/crates/sui-json-rpc/src/error.rs +++ b/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 = Result; #[derive(Debug, Error)] @@ -133,17 +133,9 @@ impl From 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 for RpcError { } QuorumDriverError::ObjectsDoubleUsed { conflicting_txes, - retried_tx, - retried_tx_success, + retried_tx_status, } => { + let weights: Vec = + conflicting_txes.values().map(|(_, stake)| *stake).collect(); + let remaining: u64 = TOTAL_VOTING_POWER - weights.iter().sum::(); + + // 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 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,16 +409,64 @@ 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(); @@ -399,10 +474,10 @@ mod tests { 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()); } diff --git a/crates/sui-rest-api/src/error.rs b/crates/sui-rest-api/src/error.rs index 3d8fab535a48b..51432315d0d37 100644 --- a/crates/sui-rest-api/src/error.rs +++ b/crates/sui-rest-api/src/error.rs @@ -70,8 +70,7 @@ impl From 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 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, ); diff --git a/crates/sui-types/src/error.rs b/crates/sui-types/src/error.rs index 30578ecbc0d48..a1fc1f9a7ea56 100644 --- a/crates/sui-types/src/error.rs +++ b/crates/sui-types/src/error.rs @@ -98,7 +98,10 @@ pub enum UserInputError { object_id: ObjectID, version: Option, }, - #[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, diff --git a/crates/sui-types/src/quorum_driver_types.rs b/crates/sui-types/src/quorum_driver_types.rs index 252ede2c5b810..6a0b6e39d1a07 100644 --- a/crates/sui-types/src/quorum_driver_types.rs +++ b/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, StakeUnit)>, - retried_tx: Option, - retried_tx_success: Option, + retried_tx_status: Option<(TransactionDigest, bool)>, }, #[error("Transaction timed out before reaching finality")] TimeoutBeforeFinality,