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

@@ -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);
+        let result = process_command(&config);
-        assert_eq!(signature.unwrap(), SIGNATURE.to_string());
+        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);
+        let result = process_command(&config);
-        assert_eq!(signature.unwrap(), SIGNATURE.to_string());
+        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);
+        let result = process_command(&config);
-        assert_eq!(signature.unwrap(), SIGNATURE.to_string());
+        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);
+        let result = process_command(&config);
-        assert_eq!(signature.unwrap(), SIGNATURE.to_string());
+        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);
+        let result = process_command(&config);
-        assert_eq!(signature.unwrap(), SIGNATURE.to_string());
+        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);
+        let result = process_command(&config);
-        assert_eq!(signature.unwrap(), SIGNATURE.to_string());
+        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);
+        let result = process_command(&config);
-        assert_eq!(signature.unwrap(), SIGNATURE.to_string());
+        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);
+        let result = process_command(&config);
-        assert_eq!(signature.unwrap(), SIGNATURE.to_string());
+        assert!(result.is_ok());
 
         let date_string = "\"2018-09-19T17:30:59Z\"";
         let dt: DateTime<Utc> = 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!(result.is_ok());
-        assert_eq!(
-            json.as_object()
-                .unwrap()
-                .get("signature")
-                .unwrap()
-                .as_str()
-                .unwrap(),
-            SIGNATURE.to_string()
-        );
 
         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!(result.is_ok());
-        assert_eq!(
-            json.as_object()
-                .unwrap()
-                .get("signature")
-                .unwrap()
-                .as_str()
-                .unwrap(),
-            SIGNATURE.to_string()
-        );
 
         let process_id = Pubkey::new_rand();
         config.command = CliCommand::TimeElapsed(bob_pubkey, process_id, dt);
         config.signers = vec![&keypair];
-        let signature = process_command(&config);
+        let result = process_command(&config);
-        assert_eq!(signature.unwrap(), SIGNATURE.to_string());
+        assert!(result.is_ok());
 
         let witness = Pubkey::new_rand();
         config.command = CliCommand::Witness(bob_pubkey, witness);
-        let signature = process_command(&config);
+        let result = process_command(&config);
-        assert_eq!(signature.unwrap(), SIGNATURE.to_string());
+        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);
+        let result = process_command(&config);
-        assert_eq!(signature.unwrap(), SIGNATURE.to_string());
+        assert!(result.is_ok());
 
         let witness = Pubkey::new_rand();
         config.command = CliCommand::Witness(bob_pubkey, witness);
-        let signature = process_command(&config);
+        let result = process_command(&config);
-        assert_eq!(signature.unwrap(), SIGNATURE.to_string());
+        assert!(result.is_ok());
 
         // sig_not_found case
         config.rpc_client = Some(RpcClient::new_mock("sig_not_found".to_string()));

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<serde_json::Value> {
         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,
         };

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
+            Some(signature_base58_str) => {
-                .parse::<Signature>()
+                let signature = signature_base58_str.parse::<Signature>().map_err(|err| {
-                .map_err(|err| RpcError::ParseError(err.to_string()).into()),
+                    Into::<ClientError>::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<Vec<Option<TransactionStatus>>> = 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::{
+    use crate::{client_error::ClientErrorKind, mock_rpc_client_request::PUBKEY};
-        client_error::ClientErrorKind,
-        mock_rpc_client_request::{PUBKEY, SIGNATURE},
-    };
     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() {