Merge #4086

4086: Port #4063 to master r=jordanschalm a=jordanschalm

- Ports #4063 to `master`
- Adds additional documentation of the issue, and a link to an issue 7364a43

Co-authored-by: Yurii Oleksyshyn <[email protected]>
Co-authored-by: Jordan Schalm <[email protected]>

Author: 3 people

consensus/hotstuff/committee.go

@@ -64,15 +64,16 @@ type Replicas interface {
 	DKG(view uint64) (DKG, error)
 
 	// IdentitiesByEpoch returns a list of the legitimate HotStuff participants for the epoch
-	// given by the input view. The list of participants is filtered by the provided selector.
+	// given by the input view.
 	// The returned list of HotStuff participants:
 	//   * contains nodes that are allowed to submit votes or timeouts within the given epoch
 	//     (un-ejected, non-zero weight at the beginning of the epoch)
 	//   * is ordered in the canonical order
 	//   * contains no duplicates.
-	// The list of all legitimate HotStuff participants for the given epoch can be obtained by using `filter.Any`
 	//
 	// CAUTION: DO NOT use this method for validating block proposals.
+	// CAUTION: This method considers epochs outside of Previous, Current, Next, w.r.t. the
+	// finalized block, to be unknown. https://github.com/onflow/flow-go/issues/4085
 	//
 	// Returns the following expected errors for invalid inputs:
 	//   * model.ErrViewForUnknownEpoch if no epoch containing the given view is known
@@ -82,6 +83,9 @@ type Replicas interface {
 
 	// IdentityByEpoch returns the full Identity for specified HotStuff participant.
 	// The node must be a legitimate HotStuff participant with NON-ZERO WEIGHT at the specified block.
+	// CAUTION: This method considers epochs outside of Previous, Current, Next, w.r.t. the
+	// finalized block, to be unknown. https://github.com/onflow/flow-go/issues/4085
+	//
 	// ERROR conditions:
 	//  * model.InvalidSignerError if participantID does NOT correspond to an authorized HotStuff participant at the specified block.
 	//
@@ -104,15 +108,13 @@ type DynamicCommittee interface {
 	Replicas
 
 	// IdentitiesByBlock returns a list of the legitimate HotStuff participants for the given block.
-	// The list of participants is filtered by the provided selector.
 	// The returned list of HotStuff participants:
 	//   * contains nodes that are allowed to submit proposals, votes, and timeouts
 	//     (un-ejected, non-zero weight at current block)
 	//   * is ordered in the canonical order
 	//   * contains no duplicates.
-	// The list of all legitimate HotStuff participants for the given epoch can be obtained by using `filter.Any`
 	//
-	// TODO - do we need this, if we are only checking a single proposer ID?
+	// No errors are expected during normal operation.
 	IdentitiesByBlock(blockID flow.Identifier) (flow.IdentityList, error)
 
 	// IdentityByBlock returns the full Identity for specified HotStuff participant.
@@ -130,7 +132,6 @@ type BlockSignerDecoder interface {
 	// consensus committee has reached agreement on validity of parent block. Consequently, the
 	// returned IdentifierList contains the consensus participants that signed the parent block.
 	// Expected Error returns during normal operations:
-	//   - model.ErrViewForUnknownEpoch if the given block's parent is within an unknown epoch
 	//   - signature.InvalidSignerIndicesError if signer indices included in the header do
 	//     not encode a valid subset of the consensus committee
 	DecodeSignerIDs(header *flow.Header) (flow.IdentifierList, error)

consensus/hotstuff/committees/consensus_committee.go

@@ -187,13 +187,17 @@ func NewConsensusCommittee(state protocol.State, me flow.Identifier) (*Consensus
 	return com, nil
 }
 
-// Identities returns the identities of all authorized consensus participants at the given block.
+// IdentitiesByBlock returns the identities of all authorized consensus participants at the given block.
 // The order of the identities is the canonical order.
+// No errors are expected during normal operation.
 func (c *Consensus) IdentitiesByBlock(blockID flow.Identifier) (flow.IdentityList, error) {
 	il, err := c.state.AtBlockID(blockID).Identities(filter.IsVotingConsensusCommitteeMember)
 	return il, err
 }
 
+// IdentityByBlock returns the identity of the node with the given node ID at the given block.
+// ERROR conditions:
+//   - model.InvalidSignerError if participantID does NOT correspond to an authorized HotStuff participant at the specified block.
 func (c *Consensus) IdentityByBlock(blockID flow.Identifier, nodeID flow.Identifier) (*flow.Identity, error) {
 	identity, err := c.state.AtBlockID(blockID).Identity(nodeID)
 	if err != nil {
@@ -210,6 +214,8 @@ func (c *Consensus) IdentityByBlock(blockID flow.Identifier, nodeID flow.Identif
 
 // IdentitiesByEpoch returns the committee identities in the epoch which contains
 // the given view.
+// CAUTION: This method considers epochs outside of Previous, Current, Next, w.r.t. the
+// finalized block, to be unknown. https://github.com/onflow/flow-go/issues/4085
 //
 // Error returns:
 //   - model.ErrViewForUnknownEpoch if no committed epoch containing the given view is known.
@@ -225,6 +231,8 @@ func (c *Consensus) IdentitiesByEpoch(view uint64) (flow.IdentityList, error) {
 
 // IdentityByEpoch returns the identity for the given node ID, in the epoch which
 // contains the given view.
+// CAUTION: This method considers epochs outside of Previous, Current, Next, w.r.t. the
+// finalized block, to be unknown. https://github.com/onflow/flow-go/issues/4085
 //
 // Error returns:
 //   - model.ErrViewForUnknownEpoch if no committed epoch containing the given view is known.

consensus/hotstuff/signature/block_signer_decoder.go

@@ -11,7 +11,7 @@ import (
 )
 
 // BlockSignerDecoder is a wrapper around the `hotstuff.DynamicCommittee`, which implements
-// the auxilluary logic for de-coding signer indices of a block (header) to full node IDs
+// the auxiliary logic for de-coding signer indices of a block (header) to full node IDs
 type BlockSignerDecoder struct {
 	hotstuff.DynamicCommittee
 }
@@ -27,7 +27,6 @@ var _ hotstuff.BlockSignerDecoder = (*BlockSignerDecoder)(nil)
 // consensus committee has reached agreement on validity of parent block. Consequently, the
 // returned IdentifierList contains the consensus participants that signed the parent block.
 // Expected Error returns during normal operations:
-//   - model.ErrViewForUnknownEpoch if the given block's parent is within an unknown epoch
 //   - signature.InvalidSignerIndicesError if signer indices included in the header do
 //     not encode a valid subset of the consensus committee
 func (b *BlockSignerDecoder) DecodeSignerIDs(header *flow.Header) (flow.IdentifierList, error) {
@@ -36,12 +35,20 @@ func (b *BlockSignerDecoder) DecodeSignerIDs(header *flow.Header) (flow.Identifi
 		return []flow.Identifier{}, nil
 	}
 
+	// we will use IdentitiesByEpoch since it's a faster call and avoids DB lookup
 	members, err := b.IdentitiesByEpoch(header.ParentView)
 	if err != nil {
 		if errors.Is(err, model.ErrViewForUnknownEpoch) {
-			return nil, fmt.Errorf("could not retrieve consensus participants for view %d: %w", header.ParentView, err)
+			// possibly, we request epoch which is far behind in the past, in this case we won't have it in cache.
+			// try asking by parent ID
+			members, err = b.IdentitiesByBlock(header.ParentID)
+			if err != nil {
+				return nil, fmt.Errorf("could not retrieve identities for block %x with QC view %d for parent %x: %w",
+					header.ID(), header.ParentView, header.ParentID, err)
+			}
+		} else {
+			return nil, fmt.Errorf("unexpected error retrieving identities for block %v: %w", header.ID(), err)
 		}
-		return nil, fmt.Errorf("unexpected error retrieving identities for block %v: %w", header.ID(), err)
 	}
 	signerIDs, err := signature.DecodeSignerIndicesToIdentifiers(members.NodeIDs(), header.ParentVoterIndices)
 	if err != nil {

consensus/hotstuff/signature/block_signer_decoder_test.go

@@ -2,6 +2,7 @@ package signature
 
 import (
 	"errors"
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/mock"
@@ -83,11 +84,12 @@ func (s *blockSignerDecoderSuite) Test_UnexpectedCommitteeException() {
 // It should propagate the sentinel error model.ErrViewForUnknownEpoch from Committee.
 func (s *blockSignerDecoderSuite) Test_UnknownEpoch() {
 	*s.committee = *hotstuff.NewDynamicCommittee(s.T())
-	s.committee.On("IdentitiesByEpoch", mock.Anything).Return(nil, model.ErrViewForUnknownEpoch)
+	s.committee.On("IdentitiesByEpoch", s.block.Header.ParentView).Return(nil, model.ErrViewForUnknownEpoch)
+	s.committee.On("IdentitiesByBlock", s.block.Header.ParentID).Return(nil, fmt.Errorf(""))
 
 	ids, err := s.decoder.DecodeSignerIDs(s.block.Header)
 	require.Empty(s.T(), ids)
-	require.ErrorIs(s.T(), err, model.ErrViewForUnknownEpoch)
+	require.Error(s.T(), err)
 }
 
 // Test_InvalidIndices verifies that `BlockSignerDecoder` returns

module/epochs.go

@@ -46,6 +46,7 @@ type QCContractClient interface {
 }
 
 // EpochLookup enables looking up epochs by view.
+// CAUTION: EpochLookup should only be used for querying the previous, current, or next epoch.
 type EpochLookup interface {
 
 	// EpochForViewWithFallback returns the counter of the epoch that the input view belongs to.

module/epochs/epoch_lookup.go

@@ -100,6 +100,7 @@ func (cache *epochRangeCache) add(epoch epochRange) error {
 }
 
 // EpochLookup implements the EpochLookup interface using protocol state to match views to epochs.
+// CAUTION: EpochLookup should only be used for querying the previous, current, or next epoch.
 type EpochLookup struct {
 	state                    protocol.State
 	mu                       sync.RWMutex