yairm210 / Unciv

Open-source Android/Desktop remake of Civ V
Mozilla Public License 2.0
8.3k stars 1.56k forks source link

Multiplayer: tapping multiplayer notif undoes current turn's actions #10410

Open Cwpute opened 10 months ago

Cwpute commented 10 months ago

Platform Smartphone, Nokia TA-1157 Android 11

Version F-droid v.4.8.14, build 924

Describe the bug In a multiplayer game, when it's your turn to play switching apps triggers the notification saying "It's your turn !". If you tap it, you're sent back to Unciv with this current turn's actions undone: the server essentially did a rollback (or wasn't sent the info at all, didn't save any). This can be exploited to explore unknown tiles and get better rewards in ruins (maybe more exploits).

To Reproduce Steps to reproduce the behavior:

  1. Start an online multiplayer game
  2. Wait for your turn
  3. Do some actions
  4. Switch apps
  5. Tap the Unciv notification
  6. Previous actions are rolled back

Expected behavior This shouldn't be possible, to prevent exploits. Game state should always be saved, probably after each action, to prevent this happening.

Screenshots https://github.com/yairm210/Unciv/assets/80604512/9c77777e-28d6-4b25-9b54-c55b0efb6428

Cwpute commented 10 months ago

Probably related to:

tuvus commented 10 months ago

You could always exploit unknown tiles. There is almost no way we can fix this reasonably. It is annoying, however, how clicking the notification reloads the game though and makes you loose all of the progress on the turn. If you go out of the game and go back in without clicking the notification the game will still be as normal. The game does not send the half-moved turn to the server, it will only send the fully-moved turn.

Cwpute commented 10 months ago

Can't you forcibly save every action done during your turn, when playing a multiplayer game ? these actions would the be sent in bulk after the turn was ended.

tuvus commented 10 months ago

We would have to send every move to the server to verify it, or else they could change it in a different way. This would take more server processing power and it would prevent some extra features like being able to make your move offline and sending the saved game once online. At some point, we just have to trust our players.

Cwpute commented 10 months ago

I'm not asking to send every action one by one to the server, but rather save every action on the device, one after the other, then send the final sate of the map once the player ended their turn. Would saving every action on the device complexify server-side verification and synchronization at the end of the turn ?

Sending unfinished turn would indeed increase the load on the servers and create so much more problems with synchronization…

tuvus commented 10 months ago

If we save it to the device, the user can modify the turns anyway. So all we are doing is adding more complexity for a tiny bit of anti-cheating. I'm not saying that this idea doesn't work, I am just saying that it probably won't be done.

Cwpute commented 10 months ago

Well then if it's not for anti-cheat, let's do it for convenience and consistence with asynchronous multiplayer advantages. As you said yourself:

It is annoying, however, how clicking the notification reloads the game though and makes you loose all of the progress on the turn.

This is especially true for longer turns in the end game when you need to micro-manage your units and take multiple variables into consideration before acting. A turn sometimes requires i spend more than 10 minutes pondering, now imagine losing those 10+ minutes. "Annoying" is a euphemism here. Consider this: you're communicating with other players through any other app on your phone (be it SMS, Discord, Signal etc…). If you want to send messages, to them, you'd need to exit the app first, and risk tapping the notification thus erasing all previous chosen actions.

The main reason i'm so much interested in asynchronous multiplayer, as provided by Unciv, is that i can pick the moment at which i want to play my turn. I don't have big moments of calm and concentration, so this is quite important to me that these 20 minutes i can focus myself on Unciv are well spent. This bug i reported can greatly hurt this essential feature, not allowing me to pause while choosing my actions, or consult either other players for communication, or other apps on my phone.

tuvus commented 10 months ago

Yes, as I said above, I agree. There is probably a much simpler solution to this problem than saving each move individually. If the application is still running we need to refocus the game without loading the multiplayer game from if we are already in a game. We only lose the game state when we load the multiplayer game from the server after pressing the notification.

SomeTroglodyte commented 10 months ago

Definitely related - as in exactly the same thing - to #9544, but no correlation to #10373.

There's two scopes here: Multiplayer distributed transactional integrity - which as you can see from the wording choice is a non-trivial task, quite a lot of giant libraries/modules/services have been written for the concept. I agree some trust in players is key. Also, if some actual reliable (true, exact, faithful - what's the word here?) replay-supporting transaction logging existed, if made dev-readable, could help a lot in AI code maintenance and optimization. But I don't see it as realistically doable.

The other is the state loss on external purely-UI influences, which plainly is a major bug, only acceptable because nobody really resizes during play. I got a branch here for that purpose for ages. The answer is simply - all those external signals triggering an internal reload must no longer do so, UI element rearrangement must be done on the fly without that reload. It is done in a way out of laziness - Gdx Layout is often finnicky, and rebuilding often behaves more reliably than rearranging. And the build part needs to be written anyway... And for WorldScreen it's hard because so much old code interacts in not really homogenous ways, as much of that interaction comes from different devs, or ages of Unciv.

Again: The UI part of the problem must be solved anyway and has nothing to do with multiplayer. I say "must" - well - as I said it's rare enough.

yairm210 commented 10 months ago

I think that as long as we trust players to behave normally, we can solve this by treating already-loaded MP games as we do non-MP games, by which I mean:

Are there exploits here that bad actors can use to cheat on MP? Sure there are, but if I have to weigh user convenience vs cheaters, convenience wins. It's not like there are financial transactions at stake, it's a game, so maximizing fun takes priority

yairm210 commented 10 months ago

That is to say: We are privileged that we can forgo distributed transactional integrity because we just don't really care that much. As long as it's my turn, accept that my version of my turn is valid, and that's it.

github-actions[bot] commented 5 months ago

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 15 days.

SeventhM commented 5 months ago

Not giving my opinion on whether or not to close this, but a sidebar

I think that as long as we trust players to behave normally

Coming from the community of Polytopia (a different mobile game), I half feel like the only reason this isn't a concern is games take so long that player don't see the incentive to play more competitively. Add in something like a turn timer or simultaneous , and this does sound like an immediate problem. Though, I do doubt it will be as much of a problem as it was for Polytopia

github-actions[bot] commented 2 months ago

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 15 days.

tuvus commented 2 months ago

I am unstaleing this issue since I think the notification needs to be fixed. I looked into it at some point but I will need to look into it more.