Send only relevant ItemStack meta fields to Client

[LizzyFleckenstein03]

The goal of this PR is to avoid massive network traffic and "bookbanning". This PR prevents sending any ItemStack metadata to the client, except the description, color and tool_capabilities. It was originally created for the Clamity server to prevent "bookbanning" and lag, but then I realized that other servers might also profit from this patch so I might as well contribute it to upstream. Bookbanning works by putting items that contain a lot of data into shulkerboxes (in case you don't know, shulkerboxes are basically portable chests, when they are dug, the items inside get stored in the metadata of the itemstack). When several of these boxes, filled with items with lots of data, are dropped into the the inventory of a player, the inventory gets so large it is too big to send and the server crashes with this message:

2021-03-01 04:58:54: ERROR[ConnectionSend]: /home/clamity/minetest-5.4.0/src/network/connection.cpp:3f9: bool con::UDPPeer::processReliableSendCommand(con::ConnectionCommand&, unsigned int): An engine assumption 'c.data.getSize() < 0x8000*512' failed.

Every time the affected player tries logging in the server will crash again, with this message.
A similar exploit exists in Minecraft, except that it does not crash the server, but rather kicks the client so it is effectively banned. In Minecraft books are used to ban players using this method; in MineClone2 books have a limit of 5kB, so even if they were filled with random characters (to avoid zlib compression), they would contain less data then e.g. an enchanted pickaxe (21kB).

This PR does not significantly reduce the network traffic that comes from having enchanted tools themselves in the inventory, since the data of enchanted tools is the tool_capabilities, but for shulkerboxes with enchanted stuff, it reduces the data from 0.5 MB to a few byte. This does not only help against bookbanning, but also removes lag.

This affects player inventories, detached inventories and inventories in node metadata.

To do

This PR is a Work in Progress.

• Remove the name field from the whitelist, since it is not needed by the client
• Add a possibility to disable this patch globally (to make local map saving possible)
• Add short_description and palette_index to the whitelist
• Add a possibility to disable this patch for certain players
• Find a solution to fix the problem that the client will sometimes incorrectly predict whether 2 stacks can combine
• Possibility to mark fields as public

How to test

Add a clientmod like this:

minetest.register_chatcommand("wielded", {
	func = function()
		return true, minetest.localplayer:get_wielded_item():to_string()
	end
}) 

and a server-side mod like this:

minetest.register_chatcommand("wielded", {
	func = function(name)
		return true, minetest.get_player_by_name(name):get_wielded_item():to_string()
	end
})

Next, you need to obtain an item with metadata. This can be done using the MineClone2 subgame, by putting a few items into a shulkerbox. Then wield the item, and run both the .wielded and /wielded command. You'll notice that the client command will print significantly less data. You can also try it with a named shulkerbox (name it using an anvil) - the colored description, will still work and also show up in the .wielded command, but the serialized inventory data of the shulkerbox does not.

[LizzyFleckenstein03]

I'd appreciate any ideas on how to disable the patch per-player. (I tried player meta but that does not seem to work very well without violating the execution model)

[sfan5]

This PR kinda seems like treating the symptoms.
Generally I'd put the responsibility on ensuring that the amount of data stored in item meta is sane onto mods, but that might not be so feasible when you have structures that recursively store items.

Also, with CSM hopefully coming (one day) it's not a good idea to have a hardcoded filter list of what is considered "relevant". Client-side code may want to read any of of the metadata keys.

For the crash, the engine should somehow handle that better in any case. (just drop the packet? disconnect the client?)

[anon55555]

This will mean the client will sometimes incorrectly predict whether 2 stacks can combine.

[anon55555]

Also it would break local map and inventory saving (i use a proxy that saves a local copy of my inventory).

[anon55555]

For the crash, the engine should somehow handle that better in any case. (just drop the packet? disconnect the client?)

I think dropping the packet, sending a warning chat message and logging it would be a good way to handle a too big packet.

[anon55555]

Inventory lag could be reduced by making the server send Keep lines.

As a temporary solution server owners can use https://gist.github.com/anon55555/4dcaa39d19343c0f9f33ae3a245cfbd4, but it would perform better if it was done inside the server process.

[LizzyFleckenstein03]

This PR kinda seems like treating the symptoms.
Generally I'd put the responsibility on ensuring that the amount of data stored in item meta is sane onto mods, but that might not be so feasible when you have structures that recursively store items.

Ok, I guess you want me to do the same thing that was done for Node meta then, adding the ability to mark fields as private? But to be honest, wouldn't it make more sense to leave things private by default and require mods to mark things as public if needed? A lot of data is sent to client that is never actually used. Shulkers do not recursively store items, they are not nestable. In 5.3 it was possible to nest them using a rather simple trick, but that is fixed in 5.4. The problem here is enchanted stuff. Minecraft calculates digging times client-side whenever they are needed, if the user starts digging a node it will be calculated directly, but we don't have that possibility in MT, so basically the tools contain a giant table with all digging times. This is no problem when stored in itemdefs, but for some enchantments these tool capabilities need to be changed dynamically (using ItemMetaRef:set_tool_capabilities) and every time an item like this is moved in inventory it needs to be sent entirely. That itself is not easy to fix, but this patch at least prevents lag from such items that are inside shulkers.

Also, with CSM hopefully coming (one day) it's not a good idea to have a hardcoded filter list of what is considered "relevant". Client-side code may want to read any of of the metadata keys.

I am honestly surprised that you tell me that Client code might want the read meta. What if client code wants to know the positions of objects around? What if client code wants to read a setting? What if client code wants to know what is inside the player's inventory without the player having to open it? Client code has basically access to nothing. The only usecase I see for accessing Item meta is .peek, which is a Hack to preview the contents of a shulkerbox. If servers want to allow clientmods that read Item meta, they have to disable the patch globally or per player. That was it can be configured how much data can be accessed by CSM.

I really don't see the point of sending insane amounts of useless data to the client.

[LizzyFleckenstein03]

This will mean the client will sometimes incorrectly predict whether 2 stacks can combine.

This could be fixed by hashing the metadata that is not sent and sending the hash of a fixed length instead.

[anon55555]

I think the best solution to large items would be allowing the description and tool capabilities to be computed client side from the metadata (like enchantments and custom name in the case of mineclone2), maybe a small lua program could be placed in the itemdefs and run in a client side sandbox which only has access to the itemstring and the itemdef.

[anon55555]

This would also have prevented the issue of enchanted picks from an older version mining obsidian too fast in mcl2.

[LizzyFleckenstein03]

Oh no! This would allow for cheating! ;-)

[anon55555]

The large tool capabilities issue could also be fixed by registering e.g. mcl_tools:pick_{wood,stone,iron,diamond,gold}_efficiency_{1,2,3,4,5} because only efficiency needs different tool capabilities (stuff like sharpness could be added with server side callbacks).

[sfan5]

Ok, I guess you want me to do the same thing that was done for Node meta then, adding the ability to mark fields as private?

I didn't suggest a specific implementation, but that'd be a possible one yes.
And as @anon55555 pointed out you still have to worry about client-side stackability prediction when changing which item meta is sent to clients.

Generally I wonder if it even makes sense to pursue private item meta when the only issue with it seems to be a specific, unfortunate usage in MCL2 (and data minimization in general).

But to be honest, wouldn't it make more sense to leave things private by default and require mods to mark things as public if needed? A lot of data is sent to client that is never actually used.

I agree, but there's no use discussing this because we cannot retroactively introduce private-by-default.

I am honestly surprised that you tell me that Client code might want the read meta. [...]

I wasn't clear enough on that, sorry.
I am talking about SSCSM (server-sent mods that run on the client). These will be able to modify the world, objects, inventory and other client behavior. In that context it'd be absolutely valid to read item metadata.

[ryvnf]

Also it would break local map and inventory saving (i use a proxy that saves a local copy of my inventory).

I think it should be up to the server admin to decide if world saving should be able to save the full itemstrings. The send_all_item_metadata setting can be used for that.

Generally I wonder if it even makes sense to pursue private item meta when the only issue with it seems to be a specific, unfortunate usage in MCL2 (and data minimization in general).

I do not think the only use case for this is minimizing network load (even though that is a big advantage). Hidden metadata can be used by mods to hide certain properties of items to clients, like what a cursed ring a player found in a dungeon does. Such metadata should not "leak" to the client unless it is used by the client.

Currently it makes no sense for clients to be able to read or inspect item metadata (other than the those used by the client like description and tool_capabilities).

I agree, but there's no use discussing this because we cannot retroactively introduce private-by-default.

I think introducing private-by-default retroactively makes sense because the metadata is currently only useful for the server. This should not break existing mods. I think the logical approach would be to introduce private-by-default and add a way to specifiy which fields should be public once SSCSM is introduced.

[anon55555]

I think it should be up to the server admin to decide if world saving should be able to save the full itemstrings. The send_all_item_metadata setting can be used for that.

Wouldn't it be better if each player could make their own choice, rather than having the admin make one choice for everyone.

[LizzyFleckenstein03]

@anon55555 Yes, look at the ToDo list.

[appgurueu]

I agree with sfan5 that it is the responsibility of mods to ensure no such pollution. They may either impose limits (in your example, either books or the shulker box may do this), or alternatively, they should store item-related data using keys: the item metadata should only contain a key to more serverside data which is persistent in mod storage. Dropping entries in mod storage is trickier: You have to determine when an item has become inaccessible/destroyed. A simple approach might be to delete the relevant entry (unique keys, such as autoincrement) if a dropped item despawns/is destroyed by lava.

I'm also not a fan of the API: A single serverside setting controls whether "all or nothing" is sent (in terms of extraneous meta). I would much prefer a node-meta-like API using meta:mark_as_private(field).

Looking at your ToDo list:

• Add a possibility to disable this patch for certain players

Sounds unclean to me. Additional information can be sent over modchannels, indexed using a key stored in meta,

• Find a solution to fix the problem that the client will sometimes incorrectly predict whether 2 stacks can combine

Would require a hack, such as the one you have mentioned (using hashes).

• Possibility to mark fields as public

Inconsistent with node meta API (although it is being argued that private would be a more reasonable default).

TL;DR: The "book bans" should definitely be fixed and the server should send a minimum of metadata. The best approach IMO is however to use keys to server-side lookups instead of implementing this, which can already be done fine using mods.

[SmallJoker]

To me it makes sense to only send what's required by the client, any newer special field would require an engine change anyway. The item (de)serialization is quite slow, and the reduced data amounts is IMO just a convenient side-effect for server owners.

@ more items/tools: This is inconvenient and slows down the server initialization and client joins. For highly dynamic items, the metadata key makes perfect sense.
@ metadata alternatives: Item metadata is easy to use, and relying on other storage methods for per-item information would be tiring. However, mods would still need an efficient storage to reduce server resources (disk I/O, RAM, CPU).

Of course there must be a way for the client to distinguish different items so they cannot be stacked. After that's done I'd even welcome making this a standard like incremental inventory sending (no setting), unless CSMs would somehow depend on specific fields (-> mark_as_public function?).

[anon55555]

I agree with sfan5 that it is the responsibility of mods to ensure no such pollution. They may either impose limits (in your example, either books or the shulker box may do this), or alternatively, they should store item-related data using keys: the item metadata should only contain a key to more serverside data which is persistent in mod storage. Dropping entries in mod storage is trickier: You have to determine when an item has become inaccessible/destroyed. A simple approach might be to delete the relevant entry (unique keys, such as autoincrement) if a dropped item despawns/is destroyed by lava.

There would probably be leaks caused by bugs and this would break local map saving.

Looking at your ToDo list:

* Add a possibility to disable this patch for certain players

Sounds unclean to me. Additional information can be sent over modchannels, indexed using a key stored in meta,

Disabling it is needed for local map saving to work correctly.

[SmallJoker]

Disabling it is needed for local map saving to work correctly.

What might help there is to disable the "reduced data sending" only when the client reports that it's saving to the disk (new RemoteClient/RemotePlayer variable on login). Manual modifications server-side could still be made if they want to prevent the map from being copied.
It would (by the way) also supersede the newly added setting in this PR (handled by map saving setting).

[anon55555]

I do not think the only use case for this is minimizing network load (even though that is a big advantage). Hidden metadata can be used by mods to hide certain properties of items to clients, like what a cursed ring a player found in a dungeon does. Such metadata should not "leak" to the client unless it is used by the client.

If what the ring does is random, it could be decided when it's first used.

[anon55555]

Disabling it is needed for local map saving to work correctly.

What might help there is to disable the "reduced data sending" only when the client reports that it's saving to the disk (new RemoteClient/RemotePlayer variable on login). Manual modifications server-side could still be made if they want to prevent the map from being copied.
It would (by the way) also supersede the newly added setting in this PR (handled by map saving setting).

What about old clients which don't say whether they are saving the map?

[nerzhul]

i suggest to add a ItemStack.network_attributes which container list of attributes to publish through network.
It's better than 0 and 1 on all attributes, and it will be ready for future CSM

[ryvnf]

I would also like to make a distinction between broken map saving and incomplete map saving. What happens with this change is that the saved map becomes incomplete. Items will still be saved, but they might not work with the mods because they are missing metadata. It should also be mentioned that map saving is incomplete anyways (since unexplored areas are not downloaded). I think most people who use map saving use it to get a snapshot of the current builds on a server, not as a way to get a fully playable world in singleplayer.

In general I do not think a client has a right to know all the metadata of items. It is good design to only send what is necessary to clients.

[anon55555]

The only other thing it does is disabling it when the client is map saving.

No? In it's current state what this PR does is:

* Hardcode a list of key-value pairs not be sent (+ hashing for stackability prediction)

* Have a global setting to disable this

* Client informs the server whether it's saving the map, the server then disables it only for this client

There's almost zero overlap with my list.

Now I understand, you think mods should decide which meta fields should be sent.

[sfan5]

Just so it's clear: I'm 👎 on this PR because I believe it to be the wrong approach.

[ryvnf]

I also think this PR should not be merged for the stated reasons. I agree with sfan5 that it is the responsibility of mods to keep the amount of metadata for items sane. The way Mineclone2 stores digging times for enchanted items is just ridiculous. It basically stores the digging time for each and every registered node in each enchanted item. The reason it does this is to get custom digging times. This is not only bad for performance reasons but has a large number of other problems. I'm working on a better solution for that problem and I'm very confident that it is possible.

But while I don't agree that this should be merged just for performance reasons I still think it is a good PR. I think it should be possible to have private metadata which is not sent to clients.

This is what I think this PR should do:

• A setting to control if all metadata is sent to clients (to support complete world downloading)
• Private metadata should not be sent to clients
• A way for mods to control the visibility of different metadata fields (preferably in itemdefs)
• Hashing to solve the stackability problem

I also think having private-by-default makes more sense than public-by-default but I'm fine with both options.

I don't think this PR should handle clients differently depending on if they have local map saving enabled or not. I think having private metadata doesn't make sense if it can be bypassed just by flipping a setting. Supporting that also makes the code more complicated for something that only a very small minority of players will ever use.

[anon55555]

I think it should be possible to have private metadata which is not sent to clients.

I don't see any use of this other than performance.

I also think having private-by-default makes more sense than public-by-default but I'm fine with both options.

Public by default would mean local map saving would still work correctly for non-hidden aspects of items.

I don't think this PR should handle clients differently depending on if they have local map saving enabled or not. I think having private metadata doesn't make sense if it can be bypassed just by flipping a setting.

If it is for reasons other than performance I agree.

Supporting that also makes the code more complicated for something that only a very small minority of players will ever use.

Looking at 2329236 I don't see much added complexity.

[ryvnf]

I don't see any use of this other than performance.

It's useful to prevent hackers from reading metadata they shouldn't have access to.

Looking at 2329236 I don't see much added complexity.

It's not much code but it involves a protocol change and it is still simpler to be without it.

[anon55555]

I don't see any use of this other than performance.

It's useful to prevent hackers from reading metadata they shouldn't have access to.

Can you give an example of where hiding metadata would stop a hacker from gaining an unfair advantage?

[appgurueu]

I don't see any use of this other than performance.

It's useful to prevent hackers from reading metadata they shouldn't have access to.

Can you give an example of where hiding metadata would stop a hacker from gaining an unfair advantage?

Off the top of my head: Ender chests / items storing inventories. A hacker can look inside the stored inventory.

[anon55555]

I don't see any use of this other than performance.

It's useful to prevent hackers from reading metadata they shouldn't have access to.

Can you give an example of where hiding metadata would stop a hacker from gaining an unfair advantage?

Off the top of my head: Ender chests / items storing inventories. A hacker can look inside the stored inventory.

That's not a very big advantage and hiding it would mean local map saving wouldn't be able to save the stored inventory.

[appgurueu]

That's not a very big advantage and hiding it would mean local map saving wouldn't be able to save the stored inventory.

Server owners maybe don't want local mapsaving to work in this case, as that allows abuse to reveal otherwise hidden information.

[anon55555]

Local map saving necessarily reveals hidden information like what's behind a wall or what's in a chest you didn't open, but I think this is worth it if it allows creating a complete local copy of an area of the map.

[appgurueu]

Local map saving necessarily reveals hidden information like what's behind a wall or what's in a chest you didn't open, but I think this is worth it if it allows creating a complete local copy of an area of the map.

I don't think so. It is being abused to look inside locked chests and the like. This is not the purpose of it. Saving structures is fine, it helps preserve works of art if servers go down or are griefed.

[anon55555]

Does it really matter if you can see what's in somebody's locked chest? I wouldn't mind this being hidden though, because you have no way of observing it on the server, what I don't like is hiding aspects of the map which you could observe on the server by e.g. opening a locked chest which you have a key for or placing and looking in a shulker box.

[nerzhul]

all thought about local map saving but forget my comment.
Instead of blacklisting attributes to send to client, i prefer to do the inverted way, we send only relevant meta fields, and mods must supply for a meta which fields are to send to client, if needed.

[appgurueu]

all thought about local map saving but forget my comment.
Instead of blacklisting attributes to send to client, i prefer to do the inverted way, we send only relevant meta fields, and mods must supply for a meta which fields are to send to client, if needed.

That's considered breaking compatibility, as the current default has been sending all fields.

[LizzyFleckenstein03]

Everyone seems to have a different opinion on this. I don't really care too much how exactly it is done. What is really important to me is that it should be possible to disable this for certain players.

I am for marking fields as public rather than private, because it could otherwise cause confusion among modders if "private" fields are sent to clients that have local map saving enabled, but this is not important to me, and most modders will probably not even care to update their mods to make everything that is not needed to be sent to client as private. And in the few rare cases in which the client actually needs to access a field in the metadata, the developer will probably notice if the changelog mentions things about this, while most developers might just ignore things that are not required for their code to work. I don't think the behavior should be changed for nodemeta; I just think that we should do it better for itemmeta. Just a thought tho.

Setting public / private fields using an ItemMetaRef:set_public or :set_private is not a realistic idea since there are many ItemStacks that are already created (in chests, inventories, dropped, serialized inside misc things etc.) and updating all of these reliably is an impossible task for a modder. With nodes, this is much easier (on_construct + LMB). Therefore these definitions have to be in the itemdef rather than than the meta. This is not my opinion; it is simply the only way it can possibly work.

There had been criticism that the problem this PR fixes is very specific to MCL2; if you don't agree with that you can skip the next paragraph.

I think that MineClone2 and it's mechanics are more important for the Minetest engine than you might think. That is not because I think cloning Minecraft is such a good idea. The problem here is that many Minetest mods and games are boring. I will explain my reasons and also what it has to do with this PR. MineClone2 having the most downloads on ContentDB is because it has actually good gameplay. Making a good game means more than being good at programming, the gameplay needs to be well planned and designed; and Minecraft has well planned and designed gameplay, it is done by professionals. The problem is that there are very few good gameplay ideas within the MT community itself. Most programmers simply suck at game design (I include myself here). I'm not saying there are no good ideas, no need for anyone to get offended. The other reason is a technical one. Usual mods & games are based on what is possible using the MT engine without workarounds, and that is the problem, because it prevents complexity. MineClone2 has a goal outside of Minetest, and that is why it includes a lot of technical challenges. If MineClone2 does something like storing items inside other items or modifying toolcaps dynamically the problem is not that "they are trying to clone Minecraft, it's their own fault", the problem is that no other game or mod does something like that. Even the games that start off with a good concept don't dare to really break the limits and do something very complex that the Minetest engine is not directly designed for. (What I'm saying is that there is complexity, but only concerning things that are intended and supported by the Engine; e.g. complex GUIs, Technic etc.). MineClone2 is effectively a benchmark for the Minetest engine. Of course there are some features in MCL2 that are very specific to Minecraft; but excuse me, storing items inside items is not. And neither is setting toolcaps dynamically. MineClone2 is just the only big game that really has its own concept, with the exception of Crafter, which even took it further than MCL2. There are small approaches; but there is no other game with both lots of content and gameplay complexity. The problem is that the other mods don't challenge the engine in the way MineClone2 does, not that MineClone2 does things that no other mod does. I personally find it sad that the best game the Minetest has to offer is just a clone of Minecraft.

[SmallJoker]

That's considered breaking compatibility, as the current default has been sending all fields.

Which compatibility? The only thing that would break with the current implementation are CSMs that use special meta fields using get_wielded_item. Manual whitelist from mods seems totally reasonable, if there's any demand for it.

This PR is neither about MineClone 2 in specific, MC vs MT, nor locked chests (<- latter is totally unrelated btw). It's an optimization step which stops client-irrelevant data from being sent. Unlike with nodes, there are no static addressable fields for formspecs (${name} notation), and CSMs are still not a common thing nowadays.

A whitelist API could be added but at the moment I honestly don't know of any use-case for it.

[anon55555]

Since the client can already request all fields, if customization of the fields sent is added, it would make more sense for the client to tell the server it wants a specific set of fields. By default the client would request the ones it uses, but CSMs could change this and it could request all fields when it's map saving. In the network protocol the client would have a way to ask for either a specific set of fields or all of them. This would also mean if new fields used by the client are added, old servers would send them.

[SmallJoker]

Closing this PR due to unfortunate implementation like mentioned by sfan5 above.