Skip to content

Commit a56fa63

Browse files
alpealpe
committed
Review feedback
1 parent 53c991f commit a56fa63

2 files changed

Lines changed: 187 additions & 73 deletions

File tree

‎block/internal/syncing/da_follower_test.go‎

Lines changed: 93 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -96,92 +96,113 @@ func TestDAFollower_HandleEvent(t *testing.T) {
9696
}
9797

9898
func TestDAFollower_HandleCatchup(t *testing.T) {
99-
t.Run("normal_sequential_fetch_success", func(t *testing.T) {
100-
daRetriever := NewMockDARetriever(t)
101-
ctx := t.Context()
99+
type spec struct {
100+
daHeight uint64
101+
initialPriorityHeights []uint64
102+
pipeErr error
103+
setupMock func(m *MockDARetriever)
104+
wantErrIs error
105+
wantPipedHeights []uint64
106+
wantRemainingPriority []uint64
107+
}
108+
109+
newFollower := func(t *testing.T, s spec, m *MockDARetriever) (*daFollower, func() []common.DAHeightEvent) {
110+
t.Helper()
102111

103112
var pipedEvents []common.DAHeightEvent
104113
pipeEvent := func(_ context.Context, ev common.DAHeightEvent) error {
105114
pipedEvents = append(pipedEvents, ev)
106-
return nil
115+
return s.pipeErr
107116
}
108117

109118
follower := &daFollower{
110-
retriever: daRetriever,
119+
retriever: m,
111120
eventSink: common.EventSinkFunc(pipeEvent),
112121
logger: zerolog.Nop(),
113-
priorityHeights: make([]uint64, 0),
114-
}
115-
116-
events := []common.DAHeightEvent{{DaHeight: 100}}
117-
daRetriever.On("RetrieveFromDA", mock.Anything, uint64(100)).Return(events, nil)
118-
119-
err := follower.HandleCatchup(ctx, 100)
120-
require.NoError(t, err)
121-
assert.Len(t, pipedEvents, 1)
122-
})
123-
124-
t.Run("normal_sequential_fetch_blob_not_found_ignored", func(t *testing.T) {
125-
daRetriever := NewMockDARetriever(t)
126-
ctx := t.Context()
127-
128-
follower := &daFollower{
129-
retriever: daRetriever,
130-
eventSink: common.EventSinkFunc(func(_ context.Context, _ common.DAHeightEvent) error { return nil }),
131-
logger: zerolog.Nop(),
132-
priorityHeights: make([]uint64, 0),
133-
}
134-
135-
daRetriever.On("RetrieveFromDA", mock.Anything, uint64(100)).Return(nil, datypes.ErrBlobNotFound)
136-
137-
err := follower.HandleCatchup(ctx, 100)
138-
require.NoError(t, err)
139-
})
140-
141-
t.Run("normal_sequential_fetch_error_propagated", func(t *testing.T) {
142-
daRetriever := NewMockDARetriever(t)
143-
ctx := t.Context()
144-
145-
follower := &daFollower{
146-
retriever: daRetriever,
147-
eventSink: common.EventSinkFunc(func(_ context.Context, _ common.DAHeightEvent) error { return nil }),
148-
logger: zerolog.Nop(),
149-
priorityHeights: make([]uint64, 0),
122+
priorityHeights: append([]uint64(nil), s.initialPriorityHeights...),
150123
}
124+
return follower, func() []common.DAHeightEvent { return pipedEvents }
125+
}
151126

152-
daRetriever.On("RetrieveFromDA", mock.Anything, uint64(100)).Return(nil, datypes.ErrHeightFromFuture)
153-
154-
err := follower.HandleCatchup(ctx, 100)
155-
require.ErrorIs(t, err, datypes.ErrHeightFromFuture)
156-
})
157-
158-
t.Run("priority_queue_handled_first", func(t *testing.T) {
159-
daRetriever := NewMockDARetriever(t)
160-
ctx := t.Context()
127+
specs := map[string]spec{
128+
"seq_ok": {
129+
daHeight: 100,
130+
wantPipedHeights: []uint64{100},
131+
setupMock: func(m *MockDARetriever) {
132+
m.On("RetrieveFromDA", mock.Anything, uint64(100)).
133+
Return([]common.DAHeightEvent{{DaHeight: 100}}, nil).Once()
134+
},
135+
},
136+
"seq_blob_missing": {
137+
daHeight: 100,
138+
setupMock: func(m *MockDARetriever) {
139+
m.On("RetrieveFromDA", mock.Anything, uint64(100)).
140+
Return(nil, datypes.ErrBlobNotFound).Once()
141+
},
142+
},
143+
"seq_err": {
144+
daHeight: 100,
145+
wantErrIs: datypes.ErrHeightFromFuture,
146+
setupMock: func(m *MockDARetriever) {
147+
m.On("RetrieveFromDA", mock.Anything, uint64(100)).
148+
Return(nil, datypes.ErrHeightFromFuture).Once()
149+
},
150+
},
151+
"prio_first": {
152+
daHeight: 100,
153+
initialPriorityHeights: []uint64{105},
154+
wantPipedHeights: []uint64{105, 100},
155+
setupMock: func(m *MockDARetriever) {
156+
m.On("RetrieveFromDA", mock.Anything, uint64(105)).
157+
Return([]common.DAHeightEvent{{DaHeight: 105}}, nil).Once()
158+
m.On("RetrieveFromDA", mock.Anything, uint64(100)).
159+
Return([]common.DAHeightEvent{{DaHeight: 100}}, nil).Once()
160+
},
161+
},
162+
"skip_stale_prio_already_included": {
163+
daHeight: 100,
164+
initialPriorityHeights: []uint64{99},
165+
wantPipedHeights: []uint64{100},
166+
setupMock: func(m *MockDARetriever) {
167+
// stale priority hint (< daHeight) is discarded; only sequential height is fetched
168+
m.On("RetrieveFromDA", mock.Anything, uint64(100)).
169+
Return([]common.DAHeightEvent{{DaHeight: 100}}, nil).Once()
170+
},
171+
},
172+
}
161173

162-
var pipedEvents []common.DAHeightEvent
163-
pipeEvent := func(_ context.Context, ev common.DAHeightEvent) error {
164-
pipedEvents = append(pipedEvents, ev)
165-
return nil
166-
}
174+
for name, s := range specs {
175+
t.Run(name, func(t *testing.T) {
176+
daRetriever := NewMockDARetriever(t)
177+
if s.setupMock != nil {
178+
s.setupMock(daRetriever)
179+
}
167180

168-
follower := &daFollower{
169-
retriever: daRetriever,
170-
eventSink: common.EventSinkFunc(pipeEvent),
171-
logger: zerolog.Nop(),
172-
priorityHeights: []uint64{105}, // Priority height to fetch
173-
}
181+
follower, getPipedEvents := newFollower(t, s, daRetriever)
182+
err := follower.HandleCatchup(t.Context(), s.daHeight)
174183

175-
daRetriever.On("RetrieveFromDA", mock.Anything, uint64(105)).Return([]common.DAHeightEvent{{DaHeight: 105}}, nil).Once()
176-
daRetriever.On("RetrieveFromDA", mock.Anything, uint64(100)).Return([]common.DAHeightEvent{{DaHeight: 100}}, nil).Once()
184+
if s.wantErrIs != nil {
185+
require.ErrorIs(t, err, s.wantErrIs)
186+
} else {
187+
require.NoError(t, err)
188+
}
177189

178-
err := follower.HandleCatchup(ctx, 100)
179-
require.NoError(t, err)
190+
pipedEvents := getPipedEvents()
191+
gotHeights := make([]uint64, 0, len(pipedEvents))
192+
for _, ev := range pipedEvents {
193+
gotHeights = append(gotHeights, ev.DaHeight)
194+
}
195+
wantHeights := s.wantPipedHeights
196+
if wantHeights == nil {
197+
wantHeights = []uint64{}
198+
}
199+
assert.Equal(t, wantHeights, gotHeights)
180200

181-
// It should pipe 105 then 100
182-
assert.Len(t, pipedEvents, 2)
183-
assert.Equal(t, uint64(105), pipedEvents[0].DaHeight)
184-
assert.Equal(t, uint64(100), pipedEvents[1].DaHeight)
185-
assert.Empty(t, follower.priorityHeights)
186-
})
201+
if s.wantRemainingPriority != nil {
202+
assert.Equal(t, s.wantRemainingPriority, follower.priorityHeights)
203+
} else {
204+
assert.Empty(t, follower.priorityHeights)
205+
}
206+
})
207+
}
187208
}

‎block/internal/syncing/syncer_test.go‎

Lines changed: 94 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -899,7 +899,100 @@ func TestProcessHeightEvent_AcceptsValidDAHint(t *testing.T) {
899899
assert.Equal(t, uint64(50), priorityHeight, "valid DA hint should be queued")
900900
}
901901

902-
func TestProcessHeightEvent_SkipsDAHintWhenAlreadyFetched(t *testing.T) {
902+
// TestProcessHeightEvent_SkipsDAHintWhenAlreadyDAIncluded verifies that when the
903+
// DA-inclusion cache already has an entry for the block height carried by a P2P
904+
// event, the DA height hint is NOT queued for priority retrieval.
905+
func TestProcessHeightEvent_SkipsDAHintWhenAlreadyDAIncluded(t *testing.T) {
906+
ds := dssync.MutexWrap(datastore.NewMapDatastore())
907+
st := store.New(ds)
908+
cm, err := cache.NewManager(config.DefaultConfig(), st, zerolog.Nop())
909+
require.NoError(t, err)
910+
911+
addr, _, _ := buildSyncTestSigner(t)
912+
cfg := config.DefaultConfig()
913+
gen := genesis.Genesis{ChainID: "tchain", InitialHeight: 1, StartTime: time.Now().Add(-time.Second), ProposerAddress: addr}
914+
915+
mockExec := testmocks.NewMockExecutor(t)
916+
mockExec.EXPECT().InitChain(mock.Anything, mock.Anything, uint64(1), "tchain").Return([]byte("app0"), nil).Once()
917+
918+
mockDAClient := testmocks.NewMockClient(t)
919+
920+
s := NewSyncer(
921+
st,
922+
mockExec,
923+
mockDAClient,
924+
cm,
925+
common.NopMetrics(),
926+
cfg,
927+
gen,
928+
extmocks.NewMockStore[*types.P2PSignedHeader](t),
929+
extmocks.NewMockStore[*types.P2PData](t),
930+
zerolog.Nop(),
931+
common.DefaultBlockOptions(),
932+
make(chan error, 1),
933+
nil,
934+
)
935+
require.NoError(t, s.initializeState())
936+
s.ctx = context.Background()
937+
s.daRetriever = NewDARetriever(nil, cm, gen, zerolog.Nop())
938+
s.daFollower = NewDAFollower(DAFollowerConfig{
939+
Retriever: s.daRetriever,
940+
Logger: zerolog.Nop(),
941+
EventSink: common.EventSinkFunc(func(_ context.Context, _ common.DAHeightEvent) error { return nil }),
942+
Namespace: []byte("ns"),
943+
StartDAHeight: 0,
944+
})
945+
946+
// Set the store height to 1 so the event at height 2 is "next".
947+
batch, err := st.NewBatch(context.Background())
948+
require.NoError(t, err)
949+
require.NoError(t, batch.SetHeight(1))
950+
require.NoError(t, batch.Commit())
951+
952+
// Simulate already DA-included header and data at height 2.
953+
cm.SetHeaderDAIncluded("somehash-hdr2", 42, 2)
954+
cm.SetDataDAIncluded("somehash-data2", 42, 2)
955+
956+
// Both hints point to DA height 100. They should be skipped due to cache hits.
957+
evt := common.DAHeightEvent{
958+
Header: &types.SignedHeader{Header: types.Header{BaseHeader: types.BaseHeader{ChainID: gen.ChainID, Height: 2}}},
959+
Data: &types.Data{Metadata: &types.Metadata{ChainID: gen.ChainID, Height: 2}},
960+
Source: common.SourceP2P,
961+
DaHeightHints: [2]uint64{100, 100},
962+
}
963+
s.processHeightEvent(t.Context(), &evt)
964+
965+
priorityHeight := s.daFollower.(*daFollower).popPriorityHeight()
966+
assert.Equal(t, uint64(0), priorityHeight,
967+
"DA hint must not be queued when header and data are already DA-included in cache")
968+
969+
// Partial case: only header is DA-included at height 3, data is not.
970+
cm2, err := cache.NewManager(config.DefaultConfig(), st, zerolog.Nop())
971+
require.NoError(t, err)
972+
s.cache = cm2
973+
cm2.SetHeaderDAIncluded("somehash-hdr3", 55, 3)
974+
975+
batch, err = st.NewBatch(context.Background())
976+
require.NoError(t, err)
977+
require.NoError(t, batch.SetHeight(2))
978+
require.NoError(t, batch.Commit())
979+
980+
evt3 := common.DAHeightEvent{
981+
Header: &types.SignedHeader{Header: types.Header{BaseHeader: types.BaseHeader{ChainID: gen.ChainID, Height: 3}}},
982+
Data: &types.Data{Metadata: &types.Metadata{ChainID: gen.ChainID, Height: 3}, Txs: types.Txs{types.Tx("tx2")}},
983+
Source: common.SourceP2P,
984+
DaHeightHints: [2]uint64{150, 151},
985+
}
986+
s.processHeightEvent(t.Context(), &evt3)
987+
988+
priorityHeight = s.daFollower.(*daFollower).popPriorityHeight()
989+
assert.Equal(t, uint64(151), priorityHeight,
990+
"data hint must be queued when only the header is already DA-included")
991+
assert.Equal(t, uint64(0), s.daFollower.(*daFollower).popPriorityHeight(),
992+
"no further hints should be queued")
993+
}
994+
995+
func TestProcessHeightEvent_SkipsDAHintWhenBelowRetrieverCursor(t *testing.T) {
903996
ds := dssync.MutexWrap(datastore.NewMapDatastore())
904997
st := store.New(ds)
905998
cm, err := cache.NewManager(config.DefaultConfig(), st, zerolog.Nop())

0 commit comments

Comments
 (0)