[Merged by Bors] - Use a fixed state hasher in bevy_reflect for deterministic Reflect::reflect_hash() across processes #7583
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alice-i-cecile
added
C-Bug
|
It's hard to imagine a case where we wouldn't want this. Why should this be a feature, rather than always enabled? |
Yeah, I guess the possibility of DOS attacks through bevy_reflect are pretty far fetched. I guess if anything it should probably an opt-in feature to make it runtime-seeded. Hard-coding to always use fixed state |
johanhelsing
changed the title
alice-i-cecile
approved these changes
Feb 10, 2023
MrGVSV
reviewed
Feb 13, 2023
crates/bevy_reflect/src/reflect.rs
Outdated
| use bevy_utils::FixedState; | ||
|
|
||
| #[inline] | ||
| pub fn reflect_hasher() -> bevy_utils::AHasher { |
There was a problem hiding this comment.
Nit: This should probably have a doc stating what it does, how/where it's used, and when/why to use it.
Conflicts: - crates/bevy_reflect/src/list.rs
johanhelsing
force-pushed
the
fixed-reflect-hash
branch
from
2f08657
to
9ce4f42
Compare
johanhelsing
requested review from
alice-i-cecile and
MrGVSV
and removed request for
alice-i-cecile and
MrGVSV
|
Sorry for the noise, still figuring out how the "reviewers" system on github works |
alice-i-cecile
approved these changes
Feb 17, 2023
james7132
approved these changes
Feb 17, 2023
james7132
added
the
S-Ready-For-Final-Review
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Feb 17, 2023
…eflect_hash() across processes (#7583) # Objective - bevy_ggrs uses `reflect_hash` in order to produce checksums for its world snapshots. These checksums are sent between clients in order to detect desyncronization. - However, since we currently use `async::AHasher` with the `std` feature, this means that hashes will always be different for different peers, even if the state is identical. - This means bevy_ggrs needs a way to get a deterministic (fixed) hash. ## Solution - ~~Add a feature to use `bevy_utils::FixedState` for the hasher used by bevy_reflect.~~ - Always use `bevy_utils::FixedState` for initializing the bevy_reflect hasher. --- ## Changelog - bevy_reflect now uses a fixed state for its hasher, which means the output of `Reflect::reflect_hash` is now deterministic across processes.
|
Build failed (retrying...): |
bors bot
pushed a commit
that referenced
this pull request
Feb 17, 2023
…eflect_hash() across processes (#7583) # Objective - bevy_ggrs uses `reflect_hash` in order to produce checksums for its world snapshots. These checksums are sent between clients in order to detect desyncronization. - However, since we currently use `async::AHasher` with the `std` feature, this means that hashes will always be different for different peers, even if the state is identical. - This means bevy_ggrs needs a way to get a deterministic (fixed) hash. ## Solution - ~~Add a feature to use `bevy_utils::FixedState` for the hasher used by bevy_reflect.~~ - Always use `bevy_utils::FixedState` for initializing the bevy_reflect hasher. --- ## Changelog - bevy_reflect now uses a fixed state for its hasher, which means the output of `Reflect::reflect_hash` is now deterministic across processes.
bors
bot
changed the title
myreprise1
pushed a commit
to myreprise1/bevy
that referenced
this pull request
Feb 18, 2023
…eflect_hash() across processes (bevyengine#7583) - bevy_ggrs uses `reflect_hash` in order to produce checksums for its world snapshots. These checksums are sent between clients in order to detect desyncronization. - However, since we currently use `async::AHasher` with the `std` feature, this means that hashes will always be different for different peers, even if the state is identical. - This means bevy_ggrs needs a way to get a deterministic (fixed) hash. - ~~Add a feature to use `bevy_utils::FixedState` for the hasher used by bevy_reflect.~~ - Always use `bevy_utils::FixedState` for initializing the bevy_reflect hasher. --- - bevy_reflect now uses a fixed state for its hasher, which means the output of `Reflect::reflect_hash` is now deterministic across processes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Objective
reflect_hashin order to produce checksums for its world snapshots. These checksums are sent between clients in order to detect desyncronization.async::AHasherwith thestdfeature, this means that hashes will always be different for different peers, even if the state is identical.Solution
Add a feature to usebevy_utils::FixedStatefor the hasher used by bevy_reflect.bevy_utils::FixedStatefor initializing the bevy_reflect hasher.Changelog
Reflect::reflect_hashis now deterministic across processes.