feat: Add `DeleteRange` method to Store #347

julienrbrt · 2025-09-09T09:38:20Z

Overview

d
Implement DeleteRange on store.
This methods truncates the head to the given height. (different from DeleteTo that truncates the tail to the given height)

Wondertan · 2025-09-09T09:49:26Z

One thing we should explore is making DeleteRange [from:to) instead of introducing a new method. It's gonna have better ergonomics, allow both usecase, and have less duplicated code and easier to maintain longer term.

We actually wanted to make it DeleteRange from the start, but then we selfishly moved towards DeleteTo because deleting from Tail is all we needed. This is the exact case where we should have gone for a more general solution it turns out.

Wondertan

First pass.

I still need to sit down and verify new logic of DeleteRange

store/store_delete.go

store/store_test.go

headertest/store.go

Wondertan · 2025-09-10T15:23:46Z

store/store_delete.go

+	deleteCtx := ctx
+	if deadline, ok := ctx.Deadline(); ok {
+		// allocate 95% of caller's set deadline for deletion
+		// and give leftover to save progress
+		sub := deadline.Sub(startTime) / 100 * 95
+		var cancel context.CancelFunc
+		deleteCtx, cancel = context.WithDeadlineCause(ctx, startTime.Add(sub), errDeleteTimeout)
+		defer cancel()
+	}
+
+	if to-from < deleteRangeParallelThreshold {
+		height, missing, err = s.deleteSequential(deleteCtx, from, to)
+	} else {
+		height, missing, err = s.deleteParallel(deleteCtx, from, to)
+	}
+
+	return err


Previously, updateTail was in the defer of the same function, so even if an error occurred, the partial progress was saved. This is no longer true as the call deleteRangeRaw short-circuits on error.

should we return from this method the actual values that were successfully deleted to?

julienrbrt · 2025-09-11T10:10:21Z

Thanks! Implementing your suggestions...

Closes: #2631 Blocked on celestiaorg/go-header#347

This PR was blocked but got merged: #2638. It isn't bad to merge it, but we are relying on a fork of go-header for the apps until celestiaorg/go-header#347 is merged. Let's delete it in the main go.mod

julienrbrt · 2025-09-22T21:36:52Z

Any update on this @Wondertan?

Wondertan · 2025-09-23T13:52:08Z

We qim to get it done by this week

renaynay · 2025-10-01T11:35:42Z

headertest/store.go

-	for h := m.TailHeight; h < to; h++ {
+func (m *Store[H]) DeleteRange(ctx context.Context, from, to uint64) error {
+	// Delete headers in the range [from:to)
+	for h := from; h < to; h++ {


let's just do quick sanity even in testing pkg to make sure from < to

renaynay · 2025-10-01T11:49:01Z

headertest/store.go

+	}
+
+	// Update HeadHeight if we deleted from the end
+	if to >= m.HeadHeight {


fail out here if to > m.HeadHeight (we should catch this err case in tests)

Or actually do we have to allow this condition as we allow deletion --> beyond store head (to sync head) ?

renaynay · 2025-10-01T11:51:19Z

interface.go


-	// DeleteTo deletes the range [Tail():to).
-	DeleteTo(ctx context.Context, to uint64) error
+	// DeleteRange deletes the range [from:to).


Suggested change

// DeleteRange deletes the range [from:to).

// DeleteRange deletes the range [from:to). It disallows the creation of gaps in the implementation's chain, ensuring contiguity between Tail --> Head.

renaynay · 2025-10-01T13:53:04Z

store/store_delete.go

+
+	// if range is empty within the current store bounds, it's a no-op
+	if from > head.Height() || to <= tail.Height() {
+		return nil


should we warn log here as passing a no-op range feels like fishy behaviour from the caller.

renaynay · 2025-10-01T14:02:04Z

store/store_delete.go

+
+	// Check if we're deleting all existing headers (making store empty)
+	// Only wipe if 'to' is exactly at head+1 (normal case) to avoid accidental wipes
+	if from <= tail.Height() && to == head.Height()+1 {


do we allow a case where caller specifies a from that is < tail.Height() ? I don't see why we shouldn't but just want to make sure this isn't some weirdness we should silently overlook.

renaynay · 2025-10-01T14:13:38Z

store/store_delete.go

+	}
+
+	// Update tail if we deleted from the beginning
+	if updateTail {


what if we wiped store (where to == head.Height+1) ? do we still need to updateTail then? won't it err out trying to get new tail getByHeight ?

renaynay · 2025-10-01T14:32:27Z

store/store_delete.go

+					"expected_to_height", to,
+					"actual_to_height", height,
+					"hdrs_not_found", missing,
+					"took(s)", time.Since(startTime),


Suggested change

"took(s)", time.Since(startTime),

"took(s)", time.Since(startTime).Seconds(),

renaynay · 2025-10-01T14:32:36Z

store/store_delete.go

+					"expected_to_height", to,
+					"actual_to_height", height,
+					"hdrs_not_found", missing,
+					"took(s)", time.Since(startTime),


Suggested change

"took(s)", time.Since(startTime),

"took(s)", time.Since(startTime).Seconds(),

renaynay · 2025-10-01T14:33:37Z

store/store_delete.go

+	deleteCtx := ctx
+	if deadline, ok := ctx.Deadline(); ok {
+		// allocate 95% of caller's set deadline for deletion
+		// and give leftover to save progress
+		sub := deadline.Sub(startTime) / 100 * 95
+		var cancel context.CancelFunc
+		deleteCtx, cancel = context.WithDeadlineCause(ctx, startTime.Add(sub), errDeleteTimeout)
+		defer cancel()
+	}
+
+	if to-from < deleteRangeParallelThreshold {
+		height, missing, err = s.deleteSequential(deleteCtx, from, to)
+	} else {
+		height, missing, err = s.deleteParallel(deleteCtx, from, to)
+	}
+
+	return err


should we return from this method the actual values that were successfully deleted to?

renaynay · 2025-10-01T14:37:42Z

store/store_test.go

 	assert.False(t, has)
 }
+
+func TestStore_DeleteRange(t *testing.T) {


can we please add test cases for partial deletes here (in case of caller ctx timeout or errors) that the store recovers properly?

feat: Add DeleteFromHead method to Store

2c37b35

refactor

4c3fc7c

julienrbrt changed the title ~~feat: Add DeleteFromHead method to Store~~ feat: Add DeleteRange method to Store Sep 9, 2025

add tests

41f7446

julienrbrt marked this pull request as ready for review September 9, 2025 13:14

julienrbrt requested review from tzdybal, renaynay, Wondertan, vgonkivs, gupadhyaya and walldiss as code owners September 9, 2025 13:14

cleanup comments

cb11b09

This was referenced Sep 9, 2025

refactor: bump go-header and use DeleteRange method for rollback evstack/ev-node#2638

Merged

refactor(server): bump go-header and use DeleteRange api evstack/ev-abci#244

Merged

Wondertan reviewed Sep 10, 2025

View reviewed changes

store/store_delete.go Outdated Show resolved Hide resolved

store/store_delete.go Outdated Show resolved Hide resolved

store/store_test.go Show resolved Hide resolved

headertest/store.go Show resolved Hide resolved

Wondertan reviewed Sep 10, 2025

View reviewed changes

feedback

a4b1924

julienrbrt mentioned this pull request Sep 12, 2025

[Feature Request]: Do we need to support rollback? evstack/ev-abci#250

Closed

julienrbrt requested a review from Wondertan September 17, 2025 15:51

Merge branch 'main' into julien/deletefromhead

642fed7

github-merge-queue bot pushed a commit to evstack/ev-node that referenced this pull request Sep 17, 2025

refactor: bump go-header and use DeleteRange method for rollback (#2638)

ec0a74a

Closes: #2631 Blocked on celestiaorg/go-header#347

julienrbrt mentioned this pull request Sep 17, 2025

build(deps): remove unecessary replace main go.mod evstack/ev-node#2675

Merged

renaynay reviewed Oct 1, 2025

View reviewed changes

	// DeleteRange deletes the range [from:to).
	// DeleteRange deletes the range [from:to). It disallows the creation of gaps in the implementation's chain, ensuring contiguity between Tail --> Head.

	"took(s)", time.Since(startTime),
	"took(s)", time.Since(startTime).Seconds(),

feat: Add DeleteRange method to Store #347

Are you sure you want to change the base?

feat: Add DeleteRange method to Store #347

Conversation

julienrbrt commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Uh oh!

Wondertan commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Wondertan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julienrbrt commented Sep 11, 2025

Uh oh!

julienrbrt commented Sep 22, 2025

Uh oh!

Wondertan commented Sep 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

feat: Add `DeleteRange` method to Store #347

feat: Add `DeleteRange` method to Store #347

julienrbrt commented Sep 9, 2025 •

edited

Loading

Wondertan commented Sep 9, 2025 •

edited

Loading