Fix Light Client validator set hash calculation (#834)

* Remove unused file

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Refactor validators RPC endpoint interface

This commit adds pagination to the `validators` method on the `Client`
trait (BREAKING).

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Ensure "total" response field is a string

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Add serializer for optional types that need to be converted to/from a string (like page numbers/per page counts)

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Refactor to ensure page numbers and per-page values are converted to/from strings first

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Convert tcp:// scheme to http:// for RPC addresses

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Add Light Client support for RPC URLs instead of net::Address

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Revert 14ad69f for now

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Revert f0c26f7

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Add CHANGELOG

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Convert infallible TryFroms to Froms

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Remove all non_exhaustive struct attributes

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Attempt to fix broken wasm_bindgen dependency

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Fixate syn dependency for light-client-js

Signed-off-by: Thane Thomson <connect@thanethomson.com>

CHANGELOG.md

@@ -5,6 +5,10 @@
 * `[tendermint]` The `tendermint::block::CommitSig` enum's members have been
   renamed to be consistent with Rust's naming conventions. For example,
   `BlockIDFlagAbsent` is now renamed to `BlockIdFlagAbsent` ([#839])
+* `[tendermint-rpc]` The `Client::validators` method now requires a `Paging`
+  parameter. Previously, this wasn't possible and, if the network had more than
+  30 validators (the default for the RPC endpoint), it only returned a subset
+  of the validators ([#831])
 * `[tendermint-rpc]` The `SubscriptionClient` trait now requires a `close`
   method, since it assumes that subscription clients will, in general, use
   long-running connections. This should not, however, break any downstream
@@ -39,10 +43,18 @@
 
 * `[tendermint]` IPv6 support has been added for `net::Address` ([#5])
 
+### BUG FIXES
+
+* `[tendermint-light-client]` Due to the RPC client's `validators` method
+  sometimes only returning a subset of validators (for networks larger than 30
+  validators), validator set hash calculations were failing. Now we are at
+  least obtaining a full validator set ([#831])
+
 [#5]: https://github.com/informalsystems/tendermint-rs/issues/5
 [#794]: https://github.com/informalsystems/tendermint-rs/pull/794
 [#812]: https://github.com/informalsystems/tendermint-rs/pull/812
 [#820]: https://github.com/informalsystems/tendermint-rs/pull/820
+[#831]: https://github.com/informalsystems/tendermint-rs/issues/831
 [#839]: https://github.com/informalsystems/tendermint-rs/pull/839
 
 ## v0.18.1

light-client-js/Cargo.toml

@@ -22,6 +22,8 @@ default = ["console_error_panic_hook"]
 [dependencies]
 serde = { version = "1.0", features = [ "derive" ] }
 serde_json = "1.0"
+# TODO(thane): Remove once https://github.com/rustwasm/wasm-bindgen/issues/2508 is resolved
+syn = "=1.0.65"
 tendermint = { version = "0.18.1", path = "../tendermint" }
 tendermint-light-client = { version = "0.18.1", path = "../light-client", default-features = false }
 wasm-bindgen = { version = "0.2.63", features = [ "serde-serialize" ] }

light-client/src/components/io.rs

@@ -91,6 +91,7 @@ mod prod {
     use tendermint::account::Id as TMAccountId;
     use tendermint::block::signed_header::SignedHeader as TMSignedHeader;
     use tendermint::validator::Set as TMValidatorSet;
+    use tendermint_rpc::Paging;
 
     /// Production implementation of the Io component, which fetches
     /// light blocks from full nodes via RPC.
@@ -166,8 +167,10 @@ mod prod {
             };
 
             let client = self.rpc_client.clone();
-            let response = block_on(self.timeout, async move { client.validators(height).await })?
-                .map_err(IoError::RpcError)?;
+            let response = block_on(self.timeout, async move {
+                client.validators(height, Paging::All).await
+            })?
+            .map_err(IoError::RpcError)?;
 
             let validator_set = match proposer_address {
                 Some(proposer_address) => {

proto/src/serializers.rs

@@ -49,6 +49,7 @@ pub mod evidence;
 pub mod from_str;
 pub mod nullable;
 pub mod optional;
+pub mod optional_from_str;
 pub mod part_set_header_total;
 pub mod time_duration;
 pub mod timestamp;

proto/src/serializers/optional_from_str.rs

@@ -0,0 +1,31 @@
+//! De/serialize an optional type that must be converted from/to a string.
+
+use serde::de::Error;
+use serde::{Deserialize, Deserializer, Serializer};
+use std::str::FromStr;
+
+pub fn serialize<S, T>(value: &Option<T>, serializer: S) -> Result<S::Ok, S::Error>
+where
+    S: Serializer,
+    T: ToString,
+{
+    match value {
+        Some(t) => serializer.serialize_some(&t.to_string()),
+        None => serializer.serialize_none(),
+    }
+}
+
+pub fn deserialize<'de, D, T>(deserializer: D) -> Result<Option<T>, D::Error>
+where
+    D: Deserializer<'de>,
+    T: FromStr,
+    T::Err: std::error::Error,
+{
+    let s = match Option::<String>::deserialize(deserializer)? {
+        Some(s) => s,
+        None => return Ok(None),
+    };
+    Ok(Some(s.parse().map_err(|e: <T as FromStr>::Err| {
+        D::Error::custom(e.to_string())
+    })?))
+}

rpc/src/client.rs

@@ -12,7 +12,9 @@ pub use transport::http::{HttpClient, HttpClientUrl};
 #[cfg(feature = "websocket-client")]
 pub use transport::websocket::{WebSocketClient, WebSocketClientDriver, WebSocketClientUrl};
 
+use crate::endpoint::validators::DEFAULT_VALIDATORS_PER_PAGE;
 use crate::endpoint::*;
+use crate::paging::Paging;
 use crate::query::Query;
 use crate::{Order, Result, SimpleRequest};
 use async_trait::async_trait;
@@ -123,12 +125,53 @@ pub trait Client {
         self.perform(consensus_state::Request::new()).await
     }
 
+    // TODO(thane): Simplify once validators endpoint removes pagination.
     /// `/validators`: get validators a given height.
-    async fn validators<H>(&self, height: H) -> Result<validators::Response>
+    async fn validators<H>(&self, height: H, paging: Paging) -> Result<validators::Response>
     where
         H: Into<Height> + Send,
     {
-        self.perform(validators::Request::new(height.into())).await
+        let height = height.into();
+        match paging {
+            Paging::Default => {
+                self.perform(validators::Request::new(Some(height), None, None))
+                    .await
+            }
+            Paging::Specific {
+                page_number,
+                per_page,
+            } => {
+                self.perform(validators::Request::new(
+                    Some(height),
+                    Some(page_number),
+                    Some(per_page),
+                ))
+                .await
+            }
+            Paging::All => {
+                let mut page_num = 1_usize;
+                let mut validators = Vec::new();
+                let per_page = DEFAULT_VALIDATORS_PER_PAGE.into();
+                loop {
+                    let response = self
+                        .perform(validators::Request::new(
+                            Some(height),
+                            Some(page_num.into()),
+                            Some(per_page),
+                        ))
+                        .await?;
+                    validators.extend(response.validators);
+                    if validators.len() as i32 == response.total {
+                        return Ok(validators::Response::new(
+                            response.block_height,
+                            validators,
+                            response.total,
+                        ));
+                    }
+                    page_num += 1;
+                }
+            }
+        }
     }
 
     /// `/commit`: get the latest block commit

rpc/src/client/bin/main.rs

@@ -7,8 +7,8 @@ use structopt::StructOpt;
 use tendermint::abci::{Path, Transaction};
 use tendermint_rpc::query::Query;
 use tendermint_rpc::{
-    Client, Error, HttpClient, Order, Result, Scheme, Subscription, SubscriptionClient, Url,
-    WebSocketClient,
+    Client, Error, HttpClient, Order, Paging, Result, Scheme, Subscription, SubscriptionClient,
+    Url, WebSocketClient,
 };
 use tracing::level_filters::LevelFilter;
 use tracing::{error, info, warn};
@@ -141,7 +141,19 @@ enum ClientRequest {
         prove: bool,
     },
     /// Get the validators at the given height.
-    Validators { height: u32 },
+    Validators {
+        /// The height at which to query the validators.
+        height: u32,
+        /// Fetch all validators.
+        #[structopt(long)]
+        all: bool,
+        /// The page of validators to retrieve.
+        #[structopt(long)]
+        page: Option<usize>,
+        /// The number of validators to retrieve per page.
+        #[structopt(long)]
+        per_page: Option<u8>,
+    },
 }
 
 #[tokio::main]
@@ -321,8 +333,24 @@ where
                 .tx_search(query, prove, page, per_page, order)
                 .await?,
         )?,
-        ClientRequest::Validators { height } => {
-            serde_json::to_string_pretty(&client.validators(height).await?)?
+        ClientRequest::Validators {
+            height,
+            all,
+            page,
+            per_page,
+        } => {
+            let paging = if all {
+                Paging::All
+            } else {
+                match page.zip(per_page) {
+                    Some((page, per_page)) => Paging::Specific {
+                        page_number: page.into(),
+                        per_page: per_page.into(),
+                    },
+                    None => Paging::Default,
+                }
+            };
+            serde_json::to_string_pretty(&client.validators(height, paging).await?)?
         }
     };
     println!("{}", result);
@@ -338,7 +366,7 @@ async fn subscription_client_request<C>(
 where
     C: SubscriptionClient,
 {
-    info!("Creating subcription for query: {}", query);
+    info!("Creating subscription for query: {}", query);
     let subs = client.subscribe(query).await?;
     match max_time {
         Some(secs) => recv_events_with_timeout(subs, max_events, secs).await,

rpc/src/endpoint/consensus_state.rs

@@ -14,7 +14,6 @@ const NIL_VOTE_STR: &str = "nil-Vote";
 
 /// Get the current consensus state.
 #[derive(Clone, Debug, Default, Deserialize, Eq, PartialEq, Serialize)]
-#[non_exhaustive]
 pub struct Request;
 
 impl Request {

rpc/src/endpoint/validators.rs

@@ -1,19 +1,55 @@
 //! `/validators` endpoint JSON-RPC wrapper
 
+use crate::{PageNumber, PerPage};
 use serde::{Deserialize, Serialize};
-
 use tendermint::{block, validator};
 
+/// The default number of validators to return per page.
+pub const DEFAULT_VALIDATORS_PER_PAGE: u8 = 30;
+
 /// List validators for a specific block
 #[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)]
 pub struct Request {
-    pub height: block::Height,
+    /// The height at which to retrieve the validator set. If not specified,
+    /// defaults to the latest height.
+    pub height: Option<block::Height>,
+    /// The number of the page to fetch.
+    #[serde(with = "tendermint_proto::serializers::optional_from_str")]
+    pub page: Option<PageNumber>,
+    /// The number of validators to fetch per page.
+    #[serde(with = "tendermint_proto::serializers::optional_from_str")]
+    pub per_page: Option<PerPage>,
 }
 
 impl Request {
-    /// List validators for a specific block
-    pub fn new(height: block::Height) -> Self {
-        Self { height }
+    /// List validators for a specific block.
+    ///
+    /// See the [Tendermint RPC] for the defaults for each option when set to
+    /// `None`.
+    ///
+    /// [Tendermint RPC]: https://docs.tendermint.com/master/rpc/#/Info/validators
+    pub fn new(
+        height: Option<block::Height>,
+        page: Option<PageNumber>,
+        per_page: Option<PerPage>,
+    ) -> Self {
+        Self {
+            height,
+            page,
+            per_page,
+        }
+    }
+}
+
+impl Default for Request {
+    fn default() -> Self {
+        // By default we get the latest validators list, page 1, maximum 30
+        // items per page (the RPC defaults).
+        Self {
+            height: None,
+            page: None,
+            per_page: None,
+        }
     }
 }
 
@@ -35,6 +71,21 @@ pub struct Response {
 
     /// Validator list
     pub validators: Vec<validator::Info>,
+
+    /// Total number of validators for this block height.
+    #[serde(with = "tendermint_proto::serializers::from_str")]
+    pub total: i32,
 }
 
 impl crate::Response for Response {}
+
+impl Response {
+    /// Constructor.
+    pub fn new(block_height: block::Height, validators: Vec<validator::Info>, total: i32) -> Self {
+        Self {
+            block_height,
+            validators,
+            total,
+        }
+    }
+}

rpc/src/error.rs

@@ -7,6 +7,8 @@ use serde::{Deserialize, Deserializer, Serialize, Serializer};
 use std::fmt::{self, Display};
 use thiserror::Error;
 
+// TODO(thane): Differentiate between RPC response errors and internal crate
+//              errors (e.g. domain type-related errors).
 /// Tendermint RPC errors
 #[derive(Clone, Debug, PartialEq, Eq, Deserialize, Serialize)]
 pub struct Error {

rpc/src/lib.rs

@@ -50,6 +50,7 @@ pub mod event;
 mod id;
 mod method;
 mod order;
+mod paging;
 pub mod query;
 pub mod request;
 pub mod response;
@@ -58,6 +59,7 @@ mod utils;
 mod version;
 
 pub use self::{
-    error::Error, id::Id, method::Method, order::Order, request::Request, request::SimpleRequest,
-    response::Response, result::Result, version::Version,
+    error::Error, id::Id, method::Method, order::Order, paging::PageNumber, paging::Paging,
+    paging::PerPage, request::Request, request::SimpleRequest, response::Response, result::Result,
+    version::Version,
 };