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

Add Byzantine Test where we have a dishonest leader #3516

Merged
merged 8 commits into from
Aug 2, 2024

Conversation

lukeiannucci
Copy link
Contributor

Closes #3515

This PR:

Uses byzantine trait in testing framework to create a dishonest leader that will replace proposed QC with a QC from a previous view

This PR does not:

Key places to review:

tests/tests_1/test_with_failures_2.rs I added the integration test at the bottom to test dishonest leader

@lukeiannucci lukeiannucci force-pushed the li/byzantine-test-dishonest-leader branch from d8ca319 to 319bbf6 Compare July 31, 2024 20:12
@lukeiannucci lukeiannucci marked this pull request as ready for review July 31, 2024 20:24
@lukeiannucci lukeiannucci requested a review from bfish713 August 1, 2024 13:38
Copy link
Contributor

@jparr721 jparr721 left a comment

Choose a reason for hiding this comment

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

Couple of comments

crates/hotshot/src/tasks/mod.rs Outdated Show resolved Hide resolved
crates/hotshot/src/tasks/mod.rs Outdated Show resolved Hide resolved
crates/hotshot/src/tasks/mod.rs Outdated Show resolved Hide resolved
crates/hotshot/src/tasks/mod.rs Outdated Show resolved Hide resolved
crates/testing/src/overall_safety_task.rs Outdated Show resolved Hide resolved
@lukeiannucci lukeiannucci requested a review from jparr721 August 1, 2024 20:55
@bfish713
Copy link
Collaborator

bfish713 commented Aug 2, 2024

I see that your adding the expected views to fail but I think it's missing the most important part. The check you have just says it's ok to have a failed view if it's expected. I think for byzantine tests we want the check to also make sure that we don't succeed in a view we expected to fail. Maybe improving the safety task with this can be a follow up

Copy link
Contributor

@jparr721 jparr721 left a comment

Choose a reason for hiding this comment

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

LGTM, I think Brendon has a comment so I'll let him approve

@jparr721 jparr721 dismissed their stale review August 2, 2024 15:49

Changes addressed

@lukeiannucci lukeiannucci reopened this Aug 2, 2024
@lukeiannucci lukeiannucci merged commit e6a53f6 into main Aug 2, 2024
40 checks passed
@lukeiannucci lukeiannucci deleted the li/byzantine-test-dishonest-leader branch August 2, 2024 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Byzantine testing] - Add Byzantine Test where we have a dishonest leader
3 participants