Lr/split validation #3495
Lr/split validation #3495
Conversation
jparr721
left a comment
jparr721
left a comment
There was a problem hiding this comment.
Great work, just a couple things I'd like your opinion on.
crates/task-impls/src/helpers.rs
Outdated
| ensure!( | ||
| view_leader_key == *sender, | ||
| "Leader key does not match key in proposal" | ||
| view_leader_key.validate(&proposal.signature, proposed_leaf.commit().as_ref()), |
There was a problem hiding this comment.
This validate_signature method exists now. Should we just use that?
| /// 1. The proposal is not for an old view | ||
| /// 2. The proposal has been correctly signed by the leader of the current view | ||
| /// 3. The justify QC is valid | ||
| QuorumProposalPreliminarilyValidated(Proposal<TYPES, QuorumProposal<TYPES>>), |
There was a problem hiding this comment.
Do you think it'd be helpful to elucidate how this event is different from the QuorumProposalValidated event? Unfortunately, it looks like the QuorumProposalValidated event's doc comment doesn't give much information right now, so an unfamiliar reader might not be able to trivially differentiate the two events without tracing the validation paths. We could also update the validated event as well with a more descriptive doc comment.
jparr721
left a comment
jparr721
left a comment
There was a problem hiding this comment.
Looks good, one last little thing
| /// All dependencies for the quorum vote are validated. | ||
| QuorumVoteDependenciesValidated(TYPES::Time), | ||
| /// A quorum proposal with the given parent leaf is validated. | ||
| /// The full validation checks include: |
crates/task-impls/src/helpers.rs
Outdated
| ); | ||
| // Validate the proposal's signature. This should also catch if the leaf_commitment does not equal our calculated parent commitment | ||
| // | ||
| // There is a mistake here originating in the genesis leaf/qc commit. This should be replaced by: |
There was a problem hiding this comment.
Looks like we might be able to remove this comment entirely? Looks like we can keep the above, but the "there is a mistake here" part seems to be rectified.
Closes #3494
This PR:
Adds a new
HotShotEvent::QuorumProposalPreliminarilyValidated. This event indicates that the received quorum proposal has been preliminarily validated. This means that the quorum proposali. is not for an old view,
ii. has been correctly signed by the leader of the current view,
iii. includes a valid justify QC.
The
HotShotEvent::QuorumProposalPreliminarilyValidatedis received by theQuorumProposalTaskStateinstead ofHotShotEvent::QuorumProposalRecv. It protects theQuorumProposalTaskStateagainst spoofed quorum proposals.Additionally:
i. Moves the signature check from
validate_proposal_safety_and_livenesstovalidate_proposal_view_and_certs.ii. Broadcasts
HighQcUpdatedinstead ofUpdateHighQcat the end ofhandle_quorum_proposal_recvbecause it is more accurate and saves us from a redundant shared consensus state update.This PR does not:
This PR allows dependency tasks-based consensus to progress when flooded with spoofed quorum proposals but the progression is much slower than normal.
Key places to review:
crates/task-impls/src/helpers.rs
crates/task-impls/src/quorum_proposal_recv/handlers.rs
crates/task-impls/src/quorum_proposal/mod.rs
How to test this PR:
All tests should pass.
The change has been tested with
demo-nativeand one of the nodes sending spoofed quorum proposals. The consensus progresses correctly. Only the faulty leader's views timeout.