Rated game variable not set correctly, use it for Rated game Cheat-prevention

[elexis]

Whether or not the current game is considered a ranked game is not updated in the correct places, resulting in some rated games being considered not rated and some non-rated games being considered rated.

Code: The variable indicating if the current game is rated is g_rankedGame in JSInterface_Lobby.cpp.

It will be set true whenever one joins the lobby (JSI_Lobby::StartXmppClient) and it will be set from the gamesetup only (JSI_Lobby::SetRankedGame which is only called from gamesetup.js whenever that checkbox changes).

However that's not sufficient to set the variable correctly.

Reproduce [making a non-rated game rated (tested)]:

1. Start a non-rated lobby game with late-observer-joins enabled
2. Join the lobby with another account
3. Join that running game
4. The developer overlay can be opened for the host, but not for the rejoined client.

Reproduce [making a rated game non-rated (not tested)]:

1. Join the lobby
2. Join a rated game, let the game start
3. Leave the game after it started
4. Join a non-rated game that has not started yet and leave it
5. Rejoin the rated game
6. Should be able to open the dev overlay

Since the gamesetup part is skipped when rejoining, SetRankedGame will never be set correctly. (Edit: That shouldn't happen

See also #3547

[elexis]

broken code rant:
What mostly stuck in memory was that there are two variables that store whether the game is rated and are set from different places with different logic, when they ought to have the same value at all times. That's basically a ​https://en.wikipedia.org/wiki/Single_source_of_truth violation.

While writing ​Phab:D2483: SetRankedGame is basically only necessary to disable the wireframe hotkey in rated games (the rest can directly access g_GameAttributes.settings.RatingEnabled and g_IsReplay to fix the anti-pattern. I discovered it's broken because SetRankedGame is only called from the gamesetup, but not for rejoining players. So if someone hosts with ratedgame=false and the other client rejoins, the wireframe hotkey will be disabled instead of enabled and the SetRankedGame c++ value will be set to true (default) when it should be false.

Then I realized I wrote precisely that 4 years ago here already and it didn't fix itself since, welcome to 2020 with the bugs from 2013- (though one can say we at least got some new bugs too):

resulting in some rated games being considered not rated and some non-rated games being considered rated.

.

The developer overlay can be opened for the host, but not for the rejoined client.

That bug was fixed in r19558 coincidentally.

beta keyword rant:
the beta keyword seems logically redundant with ">= should have", but it's tagged completely differently so what's the point of the keyword? That a ticket with the beta keyword is something that should be done for beta but should not necessarily be done the alpha stage? (Then why is it on "should-have" priority?) Or that a should-have ticket without a beta keyword should be done in the final release but not necessarily in the alpha or beta releases? Then it would be a matter of urgency, i.e. the same as the priority. So the only difference that I can see is addition of noise and using the word beta. The urgency of the bug should be indicated by the priority, otherwise there is no point in having the priority. beta as a milestone would also seem odd.)

[elexis]

Whether to delete g_rankedGame or to use it for cheat-prevention:

The g_rankedGame could be deleted and the only consumer CGameView::HandleEvent could obtain it from CSimulation2::GetInitAttributes, but it seems cleaner to have the JS side disable or enable the hotkey on session init. That the variable is stored (copied) to C++ doesn't add any cheat-prevention because the JS side can still enable and disable that variable arbitrarily (i.e. some hacker JS mod can do that without the need to recompile).

The wireframe option seems better implemented as a DeveloperOverlay dropdown that is also triggered by the hotkey (if rated games are disabled in g_GameAttributes in JS session GUI context).

Those 3 calls

g_Renderer.SetTerrainRenderMode(...); g_Renderer.SetWaterRenderMode(...); g_Renderer.SetModelRenderMode(...);

are also incorrectly located, CGameViewCGameView instead of JSI_Renderer, the hotkey should not be present in C++ (no need to).

Compare this with the other developer overlay options which entirely disregard the C++ g_isRankedGame variable. (Why the overlays use GuiInterfaceCall+ICmp instead of JSInterface GUI entrance is also dubious. If that's the preferred means then JSInterface_Simulation.cpp functions such as JSI_Simulation::SetBoundingBoxDebugOverlay are wrong.)

If one actually was to intend to harden it against JS hacker mods for rated games, then the g_rankedGame variable could be kept but should not be set from JS but from C++.

That would be from the data in CNetClient::OnGameStart which passes it to client->m_Game->StartGame(&client->m_GameAttributes, "");.

The disadvantage of doing so would be that its parsing of simulation data (that isnt really simulation data since the RatingEnabled boolean doesn't influence simulation state) in the NetClient (that doesn't really operate with the simulation) for the purpose of XmppClient logic setting (which doesn't really have to do anything with simulation nor netclient). As the code stands every commit did something unique for no reason.

So if g_XmppClient was not deleted but hardened against cheating by setting it in CNetClient::OnGameStart or whereever, then one could perhaps also make it impossible to change the perspective to "Observer", "Gaia" or the opponent and whatever else could be used for cheating. That would be JSI_Game::SetViewedPlayer having an early return for g_rankedGame.

As we have so many hundred thousands of rated games where people cheated exploited or otherwise broke the concept and fair sportsmanship of rated games, I suppose I can't delete this g_rankedGame variable but have to rewrire and harden that in anticipation of people just jumping on the next best exploit after fixing the current most popular. (Where I is a variable like X)

So I can fix "Rated game variable not set correctly" easily but "Use rated game variable correctly" is also missing and needs more code, so perhaps the ticket needs to be backlogged again.

[elexis]

It's even worse that this variable g_rankedGame is set to true for rejoining players in non-rated games, because it means that a rated-game report will be sent to the lobby bot for non-rated games.

[elexis]

In 23329:

Remove JSInterface_Lobby IsRankedGame function duplicitous with g_GameAttributes.settings.RatingEnabled && !g_IsReplay, refs #3550, D2483.