Skip to content
New issue

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

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

Already on GitHub? # to your account

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

Open
wants to merge 4 commits into
base: unstable
Choose a base branch
from

Conversation

SunnysidedJ
Copy link

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

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
@@ -1260,6 +1260,7 @@ pub fn serve<T: BeaconChainTypes>(
BroadcastValidation::default(),
duplicate_block_status_code,
network_globals,
false,
Copy link
Member

Choose a reason for hiding this comment

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

I don't really love adding a random bool parameter here. I wonder if a cleaner approach would be to add an rng: Arc<RwLock<Box<dyn RngCore>>> field to BeaconChain which is initialised either as the thread_rng or a seeded RNG depending on config? The BeaconChainHarness could initialise it in seeded mode, while the BeaconChainBuilder used in production could initialise it as thread_rng.

What do you think @SunnysidedJ?

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for making this comment with detailed pointers. I was also wondering a good place to pass RNG instances instead, but I wasn't able to due to my lack of knowledge in the codebase and Rust (I'm a beginner). One thing that I couldn't use was ThreadRng instance for production code because it is a thread-local object which cannot be passed between threads. Would a slight degrading of performance due to the use of OsRng be a concern for you? The logic otherwise seems the same according to its documentation. Nonetheless, I hope the new change is something closer to what you expect!

@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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
das Data Availability Sampling under-review A reviewer has only partially completed a review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants