Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make threshold configurable for TreeOverlay #426

Merged

Conversation

youngjoon-lee
Copy link
Contributor

@youngjoon-lee youngjoon-lee commented Sep 20, 2023

Based on PR #425

Like FlatOverlay, we should use 100% leader threshold for TreeOverlay as well (for testing) because we don't have the block catch-up mechanism yet.

Comment on lines +15 to +16
(Fraction::from(size) * threshold)
.ceil()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, FlatOverlay was using floor(), and TreeOverlay was using x * 2/3 + 1. I thought it's better to have a common function like this and chose to use ceil() to guarantee that the threshold is never smaller than the super majority.

Base automatically changed from nomos-node-tree-overlay-first-node to nomos-node-tree-overlay September 21, 2023 02:51
@youngjoon-lee youngjoon-lee force-pushed the nomos-node-tree-overlay-threshold branch from 83554c8 to 60b92b5 Compare September 21, 2023 02:56
Copy link
Collaborator

@danielSanchezQ danielSanchezQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks!

@youngjoon-lee
Copy link
Contributor Author

youngjoon-lee commented Sep 25, 2023

I'm not merging this PR now because CI fails, and I'm not sure if this failure is caused by this PR or not.
Test Suite (waku, ubuntu-latest)

waiting... 1 | 1 | 1 | 1 | 1 | 1 | 1 | 1 | 0
waiting... 1 | 1 | 1 | 1 | 1 | 1 | 1 | 1 | 0
waiting... 1 | 1 | 1 | 1 | 1 | 1 | 1 | 1 | 0
waiting... 1 | 1 | 1 | 1 | 1 | 1 | 1 | 1 | 0
thread 'ten_nodes_one_down' panicked at 'timed out waiting for nodes to reach view 20', tests/src/tests/unhappy.rs:25:24

This isn't even reproduced in my local.

@youngjoon-lee
Copy link
Contributor Author

I'm not merging this PR now because CI fails, and I'm not sure if this failure is caused by this PR or not. Test Suite (waku, ubuntu-latest)

waiting... 1 | 1 | 1 | 1 | 1 | 1 | 1 | 1 | 0
waiting... 1 | 1 | 1 | 1 | 1 | 1 | 1 | 1 | 0
waiting... 1 | 1 | 1 | 1 | 1 | 1 | 1 | 1 | 0
waiting... 1 | 1 | 1 | 1 | 1 | 1 | 1 | 1 | 0
thread 'ten_nodes_one_down' panicked at 'timed out waiting for nodes to reach view 20', tests/src/tests/unhappy.rs:25:24

This isn't even reproduced in my local.

I've also found that the same failure sometimes occur in the ten_nodes_happy test. I'm debugging it.

One interesting thing is that the failure is not reproduced in my local (A Macbook with 10 cores), but it's reproduced in my Ubuntu VM (with 2 cores) in my Macbook, which has the similar resources as Github Actions hosts.

@youngjoon-lee
Copy link
Contributor Author

I've also found that the same failure sometimes occur in the ten_nodes_happy test. I'm debugging it.

One interesting thing is that the failure is not reproduced in my local (A Macbook with 10 cores), but it's reproduced in my Ubuntu VM (with 2 cores) in my Macbook, which has the similar resources as Github Actions hosts.

I'm debugging this at #440. From my understanding so far, this failure is not highly related to this PR. But, I'm not sure yet.

@youngjoon-lee youngjoon-lee merged commit ecfe7e0 into nomos-node-tree-overlay Oct 4, 2023
6 of 11 checks passed
@youngjoon-lee youngjoon-lee deleted the nomos-node-tree-overlay-threshold branch October 5, 2023 00:52
bacv added a commit that referenced this pull request Oct 5, 2023
* Use tree overlay in nomos node

* Use tree overlay in tests module

* Handle unexpected consensus vote stream end in tally

* Spawn the next leader first (#425)

* Report unhappy blocks in happy path test (#430)

* report unhappy blocks in the happy path test

* Make threshold configurable for TreeOverlay (#426)

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

* merge fix

---------

Co-authored-by: Youngjoon Lee <[email protected]>
Co-authored-by: Al Liu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants