Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use core and alloc crates for no_std compatibility #980

Closed
wants to merge 18 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion abci/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ bytes = "1.0"
prost = "0.7"
tendermint-proto = { version = "0.22.0", path = "../proto" }
tracing = "0.1"
flex-error = { version = "0.4.1", default-features = false }
flex-error = { version = "0.4.3", default-features = false }

structopt = { version = "0.3", optional = true }
tracing-subscriber = { version = "0.2", optional = true }
5 changes: 3 additions & 2 deletions light-client/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ secp256k1 = ["tendermint/secp256k1", "tendermint-rpc/secp256k1"]
lightstore-sled = ["sled"]
unstable = []
std = [
"flex-error/std"
"flex-error/std",
"tendermint/std",
]
# Enable to execute long-running model-based tests
mbt = []
Expand All @@ -54,7 +55,7 @@ serde_derive = "1.0.106"
sled = { version = "0.34.3", optional = true }
static_assertions = "1.1.0"
tokio = { version = "1.0", features = ["rt"], optional = true }
flex-error = { version = "0.4.1", default-features = false }
flex-error = { version = "0.4.3", default-features = false }

[dev-dependencies]
tendermint-testgen = { path = "../testgen" }
Expand Down
10 changes: 4 additions & 6 deletions light-client/src/components/verifier.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,14 +128,12 @@ where
///
/// - Ensure the latest trusted header hasn't expired
/// - Ensure the header validator hashes match the given validators
/// - Ensure the header next validator hashes match the given next
/// validators
/// - Ensure the header next validator hashes match the given next validators
/// - Additional implementation specific validation via `commit_validator`
/// - Check that the untrusted block is more recent than the trusted state
/// - If the untrusted block is the very next block after the trusted block,
/// check that their (next) validator sets hashes match.
/// - Otherwise, ensure that the untrusted block has a greater height than
/// the trusted block.
/// - If the untrusted block is the very next block after the trusted block, check that their
/// (next) validator sets hashes match.
/// - Otherwise, ensure that the untrusted block has a greater height than the trusted block.
///
/// **NOTE**: If the untrusted state's `next_validators` field is `None`,
/// this will not (and will not be able to) check whether the untrusted
Expand Down
5 changes: 4 additions & 1 deletion light-client/src/contracts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,10 @@ pub fn is_within_trust_period(
now: Time,
) -> bool {
let header_time = light_block.signed_header.header.time;
header_time > now - trusting_period
match now - trusting_period {
Ok(start) => header_time > start,
Err(_) => false,
}
}

/// Whether or not the given light store contains a trusted block
Expand Down
11 changes: 7 additions & 4 deletions light-client/src/predicates.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,8 @@ pub trait VerificationPredicates: Send + Sync {
trusting_period: Duration,
now: Time,
) -> Result<(), VerificationError> {
let expires_at = trusted_header_time + trusting_period;
let expires_at =
(trusted_header_time + trusting_period).map_err(VerificationError::tendermint)?;

if expires_at > now {
Ok(())
Expand All @@ -115,7 +116,9 @@ pub trait VerificationPredicates: Send + Sync {
clock_drift: Duration,
now: Time,
) -> Result<(), VerificationError> {
if untrusted_header_time < now + clock_drift {
let drifted = (now + clock_drift).map_err(VerificationError::tendermint)?;

if untrusted_header_time < drifted {
Ok(())
} else {
Err(VerificationError::header_from_the_future(
Expand Down Expand Up @@ -300,7 +303,7 @@ mod tests {

let result_err = vp.is_within_trust_period(header.time, trusting_period, now);

let expires_at = header.time + trusting_period;
let expires_at = (header.time + trusting_period).unwrap();
match result_err {
Err(VerificationError(VerificationErrorDetail::NotWithinTrustPeriod(e), _)) => {
assert_eq!(e.expires_at, expires_at);
Expand All @@ -324,7 +327,7 @@ mod tests {
assert!(result_ok.is_ok());

// 2. ensure it fails if header is from a future time
let now = Time::now().sub(one_second * 15);
let now = Time::now().sub(one_second * 15).unwrap();
let result_err = vp.is_header_from_past(header.time, one_second, now);

match result_err {
Expand Down
5 changes: 5 additions & 0 deletions light-client/src/predicates/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,15 @@ use crate::errors::ErrorExt;
use crate::operations::voting_power::VotingPowerTally;
use crate::types::{Hash, Height, Time, Validator, ValidatorAddress};
use tendermint::account::Id;
use tendermint::Error as TendermintError;

define_error! {
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
VerificationError {
Tendermint
[ TendermintError ]
| _ | { "tendermint error" },

HeaderFromTheFuture
{
header_time: Time,
Expand Down
9 changes: 5 additions & 4 deletions light-client/tests/model_based.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,8 @@ mod mbt {
impl SingleStepTestFuzzer for HeaderVersionFuzzer {
// TODO: rehash the header and re-compute commit with it
// TODO: Unlike in tendermint-go, we don't assert for a particular version in rust
// TODO: Either add this check in verification or remove this test because otherwise there's no
// point of it
// TODO: Either add this check in verification or remove this test because otherwise there's
// no point of it
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like there are some changes the Rust nightly that cause comments to be formatted slightly differently by cargo fmt.

fn fuzz_input(input: &mut BlockVerdict) -> (String, LiteVerdict) {
let mut rng = rand::thread_rng();

Expand Down Expand Up @@ -184,8 +184,9 @@ mod mbt {
.unwrap()
.as_secs();
let rand_secs = rng.gen_range(1, secs);
input.block.signed_header.header.time =
tendermint::Time::unix_epoch() + std::time::Duration::from_secs(rand_secs);
input.block.signed_header.header.time = (tendermint::Time::unix_epoch()
+ std::time::Duration::from_secs(rand_secs))
.unwrap();
// TODO: the fuzzing below fails with one of:
// - 'overflow when adding duration to instant', src/libstd/time.rs:549:31
// - 'No such local time',
Expand Down
2 changes: 1 addition & 1 deletion p2p/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ x25519-dalek = "1.1"
zeroize = "1"
signature = "1.3.0"
aead = "0.4.1"
flex-error = { version = "0.4.1", default-features = false }
flex-error = { version = "0.4.3", default-features = false }

# path dependencies
tendermint = { path = "../tendermint", version = "0.22.0" }
Expand Down
2 changes: 1 addition & 1 deletion pbt-gen/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,5 @@ default = ["time"]
time = ["chrono"]

[dependencies]
chrono = { version = "0.4", features = ["serde"], optional = true}
chrono = { version = "0.4", default-features = false, features = ["serde"], optional = true}
proptest = "0.10.1"
20 changes: 10 additions & 10 deletions proto/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,16 @@ description = """
all-features = true

[dependencies]
prost = "0.7"
prost-types = "0.7"
bytes = "1.0"
serde = { version = "1.0", features = ["derive"] }
subtle-encoding = "0.5"
serde_bytes = "0.11"
num-traits = "0.2"
num-derive = "0.3"
chrono = { version = "0.4", features = ["serde"] }
flex-error = { version = "0.4.1", default-features = false }
prost = { version = "0.7", default-features = false }
prost-types = { version = "0.7", default-features = false }
bytes = { version = "1.0", default-features = false }
serde = { version = "1.0", default-features = false, features = ["derive"] }
serde_bytes = { version = "0.11", default-features = false, features = ["alloc"] }
subtle-encoding = { version = "0.5", default-features = false, features = ["hex", "base64", "std"] }
num-traits = { version = "0.2", default-features = false }
num-derive = { version = "0.3", default-features = false }
chrono = { version = "0.4", default-features = false, features = ["serde", "clock"] }
flex-error = { version = "0.4.3", default-features = false }

[dev-dependencies]
serde_json = "1.0"
Expand Down
7 changes: 4 additions & 3 deletions proto/src/error.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
//! This module defines the various errors that be raised during Protobuf conversions.

use crate::prelude::*;
use core::convert::TryFrom;
use core::fmt::Display;
use core::num::TryFromIntError;
use flex_error::{define_error, DisplayOnly};
use prost::{DecodeError, EncodeError};
use std::convert::TryFrom;
use std::fmt::Display;
use std::num::TryFromIntError;

define_error! {
Error {
Expand Down
42 changes: 31 additions & 11 deletions proto/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,18 @@
//! tendermint-proto library gives the developer access to the Tendermint proto-defined structs.

#![no_std]
#![deny(warnings, trivial_casts, trivial_numeric_casts, unused_import_braces)]
#![allow(clippy::large_enum_variant)]
#![forbid(unsafe_code)]
#![doc(html_root_url = "https://docs.rs/tendermint-proto/0.22.0")]

extern crate alloc;

#[cfg(feature = "std")]
extern crate std;

mod prelude;

/// Built-in prost_types with slight customization to enable JSON-encoding
#[allow(warnings)]
pub mod google {
Expand All @@ -23,13 +31,15 @@ pub use error::Error;
pub use tendermint::*;

use bytes::{Buf, BufMut};
use core::convert::{TryFrom, TryInto};
use core::fmt::Display;
use prost::encoding::encoded_len_varint;
use prost::Message;
use std::convert::{TryFrom, TryInto};
use std::fmt::Display;

pub mod serializers;

use prelude::*;

/// Allows for easy Google Protocol Buffers encoding and decoding of domain
/// types with validation.
///
Expand All @@ -38,7 +48,7 @@ pub mod serializers;
/// ```rust
/// use bytes::BufMut;
/// use prost::Message;
/// use std::convert::TryFrom;
/// use core::convert::TryFrom;
/// use tendermint_proto::Protobuf;
///
/// // This struct would ordinarily be automatically generated by prost.
Expand Down Expand Up @@ -107,9 +117,15 @@ pub mod serializers;
/// // We expect a validation error here
/// assert!(MyDomainType::decode(invalid_raw_bytes.as_ref()).is_err());
/// ```
pub trait Protobuf<T: Message + From<Self> + Default>
pub trait Protobuf<T>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have modified the Protobuf trait such that it allows the message type to implement TryFrom instead of From. This allows safe parsing of the TimeStamp type, to prevent undefined behavior caused by integer overflow in DOS attack.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally the idea here was that we wanted conversions from SerializationType -> DomainType to be fallible to allow for validation, and then from DomainType -> SerializationType to be infallible because DomainType should be a subset of SerializationType.

Changing this would imply that we could possibly have DomainTypes that possibly do not have valid serialized representations, which implies to me that those DomainTypes are incorrectly modeled, does it not?

So then the practical consequences of this architectural change would be:

  1. All From<DomainType> for SerializationType impls now need to become TryFrom<DomainType> for SerializationType, touching all domain types
  2. We need custom serde Serialize trait impls, as per https://github.com/informalsystems/tendermint-rs/pull/980/files#r717530415

This seems like a lot of work/code just to be able to cater for the DomainTimestamp -> SerializedTimestamp conversion, does it not? Is there no way to just implement a better DomainTimestamp whose conversion to SerializedTimestamp is infallible (as it should actually be)?

where
Self: Sized + Clone + TryFrom<T>,
T: Message,
T: Default,
Self: Sized,
Self: Clone,
T: TryFrom<Self>,
Self: TryFrom<T>,
<T as TryFrom<Self>>::Error: Display,
<Self as TryFrom<T>>::Error: Display,
{
/// Encode into a buffer in Protobuf format.
Expand All @@ -119,7 +135,8 @@ where
///
/// [`prost::Message::encode`]: https://docs.rs/prost/*/prost/trait.Message.html#method.encode
fn encode<B: BufMut>(&self, buf: &mut B) -> Result<(), Error> {
T::from(self.clone())
T::try_from(self.clone())
.map_err(Error::try_from::<Self, T, _>)?
.encode(buf)
.map_err(Error::encode_message)
}
Expand All @@ -133,7 +150,8 @@ where
///
/// [`prost::Message::encode_length_delimited`]: https://docs.rs/prost/*/prost/trait.Message.html#method.encode_length_delimited
fn encode_length_delimited<B: BufMut>(&self, buf: &mut B) -> Result<(), Error> {
T::from(self.clone())
T::try_from(self.clone())
.map_err(Error::try_from::<Self, T, _>)?
.encode_length_delimited(buf)
.map_err(Error::encode_message)
}
Expand Down Expand Up @@ -173,13 +191,15 @@ where
/// counterpart Protobuf data structure.
///
/// [`prost::Message::encoded_len`]: https://docs.rs/prost/*/prost/trait.Message.html#method.encoded_len
fn encoded_len(&self) -> usize {
T::from(self.clone()).encoded_len()
fn encoded_len(&self) -> Result<usize, Error> {
Ok(T::try_from(self.clone())
.map_err(Error::try_from::<Self, T, _>)?
.encoded_len())
}

/// Encodes into a Protobuf-encoded `Vec<u8>`.
fn encode_vec(&self) -> Result<Vec<u8>, Error> {
let mut wire = Vec::with_capacity(self.encoded_len());
let mut wire = Vec::with_capacity(self.encoded_len()?);
self.encode(&mut wire).map(|_| wire)
}

Expand All @@ -191,7 +211,7 @@ where

/// Encode with a length-delimiter to a `Vec<u8>` Protobuf-encoded message.
fn encode_length_delimited_vec(&self) -> Result<Vec<u8>, Error> {
let len = self.encoded_len();
let len = self.encoded_len()?;
let lenu64 = len.try_into().map_err(Error::parse_length)?;
let mut wire = Vec::with_capacity(len + encoded_len_varint(lenu64));
self.encode_length_delimited(&mut wire).map(|_| wire)
Expand Down
11 changes: 11 additions & 0 deletions proto/src/prelude.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
pub use core::prelude::v1::*;

// Re-export according to alloc::prelude::v1 because it is not yet stabilized
// https://doc.rust-lang.org/src/alloc/prelude/v1.rs.html
pub use alloc::borrow::ToOwned;
pub use alloc::boxed::Box;
pub use alloc::string::{String, ToString};
pub use alloc::vec::Vec;

pub use alloc::format;
pub use alloc::vec;
2 changes: 1 addition & 1 deletion proto/src/serializers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
//! Available serializers:
//! i64 <-> string: #[serde(with="serializers::from_str")]
//! u64 <-> string: #[serde(with="serializers::from_str")]
//! std::time::Duration <-> nanoseconds as string #[serde(with="serializers::time_duration")]
//! core::time::Duration <-> nanoseconds as string #[serde(with="serializers::time_duration")]
//! Vec<u8> <-> HexString: #[serde(with="serializers::bytes::hexstring")]
//! Vec<u8> <-> Base64String: #[serde(with="serializers::bytes::base64string")]
//! Vec<u8> <-> String: #[serde(with="serializers::bytes::string")]
Expand Down
5 changes: 5 additions & 0 deletions proto/src/serializers/bytes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

/// Serialize into hexstring, deserialize from hexstring
pub mod hexstring {
use crate::prelude::*;
use serde::{Deserialize, Deserializer, Serializer};
use subtle_encoding::hex;

Expand Down Expand Up @@ -30,6 +31,7 @@ pub mod hexstring {

/// Serialize into base64string, deserialize from base64string
pub mod base64string {
use crate::prelude::*;
use serde::{Deserialize, Deserializer, Serializer};
use subtle_encoding::base64;

Expand Down Expand Up @@ -66,6 +68,7 @@ pub mod base64string {

/// Serialize into Vec<base64string>, deserialize from Vec<base64string>
pub mod vec_base64string {
use crate::prelude::*;
use serde::{Deserialize, Deserializer, Serializer};
use subtle_encoding::base64;

Expand Down Expand Up @@ -99,6 +102,7 @@ pub mod vec_base64string {

/// Serialize into Option<base64string>, deserialize from Option<base64string>
pub mod option_base64string {
use crate::prelude::*;
use serde::{Deserialize, Deserializer, Serializer};
use subtle_encoding::base64;

Expand All @@ -125,6 +129,7 @@ pub mod option_base64string {

/// Serialize into string, deserialize from string
pub mod string {
use crate::prelude::*;
use serde::{Deserialize, Deserializer, Serializer};

/// Deserialize string into Vec<u8>
Expand Down
11 changes: 6 additions & 5 deletions proto/src/serializers/from_str.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
//! Serialize and deserialize any `T` that implements [[std::str::FromStr]]
//! and [[std::fmt::Display]] from or into string. Note this can be used for
//! Serialize and deserialize any `T` that implements [[core::str::FromStr]]
//! and [[core::fmt::Display]] from or into string. Note this can be used for
//! all primitive data types.
use crate::prelude::*;
use serde::{de::Error as _, Deserialize, Deserializer, Serialize, Serializer};

/// Deserialize string into T
pub fn deserialize<'de, D, T>(deserializer: D) -> Result<T, D::Error>
where
D: Deserializer<'de>,
T: std::str::FromStr,
<T as std::str::FromStr>::Err: std::fmt::Display,
T: core::str::FromStr,
<T as core::str::FromStr>::Err: core::fmt::Display,
{
String::deserialize(deserializer)?
.parse::<T>()
Expand All @@ -19,7 +20,7 @@ where
pub fn serialize<S, T>(value: &T, serializer: S) -> Result<S::Ok, S::Error>
where
S: Serializer,
T: std::fmt::Display,
T: core::fmt::Display,
{
format!("{}", value).serialize(serializer)
}
Loading