Skip to content
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.

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

improvement(gossip): replace all uses of std.time.milliTimestamp as a PRNG seed with a std.Random interface parameter #182

Merged

Conversation

aang114
Copy link
Contributor

@aang114 aang114 commented Jun 25, 2024

For all the functions that are directly generating random numbers using a PRNG (where the seed used is std.time.milliTimestamp), a std.Random interface parameter has now been added which generates the random numbers instead. This is done to decouple the actual PRNG implementation and its seed from the function.

This decoupling is useful during testing since we can easily find out the specific PRNG seed used when the function fails.

The functions that were modified are:

  • Bloom.random()
  • GossipPullFilterSet.consumeForGossipPullFilters()
  • GossipService.buildPullRequests()
  • NodeInstance.init()
  • ActiveSet.rotate()

Note: This PR attempts to resolve issue #176.

Update(27 Jun 2024): Based on @InKryption's review, a std.Random interface parameter has also been added to the following functions:

  • ContactInfo.toNodeInstance()
  • GossipPullFilterSet.initTest()
  • GossipService.rotateActiveSet()

Update 2(27 Jun 2024): Based on @InKryption's second review, a std.Random interface parameter has also been added to the following functions:

  • buildGossipPullFilters()
  • GossipPullFilterSet.init()

@InKryption InKryption self-requested a review June 25, 2024 14:23
Copy link
Contributor

@InKryption InKryption 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 a good start, but there appears to be a misunderstanding about the intention of the original logging code, and a few places where the change could, and must eventually go further.

The superfluous logs should be removed. As for going further with the hoisting of the PRNG implementation, efforts would be appreciated, but are not vital & can be done as part of future work.

src/bloom/bloom.zig Outdated Show resolved Hide resolved
src/gossip/active_set.zig Outdated Show resolved Hide resolved
src/gossip/active_set.zig Outdated Show resolved Hide resolved
src/gossip/data.zig Outdated Show resolved Hide resolved
src/gossip/pull_request.zig Outdated Show resolved Hide resolved
src/gossip/pull_request.zig Outdated Show resolved Hide resolved
src/gossip/service.zig Outdated Show resolved Hide resolved
src/gossip/service.zig Outdated Show resolved Hide resolved
@aang114 aang114 requested a review from InKryption June 27, 2024 11:47
Copy link
Contributor

@InKryption InKryption left a comment

Choose a reason for hiding this comment

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

Looking better, a few more amendments are in order and then this should be good to go.

src/bloom/bloom.zig Outdated Show resolved Hide resolved
src/gossip/active_set.zig Outdated Show resolved Hide resolved
src/gossip/fuzz.zig Outdated Show resolved Hide resolved
src/gossip/fuzz.zig Outdated Show resolved Hide resolved
src/gossip/fuzz.zig Outdated Show resolved Hide resolved
src/gossip/pull_request.zig Outdated Show resolved Hide resolved
src/gossip/pull_request.zig Outdated Show resolved Hide resolved
@InKryption InKryption merged commit d8889bc into Syndica:main Jun 27, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants