From 6b988155e1378ad28ce6086b5aed61eea20ec854 Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Mon, 6 Apr 2020 19:27:37 -0600 Subject: [PATCH] RpcClient: include signature check in send_transaction, bump send retries in get_num_blocks_since_signature_confirmation (#9341) * Bump rpc send retries * Add signature check to send_transaction and update mocks to test --- cli/src/cli.rs | 72 ++++++++++----------------- client/src/mock_rpc_client_request.rs | 17 +++++-- client/src/rpc_client.rs | 35 +++++++++---- 3 files changed, 67 insertions(+), 57 deletions(-) diff --git a/cli/src/cli.rs b/cli/src/cli.rs index f286f2ed3..915497cdf 100644 --- a/cli/src/cli.rs +++ b/cli/src/cli.rs @@ -3182,7 +3182,7 @@ mod tests { let process_id = Pubkey::new_rand(); config.command = CliCommand::Cancel(process_id); - assert_eq!(process_command(&config).unwrap(), SIGNATURE); + assert!(process_command(&config).is_ok()); let good_signature = Signature::new(&bs58::decode(SIGNATURE).into_vec().unwrap()); config.command = CliCommand::Confirm(good_signature); @@ -3199,8 +3199,8 @@ mod tests { commission: 0, }; config.signers = vec![&keypair, &bob_keypair, &identity_keypair]; - let signature = process_command(&config); - assert_eq!(signature.unwrap(), SIGNATURE.to_string()); + let result = process_command(&config); + assert!(result.is_ok()); let new_authorized_pubkey = Pubkey::new_rand(); config.signers = vec![&bob_keypair]; @@ -3209,8 +3209,8 @@ mod tests { new_authorized_pubkey, vote_authorize: VoteAuthorize::Voter, }; - let signature = process_command(&config); - assert_eq!(signature.unwrap(), SIGNATURE.to_string()); + let result = process_command(&config); + assert!(result.is_ok()); let new_identity_keypair = Keypair::new(); config.signers = vec![&keypair, &bob_keypair, &new_identity_keypair]; @@ -3218,8 +3218,8 @@ mod tests { vote_account_pubkey: bob_pubkey, new_identity_account: 2, }; - let signature = process_command(&config); - assert_eq!(signature.unwrap(), SIGNATURE.to_string()); + let result = process_command(&config); + assert!(result.is_ok()); let bob_keypair = Keypair::new(); let bob_pubkey = bob_keypair.pubkey(); @@ -3243,8 +3243,8 @@ mod tests { from: 0, }; config.signers = vec![&keypair, &bob_keypair]; - let signature = process_command(&config); - assert_eq!(signature.unwrap(), SIGNATURE.to_string()); + let result = process_command(&config); + assert!(result.is_ok()); let stake_pubkey = Pubkey::new_rand(); let to_pubkey = Pubkey::new_rand(); @@ -3260,8 +3260,8 @@ mod tests { fee_payer: 0, }; config.signers = vec![&keypair]; - let signature = process_command(&config); - assert_eq!(signature.unwrap(), SIGNATURE.to_string()); + let result = process_command(&config); + assert!(result.is_ok()); let stake_pubkey = Pubkey::new_rand(); config.command = CliCommand::DeactivateStake { @@ -3273,8 +3273,8 @@ mod tests { nonce_authority: 0, fee_payer: 0, }; - let signature = process_command(&config); - assert_eq!(signature.unwrap(), SIGNATURE.to_string()); + let result = process_command(&config); + assert!(result.is_ok()); let stake_pubkey = Pubkey::new_rand(); let split_stake_account = Keypair::new(); @@ -3291,8 +3291,8 @@ mod tests { fee_payer: 0, }; config.signers = vec![&keypair, &split_stake_account]; - let signature = process_command(&config); - assert_eq!(signature.unwrap(), SIGNATURE.to_string()); + let result = process_command(&config); + assert!(result.is_ok()); config.command = CliCommand::GetSlot { commitment_config: CommitmentConfig::default(), @@ -3310,8 +3310,8 @@ mod tests { to: bob_pubkey, ..PayCommand::default() }); - let signature = process_command(&config); - assert_eq!(signature.unwrap(), SIGNATURE.to_string()); + let result = process_command(&config); + assert!(result.is_ok()); let date_string = "\"2018-09-19T17:30:59Z\""; let dt: DateTime = serde_json::from_str(&date_string).unwrap(); @@ -3323,16 +3323,7 @@ mod tests { ..PayCommand::default() }); let result = process_command(&config); - let json: Value = serde_json::from_str(&result.unwrap()).unwrap(); - assert_eq!( - json.as_object() - .unwrap() - .get("signature") - .unwrap() - .as_str() - .unwrap(), - SIGNATURE.to_string() - ); + assert!(result.is_ok()); let witness = Pubkey::new_rand(); config.command = CliCommand::Pay(PayCommand { @@ -3343,27 +3334,18 @@ mod tests { ..PayCommand::default() }); let result = process_command(&config); - let json: Value = serde_json::from_str(&result.unwrap()).unwrap(); - assert_eq!( - json.as_object() - .unwrap() - .get("signature") - .unwrap() - .as_str() - .unwrap(), - SIGNATURE.to_string() - ); + assert!(result.is_ok()); let process_id = Pubkey::new_rand(); config.command = CliCommand::TimeElapsed(bob_pubkey, process_id, dt); config.signers = vec![&keypair]; - let signature = process_command(&config); - assert_eq!(signature.unwrap(), SIGNATURE.to_string()); + let result = process_command(&config); + assert!(result.is_ok()); let witness = Pubkey::new_rand(); config.command = CliCommand::Witness(bob_pubkey, witness); - let signature = process_command(&config); - assert_eq!(signature.unwrap(), SIGNATURE.to_string()); + let result = process_command(&config); + assert!(result.is_ok()); // CreateAddressWithSeed let from_pubkey = Pubkey::new_rand(); @@ -3390,13 +3372,13 @@ mod tests { assert!(process_command(&config).is_ok()); config.command = CliCommand::TimeElapsed(bob_pubkey, process_id, dt); - let signature = process_command(&config); - assert_eq!(signature.unwrap(), SIGNATURE.to_string()); + let result = process_command(&config); + assert!(result.is_ok()); let witness = Pubkey::new_rand(); config.command = CliCommand::Witness(bob_pubkey, witness); - let signature = process_command(&config); - assert_eq!(signature.unwrap(), SIGNATURE.to_string()); + let result = process_command(&config); + assert!(result.is_ok()); // sig_not_found case config.rpc_client = Some(RpcClient::new_mock("sig_not_found".to_string())); diff --git a/client/src/mock_rpc_client_request.rs b/client/src/mock_rpc_client_request.rs index 69866fbe0..f05233913 100644 --- a/client/src/mock_rpc_client_request.rs +++ b/client/src/mock_rpc_client_request.rs @@ -8,7 +8,8 @@ use serde_json::{Number, Value}; use solana_sdk::{ fee_calculator::{FeeCalculator, FeeRateGovernor}, instruction::InstructionError, - transaction::{self, TransactionError}, + signature::Signature, + transaction::{self, Transaction, TransactionError}, }; use solana_transaction_status::TransactionStatus; use std::{collections::HashMap, sync::RwLock}; @@ -40,7 +41,7 @@ impl GenericRpcClientRequest for MockRpcClientRequest { fn send( &self, request: &RpcRequest, - _params: serde_json::Value, + params: serde_json::Value, _retries: usize, ) -> Result { if let Some(value) = self.mocks.write().unwrap().remove(request) { @@ -105,7 +106,17 @@ impl GenericRpcClientRequest for MockRpcClientRequest { } RpcRequest::GetTransactionCount => Value::Number(Number::from(1234)), RpcRequest::GetSlot => Value::Number(Number::from(0)), - RpcRequest::SendTransaction => Value::String(SIGNATURE.to_string()), + RpcRequest::SendTransaction => { + let signature = if self.url == "malicious" { + Signature::new(&[8; 64]).to_string() + } else { + let tx_str = params.as_array().unwrap()[0].as_str().unwrap().to_string(); + let data = bs58::decode(tx_str).into_vec().unwrap(); + let tx: Transaction = bincode::deserialize(&data).unwrap(); + tx.signatures[0].to_string() + }; + Value::String(signature) + } RpcRequest::GetMinimumBalanceForRentExemption => Value::Number(Number::from(1234)), _ => Value::Null, }; diff --git a/client/src/rpc_client.rs b/client/src/rpc_client.rs index a1080d166..02de64921 100644 --- a/client/src/rpc_client.rs +++ b/client/src/rpc_client.rs @@ -99,9 +99,24 @@ impl RpcClient { None => { Err(RpcError::ForUser("Received result of an unexpected type".to_string()).into()) } - Some(signature_base58_str) => signature_base58_str - .parse::() - .map_err(|err| RpcError::ParseError(err.to_string()).into()), + Some(signature_base58_str) => { + let signature = signature_base58_str.parse::().map_err(|err| { + Into::::into(RpcError::ParseError(err.to_string())) + })?; + // A mismatching RPC response signature indicates an issue with the RPC node, and + // should not be passed along to confirmation methods. The transaction may or may + // not have been submitted to the cluster, so callers should verify the success of + // the correct transaction signature independently. + if signature != transaction.signatures[0] { + Err(RpcError::RpcRequestError(format!( + "RPC node returned mismatched signature {:?}, expected {:?}", + signature, transaction.signatures[0] + )) + .into()) + } else { + Ok(transaction.signatures[0]) + } + } } } @@ -956,7 +971,7 @@ impl RpcClient { .send( &RpcRequest::GetSignatureStatuses, json!([[signature.to_string()]]), - 1, + 5, ) .map_err(|err| err.into_with_command("GetSignatureStatuses"))?; let result: Response>> = serde_json::from_value(response) @@ -1094,10 +1109,7 @@ pub fn get_rpc_request_str(rpc_addr: SocketAddr, tls: bool) -> String { #[cfg(test)] mod tests { use super::*; - use crate::{ - client_error::ClientErrorKind, - mock_rpc_client_request::{PUBKEY, SIGNATURE}, - }; + use crate::{client_error::ClientErrorKind, mock_rpc_client_request::PUBKEY}; use assert_matches::assert_matches; use jsonrpc_core::{Error, IoHandler, Params}; use jsonrpc_http_server::{AccessControlAllowOrigin, DomainsValidation, ServerBuilder}; @@ -1210,12 +1222,17 @@ mod tests { let tx = system_transaction::transfer(&key, &to, 50, blockhash); let signature = rpc_client.send_transaction(&tx); - assert_eq!(signature.unwrap(), SIGNATURE.parse().unwrap()); + assert_eq!(signature.unwrap(), tx.signatures[0]); let rpc_client = RpcClient::new_mock("fails".to_string()); let signature = rpc_client.send_transaction(&tx); assert!(signature.is_err()); + + // Test bad signature returned from rpc node + let rpc_client = RpcClient::new_mock("malicious".to_string()); + let signature = rpc_client.send_transaction(&tx); + assert!(signature.is_err()); } #[test] fn test_get_recent_blockhash() {