[v0.29] Backport: Fix OBO and inconsistency in signer indices decoding used in Access API #3906

access/handler.go

@@ -492,7 +492,7 @@ func (h *Handler) GetExecutionResultForBlockID(ctx context.Context, req *access.
 func (h *Handler) blockResponse(block *flow.Block, fullResponse bool, status flow.BlockStatus) (*access.BlockResponse, error) {
 	signerIDs, err := h.signerIndicesDecoder.DecodeSignerIDs(block.Header)
 	if err != nil {
-		return nil, err
+		return nil, err // the block was retrieved from local storage - so no errors are expected
 	}
 
 	var msg *entities.Block
@@ -513,7 +513,7 @@ func (h *Handler) blockResponse(block *flow.Block, fullResponse bool, status flo
 func (h *Handler) blockHeaderResponse(header *flow.Header, status flow.BlockStatus) (*access.BlockHeaderResponse, error) {
 	signerIDs, err := h.signerIndicesDecoder.DecodeSignerIDs(header)
 	if err != nil {
-		return nil, err
+		return nil, err // the block was retrieved from local storage - so no errors are expected
 	}
 
 	msg, err := convert.BlockHeaderToMessage(header, signerIDs)

consensus/hotstuff/committee.go

@@ -122,15 +122,17 @@ type DynamicCommittee interface {
 	IdentityByBlock(blockID flow.Identifier, participantID flow.Identifier) (*flow.Identity, error)
 }
 
-// BlockSignerDecoder defines how to convert the SignerIndices field within a particular
-// block header to the identifiers of the nodes which signed the block.
+// BlockSignerDecoder defines how to convert the ParentSignerIndices field within a
+// particular block header to the identifiers of the nodes which signed the block.
 type BlockSignerDecoder interface {
 	// DecodeSignerIDs decodes the signer indices from the given block header into full node IDs.
+	// Note: A block header contains a quorum certificate for its parent, which proves that the
+	// 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:
-	//  * state.UnknownBlockError if block has not been ingested yet
-	//    TODO: this sentinel could be changed to `ErrViewForUnknownEpoch` once we merge the active pacemaker
-	//  * signature.InvalidSignerIndicesError if signer indices included in the header do
-	//    not encode a valid subset of the consensus committee
+	//   - 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/signature/block_signer_decoder.go

@@ -5,10 +5,9 @@ import (
 	"fmt"
 
 	"github.com/onflow/flow-go/consensus/hotstuff"
+	"github.com/onflow/flow-go/consensus/hotstuff/model"
 	"github.com/onflow/flow-go/model/flow"
 	"github.com/onflow/flow-go/module/signature"
-	"github.com/onflow/flow-go/state"
-	"github.com/onflow/flow-go/storage"
 )
 
 // BlockSignerDecoder is a wrapper around the `hotstuff.DynamicCommittee`, which implements
@@ -24,8 +23,11 @@ func NewBlockSignerDecoder(committee hotstuff.DynamicCommittee) *BlockSignerDeco
 var _ hotstuff.BlockSignerDecoder = (*BlockSignerDecoder)(nil)
 
 // DecodeSignerIDs decodes the signer indices from the given block header into full node IDs.
+// Note: A block header contains a quorum certificate for its parent, which proves that the
+// 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:
-//   - state.UnknownBlockError if block has not been ingested yet
+//   - model.ErrViewForUnknownEpoch if the given block 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) {
@@ -34,15 +36,12 @@ func (b *BlockSignerDecoder) DecodeSignerIDs(header *flow.Header) (flow.Identifi
 		return []flow.Identifier{}, nil
 	}
 
-	id := header.ID()
-	members, err := b.IdentitiesByBlock(id)
+	members, err := b.IdentitiesByEpoch(header.ParentView)
 	if err != nil {
-		// TODO: this potentially needs to be updated when we implement and document proper error handling for
-		//       `hotstuff.Committee` and underlying code (such as `protocol.Snapshot`)
-		if errors.Is(err, storage.ErrNotFound) {
-			return nil, state.WrapAsUnknownBlockError(id, err)
+		if errors.Is(err, model.ErrViewForUnknownEpoch) {
+			return nil, fmt.Errorf("could not retrieve identities for block %x with QC view %d: %w", header.ID(), header.ParentView, err)
 		}
-		return nil, fmt.Errorf("fail to retrieve identities for block %v: %w", 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

@@ -9,11 +9,10 @@ import (
 	"github.com/stretchr/testify/suite"
 
 	hotstuff "github.com/onflow/flow-go/consensus/hotstuff/mocks"
+	"github.com/onflow/flow-go/consensus/hotstuff/model"
 	"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/storage"
 	"github.com/onflow/flow-go/utils/unittest"
 )
 
@@ -36,7 +35,7 @@ func (s *blockSignerDecoderSuite) SetupTest() {
 
 	// mock consensus committee
 	s.committee = hotstuff.NewDynamicCommittee(s.T())
-	s.committee.On("IdentitiesByBlock", mock.Anything).Return(s.allConsensus, nil).Maybe()
+	s.committee.On("IdentitiesByEpoch", mock.Anything).Return(s.allConsensus, nil).Maybe()
 
 	// prepare valid test block:
 	voterIndices, err := signature.EncodeSignersToIndices(s.allConsensus.NodeIDs(), s.allConsensus.NodeIDs())
@@ -65,38 +64,60 @@ func (s *blockSignerDecoderSuite) Test_RootBlock() {
 	require.Empty(s.T(), ids)
 }
 
-// Test_UnknownBlock tests handling of an unknwon block.
-// At the moment, hotstuff.Committee returns an storage.ErrNotFound for an unknown block,
-// which we expect the `BlockSignerDecoder` to wrap into a `state.UnknownBlockError`
-func (s *blockSignerDecoderSuite) Test_UnknownBlock() {
+// Test_UnexpectedCommitteeException 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 `state.UnknownBlockError`
+func (s *blockSignerDecoderSuite) Test_UnexpectedCommitteeException() {
+	exception := errors.New("unexpected exception")
 	*s.committee = *hotstuff.NewDynamicCommittee(s.T())
-	s.committee.On("IdentitiesByBlock", mock.Anything).Return(nil, storage.ErrNotFound)
+	s.committee.On("IdentitiesByEpoch", mock.Anything).Return(nil, exception)
 
 	ids, err := s.decoder.DecodeSignerIDs(s.block.Header)
 	require.Empty(s.T(), ids)
-	require.True(s.T(), state.IsUnknownBlockError(err))
+	require.False(s.T(), state.IsUnknownBlockError(err))
+	require.True(s.T(), errors.Is(err, exception))
 }
 
-// Test_UnexpectedCommitteeException verifies that `BlockSignerDecoder`
-// does _not_ erroneously interpret an unexpecgted exception from the committee as
-// a sign of an unknown block, i.e. the decouder should _not_ return an `state.UnknownBlockError`
-func (s *blockSignerDecoderSuite) Test_UnexpectedCommitteeException() {
-	exception := errors.New("unexpected exception")
+// 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() {
 	*s.committee = *hotstuff.NewDynamicCommittee(s.T())
-	s.committee.On("IdentitiesByBlock", mock.Anything).Return(nil, exception)
+	s.committee.On("IdentitiesByEpoch", mock.Anything).Return(nil, model.ErrViewForUnknownEpoch)
 
 	ids, err := s.decoder.DecodeSignerIDs(s.block.Header)
 	require.Empty(s.T(), ids)
-	require.False(s.T(), state.IsUnknownBlockError(err))
-	require.True(s.T(), errors.Is(err, exception))
+	require.ErrorIs(s.T(), err, model.ErrViewForUnknownEpoch)
 }
 
 // Test_InvalidIndices verifies that `BlockSignerDecoder` returns
-// signature.InvalidSignerIndicesError is the signer indices in the provided header
+// signature.InvalidSignerIndicesError if the signer indices in the provided header
 // are not a valid encoding.
 func (s *blockSignerDecoderSuite) Test_InvalidIndices() {
 	s.block.Header.ParentVoterIndices = unittest.RandomBytes(1)
 	ids, err := s.decoder.DecodeSignerIDs(s.block.Header)
 	require.Empty(s.T(), ids)
 	require.True(s.T(), signature.IsInvalidSignerIndicesError(err))
 }
+
+// Test_EpochTransition verifies that `BlockSignerDecoder` correctly handles blocks
+// near a boundary where the committee changes - an epoch transition.
+func (s *blockSignerDecoderSuite) Test_EpochTransition() {
+	// The block under test B is the first block of a new epoch, where the committee changed.
+	// B contains a QC formed during the view of B's parent -- hence B's signatures must
+	// be validated w.r.t. the committee as of this view.
+	//
+	//   Epoch 1     Epoch 2
+	//   PARENT <- | -- B
+	blockView := s.block.Header.View
+	parentView := s.block.Header.ParentView
+	epoch1Committee := s.allConsensus
+	epoch2Committee := s.allConsensus.SamplePct(.8)
+
+	*s.committee = *hotstuff.NewDynamicCommittee(s.T())
+	s.committee.On("IdentitiesByEpoch", parentView).Return(epoch1Committee, nil).Maybe()
+	s.committee.On("IdentitiesByEpoch", blockView).Return(epoch2Committee, nil).Maybe()
+
+	ids, err := s.decoder.DecodeSignerIDs(s.block.Header)
+	require.NoError(s.T(), err)
+	require.Equal(s.T(), epoch1Committee.NodeIDs(), ids)
+}

engine/execution/rpc/engine.go

@@ -557,6 +557,7 @@ func (h *handler) GetBlockHeaderByID(
 func (h *handler) blockHeaderResponse(header *flow.Header) (*execution.BlockHeaderResponse, error) {
 	signerIDs, err := h.signerIndicesDecoder.DecodeSignerIDs(header)
 	if err != nil {
+		// the block was retrieved from local storage - so no errors are expected
 		return nil, fmt.Errorf("failed to decode signer indices to Identifiers for block %v: %w", header.ID(), err)
 	}
 

engine/protocol/handler.go

@@ -143,7 +143,7 @@ func (h *Handler) GetBlockByID(
 func (h *Handler) blockResponse(block *flow.Block, fullResponse bool) (*access.BlockResponse, error) {
 	signerIDs, err := h.signerIndicesDecoder.DecodeSignerIDs(block.Header)
 	if err != nil {
-		return nil, err
+		return nil, err // the block was retrieved from local storage - so no errors are expected
 	}
 
 	var msg *entities.Block
@@ -163,7 +163,7 @@ func (h *Handler) blockResponse(block *flow.Block, fullResponse bool) (*access.B
 func (h *Handler) blockHeaderResponse(header *flow.Header) (*access.BlockHeaderResponse, error) {
 	signerIDs, err := h.signerIndicesDecoder.DecodeSignerIDs(header)
 	if err != nil {
-		return nil, err
+		return nil, err // the block was retrieved from local storage - so no errors are expected
 	}
 
 	msg, err := convert.BlockHeaderToMessage(header, signerIDs)