Skip to content

Fix for #6296: Deterministic RNG in peer DAS publish block tests #7192

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 11 commits into from
Apr 9, 2025
Merged

Fix for #6296: Deterministic RNG in peer DAS publish block tests #7192

merged 11 commits into from
Apr 9, 2025

Conversation

SunnysidedJ
Copy link
Contributor

Issue Addressed

#6296: Deterministic RNG in peer DAS publish block tests

Proposed Changes

Made test functions to call publish-block APIs with true for the deterministic RNG boolean parameter while production code with false. This will deterministically shuffle columns for unit tests under broadcast_validation_tests.rs.

Additional Info

  1. Tested with "cargo test --workspace --release --features "$(TEST_FEATURES)" --exclude ef_tests --exclude beacon_chain --exclude slasher --exclude network --exclude web3signer_tests" which is same as "make test" but excluding web3signer_tests. The suggestion from the Lighthouse Book, update JDK/JRE to the latest (17 or above and I used 24), did not work. Please let me know if this is okay.
  2. Tests related to the issue (bn_http_api_tests) was disabled by default. I manually enabled to run tests calling related API changes. See beacon_node/http_api/tests/main.rs. Many functions here result in stack overflow for some reason on my local machine.
  3. Please clarify intention and scope of changes. I merely took the word-by-word on the issue Deterministic RNG in peer DAS publish block tests #6296 by changing existing test code to take a fixed seed. Do you want to make it more customisable or use different design? Also, it'd be appreciated to know the intention of changing it to something deterministic. How does it affect testability, and what kinds of testing can we do further?

@CLAassistant
Copy link

CLAassistant commented Mar 21, 2025 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@jimmygchen jimmygchen added ready-for-review The code is ready for review das Data Availability Sampling labels Mar 24, 2025
michaelsproul
michaelsproul reviewed Mar 25, 2025
View reviewed changes
@michaelsproul michaelsproul added under-review A reviewer has only partially completed a review. and removed ready-for-review The code is ready for review labels Mar 25, 2025
@SunnysidedJ SunnysidedJ requested a review from jxs as a code owner March 25, 2025 20:30
@jimmygchen jimmygchen added ready-for-review The code is ready for review and removed under-review A reviewer has only partially completed a review. labels Apr 1, 2025
jimmygchen
jimmygchen reviewed Apr 1, 2025
View reviewed changes
jimmygchen
jimmygchen reviewed Apr 1, 2025
View reviewed changes
@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 Apr 1, 2025
jimmygchen
jimmygchen requested changes Apr 1, 2025
View reviewed changes
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.

@SunnysidedJ thanks for the PR, the changes looks good. I've added some comments above. Let me know what you think :)

@jimmygchen jimmygchen added the test improvement Improve tests label Apr 1, 2025
@SunnysidedJ SunnysidedJ requested a review from jimmygchen April 2, 2025 19:37
jimmygchen
jimmygchen approved these changes Apr 3, 2025
View reviewed changes
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.

Looks good, thanks @SunnysidedJ 🙏

jimmygchen
jimmygchen reviewed Apr 4, 2025
View reviewed changes
beacon_node/beacon_chain/src/builder.rs Outdated
@@ -31,6 +31,7 @@ use logging::crit;
use operation_pool::{OperationPool, PersistedOperationPool};
use parking_lot::{Mutex, RwLock};
use proto_array::{DisallowedReOrgOffsets, ReOrgThreshold};
use rand::rngs::StdRng;
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be in the test mod below.

jimmygchen
jimmygchen requested changes Apr 4, 2025
View reviewed changes
Copy link
Member

@jimmygchen jimmygchen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please see comment above

dapplion
dapplion reviewed Apr 7, 2025
View reviewed changes
@mergify Mergify
Copy link

mergify bot commented Apr 7, 2025

This pull request has merge conflicts. Could you please resolve them @SunnysidedJ? 🙏

@mergify Mergify
Copy link

mergify bot commented Apr 8, 2025

This pull request has merge conflicts. Could you please resolve them @SunnysidedJ? 🙏

@jimmygchen jimmygchen removed the waiting-on-author The reviewer has suggested changes and awaits thier implementation. label Apr 9, 2025
jimmygchen
jimmygchen approved these changes Apr 9, 2025
View reviewed changes
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 - CI job is failing due to a Kurtosis issue unrelated to this PR, will merge when its fixed. Thanks @SunnysidedJ !

@jimmygchen jimmygchen added the ready-for-merge This PR is ready to merge. label Apr 9, 2025
@jimmygchen jimmygchen mentioned this pull request Apr 9, 2025
Closed
@mergify mergify bot merged commit d96b731 into sigp:unstable Apr 9, 2025
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dapplion dapplion dapplion left review comments

@jimmygchen jimmygchen jimmygchen approved these changes

@jxs jxs Awaiting requested review from jxs jxs is a code owner

@michaelsproul michaelsproul Awaiting requested review from michaelsproul

Assignees
No one assigned
Labels
das Data Availability Sampling ready-for-merge This PR is ready to merge. test improvement Improve tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@SunnysidedJ @CLAassistant @jimmygchen @michaelsproul @dapplion