Merge #4093 #4140

4093: Check reference at snapshot creation r=jordanschalm a=jordanschalm

This PR extends #4086, adding checks to snapshot creation to differentiate between exceptions and unknown blocks in BlockSignerDecoder. 
- Updates `protocol.Snapshot` to guarantee that `ErrUnknownSnapshotReference` is returned in all cases where a snapshot's reference is unknown
  - Add existence checking methods to `storage/common`, `storage.Cache`
- Updates BlockSignerDecoder to differentiate between errors from an unknown input block and exception
- Misc: marks all unexpected errors in `storage/common` as exceptions
- Misc: update `flow-emu` version to include onflow/flow-emulator#280 (avoid changes to `Headers` being breaking)

4140: Storage layer test refactor r=janezpodhostnik a=janezpodhostnik

Extracting pieces from #3736 so its easier to digest and merge.

Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>
Co-authored-by: Janez Podhostnik <janez.podhostnik@gmail.com>

consensus/hotstuff/committee.go

@@ -25,31 +25,31 @@ import (
 // So for validating votes/timeouts we use *ByEpoch methods.
 //
 // Since the voter committee is considered static over an epoch:
-// * we can query identities by view
-// * we don't need the full block ancestry prior to validating messages
+//   - we can query identities by view
+//   - we don't need the full block ancestry prior to validating messages
 type Replicas interface {
 
 	// LeaderForView returns the identity of the leader for a given view.
 	// CAUTION: per liveness requirement of HotStuff, the leader must be fork-independent.
 	//          Therefore, a node retains its proposer view slots even if it is slashed.
 	//          Its proposal is simply considered invalid, as it is not from a legitimate participant.
 	// Returns the following expected errors for invalid inputs:
-	//   * model.ErrViewForUnknownEpoch if no epoch containing the given view is known
+	//   - model.ErrViewForUnknownEpoch if no epoch containing the given view is known
 	LeaderForView(view uint64) (flow.Identifier, error)
 
 	// QuorumThresholdForView returns the minimum total weight for a supermajority
 	// at the given view. This weight threshold is computed using the total weight
 	// of the initial committee and is static over the course of an epoch.
 	// Returns the following expected errors for invalid inputs:
-	//   * model.ErrViewForUnknownEpoch if no epoch containing the given view is known
+	//   - model.ErrViewForUnknownEpoch if no epoch containing the given view is known
 	QuorumThresholdForView(view uint64) (uint64, error)
 
 	// TimeoutThresholdForView returns the minimum total weight of observed timeout objects
 	// required to safely timeout for the given view. This weight threshold is computed
 	// using the total weight of the initial committee and is static over the course of
 	// an epoch.
 	// Returns the following expected errors for invalid inputs:
-	//   * model.ErrViewForUnknownEpoch if no epoch containing the given view is known
+	//   - model.ErrViewForUnknownEpoch if no epoch containing the given view is known
 	TimeoutThresholdForView(view uint64) (uint64, error)
 
 	// Self returns our own node identifier.
@@ -60,23 +60,23 @@ type Replicas interface {
 
 	// DKG returns the DKG info for epoch given by the input view.
 	// Returns the following expected errors for invalid inputs:
-	//   * model.ErrViewForUnknownEpoch if no epoch containing the given view is known
+	//   - model.ErrViewForUnknownEpoch if no epoch containing the given view is known
 	DKG(view uint64) (DKG, error)
 
 	// IdentitiesByEpoch returns a list of the legitimate HotStuff participants for the epoch
 	// 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
+	//   - 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.
+	//   - is ordered in the canonical order
+	//   - contains no duplicates.
 	//
 	// 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
+	//   - model.ErrViewForUnknownEpoch if no epoch containing the given view is known
 	//
 	// TODO: should return identity skeleton https://github.com/dapperlabs/flow-go/issues/6232
 	IdentitiesByEpoch(view uint64) (flow.IdentityList, error)
@@ -87,10 +87,10 @@ type Replicas interface {
 	// 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.
+	//  - model.InvalidSignerError if participantID does NOT correspond to an authorized HotStuff participant at the specified block.
 	//
 	// Returns the following expected errors for invalid inputs:
-	//   * model.ErrViewForUnknownEpoch if no epoch containing the given view is known
+	//   - model.ErrViewForUnknownEpoch if no epoch containing the given view is known
 	//
 	// TODO: should return identity skeleton https://github.com/dapperlabs/flow-go/issues/6232
 	IdentityByEpoch(view uint64, participantID flow.Identifier) (*flow.Identity, error)
@@ -102,25 +102,27 @@ type Replicas interface {
 // For validating proposals, we use *ByBlock methods.
 //
 // Since the proposer committee can change at any block:
-// * we query by block ID
-// * we must have incorporated the full block ancestry prior to validating messages
+//   - we query by block ID
+//   - we must have incorporated the full block ancestry prior to validating messages
 type DynamicCommittee interface {
 	Replicas
 
 	// IdentitiesByBlock returns a list of the legitimate HotStuff participants for the given block.
 	// The returned list of HotStuff participants:
-	//   * contains nodes that are allowed to submit proposals, votes, and timeouts
+	//   - 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.
+	//   - is ordered in the canonical order
+	//   - contains no duplicates.
 	//
-	// No errors are expected during normal operation.
+	// ERROR conditions:
+	//  - state.ErrUnknownSnapshotReference if the blockID is for an unknown block
 	IdentitiesByBlock(blockID flow.Identifier) (flow.IdentityList, error)
 
 	// IdentityByBlock returns the full Identity for specified HotStuff participant.
 	// The node must be a legitimate HotStuff participant with NON-ZERO WEIGHT at the specified block.
 	// ERROR conditions:
-	//  * model.InvalidSignerError if participantID does NOT correspond to an authorized HotStuff participant at the specified block.
+	//  - model.InvalidSignerError if participantID does NOT correspond to an authorized HotStuff participant at the specified block.
+	//  - state.ErrUnknownSnapshotReference if the blockID is for an unknown block
 	IdentityByBlock(blockID flow.Identifier, participantID flow.Identifier) (*flow.Identity, error)
 }
 
@@ -132,8 +134,8 @@ 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:
-	//   - signature.InvalidSignerIndicesError if signer indices included in the header do
-	//     not encode a valid subset of the consensus committee
+	//  - 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

@@ -189,22 +189,27 @@ func NewConsensusCommittee(state protocol.State, me flow.Identifier) (*Consensus
 
 // 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.
+// ERROR conditions:
+//   - state.ErrUnknownSnapshotReference if the blockID is for an unknown block
 func (c *Consensus) IdentitiesByBlock(blockID flow.Identifier) (flow.IdentityList, error) {
 	il, err := c.state.AtBlockID(blockID).Identities(filter.IsVotingConsensusCommitteeMember)
-	return il, err
+	if err != nil {
+		return nil, fmt.Errorf("could not identities at block %x: %w", blockID, err) // state.ErrUnknownSnapshotReference or exception
+	}
+	return il, nil
 }
 
 // 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.
+//   - state.ErrUnknownSnapshotReference if the blockID is for an unknown 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 {
 		if protocol.IsIdentityNotFound(err) {
 			return nil, model.NewInvalidSignerErrorf("id %v is not a valid node id: %w", nodeID, err)
 		}
-		return nil, fmt.Errorf("could not get identity for node ID %x: %w", nodeID, err)
+		return nil, fmt.Errorf("could not get identity for node ID %x: %w", nodeID, err) // state.ErrUnknownSnapshotReference or exception
 	}
 	if !filter.IsVotingConsensusCommitteeMember(identity) {
 		return nil, model.NewInvalidSignerErrorf("node %v is not an authorized hotstuff voting participant", nodeID)

consensus/hotstuff/signature/block_signer_decoder.go

@@ -29,6 +29,7 @@ var _ hotstuff.BlockSignerDecoder = (*BlockSignerDecoder)(nil)
 // Expected Error returns during normal operations:
 //   - signature.InvalidSignerIndicesError if signer indices included in the header do
 //     not encode a valid subset of the consensus committee
+//   - state.ErrUnknownSnapshotReference if the input header is not a known incorporated block.
 func (b *BlockSignerDecoder) DecodeSignerIDs(header *flow.Header) (flow.IdentifierList, error) {
 	// root block does not have signer indices
 	if header.ParentVoterIndices == nil && header.View == 0 {
@@ -41,10 +42,12 @@ func (b *BlockSignerDecoder) DecodeSignerIDs(header *flow.Header) (flow.Identifi
 		if errors.Is(err, model.ErrViewForUnknownEpoch) {
 			// 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
+			// TODO: this assumes no identity table changes within epochs, must be changed for Dynamic Protocol State
+			//  See https://github.com/onflow/flow-go/issues/4085
 			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)
+					header.ID(), header.ParentView, header.ParentID, err) // state.ErrUnknownSnapshotReference or exception
 			}
 		} else {
 			return nil, fmt.Errorf("unexpected error retrieving identities for block %v: %w", header.ID(), err)

consensus/hotstuff/signature/block_signer_decoder_test.go

@@ -2,7 +2,6 @@ package signature
 
 import (
 	"errors"
-	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/mock"
@@ -14,6 +13,7 @@ import (
 	"github.com/onflow/flow-go/model/flow"
 	"github.com/onflow/flow-go/model/flow/order"
 	"github.com/onflow/flow-go/module/signature"
+	"github.com/onflow/flow-go/state"
 	"github.com/onflow/flow-go/utils/unittest"
 )
 
@@ -65,31 +65,57 @@ func (s *blockSignerDecoderSuite) Test_RootBlock() {
 	require.Empty(s.T(), ids)
 }
 
-// Test_UnexpectedCommitteeException verifies that `BlockSignerDecoder`
+// Test_CommitteeException verifies that `BlockSignerDecoder`
 // does _not_ erroneously interpret an unexpected exception from the committee as
 // a sign of an unknown block, i.e. the decoder should _not_ return an `model.ErrViewForUnknownEpoch` or `signature.InvalidSignerIndicesError`
-func (s *blockSignerDecoderSuite) Test_UnexpectedCommitteeException() {
-	exception := errors.New("unexpected exception")
+func (s *blockSignerDecoderSuite) Test_CommitteeException() {
+	s.Run("ByEpoch exception", func() {
+		exception := errors.New("unexpected exception")
+		*s.committee = *hotstuff.NewDynamicCommittee(s.T())
+		s.committee.On("IdentitiesByEpoch", mock.Anything).Return(nil, exception)
+
+		ids, err := s.decoder.DecodeSignerIDs(s.block.Header)
+		require.Empty(s.T(), ids)
+		require.NotErrorIs(s.T(), err, model.ErrViewForUnknownEpoch)
+		require.False(s.T(), signature.IsInvalidSignerIndicesError(err))
+		require.ErrorIs(s.T(), err, exception)
+	})
+	s.Run("ByBlock exception", func() {
+		exception := errors.New("unexpected exception")
+		*s.committee = *hotstuff.NewDynamicCommittee(s.T())
+		s.committee.On("IdentitiesByEpoch", mock.Anything).Return(nil, model.ErrViewForUnknownEpoch)
+		s.committee.On("IdentitiesByBlock", mock.Anything).Return(nil, exception)
+
+		ids, err := s.decoder.DecodeSignerIDs(s.block.Header)
+		require.Empty(s.T(), ids)
+		require.NotErrorIs(s.T(), err, model.ErrViewForUnknownEpoch)
+		require.False(s.T(), signature.IsInvalidSignerIndicesError(err))
+		require.ErrorIs(s.T(), err, exception)
+	})
+}
+
+// Test_UnknownEpoch_KnownBlock tests handling of a block from an un-cached epoch but
+// where the block is known - should return identities for block.
+func (s *blockSignerDecoderSuite) Test_UnknownEpoch_KnownBlock() {
 	*s.committee = *hotstuff.NewDynamicCommittee(s.T())
-	s.committee.On("IdentitiesByEpoch", mock.Anything).Return(nil, exception)
+	s.committee.On("IdentitiesByEpoch", s.block.Header.ParentView).Return(nil, model.ErrViewForUnknownEpoch)
+	s.committee.On("IdentitiesByBlock", s.block.Header.ParentID).Return(s.allConsensus, nil)
 
 	ids, err := s.decoder.DecodeSignerIDs(s.block.Header)
-	require.Empty(s.T(), ids)
-	require.NotErrorIs(s.T(), err, model.ErrViewForUnknownEpoch)
-	require.False(s.T(), signature.IsInvalidSignerIndicesError(err))
-	require.True(s.T(), errors.Is(err, exception))
+	require.NoError(s.T(), err)
+	require.Equal(s.T(), s.allConsensus.NodeIDs(), ids)
 }
 
-// Test_UnknownEpoch tests handling of a block from an unknown epoch.
-// It should propagate the sentinel error model.ErrViewForUnknownEpoch from Committee.
-func (s *blockSignerDecoderSuite) Test_UnknownEpoch() {
+// Test_UnknownEpoch_UnknownBlock tests handling of a block from an un-cached epoch
+// where the block is unknown - should propagate state.ErrUnknownSnapshotReference.
+func (s *blockSignerDecoderSuite) Test_UnknownEpoch_UnknownBlock() {
 	*s.committee = *hotstuff.NewDynamicCommittee(s.T())
 	s.committee.On("IdentitiesByEpoch", s.block.Header.ParentView).Return(nil, model.ErrViewForUnknownEpoch)
-	s.committee.On("IdentitiesByBlock", s.block.Header.ParentID).Return(nil, fmt.Errorf(""))
+	s.committee.On("IdentitiesByBlock", s.block.Header.ParentID).Return(nil, state.ErrUnknownSnapshotReference)
 
 	ids, err := s.decoder.DecodeSignerIDs(s.block.Header)
+	require.ErrorIs(s.T(), err, state.ErrUnknownSnapshotReference)
 	require.Empty(s.T(), ids)
-	require.Error(s.T(), err)
 }
 
 // Test_InvalidIndices verifies that `BlockSignerDecoder` returns