From 15094ba49e1062f90b1bb996679e54e2a7dedef4 Mon Sep 17 00:00:00 2001 From: Andreas Fackler Date: Tue, 14 Jul 2020 11:02:42 +0200 Subject: [PATCH 1/4] Rename validate_signature to verify_signature. That's much more common in cryptography. --- node/src/components/consensus/highway_core/highway.rs | 2 +- node/src/components/consensus/highway_core/state.rs | 2 +- node/src/components/consensus/protocols/highway.rs | 2 +- node/src/components/consensus/traits.rs | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/node/src/components/consensus/highway_core/highway.rs b/node/src/components/consensus/highway_core/highway.rs index 36e91a20fe..3f8ea788d4 100644 --- a/node/src/components/consensus/highway_core/highway.rs +++ b/node/src/components/consensus/highway_core/highway.rs @@ -239,7 +239,7 @@ impl Highway { fn do_pre_validate_vertex(&self, vertex: &Vertex) -> Result<(), VertexError> { match vertex { Vertex::Vote(vote) => { - if !C::validate_signature(&vote.hash(), self.validator_pk(&vote), &vote.signature) { + if !C::verify_signature(&vote.hash(), self.validator_pk(&vote), &vote.signature) { return Err(VoteError::Signature.into()); } Ok(self.state.pre_validate_vote(vote)?) diff --git a/node/src/components/consensus/highway_core/state.rs b/node/src/components/consensus/highway_core/state.rs index 03c8d03ac3..8874941e49 100644 --- a/node/src/components/consensus/highway_core/state.rs +++ b/node/src/components/consensus/highway_core/state.rs @@ -542,7 +542,7 @@ pub(crate) mod tests { hasher.finish() } - fn validate_signature( + fn verify_signature( hash: &Self::Hash, public_key: &Self::ValidatorId, signature: &::Signature, diff --git a/node/src/components/consensus/protocols/highway.rs b/node/src/components/consensus/protocols/highway.rs index 4da37ba976..13b1685ac2 100644 --- a/node/src/components/consensus/protocols/highway.rs +++ b/node/src/components/consensus/protocols/highway.rs @@ -377,7 +377,7 @@ impl Context for HighwayContext { hash(data) } - fn validate_signature(hash: &Digest, public_key: &PublicKey, signature: &Signature) -> bool { + fn verify_signature(hash: &Digest, public_key: &PublicKey, signature: &Signature) -> bool { verify(hash, signature, public_key).is_ok() } } diff --git a/node/src/components/consensus/traits.rs b/node/src/components/consensus/traits.rs index aa0282ce15..dd4c3beff7 100644 --- a/node/src/components/consensus/traits.rs +++ b/node/src/components/consensus/traits.rs @@ -51,7 +51,7 @@ pub(crate) trait Context: Clone + Debug + Eq + Ord + Hash { fn hash(data: &[u8]) -> Self::Hash; - fn validate_signature( + fn verify_signature( hash: &Self::Hash, public_key: &Self::ValidatorId, signature: &::Signature, From 505a1e0aed18270d5d4c06b5144cfbac41e0d759 Mon Sep 17 00:00:00 2001 From: Andreas Fackler Date: Tue, 14 Jul 2020 11:58:24 +0200 Subject: [PATCH 2/4] Remove some outdated TODOs. --- node/src/components/consensus/highway_core/active_validator.rs | 1 - node/src/components/consensus/highway_core/highway.rs | 1 - node/src/components/consensus/highway_core/vote.rs | 1 - 3 files changed, 3 deletions(-) diff --git a/node/src/components/consensus/highway_core/active_validator.rs b/node/src/components/consensus/highway_core/active_validator.rs index f3bada6dbc..394ad0cf5d 100644 --- a/node/src/components/consensus/highway_core/active_validator.rs +++ b/node/src/components/consensus/highway_core/active_validator.rs @@ -21,7 +21,6 @@ use crate::{ #[derive(Clone, Eq, PartialEq, Debug)] pub(crate) enum Effect { /// Newly vertex that should be gossiped to peers and added to the protocol state. - // TODO: This should contain a `ValidVertex`, since we created it ourselves. NewVertex(ValidVertex), /// `handle_timer` needs to be called at the specified time. ScheduleTimer(Timestamp), diff --git a/node/src/components/consensus/highway_core/highway.rs b/node/src/components/consensus/highway_core/highway.rs index 3f8ea788d4..77c19bdea2 100644 --- a/node/src/components/consensus/highway_core/highway.rs +++ b/node/src/components/consensus/highway_core/highway.rs @@ -197,7 +197,6 @@ impl Highway { match self.active_validator.as_mut() { None => { // TODO: Error? - // At least add logging about the event. warn!(%timestamp, "Observer node was called with `handle_timer` event."); vec![] } diff --git a/node/src/components/consensus/highway_core/vote.rs b/node/src/components/consensus/highway_core/vote.rs index debf695bf2..a5467921a4 100644 --- a/node/src/components/consensus/highway_core/vote.rs +++ b/node/src/components/consensus/highway_core/vote.rs @@ -107,7 +107,6 @@ impl Panorama { /// A vote sent to or received from the network. #[derive(Clone, Debug, Eq, PartialEq)] pub(crate) struct Vote { - // TODO: Signature /// The list of latest messages and faults observed by the creator of this message. pub(crate) panorama: Panorama, /// The number of earlier messages by the same creator. From b9aca6aebe880b5c921709406aee43d96db67d89 Mon Sep 17 00:00:00 2001 From: Andreas Fackler Date: Tue, 14 Jul 2020 12:05:21 +0200 Subject: [PATCH 3/4] Fix SynchronizerQueue. If there was no synchronizer effect in the queue, it would exit immediately. --- node/src/components/consensus/protocols/highway.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/node/src/components/consensus/protocols/highway.rs b/node/src/components/consensus/protocols/highway.rs index 13b1685ac2..0faa097566 100644 --- a/node/src/components/consensus/protocols/highway.rs +++ b/node/src/components/consensus/protocols/highway.rs @@ -180,13 +180,15 @@ where } fn run(mut self) -> Vec> { - while let Some(effect) = self.synchronizer_effects_queue.pop() { - self.process_synchronizer_effect(effect); - while let Some((sender, vertex)) = self.vertex_queue.pop() { + loop { + if let Some(effect) = self.synchronizer_effects_queue.pop() { + self.process_synchronizer_effect(effect); + } else if let Some((sender, vertex)) = self.vertex_queue.pop() { self.process_vertex(sender, vertex); + } else { + return self.results; } } - self.results } fn process_vertex(&mut self, sender: I, vertex: PreValidatedVertex) { From 1442f417597336a189a14f054edf199256b7bd37 Mon Sep 17 00:00:00 2001 From: Andreas Fackler Date: Tue, 14 Jul 2020 12:08:31 +0200 Subject: [PATCH 4/4] Only vote if there are options to vote for! `ActiveValidator` must not produce ballots if there are no blocks to vote for, otherwise the fork choice panics. --- .../consensus/highway_core/active_validator.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/node/src/components/consensus/highway_core/active_validator.rs b/node/src/components/consensus/highway_core/active_validator.rs index 394ad0cf5d..7f9c928f12 100644 --- a/node/src/components/consensus/highway_core/active_validator.rs +++ b/node/src/components/consensus/highway_core/active_validator.rs @@ -102,8 +102,10 @@ impl ActiveValidator { effects.push(Effect::RequestNewBlock(bctx)); } else if round_offset == self.witness_offset() { let panorama = state.panorama_cutoff(state.panorama(), timestamp); - let witness_vote = self.new_vote(panorama, timestamp, None, state); - effects.push(Effect::NewVertex(ValidVertex(Vertex::Vote(witness_vote)))) + if !panorama.is_empty() { + let witness_vote = self.new_vote(panorama, timestamp, None, state); + effects.push(Effect::NewVertex(ValidVertex(Vertex::Vote(witness_vote)))) + } } effects } @@ -119,9 +121,11 @@ impl ActiveValidator { warn!(%timestamp, "skipping outdated confirmation"); } else if self.should_send_confirmation(vhash, timestamp, state) { let panorama = self.confirmation_panorama(vhash, state); - let confirmation_vote = self.new_vote(panorama, timestamp, None, state); - let vv = ValidVertex(Vertex::Vote(confirmation_vote)); - return vec![Effect::NewVertex(vv)]; + if !panorama.is_empty() { + let confirmation_vote = self.new_vote(panorama, timestamp, None, state); + let vv = ValidVertex(Vertex::Vote(confirmation_vote)); + return vec![Effect::NewVertex(vv)]; + } } vec![] }