Change hash parameter of /tx RPC endpoint encoding to base64

.changelog/unreleased/breaking-changes/942-rpc-tx-hash-field.md

@@ -0,0 +1,3 @@
+- `[tendermint-rpc]` The `/tx` endpoint's `Request::hash` field has been changed
+  from `String` to `tendermint::abci::transaction::Hash`
+  ([#942](https://github.com/informalsystems/tendermint-rs/issues/942))

.changelog/unreleased/bug-fixes/942-rpc-tx-encoding.md

@@ -0,0 +1,4 @@
+- `[tendermint-rpc]` The encoding of the `hash` field for requests to the `/tx`
+  endpoint has been changed to base64 (from hex) to accommodate discrepancies in
+  how the Tendermint RPC encodes this field for different RPC interfaces
+  ([#942](https://github.com/informalsystems/tendermint-rs/issues/942))

rpc/src/client/bin/main.rs

@@ -2,6 +2,7 @@
 
 use std::str::FromStr;
 use structopt::StructOpt;
+use tendermint::abci::transaction::Hash;
 use tendermint::abci::{Path, Transaction};
 use tendermint_rpc::{
     Client, Error, HttpClient, Paging, Scheme, SubscriptionClient, Url, WebSocketClient,
@@ -112,6 +113,15 @@ enum ClientRequest {
     NetInfo,
     /// Get Tendermint status (node info, public key, latest block hash, etc.).
     Status,
+    /// Fetch a transaction by way of its hash.
+    Tx {
+        /// The SHA256 hash of the transaction (in hexadecimal).
+        hash: String,
+        /// Include proofs that the transaction was included in a block in the
+        /// response.
+        #[structopt(long)]
+        prove: bool,
+    },
     // TODO(thane): Implement txsearch endpoint.
     /// Get the validators at the given height.
     Validators {
@@ -305,6 +315,15 @@ where
         ClientRequest::NetInfo => {
             serde_json::to_string_pretty(&client.net_info().await?).map_err(Error::serde)?
         }
+        ClientRequest::Tx { hash, prove } => serde_json::to_string_pretty(
+            &client
+                .tx(
+                    Hash::from_str(&hash).map_err(|e| Error::parse(e.to_string()))?,
+                    prove,
+                )
+                .await?,
+        )
+        .map_err(Error::serde)?,
         ClientRequest::Status => {
             serde_json::to_string_pretty(&client.status().await?).map_err(Error::serde)?
         }

rpc/src/endpoint/tx.rs

@@ -8,17 +8,21 @@ use tendermint_proto::types::TxProof;
 /// Request for finding a transaction by its hash.
 #[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)]
 pub struct Request {
-    pub hash: String,
+    /// The hash of the transaction we want to find.
+    ///
+    /// Serialized internally into a base64-encoded string before sending to
+    /// the RPC server.
+    #[serde(with = "tendermint::serializers::hash_base64")]
+    pub hash: abci::transaction::Hash,
+    /// Whether or not to include the proofs of the transaction's inclusion in
+    /// the block.
     pub prove: bool,
 }
 
 impl Request {
     /// Constructor.
     pub fn new(hash: abci::transaction::Hash, prove: bool) -> Self {
-        Self {
-            hash: format!("0x{}", &hash),
-            prove,
-        }
+        Self { hash, prove }
     }
 }
 
@@ -34,6 +38,11 @@ impl crate::SimpleRequest for Request {}
 
 #[derive(Clone, Debug, Deserialize, Serialize)]
 pub struct Response {
+    /// The hash of the transaction.
+    ///
+    /// Deserialized from a hex-encoded string (there is a discrepancy between
+    /// the format used for the request and the format used for the response in
+    /// the Tendermint RPC).
     pub hash: abci::transaction::Hash,
     pub height: block::Height,
     pub index: u32,

rpc/src/endpoint/tx_search.rs

@@ -54,7 +54,3 @@ pub struct Response {
 }
 
 impl crate::Response for Response {}
-
-// TODO: remove this after the next breaking release
-#[deprecated(note = "use endpoint::tx::Response instead")]
-pub type ResultTx = tx::Response;

rpc/tests/kvstore_fixtures.rs

@@ -3,6 +3,7 @@
 use std::str::FromStr;
 use std::{fs, path::PathBuf};
 use subtle_encoding::{base64, hex};
+use tendermint::abci::transaction::Hash;
 use tendermint_rpc::{
     endpoint,
     error::{Error, ErrorDetail},
@@ -229,6 +230,19 @@ fn outgoing_fixtures() {
                     base64::decode("dHg1PXZhbHVl").unwrap()
                 );
             }
+            "tx" => {
+                let wrapped =
+                    serde_json::from_str::<RequestWrapper<endpoint::tx::Request>>(&content)
+                        .unwrap();
+                assert_eq!(
+                    wrapped.params().hash,
+                    Hash::new([
+                        214, 63, 156, 35, 121, 30, 97, 4, 16, 181, 118, 216, 194, 123, 181, 174,
+                        172, 147, 204, 26, 88, 82, 36, 40, 167, 179, 42, 18, 118, 8, 88, 96
+                    ])
+                );
+                assert!(!wrapped.params().prove);
+            }
             "tx_search_no_prove" => {
                 let wrapped =
                     serde_json::from_str::<RequestWrapper<endpoint::tx_search::Request>>(&content)
@@ -1313,6 +1327,17 @@ fn incoming_fixtures() {
                 );
                 assert!(result.log.value().is_empty());
             }
+            "tx" => {
+                let result = endpoint::tx::Response::from_string(content).unwrap();
+                assert_eq!(
+                    result.hash,
+                    Hash::new([
+                        214, 63, 156, 35, 121, 30, 97, 4, 16, 181, 118, 216, 194, 123, 181, 174,
+                        172, 147, 204, 26, 88, 82, 36, 40, 167, 179, 42, 18, 118, 8, 88, 96
+                    ])
+                );
+                assert_eq!(u64::from(result.height), 12u64);
+            }
             "tx_search_no_prove" => {
                 let result = endpoint::tx_search::Response::from_string(content).unwrap();
                 assert_eq!(result.total_count as usize, result.txs.len());

rpc/tests/kvstore_fixtures/incoming/tx.json

@@ -0,0 +1,46 @@
+{
+  "id": "aeca4fe2-9a71-4906-8a8e-91ea4114ea4b",
+  "jsonrpc": "2.0",
+  "result": {
+    "hash": "D63F9C23791E610410B576D8C27BB5AEAC93CC1A58522428A7B32A1276085860",
+    "height": "12",
+    "index": 2,
+    "tx": "Y29tbWl0LWtleT12YWx1ZQ==",
+    "tx_result": {
+      "code": 0,
+      "codespace": "",
+      "data": null,
+      "events": [
+        {
+          "attributes": [
+            {
+              "index": true,
+              "key": "Y3JlYXRvcg==",
+              "value": "Q29zbW9zaGkgTmV0b3dva28="
+            },
+            {
+              "index": true,
+              "key": "a2V5",
+              "value": "Y29tbWl0LWtleQ=="
+            },
+            {
+              "index": true,
+              "key": "aW5kZXhfa2V5",
+              "value": "aW5kZXggaXMgd29ya2luZw=="
+            },
+            {
+              "index": false,
+              "key": "bm9pbmRleF9rZXk=",
+              "value": "aW5kZXggaXMgd29ya2luZw=="
+            }
+          ],
+          "type": "app"
+        }
+      ],
+      "gas_used": "0",
+      "gas_wanted": "0",
+      "info": "",
+      "log": ""
+    }
+  }
+}

rpc/tests/kvstore_fixtures/outgoing/tx.json

@@ -0,0 +1,9 @@
+{
+  "id": "aeca4fe2-9a71-4906-8a8e-91ea4114ea4b",
+  "jsonrpc": "2.0",
+  "method": "tx",
+  "params": {
+    "hash": "1j+cI3keYQQQtXbYwnu1rqyTzBpYUiQop7MqEnYIWGA=",
+    "prove": false
+  }
+}

tendermint/src/abci/transaction.rs

@@ -2,7 +2,7 @@
 
 mod hash;
 
-pub use self::hash::Hash;
+pub use self::hash::{Hash, LENGTH as HASH_LENGTH};
 use serde::{de::Error as _, Deserialize, Deserializer, Serialize, Serializer};
 use std::{fmt, slice};
 use subtle_encoding::base64;

tendermint/src/abci/transaction/hash.rs

@@ -12,8 +12,8 @@ use subtle_encoding::hex;
 /// Size of a transaction hash in bytes
 pub const LENGTH: usize = 32;
 
-/// Trannsaction hashes
-#[derive(Copy, Clone, Hash, PartialEq)]
+/// Transaction hashes
+#[derive(Copy, Clone, Eq)]
 pub struct Hash([u8; LENGTH]);
 
 impl Hash {
@@ -41,6 +41,21 @@ impl ConstantTimeEq for Hash {
     }
 }
 
+impl core::hash::Hash for Hash {
+    fn hash<H>(&self, hasher: &mut H)
+    where
+        H: std::hash::Hasher,
+    {
+        self.0.hash(hasher)
+    }
+}
+
+impl PartialEq for Hash {
+    fn eq(&self, other: &Hash) -> bool {
+        self.ct_eq(other).into()
+    }
+}
+
 impl Display for Hash {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         for byte in &self.0 {

tendermint/src/serializers.rs

@@ -9,5 +9,6 @@ pub use tendermint_proto::serializers::*;
 
 pub mod apphash;
 pub mod hash;
+pub mod hash_base64;
 pub mod option_hash;
 pub mod time;

tendermint/src/serializers/hash_base64.rs

@@ -0,0 +1,32 @@
+//! Encoding/decoding ABCI transaction hashes to/from base64.
+
+use crate::abci::transaction::{Hash, HASH_LENGTH};
+use serde::{Deserialize, Deserializer, Serializer};
+use subtle_encoding::base64;
+
+/// Deserialize a base64-encoded string into a Hash
+pub fn deserialize<'de, D>(deserializer: D) -> Result<Hash, D::Error>
+where
+    D: Deserializer<'de>,
+{
+    let s = Option::<String>::deserialize(deserializer)?.unwrap_or_default();
+    let decoded = base64::decode(&s).map_err(serde::de::Error::custom)?;
+    if decoded.len() != HASH_LENGTH {
+        return Err(serde::de::Error::custom(
+            "unexpected transaction length for hash",
+        ));
+    }
+    let mut decoded_bytes = [0u8; HASH_LENGTH];
+    decoded_bytes.copy_from_slice(decoded.as_ref());
+    Ok(Hash::new(decoded_bytes))
+}
+
+/// Serialize from a Hash into a base64-encoded string
+pub fn serialize<S>(value: &Hash, serializer: S) -> Result<S::Ok, S::Error>
+where
+    S: Serializer,
+{
+    let base64_bytes = base64::encode(value.as_bytes());
+    let base64_string = String::from_utf8(base64_bytes).map_err(serde::ser::Error::custom)?;
+    serializer.serialize_str(&base64_string)
+}

tools/kvstore-test/tests/tendermint.rs

@@ -227,14 +227,28 @@ mod rpc {
     }
 
     #[tokio::test]
-    async fn transaction_subscription() {
+    async fn transactions() {
         // We run these sequentially wrapped within a single test to ensure
         // that Tokio doesn't execute them simultaneously. If they are executed
         // simultaneously, their submitted transactions interfere with each
         // other and one of them will (incorrectly) fail.
+        transaction_by_hash().await;
         simple_transaction_subscription().await;
         concurrent_subscriptions().await;
         tx_search().await;
+        tx_search_by_hash().await;
+    }
+
+    async fn transaction_by_hash() {
+        let tx = Transaction::from(String::from("txtest=value").into_bytes());
+        let r = localhost_http_client()
+            .broadcast_tx_commit(tx.clone())
+            .await
+            .unwrap();
+        let hash = r.hash;
+        let r = localhost_http_client().tx(hash, false).await.unwrap();
+        assert_eq!(r.hash, hash);
+        assert_eq!(r.tx, tx);
     }
 
     async fn simple_transaction_subscription() {
@@ -410,6 +424,26 @@ mod rpc {
         driver_handle.await.unwrap().unwrap();
     }
 
+    async fn tx_search_by_hash() {
+        let client = localhost_http_client();
+
+        let tx = Transaction::from(String::from("tx_search_by=hash").into_bytes());
+        let r = client.broadcast_tx_commit(tx).await.unwrap();
+        let hash = r.hash;
+
+        let r = client
+            .tx_search(
+                Query::from(EventType::Tx).and_eq("tx.hash", hash.to_string()),
+                false,
+                1,
+                10,
+                Order::Ascending,
+            )
+            .await
+            .unwrap();
+        assert_eq!(r.total_count, 1);
+    }
+
     async fn broadcast_tx(
         http_client: &HttpClient,
         websocket_client: &mut WebSocketClient,