Isolate RNG in PowerNet battery energy distribution (correct branch) - #987
Open
GetParanoid wants to merge 1 commit into
Open
Isolate RNG in PowerNet battery energy distribution (correct branch)#987GetParanoid wants to merge 1 commit into
GetParanoid wants to merge 1 commit into
Conversation
This file contains hidden or 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
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.
Originally merged into the wrong branch (#986), my apologies.
Problem
PowerNet.DistributeEnergyAmongBatteriesshuffles the net's battery list (batteriesShuffled.Shuffle()), consuming calls from the map's shared RNG.Whether it runs on a given tick is gated by a float comparison in
PowerNet.ChangeStoredEnergy(extra > 0f), fed byCurrentEnergyGainRate()/CurrentStoredEnergy()summing floats across every power component and battery on the net. Those sums drift slightly between machines, so the comparison can flip a tick earlier on one peer than another: one peer runs the shuffle on a tick where the other doesn't,Rand.iterationsends the tick at a different value on each, and the map desyncs withWrong random state on map 0.Evidence
From a 2-player session (MP 0.11.5, RimWorld 1.6.4871, no async time, single map, no mods patching
PowerNet): the client's and host's recordedRandstack traces match call for call — same ticks, same entities, same states — through tick 6781265, then diverge.Client — stays on tick 6781265 and makes 7 RNG calls the host never makes, all from the same stack:
Host — makes none of those calls, advancing straight from tick 6781265 to 6781266 (an unrelated
ChildcareUtility.ShouldWakeUpToAutofeedUrgentcheck).The same signature recurred 20+ times across the session, typically every few thousand ticks, and reappeared after each automatic rejoin.
DistributeEnergyAmongBatteriesshows up as a real stack frame in the traces, so it isn't being inlined.Happy to attach the full desync logs if that's useful.
Fix
Push/pop the RNG state around the method while in MP, matching the approach in #968.
This targets exactly what the desync check compares.
AsyncTimeComp.TickrecordsrandStateonce per map tick in itsfinally, andTryAddMapRandomStatestores(uint)(state >> 32)— the high half ofRand.StateCompressed, i.e.Rand.iterations. SoClientSyncOpinion.CheckForDesync's map comparison is effectively "did both peers make the same number of RNG calls this tick?"Rand.PushState()saves(seed, iterations)andPopState()restores both, so whatever the shuffle consumes is discarded before the tick boundary is recorded. A peer that runs the method and a peer that doesn't record the identical value.Why isolating the RNG is safe here
The shuffled order cannot affect the outcome, so discarding the RNG it draws changes nothing:
AmountCanAcceptacross the list, or an even share of the energy remaining. Never a per-position amount.AmountCanAcceptdepends only on the battery itself, never on the others, so no battery's share is influenced by what was processed before it.amountCanAccept <= 0f || amountCanAccept == num2), so the same batteries drop out of the list under any ordering.energyis decremented by the same value the same number of times, so the float arithmetic is order-independent too.Every permutation therefore leaves the batteries holding identical energy.
Two further notes:
PushState()with no seed continues from the current state rather than reseeding, so in-sync peers still produce the same shuffle as each other. Behaviour is unchanged beyond the isolation.Multiplayer.Client == null.Scope
PowerNet.PowerNetTickhas two other float-gated RNG consumers —partsWantingPowerOn.RandomElement()andpotentialShutdownParts.RandomElement()— reachable depending on hownum2 + num >= -1E-07flands. Both are left alone: their results decide which building powers on or shuts down, so isolating that RNG would make peers pick different buildings and diverge for real. The battery shuffle is the one consumer in this path whose result provably doesn't affect the outcome, which is what makes it safe to isolate. The other two also sit behind tick-interval and non-empty-list conditions, so they fire far more rarely than the shuffle, which runs whenever a net has surplus energy.This doesn't eliminate the underlying float drift — that's inherent to float math across machines, and the desync check doesn't look at it. It removes the drift's ability to become a random-state mismatch.
Testing
dev, no new warnings.DistributeEnergyAmongBatteriesno longer draws from the shared RNG, took a 2-player colony from desyncing and rejoining every few thousand ticks to roughly 3 hours with no desync at all.