This repository has been archived by the owner on Jun 3, 2020. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 45
State tracking for double sign protection #193
Merged
Merged
Changes from 12 commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
fd968f5
Work in progress of tracking the state of what we most recently signe…
4bb0c03
Cargo fmt
455179b
Add double sign detection test
11a00f2
Better Constructor
fe8b613
Merge branch 'master' of github.com:tendermint/kms into zaki/priv_val…
89204d7
Introduce state checking
80a30fb
Integrate double sign protection into session
6e01265
Fix consensus step
a798ec4
Fix formatting
12644bc
Improve the filepaths
252afe8
Please clippy
ad05e72
Update src/last_sign_state.rs
liamsi 74f6c5a
fix tests: file wasn't opened with write option but we were trying to
liamsi 73a1ac5
Remove the round regression error
bc8fceb
Delete the state file at begining of the run
c229ea4
fix formatting
9f54d31
Clean up tests at then end
5e9a2f7
Format
8927d0f
Run integration tests sequentially to prevent race conditions
9917287
Merge branch 'master' into zaki/priv_validator_state
liamsi File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,200 @@ | ||
use abscissa::Error; | ||
use serde_json; | ||
use std::{ | ||
fmt::{self, Display}, | ||
fs::File, | ||
io::prelude::*, | ||
path::Path, | ||
}; | ||
use tendermint::{block, chain}; | ||
|
||
#[derive(Serialize, Deserialize)] | ||
struct LastSignData { | ||
pub height: i64, | ||
pub round: i64, | ||
pub step: i8, | ||
pub block_id: Option<block::Id>, | ||
} | ||
|
||
const EMPTY_DATA: LastSignData = LastSignData { | ||
height: 0, | ||
round: 0, | ||
step: 0, | ||
block_id: None, | ||
}; | ||
|
||
pub struct LastSignState { | ||
data: LastSignData, | ||
file: File, | ||
_chain_id: chain::Id, | ||
} | ||
|
||
/// Error type | ||
#[derive(Debug)] | ||
pub struct LastSignError(Error<LastSignErrorKind>); | ||
|
||
#[derive(Copy, Clone, Eq, PartialEq, Debug, Fail)] | ||
pub enum LastSignErrorKind { | ||
#[fail(display = "height regression")] | ||
HeightRegression, | ||
#[fail(display = "step regression")] | ||
StepRegression, | ||
#[fail(display = "round regression")] | ||
RoundRegression, | ||
#[fail(display = "invalid block id")] | ||
DoubleSign, | ||
} | ||
|
||
impl From<Error<LastSignErrorKind>> for LastSignError { | ||
fn from(other: Error<LastSignErrorKind>) -> Self { | ||
LastSignError(other) | ||
} | ||
} | ||
|
||
impl Display for LastSignError { | ||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
self.0.fmt(f) | ||
} | ||
} | ||
|
||
impl LastSignState { | ||
pub fn load_state(path: &Path, chain_id: chain::Id) -> std::io::Result<LastSignState> { | ||
if !path.exists() { | ||
let mut lst = LastSignState { | ||
data: EMPTY_DATA, | ||
file: File::create(path)?, | ||
_chain_id: chain_id, | ||
}; | ||
lst.sync_to_disk()?; | ||
return Ok(lst); | ||
} | ||
let mut lst = LastSignState { | ||
data: EMPTY_DATA, | ||
file: File::open(path)?, | ||
_chain_id: chain_id, | ||
}; | ||
|
||
let mut contents = String::new(); | ||
lst.file.read_to_string(&mut contents)?; | ||
lst.data = serde_json::from_str(&contents)?; | ||
Ok(lst) | ||
} | ||
|
||
pub fn sync_to_disk(&mut self) -> std::io::Result<()> { | ||
self.file | ||
.write_all(serde_json::to_string(&self.data).unwrap().as_ref())?; | ||
self.file.sync_all()?; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could be made a bit more atomic by writing the new state to a separate file (e.g. |
||
Ok(()) | ||
} | ||
|
||
pub fn check_and_update_hrs( | ||
&mut self, | ||
height: i64, | ||
round: i64, | ||
step: i8, | ||
block_id: Option<block::Id>, | ||
) -> Result<(), LastSignError> { | ||
if height < self.data.height { | ||
fail!( | ||
LastSignErrorKind::HeightRegression, | ||
"last height:{} new height:{}", | ||
self.data.height, | ||
height | ||
); | ||
} | ||
if height == self.data.height { | ||
if round < self.data.round { | ||
fail!( | ||
LastSignErrorKind::RoundRegression, | ||
"round regression at height:{} last round:{} new round:{}", | ||
height, | ||
self.data.round, | ||
round | ||
) | ||
} | ||
if round == self.data.round { | ||
if step < self.data.step { | ||
fail!( | ||
LastSignErrorKind::StepRegression, | ||
"round regression at height:{} round:{} last step:{} new step:{}", | ||
height, | ||
round, | ||
self.data.step, | ||
step | ||
) | ||
} | ||
|
||
if block_id != None && self.data.block_id != None && self.data.block_id != block_id | ||
{ | ||
fail!( | ||
LastSignErrorKind::DoubleSign, | ||
"Attempting to sign a second proposal at height:{} round:{} step:{} old block id:{} new block {}", | ||
height, | ||
round, | ||
step, | ||
self.data.block_id.clone().unwrap(), | ||
block_id.unwrap() | ||
) | ||
} | ||
} | ||
} | ||
self.data.height = height; | ||
self.data.round = round; | ||
self.data.step = step; | ||
self.data.block_id = block_id; | ||
Ok(()) | ||
} | ||
} | ||
#[cfg(test)] | ||
mod tests { | ||
use super::*; | ||
use std::str::FromStr; | ||
use tendermint::{block, chain}; | ||
|
||
const EXAMPLE_BLOCK_ID: &str = | ||
"26C0A41F3243C6BCD7AD2DFF8A8D83A71D29D307B5326C227F734A1A512FE47D"; | ||
|
||
const EXAMPLE_DOUBLE_SIGN_BLOCK_ID: &str = | ||
"2470A41F3243C6BCD7AD2DFF8A8D83A71D29D307B5326C227F734A1A512FE47D"; | ||
|
||
#[test] | ||
fn hrs_test() { | ||
let mut last_sign_state = LastSignState { | ||
data: LastSignData { | ||
height: 1, | ||
round: 1, | ||
step: 0, | ||
block_id: None, | ||
}, | ||
file: File::create("/tmp/tmp_state.json").unwrap(), | ||
_chain_id: "example-chain".parse::<chain::Id>().unwrap(), | ||
}; | ||
assert_eq!( | ||
last_sign_state.check_and_update_hrs(2, 0, 0, None).unwrap(), | ||
() | ||
) | ||
} | ||
|
||
#[test] | ||
fn hrs_test_double_sign() { | ||
let mut last_sign_state = LastSignState { | ||
data: LastSignData { | ||
height: 1, | ||
round: 1, | ||
step: 0, | ||
block_id: Some(block::Id::from_str(EXAMPLE_BLOCK_ID).unwrap()), | ||
}, | ||
file: File::create("/tmp/tmp_state.json").unwrap(), | ||
_chain_id: "example-chain".parse::<chain::Id>().unwrap(), | ||
}; | ||
let double_sign_block = block::Id::from_str(EXAMPLE_DOUBLE_SIGN_BLOCK_ID).unwrap(); | ||
let err = last_sign_state.check_and_update_hrs(1, 1, 1, Some(double_sign_block)); | ||
|
||
let double_sign_error = LastSignErrorKind::DoubleSign; | ||
|
||
assert_eq!( | ||
err.expect_err("Expect Double Sign error").0.kind(), | ||
&double_sign_error | ||
) | ||
} | ||
} |
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
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,11 @@ | ||
use super::{ | ||
block_id::{BlockId, CanonicalBlockId, CanonicalPartSetHeader}, | ||
remote_error::RemoteError, | ||
signature::{SignableMsg, SignedMsgType}, | ||
signature::{ConsensusState, SignableMsg, SignedMsgType}, | ||
time::TimeMsg, | ||
validate::{ConsensusMessage, ValidationError, ValidationErrorKind::*}, | ||
}; | ||
use crate::block::ParseId; | ||
use crate::{block, chain, error::Error}; | ||
use bytes::BufMut; | ||
use prost::{EncodeError, Message}; | ||
|
@@ -129,6 +130,25 @@ impl SignableMsg for SignProposalRequest { | |
None => Err(MissingConsensusMessage.into()), | ||
} | ||
} | ||
fn consensus_state(&self) -> Option<ConsensusState> { | ||
match self.proposal { | ||
Some(ref p) => Some(ConsensusState { | ||
height: p.height, | ||
round: p.round, | ||
step: 3, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: I think it would be better to use constants / enums for this. See https://github.com/tendermint/tendermint/blob/411bc5e49fcc3dda866ae799ba2ceda0084f80d4/privval/file.go#L17-L35 |
||
block_id: { | ||
match p.block_id { | ||
Some(ref b) => match b.parse_block_id() { | ||
Ok(id) => Some(id), | ||
Err(_) => None, | ||
}, | ||
None => None, | ||
} | ||
}, | ||
}), | ||
None => None, | ||
} | ||
} | ||
} | ||
|
||
impl ConsensusMessage for Proposal { | ||
|
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these ever actually be negative?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not entirely sure. I have a vague memory that in Tendermint sometimes these values get set at -1 as a marker.
But I haven't actually looked through all the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also those are the types in the Tendermint implementation.
There was a problem hiding this comment.
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 only thing that can be negative is the Proposal.POLRound (can be -1 as @zmanian mentioned). The other fields can not be negative. But yeah the types are all signed in the tendermint golang code ... Not sure we need to follow this tho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might suggest using unsigned integers for anything that isn't explicitly negative, and potentially
Option
in lieu of-1