Balance resets

[quiquelhappy]

Moving the private chat with Unmn3d on Discord here.

We've experienced balance resets when using RedisEconomy due to a race condition caused by EssentialsX economy forwarding vault transactions for some plugins, notably (but probably not limited to) quickshop.

Debug trace follows:

[02:09:32 INFO]: 01 Sent update account <account> to 0.0 [A]
[02:09:32 INFO]: 01 Sent update account <account> to 98668.0 [B]

Essentials, instead of doing a balance update based on addition and substraction, will reset the entire balance to 0, then set the new balance with the precomputed value. Since these two actions are triggered pretty much at the same time (not concurrently, but one right after the other), depending on the database and server load, the instructions might not be stored in the database in strict chronological order - needless to say this could be an issue, allowing some exploits in terms of balance amounts.

TLDR; classic race condition issue, [A] [B] should be executed in that order, but [B] ends up being stored before [A].

Proposed solutions (as someone who has never used redis):

• adding a timestamp to balance updates, so values are only applied if they are newer than the last stored balance update (using millis [probably enough] or nanos as a timestamp value, ofc)
• postfix solution in which the plugin backtracks local transaction history to apply the last correct expected value
• (easiest fix but worst for performance for servers with a very large transaction volume or slow databases) adding locks to balance updates in order to limit concurrency.

Extra resources

• Plugin I coded to test transaction burst with a similar behavior: link (can be used with /testbalance <cycle count> <player>
• Test enviroment that will automatically spin up two papermc instances + redis with the provided plugin: link (docker compose up, ports 50001, 50002)

Note

• /browse-transaction seems to always return the correct order, didn't test it in depth tho

[quiquelhappy]

We are now using the locks solution, because it fits our needs. Updated code

in this case, there are clear drawbacks, such as performance is now limited, in reality the lock should be player-pair based, so a hashmap with key of emitting player (could be null) + receiver player with locks as keys
that should make the system faster overall because it allows for concurrency for all transactions from different players
but the global lock works well enough for us right now, we dont really have a high volume of balance updates

[Emibergo02]

Fixed in 4.3.9