Tendermint fixes #5415
Tendermint fixes #5415
Conversation
keorn
added
A0-pleasereview 🤓
|
any chance of a unit test? |
|
Will try, but it has to do with timing. |
| @@ -260,8 +241,17 @@ impl Tendermint { | |||
| self.validators.contains(&*self.proposal_parent.read(), address) | |||
| } | |||
|
|
|||
| fn is_above_threshold(&self, n: usize) -> bool { | |||
| n > self.validators.count(&*self.proposal_parent.read()) * 2/3 | |||
| fn is_above_threshold(&self, n: usize) -> Result<(), EngineError> { | |||
There was a problem hiding this comment.
Functions with is_ prefix usually return bool. Maybe check_ or validate_if would be a better name?
| fn is_above_threshold(&self, n: usize) -> bool { | ||
| n > self.validators.count(&*self.proposal_parent.read()) * 2/3 | ||
| fn is_above_threshold(&self, n: usize) -> Result<(), EngineError> { | ||
| let threshold = self.validators.count(&*self.proposal_parent.read()) * 2/3; |
There was a problem hiding this comment.
That won't really work for a single validator, right? If you ever reach to such state you will never be able to mint any new blocks.
There was a problem hiding this comment.
Integer arithmetic, threshold will be 0, so 1 is enough.
| if let Ok(proposal) = ConsensusMessage::new_proposal(header) { | ||
| let proposer = proposal.verify()?; | ||
| if !self.is_authority(&proposer) { | ||
| Err(EngineError::NotAuthorized(proposer))? |
There was a problem hiding this comment.
I think return Err(EngineError::NotAuthorized(proposer)) would be way clearer, but I saw that you're using this pattern in many places already.
There was a problem hiding this comment.
You have to do into as well, but can change.
| } else { | ||
| let vote_step = VoteStep::new(header.number() as usize, consensus_view(header)?, Step::Precommit); | ||
| let precommit_hash = message_hash(vote_step.clone(), header.bare_hash()); | ||
| let ref signatures_field = header.seal()[2]; |
There was a problem hiding this comment.
let signatures_field = &header.seal()[2]; would be enough. Do we ever prove that seal has at least 2 elements? explicit expect would have been better.
There was a problem hiding this comment.
In verify_block_basic, sure can do.
| let signatures_len = header.seal()[2].len(); | ||
| if signatures_len >= 1 { | ||
| if (proposal_len == 1) ^ (signatures_len == 1) { |
There was a problem hiding this comment.
I don't really get that change here.
So the block is valid either if:
- there is one proposal and the number of signatures is not one (so 0 or 2 sigs is ok)
- or there is one signature and the number of proposals is not one (so 0 or 2 proposals is ok),
Why is that correct? (Shouldn't it be either (a proposal and 0 sigs) or (a signature and 0 proposals)?)
There was a problem hiding this comment.
This is sort of wonky, since its just the length of the seal field (1 means no signatures). Will change it to comparing actual RLPs.
There was a problem hiding this comment.
!= instead of ^ reads better for bools
|
|
||
| if header.number() == 0 { | ||
| Err(BlockError::RidiculousNumber(OutOfBounds { min: Some(1), max: None, found: header.number() }))?; | ||
| self.is_above_threshold(signature_count)? |
There was a problem hiding this comment.
origins.len() would work here, signature_count is redundant.
| } else { | ||
| Some(Step::Commit) | ||
| let bh = message.block_hash.expect("previous guard ensures is_some; qed"); | ||
| if *self.last_proposed.read() == bh { |
There was a problem hiding this comment.
Are self.step and self.last_proposed always locked in the same order?
There was a problem hiding this comment.
They are never write in the same context.
| } | ||
| self.broadcast_message(rlp.as_raw().to_vec()); | ||
| if self.votes.vote(message.clone(), &sender).is_some() { | ||
| self.validators.report_malicious(&sender); | ||
| Err(EngineError::DoubleVote(sender))? | ||
| return Err(From::from(EngineError::DoubleVote(sender))); |
| max: None, | ||
| found: signatures_len | ||
| }))) | ||
| warn!(target: "engine", "verify_block_basic: Block is neither a Commit nor Proposal."); |
There was a problem hiding this comment.
this branch is also taken when the seal advertises both, right?
There was a problem hiding this comment.
Yes, but currently this is never possible.
arkpar
added
A8-looksgood 🦄
Fix proposal sync and commit race condition.
Changes the seal format