Skip to content

Handle multiple proposals in a view correctly in Tendermint #1793

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

Merged
merged 2 commits into from
Sep 26, 2019
Merged

Handle multiple proposals in a view correctly in Tendermint #1793

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9c0d361
Handle multiple proposals in a view correctly in Tendermint
majecty Sep 26, 2019
1d7e06e
Make the order of votes consistent
majecty Sep 26, 2019
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
20 changes: 15 additions & 5 deletions core/src/consensus/tendermint/vote_collector.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
// You should have received a copy of the GNU Affero General Public License
// along with this program. If not, see <https://www.gnu.org/licenses/>.

use std::collections::{BTreeMap, HashMap, HashSet};
use std::collections::{BTreeMap, HashMap};
use std::iter::Iterator;

use ckey::SchnorrSignature;
Expand All @@ -35,7 +35,7 @@ pub struct VoteCollector {
struct StepCollector {
voted: HashMap<usize, ConsensusMessage>,
block_votes: HashMap<Option<H256>, BTreeMap<usize, SchnorrSignature>>,
messages: HashSet<ConsensusMessage>,
messages: Vec<ConsensusMessage>,
}

#[derive(Debug)]
Expand Down Expand Up @@ -64,7 +64,8 @@ impl StepCollector {
/// Returns Some(&Address) when validator is double voting.
fn insert(&mut self, message: ConsensusMessage) -> Option<DoubleVote> {
// Do nothing when message was seen.
if self.messages.insert(message.clone()) {
if !self.messages.contains(&message) {
self.messages.push(message.clone());
if let Some(previous) = self.voted.insert(message.signer_index(), message.clone()) {
// Bad validator sent a different message.
return Some(DoubleVote {
Expand Down Expand Up @@ -207,12 +208,21 @@ impl VoteCollector {
.unwrap_or_else(Vec::new)
}

pub fn has_votes_for(&self, round: &VoteStep, block_hash: H256) -> bool {
let votes = self
.votes
.get(round)
.map(|c| c.block_votes.keys().cloned().filter_map(|x| x).collect())
.unwrap_or_else(Vec::new);
votes.into_iter().any(|vote_block_hash| vote_block_hash == block_hash)
}

pub fn get_all(&self) -> Vec<ConsensusMessage> {
self.votes.iter().flat_map(|(_round, collector)| collector.messages.iter()).cloned().collect()
self.votes.iter().flat_map(|(_round, collector)| collector.messages.clone()).collect()
}

pub fn get_all_votes_in_round(&self, round: &VoteStep) -> Vec<ConsensusMessage> {
self.votes.get(round).map(|c| c.messages.iter().cloned().collect()).unwrap_or_default()
self.votes.get(round).map(|c| c.messages.clone()).unwrap_or_default()
}

pub fn get_all_votes_and_indices_in_round(&self, round: &VoteStep) -> Vec<(usize, ConsensusMessage)> {
Expand Down
46 changes: 22 additions & 24 deletions core/src/consensus/tendermint/worker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,7 +453,7 @@ impl Worker {
self.validators.next_block_proposer(prev_block_hash, view)
}

pub fn proposal_at(&self, height: Height, view: View) -> Option<(SchnorrSignature, usize, Bytes)> {
pub fn first_proposal_at(&self, height: Height, view: View) -> Option<(SchnorrSignature, usize, Bytes)> {
let vote_step = VoteStep {
height,
view,
Expand All @@ -475,11 +475,9 @@ impl Worker {
step: Step::Propose,
});

if let Some(proposal) = all_votes.first() {
proposal.on.block_hash.expect("Proposal message always include block hash") == block_hash
} else {
false
}
all_votes
.into_iter()
.any(|proposal| proposal.on.block_hash.expect("Proposal message always include block hash") == block_hash)
}

pub fn vote_step(&self) -> VoteStep {
Expand Down Expand Up @@ -670,6 +668,7 @@ impl Worker {
match state.to_step() {
Step::Propose => {
cinfo!(ENGINE, "move_to_step: Propose.");
// If there are multiple proposals, use the first proposal.
if let Some(hash) = self.votes.get_block_hashes(&vote_step).first() {
if self.client().block(&BlockId::Hash(*hash)).is_some() {
self.proposal = Proposal::new_imported(*hash);
Expand All @@ -683,9 +682,9 @@ impl Worker {
} else {
let parent_block_hash = self.prev_block_hash();
if self.is_signer_proposer(&parent_block_hash) {
if let TwoThirdsMajority::Lock(lock_view, _) = self.last_two_thirds_majority {
if let TwoThirdsMajority::Lock(lock_view, locked_block_hash) = self.last_two_thirds_majority {
cinfo!(ENGINE, "I am a proposer, I'll re-propose a locked block");
match self.locked_proposal_block(lock_view) {
match self.locked_proposal_block(lock_view, locked_block_hash) {
Ok(block) => self.repropose_block(block),
Err(error_msg) => cwarn!(ENGINE, "{}", error_msg),
}
Expand Down Expand Up @@ -796,14 +795,14 @@ impl Worker {
}
}

fn locked_proposal_block(&self, locked_view: View) -> Result<encoded::Block, String> {
fn locked_proposal_block(&self, locked_view: View, locked_proposal_hash: H256) -> Result<encoded::Block, String> {
let vote_step = VoteStep::new(self.height, locked_view, Step::Propose);
let locked_proposal_hash = self.votes.get_block_hashes(&vote_step).first().cloned();
let received_locked_block = self.votes.has_votes_for(&vote_step, locked_proposal_hash);

let locked_proposal_hash = locked_proposal_hash.ok_or_else(|| {
if !received_locked_block {
self.request_proposal_to_any(self.height, locked_view);
format!("Have a lock on {}-{}, but do not received a locked proposal", self.height, locked_view)
})?;
return Err(format!("Have a lock on {}-{}, but do not received a locked proposal", self.height, locked_view))
}

let locked_proposal_block = self.client().block(&BlockId::Hash(locked_proposal_hash)).ok_or_else(|| {
format!(
Expand Down Expand Up @@ -970,9 +969,9 @@ impl Worker {
});

let current_height = self.height;
let vote_step = VoteStep::new(self.height, self.view, self.step.to_step());
let proposal_at_current_view = self.votes.get_block_hashes(&vote_step).first().cloned();
if proposal_at_current_view == Some(proposal.hash()) {
let current_vote_step = VoteStep::new(self.height, self.view, self.step.to_step());
let proposal_is_for_current = self.votes.has_votes_for(&current_vote_step, proposal.hash());
if proposal_is_for_current {
self.proposal = Proposal::new_imported(proposal.hash());
let current_step = self.step.clone();
match current_step {
Expand Down Expand Up @@ -1005,16 +1004,15 @@ impl Worker {
self.move_to_height(height);
let prev_block_view = TendermintSealView::new(proposal.seal()).previous_block_view().unwrap();
self.save_last_confirmed_view(prev_block_view);
let proposal_at_view_0 = self
.votes
.get_block_hashes(&VoteStep {
let proposal_is_for_view0 = self.votes.has_votes_for(
&VoteStep {
height,
view: 0,
step: Step::Propose,
})
.first()
.cloned();
if proposal_at_view_0 == Some(proposal.hash()) {
},
proposal.hash(),
);
if proposal_is_for_view0 {
self.proposal = Proposal::new_imported(proposal.hash())
}
self.move_to_step(TendermintState::Prevote, false);
Expand Down Expand Up @@ -1896,7 +1894,7 @@ impl Worker {
return
}

if let Some((signature, _signer_index, block)) = self.proposal_at(request_height, request_view) {
if let Some((signature, _signer_index, block)) = self.first_proposal_at(request_height, request_view) {
ctrace!(ENGINE, "Send proposal {}-{} to {:?}", request_height, request_view, token);
self.send_proposal_block(signature, request_view, block, result);
return
Expand Down