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

[AUDIT][LOW][CX_HARDENING] - Add Bespoke Tests For VoteDependencyHandle #3517

Merged
merged 5 commits into from
Aug 2, 2024

Conversation

jparr721
Copy link
Contributor

@jparr721 jparr721 commented Jul 30, 2024

Closes #3452

This PR:

Currently our vote task tests are not invariant to all possible orderings

Quorum Vote Task has a problem during its units tests where no matter what order the events are received, QuorumProposalValidated is ALWAYS the first event processed when handle_dep_result is called within the dependency task:

Consider this list of events

Inputs
0: DaCertificateRecv(view_number=ViewNumber(2))
1: VIDShareRecv(view_number=ViewNumber(2))
2: QuorumProposalValidated(view_number=ViewNumber(2))

Re-Broadcasts
3: DaCertificateValidated(view_number=ViewNumber(2))
4: VIDShareValidated(view_number=ViewNumber(2))

Outputs
5: QuorumVoteDependenciesValidated(view_number=ViewNumber(2))
6: ValidatedStateUpdated(view_number=ViewNumber(2))
7: QuorumVoteSend(view_number=ViewNumber(2))

The events vec will always shuffle DaCertificateRecv and VIDShareRecv accordingly, but QuorumProposalValidated is ALWAYS first, even when it comes last in the inputs. This is due to how the events are processed in the unit test:

Starting here, we begin processing events. If you notice, however, we deviate our behavior slightly from the Quorum Proposal Task here. The event is optional because of these:

The proposal task has no use for validation, but voting does, which means its custom events incur a rebroadcast delay (however slight), but only for the above tasks, not QuorumProposalValidated, leading to it always being the first event received by the spawned dependency tasks. As a result, these events beget None in the above code block when creating the background task in the voting logic, but the value is ALWAYS Some and, hence, does not use Option in the same function for the proposal task.

Because of this, during the testing, the events are applied instantly, meaning that the infinitessimal delay in broadacast to create the new DaCertificate and VidShare events always results in QuorumProposalValidated getting there first, meaning that we cannot ever have a test where this does not happen first in the current logic.

This PR adds a specific test to the vote dependency task with all permutations.

This PR does not:

Key places to review:

@jparr721 jparr721 requested a review from bfish713 as a code owner July 30, 2024 18:20
@jparr721 jparr721 changed the title Jp/3452 [AUDIT][LOW][CX_HARDENING] - Add Bespoke Tests For VoteDependencyHandle Jul 30, 2024
@jparr721 jparr721 requested a review from ss-es as a code owner August 1, 2024 15:13
@jparr721 jparr721 requested a review from QuentinI August 1, 2024 19:18
@jparr721 jparr721 merged commit 4b73994 into main Aug 2, 2024
36 checks passed
@jparr721 jparr721 deleted the jp/3452 branch August 2, 2024 15:11
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.

[CX_HARDENING] - Add Bespoke Tests For VoteDependencyHandle
4 participants