Double signing detection and logging improvements #348
Conversation
It appears the double signing detection wasn't working correctly in cases related to round step changes because the round step is presently hardcoded to `6` in the Amino parser: https://github.com/interchainio/tendermint-rs/blob/566dfb6/src/amino_types/vote.rs#L170 We have the step information in the form of the Amino `SignedMsgType`: https://docs.rs/tendermint/0.10.0/tendermint/amino_types/signature/enum.SignedMsgType.html This commit adds a `parse_request` method which sets the step number to the following mapping suggested by @bucky: - Proposal => 0 - PreVote => 1 - PreCommit => 2 (Note: Ideally this would get fixed upstream in `tendermint-rs`) Additionally, it changes the double signing detection logic to permit a case which was previously disallowed, which is a `PreVote` for a particular block ID followed by a `PreCommit` for `<nil>`. This case was added as a new test to the existing cases, and now passes.
| let mut consensus_state = match request.consensus_state() { | ||
| Some(state) => state, | ||
| None => Err(err!(ProtocolError, "no consensus state in request"))?, | ||
| }; |
There was a problem hiding this comment.
Note that this relies on every message we sign including both of these, but I think that's the case anyway?
| request.validate()?; | ||
|
|
||
| let registry = chain::REGISTRY.get(); | ||
| let chain = registry.get_chain(&self.config.chain_id).unwrap(); | ||
|
|
||
| if let Some(request_state) = &request.consensus_state() { |
There was a problem hiding this comment.
It seems like this skips the double signing check if the requested message's consensus state parses as None, i.e. if the message doesn't contain a vote:
https://github.com/interchainio/tendermint-rs/blob/566dfb6/src/amino_types/vote.rs#L163
The new logic will return an error in that case, rather than what the old logic did, which is unwrap a None value and panic!
https://github.com/interchainio/tendermint-rs/blob/566dfb6/src/amino_types/vote.rs#L144
| @@ -70,7 +70,7 @@ impl State { | |||
| &mut self, | |||
| new_state: consensus::State, | |||
| ) -> Result<(), StateError> { | |||
| // TODO(tarcieri): rewrite this using `Ord` impl on `consensus::State` | |||
| // TODO(tarcieri): rewrite this using `PartialOrd` impl on `consensus::State` | |||
There was a problem hiding this comment.
Opened an upstream issue about how I think this should get fixed upstream in tendermint-rs to better reflect the Tendermint spec:
|
Seems like this is the best path forward for now, so I'm going to go ahead and merge this. |
It appears the double signing detection wasn't working correctly in cases related to round step changes because the round step is presently hardcoded to
6in the Amino parser:https://github.com/interchainio/tendermint-rs/blob/566dfb6/src/amino_types/vote.rs#L170
We have the step information in the form of the Amino
SignedMsgType:https://docs.rs/tendermint/0.10.0/tendermint/amino_types/signature/enum.SignedMsgType.html
This commit adds a
parse_requestmethod which sets the step number to the following mapping suggested by @ebuchman:(Note: Ideally this would get fixed upstream in
tendermint-rs. Opened informalsystems/tendermint-rs#8 to track that)Additionally, it changes the double signing detection logic to permit a case which was previously disallowed, which is a
PreVotefor a particular block ID followed by aPreCommitfor<nil>. This case was added as a new test to the existing cases, and now passes.