Validate construction and deserialization of validator::Set (#1350)

* proto: deserialize total_voting_power in ValidatorSet

If the field is present, deserialize it into the struct field
rather than skipping it completely. This will allow validation
when deserializing validator::Set in tendermint.

* tendermint: harden total_voting_power computation for Set

Calculate the total voting power without overflows and validate
against the protocol-defined limit, provided as the
Set::MAX_TOTAL_VOTING_POWER const.

In the conversion from tendermint-proto struct VaidatorSet,
check the unmarshalled total_voting_power value to match the calculated
sum of the list's validators, unless it's the default 0.

* tendermint: deserialize validator::Set via TryFrom

Use the validating conversion from the protobuf struct
ValidatorSet, which should have the same schema including
the optionally deserialized total_voting_power field.
This is to ensure we don't blindly trust total_voting_power if it's
present in the serialization, and accept when it's absent.

* tendermint: fix formatting of InvalidKey error

The Display impl called itself in infinite recursion.

* proto: restore custom PublicKey serializers

These are used in serialization for ValidatorSet and
LightClientAttackEvidence.

* proto: add helper module serializers::from_str_allow_null

* proto: allow Validator.proposer_priority be null

Some light-client-js tests use fixtures where it was null.
The field used to be skipped in previous serialization.

.changelog/unreleased/bug-fixes/1348-valset-deserialization-fixes.md

@@ -0,0 +1,3 @@
+- `[tendermint]` Integer overflows are prevented when calculating the total
+  voting power value in `validator::Set`
+  ([\#1348](https://github.com/informalsystems/tendermint-rs/issues/1348)).

.changelog/unreleased/bug-fixes/1350-pubkey-serde-fixes.md

@@ -0,0 +1,4 @@
+- `[tendermint-proto]` `Serialize` and `Deserialize` impls for
+  `v*::crypto::PublicKey` are corrected to match the JSON schema used by
+  other implementations
+  ([\#1350](https://github.com/informalsystems/tendermint-rs/pull/1350)).

.changelog/unreleased/improvements/1348-valset-deserialization-fixes.md

@@ -0,0 +1,12 @@
+- `[tendermint]` Improve and validate deserialization of `validator::Set`
+  ([\#1348](https://github.com/informalsystems/tendermint-rs/issues/1348)).
+  The `total_voting_power` field no longer has to be present in the format
+  processed by `Deserialize`. If it is present, it is validated against the
+  sum of the `voting_power` values of the listed validators. The sum value
+  is also checked against the protocol-defined maximum.
+- `[tendermint-proto]` In the `Deserialize` impls derived for
+  `v*::types::ValidatorSet`, the `total_voting_power` field value is retrieved
+  when present.
+- `[tendermint-proto]` Add serialziation helper module
+  `serializers::from_str_allow_null`. Use it to allow the `proposed_priority`
+  field value of null in the deserialization of `v*::types::Validator`.

proto/src/prost/v0_34/tendermint.crypto.rs

@@ -56,7 +56,6 @@ pub struct ProofOps {
     pub ops: ::prost::alloc::vec::Vec<ProofOp>,
 }
 /// PublicKey defines the keys available for use with Validators
-#[derive(::serde::Deserialize, ::serde::Serialize)]
 #[allow(clippy::derive_partial_eq_without_eq)]
 #[derive(Clone, PartialEq, ::prost::Message)]
 pub struct PublicKey {

proto/src/prost/v0_34/tendermint.types.rs

@@ -7,7 +7,8 @@ pub struct ValidatorSet {
     #[prost(message, optional, tag = "2")]
     pub proposer: ::core::option::Option<Validator>,
     #[prost(int64, tag = "3")]
-    #[serde(skip)]
+    #[serde(with = "crate::serializers::from_str", default)]
+    #[serde(skip_serializing)]
     pub total_voting_power: i64,
 }
 #[derive(::serde::Deserialize, ::serde::Serialize)]
@@ -23,7 +24,8 @@ pub struct Validator {
     #[serde(alias = "power", with = "crate::serializers::from_str")]
     pub voting_power: i64,
     #[prost(int64, tag = "4")]
-    #[serde(with = "crate::serializers::from_str", default)]
+    #[serde(with = "crate::serializers::from_str_allow_null")]
+    #[serde(default)]
     pub proposer_priority: i64,
 }
 #[allow(clippy::derive_partial_eq_without_eq)]

proto/src/prost/v0_37/tendermint.crypto.rs

@@ -56,7 +56,6 @@ pub struct ProofOps {
     pub ops: ::prost::alloc::vec::Vec<ProofOp>,
 }
 /// PublicKey defines the keys available for use with Validators
-#[derive(::serde::Deserialize, ::serde::Serialize)]
 #[allow(clippy::derive_partial_eq_without_eq)]
 #[derive(Clone, PartialEq, ::prost::Message)]
 pub struct PublicKey {

proto/src/prost/v0_37/tendermint.types.rs

@@ -7,7 +7,8 @@ pub struct ValidatorSet {
     #[prost(message, optional, tag = "2")]
     pub proposer: ::core::option::Option<Validator>,
     #[prost(int64, tag = "3")]
-    #[serde(skip)]
+    #[serde(with = "crate::serializers::from_str", default)]
+    #[serde(skip_serializing)]
     pub total_voting_power: i64,
 }
 #[derive(::serde::Deserialize, ::serde::Serialize)]
@@ -23,7 +24,8 @@ pub struct Validator {
     #[serde(alias = "power", with = "crate::serializers::from_str")]
     pub voting_power: i64,
     #[prost(int64, tag = "4")]
-    #[serde(with = "crate::serializers::from_str", default)]
+    #[serde(with = "crate::serializers::from_str_allow_null")]
+    #[serde(default)]
     pub proposer_priority: i64,
 }
 #[allow(clippy::derive_partial_eq_without_eq)]

proto/src/prost/v0_38/tendermint.crypto.rs

@@ -56,7 +56,6 @@ pub struct ProofOps {
     pub ops: ::prost::alloc::vec::Vec<ProofOp>,
 }
 /// PublicKey defines the keys available for use with Validators
-#[derive(::serde::Deserialize, ::serde::Serialize)]
 #[allow(clippy::derive_partial_eq_without_eq)]
 #[derive(Clone, PartialEq, ::prost::Message)]
 pub struct PublicKey {

proto/src/prost/v0_38/tendermint.types.rs

@@ -104,7 +104,8 @@ pub struct ValidatorSet {
     #[prost(message, optional, tag = "2")]
     pub proposer: ::core::option::Option<Validator>,
     #[prost(int64, tag = "3")]
-    #[serde(skip)]
+    #[serde(with = "crate::serializers::from_str", default)]
+    #[serde(skip_serializing)]
     pub total_voting_power: i64,
 }
 #[derive(::serde::Deserialize, ::serde::Serialize)]
@@ -120,7 +121,8 @@ pub struct Validator {
     #[serde(alias = "power", with = "crate::serializers::from_str")]
     pub voting_power: i64,
     #[prost(int64, tag = "4")]
-    #[serde(with = "crate::serializers::from_str", default)]
+    #[serde(with = "crate::serializers::from_str_allow_null")]
+    #[serde(default)]
     pub proposer_priority: i64,
 }
 #[allow(clippy::derive_partial_eq_without_eq)]

proto/src/serializers.rs

@@ -57,10 +57,13 @@ pub mod allow_null;
 pub mod bytes;
 pub mod evidence;
 pub mod from_str;
+pub mod from_str_allow_null;
 pub mod nullable;
 pub mod optional;
 pub mod optional_from_str;
 pub mod part_set_header_total;
 pub mod time_duration;
 pub mod timestamp;
 pub mod txs;
+
+mod public_key;

proto/src/serializers/from_str_allow_null.rs

@@ -0,0 +1,42 @@
+//! Combines [`from_str`] and [`allow_null`].
+//!
+//! Use this module to serialize and deserialize any `T` where `T` implements
+//! [`FromStr`] and [`Display`] to convert from or into a string.
+//! The serialized form is that of `Option<String>`,
+//! and a nil is deserialized to the `Default` value. For JSON, this means both
+//! quoted string values and `null` are accepted. A value is always serialized
+//! as `Some<String>`.
+//! Note that this can be used for all primitive data types.
+//!
+//! [`from_str`]: super::from_str
+//! [`allow_null`]: super::allow_null
+
+use alloc::borrow::Cow;
+use core::fmt::Display;
+use core::str::FromStr;
+
+use serde::{de::Error as _, Deserialize, Deserializer, Serializer};
+
+use crate::prelude::*;
+
+/// Deserialize a nullable string into T
+pub fn deserialize<'de, D, T>(deserializer: D) -> Result<T, D::Error>
+where
+    D: Deserializer<'de>,
+    T: FromStr + Default,
+    <T as FromStr>::Err: Display,
+{
+    match <Option<Cow<'_, str>>>::deserialize(deserializer)? {
+        Some(s) => s.parse::<T>().map_err(D::Error::custom),
+        None => Ok(T::default()),
+    }
+}
+
+/// Serialize from T into string
+pub fn serialize<S, T>(value: &T, serializer: S) -> Result<S::Ok, S::Error>
+where
+    S: Serializer,
+    T: Display,
+{
+    serializer.serialize_some(&value.to_string())
+}

proto/src/serializers/public_key.rs

@@ -0,0 +1,71 @@
+mod v0_34 {
+    use crate::v0_34::crypto::{public_key, PublicKey};
+    use serde::{Deserialize, Deserializer, Serialize, Serializer};
+
+    impl<'de> Deserialize<'de> for PublicKey {
+        fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+        where
+            D: Deserializer<'de>,
+        {
+            let sum = Option::<public_key::Sum>::deserialize(deserializer)?;
+            Ok(Self { sum })
+        }
+    }
+
+    impl Serialize for PublicKey {
+        fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+        where
+            S: Serializer,
+        {
+            self.sum.serialize(serializer)
+        }
+    }
+}
+
+mod v0_37 {
+    use crate::v0_37::crypto::{public_key, PublicKey};
+    use serde::{Deserialize, Deserializer, Serialize, Serializer};
+
+    impl<'de> Deserialize<'de> for PublicKey {
+        fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+        where
+            D: Deserializer<'de>,
+        {
+            let sum = Option::<public_key::Sum>::deserialize(deserializer)?;
+            Ok(Self { sum })
+        }
+    }
+
+    impl Serialize for PublicKey {
+        fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+        where
+            S: Serializer,
+        {
+            self.sum.serialize(serializer)
+        }
+    }
+}
+
+mod v0_38 {
+    use crate::v0_38::crypto::{public_key, PublicKey};
+    use serde::{Deserialize, Deserializer, Serialize, Serializer};
+
+    impl<'de> Deserialize<'de> for PublicKey {
+        fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+        where
+            D: Deserializer<'de>,
+        {
+            let sum = Option::<public_key::Sum>::deserialize(deserializer)?;
+            Ok(Self { sum })
+        }
+    }
+
+    impl Serialize for PublicKey {
+        fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+        where
+            S: Serializer,
+        {
+            self.sum.serialize(serializer)
+        }
+    }
+}

tendermint/src/error.rs

@@ -16,7 +16,7 @@ define_error! {
 
         InvalidKey
             { detail: String }
-            |e| { format_args!("invalid key: {e}") },
+            |e| { format_args!("invalid key: {}", e.detail) },
 
         Length
             |_| { format_args!("length error") },
@@ -229,6 +229,12 @@ define_error! {
         NegativeProofIndex
             [ DisplayOnly<TryFromIntError> ]
             |_| { "negative item index in proof" },
+
+        TotalVotingPowerMismatch
+            |_| { "total voting power in validator set does not match the sum of participants' powers" },
+
+        TotalVotingPowerOverflow
+            |_| { "total voting power in validator set exceeds the allowed maximum" },
     }
 }