Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate signer to be in validator set (BFT-384) #45

Merged
merged 5 commits into from
Dec 5, 2023
Merged

Validate signer to be in validator set (BFT-384) #45

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
958212a
Add signer in validator set check
moshababo Dec 1, 2023
e84c3e1
Add comments
moshababo Dec 1, 2023
5415632
Merge branch 'main' into signer_in_val_set
moshababo Dec 4, 2023
71f7d78
Add error field to error message
moshababo Dec 4, 2023
55025a4
Remove `replica_prepare_non_validator_signer_one_from_many`
moshababo Dec 4, 2023
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
14 changes: 14 additions & 0 deletions node/actors/bft/src/leader/replica_commit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,12 @@ pub(crate) enum Error {
/// Local version.
local_version: ProtocolVersion,
},
/// Message signer isn't part of the validator set.
#[error("Message signer isn't part of the validator set (signer: {signer:?})")]
NonValidatorSigner {
/// Signer of the message.
signer: validator::PublicKey,
},
/// Unexpected proposal.
#[error("unexpected proposal")]
UnexpectedProposal,
Expand Down Expand Up @@ -77,6 +83,14 @@ impl StateMachine {
});
}

// Check that the message signer is in the validator set.
consensus
.validator_set
.index(author)
.ok_or(Error::NonValidatorSigner {
signer: author.clone(),
})?;
moshababo marked this conversation as resolved.
Show resolved Hide resolved

// If the message is from the "past", we discard it.
if (message.view, validator::Phase::Commit) < (self.view, self.phase) {
return Err(Error::Old {
Expand Down
14 changes: 14 additions & 0 deletions node/actors/bft/src/leader/replica_prepare.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,12 @@ pub(crate) enum Error {
/// Local version.
local_version: ProtocolVersion,
},
/// Message signer isn't part of the validator set.
#[error("Message signer isn't part of the validator set (signer: {signer:?})")]
NonValidatorSigner {
/// Signer of the message.
signer: validator::PublicKey,
moshababo marked this conversation as resolved.
Show resolved Hide resolved
},
/// Past view or phase.
#[error("past view/phase (current view: {current_view:?}, current phase: {current_phase:?})")]
Old {
Expand Down Expand Up @@ -89,6 +95,14 @@ impl StateMachine {
});
}

// Check that the message signer is in the validator set.
consensus
.validator_set
.index(author)
.ok_or(Error::NonValidatorSigner {
signer: author.clone(),
})?;
moshababo marked this conversation as resolved.
Show resolved Hide resolved

// If the message is from the "past", we discard it.
if (message.view, validator::Phase::Prepare) < (self.view, self.phase) {
return Err(Error::Old {
Expand Down
89 changes: 52 additions & 37 deletions node/actors/bft/src/leader/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,24 +56,6 @@ async fn replica_prepare_sanity_yield_leader_prepare() {
);
}

#[tokio::test]
async fn replica_prepare_incompatible_protocol_version() {
let mut util = UTHarness::new_one().await;

let incompatible_protocol_version = util.incompatible_protocol_version();
let replica_prepare = util.new_current_replica_prepare(|msg| {
msg.protocol_version = incompatible_protocol_version;
});
let res = util.dispatch_replica_prepare_one(replica_prepare);
assert_matches!(
res,
Err(ReplicaPrepareError::IncompatibleProtocolVersion { message_version, local_version }) => {
assert_eq!(message_version, incompatible_protocol_version);
assert_eq!(local_version, util.protocol_version());
}
)
}

#[tokio::test]
async fn replica_prepare_sanity_yield_leader_prepare_reproposal() {
let mut util = UTHarness::new_many().await;
Expand Down Expand Up @@ -119,6 +101,41 @@ async fn replica_prepare_sanity_yield_leader_prepare_reproposal() {
);
}

#[tokio::test]
async fn replica_prepare_incompatible_protocol_version() {
let mut util = UTHarness::new_one().await;

let incompatible_protocol_version = util.incompatible_protocol_version();
let replica_prepare = util.new_current_replica_prepare(|msg| {
msg.protocol_version = incompatible_protocol_version;
});
let res = util.dispatch_replica_prepare_one(replica_prepare);
assert_matches!(
res,
Err(ReplicaPrepareError::IncompatibleProtocolVersion { message_version, local_version }) => {
assert_eq!(message_version, incompatible_protocol_version);
assert_eq!(local_version, util.protocol_version());
}
)
}

#[tokio::test]
async fn replica_prepare_non_validator_signer() {
let mut util = UTHarness::new_one().await;

let replica_prepare = util.new_current_replica_prepare(|_| {}).cast().unwrap().msg;
let non_validator_key: validator::SecretKey = util.rng().gen();
let signed = non_validator_key.sign_msg(ConsensusMsg::ReplicaPrepare(replica_prepare));

let res = util.dispatch_replica_prepare_one(signed);
assert_matches!(
res,
Err(ReplicaPrepareError::NonValidatorSigner { signer }) => {
assert_eq!(signer, non_validator_key.public());
}
);
}

#[tokio::test]
async fn replica_prepare_old_view() {
let mut util = UTHarness::new_one().await;
Expand Down Expand Up @@ -272,25 +289,6 @@ async fn replica_prepare_high_qc_of_future_view() {
);
}

#[ignore = "fails/unsupported"]
#[tokio::test]
async fn replica_prepare_non_validator_signer() {
moshababo marked this conversation as resolved.
Show resolved Hide resolved
let mut util = UTHarness::new_with(2).await;

let view = ViewNumber(2);
util.set_view(view);
assert_eq!(util.view_leader(view), util.key_at(0).public());

let replica_prepare = util.new_current_replica_prepare(|_| {});
let _ = util.dispatch_replica_prepare_one(replica_prepare.clone());

let non_validator: validator::SecretKey = util.rng().gen();
let replica_prepare = non_validator.sign_msg(replica_prepare.msg);
util.dispatch_replica_prepare_one(replica_prepare).unwrap();
// PANICS:
// "Couldn't create justification from valid replica messages!: Message signer isn't in the validator set"
}

#[tokio::test]
async fn replica_commit_sanity() {
let mut util = UTHarness::new_many().await;
Expand Down Expand Up @@ -356,6 +354,23 @@ async fn replica_commit_incompatible_protocol_version() {
)
}

#[tokio::test]
async fn replica_commit_non_validator_signer() {
let mut util = UTHarness::new_one().await;

let replica_commit = util.new_current_replica_commit(|_| {}).cast().unwrap().msg;
let non_validator_key: validator::SecretKey = util.rng().gen();
let signed = non_validator_key.sign_msg(ConsensusMsg::ReplicaCommit(replica_commit));

let res = util.dispatch_replica_commit_one(signed);
assert_matches!(
res,
Err(ReplicaCommitError::NonValidatorSigner { signer }) => {
assert_eq!(signer, non_validator_key.public());
}
);
}

#[tokio::test]
async fn replica_commit_old() {
let mut util = UTHarness::new_one().await;
Expand Down
2 changes: 2 additions & 0 deletions node/actors/bft/src/testonly/ut_harness.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,7 @@ impl UTHarness {
.unwrap()
}

#[allow(clippy::result_large_err)]
pub(crate) fn dispatch_replica_commit_one(
&mut self,
msg: Signed<ConsensusMsg>,
Expand All @@ -334,6 +335,7 @@ impl UTHarness {
)
}

#[allow(clippy::result_large_err)]
pub(crate) fn dispatch_replica_commit_many(
&mut self,
messages: Vec<ReplicaCommit>,
Expand Down
Loading