[BFT] Refactor `model.Proposal`

[durkmurder]

💡 Possibility to remove a whole bunch of unused code?

I just took a look at the usage of Proposal.SigData. Not entirely sure, but my impression is that there is a lot of code that writes to this field that we never use (anymore).

At the moment, the broader hotstuff package knows the data structures Proposal, which is essentially a wrapper around hotstuff's notion of Block: https://github.com/onflow/flow-go/blob/9a8918431026277e9eac4e763a700366d554f335/consensus/hotstuff/model/proposal.go#L9-L13

Conclusions:

• conceptually, there is a strict causal dependency:
  1. first, we need to construct the unsigned proposal, which must include proof of a protocol-compliant transition into the proposal's view (which requires the QC and optionally a TC)
  2. Only for this combination of block (including the QC pointing to the parent) and TC, we can decide whether the data structure is safe to sign (see Safety Rules's IsSafeToVote method). Note that the proposer's signature for its own block is not required.
• This suggest that a more differentiated stack of data structures could be useful here. to accurately express the conceptual dependencies. Specifically, I would suggest:
  1. Rename the existing Proposal to SignedProposal.
  2. Now modify the resulting SignedProposal struct to:

      // SignedProposal represent a new proposed block within HotStuff (and thus
      // a header in the bigger picture), signed by the proposer.
      type SignedProposal struct {
      	Proposal
      	SigData []byte
      }
     

     (as we previously renamed Proposal to SignedProposal, in the resulting code the new field Proposal has now an undefined type)
  3. We implement the unsigned Proposal now as

     // Proposal represent a block proposal under construction.
     // In order to decide whether a proposal is safe to sign, HotStuff's Safety Rules require
     // proof that the leader entered the respective view in a protocol-compliant manner. Specifically,
     // we require the QC (mandatory part of every block) and optionally a TC (of the QC in the block
     // is _not_ for the immediately previous view). Note that the proposer's signature for its own
     // block is _not_ required.
     // By explicitly differentiating the Proposal from the SignedProposal (extending Proposal by
     // adding the proposer's signature), we can unify the algorithmic path of signing block proposals.
     // This codifies the important aspect that a proposer's signature for their own block
     // is conceptually also just a vote (we explicitly use that for aggregating votes, including the
     // proposer's own vote to a QC). In order to express this conceptual equivalence in code, the
     // voting logic in Safety Rules must also operate on an unsigned Proposal.
     type Proposal struct {
     	Block      *Block
     	LastViewTC *flow.TimeoutCertificate
     }
     

• With these changes, we should be able to consolidate the SafetyRules
  • I would extract the core business logic for voting, that we want to use both for signing own proposals (under construction, without signature) and other replica's proposals (with signature):

    func (r *SafetyRules) ProduceVote(signedProposal *model.SignedProposal, curView uint64) (*model.Vote, error) {
    	return r.produceVote(&signedProposal.Proposal, curView)
    }
    

  • with

    // produceVote implements the core Safety Rules to validate whether it is safe to vote.
    // This method is to be used for voting for other leaders' blocks as well as this node's own proposals
    // under construction. We explicitly codify the important aspect that a proposer's signature for their
    // own block is conceptually also just a vote (we explicitly use that property when aggregating votes and
    // including the proposer's own vote into a QC). In order to express this conceptual equivalence in code, the
    // voting logic in Safety Rules must also operate on an unsigned Proposal.
    //
    // The curView is taken as input to ensure SafetyRules will only vote for proposals at current view and prevent double voting.
    // Returns:
    //   - (vote, nil): On the _first_ block for the current view that is safe to vote for.
    //     Subsequently, voter does _not_ vote for any other block with the same (or lower) view.
    //   - (nil, model.NoVoteError): If the voter decides that it does not want to vote for the given block.
    //     This is a sentinel error and _expected_ during normal operation.
    //
    // All other errors are unexpected and potential symptoms of uncovered edge cases or corrupted internal state (fatal).
    func (r *SafetyRules) produceVote(proposal *model.Proposal, curView uint64) (*model.Vote, error) {
    
          ⋮
          core business logic https://github.com/onflow/flow-go/blob/9a8918431026277e9eac4e763a700366d554f335/consensus/hotstuff/safetyrules/safety_rules.go#L77-L133
          ⋮
    
    }
    

Final Benefit:

• at this point, I think we have a cleaner representation of the equivalence of votes and proposer signatures in the code
• However, the main benefit would be that we could also cleanly remove the following logic, which is think is purely legacy at this point:
  • method StakingSigner.CreateProposal could be removed entirely in my opinion(requires some updates of tests)
  • similarly, I hope CombinedSignerV3.CreateProposal could be removed
• Lastly, we reduce data structures, that are used with some values still undefined. I am strongly discouraging the pattern of using partially defined data structures, as this is one of the root causes of our malleability problems we have with some of our other structs.

I think this is probably entirely out of scope of this PR, but it would be a nice follow-up consolidation of code. Yurii, would you mind creating an issue? Thanks

Originally posted by @AlexHentschel in https://github.com/onflow/flow-go/pull/6463#discussion_r1779370452