[CX_CLEANUP] - Decompose QuorumProposalRecv into module functions

[jparr721]

What is this task and why do we need to work on it?

QuorumProposalRecv is one of two macro handles that we use in the consensus task. This handler is exceptionally large and, as a result, the change surface for refactoring the method will be an encumbrance, the file's large size creates painfully slow diagnostics, and editing the handler can be nontrivial as variables defined at the beginning are used throughout. As a result, we need to pare this method down.

This issue encapsulates the decomposition of this work via equivalent methods which are defined over a subset of behavior. Consider the first block of code in the handler:

HotShotEvent::QuorumProposalRecv(proposal, sender) => {
  let sender = sender.clone();
  debug!(
      "Received Quorum Proposal for view {}",
      *proposal.data.view_number
  );

  // stop polling for the received proposal
  self.quorum_network
      .inject_consensus_info(ConsensusIntentEvent::CancelPollForProposal(
          *proposal.data.view_number,
      ))
      .await;

  let view = proposal.data.get_view_number();
  if view < self.cur_view {
      debug!("Proposal is from an older view {:?}", proposal.data.clone());
      return;
  }

  let view_leader_key = self.quorum_membership.get_leader(view);
  if view_leader_key != sender {
      warn!("Leader key does not match key in proposal");
      return;
  }

  // Verify a timeout certificate OR a view sync certificate exists and is valid.
  if proposal.data.justify_qc.get_view_number() != view - 1 {
      if let Some(received_proposal_cert) = proposal.data.proposal_certificate.clone()
      {
          match received_proposal_cert {
              ViewChangeEvidence::Timeout(timeout_cert) => {
                  if timeout_cert.get_data().view != view - 1 {
                      warn!("Timeout certificate for view {} was not for the immediately preceding view", *view);
                      return;
                  }

                  if !timeout_cert.is_valid_cert(self.timeout_membership.as_ref()) {
                      warn!("Timeout certificate for view {} was invalid", *view);
                      return;
                  }
              }
              ViewChangeEvidence::ViewSync(view_sync_cert) => {
                  if view_sync_cert.view_number != view {
                      debug!(
                          "Cert view number {:?} does not match proposal view number {:?}",
                          view_sync_cert.view_number, view
                      );
                      return;
                  }

                  // View sync certs must also be valid.
                  if !view_sync_cert.is_valid_cert(self.quorum_membership.as_ref()) {
                      debug!("Invalid ViewSyncFinalize cert provided");
                      return;
                  }
              }
          }
      } else {
          warn!(
              "Quorum proposal for view {} needed a timeout or view sync certificate, but did not have one",
              *view);
          return;
      };
  }
...
}

This is an extremely large block as it is, and its burden is quite broad. Instead, this work would move this function to another file within the consensus module, and define something like:

pub fn validate_proposal_view_and_certs<TYPES: NodeType>(
    proposal: &Proposal<TYPES, QuorumProposal<TYPES>>,
    sender: TYPES::SignatureKey,
    cur_view: TYPES::Time,
    quorum_membership: Arc<TYPES::Membership>,
    timeout_membership: Arc<TYPES::Membership>,
) -> Result<()> {
    let view = proposal.data.get_view_number();
    ensure!(
        view > cur_view,
        "Proposal is from an older view {:?}",
        proposal.data.clone()
    );

    let view_leader_key = quorum_membership.get_leader(view);
    ensure!(
        view_leader_key == sender,
        "Leader key does not match key in proposal"
    );

    // Verify a timeout certificate OR a view sync certificate exists and is valid.
    if proposal.data.justify_qc.get_view_number() != view - 1 {
        if let Some(received_proposal_cert) = proposal.data.proposal_certificate.clone() {
            match received_proposal_cert {
                ViewChangeEvidence::Timeout(timeout_cert) => {
                    ensure!(timeout_cert.get_data().view == view - 1, "Timeout certificate for view {} was not for the immediately preceding view", *view);
                    ensure!(
                        timeout_cert.is_valid_cert(timeout_membership.as_ref()),
                        "Timeout certificate for view {} was invalid",
                        *view
                    );
                }
                ViewChangeEvidence::ViewSync(view_sync_cert) => {
                    ensure!(
                        view_sync_cert.view_number == view,
                        "View sync cert view number {:?} does not match proposal view number {:?}",
                        view_sync_cert.view_number,
                        view
                    );

                    // View sync certs must also be valid.
                    ensure!(
                        view_sync_cert.is_valid_cert(quorum_membership.as_ref()),
                        "Invalid view sync finalize cert provided"
                    );
                }
            }
        } else {
            bail!(
                "Quorum proposal for view {} needed a timeout or view sync certificate, but did not have one",
                *view);
        };
    }

    // Validate the upgrade certificate -- this is just a signature validation.
    // Note that we don't do anything with the certificate directly if this passes; it eventually gets stored as part of the leaf if nothing goes wrong.
    UpgradeCertificate::validate(&proposal.data.upgrade_certificate, &quorum_membership)?;

    Ok(())
}

This method is much easier to reason about, and can be expanded if we add new types of certificates, or need to do anything special with the view and cert validation. This is one example of the multiple functions that exist within this handler.

What work will need to be done to complete this task?

To keep this PR relatively small and easy to review/integrate, this task should be done by looking for logical partitions in the task and sending the appropriate parameters to the module functions. The goal is for the quorum proposal task handler to be no more than ~10 lines or so.

Are there any other details to include?

The method signatures are quite long, and will continue to be while we partition the code. This is an unfortunate side-effect of relying on self as we have. This will be handled in later tasks.

What are the acceptance criteria to close this issue?

This task should not require any testing changes and, thus, all tests must pass as-is. Breaking changes are explicitly out of scope for this work.

Branch work will be merged to (if not the default branch)

No response