Skip to content

Commit f2e2817

Browse files
julienrbrtjulienrbrt
committed
feedback
1 parent e668b57 commit f2e2817

3 files changed

Lines changed: 32 additions & 50 deletions

File tree

‎block/internal/submitting/submitter.go‎

Lines changed: 12 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -324,20 +324,20 @@ func (s *Submitter) processDAInclusionLoop() {
324324
nextHeight := currentDAIncluded + 1
325325

326326
// Get block data first
327-
header, data, err := s.store.GetBlockData(s.ctx, nextHeight)
327+
_, data, err := s.store.GetBlockData(s.ctx, nextHeight)
328328
if err != nil {
329329
break
330330
}
331331

332332
// Check if this height is DA included
333-
if included, err := s.IsHeightDAIncluded(nextHeight, header, data); err != nil || !included {
333+
if included, err := s.IsHeightDAIncluded(nextHeight, data); err != nil || !included {
334334
break
335335
}
336336

337337
s.logger.Debug().Uint64("height", nextHeight).Msg("advancing DA included height")
338338

339339
// Set node height to DA height mapping using already retrieved data
340-
if err := s.setNodeHeightToDAHeight(s.ctx, nextHeight, header, data, currentDAIncluded == 0); err != nil {
340+
if err := s.setNodeHeightToDAHeight(s.ctx, nextHeight, data, currentDAIncluded == 0); err != nil {
341341
s.logger.Error().Err(err).Uint64("height", nextHeight).Msg("failed to set node height to DA height mapping")
342342
break
343343
}
@@ -428,18 +428,13 @@ func (s *Submitter) sendCriticalError(err error) {
428428

429429
// setNodeHeightToDAHeight persists the DA heights for a block's header and data.
430430
// For empty-tx blocks, both use the header DA height since no data blob is posted.
431-
func (s *Submitter) setNodeHeightToDAHeight(ctx context.Context, height uint64, header *types.SignedHeader, data *types.Data, genesisInclusion bool) error {
432-
headerHash, dataHash := header.Hash(), data.DACommitment()
431+
func (s *Submitter) setNodeHeightToDAHeight(ctx context.Context, height uint64, data *types.Data, genesisInclusion bool) error {
432+
dataHash := data.DACommitment()
433433

434434
headerDaHeightBytes := make([]byte, 8)
435-
// Try real hash first; fall back to height-based lookup for post-restart
436-
// placeholders (before the DA retriever has re-fired the real hash).
437-
daHeightForHeader, ok := s.cache.GetHeaderDAIncluded(headerHash.String())
435+
daHeightForHeader, ok := s.cache.GetHeaderDAIncludedByHeight(height)
438436
if !ok {
439-
daHeightForHeader, ok = s.cache.GetHeaderDAIncludedByHeight(height)
440-
}
441-
if !ok {
442-
return fmt.Errorf("header hash %s not found in cache for height %d", headerHash, height)
437+
return fmt.Errorf("header for height %d not found in cache", height)
443438
}
444439
binary.LittleEndian.PutUint64(headerDaHeightBytes, daHeightForHeader)
445440

@@ -453,12 +448,9 @@ func (s *Submitter) setNodeHeightToDAHeight(ctx context.Context, height uint64,
453448
if bytes.Equal(dataHash, common.DataHashForEmptyTxs) {
454449
binary.LittleEndian.PutUint64(dataDaHeightBytes, daHeightForHeader)
455450
} else {
456-
daHeightForData, ok := s.cache.GetDataDAIncluded(dataHash.String())
457-
if !ok {
458-
daHeightForData, ok = s.cache.GetDataDAIncludedByHeight(height)
459-
}
451+
daHeightForData, ok := s.cache.GetDataDAIncludedByHeight(height)
460452
if !ok {
461-
return fmt.Errorf("data hash %s not found in cache for height %d", dataHash.String(), height)
453+
return fmt.Errorf("data for height %d not found in cache", height)
462454
}
463455
binary.LittleEndian.PutUint64(dataDaHeightBytes, daHeightForData)
464456

@@ -487,7 +479,7 @@ func (s *Submitter) setNodeHeightToDAHeight(ctx context.Context, height uint64,
487479
}
488480

489481
// IsHeightDAIncluded reports whether the block at height has been DA-included.
490-
func (s *Submitter) IsHeightDAIncluded(height uint64, header *types.SignedHeader, data *types.Data) (bool, error) {
482+
func (s *Submitter) IsHeightDAIncluded(height uint64, data *types.Data) (bool, error) {
491483
// Already finalized — cache entries were cleared, but we know it's included.
492484
if height <= s.GetDAIncludedHeight() {
493485
return true, nil
@@ -504,17 +496,8 @@ func (s *Submitter) IsHeightDAIncluded(height uint64, header *types.SignedHeader
504496

505497
dataCommitment := data.DACommitment()
506498

507-
// Try real hash first; fall back to height-based lookup for post-restart
508-
// state before the DA retriever has re-fired the real hashes.
509-
_, headerIncluded := s.cache.GetHeaderDAIncluded(header.Hash().String())
510-
_, dataIncluded := s.cache.GetDataDAIncluded(dataCommitment.String())
511-
512-
if !headerIncluded {
513-
_, headerIncluded = s.cache.GetHeaderDAIncludedByHeight(height)
514-
}
515-
if !dataIncluded {
516-
_, dataIncluded = s.cache.GetDataDAIncludedByHeight(height)
517-
}
499+
_, headerIncluded := s.cache.GetHeaderDAIncludedByHeight(height)
500+
_, dataIncluded := s.cache.GetDataDAIncludedByHeight(height)
518501

519502
dataIncluded = bytes.Equal(dataCommitment, common.DataHashForEmptyTxs) || dataIncluded
520503

‎block/internal/submitting/submitter_test.go‎

Lines changed: 18 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ func TestSubmitter_IsHeightDAIncluded(t *testing.T) {
124124

125125
h1, d1 := newHeaderAndData("chain", 3, true)
126126
h2, d2 := newHeaderAndData("chain", 4, true)
127-
h3, d3 := newHeaderAndData("chain", 2, true) // already DA included, cache was cleared
127+
_, d3 := newHeaderAndData("chain", 2, true) // already DA included, cache was cleared
128128

129129
cm.SetHeaderDAIncluded(h1.Hash().String(), 100, 3)
130130
cm.SetDataDAIncluded(d1.DACommitment().String(), 100, 3)
@@ -134,21 +134,20 @@ func TestSubmitter_IsHeightDAIncluded(t *testing.T) {
134134

135135
specs := map[string]struct {
136136
height uint64
137-
header *types.SignedHeader
138137
data *types.Data
139138
exp bool
140139
expErr bool
141140
}{
142-
"below store height and cached": {height: 3, header: h1, data: d1, exp: true},
143-
"above store height": {height: 6, header: h2, data: d2, exp: false},
144-
"data missing": {height: 4, header: h2, data: d2, exp: false},
145-
"at daIncludedHeight - cache cleared": {height: 2, header: h3, data: d3, exp: true},
146-
"below daIncludedHeight - cache cleared": {height: 1, header: h3, data: d3, exp: true},
141+
"below store height and cached": {height: 3, data: d1, exp: true},
142+
"above store height": {height: 6, data: d2, exp: false},
143+
"data missing": {height: 4, data: d2, exp: false},
144+
"at daIncludedHeight - cache cleared": {height: 2, data: d3, exp: true},
145+
"below daIncludedHeight - cache cleared": {height: 1, data: d3, exp: true},
147146
}
148147

149148
for name, spec := range specs {
150149
t.Run(name, func(t *testing.T) {
151-
included, err := s.IsHeightDAIncluded(spec.height, spec.header, spec.data)
150+
included, err := s.IsHeightDAIncluded(spec.height, spec.data)
152151
if spec.expErr {
153152
require.Error(t, err)
154153
return
@@ -200,7 +199,7 @@ func TestSubmitter_setSequencerHeightToDAHeight(t *testing.T) {
200199
mockStore.On("SetMetadata", mock.Anything, dataKey, dBz).Return(nil).Once()
201200
mockStore.On("SetMetadata", mock.Anything, store.GenesisDAHeightKey, gBz).Return(nil).Once()
202201

203-
require.NoError(t, s.setNodeHeightToDAHeight(ctx, 1, h, d, true))
202+
require.NoError(t, s.setNodeHeightToDAHeight(ctx, 1, d, true))
204203
}
205204

206205
func TestSubmitter_setNodeHeightToDAHeight_Errors(t *testing.T) {
@@ -214,11 +213,11 @@ func TestSubmitter_setNodeHeightToDAHeight_Errors(t *testing.T) {
214213
// No cache entries -> expect error on missing header
215214
_, ok := cm.GetHeaderDAIncluded(h.Hash().String())
216215
assert.False(t, ok)
217-
assert.Error(t, s.setNodeHeightToDAHeight(ctx, 1, h, d, false))
216+
assert.Error(t, s.setNodeHeightToDAHeight(ctx, 1, d, false))
218217

219218
// Add header, missing data
220219
cm.SetHeaderDAIncluded(h.Hash().String(), 10, 1)
221-
assert.Error(t, s.setNodeHeightToDAHeight(ctx, 1, h, d, false))
220+
assert.Error(t, s.setNodeHeightToDAHeight(ctx, 1, d, false))
222221
}
223222

224223
func TestSubmitter_initializeDAIncludedHeight(t *testing.T) {
@@ -649,7 +648,7 @@ func TestSubmitter_IsHeightDAIncluded_AfterRestart(t *testing.T) {
649648
_, realHeaderFound := cm2.GetHeaderDAIncluded(h3.Hash().String())
650649
assert.False(t, realHeaderFound, "real hash must not be present before DA retriever re-fires")
651650

652-
included, err := s.IsHeightDAIncluded(3, h3, d3)
651+
included, err := s.IsHeightDAIncluded(3, d3)
653652
require.NoError(t, err)
654653
assert.True(t, included,
655654
"IsHeightDAIncluded must return true for in-flight height using snapshot placeholder, "+
@@ -662,7 +661,7 @@ func TestSubmitter_IsHeightDAIncluded_AfterRestart(t *testing.T) {
662661
_, realHeaderFound = cm2.GetHeaderDAIncluded(h3.Hash().String())
663662
assert.True(t, realHeaderFound, "real hash must be present after DA retriever re-fires")
664663

665-
included, err = s.IsHeightDAIncluded(3, h3, d3)
664+
included, err = s.IsHeightDAIncluded(3, d3)
666665
require.NoError(t, err)
667666
assert.True(t, included, "IsHeightDAIncluded must still return true after real hash is written")
668667
}
@@ -675,9 +674,9 @@ func TestSubmitter_IsHeightDAIncluded_AfterRestart(t *testing.T) {
675674
// The bug this guards against:
676675
// - Snapshot encodes [blockHeight → daHeight] pairs, not real content hashes.
677676
// - On restart, RestoreFromStore installs placeholder keys (indexed by height).
678-
// - setNodeHeightToDAHeight calls GetHeaderDAIncluded(realHash) which MISSES.
679-
// - Without the height-based fallback it returns an error and processDAInclusionLoop
680-
// logs the error and stalls — the submitter can never write HeightToDAHeight
677+
// - setNodeHeightToDAHeight uses GetHeaderDAIncludedByHeight which resolves
678+
// the placeholder entry directly, so it succeeds before the DA retriever
679+
// re-fires SetHeaderDAIncluded with the real content hash.
681680
// metadata and DAIncludedHeight never advances.
682681
// - With GetHeaderDAIncludedByHeight / GetDataDAIncludedByHeight the lookup
683682
// succeeds via the placeholder and the metadata is written correctly.
@@ -735,9 +734,9 @@ func TestSubmitter_setNodeHeightToDAHeight_AfterRestart(t *testing.T) {
735734
// ── Step 3: call setNodeHeightToDAHeight — must succeed via fallback ──────
736735
// Before this fix, GetHeaderDAIncluded(realHash) would miss and the function
737736
// would return an error, stalling processDAInclusionLoop.
738-
err = s.setNodeHeightToDAHeight(ctx, 3, h3, d3, false)
737+
err = s.setNodeHeightToDAHeight(ctx, 3, d3, false)
739738
require.NoError(t, err,
740-
"setNodeHeightToDAHeight must succeed via height-based fallback before DA retriever re-fires")
739+
"setNodeHeightToDAHeight must succeed via height-based lookup before DA retriever re-fires")
741740

742741
// ── Step 4: verify the HeightToDAHeight metadata was written correctly ─────
743742
headerDABz, err := st1.GetMetadata(ctx, store.GetHeightToDAHeightHeaderKey(3))
@@ -756,6 +755,6 @@ func TestSubmitter_setNodeHeightToDAHeight_AfterRestart(t *testing.T) {
756755
cm2.SetHeaderDAIncluded(h3.Hash().String(), 12, 3)
757756
cm2.SetDataDAIncluded(d3.DACommitment().String(), 12, 3)
758757

759-
err = s.setNodeHeightToDAHeight(ctx, 3, h3, d3, false)
758+
err = s.setNodeHeightToDAHeight(ctx, 3, d3, false)
760759
require.NoError(t, err, "setNodeHeightToDAHeight must still work once real hashes are populated")
761760
}

‎node/single_sequencer_integration_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -206,10 +206,10 @@ func (s *FullNodeTestSuite) TestSubmitBlocksToDA() {
206206
err = waitForAtLeastNDAIncludedHeight(s.node, n)
207207
s.NoError(err, "Failed to get DA inclusion")
208208
for height := uint64(1); height <= n; height++ {
209-
header, data, err := s.node.Store.GetBlockData(s.ctx, height)
209+
_, data, err := s.node.Store.GetBlockData(s.ctx, height)
210210
require.NoError(s.T(), err)
211211

212-
ok, err := castState(s.T(), s.node).bc.Submitter.IsHeightDAIncluded(height, header, data)
212+
ok, err := castState(s.T(), s.node).bc.Submitter.IsHeightDAIncluded(height, data)
213213
require.NoError(s.T(), err)
214214
require.True(s.T(), ok, "Block at height %d is not DA included", height)
215215
}

0 commit comments

Comments
 (0)