-
Notifications
You must be signed in to change notification settings - Fork 43
State tracking for double sign protection #193
Changes from all commits
fd968f5
4bb0c03
455179b
11a00f2
fe8b613
89204d7
80a30fb
6e01265
a798ec4
12644bc
252afe8
ad05e72
74f6c5a
73a1ac5
bc8fceb
c229ea4
9f54d31
5e9a2f7
8927d0f
9917287
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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, OpenOptions}, | ||
io::prelude::*, | ||
path::Path, | ||
}; | ||
use tendermint::{block, chain}; | ||
|
||
#[derive(Serialize, Deserialize)] | ||
struct LastSignData { | ||
pub height: i64, | ||
pub round: i64, | ||
pub step: i8, | ||
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. Can these ever actually be negative? 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. 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 commentThe 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 commentThe 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 commentThe 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 |
||
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: OpenOptions::new().read(true).write(true).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)?.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 | ||
) | ||
} | ||
} |
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 { | ||
|
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.
A simpler (or faster) way to make sure tests-files do not interfere with each other would be to use different chain-ids per test