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.

Sign up for GitHub

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

block id in vote could be empty #103

Merged
merged 3 commits into from
Jan 10, 2020
Merged

block id in vote could be empty #103

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bd8eea7
block id in vote could be empty
yihuang Dec 24, 2019
7cc70f7
Merge branch 'master' into empty-block-id-in-vote
liamsi Jan 10, 2020
c586158
Remove dupl `parse_non_empty_block_id` caused by merge master hickup
liamsi Jan 10, 2020
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
6 changes: 3 additions & 3 deletions tendermint/src/amino_types/vote.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,9 @@ impl From<&vote::Vote> for Vote {
vote_type: vote.vote_type.to_u32(),
height: vote.height.value() as i64, // TODO potential overflow :-/
round: vote.round as i64,
block_id: Some(BlockId {
hash: vote.block_id.hash.as_bytes().to_vec(),
parts_header: vote.block_id.parts.as_ref().map(PartsSetHeader::from),
block_id: vote.block_id.as_ref().map(|block_id| BlockId {
Copy link
Member

Choose a reason for hiding this comment

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

Interesting that the amino_types vote had the Option but the main vote didn't - I would expect that to have been preserved. I'm not sure, but I think that might mean that if we were eg. using this code to feed the KMS, we'd end up signing something incorrect, since we'd end up with a vote for a Some(BlockId) containing an empty hash and a None header instead of a vote with a None block_id.

Copy link
Contributor Author

@yihuang yihuang Jan 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

As far as I know, the rust version of amino proto is like this:

  • None is skiped, just the same as go version skipping a default value.
  • Some(value) encodes the value as is, won't skip it if it's the default value.
    And the go version seems to automatically skip default values, so if rust version encodes Some(default_value), it won't match the go version, we need to prevent this from happening. And the new version is actually more correct in this regard.

hash: block_id.hash.as_bytes().to_vec(),
parts_header: block_id.parts.as_ref().map(PartsSetHeader::from),
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why this is an Option? Shouldn't it be always there and instead we use Option on the BlockId itself?

Copy link
Contributor Author

@yihuang yihuang Jan 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I believe there are situation when block_id has valid hash, but don't has parts_header, or have a parts_header with default values(can't recall the case for now), for the go version of protobuf, default value is skipped automatically when encoding, but rust version won't check the default value automatically, so we need an Option to keep the encoding exactly the same as the go version.

}),
timestamp: Some(TimeMsg::from(vote.timestamp)),
validator_address: vote.validator_address.as_bytes().to_vec(),
Expand Down
6 changes: 3 additions & 3 deletions tendermint/src/block.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,13 @@ mod size;

pub use self::{
commit::*,
header::{parse_non_empty_block_id, Header},
header::Header,
height::*,
id::{Id, ParseId},
meta::Meta,
size::Size,
};
use crate::{abci::transaction, evidence};
use crate::{abci::transaction, evidence, serializers};
use serde::{Deserialize, Deserializer, Serialize};

/// Blocks consist of a header, transactions, votes (the commit), and a list of
Expand Down Expand Up @@ -46,7 +46,7 @@ where
{
#[derive(Deserialize)]
struct TmpCommit {
#[serde(deserialize_with = "parse_non_empty_block_id")]
#[serde(deserialize_with = "serializers::parse_non_empty_block_id")]
block_id: Option<Id>,
precommits: Option<Precommits>,
}
Expand Down
41 changes: 2 additions & 39 deletions tendermint/src/block/header.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@ use crate::merkle::simple_hash_from_byte_vectors;
use crate::serializers;
use crate::{account, amino_types, block, chain, lite, Hash, Time};
use amino_types::{message::AminoMessage, BlockId, ConsensusVersion, TimeMsg};
use serde::{de::Error as _, Deserialize, Deserializer, Serialize};
use std::str::FromStr;
use serde::{Deserialize, Serialize};

/// Block `Header` values contain metadata about the block and about the
/// consensus, as well as commitments to the data in the current block, the
Expand Down Expand Up @@ -40,7 +39,7 @@ pub struct Header {
pub total_txs: u64,

/// Previous block info
#[serde(deserialize_with = "parse_non_empty_block_id")]
#[serde(deserialize_with = "serializers::parse_non_empty_block_id")]
pub last_block_id: Option<block::Id>,

/// Commit from validators from the last block
Expand Down Expand Up @@ -129,42 +128,6 @@ impl lite::Header for Header {
}
}

/// Parse empty block id as None.
pub fn parse_non_empty_block_id<'de, D>(deserializer: D) -> Result<Option<block::Id>, D::Error>
where
D: Deserializer<'de>,
{
#[derive(Deserialize)]
struct Parts {
#[serde(deserialize_with = "serializers::parse_u64")]
total: u64,
hash: String,
}
#[derive(Deserialize)]
struct BlockId {
hash: String,
parts: Parts,
}
let tmp_id = BlockId::deserialize(deserializer)?;
if tmp_id.hash.is_empty() {
Ok(None)
} else {
Ok(Some(block::Id {
hash: Hash::from_str(&tmp_id.hash)
.map_err(|err| D::Error::custom(format!("{}", err)))?,
parts: if tmp_id.parts.hash.is_empty() {
None
} else {
Some(block::parts::Header {
total: tmp_id.parts.total,
hash: Hash::from_str(&tmp_id.parts.hash)
.map_err(|err| D::Error::custom(format!("{}", err)))?,
})
},
}))
}
}

/// `Version` contains the protocol version for the blockchain and the
/// application.
///
Expand Down
40 changes: 39 additions & 1 deletion tendermint/src/serializers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//! Serde serializers

use crate::Hash;
use crate::{block, Hash};
use serde::{de::Error as _, Deserialize, Deserializer, Serialize, Serializer};
use std::str::FromStr;
use std::time::Duration;
Expand Down Expand Up @@ -144,3 +144,41 @@ where
let v = Option::deserialize(deserializer)?;
Ok(v.map(|Wrapper(a)| a))
}

/// Parse empty block id as None.
pub(crate) fn parse_non_empty_block_id<'de, D>(
Copy link
Member

Choose a reason for hiding this comment

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

I'm kind of hoping this will go away with #101. In which case maybe we'll want to use an explicit VoteValue enum with variants BlockId and Nil ...

Copy link
Contributor Author

@yihuang yihuang Jan 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I actually don't think enum would help us in these cases, because it's adding a branch just like Option, you just move the branch from Option to your own type.
The core problem lies in the protobuf semantic of optional field and default values, and the inconsistency between json and binary version of protobuf encoding.
In the semantic of protobuf, basically, there's no separation between null value and default value, they can be encoded as the same(skip), but in the json version, the default value is encoded as is, not skipped or null, it is actually correct in protobuf's semantic, because a default value is the same as a skipped or null value, but when we use Option to distingish null value and default value in rust, we need extra effort to handle this.

deserializer: D,
) -> Result<Option<block::Id>, D::Error>
where
D: Deserializer<'de>,
{
#[derive(Deserialize)]
struct Parts {
#[serde(deserialize_with = "parse_u64")]
total: u64,
hash: String,
}
#[derive(Deserialize)]
struct BlockId {
hash: String,
parts: Parts,
}
let tmp_id = BlockId::deserialize(deserializer)?;
if tmp_id.hash.is_empty() {
Ok(None)
} else {
Ok(Some(block::Id {
hash: Hash::from_str(&tmp_id.hash)
.map_err(|err| D::Error::custom(format!("{}", err)))?,
parts: if tmp_id.parts.hash.is_empty() {
None
} else {
Some(block::parts::Header {
total: tmp_id.parts.total,
hash: Hash::from_str(&tmp_id.parts.hash)
.map_err(|err| D::Error::custom(format!("{}", err)))?,
})
},
}))
}
}
3 changes: 2 additions & 1 deletion tendermint/src/vote.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ pub struct Vote {
pub round: u64,

/// Block ID
pub block_id: block::Id,
#[serde(deserialize_with = "serializers::parse_non_empty_block_id")]
pub block_id: Option<block::Id>,

/// Timestamp
pub timestamp: Time,
Expand Down
16 changes: 16 additions & 0 deletions tendermint/tests/rpc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,22 @@ mod endpoints {
assert_eq!(last_commit.unwrap().precommits.len(), 65);
}

#[test]
fn block_empty_block_id() {
let response =
endpoint::block::Response::from_string(&read_json_fixture("block_empty_block_id"))
.unwrap();

let tendermint::Block { last_commit, .. } = response.block;

assert_eq!(last_commit.as_ref().unwrap().precommits.len(), 2);
assert!(last_commit.unwrap().precommits[0]
.as_ref()
.unwrap()
.block_id
.is_none());
}

#[test]
fn first_block() {
let response =
Expand Down
120 changes: 120 additions & 0 deletions tendermint/tests/support/rpc/block_empty_block_id.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
{
"jsonrpc": "2.0",
"id": "",
"result": {
"block_meta": {
"block_id": {
"hash": "17E077714CE817C0DB3118D21EA060B2A16D9F07CB08AB603292F797FFD7F361",
"parts": {
"total": "1",
"hash": "1ED5B68A21D82D091C25D535F52C2D47419162618245C60D69B162A366F97A45"
}
},
"header": {
"version": {
"block": "10",
"app": "0"
},
"chain_id": "test-ab",
"height": "20",
"time": "2019-12-24T11:03:18.276852105Z",
"num_txs": "0",
"total_txs": "0",
"last_block_id": {
"hash": "08150B2F0F9D873C5446D77BF10EF1FD33A6C3FDE03BB76CC99F3A047A7A93CB",
"parts": {
"total": "1",
"hash": "EF93CDE980C98BD4720ADA7984121B4E0E153B004BB94E583809281C576FFAB1"
}
},
"last_commit_hash": "4D58DEA6B78D344255F6A7B0756F18D98DDFDF870D930337E13B6C7236BD07B0",
"data_hash": "",
"validators_hash": "A3E1386AC4A481A2A9DEB063B4D85973F1B789A2ECB15730B7E77FFFC698EC01",
"next_validators_hash": "A3E1386AC4A481A2A9DEB063B4D85973F1B789A2ECB15730B7E77FFFC698EC01",
"consensus_hash": "048091BC7DDC283F77BFBF91D73C44DA58C3DF8A9CBC867405D8B7F3DAADA22F",
"app_hash": "98F00A4EA270197259504954450D5ED5FA878284920FCC08070385C06F9B6515",
"last_results_hash": "",
"evidence_hash": "",
"proposer_address": "F18E6EE0D8AD707CED69D0606F30035B07E37DDF"
}
},
"block": {
"header": {
"version": {
"block": "10",
"app": "0"
},
"chain_id": "test-ab",
"height": "20",
"time": "2019-12-24T11:03:18.276852105Z",
"num_txs": "0",
"total_txs": "0",
"last_block_id": {
"hash": "08150B2F0F9D873C5446D77BF10EF1FD33A6C3FDE03BB76CC99F3A047A7A93CB",
"parts": {
"total": "1",
"hash": "EF93CDE980C98BD4720ADA7984121B4E0E153B004BB94E583809281C576FFAB1"
}
},
"last_commit_hash": "4D58DEA6B78D344255F6A7B0756F18D98DDFDF870D930337E13B6C7236BD07B0",
"data_hash": "",
"validators_hash": "A3E1386AC4A481A2A9DEB063B4D85973F1B789A2ECB15730B7E77FFFC698EC01",
"next_validators_hash": "A3E1386AC4A481A2A9DEB063B4D85973F1B789A2ECB15730B7E77FFFC698EC01",
"consensus_hash": "048091BC7DDC283F77BFBF91D73C44DA58C3DF8A9CBC867405D8B7F3DAADA22F",
"app_hash": "98F00A4EA270197259504954450D5ED5FA878284920FCC08070385C06F9B6515",
"last_results_hash": "",
"evidence_hash": "",
"proposer_address": "F18E6EE0D8AD707CED69D0606F30035B07E37DDF"
},
"data": {
"txs": null
},
"evidence": {
"evidence": null
},
"last_commit": {
"block_id": {
"hash": "08150B2F0F9D873C5446D77BF10EF1FD33A6C3FDE03BB76CC99F3A047A7A93CB",
"parts": {
"total": "1",
"hash": "EF93CDE980C98BD4720ADA7984121B4E0E153B004BB94E583809281C576FFAB1"
}
},
"precommits": [
{
"type": 2,
"height": "19",
"round": "0",
"block_id": {
"hash": "",
"parts": {
"total": "0",
"hash": ""
}
},
"timestamp": "2019-12-24T11:03:18.534146209Z",
"validator_address": "9ACE9BC3DC3455BC89427F53172077D84E54D5BD",
"validator_index": "0",
"signature": "1Go3/vHT/g9tplm3TtaiFupZG1tYYycLf4HXbVJvN3lWD91KwZA4SNKSfzRX5ccOQXy1QTqjLk1Zquc/4w00DQ=="
},
{
"type": 2,
"height": "19",
"round": "0",
"block_id": {
"hash": "08150B2F0F9D873C5446D77BF10EF1FD33A6C3FDE03BB76CC99F3A047A7A93CB",
"parts": {
"total": "1",
"hash": "EF93CDE980C98BD4720ADA7984121B4E0E153B004BB94E583809281C576FFAB1"
}
},
"timestamp": "2019-12-24T11:03:18.276852105Z",
"validator_address": "F18E6EE0D8AD707CED69D0606F30035B07E37DDF",
"validator_index": "1",
"signature": "pE4CEh2HnXmZ8UfCcvuQ/3udpzoWTT9cxRzEv5XdTyV+WEQNna8oO2uflFc/5wORB/DoGmUjOlfS512HcfFuBQ=="
}
]
}
}
}
}