Skip to content

Conversation

SunnysidedJ
Copy link
Contributor

@SunnysidedJ SunnysidedJ commented Mar 31, 2025

Issue Addressed

#6853 Update store tests to cover data column pruning

Proposed Changes

Created a helper function check_data_column_existence which is a copy of check_blob_existence but checking data columns instead.
The helper function is then used to check whether data columns are also pruned when blobs are pruned if PeerDAS is enabled.

Additional Info

There were three bugs discovered when writing these tests

  1. Data column info was not updated post fulu
  2. Blob info was continuing to be updated post fulu
  3. delete_if was broken for leveldb

For bug 3, we now seek to the correct column before trying to delete values. The prune_blobs_across_fork_boundary test provides test coverage for this change. When there are both blobs and data columns in the blob store, delete_if properly prunes blobs (or data columns). Without seek blobs would never be pruned as soon as the first data column was inserted into the blob store

@jimmygchen
Copy link
Member

Thanks for the PR @SunnysidedJ !

Please see my comments above and let me know if it's unclear or if you have any questions!

@jimmygchen jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. das Data Availability Sampling labels Apr 1, 2025
@SunnysidedJ
Copy link
Contributor Author

Major changes:

  • Separated tests for data column pruning.
  • Blob pruning tests only run on Deneb while column ones only run on Fulu with PeerDAS enabled.
  • Found that fork_boundary test is not working (see inline comments).

@SunnysidedJ SunnysidedJ requested a review from jimmygchen April 2, 2025 19:47
@SunnysidedJ
Copy link
Contributor Author

When I enable Fulu and try blob pruning, the blobs aren't pruned correctly. It'd be appreciated if you could have a look on the commit that reproduces the problem: SunnysidedJ@6217cf9

@mergify
Copy link

mergify bot commented May 19, 2025

Some required checks have failed. Could you please take a look @SunnysidedJ? 🙏

@mergify
Copy link

mergify bot commented May 19, 2025

Hi @SunnysidedJ, this pull request has been closed automatically due to 30 days of inactivity. If you’d like to continue working on it, feel free to reopen at any time.

@mergify mergify bot closed this May 19, 2025
@mergify mergify bot added the stale Stale PRs that have been inactive and is now outdated label May 19, 2025
@eserilev
Copy link
Member

I'm going to take a look at the fulu blob pruning test issues

@eserilev eserilev reopened this May 22, 2025
@eserilev eserilev added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. stale Stale PRs that have been inactive and is now outdated labels May 22, 2025
@eserilev
Copy link
Member

The test is now passing. The reason it was failing was two fold:

  • we werent advancing the slot clock before extending the chain
  • we tried to fork from deneb directly to fulu

I've added an intermediary step to fork to electra and run some checks there. Not sure if theres any additional checks we want to make in this test.

@mergify
Copy link

mergify bot commented May 22, 2025

Some required checks have failed. Could you please take a look @SunnysidedJ? 🙏

@mergify mergify bot added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels May 22, 2025
@eserilev
Copy link
Member

ah still need to fix one other test, working on that now

mergify bot added a commit that referenced this pull request Oct 16, 2025
mergify bot added a commit that referenced this pull request Oct 16, 2025
@mergify
Copy link

mergify bot commented Oct 16, 2025

This pull request has been removed from the queue for the following reason: pull request dequeued.

Pull request #7228 has been dequeued. The pull request could not be merged. This could be related to an activated branch protection or ruleset rule that prevents us from merging. (details: 1 review requesting changes and 1 approving review by reviewers with write access.).

You can check the last failing draft PR here: #8209.

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.
If you do update this pull request, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

@mergify mergify bot removed the queued label Oct 16, 2025
@mergify mergify bot added the queued label Oct 16, 2025
mergify bot added a commit that referenced this pull request Oct 16, 2025
mergify bot added a commit that referenced this pull request Oct 16, 2025
@mergify
Copy link

mergify bot commented Oct 16, 2025

This pull request has been removed from the queue for the following reason: checks timeout.

The merge conditions cannot be satisfied due to checks timeout:

You can check the last failing draft PR here: #8212.

You may have to fix your CI before adding the pull request to the queue again.

If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

@mergify mergify bot removed the queued label Oct 16, 2025
@jimmygchen
Copy link
Member

@mergify requeue

@mergify
Copy link

mergify bot commented Oct 16, 2025

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

@mergify mergify bot added the queued label Oct 16, 2025
mergify bot added a commit that referenced this pull request Oct 16, 2025
@mergify
Copy link

mergify bot commented Oct 16, 2025

This pull request has been removed from the queue for the following reason: pull request dequeued.

Pull request #7228 has been dequeued. The pull request could not be merged. This could be related to an activated branch protection or ruleset rule that prevents us from merging. (details: 1 review requesting changes and 1 approving review by reviewers with write access.).

You can check the last failing draft PR here: #8220.

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.
If you do update this pull request, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

@mergify mergify bot removed the queued label Oct 16, 2025
@jimmygchen jimmygchen dismissed their stale review October 16, 2025 13:52

outdated and been rereviewed

@mergify mergify bot added the queued label Oct 16, 2025
mergify bot added a commit that referenced this pull request Oct 16, 2025
mergify bot added a commit that referenced this pull request Oct 16, 2025
@mergify mergify bot merged commit d1e06dc into sigp:unstable Oct 16, 2025
37 of 38 checks passed
@mergify mergify bot removed the queued label Oct 16, 2025
jchavarri pushed a commit to jchavarri/lighthouse that referenced this pull request Oct 21, 2025
sigp#6853 Update store tests to cover data column pruning


  Created a helper function `check_data_column_existence` which is a copy of `check_blob_existence` but checking data columns instead.
The helper function is then used to check whether data columns are also pruned when blobs are pruned if PeerDAS is enabled.


Co-Authored-By: SunnysidedJ <j@testinprod.io>

Co-Authored-By: Eitan Seri-Levi <eserilev@ucsc.edu>

Co-Authored-By: Michael Sproul <michael@sigmaprime.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

das Data Availability Sampling ready-for-merge This PR is ready to merge. v8.0.0 Q4 2025 Fusaka Mainnet Release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants