NetworkVariable dictionary write permission and duplicate key issues

[ezoray]

Description

1. The network variable dictionary ignores Owner/Server write permission allowing the non-owner/client to write to the dictionary (possibly its own copy but I'm not sure on that).
2. If a client connects while entries are being added to the dictionary a duplicate key error is reported in the client. Something similar happens on change of ownership.

Reproduce Steps

The first issue can be replicated running the code here.
To see the second issue run the code in the attached unitypackage.

Actual Outcome

1. Client can write to the dictionary when it doesn't have permission.
2. On connection the client reports the error: ArgumentException: An item with the same key has already been added. Key: n

Environment

• OS: macOS Big Sur
• Unity Version: 6
• Netcode Version: Release 2.0.0
  netdictionary.unitypackage.zip

[NoelStephensUnity]

@ezoray
Ok, this was a very good catch and a bit tricky to prevent. I went ahead and pulled down your repo to use your examples (nice btw! 👍 ) and came up with a solution for this that prevents local modification of a collection with still a few minor caveats.

The underlying issue is that default .NET collections have no way to be notified when the collection is modified and since we have already committed to supporting these types it is a bit of a catch 22 scenario:

• Collection properties are really references to the collection itself
  • Due to this, simple assignment only creates a reference/pointer to the same collection and as such full duplication of the collection is required in order to have two unique collections for comparison purposes.
• Since we don't want to poll every NetworkVariable property of every NetworkBehaviour component (very bad for performance reasons) and since only the client/server with write permissions can mark something dirty that then adds the NetworkObject to a post late update queue that is then polled for changes to NetworkVariable properties (i.e. clients without write permissions will never have this check), I had to come up with a way to provide developers a mechanism to check for changes on non-authority instances.

The solution (thus far) is to keep one more internal duplicate of the NetworkVariable that is a duplicate of the current value and can be used to check against the current value when:

• Handling reads (deltas or field), if the m_InternalValue is not equal to the m_InternalOriginalValue then prior to reading the update on non-authority instances the m_InternalOriginalValue is applied first, then the change is applied, then a duplication of the m_InternalValue is applied to the m_InternalOriginalValue.
  • This handles any edge case scenarios where a delta could be processed but the collection on the non-authority side could still have been modified.
• NetworkVariable.CheckDirtyState will first determine if the client has write permissions and if not it does the same comparison between the m_InternalValue and m_InternalOriginalValue and if there is a delta it applies the m_InternalOriginalValue to the m_InternalValue (i.e. reverts back to the last known current value).
• NetworkVariable.IsDirty does the same kind of check as NetworkVariable.CheckDirtyState and if the two don't match on a client without write permissions the value is reverted back to the last known current value (which is not the same as the previous value but just a duplicate of the current value).

Due to the underlying issue of standard .NET collections and lack of any notification (without using a modified/alternate version...since we are supporting the standard .NET collections), the best way to handle the scenario where a client could modify a collection without invoking the NetworkVariable.CheckDirtyState was to provide the alternate option of invoking NetworkVariable.IsDirty which reverts any local changes on clients without write permissions and returns true (without actually setting the dirty state) so developers can have some way of detecting if a collection was modified on a client without write permissions. This is only in the event you might want to take some other action and/or generate some other event (if trying to detect cheating or the like), but doesn't require reverting back to the last known current state since it automatically does this for you.

I need to mull over these modifications a bit more and extend the NetworkVariable collections integration tests to validate the modifications... but should have a PR up for this by end of day. Once that is done, I will post the modified version of your project here that will also point to the PR branch so you can test it yourself (in the event you want to do a second pass to make sure it works as you would expect and to see how I was testing using your project).

Also wanted to thank you very much for your efforts in helping us catch these kinds of things and for your awesome project that hopefully others will be able to learn from... super super awesome stuff you have there!
👍 💯

[ezoray]

Thanks Noel for the explanation, I think I'm getting it. It does seem that having NetworkVariables handling collections is pretty awkward and outside their original remit. I must admit I'm not really liking having the receiver have to do a comparison of the collections to find out what's changed, but as it's the only option I guess I'll have to get used to it :) Please post the modified code when you're done and I'll be happy to test it.

Thanks for your kind words on the project. I hope it's useful to others, it did motivate me to bring some order to a lot of test code scattered across various projects. I've also learned and am still learning things I missed or wouldn't have come across otherwise, so win win. :)

[NoelStephensUnity]

@ezoray
Ok, I am attaching the modified project that starts off with the NetworkVariableDictionaryScene scene. I sort of moved a few things around (I prefer to make stand alone builds as a final pass and added my NetworkManagerHelper that has the connect/disconnect simple UI with the generic/default runtime console logging) and added a CheckDirty toggle that only displays on the non-owner side and when disabled prevents NetworkVariable.CheckDirtyState from being invoked within InScene.DictionaryUpdate and uses the NetworkVariable.IsDirty method instead. The manifest points to the PR's branch.

NGOTOGO_CollectionsFix.zip

Let me know if this seems like it works more like you would expect?

Cheers!
😸

[ezoray]

I had a look and there is an issue, I think down to the first client interaction with the dictionary after switching ownership to the client. There's different ways to produce it, the dictionary just needs to have multiple entries.

One way, as server add a couple of entries to the dictionary:
Dictionary - key: 0 value: 10
Dictionary - key: 1 value: 10

Switch ownership to client, then add another entry to the dictionary:
Clients dictionary, count 3:
Dictionary - key: 0 value: 10
Dictionary - key: 1 value: 10
Dictionary - key: 2 value: 10

Server throws exception ArgumentException: An item with the same key has already been added. Key: 1
I think the key in the exception is the last entry that the server added before switching ownership.
Server dictionary count remains 2.

It may be related to the first call of CheckDirtyState on the client after ownership changes to the client. I noticed if no change has been made to the dictionary and CheckDirtyState is called it triggers OnValueChanged with the previous dictionary being empty.

I should add this looks to be a separate issue and the permission changes seem to work fine. :)

[NoelStephensUnity]

@ezoray Very good catch!
Using the same modified project provided, delete the packages-lock.json file to get the most recent update. The replication is specific to the scenario where the client is already connected, then the entries are added, then ownership transfers to the client, and finally the new client owner adds an entry. Under this scenario, the client does not update its previous value (as you pointed out) which upon the client making a change it sends the last/previous entry added and the newly added entry causing the key already exists exception.
The most recent push to the PR should resolve this issue. It basically assure that any owner write permission NetworkVariable has its previous and original value updated to mirror the current value.
👍

[ezoray]

Looks fixed to me. 👍

The only other issue is one I mentioned previously. While entries are being added constantly to the dictionary by the server, when the client connects they get the contents on spawn but also a duplicate of the last entry resulting in an exception. This was happening on ownership change as well but I haven't checked this with the modified project code.