Fix #1680 #1682

asonnino · 2022-04-30T00:03:25Z

More info: #1680

gdanezis

Good stuff -- document the magic constant, and it seems v low. Second, there seems to be complex separate tasks to maintain the hashmap -- is all this necessary as separate components?

gdanezis · 2022-05-02T13:39:59Z

sui/src/sui_commands.rs

        rx_sui_to_consensus,
        rx_consensus_to_sui,
+        /* max_pending_transactions */ 10_000,


What is the implication of this? Make a const and document what it does?

adding a comment

sui_core/src/consensus_adapter.rs

gdanezis · 2022-05-02T13:49:11Z

sui_core/src/consensus_adapter.rs

    /// Receive consensus outputs.
    rx_consensus_output: Receiver<ConsensusOutput>,
+    /// The maximum number of pending replies.
+    max_pending_transactions: usize,
    /// Keep a map of all consensus inputs that are currently being sequenced.
    pending: HashMap<ConsensusTransactionDigest, Vec<Replier>>,


Should we keep the full certificate instead of just the digest? We might want to re-submit, or share at some point? Should this be persisted to disk, or are we ok if it goes away upon crash.

It will be the job of the client (and gateway) to handle resubmit in case of timeouts or crashes. It is ok if this goes away upon crash, the client will simply have to re-submit the certificate.

gdanezis · 2022-05-02T13:51:57Z

sui_core/src/consensus_adapter.rs

-            }
-        }
-    }
-
    /// Main loop receiving messages input to consensus and notifying the caller once the inputs
    /// are sequenced (of if an error happened).
    async fn run(&mut self) {


Key question: why is all this happening through message passing and separate tasks instead of the task that interacts with consensus keeping the hashmap and updating when transactions go in, and when transactions go out? Are there more than 1 threads injecting and consuming messages to/from consensus?

It is not clear to me how to best embed this with the current Sui logic. There is one task (ConsensusListener) that receives consensus outputs. The ConsensusAdapter is not a task, it simply provides helpers to interact with consensus that the AuthorityServer uses.

We have many tasks injecting transactions to consensus (the same tasks running the AuthorityServer, one per client). We however have a single task handling the output of consensus (ConsensusListener).

oxade · 2022-05-02T14:15:17Z

@asonnino
I thought we generally agreed to favor BTreeMaps over HashMaps in Sui, although it may not matter much here

asonnino · 2022-05-02T14:15:59Z

@asonnino
I thought we generally agreed to favor BTreeMaps over HashMaps in Sui, although it may not matter much here

It doesn't matter here, we don't need ordering of any kind

sui/tests/shared_objects_tests.rs

sui_core/src/authority.rs

huitseeker · 2022-05-02T14:17:42Z

sui_core/src/authority/authority_store.rs

@@ -838,9 +838,7 @@ impl<const ALL_OBJ_VER: bool, S: Eq + Serialize + for<'de> Deserialize<'de>>
                // sequence number (`OBJECT_START_VERSION`). Otherwise use the `scheduled` map to
                // to assign the next sequence number.
                let version = v.unwrap_or_else(|| OBJECT_START_VERSION);
-                let next_version = v
-                    .map(|v| v.increment())
-                    .unwrap_or_else(|| SequenceNumber::from(2));


Could you unwrap_or_else a const at OBJECT_START_VERSION + 1?

we could, but this is even simpler let next_version = version.increment();

huitseeker · 2022-05-02T14:19:33Z

sui_core/src/consensus_adapter.rs

+            Ok(reply) => reply.expect("Channel with consensus listener dropped"),
+            Err(e) => {
+                let message = ConsensusListenerMessage::Cleanup(serialized);
+                self.tx_consensus_listener
+                    .send(message)
+                    .await
+                    .expect("Channel with consensus listener dropped");


Nit: we're reusing the same message in two places. Local var?

Could we also use warn! logging under a suitable span?

It is not the same, once it is New the other is Cleanup

Sorry, I meant error message: "Channel with consensus listener dropped"

ah ok, made clearer error messages (they could be made more specific)

huitseeker · 2022-05-02T14:29:13Z

sui_core/src/consensus_adapter.rs

+                        ConsensusListenerMessage::Cleanup(transaction) => {
+                            let digest = Self::hash(&transaction);
+                            let _ = self.pending.get_mut(&digest).and_then(|x| x.pop());
+                            if self.pending.get(&digest).map_or_else(|| false, |x| x.is_empty()) {


I can't wait for inspect to stabilize.

Is this really necessary, or could we just periodically self.pending.retain(|x| !x.is_empty())?

We could but it's extra LOC

huitseeker · 2022-05-02T14:56:17Z

sui_core/src/consensus_adapter.rs

+            Ok(reply) => reply.expect("Channel with consensus listener dropped"),
+            Err(e) => {
+                let message = ConsensusListenerMessage::Cleanup(serialized);
+                self.tx_consensus_listener
+                    .send(message)
+                    .await
+                    .expect("Channel with consensus listener dropped");


Sorry, I meant error message: "Channel with consensus listener dropped"

LefKok · 2022-05-02T15:54:52Z

sui_core/src/consensus_adapter.rs

+    /// than this cap, the transactions will be handled by consensus as usual but this module won't
+    /// be keeping track of when they are sequenced. Its only purpose is to ensure the field called
+    /// `pending` has a maximum size.
+    max_pending_transactions: usize,


So if this cap is exceeded there can be clients waiting for an ack forever? Shouldn't we be rejecting instead

The channel drops so the client will knows automatically. Although you're right that this can be more explicit

let me make a note for this

Fix 1680

asonnino added 18 commits April 27, 2022 20:03

Configure consensus

5b7d3e6

Always execute shared-object certificartes

299c7cc

Rename consensus listener

cc0f6ff

Fix tests

4ee8de2

Fix tests

66c1d68

Update tests & narwhal dependency

b77dddd

fix clippy

8535202

Fix

ce9a796

Merge branch 'main' into fix-1624

297a4c5

Fix clippy

7eec483

fix bug

fac67a9

Cleanup

b967a92

Uncomment test

17064dd

Update narwhal

ebdd7cc

Merge branch 'main' into so-fix

f45f648

Add sync test

448f3a5

Fix 1680

24127d1

Simplify types

c07faa6

asonnino requested review from akichidis, bmwill, gdanezis, huitseeker and lxfind April 30, 2022 00:03

asonnino self-assigned this Apr 30, 2022

asonnino added Priority: Medium A nice to have feature or annoying bug, non-blocking and no delays expected if we punt on it Status: Needs Review PR is ready for review sui-node Type: Bug Something isn't working labels Apr 30, 2022

asonnino added this to the Pre Testnet milestone Apr 30, 2022

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

Make ConsensusListener constant memory #1680

Closed

asonnino requested a review from LefKok April 30, 2022 00:06

asonnino enabled auto-merge (squash) April 30, 2022 00:07

gdanezis approved these changes May 2, 2022

View reviewed changes

Fix comments

53eab3d

Fix comments

7f00742

huitseeker reviewed May 2, 2022

View reviewed changes

Merge branch 'main' into fix-1680

d0c4e8f

huitseeker approved these changes May 2, 2022

View reviewed changes

Fix more comments

184f5ca

asonnino merged commit f9e4b5e into main May 2, 2022

asonnino deleted the fix-1680 branch May 2, 2022 15:51

LefKok reviewed May 2, 2022

View reviewed changes

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

Fix #1680 (#1682)

875de8d

Fix 1680

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

Fix #1680 (#1682)

0edf156

Fix 1680

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #1680 #1682

Fix #1680 #1682

asonnino commented Apr 30, 2022

gdanezis left a comment

gdanezis May 2, 2022

asonnino May 2, 2022

gdanezis May 2, 2022

asonnino May 2, 2022

gdanezis May 2, 2022

asonnino May 2, 2022

oxade commented May 2, 2022

asonnino commented May 2, 2022

huitseeker May 2, 2022

asonnino May 2, 2022

huitseeker May 2, 2022

asonnino May 2, 2022

huitseeker May 2, 2022

asonnino May 2, 2022

huitseeker May 2, 2022

huitseeker May 2, 2022

asonnino May 2, 2022

huitseeker May 2, 2022

LefKok May 2, 2022

asonnino May 2, 2022

asonnino May 2, 2022

Fix #1680 #1682

Fix #1680 #1682

Conversation

asonnino commented Apr 30, 2022

gdanezis 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

oxade commented May 2, 2022

asonnino commented May 2, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment