Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(onesync/server): basic state bag write policies #2257

Conversation

tens0rfl0w
Copy link
Contributor

This introduces a basic rejection logic for replicated player/entity state bags.
"Use-case" should be any type of state bag usage where "truthful" data matters, for example syncing economy-related data.

Implementation:

  • ScRTs can use SET_STATE_BAG_KEY_POLICY to set a write policy for a certain state bag key.
  • If write-protected, any client changes will be rejected by the server and the server value will be resent to the requesting client.
  • A write policy affects all state bags. If set this will reject all value changes on the protected key.

I originally implemented this on a per state-bag/entity/player -> key logic, but reconsidering the actual use cases for this type of feature this didn't make much sense.

* Basic rejection logic for write-protected keys in player/entity state bags.
* Resync replicated state bags with the source client (on rejection).
* Native handler 'SET_STATE_BAG_KEY_POLICY' to allow setting write policies per key.
@tens0rfl0w tens0rfl0w force-pushed the feat/basic-statebag-writepolicy-v2 branch from 4a7d7e5 to dbdc89c Compare November 1, 2023 06:37
@thorium-cfx
Copy link
Contributor

thorium-cfx commented Nov 3, 2023

I'm changing this into a draft, as this feature requires more design work and consider more use-cases before it's a feature we can keep supporting in the future.

Some internal points I have on this, to consider and to give context:

  • Allowing/disallowing clients to read/write global, player, and entity values, checked on both server and client.
  • Allowing/disallowing server to read/write global, player, and entity values.
  • Different policies.
  • Be performant.
  • Updates (or not) statebag changes according to their rules.
  • Allows the resource developer to set their desired policy on a per key basis.
  • Delays between server and client (correcting changed values mid-policy change).
  • Several use-cases (per-key, per-bag, etc.)

@thorium-cfx thorium-cfx marked this pull request as draft November 3, 2023 13:20
@thorium-cfx thorium-cfx changed the title fix(onesync/server): basic state bag write policies feat(onesync/server): basic state bag write policies Nov 3, 2023
@tens0rfl0w
Copy link
Contributor Author

Hey @thorium-cfx, thank you for noting the internal points you have on this here.

While trying to come up with an implementation for most of your proposed features/policies, I've come across one major concern that's hard to solve:
Exposing an interface/a 'native handler' for all the proposed policy options that is

  1. Not too complex.
  2. Has enough different parameters to expose enough policy options to ScRTs/resource developers.

Also, I do have to disagree with some of the proposed policy scopes:
-Allowing clients to write to global states shouldn't be an option as global states should only contain server-authoritative data.
-Read policies are not necessary, as replication-state already works as "not exposing" bags to clients, and having a server-read policy introduces possible unwanted side-effects/confusing logic issues when used by resource developers.
-The server as the authoritative entity should always be able to write to bags.

IMO, the disagreement about policy scopes comes from the fact that this PR only exists to solve the "main" reason this feature is so anticipated: Having server-authoritative player/entity state bags.

Reading comments about this feature on the forums, the Discord guild, or here shows that the "main" reason users want this feature is to implement use cases like:
-Syncing economy-related data
-Syncing inventories via state-bags
-Having entity-attached data like vehicle trunk inventories
-In general: Blocking client-writes to entity/player state bags

I do agree that broader policy scopes like per state bag category make sense, but other than that I don't see the need for the other proposed policy options.

@AvarianKnight
Copy link
Contributor

AvarianKnight commented Dec 13, 2023

I think there should be a way to go back to the default way how state bags worked before, where only the server could create state bags, and also have a way to default everything to be server write, client read unless set otherwise for the state bag.

This would be the most sane security default but I don't think it can be swapped easily without breaking compatibility so it would be good to just have a "sane default" ConVar.

Allowing/disallowing clients to read/write global, player, and entity values, checked on both server and client.

I think @tens0rfl0w hit this on the head, clients should never be allowed to write to global state. As for the disallowing clients to read player/entity/global state the simplest solution would to probably be to have an enum.

enum eStateSyncFlags {
	NoFlags = 0, // default flag
	OwnerOnly = 1, // only sync the state to the owning client
	NoSync = 2, // only the server should ever know this value, and it shouldn't ever be sync'd to clients
	ReadOnly = 4, // this would reject any change made by the client before ever being sent to the server
	// more if needed later
}

Allowing/disallowing server to read/write global, player, and entity values.

I don't think this should be under consideration, the server is meant to be authoritative, nothing should be able to tell it its not able to touch anything.

Different policies.

I think more context is needed here @thorium-cfx (I'm assuming this might be related to other stuff i posted here already)

Be performant.

On this regard it might be worth swapping away from std::map as iirc @gottfriedleibniz mentioned this was causing some bad access times (though I can't find the comment right now).

It might be work making our own way of handling data to maintain consistency, like maintaining ordering that state bags are set in or allow state bags to be assigned priority to be able to be used consistently to avoid some confusion

Updates (or not) statebag changes according to their rules.

I think this is somewhat explained how I would think to do it elsewhere

Another other issue that should probably be addressed #1854

@thelindat
Copy link
Contributor

consider more use-cases
Be performant.

So long as policies can be implemented in way future updates don't break backwards compatibility, I believe it's worth implementing what can easily be done right now. Right now there is nothing in place to prevent bad actors from creating a limitless number of statebag keys with enormous amounts of data.

  • There is nothing to prevent instantiating arbitrary statebags.
  • There is no rate limit on how frequently the client can update statebags.
  • There is no limit on the size of the data being set.
  • There is no limit on the number of states that can be stored on a statebag.

I ran a simple function to spam statebags and managed to increase fxserver's memory use by 1gb, which cleared the moment I dropped and statebags were wiped. Considering the 100,000 states I added would then get synced to any player who entered my scope.. bad time for all.

@AvarianKnight
Copy link
Contributor

I ran a simple function to spam statebags and managed to increase fxserver's memory use by 1gb, which cleared the moment I dropped and statebags were wiped. Considering the 100,000 states I added would then get synced to any player who entered my scope.. bad time for all.

In this situation I'm not sure why the default was changed away from a server has to initialize the state bag to client can initialize any state bag it wants (which is the main reason this becomes an issue).

@tens0rfl0w
Copy link
Contributor Author

Superseded by an internal proposal for a new state bag API.

@tens0rfl0w tens0rfl0w closed this Jun 11, 2024
@tens0rfl0w tens0rfl0w deleted the feat/basic-statebag-writepolicy-v2 branch June 11, 2024 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants