From 39bb40e54dcb546bac4e883a1d26462e12e09474 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Mon, 20 Mar 2023 19:18:20 +0200 Subject: [PATCH 1/2] Implemented a fallback for DecodeSignerIDs when IdentitiesByEpoch fails with sentinel --- consensus/hotstuff/signature/block_signer_decoder.go | 11 +++++++++-- .../hotstuff/signature/block_signer_decoder_test.go | 1 + 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/consensus/hotstuff/signature/block_signer_decoder.go b/consensus/hotstuff/signature/block_signer_decoder.go index 4cbc2b79da3..b0eb6930fce 100644 --- a/consensus/hotstuff/signature/block_signer_decoder.go +++ b/consensus/hotstuff/signature/block_signer_decoder.go @@ -36,12 +36,19 @@ 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 identities for block %x with QC view %d: %w", header.ID(), 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 { diff --git a/consensus/hotstuff/signature/block_signer_decoder_test.go b/consensus/hotstuff/signature/block_signer_decoder_test.go index 7a612af9adf..73341c8df2a 100644 --- a/consensus/hotstuff/signature/block_signer_decoder_test.go +++ b/consensus/hotstuff/signature/block_signer_decoder_test.go @@ -13,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" ) From 3586c81872770faeeb65be846895a5f03b121e5e Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Mon, 20 Mar 2023 22:37:21 +0200 Subject: [PATCH 2/2] Fixed unit test. Updated godoc --- consensus/hotstuff/committee.go | 1 - consensus/hotstuff/signature/block_signer_decoder.go | 4 ++-- consensus/hotstuff/signature/block_signer_decoder_test.go | 6 ++++-- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/consensus/hotstuff/committee.go b/consensus/hotstuff/committee.go index 9b88ac769d1..5203ebc7bee 100644 --- a/consensus/hotstuff/committee.go +++ b/consensus/hotstuff/committee.go @@ -130,7 +130,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) diff --git a/consensus/hotstuff/signature/block_signer_decoder.go b/consensus/hotstuff/signature/block_signer_decoder.go index b0eb6930fce..ad70979c08f 100644 --- a/consensus/hotstuff/signature/block_signer_decoder.go +++ b/consensus/hotstuff/signature/block_signer_decoder.go @@ -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 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) { @@ -44,7 +43,8 @@ func (b *BlockSignerDecoder) DecodeSignerIDs(header *flow.Header) (flow.Identifi // 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) + 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) diff --git a/consensus/hotstuff/signature/block_signer_decoder_test.go b/consensus/hotstuff/signature/block_signer_decoder_test.go index 73341c8df2a..ae809cf5f23 100644 --- a/consensus/hotstuff/signature/block_signer_decoder_test.go +++ b/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