beyond-all-reason / beyond-all-reason/RecoilEngine
Network protocol: redundant playerID fields
- Dominant language
- C++
- Stars
- 679
- Forks
- 290
- Avg merge
- 3d 2h
- Merged PRs (30d)
- 40
Description
Multiple message types in the network protocol require a client to put his playerID in the message contents even though the sender is already known by the server. The message then gets dropped if it doesn't match.
https://github.com/beyond-all-reason/spring/blob/d1abbf526819b8834abab24ddad13166bd7fc498/rts/Net/GameServer.cpp#L1186-L1188
It's ancient and seems to have been there in the first commit already so there is little clue on why it was added:
https://github.com/beyond-all-reason/spring/blob/d1811b38413e5e98daff05e1eba3b4b1660e38f5/rts/GameServer.cpp#L265-L266
It doesn't give any extra spoofing protection compared to just using the value it's already being checked against. Maybe at the time it was possible to connect in slot A but have the client think it's in slot B? But this doesn't happen anymore, and I guess you would just play as slot A and the client would be none the wiser if that happened. So it looks redundant.
Many messages have this field so perhaps there could be some savings in replay size (and network usage, but that is peanuts already), plus simplification is always elegant. Some data from real replays (especially non-ZK/BAR games because those two send an absolute shitton of lua traffic and that gets saved in the replay too) to measure the gains would be nice.
A big reason not to change the protocol would be replay parsers. Would be good to check what parsers exist and whether they handle messages with a mismatch correctly.
Contributor guide
Research direction
Start at rts/Net/GameServer.cpp lines 1186-1188 and compare the historical rts/GameServer.cpp lines 265-266 to trace how playerID fields are validated. Inspect the affected message types, replay-parser compatibility, and replay data before deciding whether the fields can be removed; done means a scoped protocol change with measured impact and compatibility accounted for.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- networking
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 35/100