Nomos node tree overlay #415

bacv · 2023-09-18T12:44:17Z

Added tree overlay to nomos-node. Some configuration parameters were missing in example config.yaml, so the file was updated.

danielSanchezQ

Some of the overlay config will have to go away eventually as it must be calculated from chain instead. Overall looks good. I just do not know we we didn't mix a couple branches in this PR.

danielSanchezQ · 2023-09-18T13:17:25Z

mixnet/client/src/config.rs

+    #[serde(default = "MixnetClientConfig::default_max_retries")]
    pub max_retries: usize,
+    #[serde(default = "MixnetClientConfig::default_retry_delay")]


Is this from another branch?

No, the another branch had defaults for MixnetNode, I've added these for MixnetClient config parsing.

danielSanchezQ · 2023-09-18T13:17:49Z

nodes/nomos-node/config.yaml

+da:
+  da_protocol:
+    num_attestations: 1
+  backend:
+    max_capacity: 10
+    evicting_period: 
+      secs: 3600
+      nanos: 0


Same, this is not from this pr or is it?

This is from this PR, had to add these changes for config to work. Mentioned it in the description:

Some configuration parameters were missing in example config.yaml, so the file was updated.

Added these changes to PR because they are small, these are separate commits:

nomos-services/consensus/src/tally/happy.rs

bacv · 2023-09-19T13:39:09Z

@zeegomo - After changing FlatOverlay to TreeOverlay in nomos node and running integration tests, ten happy path nodes network had some panicking nodes and failed to progress with views. @danielSanchezQ helped debug it and error handling for this case was added. Please review.

I've also increased the unhappy case timeout, tree overlay added more latency.

danielSanchezQ · 2023-09-19T13:40:30Z

nomos-services/consensus/src/lib.rs

+                    tracing::debug!("Failed to gather initial votes");
+                    return Event::None;


@zeegomo This should be ok right? I have been working with Gusto and in reality if failing to gather the initial votes nodes should catch up anyway.

Failing to gather initial votes means we go to the timeout routine, why does this happen now?

zeegomo · 2023-09-19T13:51:12Z

I'll review it after our meeting 👍🏼

zeegomo

What were the panics you saw in tests? I suspect they were caused by a timeout triggering the closing of the vote stream.
Those changes seems ok (and we were looking to add them), but since the tree overlay with 1 committee is exactly like a flat one, I'd like to investigate why we need the additional time

I've also increased the unhappy case timeout, tree overlay added more latency.

As in, this shouldn't be the case for a 1 committee tree overlay

zeegomo · 2023-09-19T14:55:47Z

nomos-services/consensus/src/lib.rs

+                    tracing::debug!("Failed to gather initial votes");
+                    return Event::None;


Failing to gather initial votes means we go to the timeout routine, why does this happen now?

danielSanchezQ · 2023-09-19T15:01:30Z

What were the panics you saw in tests? I suspect they were caused by a timeout triggering the closing of the vote stream. Those changes seems ok (and we were looking to add them), but since the tree overlay with 1 committee is exactly like a flat one, I'd like to investigate why we need the additional time

I've also increased the unhappy case timeout, tree overlay added more latency.

As in, this shouldn't be the case for a 1 committee tree overlay

Yes, issue need to be investigated. Apparantly after some number of nodes, some get unresponsive 🤔 , starting with the first leader.

zeegomo · 2023-09-19T15:03:18Z

I think it's worth investigating this issue now that the playground is still small, debugging something like this at greater scales is almost impossible

bacv · 2023-09-19T15:14:47Z

What were the panics you saw in tests? I suspect they were caused by a timeout triggering the closing of the vote stream.
Those changes seems ok (and we were looking to add them), but since the tree overlay with 1 committee is exactly like a flat one, I'd like to investigate why we need the additional time

The panic was caused by executing unreachable code here: https://github.com/logos-co/nomos-node/pull/415/files#diff-12bf2e7f638b9635b046010a05a3ac31b6c7abf580576c6a6b04bca1f33a394fL48-L83

It's most likely caused by the tree overlay. The increase in the time it takes to complete the test cases is probably related to the added error handling that triggers unhappy path?

I think it's worth investigating this issue now that the playground is still small, debugging something like this at greater scales is almost impossible

Agree, I'll ping you how we could organize this.

bacv · 2023-09-19T16:25:37Z

Moved code unrelated to tree overlay to #423

tests/src/nodes/nomos.rs

youngjoon-lee · 2023-09-21T09:08:26Z

I merged #425 to this branch, but still we need #426 to be merged to make the ten-nodes-happy test passed.

* report unhappy blocks in the happy path test

…only (#442)

youngjoon-lee · 2023-10-05T07:41:00Z

I merged all fixes to this PR. If CI passes, I think we can consider merging this.

Use tree overlay in nomos node

98221e3

bacv self-assigned this Sep 18, 2023

bacv added the node label Sep 18, 2023

bacv requested review from al8n and danielSanchezQ September 18, 2023 12:44

bacv added this to the Nomos testnet (playground) milestone Sep 18, 2023

danielSanchezQ approved these changes Sep 18, 2023

View reviewed changes

al8n approved these changes Sep 18, 2023

View reviewed changes

youngjoon-lee approved these changes Sep 18, 2023

View reviewed changes

bacv requested review from danielSanchezQ and zeegomo September 19, 2023 13:25

youngjoon-lee reviewed Sep 19, 2023

View reviewed changes

nomos-services/consensus/src/tally/happy.rs Show resolved Hide resolved

danielSanchezQ reviewed Sep 19, 2023

View reviewed changes

bacv force-pushed the nomos-node-tree-overlay branch from d076749 to eabdf6f Compare September 19, 2023 14:48

zeegomo reviewed Sep 19, 2023

View reviewed changes

bacv added 2 commits September 19, 2023 19:17

Use tree overlay in tests module

ddeaf24

Handle unexpected consensus vote stream end in tally

8e78539

bacv force-pushed the nomos-node-tree-overlay branch from eabdf6f to 8e78539 Compare September 19, 2023 16:18

youngjoon-lee reviewed Sep 20, 2023

View reviewed changes

tests/src/nodes/nomos.rs Show resolved Hide resolved

youngjoon-lee mentioned this pull request Sep 20, 2023

test: Spawn the next leader first #425

Merged

youngjoon-lee and others added 2 commits September 21, 2023 11:51

Spawn the next leader first (#425)

f57bfc8

Merge branch 'master' into nomos-node-tree-overlay

f4f7d62

bacv force-pushed the nomos-node-tree-overlay branch from 20082e2 to f4f7d62 Compare September 21, 2023 06:41

Merge branch 'master' into nomos-node-tree-overlay

80844f5

youngjoon-lee mentioned this pull request Sep 26, 2023

Fix unhappy test: Handle the case where TARGET_VIEW is reached by timeout_qc #438

Merged

Report unhappy blocks in happy path test (#430)

5b8fb85

* report unhappy blocks in the happy path test

youngjoon-lee mentioned this pull request Sep 26, 2023

Block proposal isn't propagated to some nodes #440

Closed

Merge branch 'master' into nomos-node-tree-overlay

602ea51

youngjoon-lee mentioned this pull request Sep 27, 2023

Modified test, so that all nodes don’t all connect to the first node only #442

Merged

youngjoon-lee added 4 commits October 5, 2023 08:55

Make threshold configurable for TreeOverlay (#426)

ecfe7e0

Modified test, so that all nodes don’t all connect to the first node …

f069890

…only (#442)

Merge branch 'master' into nomos-node-tree-overlay

a9cd499

merge fix

5c5d813

bacv merged commit 99d77f7 into master Oct 5, 2023
7 checks passed

bacv deleted the nomos-node-tree-overlay branch October 5, 2023 08:56

youngjoon-lee linked an issue Oct 20, 2023 that may be closed by this pull request

vote_stream is sometimes closed before the tally is met #329

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nomos node tree overlay #415

Nomos node tree overlay #415

bacv commented Sep 18, 2023

danielSanchezQ left a comment

danielSanchezQ Sep 18, 2023

bacv Sep 18, 2023

danielSanchezQ Sep 18, 2023

bacv Sep 18, 2023 •

edited

bacv commented Sep 19, 2023

danielSanchezQ Sep 19, 2023

zeegomo Sep 19, 2023

zeegomo commented Sep 19, 2023

zeegomo left a comment •

edited

zeegomo Sep 19, 2023

danielSanchezQ commented Sep 19, 2023

zeegomo commented Sep 19, 2023

bacv commented Sep 19, 2023

bacv commented Sep 19, 2023

youngjoon-lee commented Sep 21, 2023

youngjoon-lee commented Oct 5, 2023 •

edited

		tracing::debug!("Failed to gather initial votes");
		return Event::None;

Nomos node tree overlay #415

Nomos node tree overlay #415

Conversation

bacv commented Sep 18, 2023

danielSanchezQ left a comment

Choose a reason for hiding this comment

danielSanchezQ Sep 18, 2023

Choose a reason for hiding this comment

bacv Sep 18, 2023

Choose a reason for hiding this comment

danielSanchezQ Sep 18, 2023

Choose a reason for hiding this comment

bacv Sep 18, 2023 • edited

Choose a reason for hiding this comment

bacv commented Sep 19, 2023

danielSanchezQ Sep 19, 2023

Choose a reason for hiding this comment

zeegomo Sep 19, 2023

Choose a reason for hiding this comment

zeegomo commented Sep 19, 2023

zeegomo left a comment • edited

Choose a reason for hiding this comment

zeegomo Sep 19, 2023

Choose a reason for hiding this comment

danielSanchezQ commented Sep 19, 2023

zeegomo commented Sep 19, 2023

bacv commented Sep 19, 2023

bacv commented Sep 19, 2023

youngjoon-lee commented Sep 21, 2023

youngjoon-lee commented Oct 5, 2023 • edited

bacv Sep 18, 2023 •

edited

zeegomo left a comment •

edited

youngjoon-lee commented Oct 5, 2023 •

edited