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.

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

fix(rand): remove timestamp randomness #257

Merged
merged 22 commits into from
Sep 3, 2024
Merged

fix(rand): remove timestamp randomness #257

merged 22 commits into from
Sep 3, 2024

Conversation

0xNineteen
Copy link
Contributor

@0xNineteen 0xNineteen commented Aug 28, 2024 •
edited
Loading

  • changed the imports across various files to match the current approach (sig.core.account; not @import)
  • remove excess path naming of tests (test "bloom.bloom: ..." -> test "...")
  • move getWallclockMs into sig.time (doesnt make sense to be in gossip)
  • add seed parameter to buildMessages which is used for all random operations (prev seed was wallclock)
  • add seed parameter to PullRequestTask (instead of using wallclock)
  • add now parameter to buildPullRequests for more deterministic results (prev was reading wallclock)
  • modifications to test "test build pull requests" to be a more deterministic test

note: the last two changes were aimed to make test "test build pull requests" more deterministic and fix #157 -- ive re-run the tests many times and it seems to be fixed

  • remove unnecessary log lines in gossip
  • add additional context to readme.md

@0xNineteen 0xNineteen marked this pull request as ready for review August 28, 2024 20:57
@0xNineteen 0xNineteen self-assigned this Aug 28, 2024
@InKryption InKryption self-requested a review August 28, 2024 21:09
yewman
yewman reviewed Aug 29, 2024
View reviewed changes
Copy link
Contributor

@yewman yewman left a comment

Choose a reason for hiding this comment

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

Couple of minor suggestions on test names otherwise looks good :)

src/gossip/active_set.zig Outdated Show resolved Hide resolved
src/gossip/data.zig Outdated Show resolved Hide resolved
src/gossip/data.zig Outdated Show resolved Hide resolved
src/gossip/data.zig Outdated Show resolved Hide resolved
src/gossip/data.zig Outdated Show resolved Hide resolved
src/gossip/data.zig Outdated Show resolved Hide resolved
src/gossip/service.zig Outdated Show resolved Hide resolved
src/gossip/shards.zig Outdated Show resolved Hide resolved
src/net/net.zig Outdated Show resolved Hide resolved
dnut
dnut reviewed Aug 30, 2024
View reviewed changes
Copy link
Contributor

@dnut dnut left a comment

Choose a reason for hiding this comment

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

Overall looks good. I just have one concern about determinism in gossip.

I also left some comments with my thoughts about imports. Nothing all that important though.

src/accountsdb/accounts_file.zig Show resolved Hide resolved
src/accountsdb/index.zig Show resolved Hide resolved
src/gossip/fuzz_table.zig Outdated Show resolved Hide resolved
src/gossip/pull_request.zig Show resolved Hide resolved
src/gossip/service.zig Outdated Show resolved Hide resolved
@InKryption InKryption removed their request for review August 30, 2024 15:48
0xNineteen and others added 22 commits September 3, 2024 12:22
@0xNineteen
move time import and fix imports on multiple gossip files
8e80faa
@0xNineteen
more import renames
b53e8da
@0xNineteen
fixes
deb9d2b
@0xNineteen
change back ts seed in tests
e2342cb
@0xNineteen
revert test change
99623d2
@0xNineteen
rever test change
d342efc
@0xNineteen
remove unneeded log lines
6b7e7d7
@0xNineteen
add comment on readme about main entrypoints
24eb1e4
@0xNineteen
add docker install link
4c1f57b
@0xNineteen
small fix
48e8d9f
@0xNineteen
fix lint
06b4f32
@0xNineteen @yewman
Update src/gossip/active_set.zig
91aec75 
Co-authored-by: Harold <[email protected]>
@0xNineteen @yewman
Update src/gossip/data.zig
415ca75 
Co-authored-by: Harold <[email protected]>
@0xNineteen @yewman
Update src/gossip/data.zig
9887ae6 
Co-authored-by: Harold <[email protected]>
@0xNineteen @yewman
Update src/gossip/data.zig
09e3f9d 
Co-authored-by: Harold <[email protected]>
@0xNineteen @yewman
Update src/gossip/data.zig
4b45c0b 
Co-authored-by: Harold <[email protected]>
@0xNineteen @yewman
Update src/gossip/data.zig
0a6241c 
Co-authored-by: Harold <[email protected]>
@0xNineteen @yewman
Update src/gossip/service.zig
c6b9b42 
Co-authored-by: Harold <[email protected]>
@0xNineteen @yewman
Update src/gossip/shards.zig
20bcd90 
Co-authored-by: Harold <[email protected]>
@0xNineteen @yewman
Update src/net/net.zig
3b368d7 
Co-authored-by: Harold <[email protected]>
@0xNineteen
add back std spacing
c6524e4
@0xNineteen
change pull request seeding approach
49eaff0
@0xNineteen 0xNineteen merged commit 93d2eb8 into main Sep 3, 2024
6 checks passed
@0xNineteen 0xNineteen deleted the 19/stable-rand branch September 3, 2024 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yewman yewman yewman left review comments

@dnut dnut dnut approved these changes

Assignees

@0xNineteen 0xNineteen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

gossip: Random test failure in gossip test "test filtering values works"
3 participants
@0xNineteen @dnut @yewman