feat: Add DeleteRange method to Store

[julienrbrt]

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]

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

[julienrbrt]

Thanks! Implementing your suggestions...

[julienrbrt]

Any update on this @Wondertan?

[Wondertan]

We qim to get it done by this week

[tac0turtle]

gentle ping on this

[Wondertan]

@tac0turtle, hey, apologies for the delay here. The merge is blocked mainly by me. I am botted with fibre da and can't find time to sit down and do another proper pass here, but we will get to it, I promise.

[Wondertan]

Finally got to review the PR. These are the final comments from the in-depth review:

• The partial update comment was not fixed. Currently, if you interrupt DeleteRange, it won't save the progress. Funnily, you added tests that assert progress is not saved, but that's not what the interface contract expects.
• DeleteRange allows deleting the current head and silently clamps this. This is a footgun
• The logic that wipes the store only checks pending cache, while it should check all caches, including disk.
• There is a little bug with boundaries in headertest.Store mock.
• nit: the DeleteRange should be at the top of the file as its a public method and that's how DeleteTo was
• needs rebase

[Wondertan]

@tac0turtle, @julienrbrt, I understand that coming back to this after a while might be hard, and context switching here is relatively expensive, so I made this PR to your PR that addresses the comments above. This should accelerate the lingering completion of this PR (my fault) and meet maintenance-quality expectations.

[renaynay]

Pls take a look at julienrbrt#3 @julienrbrt

[Wondertan]

weird, linter complains after merging main in, but looking at main branches ci - linter is doing fine

[julienrbrt]

yeah! this PR hasn't even touched those files

[Wondertan]

This sometimes happens in node's repo too, its likely linter misfunctioning and they only solution we found was actually fixing the complaints. Can you do this for the last time hopefully?

[julienrbrt]

This sometimes happens in node's repo too, its likely linter misfunctioning and they only solution we found was actually fixing the complaints. Can you do this for the last time hopefully?

golangci-lint run ./... --fix --config .golangci.yml does not fix the issues, nor find them. I've bumped the action instead. Let's see.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 58.08081% with 83 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.51%. Comparing base (aa5c4a6) to head (a751546).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
store/store_delete.go	60.46%	57 Missing and 11 partials ⚠️
headertest/store.go	0.00%	15 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #347      +/-   ##
==========================================
+ Coverage   52.99%   54.51%   +1.52%     
==========================================
  Files          41       41              
  Lines        4663     4760      +97     
==========================================
+ Hits         2471     2595     +124     
+ Misses       2007     1969      -38     
- Partials      185      196      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Wondertan]

Failed again : (
@julienrbrt, could it be that your local version of golangci-lint is outdated?

[julienrbrt]

Failed again : ( @julienrbrt, could it be that your local version of golangci-lint is outdated?

looks like it was. Just installed from source (2.8), fedora repo had 2.7 only.

[Wondertan]

Nice, CI passed. Congrats!