Skip to content

Commit

Permalink
Merge #4093
Browse files Browse the repository at this point in the history 
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 <jordan@dapperlabs.com>
  • Loading branch information
@bors @jordanschalm
bors[bot] and jordanschalm authored Apr 4, 2023
2 parents 96f42bf + 01b9246 commit 55a0f59
Show file tree
Hide file tree
Showing 24 changed files with 492 additions and 178 deletions.
44 changes: 23 additions & 21 deletions consensus/hotstuff/committee.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
}

Expand All @@ -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)
}

Expand Down
11 changes: 8 additions & 3 deletions consensus/hotstuff/committees/consensus_committee.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
5 changes: 4 additions & 1 deletion consensus/hotstuff/signature/block_signer_decoder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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)
Expand Down
54 changes: 40 additions & 14 deletions consensus/hotstuff/signature/block_signer_decoder_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package signature

import (
"errors"
"fmt"
"testing"

"github.com/stretchr/testify/mock"
Expand All @@ -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"
)

Expand Down Expand Up @@ -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
Expand Down
22 changes: 11 additions & 11 deletions go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ module github.com/onflow/flow-go
go 1.19

require (
cloud.google.com/go/compute/metadata v0.2.1
cloud.google.com/go/compute/metadata v0.2.3
cloud.google.com/go/profiler v0.3.0
cloud.google.com/go/storage v1.27.0
github.com/antihax/optional v1.0.0
Expand Down Expand Up @@ -89,9 +89,9 @@ require (
golang.org/x/text v0.7.0
golang.org/x/time v0.0.0-20210723032227-1f47c861a9ac
golang.org/x/tools v0.4.0
google.golang.org/api v0.102.0
google.golang.org/genproto v0.0.0-20221118155620-16455021b5e6
google.golang.org/grpc v1.52.3
google.golang.org/api v0.103.0
google.golang.org/genproto v0.0.0-20230110181048-76db0878b65f
google.golang.org/grpc v1.53.0
google.golang.org/grpc/cmd/protoc-gen-go-grpc v1.1.0
google.golang.org/protobuf v1.28.1
gotest.tools v2.2.0+incompatible
Expand All @@ -104,9 +104,9 @@ require (
)

require (
cloud.google.com/go v0.105.0 // indirect
cloud.google.com/go/compute v1.12.1 // indirect
cloud.google.com/go/iam v0.7.0 // indirect
cloud.google.com/go v0.107.0 // indirect
cloud.google.com/go/compute v1.15.1 // indirect
cloud.google.com/go/iam v0.8.0 // indirect
github.com/aws/aws-sdk-go-v2 v1.17.3 // indirect
github.com/aws/aws-sdk-go-v2/credentials v1.13.10 // indirect
github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.12.21 // indirect
Expand Down Expand Up @@ -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
Expand All @@ -159,7 +159,7 @@ require (
github.com/golang/snappy v0.0.4 // indirect
github.com/google/gopacket v1.1.19 // indirect
github.com/googleapis/enterprise-certificate-proxy v0.2.0 // indirect
github.com/googleapis/gax-go/v2 v2.6.0 // indirect
github.com/googleapis/gax-go/v2 v2.7.0 // indirect
github.com/gorilla/websocket v1.5.0 // indirect
github.com/hashicorp/errwrap v1.1.0 // indirect
github.com/hashicorp/hcl v1.0.0 // indirect
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -267,7 +267,7 @@ require (
go.uber.org/zap v1.24.0 // indirect
golang.org/x/mod v0.7.0 // indirect
golang.org/x/net v0.7.0 // indirect
golang.org/x/oauth2 v0.3.0 // indirect
golang.org/x/oauth2 v0.4.0 // indirect
golang.org/x/term v0.5.0 // indirect
golang.org/x/xerrors v0.0.0-20220907171357-04be3eba64a2 // indirect
google.golang.org/appengine v1.6.7 // indirect
Expand Down
Loading

0 comments on commit 55a0f59

Please sign in to comment.