This repository has been archived by the owner on Jun 3, 2020. It is now read-only.
Double signing detection and logging improvements #348
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
tarcieri
commented
Aug 9, 2019
| 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?
tarcieri
commented
Aug 9, 2019
| 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:
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!
tarcieri
commented
Aug 9, 2019
| @@ -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. |
Merged
Merged
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.