Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
refactor(p2p): minor pre-IPC refactors [part 5/11] #1165
base: refactor/remove-vertex-storage-protocol
Are you sure you want to change the base?
refactor(p2p): minor pre-IPC refactors [part 5/11] #1165
Changes from 1 commit
5f45ef6
b9b84ea
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems better to simply pass an rng to the protocol. Isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RNG should be the same in the
ConnectionsManager
and inHathorProtocol
, right? At least for simulation purposes.If that's the case, we can't simply pass the RNG between them because now the protocol will live in a different process. Otherwise, if we can have different RNGs, then yes, it would be simpler to just have its own RNG here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use a "derived" rng instance, where rng in the main process just generates a seed that is used to instantiate the rng in the subprocess.
There's really only one reason for using an rng instance instead of a global rng, and that is using seeds to reproduce test cases. It's an important for reproducing rare cases that happen randomly in tests.
Considering there's parallelism, some events can happen in different order regardless of the initial seed, and this will affect how we generate random bytes regardless if we query the main thread for random bytes or generate them locally, so in that sense I think generating them locally is better because it avoids an IPC call. And if we really need to we can focus on avoiding that parallelism in tests or simulating it in a reproducible way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove this? It seems unrelated to the PR and it would change the behavior of sync-v2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable is unused in this context. It's just a local variable that we set here, and the method returns just below.
In fact, in the latest commit (b9b84ea) I completely removed as it's just used to toggle a debug message that would never change.