[ZKS-02] Update the set of authorized validators #3118
Conversation
raychu86
changed the title
| @@ -32,35 +32,35 @@ use tracing::*; | |||
| #[derive(Debug)] | |||
| pub struct MockLedgerService<N: Network> { | |||
| committee: Committee<N>, | |||
| height_to_hash: Mutex<BTreeMap<u32, N::BlockHash>>, | |||
| height_to_round_and_hash: Mutex<BTreeMap<u32, (u64, N::BlockHash)>>, | |||
There was a problem hiding this comment.
it would probably be better if this was an RwLock, as I can see several read-only operations available
node/bft/src/gateway.rs
Outdated
| .ledger | ||
| .current_committee() | ||
| .map_or(false, |committee| committee.is_committee_member(validator_address)) | ||
| exists_in_previous_committee_lookback || exists_in_current_committee_lookback || exists_in_latest_committee |
There was a problem hiding this comment.
there's some potential for faster early returns here; once we know that exists_in_previous_committee_lookback is true, we can immediately return true;, same thing with exists_in_current_committee_lookback; we only need to proceed with further checks if the previous ones are false
There was a problem hiding this comment.
in addition, I'd guess that the first of the checks is the slowest and the last one is the fastest, so it would probably be a good idea to reverse their order for the purpose for potential early returns
|
Note: This PR was tested on a network where validators continuously bonded/unbonded and did not encounter block production issues. The |
Motivation
This PR updates the set of authorized validators to include current committee members along with committee members from rounds up until the sync range tolerance, which is currently set at one block before nodes are considered out-of-sync.
Previously, the code would fetch the committee lookback at the latest ledger round, but this is not always the same as the committee lookback from the storage’s current round. For example, if the latest ledger round is ‘n’ but the current round is ‘n+2’, the committees may disagree. In some cases, this may lead to validators being disconnected who should still be considered as authorized to participate in consensus.
One of the implications of the previous logic is that if these committees differ substantially and affect (2f+1), then a validator will be unable to connect to a quorum of nodes in the current committee and halt.
Audit Finding: [zksecurity 02] Dynamic Committee Feature is Not Safe