fix: stale auth state poisoning#618
Conversation
c9de48e to
f7031d9
Compare
elementbound
left a comment
There was a problem hiding this comment.
Added some suggestions, excellent stuff otherwise!
f7031d9 to
abe463a
Compare
|
Please don't merge it yet. I found a possible misbehavior caused by this change. I'd like to verify it is safe first :) |
177bed1 to
43cbf42
Compare
43cbf42 to
511dbdb
Compare
UpdateThe first iteration of the fix introduced another regression. Position jittering due to continuous "fighting" of authoritative states and predicted local simulation. The main reason is that clients received incomplete snapshots and tried to extrapolate player position from it:
I tried to plug them all but I don't think it's the right solution anymore. I initially wanted to avoid wasting more memory by a band-aid solution of truncating history and letting local resim reconstruct from there. It doesn't work well. Proper solutionRemember the snapshot that was sent to each peer and calculate diffs against that instead of self-owned state. How much more memory and CPU will this need? YES! There is probably room for optimization there but I think this solution closes a LOT of awkward edge cases instead of trying to work around them. For example, I think it fixes another issue I noticed but haven't attempted to debug yet: under heavy latency (tested via Clumsy, starting at around 250ms TTR), a peer sometimes completely hangs for a fraction of a second. My suspicion is that diff calculation flakes out and doesn't send anything so we're waiting for the next full state. |
79ec9d2 to
ef932ae
Compare
|
@aromancev Thinking about the approach. I'm not against merging this solution, since you can't get more correct than tracking what has been sent and diffing against that. Unfortunately, whatever we do, it will be inherently "lossy", since we send our state updates as unreliable. So even if we remember what we've sent and diff against that, there's a chance that the target never received that snapshot we've just saved as reference. Would it improve things if we sent empty diff states instead? So in case nothing has changed, the clients don't treat it as "no idea", but as "received an actual state from the server". The other idea I had was to treat all of our sent snapshots as a continuous stream per peer, and only remember the last sent snapshot. This way we don't need to keep a full history, just a single snapshot per peer, which we diff against before sending the next one, and then remember that. Would that be a realistic compromise? |
|
I think making things as correct as theoretically possible and then trying to optimize is a rational approach, which is why I started with uncovering all the edge cases where rollback shows unexpected behavior. I think this is the last one. As you noted, you can't get more correct than that. And very true that we're building on an inherently unreliable communication channel because 100% correctness costs too much in responsiveness. That said, there is a big difference between 1% loss and 100% loss. Just because something is not possible to achieve at 100% doesn't mean it's not worth trying to get as close as possible. The main question is what is the price, and the honest answer is I don't know yet :) But I'm already working towards CPU optimizations. I don't think sending empty states or only keeping the last sent snapshot can ensure the same level of correctness. The main problem is as follows:
I outlined a specific example is this comment: #631 (comment) As always, the probability of those mispredictions is proportional to the frequency and complexity of interactions. For some games correctness has a higher payoff than for others. If we're not making it the default behavior, I think this "as correct as possible" mode should be available as an opt-in. I would also propose to measure the actual price we're paying for correctness. Maybe it's not that much compared to where Netfox already is :) |
Let me clarify, historical state would be sent the same! The difference is that we don't keep track of all the states we sent, only the latest one. So instead of:
We'd do:
The difference is in 4., where we diff against @3 instead of @1. This difference allows us to keep only one snapshot per peer, while still sending all ticks, so we get historical state data on peers. |
|
Ah, sorry. Maybe I misunderstood then. So in this scenario:
"On next loop" means after resimulation from tick @1? So essentially, we're sending a state for tick @2 diffed against state @3. That means state values from resimulated @2 that happen to have the same values as the old @3 will not be sent even though they may have changed after the resimulation from the old @2. For example,
It's a bit hard to speculate about those things so please double-check my logic. I'm sure it's not the only edge case though where this will happen. To be honest, I would probably look for corners to cut elsewhere, this corner is too sharp. For example, we could see if we can store diffs per visibility group instead of per player. Players that have the same visibility filters will have the same diff. In my game all players see everything anyway. Also important to understand if we want to optimize memory or CPU. I'm assuming storing field values per player per tick is still much cheaper than what textures, models, and animations take. CPU is another story, but there are also other ways we could optimize. |
I think this would also be solved by sending empty diffs. In general, I think sending empty diffs is better than not sending them, since that way it would be unambiguous whether we received state that was identical, or if the packet just didn't make it. In general, I'm now realizing that diff states make more sense when not in a rollback context. The reason I'm thinking about this is that I'm a bit wary of how involved the diff logic is becoming 😅 But the reason probably is that most games do diff states in a non-rollback context, where the reference is obvious, the diff is against the last sent snapshot, which is also the last simulated snapshot. With rollback, this becomes much less obvious. The reference data that the diff was made against could be the last sent snapshot, the last sent version of this current snapshot, or even the chronologically previous snapshot. So now this becomes a choice with each option having their own drawbacks. Anyway, I'm open to merging this in its current form. We'll just need to make sure to properly free peer-related data when a peer disconnects. I think the main breaking point could be having many rollbacked nodes, each with their own state, since now we duplicate that for each peer. So memory multiplies with peers and nodes. |
Currently, a stale auth state can poison history preventing local corrections when diff states are enabled.
Failure mode:
This change truncates all local history on receiving auth state from the past. Tested with out of order delivery and did not discover any new failure modes.
This is just one edge case with diff + auth state history. It is probably less risky and easier to maintain if it's truncated to avoid playing whack-a-mole with more edge cases.