Skip to content

Conversation

@eserilev
Copy link
Member

@eserilev eserilev commented Apr 15, 2025

Issue Addressed

Closes #6855

Proposed Changes

Add PeerDAS broadcast validation tests and fix a small bug where sampling_columns_indices is none (indicating that we've already sampled the necessary columns) and process_gossip_data_columns gets called

Additional Info

If theres a better way to format these tests (maybe have CI feed in a list of forks to the relevant tests?) I can do that instead

@eserilev eserilev added das Data Availability Sampling fulu Required for the upcoming Fulu hard fork test improvement Improve tests ready-for-review The code is ready for review labels Apr 15, 2025
@michaelsproul michaelsproul 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 Apr 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. labels May 14, 2025
@mergify
Copy link

mergify bot commented May 15, 2025

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

@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 15, 2025
@mergify
Copy link

mergify bot commented May 20, 2025

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

@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. labels May 20, 2025
Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, i didn't realise the tests can be reused without needing any modification 😄 I think we could probably remove test_spec() in the tests that uses InteractiveTester::<E>::new(None,... as it already defaults to test_spec.

@jimmygchen jimmygchen 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 Jun 27, 2025
@jimmygchen jimmygchen assigned jimmygchen and unassigned jimmygchen Jul 11, 2025
Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jimmygchen jimmygchen dismissed michaelsproul’s stale review July 17, 2025 06:54

Suggestions addressed.

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jul 17, 2025
mergify bot added a commit that referenced this pull request Jul 17, 2025
@mergify mergify bot merged commit d6de8a7 into sigp:unstable Jul 17, 2025
34 checks passed
danielrachi1 pushed a commit to danielrachi1/lighthouse that referenced this pull request Jul 18, 2025
Closes sigp#6855


  Add PeerDAS broadcast validation tests and fix a small bug where `sampling_columns_indices` is none (indicating that we've already sampled the necessary columns) and `process_gossip_data_columns` gets called
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

das Data Availability Sampling fulu Required for the upcoming Fulu hard fork ready-for-merge This PR is ready to merge. test improvement Improve tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants