Consensus Adapter #1449

asonnino · 2022-04-19T16:57:20Z

What this PR does

This PR introduces the Consensus Adapter module who bridges the Sui with consensus. It works in the following way:

Client submit a certificate referencing shared objects to Sui
The consensus adapter checks the certificate and forwards it to the consensus node
The consensus node sequences the certificate, the AuthorityState assigns locks to the shared objects (not in this PR)
The consensus adapter notifies the Authority Server
The authority Server attemps to execute the certificate (if all locks are assigned correctly) and replies to the client

What this PR does not do

The top level SuiCommand module needs to spawn the consensus node (blocked until Narwhal is open sourced)
The Authority state needs to implement the trait ExecutionState and run a Narwhal client (blocked until Narhwal is open sourced)

gdanezis · 2022-04-19T17:03:23Z

Make me a favour on the checkpointing side, which is also going to use the the consensus:

Do add a description to the PR otherwise no one knows that they are reviewing.
Define an enum for the messages that are sequenced. Right now there will be one, containing a certificate, but down the line there will be info for checkpointing too.

asonnino · 2022-04-19T17:09:05Z

Make me a favour on the checkpointing side, which is also going to use the the consensus:

Do add a description to the PR otherwise no one knows that they are reviewing.

You have been too fast :) I was writing the description of the PR (but I am a slow writer)

Define an enum for the messages that are sequenced. Right now there will be one, containing a certificate, but down the line there will be info for checkpointing too.

Good idea fixing this

codecov · 2022-04-19T17:31:48Z

Codecov Report

Merging #1449 (923995b) into main (c1e46f0) will increase coverage by 0.00%.
The diff coverage is 87.09%.

❗ Current head 923995b differs from pull request most recent head 195b4af. Consider uploading reports for the commit 195b4af to get more accurate results

@@           Coverage Diff            @@
##             main    #1449    +/-   ##
========================================
  Coverage   82.19%   82.19%            
========================================
  Files         104      107     +3     
  Lines       21145    21385   +240     
========================================
+ Hits        17380    17578   +198     
- Misses       3765     3807    +42

Impacted Files	Coverage Δ
sui/src/benchmark/load_generator.rs	`0.00% <0.00%> (ø)`
sui_core/src/lib.rs	`100.00% <ø> (ø)`
sui_types/src/error.rs	`50.00% <ø> (ø)`
test_utils/src/lib.rs	`38.46% <33.33%> (-61.54%)`	⬇️
sui_core/src/authority_server.rs	`82.23% <47.36%> (-2.98%)`	⬇️
test_utils/src/network.rs	`91.66% <91.66%> (ø)`
sui_core/src/consensus_adapter.rs	`93.15% <93.15%> (ø)`
sui/src/sui_commands.rs	`77.07% <94.73%> (+1.15%)`	⬆️
sui_core/src/unit_tests/consensus_adapter_tests.rs	`98.50% <98.50%> (ø)`
sui/src/config.rs	`83.54% <100.00%> (+0.99%)`	⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4fa31b4...195b4af. Read the comment docs.

asonnino · 2022-04-19T17:44:59Z

Make me a favour on the checkpointing side, which is also going to use the the consensus:

Define an enum for the messages that are sequenced. Right now there will be one, containing a certificate, but down the line there will be info for checkpointing too.

Fixed

Fix issue #1450

todd-mystenlabs · 2022-04-20T18:19:42Z

@lxfind - I want to get this PR landed. Who is the best to review it?

lxfind · 2022-04-20T18:25:14Z

@lxfind - I want to get this PR landed. Who is the best to review it?

I recommend @bmwill to take a look given this PR is primarily about network setup.

bmwill · 2022-04-20T18:45:39Z

Do we know what we want the interface between consensus and sui to be? As in we expect this to simply be a point to point link between narwhal and sui? Maybe we can reuse some of the grpc work that's being worked on for this interface as well?

asonnino · 2022-04-20T18:48:41Z

Do we know what we want the interface between consensus and sui to be? As in we expect this to simply be a point to point link between narwhal and sui? Maybe we can reuse some of the grpc work that's being worked on for this interface as well?

Yes eventually we may use the grpc interface. The challenge is that the Narwhal interface also needs to work for Celo and Somelier (not only Sui), so I am not sure this can be done before dev net

The current interface is simple: Sui submits serialized certificates to Narwhal (who see them as bytes). Consensus sequences those certificates and assigns a lock to each shared object. Sui (or the client) then pick up the certificate and execute it (on the Sui side).

bmwill

Given our timeline we can get this in once the tests are passing (they seemed to timeout?). I think we'll want to spend a bit more time looking at our interfaces but we can do that at a future time.

asonnino · 2022-04-20T18:55:51Z

Given our timeline we can get this in once the tests are passing (they seemed to timeout?). I think we'll want to spend a bit more time looking at our interfaces but we can do that at a future time.

Tests work fine on my local machine, debugging the CI now

huitseeker

The current implementation is going to run out of fds (not to mention ports) fast.

huitseeker · 2022-04-20T17:38:08Z

sui/src/config.rs

+        let consensus_address = if let Some(val) = json.get("consensus_address") {
+            SocketAddr::deserialize(val).map_err(serde::de::Error::custom)?
+        } else {
+            let port = PORT_ALLOCATOR
+                .lock()
+                .map_err(serde::de::Error::custom)?
+                .next_port()
+                .ok_or_else(|| serde::de::Error::custom("No available port."))?;
+            format!("127.0.0.1:{port}").parse().unwrap()
+        };


Heads up in regard to #1469 : what we're really managing here is a bind address (for lack of a way to determine the public address), and we should probably use variable names that reflect this. Dropping a log line here or in make_authority would also help.

cc. @bmwill for an ubiquitous pain point of the current implementation: we bind where we (think we) are reachable, and in absence of that public address, bind to localhost 😅

huitseeker · 2022-04-20T17:46:11Z

sui_core/src/consensus_adapter.rs

+
+                Some(output) = self.rx_consensus_output.recv() => {
+                    // Notify the caller that the transaction has been sequenced.
+                    let (outcome, transaction) = output;


nit: You could bind this pattern two lines earlier.

fixed in #1467

huitseeker · 2022-04-20T17:46:58Z

sui_core/src/consensus_adapter.rs

+                    if let Some(repliers) = self.pending.get_mut(&digest) {
+                        if let Some(replier) = repliers.pop_front() {


Suggested change

if let Some(repliers) = self.pending.get_mut(&digest) {

if let Some(replier) = repliers.pop_front() {

if let Some(replier) = self.pending.get_mut(&digest).and_then(|r| r.pop_front()) {

Fixed in #1467

huitseeker · 2022-04-20T18:59:23Z

sui_core/src/consensus_adapter.rs

+        // Send certificate to consensus
+        let serialized = serialize_consensus_transaction(certificate);
+        let bytes = Bytes::from(serialized.clone());
+        // TODO [issue #1452]: We are re-creating a connection every time. This is wasteful but does not


The problem with this (#1452) is not so much that you're wasteful, it's that you're going to run against an fd limit pretty fast.
An immediate workaround is the tricks akin to 43e9190 but a better fix is just to have an ad-hoc connection pool using a library like bb8, I think.

There's a couple other ways to approach this:

Use a real RPC like framework that just handles these things for us (e.g. gRPC like we're doing for our other narwhal integrations)

store an Option<TcpStream> and if its none do the reconnection otherwise use the existing connection.

I initially went for option 2 (store an Option<TcpStream>) but this function cannot take a mutable reference to self (for reasons linked to authority state). but what francois mentions is a problem I will try to fix it in a separate PR

asonnino added 2 commits April 18, 2022 19:42

Consensus adapter module

c7693f7

Update tests

1cc2d95

asonnino changed the title ~~Consensus-adapter~~ Consensus Adapter Apr 19, 2022

asonnino self-assigned this Apr 19, 2022

asonnino added this to the DevNet milestone Apr 19, 2022

asonnino requested a review from gdanezis April 19, 2022 17:07

Fix comments

3b792f9

Fix issue #1450 (#1462)

50da13b

Fix issue #1450

asonnino linked an issue Apr 19, 2022 that may be closed by this pull request

Spawn consensus adapter #1450

Closed

asonnino added 2 commits April 19, 2022 17:40

Add unit tests

380fb0a

Remove println..

923995b

asonnino requested a review from huitseeker April 20, 2022 13:52

todd-mystenlabs mentioned this pull request Apr 20, 2022

Consensus Integration #1467

Merged

asonnino requested a review from lxfind April 20, 2022 18:22

lxfind requested a review from bmwill April 20, 2022 18:22

bmwill approved these changes Apr 20, 2022

View reviewed changes

asonnino enabled auto-merge (squash) April 20, 2022 18:53

huitseeker reviewed Apr 20, 2022

View reviewed changes

asonnino added 3 commits April 20, 2022 16:24

Fix bug test

9727e26

Merge branch 'main' into consensus-adapter

4f87adf

Fix format test

195b4af

asonnino merged commit cf53ad5 into main Apr 20, 2022

asonnino deleted the consensus-adapter branch April 20, 2022 21:08

huitseeker mentioned this pull request Oct 4, 2022

[consensus_adapter] consensus_adapter_tests is orphaned #4983

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consensus Adapter #1449

Consensus Adapter #1449

asonnino commented Apr 19, 2022 •

edited

Loading

gdanezis commented Apr 19, 2022

asonnino commented Apr 19, 2022 •

edited

Loading

codecov bot commented Apr 19, 2022 •

edited

Loading

asonnino commented Apr 19, 2022 •

edited

Loading

todd-mystenlabs commented Apr 20, 2022

lxfind commented Apr 20, 2022

bmwill commented Apr 20, 2022

asonnino commented Apr 20, 2022 •

edited

Loading

bmwill left a comment

asonnino commented Apr 20, 2022

huitseeker left a comment

huitseeker Apr 20, 2022

huitseeker Apr 20, 2022

asonnino Apr 20, 2022

huitseeker Apr 20, 2022

asonnino Apr 20, 2022

huitseeker Apr 20, 2022

bmwill Apr 20, 2022

asonnino Apr 20, 2022

		if let Some(repliers) = self.pending.get_mut(&digest) {
		if let Some(replier) = repliers.pop_front() {

	if let Some(repliers) = self.pending.get_mut(&digest) {
	if let Some(replier) = repliers.pop_front() {
	if let Some(replier) = self.pending.get_mut(&digest).and_then(\|r\| r.pop_front()) {

Consensus Adapter #1449

Consensus Adapter #1449

Conversation

asonnino commented Apr 19, 2022 • edited Loading

What this PR does

What this PR does not do

gdanezis commented Apr 19, 2022

asonnino commented Apr 19, 2022 • edited Loading

codecov bot commented Apr 19, 2022 • edited Loading

Codecov Report

asonnino commented Apr 19, 2022 • edited Loading

todd-mystenlabs commented Apr 20, 2022

lxfind commented Apr 20, 2022

bmwill commented Apr 20, 2022

asonnino commented Apr 20, 2022 • edited Loading

bmwill left a comment

Choose a reason for hiding this comment

asonnino commented Apr 20, 2022

huitseeker left a 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

asonnino commented Apr 19, 2022 •

edited

Loading

asonnino commented Apr 19, 2022 •

edited

Loading

codecov bot commented Apr 19, 2022 •

edited

Loading

asonnino commented Apr 19, 2022 •

edited

Loading

asonnino commented Apr 20, 2022 •

edited

Loading