Use Safety Rules for signing block proposals

Copied issue from proprietary repo: https://github.com/dapperlabs/flow-go/issues/6900

Context

HotStuff's EventHandler uses the BlockProducer.MakeBlockProposal to generate block proposals. At the moment, the BlockProducer has its own independent logic for signing the proposals it produces. This is problematic for the following reasons:

Consensus safety is guaranteed by the SafetyRules component. Specifically, this component should have the sole authority to vote. Otherwise, it is very hard to verify that the implementation satisfies all consensus rules.

Note that the leader's signature on its own block proposal is technically also a vote (a vote from the proposer for its own block, which the protocol requires to be part of the block header (field ProposerSigData). The leader's signature must be structurally identical to a vote, because we also include it in the aggregation process to produce a QC.
We had a case, where a node crashed at an unlucky time where it had already produced a block proposal and stored it in the data base, but HotStuff was not aware of it as the node crashed just before the EventHandler could persist its updated state.
At the moment, subtle implementation details prevent a node under such circumstances from accidentally equivocating (double-proposing, which is a slashable protocol violation). However, these implementation details are not explicitly intended to prevent equivocation, nor is that aspect documented. Furthermore, this is a subtle interplay of various components that is very very hard for an engineer to notice.

ToDo

With a little more work, we can utilize the EventHandler's SafetyRules to generate the block signature. There is a bit of subtly around protecting SafetyRules from potentially concurrent access, because we are handing it to external code.

Suggestion

SafetyRules could expose the method
```
SignOwnProposal(unsignedProposal *model.Proposal) (*model.Vote, error)
```
- checks that the block was proposed by this node, essentially this check here that currently lives in the signer.
- then calls SafetyRules.ProduceVote()
Thereby, we have all consensus compliance checks localized in SafetyRules
remove method Signer.CreateProposal as this is no longer needed
EventHandler provides SafetyRules as an argument to the BlockProducer.MakeBlockProposal( .. ) call.

BlockProducer is still part of the HoStuff package. Therefore, I think it is fine for it to adhere to the single-threading design of the HotStuff event loop.

However, the module.Builder lives in a different package, for which reason I think we should try to decouple the logic as much as possible. Specifically, I think we should assume that within the module.Builder, SafetyRules's signing functionality might be potentially accessed concurrently (most general assumption).

We can guarantee concurrency safety, by having the BlockProducer wrap the SafetyRules. The wrapper could contain an atomic int that guarantees proper synchronization like this:

// safetyRulesConcurrencyWrapper wraps `hotstuff.SafetyRules` to allow its use in concurrent environment.
// Correctness requirements:
//
//     (i) The wrapper's Sign function is called exactly once (wrapper errors on repeated Sign calls)
//    (ii) SafetyRules is not accessed outside the wrapper concurrently. The wrapper cannot enforce this.
//
// The correctness condition (ii) holds because there is a single dedicated thread executing the Event Loop,
// including the EventHandler, that also runs the logic of `BlockProducer.MakeBlockProposal`.
// 
// Concurrency safety:
//  (a) There is one dedicated thread executing the Event Loop, including the EventHandler, that also runs the logic of
//      `BlockProducer.MakeBlockProposal`. Hence, while the "Event Loop Thread" is in `MakeBlockProposal`, we are guaranteed
//      the only interactions with `SafetyRules` are in `module.Builder.BuildOn`
//  (b) The Event Loop Thread instantiates the variable `signingStatus`. Furthermore, the `signer` call first reads `signingStatus`.
//      Therefore, all operations in the EventHandler prior to calling `Builder.BuildOn(..)` happen before the call to `signer`.
//      Hence, it is guaranteed that the `signer` uses the most recent state of `SafetyRules`, even if signer is executed by a
//      different thread.
//  (c) Just before the `signer` call returns, it writes `signingStatus`. Furthermore, the Event Loop Thread reads `signingStatus`
//      right after the `Builder.BuildOn(..)` call returns. Thereby, Event Loop Thread sees the most recent state of `SafetyRules`
//      after completing the signing operation
// With the transitivity of the 'Happens Before' relationship (-> go Memory Model https://go.dev/ref/mem#atomic), we have proven
// that concurrent access of the wrapped `safetyRules` is safe for the state transition:
//  instantiate signingStatus to 0  ─►  update signingStatus from 0 to 1 → signer → update signingStatus from 1 to 2  ─►  confirm signingStatus has value 2
// ╰──────────────┬───────────────╯    ╰──────────────────────────────────────┬─────────────────────────────────────╯    ╰────────────────┬────────────────╯
//        Event Loop Thread                                 within the scope of Builder.BuildOn                                  Event Loop Thread
//
// All state transitions _other_ than the one above yield exceptions without modifying `SafetyRules`.
type safetyRulesConcurrencyWrapper struct {
// signingStatus guarantees concurrency safety and encodes the progress of the signing process.
// We differentiate between 4 different states:
//  - value 0: signing is not yet started
//  - value 1: one thread has already entered the signing process, which is currently ongoing
//  - value 2: the thread that set `signingStatus` to value 1 has completed the signing
//  - value 3: proposal has been produced and signed and Event Loop thread has continued executing the `BlockProducer` logic
signingStatus atomic.Uint32
safetyRules   hotstuff.SafetyRules
}

func NewSafetyRulesConcurrencyWrapper(safetyRules hotstuff.SafetyRules) *safetyRulesConcurrencyWrapper {
return &safetyRulesConcurrencyWrapper{safetyRules: safetyRules}
}

// Sign modifies the given unsignedHeader by including the proposer's signature date.
// Safe under concurrent calls. Per convention, this method should be called exactly once.
// Only the first call will succeed, and subsequent calls error. The implementation is backed
// by `SafetyRules` and thereby guarantees consensus safety for singing block proposals.
// No errors expected during normal operations
func (w *safetyRulesConcurrencyWrapper) Sign(unsignedHeader *flow.Header) error {
if !w.signingStatus.CompareAndSwap(0, 1) { // value of `signingStatus` is something else than 0
    return fmt.Errorf("signer has already commenced signing; possebly repeated signer call")
} // signer is now in state 1, and this thread is the only one every going to execute the following logic

// signature for own block is structurally a vote
vote, err := w.safetyRules.SignOwnProposal(model.ProposalFromFlow(unsignedHeader))
if err != nil {
    return fmt.Errorf("could not sign block proposal: %w", err)
}
unsignedHeader.ProposerSigData = vote.SigData

// value of `signingStatus` is always 1, i.e. the following check always succeeds.
if !w.signingStatus.CompareAndSwap(1, 2) { // sanity check protects logic from future modifications accidentally breaking this invariant
    panic("signer wrapper completed its work but encountered state other than 1") // never happens
}
return nil
}

// IsSigningComplete atomically checks whether the Sign logic has concluded, and returns true exaclty in this case.
// By reading the atomic `signingStatus` and confirming it has the expected value, it is guaranteed that any state
// changes of `safetyRules` that happened within `Sign` are visible to the Event Loop Thread.
// No errors expected during normal operations
func (w *safetyRulesConcurrencyWrapper) IsSigningComplete() bool {
return w.signingStatus.Load() == 2
}

The only additional change in the business logic is then to minimally extend BuildOn call in the BlockProducer:

// Build block proposal and return its header. Details:
//  - By convention, this logic is executed by The EventLoop Thread. The BlockProducer is part of the hostuff package and can therefore
//    be expected to comply with hotstuff-internal design patterns, such as there being a single dedicated thread executing the Event Loop,
//    including EventHandler, SafetyRules, and BlockProducer.
//  - However, the `module.Builder` lives in a different package, for which reason I think we should try to decouple the logic as much
//    as possible. Specifically, I think we should assume that within the module.Builder, SafetyRules's signing functionality might be
//    potentially accessed concurrently (most general assumption).
//  - To minimize implementation dependencies and reduce the chance of safety-critical consensus bugs, we therefore wrap `SafetyRules`
//    to correctness under concurrent access.
signer := NewSafetyRulesConcurrencyWrapper(safetyRules)
header, err := bp.builder.BuildOn(qc.BlockID, setHotstuffFields, signer.Sign)
if err != nil {
return nil, fmt.Errorf("could not build block proposal on top of %v: %w", qc.BlockID, err)
}
if !signer.IsSigningComplete() {
return nil, fmt.Errorf("signer has not yet completed signing")
}

onflow / flow-go

Use Safety Rules for signing block proposals #6389

Context

ToDo

importance