Merge #4093

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)

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

Author: bors[bot]

access/validator.go

@@ -5,12 +5,12 @@ import (
 	"fmt"
 
 	"github.com/onflow/flow-go/crypto"
+	"github.com/onflow/flow-go/state"
 
 	"github.com/onflow/cadence/runtime/parser"
 
 	"github.com/onflow/flow-go/model/flow"
 	"github.com/onflow/flow-go/state/protocol"
-	"github.com/onflow/flow-go/storage"
 )
 
 type Blocks interface {
@@ -29,7 +29,7 @@ func NewProtocolStateBlocks(state protocol.State) *ProtocolStateBlocks {
 func (b *ProtocolStateBlocks) HeaderByID(id flow.Identifier) (*flow.Header, error) {
 	header, err := b.state.AtBlockID(id).Head()
 	if err != nil {
-		if errors.Is(err, storage.ErrNotFound) {
+		if errors.Is(err, state.ErrUnknownSnapshotReference) {
 			return nil, nil
 		}
 

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

engine/consensus/ingestion/core.go

@@ -18,6 +18,7 @@ import (
 	"github.com/onflow/flow-go/module/metrics"
 	"github.com/onflow/flow-go/module/signature"
 	"github.com/onflow/flow-go/module/trace"
+	"github.com/onflow/flow-go/state"
 	"github.com/onflow/flow-go/state/protocol"
 	"github.com/onflow/flow-go/storage"
 )
@@ -158,7 +159,7 @@ func (e *Core) validateGuarantors(guarantee *flow.CollectionGuarantee) error {
 	snapshot := e.state.AtBlockID(guarantee.ReferenceBlockID)
 	cluster, err := snapshot.Epochs().Current().ClusterByChainID(guarantee.ChainID)
 	// reference block not found
-	if errors.Is(err, storage.ErrNotFound) {
+	if errors.Is(err, state.ErrUnknownSnapshotReference) {
 		return engine.NewUnverifiableInputError(
 			"could not get clusters with chainID %v for unknown reference block (id=%x): %w", guarantee.ChainID, guarantee.ReferenceBlockID, err)
 	}
@@ -212,7 +213,7 @@ func (e *Core) validateOrigin(originID flow.Identifier, guarantee *flow.Collecti
 	valid, err := protocol.IsNodeAuthorizedWithRoleAt(refState, originID, flow.RoleCollection)
 	if err != nil {
 		// collection with an unknown reference block is unverifiable
-		if errors.Is(err, storage.ErrNotFound) {
+		if errors.Is(err, state.ErrUnknownSnapshotReference) {
 			return engine.NewUnverifiableInputError("could not get origin (id=%x) for unknown reference block (id=%x): %w", originID, guarantee.ReferenceBlockID, err)
 		}
 		return fmt.Errorf("unexpected error checking collection origin %x at reference block %x: %w", originID, guarantee.ReferenceBlockID, err)

go.mod

@@ -136,7 +136,7 @@ require (
 	github.com/dgraph-io/ristretto v0.0.3-0.20200630154024-f66de99634de // indirect
 	github.com/dgryski/go-farm v0.0.0-20190423205320-6a90982ecee2 // indirect
 	github.com/docker/go-units v0.5.0 // indirect
-	github.com/dustin/go-humanize v1.0.0 // indirect
+	github.com/dustin/go-humanize v1.0.1 // indirect
 	github.com/elastic/gosigar v0.14.2 // indirect
 	github.com/felixge/fgprof v0.9.3 // indirect
 	github.com/flynn/noise v1.0.0 // indirect
@@ -207,7 +207,7 @@ require (
 	github.com/marten-seemann/qtls-go1-19 v0.1.1 // indirect
 	github.com/marten-seemann/tcp v0.0.0-20210406111302-dfbc87cc63fd // indirect
 	github.com/mattn/go-colorable v0.1.13 // indirect
-	github.com/mattn/go-isatty v0.0.16 // indirect
+	github.com/mattn/go-isatty v0.0.17 // indirect
 	github.com/mattn/go-pointer v0.0.1 // indirect
 	github.com/mattn/go-runewidth v0.0.13 // indirect
 	github.com/matttproud/golang_protobuf_extensions v1.0.4 // indirect

go.sum

@@ -294,8 +294,9 @@ github.com/docker/go-units v0.5.0 h1:69rxXcBk27SvSaaxTtLh/8llcHD8vYHT7WSdRZ/jvr4
 github.com/docker/go-units v0.5.0/go.mod h1:fgPhTUdO+D/Jk86RDLlptpiXQzgHJF7gydDDbaIK4Dk=
 github.com/dop251/goja v0.0.0-20200219165308-d1232e640a87/go.mod h1:Mw6PkjjMXWbTj+nnj4s3QPXq1jaT0s5pC0iFD4+BOAA=
 github.com/dustin/go-humanize v0.0.0-20171111073723-bb3d318650d4/go.mod h1:HtrtbFcZ19U5GC7JDqmcUSB87Iq5E25KnS6fMYU6eOk=
-github.com/dustin/go-humanize v1.0.0 h1:VSnTsYCnlFHaM2/igO1h6X3HA71jcobQuxemgkq4zYo=
 github.com/dustin/go-humanize v1.0.0/go.mod h1:HtrtbFcZ19U5GC7JDqmcUSB87Iq5E25KnS6fMYU6eOk=
+github.com/dustin/go-humanize v1.0.1 h1:GzkhY7T5VNhEkwH0PVJgjz+fX1rhBrR7pRT3mDkpeCY=
+github.com/dustin/go-humanize v1.0.1/go.mod h1:Mu1zIs6XwVuF/gI1OepvI0qD18qycQx+mFykh5fBlto=
 github.com/eapache/go-resiliency v1.1.0/go.mod h1:kFI+JgMyC7bLPUVY133qvEBtVayf5mFgVsvEsIPBvNs=
 github.com/eapache/go-xerial-snappy v0.0.0-20180814174437-776d5712da21/go.mod h1:+020luEh2TKB4/GOp8oxxtq0Daoen/Cii55CzbTV6DU=
 github.com/eapache/queue v1.1.0/go.mod h1:6eCeP0CKFpHLu8blIFXhExK/dRa7WDZfr6jVFPTqq+I=
@@ -1069,8 +1070,9 @@ github.com/mattn/go-isatty v0.0.9/go.mod h1:YNRxwqDuOph6SZLI9vUUz6OYw3QyUt7WiY2y
 github.com/mattn/go-isatty v0.0.12/go.mod h1:cbi8OIDigv2wuxKPP5vlRcQ1OAZbq2CE4Kysco4FUpU=
 github.com/mattn/go-isatty v0.0.13/go.mod h1:cbi8OIDigv2wuxKPP5vlRcQ1OAZbq2CE4Kysco4FUpU=
 github.com/mattn/go-isatty v0.0.14/go.mod h1:7GGIvUiUoEMVVmxf/4nioHXj79iQHKdU27kJ6hsGG94=
-github.com/mattn/go-isatty v0.0.16 h1:bq3VjFmv/sOjHtdEhmkEV4x1AJtvUvOJ2PFAZ5+peKQ=
 github.com/mattn/go-isatty v0.0.16/go.mod h1:kYGgaQfpe5nmfYZH+SKPsOc2e4SrIfOl2e/yFXSvRLM=
+github.com/mattn/go-isatty v0.0.17 h1:BTarxUcIeDqL27Mc+vyvdWYSL28zpIhv3RoTdsLMPng=
+github.com/mattn/go-isatty v0.0.17/go.mod h1:kYGgaQfpe5nmfYZH+SKPsOc2e4SrIfOl2e/yFXSvRLM=
 github.com/mattn/go-pointer v0.0.1 h1:n+XhsuGeVO6MEAp7xyEukFINEa+Quek5psIR/ylA6o0=
 github.com/mattn/go-pointer v0.0.1/go.mod h1:2zXcozF6qYGgmsG+SeTZz3oAbFLdD3OWqnUbNvJZAlc=
 github.com/mattn/go-runewidth v0.0.2/go.mod h1:LwmH8dsx7+W8Uxz3IHJYH5QSwggIsqBzpuz5H//U1FU=