fix(rando): clamp upgrade levels so a 4th wallet can't corrupt logic - #193
Merged
Conversation
UPG_WALLET is a 2-bit field, but SetUpgrade wrote level << shift unmasked. A 4th Progressive Wallet application wrote 0x4000 - outside the wallet's mask and into UPG_BULLET_BAG - so CurrentUpgrade(UPG_WALLET) read back 0 and the modelled capacity collapsed from 999 to 99. That made logic non-monotonic (more wallets = less reachable), which an assumed fill cannot tolerate: every shuffled shop slot priced above 99 became permanently unreachable, and generation failed validation with items stranded in shops. Masking the write is not enough (4 & 3 == 0); the level itself has to be capped at what the field holds.
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.
Problem
Seed generation failed repeatedly with advancement items stranded in OOT shop slots:
Root cause
UPG_WALLETis a 2-bit field (gUpgradeMasks[4] = 0x3000, max level 3), butLogic::SetUpgradewrotelevel << shiftwith no bounds check, and the progressive-wallet effect increments without a clamp.A 4th
RG_PROGRESSIVE_WALLETapplication therefore wrote4 << 12 = 0x4000— outside the wallet's mask, intoUPG_BULLET_BAG's bits — andCurrentUpgrade(UPG_WALLET)read back 0. Modelled wallet capacity collapsed 999 -> 99.Measured in a failing generation, bracketing one oracle query:
0x1200000x00000x36F4DB0x30000x5745240x4000This makes logic non-monotonic — collecting more wallets makes shop checks less reachable — which an assumed fill cannot tolerate. Every shuffled shop slot priced above 99 became permanently unreachable, so the fill placed items into slots its own validation could never reach.
Why it surfaced now
The bug is old but was inert. Until the price-aware fill/oracle work (
f147be263, 2026-07-13), the oracle never re-established shop prices afterItemReset, soGetCheckPrice()returned 0 and0 <= 99passed for every shop slot regardless of wallet state. Once prices became real, any seed with shopsanity and prices above 99 hit the overflow immediately.Fix
Clamp the level to what the field can hold before shifting. Masking the write alone is not sufficient —
(4 << 12) & 0x3000is still 0, i.e. the same corruption.Follow-ups (not in this PR)
StartingWallet=0and the pool holds 3. The clamp makes it harmless (3 is the correct ceiling), but it is worth tracing.Combo_SOH_Rando_Reset(OTRGlobals.cpp:5096) and again viaResetLogic(applyInventory=true)(fill.cpp:524->:329). Native applies it once, so every starting progressive is double-counted in logic. Likely the source of the 4th wallet, but fixing it changes logic for every seed with a starting inventory, so it deserves its own change.Testing
comborandogenerates cleanly.Build Artifacts