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

Organize gossip random packet generation #4880

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alexpyattaev
Copy link

@alexpyattaev alexpyattaev commented Feb 8, 2025

Problem

Gossip's random packet generation was never designed to be reproducible, and did not cover all variants of all packets. As a result, it is really hard to make reproducible pseudorandom gossip packets for purposes such as wire format validation and to track down possible incompatibilities between implemenations.

Also all of that code was not guarded by dev-context-only-utils feature-gate, making it accidentally usable in prod code.

Summary of Changes

All of the changes here concern only the code that runs in tests and benches.

  • Added testing_fixtures.rs to organize the test-related code better
  • Added FormatValidation trait to organize the new_rand functions for later use in wire format example generation
  • Extended the new_rand implementations where appropriate to cover all variants of all messages

This intended to be the first step to address #4879

@alexpyattaev alexpyattaev force-pushed the gossip_random_packets branch 2 times, most recently from 7f7a270 to cd7b6bf Compare February 8, 2025 22:56
@alexpyattaev alexpyattaev force-pushed the gossip_random_packets branch from cd7b6bf to 761c43a Compare February 8, 2025 23:16
@alexpyattaev alexpyattaev marked this pull request as ready for review February 9, 2025 16:38
Copy link

@behzadnouri behzadnouri left a comment

Choose a reason for hiding this comment

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

Lets hold on here.
We need to discuss what is the end-goal, and what are the alternatives.
If the end goal is to verify wire formats there are better and more accurate ways to do it.

The new_rand methods were not meant to create any realistic values, and are not suitable for this purpose.
Also not clear what is the purpose of FormatValidation trait and why do we need that, and imposing that on CrdsValue would be a maintenance burden.

@alexpyattaev alexpyattaev marked this pull request as draft February 10, 2025 16:22
@behzadnouri
Copy link

To provide transparency to other folks, this was discussed over zoom/slack.
Some notes are added on the github tracking issue: #4879

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.

2 participants