[Core] Refactor of authority server #608

asonnino · 2022-03-01T14:47:22Z

What is done

Refactor of the authority server to have a main reactor loop. This removes the current (rigid) structure of the server and allows to feed the core with messages in a more flexible way. This is for instance necessary to feed consensus outputs to the core and support shared objects: see [fastx] How to best integrate information from a consensus core to allow more general fastx smart contracts #195
Get rid of the current network and use instead the same network as Narwhal. The Narwhal's network will eventually be updated, but it will be a lot easier to use it in Sui as well.
Simplify the core message structure
Update the client to also use the Narwhal network. I am first waiting for the following PR to land to minimise merge conflicts: Refactor ClientState API #584

What will be done in a next PR

Separate unit tests into appropriate files. We currently have gigantic unit tests files (1,700 LOC).
User Serde and bincode instead of the current custom serialisation format. If I remember correctly, the current serialisation format does not provide any speed benefit with respect to Serde (Serde was not that big at the time).
Isolate the error messages of the Core to not have a gigantic errors enum (suggested by @huitseeker).
Add a mock (single-process) sequencer. We will eventually replace it with a proper consensus protocol.
Write better unit tests for the core (I think some corner cases are missing).
Rename 'mutable-objects' -> 'owned-objects' everywhere (inspired from @lxfind comments)

Questions

Can I get rid of the gigantic formatter test (sui.yaml)?
Can I make a separate crate for the authority and the client? They are currently both in sui_core but do not have a lot of code in common.
Can I add the network information of the authorities into the Committee structure (that currently only holds their voting rights and cryptographic keys)? We separated them at the time because we wanted to have a 'simulator' of FastPay running on a fake/mock network.

Fetch

velvia · 2022-03-01T22:50:09Z

@asonnino Hey Alberto! I don't have much context on this change, though moving from a custom stack to Tokio for handling server commands sounds great. Just wondering what is the rough timeline for this change, as it moves authority files around and I'm hoping to come out with a PR this week which has to do with logging where I'll be touching most of the files in the authority core. Thanks!

asonnino · 2022-03-02T00:31:31Z

I’m full time on this PR and will try to land as soon as possible @ade is also helping out

oxade · 2022-03-02T01:27:23Z

@asonnino looks like a lot of this code is verbatim from Narwhal https://github.com/MystenLabs/narwhal/tree/main/network/src

Doesnt it make more sense to put the networking stuff in mysten-infra where both Narwhal and Sui can depend on it?

oxade · 2022-03-02T03:43:22Z

sui/src/sui_commands.rs

+        config.config_path()
+    );
+
+    // Spawning the authorities is only required to create the database folder.


One of the problems here is that genesis is not supposed to start the authorities.
It should simply config/provision them. So spawning the server here causes various issues because they're again started in the SuiCommand::Start. This causes DB lock issues.

Basically this is the reason for test failures.
Authorities are started here https://github.com/MystenLabs/fastnft/blob/bf9fcdc254d4ac271c20a85f3c72385cdd429a35/sui/src/unit_tests/cli_tests.rs#L539
And then again here in
https://github.com/MystenLabs/fastnft/blob/bf9fcdc254d4ac271c20a85f3c72385cdd429a35/sui/src/unit_tests/cli_tests.rs#L548
Second time around causes issues.

Why did you change to code to start authorities in Genesis?

One of the problems here is that genesis is not supposed to start the authorities.
It should simply config/provision them. So spawning the server here causes various issues because they're again started in the SuiCommand::Start. This causes DB lock issues.

Ha well that was a silly issue after all :)

I didn't change the code of the authorities to start in genesis, it was already there. The original code however took down the authorities immediately after spawning them (they were only there to generate the storage file).

asonnino · 2022-03-02T11:13:55Z

@asonnino looks like a lot of this code is verbatim from Narwhal https://github.com/MystenLabs/narwhal/tree/main/network/src

Doesnt it make more sense to put the networking stuff in mysten-infra where both Narwhal and Sui can depend on it?

Yes this is the idea

Fetch

gdanezis · 2022-03-02T15:35:02Z

@asonnino slow down a bit on this line of work: the concurrency story for Sui is very different from the concurrency story for narwhal/Tusk, because we do execution as well, so we are not bounded (just) on IO.

Right now we implement this though shared memory primitives: there is a Task per TCP connection, and they all share an AuthorityState, but execute on all cores. The architecture you are implementing moves all work into an inner loop for the authority, that processes all messages, which I think runs on a single core. This is a bad idea for Sui right now (it is also a bad idea for narwhal, but it is what it is).

gdanezis · 2022-03-02T15:36:02Z

Also there is a lot of copy paste code from Narwhal/Tusk here, which we will have to maintain twice. Better to separate into a crate we maintain once, like we did for DBMap.

asonnino · 2022-03-02T15:45:44Z

@asonnino slow down a bit on this line of work: the concurrency story for Sui is very different from the concurrency story for narwhal/Tusk, because we do execution as well, so we are not bounded (just) on IO.

Right now we implement this though shared memory primitives: there is a Task per TCP connection, and they all share an AuthorityState, but execute on all cores. The architecture you are implementing moves all work into an inner loop for the authority, that processes all messages, which I think runs on a single core. This is a bad idea for Sui right now (it is also a bad idea for narwhal, but it is what it is).

The idea was to re-use the same network as narwhal (to be moved into Mysten-infra) and get rid of the custom network we currently have. There is still one task per TCP connection (in the network stack) and then we execute on a single core; the idea was to use all the cores with sharding (eg. run 16 shards per machine).

asonnino · 2022-03-02T15:46:01Z

Also there is a lot of copy paste code from Narwhal/Tusk here, which we will have to maintain twice. Better to separate into a crate we maintain once, like we did for DBMap.

Yes I wanted to move it into Mysten-infra

asonnino · 2022-03-02T15:47:13Z

Also with the current design it is not clear how to receive inputs from consensus (for shared objects)

gdanezis · 2022-03-02T15:50:44Z

then we execute on a single core; the idea was to use all the cores with sharding (eg. run 16 shards per machine).

Yeah, that is not what we want. We want to execute on all cores, with minimal locking between them, which is right now managed just by the authority store locks table. Eventually we will even execute on separate machines, so an architecture that centralises and executes all messages moves us away from our goal.

asonnino · 2022-03-02T15:52:44Z

I see, should I then drop this PR?

gdanezis · 2022-03-02T15:52:47Z

Also with the current design it is not clear how to receive inputs from consensus (for shared objects)

What do you need there? Lets discuss minimal changes, we cannot change 1000 lines to find a way to do this.

asonnino · 2022-03-02T15:56:11Z

For the shared object, we need a way to receive inputs from the consensus. The consensus may feed us its output either through a channel or TCP.

gdanezis

See comments on the specific places where we are regressing in terms of the architecture we want.

gdanezis · 2022-03-02T15:54:38Z

sui/src/bench.rs

@@ -1,376 +0,0 @@
-// Copyright (c) 2021, Facebook, Inc. and its affiliates


This is currently the only working bench we have. We cannot just drop it.

gdanezis · 2022-03-02T15:56:48Z

sui_core/src/authority/authority_server.rs

+    /// as the network or the consensus) and drives the core.
+    async fn run(&mut self) {
+        loop {
+            tokio::select! {


This runs on one big single core loop -- so we are regressing in terms of our architecture that currently uses all cores through shared mem, and tomorrow will use many machines that share a DB.

gdanezis · 2022-03-02T15:58:27Z

sui_core/src/authority/authority_server.rs

+    /// Create an `AuthorityServer` and spawn it in a new tokio task.
+    pub fn spawn(
+        state: AuthorityState,
+        rx_client_core_message: Receiver<(ClientToAuthorityCoreMessage, Replier)>,


I am not any more a great fan of losing the connection relation: we want to fairly share time we spend between connections / IPs, and therefore we should probably keep that information and have tasks that do all the work and map to these, so that tokio can fairly schedule. Or we schedule. But this moves us to an architecture where we lose this information.

gdanezis · 2022-03-02T16:01:06Z

For the shared object, we need a way to receive inputs from the consensus. The consensus may feed us its output either through a channel or TCP.

Sounds great. Lets have a task that manages incoming consensus messages, and has a copy (of the Arc) of the AuthorityState to update the locks tables there that result from the consensus? It can also call handle_certificate if you like on the AuthorityState. Does that resolve the immediate issue?

The key to all this is: you can have concurrent access to the AuthorityState and indirectly the authority store. And we are engineering these to make it safe.

asonnino · 2022-03-02T16:03:05Z

yes we can try to do that, should I then close this PR and start a new one? And then think of the network on another day with a separate PR

gdanezis · 2022-03-02T16:10:10Z

Yep, priority is on what you suggest: to make inputs from consensus work to support shared objects.

Some aspects of this PR would be lovely as separate smaller PRs (to not kill others who have to rebase to them):

Eliminating silly things like double wrapping our certificates for no reason.
Separate errors that go over the network, vs the ones that stay in client / authority.
Refactor tests away from mega files, if that does not increase the line count.
mock sequencer
rename mutable to owned

What I think needs more discussion:

Moving networking to a crate and seeing what may be re-used.
The interaction between networking and then the concurrency story of the core.
Bincode/serde, there are reasons at places we use what we use, so need to understand them.
separate client / authority: yes, but need to ensure you break this in ways that does not make other people's PRs hell.

velvia · 2022-03-03T18:28:47Z

I see this got closed, and there was a discussion on sharding. When we get to scaling our storage, we'll want to have a discussion on sharding and what it means, but of course that is a separate discussion from sharding an authority.

* crypto: expose recover pubkey feature for Secp256k1Signature * better error handling and test

…abs#608) * crypto: expose recover pubkey feature for Secp256k1Signature * better error handling and test

Alberto Sonnino and others added 6 commits February 24, 2022 11:50

Minor spell check

2bbb1d2

Merge pull request #1 from MystenLabs/main

8fdc5b3

Fetch

Merge pull request #2 from MystenLabs/main

c77b64c

Fetch

New authority server

03c57f7

Add Narwhal network

2ddc9cf

Fix tests

6ee5ba6

asonnino marked this pull request as draft March 1, 2022 14:47

asonnino self-assigned this Mar 1, 2022

asonnino added the Refactor label Mar 1, 2022

asonnino added this to the Post-GDC milestone Mar 1, 2022

Fix sui commands

bf9fcdc

oxade reviewed Mar 2, 2022

View reviewed changes

asonnino and others added 3 commits March 2, 2022 12:24

Merge pull request #3 from MystenLabs/main

60be17f

Fetch

Merge branch 'main' into new-network

cd29bc0

Minimal client net support

5dda2ea

gdanezis reviewed Mar 2, 2022

View reviewed changes

asonnino closed this Mar 2, 2022

mwtian pushed a commit that referenced this pull request Sep 12, 2022

crypto: expose recover pubkey feature for Secp256k1Signature (#608)

746ffe4

* crypto: expose recover pubkey feature for Secp256k1Signature * better error handling and test

mwtian pushed a commit to mwtian/sui that referenced this pull request Sep 29, 2022

crypto: expose recover pubkey feature for Secp256k1Signature (MystenL…

6806c65

…abs#608) * crypto: expose recover pubkey feature for Secp256k1Signature * better error handling and test

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core] Refactor of authority server #608

[Core] Refactor of authority server #608

asonnino commented Mar 1, 2022 •

edited

Loading

velvia commented Mar 1, 2022

asonnino commented Mar 2, 2022 via email •

edited

Loading

oxade commented Mar 2, 2022

oxade Mar 2, 2022

oxade Mar 2, 2022

asonnino Mar 2, 2022

asonnino Mar 2, 2022

asonnino commented Mar 2, 2022

gdanezis commented Mar 2, 2022

gdanezis commented Mar 2, 2022

asonnino commented Mar 2, 2022

asonnino commented Mar 2, 2022

asonnino commented Mar 2, 2022

gdanezis commented Mar 2, 2022 •

edited

Loading

asonnino commented Mar 2, 2022

gdanezis commented Mar 2, 2022

asonnino commented Mar 2, 2022

gdanezis left a comment

gdanezis Mar 2, 2022

gdanezis Mar 2, 2022

gdanezis Mar 2, 2022

gdanezis commented Mar 2, 2022 •

edited

Loading

asonnino commented Mar 2, 2022 •

edited

Loading

gdanezis commented Mar 2, 2022

velvia commented Mar 3, 2022

		@@ -1,376 +0,0 @@
		// Copyright (c) 2021, Facebook, Inc. and its affiliates

[Core] Refactor of authority server #608

[Core] Refactor of authority server #608

Conversation

asonnino commented Mar 1, 2022 • edited Loading

What is done

What will be done in a next PR

Questions

velvia commented Mar 1, 2022

asonnino commented Mar 2, 2022 via email • edited Loading

oxade commented Mar 2, 2022

oxade Mar 2, 2022

Choose a reason for hiding this comment

oxade Mar 2, 2022

Choose a reason for hiding this comment

asonnino Mar 2, 2022

Choose a reason for hiding this comment

asonnino Mar 2, 2022

Choose a reason for hiding this comment

asonnino commented Mar 2, 2022

gdanezis commented Mar 2, 2022

gdanezis commented Mar 2, 2022

asonnino commented Mar 2, 2022

asonnino commented Mar 2, 2022

asonnino commented Mar 2, 2022

gdanezis commented Mar 2, 2022 • edited Loading

asonnino commented Mar 2, 2022

gdanezis commented Mar 2, 2022

asonnino commented Mar 2, 2022

gdanezis left a comment

Choose a reason for hiding this comment

gdanezis Mar 2, 2022

Choose a reason for hiding this comment

gdanezis Mar 2, 2022

Choose a reason for hiding this comment

gdanezis Mar 2, 2022

Choose a reason for hiding this comment

gdanezis commented Mar 2, 2022 • edited Loading

asonnino commented Mar 2, 2022 • edited Loading

gdanezis commented Mar 2, 2022

velvia commented Mar 3, 2022

asonnino commented Mar 1, 2022 •

edited

Loading

asonnino commented Mar 2, 2022 via email •

edited

Loading

gdanezis commented Mar 2, 2022 •

edited

Loading

gdanezis commented Mar 2, 2022 •

edited

Loading

asonnino commented Mar 2, 2022 •

edited

Loading