CanonicalVote and CanonicalProposal now correctly encode/decode BlockId

CHANGELOG.md

@@ -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:
 
@@ -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

tendermint/src/block/id.rs

@@ -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,
 )]

tendermint/src/proposal.rs

@@ -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();

tendermint/src/proposal/canonical_proposal.rs

@@ -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(),
         })
@@ -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(),
@@ -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(),
         }
@@ -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());
+    }
+}

tendermint/src/vote/canonical_vote.rs

@@ -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)?,
         })
@@ -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(),
         }
@@ -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());
+    }
+}

tendermint/src/vote/sign_vote.rs

@@ -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());