feat(sequencer): remove byzantine validators in begin_block #429
Conversation
SuperFluffy
left a comment
There was a problem hiding this comment.
Looks good. So this just makes use of cometbft telling us which validators are Byzantine?
Also, is there a chance of this removing all validators? If so, what would be the implications?
| _begin_block: &BeginBlock, | ||
| ) { | ||
| #[instrument(name = "AuthorityComponent::begin_block", skip(state))] | ||
| async fn begin_block<S: StateWriteExt + 'static>(state: &mut Arc<S>, begin_block: &BeginBlock) { |
There was a problem hiding this comment.
Should probably return an error and let tower-abci take down the app gracefully
| 0 => self.0.remove(&pub_key), | ||
| _ => self.0.insert(pub_key, update), | ||
| 0 => self.0.remove(&address), | ||
| _ => self.0.insert(address, update), |
There was a problem hiding this comment.
Does it make sense to use the remove and push_update methods you defined above?
There was a problem hiding this comment.
I think this is okay since push_update recalculates the address which isn't needed
|
@SuperFluffy I suppose if all validators were byzantine then they could all be removed, which seems like a bigger problem :p I guess if there are no validators, the chain will stop. I'm not sure if it will panic or just stall, if it stalls we can add new validators with the sudo key. |
Summary
remove byzantine validators in
begin_blockBackground
needed to decide how to handle byzantine validators; for now, since we only have a PoA implementation, we can just remove them
Changes
ValidatorSetHashMap key to beaccount::IdAuthorityComponent::begin_blockto remove byzantine validators from the validator setTesting
unit tests
Related Issues
closes #360