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.

Sign up for GitHub

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

Jump to bottom

Post-consensus test: Duplicate message then quorum test #333

Merged
merged 7 commits into from
Feb 7, 2024
Merged

Post-consensus test: Duplicate message then quorum test #333

merged 7 commits into from
Feb 7, 2024

Conversation

MatheusFranco99
Copy link
Contributor

  • Post-consensus - DuplicateMsg and then quorum. The test asserts that the error doesn't disallow terminating a duty after a quorum is reached.

@MatheusFranco99 MatheusFranco99 self-assigned this Jan 2, 2024
GalRogozinski
GalRogozinski approved these changes Jan 3, 2024
View reviewed changes
Copy link
Contributor

@GalRogozinski GalRogozinski left a comment

Choose a reason for hiding this comment

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

Nice!

@GalRogozinski
Copy link
Contributor

refers to #264

MatusKysel
MatusKysel approved these changes Jan 3, 2024
View reviewed changes
Copy link
Contributor

@MatusKysel MatusKysel left a comment

Choose a reason for hiding this comment

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

LGTM

moshe-blox
moshe-blox approved these changes Jan 21, 2024
View reviewed changes
Copy link
Contributor

@moshe-blox moshe-blox left a comment

Choose a reason for hiding this comment

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

LGTM

GalRogozinski
GalRogozinski requested changes Feb 5, 2024
View reviewed changes
Copy link
Contributor

@GalRogozinski GalRogozinski left a comment

Choose a reason for hiding this comment

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

Hey @MatheusFranco99

Don't want to do changes in your PR myself (besides merging and resolving conflicts).
Shouldn't the tests here be "Duplicate message with different roots then quorum"...

It is just that we have duplicate_msg.go and duplicate_msg_different_root.go and this test seems like an extension of the latter...

maybe other good tests will be to create quorum of dup messages and a test with one dup message that has the same root (not for this PR)

@MatheusFranco99
Copy link
Contributor Author

Hey @GalRogozinski
The test is indeed a duplicate message (from an already received signer) with a different signing root and then the operator receives a quorum successfully.

You mean that a more appropriate test name (and file name) should include something like "msg different root", correct?
I agree. Will wait on your response to change it :)

@GalRogozinski
Copy link
Contributor

@MatheusFranco99 yes

@GalRogozinski GalRogozinski mentioned this pull request Feb 6, 2024
Open
@MatheusFranco99
Copy link
Contributor Author

@GalRogozinski done

@GalRogozinski GalRogozinski merged commit b64e3e8 into main Feb 7, 2024
2 checks passed
@GalRogozinski GalRogozinski deleted the postconsensus-duplicate-quorum-test branch February 12, 2024 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@y0sher y0sher y0sher approved these changes

@MatusKysel MatusKysel MatusKysel approved these changes

@GalRogozinski GalRogozinski GalRogozinski approved these changes

@moshe-blox moshe-blox moshe-blox approved these changes

Assignees

@MatheusFranco99 MatheusFranco99

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@MatheusFranco99 @GalRogozinski @y0sher @MatusKysel @moshe-blox