Skip to content
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.

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

[Consensus 2.0] fix mysticeti committee member ordering #16679

Merged
merged 4 commits into from
Mar 18, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
7 changes: 6 additions & 1 deletion consensus/config/src/committee.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,18 @@ pub struct Committee {
}

impl Committee {
pub fn new(epoch: Epoch, authorities: Vec<Authority>) -> Self {
pub fn new(epoch: Epoch, mut authorities: Vec<Authority>) -> Self {
assert!(!authorities.is_empty(), "Committee cannot be empty!");
assert!(
authorities.len() < u32::MAX as usize,
"Too many authorities ({})!",
authorities.len()
);

// sort the authorities by their protocol (public) key in ascending order - critical to be
// aligned with the SUI committee.
authorities.sort_by(|a1, a2| a1.protocol_key.cmp(&a2.protocol_key));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order should be provided by the caller or SUI in this case right? That would make sure we are aligned if something changes in the future on the Sui side?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I was very in between this approach and the one that I went with , but it seems it makes more sense. I'll refactor. Thanks @arun-koshy


let total_stake = authorities.iter().map(|a| a.stake).sum();
assert_ne!(total_stake, 0, "Total stake cannot be zero!");
let quorum_threshold = 2 * total_stake / 3 + 1;
Expand Down
42 changes: 21 additions & 21 deletions consensus/config/tests/snapshots/committee_test__committee.snap
Original file line number Diff line number Diff line change
Expand Up @@ -7,36 +7,21 @@ total_stake: 55
quorum_threshold: 37
validity_threshold: 19
authorities:
- stake: 1
address: ""
hostname: test_host
network_key: lscBzpFcgepvETNDWHp61B+teyauHrk6kb5XBszDufg=
protocol_key: tembhBjGw+XVGXywAYDja1R67d8pSKGhYypbgeRUJmTTdzfNnr/LHJ3U0bZY49ChGXw6mKwBVJV4KP3na8YFaef9wQuHKgwjvhJT3FnimCPyBwIz4cFasq1d3As4/yeP
- stake: 2
address: ""
hostname: test_host
network_key: VnXFtZxaTby9AquydHFr9dy8+GlVoPcsm8icljGTbWQ=
protocol_key: hLIJiig6cqajwuIlnxjL+bxhZxCgEWUGdN9VyJSqMAvuN6tDm7qTcBplvDICmgSlGJwHZ5IzyOH6N+Wj1Fa1odxsM86eLf9g+MjzT/h869sqV9EpyXtpYbufPYCW0fkT
- stake: 3
address: ""
hostname: test_host
network_key: +yTI/ZSBZa9CmHP/Qhjtf2bgTR0lC+9NUP8BgQq0OPw=
protocol_key: kci5lWPW4LlAr1jLSyEkGtYeSfstp3daJ0pChi5zo4KaDOOQd7IbnRPyP/zMqkfTE9MIyymjSkhnI72/Wogbqedrc5lhIp+C2wtbv79mdC8SrbHb2Y/LHvYD7w1mOJFM
- stake: 4
- stake: 9
address: ""
hostname: test_host
network_key: TLb93AG1NlYtf9dVMnS6LJE5aRfqt2LsSBJeuoa3wHk=
protocol_key: onGDFBk46p2hpQFq3QJbyAVD0G05FOxRaJQH3LacRTRRfdXWLtewyUi1Z0MMX4H3CpoldjqoGeMbESvqgTZeKt6HE3d8pn6TApMzf632WI3DfUGUUMBM7ciCivNaKTBS
network_key: YxoEz+3fDnLfrOkpqVkRxfj/HqlqsoQqt9bzSLjxJEc=
protocol_key: iIEIwu3rHEXU1fy+2VNUKjqV2tFLcu35a5Uo+qO99rQ63nywUvOsiDxrUvzOpJYoDZ/Xa4PnzgkhFTiGVi/Qt6ASIBhu/66jjxhDs6Pt7h1CtRFnD3zWRkgYinIzmxuQ
- stake: 5
address: ""
hostname: test_host
network_key: 7DYZ4De7EKRhhH0GBMTKlBqUeXnFMgkzLKjekMAuesw=
protocol_key: iM3E2wCApVtOKv1I/jSpNXO1kPRCnHo0c6H/qgyxvJngmgW1KYnWtycts8drNLIUGLtinT83V8D3Xnc3whUAqiKYrJ5vu2knQXqFBCmgrqo7N0xZ0T/uuKVWjoNqyHHl
- stake: 6
address: ""
hostname: test_host
network_key: B2I7VErvTa/POXtLemf+iDEt/966mNqq3F+DdHcvi8U=
protocol_key: pEiGWt/9mL2YXfHkYntXWy7OzGdS1psbJQ2D5Xp5FS7XZWYQzh7+zz/bkOfgWCRaFoV8bbsmb7eVwuMvOTnPvCSamtyUQ3pc+SKVTsXtRbwRNvWtXjTVHGQQSXE5R1kp
- stake: 7
address: ""
hostname: test_host
Expand All @@ -47,14 +32,29 @@ authorities:
hostname: test_host
network_key: +0ebVcUY7X8QIafWGbUmzlhy/Vc2LtNRqqLzk4NgLoU=
protocol_key: jDvXpT0oTY7w9BWERszWiLBu9Aajj9WbbPkfmE9M+ijxGCooLK14P1lsqOZ67XMoAIfh8Z5t/hNanV5fH+3PtJT6o8APO5pCoTcLi5SoScRSQKMCevHqHAizI57wG4cl
- stake: 9
- stake: 3
address: ""
hostname: test_host
network_key: YxoEz+3fDnLfrOkpqVkRxfj/HqlqsoQqt9bzSLjxJEc=
protocol_key: iIEIwu3rHEXU1fy+2VNUKjqV2tFLcu35a5Uo+qO99rQ63nywUvOsiDxrUvzOpJYoDZ/Xa4PnzgkhFTiGVi/Qt6ASIBhu/66jjxhDs6Pt7h1CtRFnD3zWRkgYinIzmxuQ
network_key: +yTI/ZSBZa9CmHP/Qhjtf2bgTR0lC+9NUP8BgQq0OPw=
protocol_key: kci5lWPW4LlAr1jLSyEkGtYeSfstp3daJ0pChi5zo4KaDOOQd7IbnRPyP/zMqkfTE9MIyymjSkhnI72/Wogbqedrc5lhIp+C2wtbv79mdC8SrbHb2Y/LHvYD7w1mOJFM
- stake: 10
address: ""
hostname: test_host
network_key: lT6jnl+ZiC2MhDfooUetAJFDd6cRQy1Lq9KKjKqOevo=
protocol_key: lxvqS7Rm/l8gmFj6Q3CJ3Qv994gzsM/hSt5YoBrE8v4UmfoyuvQ0JQ9gw2qNbMuiDwvkCAxotLiCa0yjKjZe5Kk8QS37dGZw5akNHQ2SHkc+CK7bccWVkdfRnF4iQal5
- stake: 4
address: ""
hostname: test_host
network_key: TLb93AG1NlYtf9dVMnS6LJE5aRfqt2LsSBJeuoa3wHk=
protocol_key: onGDFBk46p2hpQFq3QJbyAVD0G05FOxRaJQH3LacRTRRfdXWLtewyUi1Z0MMX4H3CpoldjqoGeMbESvqgTZeKt6HE3d8pn6TApMzf632WI3DfUGUUMBM7ciCivNaKTBS
- stake: 6
address: ""
hostname: test_host
network_key: B2I7VErvTa/POXtLemf+iDEt/966mNqq3F+DdHcvi8U=
protocol_key: pEiGWt/9mL2YXfHkYntXWy7OzGdS1psbJQ2D5Xp5FS7XZWYQzh7+zz/bkOfgWCRaFoV8bbsmb7eVwuMvOTnPvCSamtyUQ3pc+SKVTsXtRbwRNvWtXjTVHGQQSXE5R1kp
- stake: 1
address: ""
hostname: test_host
network_key: lscBzpFcgepvETNDWHp61B+teyauHrk6kb5XBszDufg=
protocol_key: tembhBjGw+XVGXywAYDja1R67d8pSKGhYypbgeRUJmTTdzfNnr/LHJ3U0bZY49ChGXw6mKwBVJV4KP3na8YFaef9wQuHKgwjvhJT3FnimCPyBwIz4cFasq1d3As4/yeP

4 changes: 2 additions & 2 deletions crates/sui-core/src/consensus_manager/mysticeti_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,10 +110,10 @@ impl ConsensusManagerTrait for MysticetiManager {

let name: AuthorityName = self.keypair.public().into();

let sui_committee = system_state.get_sui_committee();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you explain why you got the committee from the system state here instead of epoch store? What is the difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There shouldn't be a difference (in theory & in practice) - nothing wrong reading from epoch store - but since we are getting the mysticeti committee via the system state then I've considered more straightforward to do the same for the SUI committee as well so we don't read relevant information from two different sources.

let authority_index: AuthorityIndex = committee
.to_authority_index(
epoch_store
.committee()
sui_committee
.authority_index(&name)
.expect("Should have valid index for own authority") as usize,
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -298,3 +298,87 @@ impl EpochStartValidatorInfoV1 {
(&self.protocol_pubkey).into()
}
}

#[cfg(test)]
mod test {
use crate::base_types::SuiAddress;
use crate::committee::CommitteeTrait;
use crate::crypto::{get_key_pair, AuthorityKeyPair};
use crate::sui_system_state::epoch_start_sui_system_state::{
EpochStartSystemStateTrait, EpochStartSystemStateV1, EpochStartValidatorInfoV1,
};
use fastcrypto::traits::KeyPair;
use mysten_network::Multiaddr;
use narwhal_crypto::NetworkKeyPair;
use rand::thread_rng;
use sui_protocol_config::ProtocolVersion;

#[test]
fn test_sui_and_mysticeti_committee_are_same() {
// GIVEN
let mut active_validators = vec![];

for i in 0..10 {
let (sui_address, protocol_key): (SuiAddress, AuthorityKeyPair) = get_key_pair();
let narwhal_network_key = NetworkKeyPair::generate(&mut thread_rng());

active_validators.push(EpochStartValidatorInfoV1 {
sui_address,
protocol_pubkey: protocol_key.public().clone(),
narwhal_network_pubkey: narwhal_network_key.public().clone(),
narwhal_worker_pubkey: narwhal_network_key.public().clone(),
sui_net_address: Multiaddr::empty(),
p2p_address: Multiaddr::empty(),
narwhal_primary_address: Multiaddr::empty(),
narwhal_worker_address: Multiaddr::empty(),
voting_power: 1_000,
hostname: format!("host-{i}").to_string(),
})
}

let state = EpochStartSystemStateV1 {
epoch: 10,
protocol_version: ProtocolVersion::MAX.as_u64(),
reference_gas_price: 0,
safe_mode: false,
epoch_start_timestamp_ms: 0,
epoch_duration_ms: 0,
active_validators,
};

// WHEN
let sui_committee = state.get_sui_committee();
let mysticeti_committee = state.get_mysticeti_committee();

// THEN
// assert the validators details
assert_eq!(sui_committee.num_members(), 4);
assert_eq!(sui_committee.num_members(), mysticeti_committee.size());
assert_eq!(
sui_committee.validity_threshold(),
mysticeti_committee.validity_threshold()
);
assert_eq!(
sui_committee.quorum_threshold(),
mysticeti_committee.quorum_threshold()
);
assert_eq!(state.epoch, mysticeti_committee.epoch());

for (authority_index, mysticeti_authority) in mysticeti_committee.authorities() {
let sui_authority_name = sui_committee
.authority_by_index(authority_index.value() as u32)
.unwrap();

assert_eq!(
mysticeti_authority.protocol_key.pubkey.to_bytes(),
sui_authority_name.0,
"Mysten & SUI committee member of same index correspond to different public key"
);
assert_eq!(
mysticeti_authority.stake,
sui_committee.weight(sui_authority_name),
"Mysten & SUI committee member stake differs"
);
}
}
}
Loading