Skip to content
This repository has been archived by the owner on Jun 3, 2020. It is now read-only.

Double signing detection and logging improvements #348

Merged
merged 1 commit into from
Aug 13, 2019
Merged

Double signing detection and logging improvements #348

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
38 changes: 22 additions & 16 deletions src/chain/state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened an upstream issue about how I think this should get fixed upstream in tendermint-rs to better reflect the Tendermint spec:

informalsystems/tendermint-rs#9

if new_state.height < self.consensus_state.height {
fail!(
StateErrorKind::HeightRegression,
Expand Down Expand Up @@ -99,21 +99,21 @@ impl State {
)
}

if self.consensus_state.block_id != new_state.block_id &&
// If we incremented rounds allow locking on a new block
new_state.round == self.consensus_state.round &&
// if we precommitting after prevoting nil allow locking on new block
!(new_state.step == 3 && self.consensus_state.step == 2 && self.consensus_state.block_id.is_none())
if new_state.block_id != self.consensus_state.block_id &&
// disallow voting for two different block IDs during different steps
((new_state.block_id.is_some() && self.consensus_state.block_id.is_some()) ||
// disallow voting `<nil>` and for a block ID on the same step
(new_state.step == self.consensus_state.step))
{
fail!(
StateErrorKind::DoubleSign,
"Attempting to sign a second proposal at height:{} round:{} step:{} old block id:{} new block {}",
new_state.height,
new_state.round,
new_state.step,
self.consensus_state.block_id_prefix(),
new_state.block_id_prefix()
)
StateErrorKind::DoubleSign,
"Attempting to sign a second proposal at height:{} round:{} step:{} old block id:{} new block {}",
new_state.height,
new_state.round,
new_state.step,
self.consensus_state.block_id_prefix(),
new_state.block_id_prefix()
);
}
}
}
Expand Down Expand Up @@ -246,8 +246,8 @@ mod tests {

successful_update_test!(
step_update_with_nil_to_some_block_id_success,
state!(1, 1, 2, None),
state!(1, 1, 3, block_id!(EXAMPLE_BLOCK_ID))
state!(1, 1, 1, None),
state!(1, 1, 2, block_id!(EXAMPLE_BLOCK_ID))
);

successful_update_test!(
Expand All @@ -262,6 +262,12 @@ mod tests {
state!(2, 0, 0, None)
);

successful_update_test!(
step_update_with_block_id_and_nil_success,
state!(1, 0, 0, block_id!(EXAMPLE_BLOCK_ID)),
state!(1, 0, 1, None)
);

double_sign_test!(
step_update_with_different_block_id_double_sign,
state!(1, 1, 0, block_id!(EXAMPLE_BLOCK_ID)),
Expand Down
106 changes: 64 additions & 42 deletions src/session.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@ use crate::{
};
use std::{fmt::Debug, os::unix::net::UnixStream, time::Instant};
use tendermint::{
amino_types::{PingRequest, PingResponse, PubKeyRequest, PubKeyResponse, RemoteError},
consensus::state::NIL_PLACEHOLDER,
net,
amino_types::{
PingRequest, PingResponse, PubKeyRequest, PubKeyResponse, RemoteError, SignedMsgType,
},
consensus, net,
};

/// Encrypted session with a validator node
Expand Down Expand Up @@ -119,25 +120,26 @@ impl Session {
}

/// Perform a digital signature operation
fn sign<T: TendermintRequest + Debug>(&mut self, mut request: T) -> Result<Response, Error> {
fn sign<R>(&mut self, mut request: R) -> Result<Response, Error>
where
R: TendermintRequest + Debug,
{
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() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

let mut chain_state = chain.state.lock().unwrap();

if let Err(e) = chain_state.update_consensus_state(request_state.clone()) {
// Report double signing error back to the validator
if e.kind() == StateErrorKind::DoubleSign {
return self.handle_double_signing(
request,
&chain_state.consensus_state().block_id_prefix(),
);
} else {
return Err(e.into());
}
let (_, request_state) = parse_request(&request)?;
let mut chain_state = chain.state.lock().unwrap();

if let Err(e) = chain_state.update_consensus_state(request_state) {
// Report double signing error back to the validator
if e.kind() == StateErrorKind::DoubleSign {
return self.handle_double_signing(
request,
&chain_state.consensus_state().block_id_prefix(),
);
} else {
return Err(e.into());
}
}

Expand All @@ -161,7 +163,7 @@ impl Session {
let started_at = Instant::now();
let signature = chain.keyring.sign_ed25519(None, &to_sign)?;

self.log_signing_request(&request, started_at);
self.log_signing_request(&request, started_at).unwrap();
request.set_signature(&signature);

Ok(request.build_response(None))
Expand All @@ -184,43 +186,38 @@ impl Session {
}

/// Write an INFO logline about a signing request
fn log_signing_request<T: TendermintRequest + Debug>(&self, request: &T, started_at: Instant) {
let (consensus_state, block_id) = request
.consensus_state()
.map(|state| (state.to_string(), state.block_id_prefix()))
.unwrap_or_else(|| (NIL_PLACEHOLDER.to_owned(), NIL_PLACEHOLDER.to_owned()));

let msg_type = request
.msg_type()
.map(|t| format!("{:?}", t))
.unwrap_or_else(|| "Unknown".to_owned());
fn log_signing_request<R>(&self, request: &R, started_at: Instant) -> Result<(), Error>
where
R: TendermintRequest + Debug,
{
let (msg_type, request_state) = parse_request(request)?;

info!(
"[{}@{}] signed {}:{} at h/r/s {} ({} ms)",
"[{}@{}] signed {:?}:{} at h/r/s {} ({} ms)",
&self.config.chain_id,
&self.config.addr,
msg_type,
block_id,
consensus_state,
request_state.block_id_prefix(),
request_state,
started_at.elapsed().as_millis(),
);

Ok(())
}

/// Handle attempted double signing
fn handle_double_signing<T: TendermintRequest + Debug>(
fn handle_double_signing<R>(
&self,
request: T,
request: R,
original_block_id: &str,
) -> Result<Response, Error> {
let msg_type = request
.msg_type()
.map(|t| format!("{:?}", t))
.unwrap_or_else(|| "Unknown".to_owned());

let request_state = request.consensus_state().unwrap();
) -> Result<Response, Error>
where
R: TendermintRequest + Debug,
{
let (msg_type, request_state) = parse_request(&request)?;

error!(
"[{}:{}] attempted double sign for {} at h/r/s: {} ({} != {})",
"[{}:{}] attempted double sign {:?} at h/r/s: {} ({} != {})",
&self.config.chain_id,
&self.config.addr,
msg_type,
Expand All @@ -233,3 +230,28 @@ impl Session {
Ok(request.build_response(Some(remote_err)))
}
}

/// Parse the consensus state from an incoming request
// TODO(tarcieri): fix the upstream Amino parser to do this correctly for us
fn parse_request<R>(request: &R) -> Result<(SignedMsgType, consensus::State), Error>
where
R: TendermintRequest + Debug,
{
let msg_type = match request.msg_type() {
Some(ty) => ty,
None => Err(err!(ProtocolError, "no message type for this request"))?,
};

let mut consensus_state = match request.consensus_state() {
Some(state) => state,
None => Err(err!(ProtocolError, "no consensus state in request"))?,
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this relies on every message we sign including both of these, but I think that's the case anyway?


consensus_state.step = match msg_type {
SignedMsgType::Proposal => 0,
SignedMsgType::PreVote => 1,
SignedMsgType::PreCommit => 2,
};

Ok((msg_type, consensus_state))
}