gschup / bevy_ggrs

Bevy plugin for the GGRS P2P rollback networking library.
Other
294 stars 41 forks source link

Checksums are not portable #98

Closed aleksa2808 closed 6 months ago

aleksa2808 commented 8 months ago

Describe the bug

When running bevy_ggrs on two different architectures (in my case these were 64bit x86 and 32bit WASM) false desyncs can occur because the checksum code is not portable. Several reasons contribute to this:

  1. bevy::utils::FixedState is used for calculating checksums which uses the non-portable aHash crate, meaning that two machines can give two different checksums from the same number. Quoting the aHash crate repo:
    Because it does not have a fixed standard, different computers or computers on different versions of the code will observe different  
    hash values. As such, aHash is not recommended for use other than in-memory maps. Specifically, aHash is not intended for network  
    use or in applications which persist hashed values. (In these cases HighwayHash would be a better choice)
  2. Checksums are sometimes calculated from usize, for example the return of a .len() call. Even with a portable hasher, from the same source usize number you would get different hashes on 32bit and 64bit machines, as this type is 32 bit and 64 bit wide, respectively.

I resolved these issues (or at least up to what my game needed) in a fork branch. Here, I did the following:

  1. Used a forked version of a portable hasher, SeaHash. The only reason for forking was to add panics when trying to hash usize/isize variables, as a prevention measure.
  2. Limited the use of usize/isize types in bevy_ggrs, usually by using or casting to u64.

Expected behavior

I expect the checksums to be the same across architectures. I'm not yet sure what would be the best solution for usize/isize variables.

Desktop (please complete the following information):

johanhelsing commented 8 months ago

Merging #94 first would make fixing this a bit simpler, because then it's just one place to change the hasher.

johanhelsing commented 7 months ago

@aleksa2808 Thanks for the detailed write-up. Do you want to contribute this as a PR (casting to u64 and replacing with SeaHash or another portable hasher)?

aleksa2808 commented 7 months ago

Sure, I can make this a PR. I'm just not sure what to do with SeaHash as I'm currently using my own fork of it where I intentionally panic when trying to hash usize/isize values since this would lead to desyncs on different architectures. I could use the original, non-panicky version, but I feel like users could easily make the mistake of hashing some .len() value for example and in this case I think it is good to error out (or better yet prevent compilation if possible). Do you have any ideas regarding this?

johanhelsing commented 7 months ago

We don't necessarily have to do everything in one go. We could do it like this:

  1. Get rid of usize in internal checksums
  2. Change aHash to SeaHash or something else
  3. Document this gotcha (#35)
aleksa2808 commented 6 months ago

Hey @johanhelsing, sorry for the late reply. I created a PR for this now with both changes included as they are both essential to calculate checksums in a portable manner.

Additionally, regarding the mentioned SeaHash fork, in this PR I used the regular, crates.io provided version. This means that users will still be able to use usize/isize types in their code which would break portability, so it is a good idea to have this mentioned in a gotcha.