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

[Consensus2] - Decouple quorum proposal task #3151

Merged
merged 41 commits into from
May 22, 2024

Conversation

jparr721
Copy link
Contributor

@jparr721 jparr721 commented May 11, 2024

Closes #3171

This PR:

Right now the quorum proposal task relies on calling the existing consensus functions, but isolates the call site so that way it can exist in an idempotent manner. This has created some issues due to the need to keep backwards compatibility with the consensus task. Instead, we will author our own new logic and only reuse functions that have limited scope. Specifically:

  • Makes a new Quorum Proposal Task module which will hold all relevant logic for that task
  • Fully removes all references to the old consensus task unless the functionality will live on in the new tasks
  • Fully tests and evaluate all new code in the unit testing suite to ensure that all expected behaviors are understood.

This PR does not:

  • Handle upgrades.
  • Handle the situation where the parent isn't found in the propose routine.

Key places to review:

  • Please look at all instances of ValidatedStateUpdated, as this method is a gating requirement to be able to propose. Does it make sense to gate on this method? This updates validated_state_map, which we use when we propose.
  • Please look at my re-implementation of the leaf traversal
  • Please look at my unit tests, and answer the following questions to yourself. If ANY of it is unclear, let me know ASAP.
    • Is QuorumProposalValidated referencing the right proposal and leaf?
    • Is the test_view_1 test evaluating the right genesis views?
    • Do the new events make sense?
    • Are timeouts and view sync handled properly?
  • Do these dependencies make sense?
    • Please see the design document in notion for additional explanation.
  • Does blocking on the consensus high qc update make sense?
    • Please see the design document in notion for additional explanation.

Also PLEASE look at the design doc in notion! It features a deep-dive of all the changes as well as their motivation.

@jparr721 jparr721 changed the base branch from main to jp/consensus2 May 11, 2024 20:16
@jparr721 jparr721 marked this pull request as ready for review May 15, 2024 22:26
crates/task-impls/src/consensus/helpers.rs Show resolved Hide resolved
crates/task-impls/src/events.rs Show resolved Hide resolved
crates/task-impls/src/quorum_proposal/mod.rs Show resolved Hide resolved
crates/task-impls/src/quorum_proposal/mod.rs Show resolved Hide resolved
crates/task-impls/src/events.rs Outdated Show resolved Hide resolved
crates/task-impls/src/quorum_proposal/handlers.rs Outdated Show resolved Hide resolved
crates/task-impls/src/quorum_proposal/handlers.rs Outdated Show resolved Hide resolved
crates/task-impls/src/quorum_proposal/handlers.rs Outdated Show resolved Hide resolved
crates/testing/tests/tests_1/quorum_proposal_task.rs Outdated Show resolved Hide resolved
Base automatically changed from jp/consensus2 to main May 20, 2024 14:40
shenkeyao
shenkeyao previously approved these changes May 21, 2024
Copy link
Member

@shenkeyao shenkeyao left a comment

Choose a reason for hiding this comment

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

Looks good, just a comment about reordering code.

crates/task-impls/src/quorum_proposal/handlers.rs Outdated Show resolved Hide resolved
crates/task-impls/src/quorum_vote.rs Show resolved Hide resolved
Copy link
Member

@shenkeyao shenkeyao left a comment

Choose a reason for hiding this comment

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

LGTM

@jparr721 jparr721 merged commit 268207b into main May 22, 2024
24 checks passed
@jparr721 jparr721 deleted the jp/decouple-quorum_proposal_task branch May 22, 2024 14:35
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_CLEANUP] - Diverge the QuorumProposal task
4 participants