From ba0c30b628e9665772bb1aa97e45610c85fadfd9 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Mon, 20 Mar 2023 19:18:20 +0200 Subject: [PATCH 1/6] 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 95fafcd688d..72771545dfa 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 consensus participants for view %d: %w", 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 4325b50c7b7..5294fa25a5d 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 ac73708235c0dc12f4a89e36cdec5acee5a01493 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Mon, 20 Mar 2023 22:37:21 +0200 Subject: [PATCH 2/6] 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 72771545dfa..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'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 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 5294fa25a5d..e37112d1bf0 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" @@ -84,11 +85,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 From 7364a433caa73648f8409f5da6fa7963666a0b2f Mon Sep 17 00:00:00 2001 From: Jordan Schalm Date: Wed, 22 Mar 2023 17:47:13 -0400 Subject: [PATCH 3/6] add docs, link to issue --- consensus/hotstuff/committee.go | 5 +++++ consensus/hotstuff/committees/consensus_committee.go | 7 ++++++- module/epochs.go | 1 + module/epochs/epoch_lookup.go | 1 + 4 files changed, 13 insertions(+), 1 deletion(-) diff --git a/consensus/hotstuff/committee.go b/consensus/hotstuff/committee.go index 5203ebc7bee..556c5ca6bee 100644 --- a/consensus/hotstuff/committee.go +++ b/consensus/hotstuff/committee.go @@ -73,6 +73,8 @@ type Replicas interface { // The list of all legitimate HotStuff participants for the given epoch can be obtained by using `filter.Any` // // 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 @@ -82,6 +84,9 @@ type Replicas interface { // IdentityByEpoch returns the full Identity for specified HotStuff participant. // The node must be a legitimate HotStuff participant with NON-ZERO WEIGHT at the specified block. + // 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 + // // ERROR conditions: // * model.InvalidSignerError if participantID does NOT correspond to an authorized HotStuff participant at the specified block. // diff --git a/consensus/hotstuff/committees/consensus_committee.go b/consensus/hotstuff/committees/consensus_committee.go index c61ff5941ac..08e2b861e8d 100644 --- a/consensus/hotstuff/committees/consensus_committee.go +++ b/consensus/hotstuff/committees/consensus_committee.go @@ -187,13 +187,14 @@ func NewConsensusCommittee(state protocol.State, me flow.Identifier) (*Consensus return com, nil } -// Identities returns the identities of all authorized consensus participants at the given block. +// IdentitiesByBlock returns the identities of all authorized consensus participants at the given block. // The order of the identities is the canonical order. func (c *Consensus) IdentitiesByBlock(blockID flow.Identifier) (flow.IdentityList, error) { il, err := c.state.AtBlockID(blockID).Identities(filter.IsVotingConsensusCommitteeMember) return il, err } +// IdentityByBlock returns the identity of the node with the given node ID at the given 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 { @@ -210,6 +211,8 @@ func (c *Consensus) IdentityByBlock(blockID flow.Identifier, nodeID flow.Identif // IdentitiesByEpoch returns the committee identities in the epoch which contains // the given view. +// 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 // // Error returns: // - model.ErrViewForUnknownEpoch if no committed epoch containing the given view is known. @@ -225,6 +228,8 @@ func (c *Consensus) IdentitiesByEpoch(view uint64) (flow.IdentityList, error) { // IdentityByEpoch returns the identity for the given node ID, in the epoch which // contains the given view. +// 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 // // Error returns: // - model.ErrViewForUnknownEpoch if no committed epoch containing the given view is known. diff --git a/module/epochs.go b/module/epochs.go index 874fc44a4fc..6964959e950 100644 --- a/module/epochs.go +++ b/module/epochs.go @@ -46,6 +46,7 @@ type QCContractClient interface { } // EpochLookup enables looking up epochs by view. +// CAUTION: EpochLookup should only be used for querying the previous, current, or next epoch. type EpochLookup interface { // EpochForViewWithFallback returns the counter of the epoch that the input view belongs to. diff --git a/module/epochs/epoch_lookup.go b/module/epochs/epoch_lookup.go index f0ec869a0ec..195c72159f7 100644 --- a/module/epochs/epoch_lookup.go +++ b/module/epochs/epoch_lookup.go @@ -100,6 +100,7 @@ func (cache *epochRangeCache) add(epoch epochRange) error { } // EpochLookup implements the EpochLookup interface using protocol state to match views to epochs. +// CAUTION: EpochLookup should only be used for querying the previous, current, or next epoch. type EpochLookup struct { state protocol.State mu sync.RWMutex From a0b3ea19fc1b9732f002d018e88aa4d46d50a0a2 Mon Sep 17 00:00:00 2001 From: Jordan Schalm Date: Wed, 22 Mar 2023 17:57:54 -0400 Subject: [PATCH 4/6] lint --- consensus/hotstuff/signature/block_signer_decoder_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/consensus/hotstuff/signature/block_signer_decoder_test.go b/consensus/hotstuff/signature/block_signer_decoder_test.go index e37112d1bf0..0a399797c46 100644 --- a/consensus/hotstuff/signature/block_signer_decoder_test.go +++ b/consensus/hotstuff/signature/block_signer_decoder_test.go @@ -14,7 +14,6 @@ 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 530e2d602eb8a72dbadcfd7f6d81b7076604c732 Mon Sep 17 00:00:00 2001 From: Jordan Schalm Date: Thu, 23 Mar 2023 16:27:07 -0400 Subject: [PATCH 5/6] add error docs to commitee impl --- consensus/hotstuff/committee.go | 2 +- consensus/hotstuff/committees/consensus_committee.go | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/consensus/hotstuff/committee.go b/consensus/hotstuff/committee.go index 556c5ca6bee..454d5c5ecea 100644 --- a/consensus/hotstuff/committee.go +++ b/consensus/hotstuff/committee.go @@ -117,7 +117,7 @@ type DynamicCommittee interface { // * contains no duplicates. // The list of all legitimate HotStuff participants for the given epoch can be obtained by using `filter.Any` // - // TODO - do we need this, if we are only checking a single proposer ID? + // No errors are expected during normal operation. IdentitiesByBlock(blockID flow.Identifier) (flow.IdentityList, error) // IdentityByBlock returns the full Identity for specified HotStuff participant. diff --git a/consensus/hotstuff/committees/consensus_committee.go b/consensus/hotstuff/committees/consensus_committee.go index 08e2b861e8d..156db004848 100644 --- a/consensus/hotstuff/committees/consensus_committee.go +++ b/consensus/hotstuff/committees/consensus_committee.go @@ -189,12 +189,15 @@ 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. func (c *Consensus) IdentitiesByBlock(blockID flow.Identifier) (flow.IdentityList, error) { il, err := c.state.AtBlockID(blockID).Identities(filter.IsVotingConsensusCommitteeMember) return il, err } // 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. func (c *Consensus) IdentityByBlock(blockID flow.Identifier, nodeID flow.Identifier) (*flow.Identity, error) { identity, err := c.state.AtBlockID(blockID).Identity(nodeID) if err != nil { From 9e73621f9e2e3ee6eb279cb3cf7fe4d0273db9c7 Mon Sep 17 00:00:00 2001 From: Jordan Schalm Date: Fri, 24 Mar 2023 09:51:10 -0400 Subject: [PATCH 6/6] update godoc --- consensus/hotstuff/committee.go | 5 +---- consensus/hotstuff/signature/block_signer_decoder.go | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/consensus/hotstuff/committee.go b/consensus/hotstuff/committee.go index 454d5c5ecea..120b0420e5b 100644 --- a/consensus/hotstuff/committee.go +++ b/consensus/hotstuff/committee.go @@ -64,13 +64,12 @@ type Replicas interface { DKG(view uint64) (DKG, error) // IdentitiesByEpoch returns a list of the legitimate HotStuff participants for the epoch - // given by the input view. The list of participants is filtered by the provided selector. + // 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 // (un-ejected, non-zero weight at the beginning of the epoch) // * is ordered in the canonical order // * contains no duplicates. - // The list of all legitimate HotStuff participants for the given epoch can be obtained by using `filter.Any` // // CAUTION: DO NOT use this method for validating block proposals. // CAUTION: This method considers epochs outside of Previous, Current, Next, w.r.t. the @@ -109,13 +108,11 @@ type DynamicCommittee interface { Replicas // IdentitiesByBlock returns a list of the legitimate HotStuff participants for the given block. - // The list of participants is filtered by the provided selector. // The returned list of HotStuff participants: // * 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. - // The list of all legitimate HotStuff participants for the given epoch can be obtained by using `filter.Any` // // No errors are expected during normal operation. IdentitiesByBlock(blockID flow.Identifier) (flow.IdentityList, error) diff --git a/consensus/hotstuff/signature/block_signer_decoder.go b/consensus/hotstuff/signature/block_signer_decoder.go index ad70979c08f..cfcf94e1d5c 100644 --- a/consensus/hotstuff/signature/block_signer_decoder.go +++ b/consensus/hotstuff/signature/block_signer_decoder.go @@ -11,7 +11,7 @@ import ( ) // BlockSignerDecoder is a wrapper around the `hotstuff.DynamicCommittee`, which implements -// the auxilluary logic for de-coding signer indices of a block (header) to full node IDs +// the auxiliary logic for de-coding signer indices of a block (header) to full node IDs type BlockSignerDecoder struct { hotstuff.DynamicCommittee }