[node] Add plumbing that allows authorities to connect to other authorities #1610

gdanezis · 2022-04-27T14:41:06Z

We add the plumbing necessary for an authority to initiate connections to other authorities. This will be used for the 'active' logic of authorities and possibly full nodes, and in particular:

To allow authorities to proactively finalise transactions and certificates seen.
Enable them to gossip transactions they have seen in a sparse network overlay.
Drive the check-pointing protocol.

lxfind · 2022-04-27T15:01:32Z

sui_core/src/authority_active.rs

+
+        Ok(ActiveAuthority {
+            authority,
+            net: AuthorityAggregator::new(committee, authority_clients),


So we don't need SafeClient?

Yes, I did wonder about this. The call the AuthorityAggregator::new wraps the network client into a SafeClient, which is a little obscure, BUT safe, since you cannot make an aggregator with an unsafe client.

lxfind · 2022-04-27T15:03:31Z

sui_core/src/authority_active.rs

+
+impl<A> ActiveAuthority<A>
+where
+    A: AuthorityAPI + Send + Sync + 'static + Clone,


Do we need to define a new trait for the communication among authorities besides AuthorityAPI?

Nope. The rule is: everything that the authority can do for itself, an external client can do for the authority too. Active parts in the authority do not exclude active parts outside the authority. Lets try to keep to this as much as we can. This include the checkpointing protocol (which I shall document before my draft pr stops being draft).

After extending the AuthorityAPI, I feel like it is getting clunky and keeping smaller, more specialized traits will make it much easier to test and extend functionality.

lxfind · 2022-04-27T15:07:36Z

sui/src/bin/validator.rs

    if let Err(e) = make_server(
        net_cfg,
        &Committee::from(&network_config),
        network_config.buffer_size,
        &consensus_committee,
        &consensus_store_path,
        &consensus_parameters,
+        Some(net),


Seems like make_server always take the net as parameter. Might as well take network_config.authorities as param and construct net in there to avoid code redundancy

I am reluctant to push all the logic in, since I am not entirely familiar with all the call points.

huitseeker · 2022-04-27T16:19:02Z

sui/src/sui_commands.rs

@@ -445,6 +463,7 @@ pub async fn make_server(
    consensus_committee: &ConsensusCommittee<Ed25519PublicKey>,
    consensus_store_path: &Path,
    consensus_parameters: &ConsensusParameters,
+    net_parameters: Option<Vec<(AuthorityName, String, u16)>>,


There's an AuthorityInfo type here that contains all you need:

sui/sui/src/config.rs

Lines 40 to 57 in 7fec599

#[derive(Serialize, Deserialize, Clone, Debug)]

pub struct AuthorityInfo {

#[serde(serialize_with = "bytes_as_hex", deserialize_with = "bytes_from_hex")]

pub name: AuthorityName,

pub host: String,

pub base_port: u16,

}

#[derive(Serialize, Debug)]

pub struct AuthorityPrivateInfo {

pub address: SuiAddress,

pub key_pair: KeyPair,

pub host: String,

pub port: u16,

pub db_path: PathBuf,

pub stake: usize,

pub consensus_address: SocketAddr,

}

You could use it with an impl From<AuthorityPrivateInfo> for AuthorityInfo and eliminate this type signature along with ~25 lines of iterator calls.

Oh I like that!

In fact there is already a lovely pub fn get_authority_infos(&self) -> Vec<AuthorityInfo> that I will use

lanvidr · 2022-04-27T17:24:31Z

sui_core/src/authority_active.rs

+
+impl<A> ActiveAuthority<A>
+where
+    A: AuthorityAPI + Send + Sync + 'static + Clone,


After extending the AuthorityAPI, I feel like it is getting clunky and keeping smaller, more specialized traits will make it much easier to test and extend functionality.

lanvidr · 2022-04-27T17:26:23Z

sui_core/src/authority_active.rs

+    A: AuthorityAPI + Send + Sync + 'static + Clone,
+{
+    // TODO: Active tasks go here + logic to spawn them all
+    pub async fn spawn_all_active_processes(self) -> Option<()> {


These will be the processes for gossip?

This should be the location that acts as the root that spawns all tasks dealing with network. So yes, if / when the authority does gossip I presume it will go there?

gdanezis · 2022-04-27T17:38:56Z

After extending the AuthorityAPI, I feel like it is getting clunky and keeping smaller, more specialized traits will make it much easier to test and extend functionality.

@laura-makdah looking forward to have you back around all this when you are done wrangling narwhals :)

huitseeker · 2022-04-27T21:06:50Z

This is probably in need of a rebase on #1626, it's about the 5th time I have it fail due to #1624

…rities (#1610) * Add plumbing to allows authorities to connect to other authorities Co-authored-by: George Danezis <george@danez.is>

gdanezis changed the title ~~[node] Add plumbing to allows authorities to connect to other authorities~~ [node] Add plumbing that allows authorities to connect to other authorities Apr 27, 2022

lxfind reviewed Apr 27, 2022

View reviewed changes

huitseeker reviewed Apr 27, 2022

View reviewed changes

lanvidr approved these changes Apr 27, 2022

View reviewed changes

George Danezis added 2 commits April 28, 2022 07:48

Add plumbing to allows authorities to connect to other authorities

391dafc

Use AuthorityInfo

98945a2

gdanezis force-pushed the authority-active branch from 24d4052 to 98945a2 Compare April 28, 2022 07:01

gdanezis merged commit 3b6cd6b into main Apr 28, 2022

gdanezis deleted the authority-active branch April 28, 2022 09:52

longbowlu pushed a commit that referenced this pull request May 12, 2022

[node] Add plumbing that allows authorities to connect to other autho…

525ff38

…rities (#1610) * Add plumbing to allows authorities to connect to other authorities Co-authored-by: George Danezis <george@danez.is>

punwai pushed a commit that referenced this pull request Jul 27, 2022

[node] Add plumbing that allows authorities to connect to other autho…

730749e

…rities (#1610) * Add plumbing to allows authorities to connect to other authorities Co-authored-by: George Danezis <george@danez.is>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[node] Add plumbing that allows authorities to connect to other authorities #1610

[node] Add plumbing that allows authorities to connect to other authorities #1610

gdanezis commented Apr 27, 2022

lxfind Apr 27, 2022

gdanezis Apr 27, 2022

lxfind Apr 27, 2022

gdanezis Apr 27, 2022 •

edited

Loading

lanvidr Apr 27, 2022

lxfind Apr 27, 2022

gdanezis Apr 27, 2022

huitseeker Apr 27, 2022

gdanezis Apr 27, 2022

gdanezis Apr 27, 2022

lanvidr Apr 27, 2022

lanvidr Apr 27, 2022

gdanezis Apr 27, 2022

gdanezis commented Apr 27, 2022

huitseeker commented Apr 27, 2022

	#[derive(Serialize, Deserialize, Clone, Debug)]
	pub struct AuthorityInfo {
	#[serde(serialize_with = "bytes_as_hex", deserialize_with = "bytes_from_hex")]
	pub name: AuthorityName,
	pub host: String,
	pub base_port: u16,
	}

	#[derive(Serialize, Debug)]
	pub struct AuthorityPrivateInfo {
	pub address: SuiAddress,
	pub key_pair: KeyPair,
	pub host: String,
	pub port: u16,
	pub db_path: PathBuf,
	pub stake: usize,
	pub consensus_address: SocketAddr,
	}

[node] Add plumbing that allows authorities to connect to other authorities #1610

[node] Add plumbing that allows authorities to connect to other authorities #1610

Conversation

gdanezis commented Apr 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdanezis Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdanezis commented Apr 27, 2022

huitseeker commented Apr 27, 2022

gdanezis Apr 27, 2022 •

edited

Loading