Skip to content

Commit

Permalink
CanonicalVote and CanonicalProposal now correctly encode/decode Block…
Browse files Browse the repository at this point in the history 
…Id (#664)

* CanonicalVote and CanonicalProposal now correctly encode/decode CanonicalBlockId

* Update CHANGELOG.md

Co-authored-by: Thane Thomson <thane@informal.systems>

* Added regression tests to empty block id

* Fix minor spelling mistake

Co-authored-by: Thane Thomson <thane@informal.systems>
  • Loading branch information
@greg-szabo @thanethomson
greg-szabo and thanethomson authored Nov 10, 2020
1 parent 9b6239d commit 7ab136c
Show file tree
Hide file tree
Showing 6 changed files with 265 additions and 21 deletions.
7 changes: 5 additions & 2 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@
deemed valid instead of invalid ([#650])
- `[light-client]` Revert a change introduced in [#652] that would enable DoS attacks,
where full nodes could spam the light client with massive commits (eg. 10k validators).
- `[tendermint]` (Since RC1) Time serialization fix (part of [#665])
- `[tendermint-proto]` (Since RC1) Timestamp serialization fix (part of [#665])
- `[tendermint]` (Since v0.17.0-rc1) CanonicalBlockId is now correctly decoded to `None`
in `CanonicalVote` and `CanonicalProposal` when its hash is empty.
- `[tendermint]` (Since v0.17.0-rc1) Time serialization fix (part of [#665])
- `[tendermint-proto]` (Since v0.17.0-rc1) Timestamp serialization fix (part of [#665])

### FEATURES:

Expand All @@ -27,6 +29,7 @@
[#652]: https://github.com/informalsystems/tendermint-rs/pulls/652
[#639]: https://github.com/informalsystems/tendermint-rs/pull/639
[#660]: https://github.com/informalsystems/tendermint-rs/issues/660
[#663]: https://github.com/informalsystems/tendermint-rs/issues/663
[#665]: https://github.com/informalsystems/tendermint-rs/issues/665

## v0.17.0-rc1
Expand Down
4 changes: 4 additions & 0 deletions tendermint/src/block/id.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ pub const PREFIX_LENGTH: usize = 10;
///
/// Default implementation is an empty Id as defined by the Go implementation in
/// https://github.com/tendermint/tendermint/blob/1635d1339c73ae6a82e062cd2dc7191b029efa14/types/block.go#L1204 .
///
/// If the Hash is empty in BlockId, the BlockId should be empty (encoded to None).
/// This is implemented outside of this struct. Use the Default trait to check for an empty BlockId.
/// See: https://github.com/informalsystems/tendermint-rs/issues/663
#[derive(
Serialize, Deserialize, Copy, Clone, Debug, Default, Hash, Eq, PartialEq, PartialOrd, Ord,
)]
Expand Down
80 changes: 80 additions & 0 deletions tendermint/src/proposal.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,86 @@ mod tests {
assert_eq!(got, want)
}

#[test]
// Test proposal encoding with a malformed block ID which is considered null in Go.
fn test_encoding_with_empty_block_id() {
let dt = "2018-02-11T07:09:22.765Z".parse::<DateTime<Utc>>().unwrap();
let proposal = Proposal {
msg_type: Type::Proposal,
height: Height::from(12345_u32),
round: Round::from(23456_u16),
pol_round: None,
block_id: Some(BlockId {
hash: Hash::from_hex_upper(Algorithm::Sha256, "").unwrap(),
part_set_header: Header::new(
65535,
Hash::from_hex_upper(
Algorithm::Sha256,
"0022446688AACCEE1133557799BBDDFF0022446688AACCEE1133557799BBDDFF",
)
.unwrap(),
)
.unwrap(),
}),
timestamp: Some(dt.into()),
signature: Signature::Ed25519(Ed25519Signature::new([0; ED25519_SIGNATURE_SIZE])),
};

let mut got = vec![];

let request = SignProposalRequest {
proposal,
chain_id: ChainId::from_str("test_chain_id").unwrap(),
};

let _have = request.to_signable_bytes(&mut got);

// the following vector is generated via:
/*
import (
"encoding/hex"
"fmt"
prototypes "github.com/tendermint/tendermint/proto/tendermint/types"
"github.com/tendermint/tendermint/types"
"strings"
"time"
)
func proposalSerialize() {
stamp, _ := time.Parse(time.RFC3339Nano, "2018-02-11T07:09:22.765Z")
block_hash, _ := hex.DecodeString("")
part_hash, _ := hex.DecodeString("0022446688AACCEE1133557799BBDDFF0022446688AACCEE1133557799BBDDFF")
proposal := &types.Proposal{
Type: prototypes.SignedMsgType(prototypes.ProposalType),
Height: 12345,
Round: 23456,
POLRound: -1,
BlockID: types.BlockID{
Hash: block_hash,
PartSetHeader: types.PartSetHeader{
Hash: part_hash,
Total: 65535,
},
},
Timestamp: stamp,
}
signBytes := types.ProposalSignBytes("test_chain_id", proposal.ToProto())
fmt.Println(strings.Join(strings.Split(fmt.Sprintf("%v", signBytes), " "), ", "))
}
*/

let want = vec![
102, 8, 32, 17, 57, 48, 0, 0, 0, 0, 0, 0, 25, 160, 91, 0, 0, 0, 0, 0, 0, 32, 255, 255,
255, 255, 255, 255, 255, 255, 255, 1, 42, 40, 18, 38, 8, 255, 255, 3, 18, 32, 0, 34,
68, 102, 136, 170, 204, 238, 17, 51, 85, 119, 153, 187, 221, 255, 0, 34, 68, 102, 136,
170, 204, 238, 17, 51, 85, 119, 153, 187, 221, 255, 50, 12, 8, 162, 216, 255, 211, 5,
16, 192, 242, 227, 236, 2, 58, 13, 116, 101, 115, 116, 95, 99, 104, 97, 105, 110, 95,
105, 100,
];

assert_eq!(got, want)
}

#[test]
fn test_deserialization() {
let dt = "2018-02-11T07:09:22.765Z".parse::<DateTime<Utc>>().unwrap();
Expand Down
46 changes: 44 additions & 2 deletions tendermint/src/proposal/canonical_proposal.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,15 @@ impl TryFrom<RawCanonicalProposal> for CanonicalProposal {
i32::try_from(n).map_err(|e| Kind::IntegerOverflow.context(e))?,
)?),
};
// If the Hash is empty in BlockId, the BlockId should be empty.
// See: https://github.com/informalsystems/tendermint-rs/issues/663
let block_id = value.block_id.filter(|i| !i.hash.is_empty());
Ok(CanonicalProposal {
msg_type: value.r#type.try_into()?,
height: value.height.try_into()?,
round,
pol_round,
block_id: value.block_id.map(TryInto::try_into).transpose()?,
block_id: block_id.map(TryInto::try_into).transpose()?,
timestamp: value.timestamp.map(TryInto::try_into).transpose()?,
chain_id: ChainId::try_from(value.chain_id).unwrap(),
})
Expand All @@ -60,6 +63,9 @@ impl TryFrom<RawCanonicalProposal> for CanonicalProposal {

impl From<CanonicalProposal> for RawCanonicalProposal {
fn from(value: CanonicalProposal) -> Self {
// If the Hash is empty in BlockId, the BlockId should be empty.
// See: https://github.com/informalsystems/tendermint-rs/issues/663
let block_id = value.block_id.filter(|i| i != &BlockId::default());
RawCanonicalProposal {
r#type: value.msg_type.into(),
height: value.height.into(),
Expand All @@ -68,7 +74,7 @@ impl From<CanonicalProposal> for RawCanonicalProposal {
None => -1,
Some(p) => i32::from(p) as i64,
},
block_id: value.block_id.map(Into::into),
block_id: block_id.map(Into::into),
timestamp: value.timestamp.map(Into::into),
chain_id: value.chain_id.as_str().to_string(),
}
Expand All @@ -89,3 +95,39 @@ impl CanonicalProposal {
}
}
}

#[cfg(test)]
mod tests {
use crate::proposal::canonical_proposal::CanonicalProposal;
use crate::proposal::Type;
use std::convert::TryFrom;
use tendermint_proto::types::CanonicalBlockId as RawCanonicalBlockId;
use tendermint_proto::types::CanonicalPartSetHeader as RawCanonicalPartSetHeader;
use tendermint_proto::types::CanonicalProposal as RawCanonicalProposal;

#[test]
fn canonical_proposal_domain_checks() {
// RawCanonicalProposal with edge cases to test domain knowledge
// pol_round = -1 should decode to None
// block_id with empty hash should decode to None
let proto_cp = RawCanonicalProposal {
r#type: 32,
height: 2,
round: 4,
pol_round: -1,
block_id: Some(RawCanonicalBlockId {
hash: vec![],
part_set_header: Some(RawCanonicalPartSetHeader {
total: 1,
hash: vec![1],
}),
}),
timestamp: None,
chain_id: "testchain".to_string(),
};
let cp = CanonicalProposal::try_from(proto_cp).unwrap();
assert_eq!(cp.msg_type, Type::Proposal);
assert!(cp.pol_round.is_none());
assert!(cp.block_id.is_none());
}
}
70 changes: 53 additions & 17 deletions tendermint/src/vote/canonical_vote.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,18 +43,14 @@ impl TryFrom<RawCanonicalVote> for CanonicalVote {
// CanonicalVote uses sfixed64, Vote uses int32. They translate to u64 vs i32 in Rust.
return Err(IntegerOverflow.into());
}
// The JSON encoding says, if the Hash is empty in BlockId, the BlockId should be empty.
let block_id =
if value.block_id.is_some() && value.block_id.clone().unwrap().hash.is_empty() {
None
} else {
value.block_id.map(TryInto::try_into).transpose()?
};
// If the Hash is empty in BlockId, the BlockId should be empty.
// See: https://github.com/informalsystems/tendermint-rs/issues/663
let block_id = value.block_id.filter(|i| !i.hash.is_empty());
Ok(CanonicalVote {
vote_type: value.r#type.try_into()?,
height: value.height.try_into()?,
round: (value.round as i32).try_into()?,
block_id,
block_id: block_id.map(TryInto::try_into).transpose()?,
timestamp: value.timestamp.map(TryInto::try_into).transpose()?,
chain_id: ChainId::try_from(value.chain_id)?,
})
Expand All @@ -63,19 +59,14 @@ impl TryFrom<RawCanonicalVote> for CanonicalVote {

impl From<CanonicalVote> for RawCanonicalVote {
fn from(value: CanonicalVote) -> Self {
// The JSON encoding says, if the Hash is empty in BlockId, the BlockId should be empty.
// Todo: Does the protobuf encoding have the same rule?
let block_id =
if value.block_id.is_some() && value.block_id.clone().unwrap().hash.is_empty() {
None
} else {
value.block_id.map(Into::into)
};
// If the Hash is empty in BlockId, the BlockId should be empty.
// See: https://github.com/informalsystems/tendermint-rs/issues/663
let block_id = value.block_id.filter(|i| i != &block::Id::default());
RawCanonicalVote {
r#type: value.vote_type.into(),
height: value.height.into(),
round: value.round.value().into(),
block_id,
block_id: block_id.map(Into::into),
timestamp: value.timestamp.map(Into::into),
chain_id: value.chain_id.to_string(),
}
Expand All @@ -95,3 +86,48 @@ impl CanonicalVote {
}
}
}

#[cfg(test)]
mod tests {
use crate::vote::canonical_vote::CanonicalVote;
use crate::vote::Type;
use std::convert::TryFrom;
use tendermint_proto::google::protobuf::Timestamp;
use tendermint_proto::types::CanonicalBlockId as RawCanonicalBlockId;
use tendermint_proto::types::CanonicalPartSetHeader as RawCanonicalPartSetHeader;
use tendermint_proto::types::CanonicalVote as RawCanonicalVote;

#[test]
fn canonical_vote_domain_checks() {
// RawCanonicalVote with edge cases to test domain knowledge
// block_id with empty hash should decode to None
// timestamp at EPOCH is still considered valid time
let proto_cp = RawCanonicalVote {
r#type: 1,
height: 2,
round: 4,
block_id: Some(RawCanonicalBlockId {
hash: vec![],
part_set_header: Some(RawCanonicalPartSetHeader {
total: 1,
hash: vec![1],
}),
}),
timestamp: Some(Timestamp {
seconds: 0,
nanos: 0,
}),
chain_id: "testchain".to_string(),
};
let cp = CanonicalVote::try_from(proto_cp).unwrap();
assert_eq!(cp.vote_type, Type::Prevote);
assert!(cp.block_id.is_none());
assert!(cp.timestamp.is_some());

// No timestamp is not acceptable
// See: https://github.com/informalsystems/tendermint-rs/issues/649
let mut proto_cp: RawCanonicalVote = cp.into();
proto_cp.timestamp = None;
assert!(CanonicalVote::try_from(proto_cp).is_err());
}
}
79 changes: 79 additions & 0 deletions tendermint/src/vote/sign_vote.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,85 @@ mod tests {
assert_eq!(got2, want);
}

#[test]
// Test vote encoding with a malformed block_id (no hash) which is considered nil in Go.
fn test_vote_encoding_with_empty_block_id() {
let dt = "2017-12-25T03:00:01.234Z".parse::<DateTime<Utc>>().unwrap();
let vote = Vote {
vote_type: Type::Prevote,
height: Height::from(12345_u32),
round: Round::from(2_u16),
timestamp: Some(dt.into()),
block_id: Some(BlockId {
hash: Hash::try_from(b"".to_vec()).unwrap(),
part_set_header: Header::new(
1_000_000,
Hash::try_from(b"0022446688AACCEE1133557799BBDDFF".to_vec()).unwrap(),
)
.unwrap(),
}),
validator_address: AccountId::try_from(vec![
0xa3, 0xb2, 0xcc, 0xdd, 0x71, 0x86, 0xf1, 0x68, 0x5f, 0x21, 0xf2, 0x48, 0x2a, 0xf4,
0xfb, 0x34, 0x46, 0xa8, 0x4b, 0x35,
])
.unwrap(),
validator_index: ValidatorIndex::try_from(56789).unwrap(),
signature: Signature::try_from(vec![
130u8, 246, 183, 50, 153, 248, 28, 57, 51, 142, 55, 217, 194, 24, 134, 212, 233,
100, 211, 10, 24, 174, 179, 117, 41, 65, 141, 134, 149, 239, 65, 174, 217, 42, 6,
184, 112, 17, 7, 97, 255, 221, 252, 16, 60, 144, 30, 212, 167, 39, 67, 35, 118,
192, 133, 130, 193, 115, 32, 206, 152, 91, 173, 10,
])
.unwrap(),
};

let request = SignVoteRequest {
vote,
chain_id: ChainId::from_str("test_chain_id").unwrap(),
};

let got = request.to_signable_vec().unwrap();

// the following vector is generated via:
/*
import (
"fmt"
prototypes "github.com/tendermint/tendermint/proto/tendermint/types"
"github.com/tendermint/tendermint/types"
"strings"
"time"
)
func voteSerialize() {
stamp, _ := time.Parse(time.RFC3339Nano, "2017-12-25T03:00:01.234Z")
vote := &types.Vote{
Type: prototypes.PrevoteType, // pre-vote
Height: 12345,
Round: 2,
Timestamp: stamp,
BlockID: types.BlockID{
Hash: []byte(""),
PartSetHeader: types.PartSetHeader{
Total: 1000000,
Hash: []byte("0022446688AACCEE1133557799BBDDFF"),
},
},
ValidatorAddress: []byte{0xa3, 0xb2, 0xcc, 0xdd, 0x71, 0x86, 0xf1, 0x68, 0x5f, 0x21,
0xf2, 0x48, 0x2a, 0xf4, 0xfb, 0x34, 0x46, 0xa8, 0x4b, 0x35}, ValidatorIndex: 56789}
signBytes := types.VoteSignBytes("test_chain_id", vote.ToProto())
fmt.Println(strings.Join(strings.Split(fmt.Sprintf("%v", signBytes), " "), ", "))
}
*/

let want = vec![
90, 8, 1, 17, 57, 48, 0, 0, 0, 0, 0, 0, 25, 2, 0, 0, 0, 0, 0, 0, 0, 34, 40, 18, 38, 8,
192, 132, 61, 18, 32, 48, 48, 50, 50, 52, 52, 54, 54, 56, 56, 65, 65, 67, 67, 69, 69,
49, 49, 51, 51, 53, 53, 55, 55, 57, 57, 66, 66, 68, 68, 70, 70, 42, 11, 8, 177, 211,
129, 210, 5, 16, 128, 157, 202, 111, 50, 13, 116, 101, 115, 116, 95, 99, 104, 97, 105,
110, 95, 105, 100,
];
assert_eq!(got, want);
}

#[test]
fn test_sign_bytes_compatibility() {
let cv = CanonicalVote::new(Vote::default(), ChainId::try_from("A").unwrap());
Expand Down

0 comments on commit 7ab136c

Please sign in to comment.