Skip to content

Commit 557e4cf

Browse files
committed
Updated follower state to stop checking orphan blocks
1 parent 5d94dd9 commit 557e4cf

File tree

4 files changed

+75
-45
lines changed

4 files changed

+75
-45
lines changed

engine/common/follower/core.go

Lines changed: 10 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ import (
1818
"github.com/onflow/flow-go/module/component"
1919
"github.com/onflow/flow-go/module/irrecoverable"
2020
"github.com/onflow/flow-go/module/trace"
21-
"github.com/onflow/flow-go/state"
2221
"github.com/onflow/flow-go/state/protocol"
2322
)
2423

@@ -121,7 +120,7 @@ func NewCore(log zerolog.Logger,
121120
// Effectively, this function validates incoming batch, adds it to cache of pending blocks and possibly schedules blocks for further
122121
// processing if they were certified.
123122
// This function is safe to use in concurrent environment.
124-
// Caution: this function might block if internally too many certified blocks are queued in the channel `certifiedBlocksChan`.
123+
// Caution: this function might block if internally too many certified blocks are queued in the channel `certifiedBlocksChan`.
125124
// Expected errors during normal operations:
126125
// - ErrDisconnectedBatch
127126
func (c *Core) OnBlockRange(originID flow.Identifier, batch []*flow.Block) error {
@@ -147,16 +146,16 @@ func (c *Core) OnBlockRange(originID flow.Identifier, batch []*flow.Block) error
147146

148147
if c.pendingCache.Peek(hotstuffProposal.Block.BlockID) == nil {
149148
log.Debug().Msg("block not found in cache, performing validation")
150-
// Caution: we are _not_ verifying the proposal's full validity here. Instead, we need to check
151-
// the following two critical properties:
152-
// 1. The block has been signed by the legitimate primary for the view. This is important in case
153-
// there are multiple blocks for the view. We need to differentiate the following byzantine cases:
149+
// Caution: we are _not_ verifying the proposal's full validity here. Instead, we need to check
150+
// the following two critical properties:
151+
// 1. The block has been signed by the legitimate primary for the view. This is important in case
152+
// there are multiple blocks for the view. We need to differentiate the following byzantine cases:
154153
// (i) Some other consensus node that is _not_ primary is trying to publish a block.
155154
// This would result in the validation below failing with and `InvalidBlockError`.
156155
// (ii) The legitimate primary for the view is equivocating. In this case, the validity check
157-
// below would pass. Though, the `PendingTree` would eventually notice this, when we connect
158-
// the equivocating blocks to the latest finalized block.
159-
// 2. The QC within the block is valid. A valid QC proves validity of all ancestors.
156+
// below would pass. Though, the `PendingTree` would eventually notice this, when we connect
157+
// the equivocating blocks to the latest finalized block.
158+
// 2. The QC within the block is valid. A valid QC proves validity of all ancestors.
160159
err := c.validator.ValidateProposal(hotstuffProposal)
161160
if err != nil {
162161
if model.IsInvalidBlockError(err) {
@@ -233,8 +232,8 @@ func (c *Core) processCoreSeqEvents(ctx irrecoverable.SignalerContext, ready com
233232
// OnFinalizedBlock updates local state of pendingCache tree using received finalized block and queues finalized block
234233
// to be processed by internal goroutine.
235234
// This function is safe to use in concurrent environment.
236-
// CAUTION: this function blocks and is therefore not compliant with the `FinalizationConsumer.OnFinalizedBlock`
237-
// interface. This function should only be executed within the a worker routine.
235+
// CAUTION: this function blocks and is therefore not compliant with the `FinalizationConsumer.OnFinalizedBlock`
236+
// interface. This function should only be executed within the a worker routine.
238237
func (c *Core) OnFinalizedBlock(final *flow.Header) {
239238
c.pendingCache.PruneUpToView(final.View)
240239

@@ -278,9 +277,6 @@ func (c *Core) extendCertifiedBlocks(parentCtx context.Context, connectedBlocks
278277
err := c.state.ExtendCertified(ctx, certifiedBlock.Block, certifiedBlock.QC)
279278
span.End()
280279
if err != nil {
281-
if state.IsOutdatedExtensionError(err) {
282-
continue
283-
}
284280
return fmt.Errorf("could not extend protocol state with certified block: %w", err)
285281
}
286282

state/protocol/badger/mutator.go

Lines changed: 28 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -110,30 +110,23 @@ func NewFullConsensusState(
110110
// candidate.View == certifyingQC.View && candidate.ID() == certifyingQC.BlockID
111111
//
112112
// NOTE: this function expects that `certifyingQC` has been validated.
113-
// Expected errors during normal operations:
114-
// - state.OutdatedExtensionError if the candidate block is outdated (e.g. orphaned)
113+
// No errors are expected during normal operations.
115114
func (m *FollowerState) ExtendCertified(ctx context.Context, candidate *flow.Block, certifyingQC *flow.QuorumCertificate) error {
116115
span, ctx := m.tracer.StartSpanFromContext(ctx, trace.ProtoStateMutatorHeaderExtend)
117116
defer span.End()
118117

119-
// there are no cases where certifyingQC can be nil.
120-
if certifyingQC != nil {
121-
blockID := candidate.ID()
122-
// sanity check if certifyingQC actually certifies candidate block
123-
if certifyingQC.View != candidate.Header.View {
124-
return fmt.Errorf("qc doesn't certify candidate block, expect %d view, got %d", candidate.Header.View, certifyingQC.View)
125-
}
126-
if certifyingQC.BlockID != blockID {
127-
return fmt.Errorf("qc doesn't certify candidate block, expect %x blockID, got %x", blockID, certifyingQC.BlockID)
128-
}
118+
blockID := candidate.ID()
119+
// sanity check if certifyingQC actually certifies candidate block
120+
if certifyingQC.View != candidate.Header.View {
121+
return fmt.Errorf("qc doesn't certify candidate block, expect %d view, got %d", candidate.Header.View, certifyingQC.View)
122+
}
123+
if certifyingQC.BlockID != blockID {
124+
return fmt.Errorf("qc doesn't certify candidate block, expect %x blockID, got %x", blockID, certifyingQC.BlockID)
129125
}
130126

131-
// check if the block header is a valid extension of the finalized state
127+
// check if the block header is a valid extension of parent block
132128
err := m.headerExtend(candidate)
133129
if err != nil {
134-
if state.IsOutdatedExtensionError(err) {
135-
return fmt.Errorf("candidate block is an outdated extension: %w", err)
136-
}
137130
// since we have a QC for this block, it cannot be an invalid extension
138131
return fmt.Errorf("unexpected invalid block (id=%x) with certifying qc (id=%x): %s",
139132
candidate.ID(), certifyingQC.ID(), err.Error())
@@ -163,12 +156,21 @@ func (m *ParticipantState) Extend(ctx context.Context, candidate *flow.Block) er
163156
span, ctx := m.tracer.StartSpanFromContext(ctx, trace.ProtoStateMutatorExtend)
164157
defer span.End()
165158

166-
// check if the block header is a valid extension of the finalized state
159+
// check if the block header is a valid extension of parent block
167160
err := m.headerExtend(candidate)
168161
if err != nil {
169162
return fmt.Errorf("header not compliant with chain state: %w", err)
170163
}
171164

165+
// check if the block header is a valid extension of the finalized state
166+
err = m.checkOutdatedExtension(candidate.Header)
167+
if err != nil {
168+
if state.IsOutdatedExtensionError(err) {
169+
return fmt.Errorf("candidate block is an outdated extension: %w", err)
170+
}
171+
return fmt.Errorf("could not check if block is an outdated extension: %w", err)
172+
}
173+
172174
// check if the guarantees in the payload is a valid extension of the finalized state
173175
err = m.guaranteeExtend(ctx, candidate)
174176
if err != nil {
@@ -199,7 +201,6 @@ func (m *ParticipantState) Extend(ctx context.Context, candidate *flow.Block) er
199201
// headerExtend verifies the validity of the block header (excluding verification of the
200202
// consensus rules). Specifically, we check that the block connects to the last finalized block.
201203
// Expected errors during normal operations:
202-
// - state.OutdatedExtensionError if the candidate block is outdated (e.g. orphaned)
203204
// - state.InvalidExtensionError if the candidate block is invalid
204205
func (m *FollowerState) headerExtend(candidate *flow.Block) error {
205206
// FIRST: We do some initial cheap sanity checks, like checking the payload
@@ -240,13 +241,17 @@ func (m *FollowerState) headerExtend(candidate *flow.Block) error {
240241
return fmt.Errorf("validating block's time stamp failed with unexpected error: %w", err)
241242
}
242243

243-
// THIRD: Once we have established the block is valid within itself, and the
244-
// block is valid in relation to its parent, we can check whether it is
245-
// valid in the context of the entire state. For this, the block needs to
246-
// directly connect, through its ancestors, to the last finalized block.
244+
return nil
245+
}
247246

247+
// checkOutdatedExtension checks whether candidate block is
248+
// valid in the context of the entire state. For this, the block needs to
249+
// directly connect, through its ancestors, to the last finalized block.
250+
// Expected errors during normal operations:
251+
// - state.OutdatedExtensionError if the candidate block is outdated (e.g. orphaned)
252+
func (m *ParticipantState) checkOutdatedExtension(header *flow.Header) error {
248253
var finalizedHeight uint64
249-
err = m.db.View(operation.RetrieveFinalizedHeight(&finalizedHeight))
254+
err := m.db.View(operation.RetrieveFinalizedHeight(&finalizedHeight))
250255
if err != nil {
251256
return fmt.Errorf("could not retrieve finalized height: %w", err)
252257
}
@@ -276,7 +281,6 @@ func (m *FollowerState) headerExtend(candidate *flow.Block) error {
276281
}
277282
ancestorID = ancestor.ParentID
278283
}
279-
280284
return nil
281285
}
282286

state/protocol/badger/mutator_test.go

Lines changed: 36 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1940,16 +1940,17 @@ func TestExtendBlockProcessable(t *testing.T) {
19401940
})
19411941
}
19421942

1943-
func TestHeaderExtendBlockNotConnected(t *testing.T) {
1943+
// TestFollowerHeaderExtendBlockNotConnected tests adding orphan block to the finalized state
1944+
// add 2 blocks, where:
1945+
// first block is added and then finalized;
1946+
// second block is a sibling to the finalized block
1947+
// The Follower should accept this block since tracking of orphan blocks is implemented by another component.
1948+
func TestFollowerHeaderExtendBlockNotConnected(t *testing.T) {
19441949
rootSnapshot := unittest.RootSnapshotFixture(participants)
19451950
util.RunWithFollowerProtocolState(t, rootSnapshot, func(db *badger.DB, state *protocol.FollowerState) {
19461951
head, err := rootSnapshot.Head()
19471952
require.NoError(t, err)
19481953

1949-
// add 2 blocks, where:
1950-
// first block is added and then finalized;
1951-
// second block is a sibling to the finalized block
1952-
// The Follower should reject this block as an outdated chain extension
19531954
block1 := unittest.BlockWithParentFixture(head)
19541955
err = state.ExtendCertified(context.Background(), block1, unittest.CertifyBlock(block1.Header))
19551956
require.NoError(t, err)
@@ -1960,6 +1961,36 @@ func TestHeaderExtendBlockNotConnected(t *testing.T) {
19601961
// create a fork at view/height 1 and try to connect it to root
19611962
block2 := unittest.BlockWithParentFixture(head)
19621963
err = state.ExtendCertified(context.Background(), block2, unittest.CertifyBlock(block2.Header))
1964+
require.NoError(t, err)
1965+
1966+
// verify seal not indexed
1967+
var sealID flow.Identifier
1968+
err = db.View(operation.LookupLatestSealAtBlock(block2.ID(), &sealID))
1969+
require.NoError(t, err)
1970+
})
1971+
}
1972+
1973+
// TestParticipantHeaderExtendBlockNotConnected tests adding orphan block to the finalized state
1974+
// add 2 blocks, where:
1975+
// first block is added and then finalized;
1976+
// second block is a sibling to the finalized block
1977+
// The Participant should reject this block as an outdated chain extension
1978+
func TestParticipantHeaderExtendBlockNotConnected(t *testing.T) {
1979+
rootSnapshot := unittest.RootSnapshotFixture(participants)
1980+
util.RunWithFullProtocolState(t, rootSnapshot, func(db *badger.DB, state *protocol.ParticipantState) {
1981+
head, err := rootSnapshot.Head()
1982+
require.NoError(t, err)
1983+
1984+
block1 := unittest.BlockWithParentFixture(head)
1985+
err = state.Extend(context.Background(), block1)
1986+
require.NoError(t, err)
1987+
1988+
err = state.Finalize(context.Background(), block1.ID())
1989+
require.NoError(t, err)
1990+
1991+
// create a fork at view/height 1 and try to connect it to root
1992+
block2 := unittest.BlockWithParentFixture(head)
1993+
err = state.Extend(context.Background(), block2)
19631994
require.True(t, st.IsOutdatedExtensionError(err), err)
19641995

19651996
// verify seal not indexed

state/protocol/state.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,7 @@ type FollowerState interface {
5151
// has been certified, and it's safe to add it to the protocol state.
5252
// QC cannot be nil and must certify candidate block (candidate.View == qc.View && candidate.BlockID == qc.BlockID)
5353
// The `candidate` block and its QC _must be valid_ (otherwise, the state will be corrupted).
54-
// Expected errors during normal operations:
55-
// * state.OutdatedExtensionError if the candidate block is outdated (e.g. orphaned)
54+
// No errors are expected during normal operations.
5655
ExtendCertified(ctx context.Context, candidate *flow.Block, qc *flow.QuorumCertificate) error
5756

5857
// Finalize finalizes the block with the given hash.

0 commit comments

Comments
 (0)