Fix the player information version_string return value #8616

Lejo1 · 2019-06-22T19:50:28Z

What for?

This PR allows to access more player information just like the version string which was before only available on the debug-build.
The version_string never worked not even in debug-build thats now fixed too.
It should help server owners to check which version a client has so that they for example can ask them to update.

How to test

minetest.register_chatcommand("pinfo", {
  params = "<name>",
  description = "Get some infos of a player.",
  privs = {ban=true},
  func = function(name, param)
    if not minetest.get_player_by_name(param) then
      return false, "Player is not online"
    end
    for key, value in pairs(minetest.get_player_information(param)) do
      minetest.chat_send_player(name, tostring(key).." is "..tostring(value))
    end
  end,
})

EDIT by SmallJoker: ~~PS: How to format this code correctly?~~

SmallJoker · 2019-06-22T20:24:07Z

The version string can be anything. Really anything. What matters is the protocol and formspec compatibility. Former can already be read, latter not.
I don't think this helps server owners in any way except of a false feeling about having more control or being sure they can now block specific forks 👎 . Never thrust the client data.

Lejo1 · 2019-06-22T20:38:18Z

I know that the client can lie but the use case is not to block users. It's more to understand why a user has a bug and to check for updates or incompatibles.
As long as the client is not modified the version string works perfect.

AKryukov92 · 2019-07-09T09:16:58Z

Hackers are minority of playerbase. Majority are peaceful peasants.
If server owner would know version number of client, then he will be more accurate in reproducing bugs.
I think it is good thing.

ClobberXD · 2019-07-09T09:49:58Z

As long as the client is not modified the version string works perfect.

If server owner would know version number of client, then he will be more accurate in reproducing bugs.

The problem is that the version string can be any arbitrary string. This has nothing to do with hacking and modified clients. As mentioned by SmallJoker and even lua_api.txt, there are compatible forks, both official (Multicraft comes to mind) and unofficial, that follow completely different versioning schemes of their own. We can't expect the same major.minor.patch format from all of them.

That's why we have a protocol version. Forks can also modify the protocol version to their own liking, but then those forks won't be compatible with Minetest anyway. Server-client compat. is solely (AFAIK) determined by the min. max. protocol versions, so checking the protocol versions is a much safer bet than the arbitrary version string.

sfan5 · 2019-07-09T11:18:02Z

Does it even return any useful data?
I don't see the value being filled anywhere https://github.com/minetest/minetest/search?q=m_full_version&unscoped_q=m_full_version

AKryukov92 · 2019-07-09T11:30:34Z

As mentioned by SmallJoker and even lua_api.txt, there are compatible forks, both official (Multicraft comes to mind) and unofficial, that follow completely different versioning schemes of their own. We can't expect the same major.minor.patch format from all of them.

This is exactly why getting "any arbitrary string" is useful. Especially if there are another clients with compatible protocols.
If there is another compatible and official client, which can connect to server, then it needs to be somehow differentiated from regular Minetest clients. The more information is available to server owner, the better.

Why do you think, there is User-Agent in HTTP protocol? It is HTTP request header and can be arbitrary set in each request. It is very useful for gathering information about users and improving their experience.

Lejo1 · 2019-07-09T11:36:48Z

Does it even return any useful data?
I don't see the value being filled anywhere

Yes in this function which gets called by another function called by another function...

minetest/src/clientiface.h

Line 329 in 297beea

void setVersionInfo(u8 major, u8 minor, u8 patch, const std::string &full)

ClobberXD · 2019-07-09T14:17:34Z

If there is another compatible and official client, which can connect to server, then it needs to be somehow differentiated from regular Minetest clients. The more information is available to server owner, the better.

May I know why unofficial clients need to be differentiated? This is would probably make sense for websites, but I don't understand why this is required for Minetest.

AKryukov92 · 2019-07-09T16:51:17Z

May I know why unofficial clients need to be differentiated?

answer is in the first post.

It should help server owners to check which version a client has so that they for example can ask them to update.

If you don't need this functionality now, it doesn't mean that noone needs it.

There might be important differencies in functionality and bugfixes even between MT with different minor version. This is the main reason for adding this property.
It is also very likely that there will be more differences between MT and unofficial MT. This is additional benefit from having this functionality.

ClobberXD · 2019-07-11T10:52:06Z

~~Comment deleted out of sheer embarrassment~~

src/clientiface.h

Lejo1 · 2019-07-11T13:05:27Z

@ClobberXD
It's accessible using minetest.get_player_information
I added it in lua_api.txt

ClobberXD · 2019-07-24T12:36:35Z

Oh sorry, I got confused. What I pointed to was server-side. My bad.

paramat · 2019-07-24T21:20:08Z

The version_string never worked not even in debug-build thats now fixed too.

The bugfix part of this is valuable.

This PR is very simple, so i don't think it needs much justification. The extra information may not be particularly useful, but it is harmful in any way?
Seems to me a good thing that servers have extra information about players, such as the version string, it's the server's own fault if they misunderstand the relevance or reliability of that information.
I support giving more information and power to servers.

SmallJoker · 2019-07-25T17:04:36Z

I'll be a volunteer to pass gibberish version strings to the server if this gets merged. The data is already sent to the server, so my only concern is that server admins begin to rely on these values rather than checking in-game whether it's a "quality player". Sure go on, lock those mobile users (and spin-offs) out if you're happy with that (if there's a second approval).
EDIT:
@AKryukov92

Especially if there are another clients with compatible protocols.

That's exactly why there's the protocol version checking. No spin-off would change these limits without providing full network compatibility. Things would break, stop working or simply annoy players.

paramat · 2019-07-25T17:45:16Z

I support the bugfix part of this, but am neutral about the rest.

AKryukov92 · 2019-07-25T18:23:41Z

I'll be a volunteer to pass gibberish version strings to the server if this gets merged.

I'll be a volunteer to stand against such change. What is the point for making thrash from useful data?
Main point of this is not to restrict players, but to know more about them.

Lejo1 · 2019-07-25T19:48:18Z

Are we here really only talking about the Version string?
The ser_vers, minor, major, patch would already be enough to block fork players. BUT that’s not the goal!
It helps to understand problems.
And especially I really would like to have at least the state which is pretty useful for mods.

Just one thing about Forks:
There are many views what to do with them.
But I really really really don’t want to block them! I just want to tell them that there is Minetest and that this is a Minetest server.
This isn’t ’t possible as you can’t detect them without this new infos. I think this should be good for any MINETEST player.

sfan5 · 2019-07-25T20:57:30Z

👍 bugfix, allowing serialization_version in non-debug mode
👎 everything else

It's more to understand why a user has a bug and to check for updates or incompatibles.

From just the protocol version you can already determine the Minetest version the user is running.
This is enough to "check for updates or incompatibilities".

Lejo1 · 2019-07-27T08:39:36Z

Why passing less to mods if it already exist in the server?
Because some admins thing that that data is always true? They managed it to set up a server they will know how to handle this.

Secondary you just need a filter out very uncommon versions.

You can make yourself a Minetest player or a fork. It doesn’t matter it’s then your fault if you get infos to please play Minetest.
The Mainstream won’t manipulate this data so it’s useful.
I made some testing on my old(0.4.17) server with this data. There weren’t clients which sent unlogic data. You can always sent shit but it’s for the main stream!

ClobberXD · 2019-07-27T12:50:03Z

The Mainstream won’t manipulate this data so it’s useful.

They will, once servers and mods start making use of this to filter out the non-conforming clients.

ClobberXD · 2019-08-09T10:30:23Z

Seems like paramat supports this PR too: #8616 (comment)

ClobberXD · 2019-08-09T10:52:11Z

Just to be clear, I'm neutral on this PR.

On one side, I'm against giving servers an extra reason to discriminate players by. While it's not necessary for server owners to be hostile towards forks, that's quite possible, given that almost all forks (apart from a handful of legit ones) are unofficial/illegal.

On the other hand, MT is a game engine. And as such, it ought to be unrestrictive, and give games/mods as much info as possible, and let the mods do what they want.

rubenwardy · 2019-08-09T10:57:42Z

I have the same view as you, so am also neutral

paramat · 2019-08-10T22:40:28Z

My opinion is:
#8616 (comment)

I support the bugfix part of this, but am neutral about the rest.

This bugfix part of this has support.
The other part of this is disapproved.
Keeping open for now so that the bugfix part is not forgotten.
@Lejo1 please modify this PR to be bugfix only, or let us know if you decide to not do this so that someone else can.

Lejo1 · 2019-08-11T09:58:17Z

With or without ser_vers in non debug mode?
#8616 (comment)

Lejo1 · 2019-08-11T12:04:21Z

Just before you say we don’t want to inform Forks about Minetest or don’t want to make a difference possible to inform them. Some statistics of my old(0.4.17) server:

[join count/percentage] protocol_version, ser_vers, major, minor, patch, version_string

⁃	[2/0.0030280549289164%] 32 ||| 28 ||| 0 ||| 4 ||| 17 ||| 0.4.17-86e29ae5-Android
⁃	[1/0.0015140274644582%] 32 ||| 28 ||| 1 ||| 1 ||| 8 ||| 1.1.8-9c79b8ba-Android
⁃	[458/0.69342457872186%] 28 ||| 26 ||| 0 ||| 4 ||| 15 ||| 0.4.15
⁃	[2/0.0030280549289164%] 25 ||| 26 ||| 0 ||| 4 ||| 13 ||| 1.0
⁃	[2/0.0030280549289164%] 24 ||| 26 ||| 0 ||| 4 ||| 12 ||| 0.4.12-dirty
⁃	[5591/8.4649275537858%] 32 ||| 28 ||| 0 ||| 4 ||| 16 ||| 0.4.16 MT official 
⁃	[41834/63.337824948145%] 27 ||| 26 ||| 0 ||| 4 ||| 13 ||| 0.4.13  This are all these name123 players. If someone knows which game it is please tell me.
⁃	[2312/3.5004314978274%] 32 ||| 28 ||| 0 ||| 4 ||| 18 ||| 0.4.18.5 MT unofficial 
⁃	[220/0.33308604218081%] 24 ||| 26 ||| 0 ||| 4 ||| 11 ||| 0.4.11
⁃	[610/0.92355675331951%] 32 ||| 28 ||| 1 ||| 1 ||| 10 ||| 1.1.10--Android
⁃	[2/0.0030280549289164%] 27 ||| 26 ||| 1 ||| 1 ||| 2 ||| 1.1.2-6a46c7bf-Android
⁃	[39/0.05904707111387%] 32 ||| 28 ||| 1 ||| 1 ||| 8 ||| 1.1.8--Android
⁃	[24/0.036336659146997%] 27 ||| 26 ||| 0 ||| 4 ||| 14 ||| 0.4.14
⁃	[91/0.1377764992657%] 32 ||| 28 ||| 1 ||| 1 ||| 10 ||| 1.1.10
⁃	[500/0.7570137322291%] 28 ||| 26 ||| 1 ||| 1 ||| 4 ||| 1.1.4
⁃	[20/0.030280549289164%] 24 ||| 26 ||| 0 ||| 4 ||| 12 ||| 0.4.12
⁃	[50/0.07570137322291%] 32 ||| 28 ||| 1 ||| 1 ||| 8 ||| 1.1.8-af903375-Android
⁃	[894/1.3535405532256%] 27 ||| 26 ||| 1 ||| 1 ||| 4 ||| 1.1.4--Android
⁃	[1/0.0015140274644582%] 28 ||| 26 ||| 0 ||| 4 ||| 14 ||| 0.4.14-dev
⁃	[167/0.25284258656452%] 30 ||| 26 ||| 0 ||| 4 ||| 15 ||| 0.4.15-3ad68c05-Android
⁃	[1/0.0015140274644582%] 32 ||| 28 ||| 0 ||| 4 ||| 16 ||| 0.4.16-dev-10.11.6-816bca3
⁃	[28/0.04239276900483%] 32 ||| 28 ||| 0 ||| 4 ||| 18 ||| 0.4.18.4-ae91f61 MT unofficial 
⁃	[7/0.010598192251207%] 32 ||| 28 ||| 1 ||| 3 ||| 2 ||| 1.3.2
⁃	[5285/8.0016351496616%] 32 ||| 28 ||| 1 ||| 3 ||| 0 ||| 1.3.0
⁃	[90/0.13626247180124%] 32 ||| 28 ||| 0 ||| 4 ||| 16 ||| 0.4.16--Android
⁃	[3495/5.2915259882814%] 32 ||| 28 ||| 1 ||| 1 ||| 10 ||| 1.1.10-359e7472-Android
⁃	[91/0.1377764992657%] 32 ||| 28 ||| 1 ||| 1 ||| 4 ||| 1.1.4
⁃	[373/0.56473224424291%] 32 ||| 28 ||| 1 ||| 1 ||| 10 ||| 1.1.10-d90d387a-Android
⁃	[1/0.0015140274644582%] 28 ||| 26 ||| 0 ||| 4 ||| 14 ||| 0.4.14.8-2a346ab5-Android-armeabi-v7a
⁃	[1/0.0015140274644582%] 32 ||| 28 ||| 0 ||| 4 ||| 17 ||| 0.4.17-4a48cd57-Android
⁃	[806/1.2203061363533%] 26 ||| 26 ||| 0 ||| 4 ||| 13 ||| 0.4.13
⁃	[257/0.38910505836576%] 32 ||| 28 ||| 1 ||| 2 ||| 0 ||| 1.2.0
⁃	[50/0.07570137322291%] 24 ||| 26 ||| 0 ||| 4 ||| 11 ||| 0.4.11-dirty
⁃	[1/0.0015140274644582%] 28 ||| 26 ||| 0 ||| 4 ||| 15 ||| 0.4.15-231ac33d-Android
⁃	[873/1.321745976472%] 32 ||| 28 ||| 0 ||| 4 ||| 17 ||| 0.4.17 MT official 
⁃	[8/0.012112219715666%] 32 ||| 28 ||| 0 ||| 4 ||| 17 ||| 0.4.17-2d523754-Android
⁃	[22/0.033308604218081%] 32 ||| 28 ||| 0 ||| 4 ||| 17 ||| 0.4.17.1-d25a7147
⁃	[1274/1.9288709897198%] 32 ||| 28 ||| 0 ||| 4 ||| 17 ||| 0.4.17.1 MT official 
⁃	[1/0.0015140274644582%] 28 ||| 26 ||| 0 ||| 4 ||| 14 ||| 0.4.14-74a92926-Android
⁃	[565/0.85542551741889%] 32 ||| 28 ||| 1 ||| 1 ||| 8 ||| 1.1.8-4cd987b6-Android

If you know more about some of this clients on the list please just edit my post or ask me to edit.

We should really inform these Forks about Minetest they are more then 65%

If someone wants to try out just use my fork branch version_string4 for 0.4.17 or version_string5 for 5.0.1.
You can also use my client_matcher mod to list all different versions.

paramat · 2019-08-11T22:13:06Z

With or without ser_vers in non debug mode?

To be clear, i support what sfan5 suggests:

bugfix, allowing serialization_version in non-debug mode

SmallJoker does to (see emoticons) so that will probably be merged.

src/script/lua_api/l_server.cpp

ClobberXD · 2019-08-17T09:10:18Z

👍 for the bugfix

ClobberXD · 2019-08-17T09:11:09Z

doc/lua_api.txt

+          major = 0,                 -- major version number
+          minor = 4,                 -- minor version number
+          patch = 10,                -- patch version number
+          version_string = "0.4.9-git", -- full version string


This needs updating - most of these fields are now unavailable in Release builds again.

Lejo1 · 2019-12-20T16:43:42Z

Obviously no Core-dev for the other information.
So just the bugfix.

* Give more player information * Correct lua_api.txt * Correct keys in lua_api.txt * Improve Code * Only Bugfix+ser_vers * Correct doc * Fix double

Give more player information

168eec3

Lejo1 changed the title ~~Give more player informationto~~ Give more player information Jun 22, 2019

paramat added @ Server / Client / Env. Feature ✨ PRs that add or enhance a feature labels Jun 22, 2019

nerzhul requested changes Jul 11, 2019

View reviewed changes

src/clientiface.h Outdated Show resolved Hide resolved

Correct lua_api.txt

4edab8a

Correct keys in lua_api.txt

c3cc84e

paramat added the Bugfix 🐛 PRs that fix a bug label Jul 24, 2019

Lejo1 closed this Jul 27, 2019

Lejo1 reopened this Jul 27, 2019

Improve Code

e6f2f9e

paramat closed this Aug 10, 2019

paramat reopened this Aug 10, 2019

paramat added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Aug 10, 2019

paramat removed the Possible close label Aug 11, 2019

Only Bugfix+ser_vers

c4308e8

paramat removed Action / change needed Code still needs changes (PR) / more information requested (Issues) Controversial labels Aug 12, 2019

ClobberXD reviewed Aug 17, 2019

View reviewed changes

src/script/lua_api/l_server.cpp Outdated Show resolved Hide resolved

ClobberXD reviewed Aug 17, 2019

View reviewed changes

Lejo1 added 2 commits December 20, 2019 17:40

Correct doc

b550350

Resolve

be5041d

Fix double

6b00dfc

SmallJoker approved these changes Dec 20, 2019

View reviewed changes

SmallJoker changed the title ~~Give more player information~~ Fix the player information version_string return value Dec 20, 2019

SmallJoker added Trivial The change is a trivial bug fix, documentation or maintenance change, as per the Git Guidelines and removed Feature ✨ PRs that add or enhance a feature labels Dec 20, 2019

SmallJoker merged commit 37f771a into minetest:master Dec 20, 2019

Lejo1 deleted the version_string branch February 16, 2020 15:35

Bastrabun mentioned this pull request Sep 5, 2020

Increment formspec/protocol version at every release to indicate client version #10368

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the player information version_string return value #8616

Fix the player information version_string return value #8616

Lejo1 commented Jun 22, 2019 •

edited by SmallJoker

Loading

SmallJoker commented Jun 22, 2019

Lejo1 commented Jun 22, 2019 •

edited

Loading

AKryukov92 commented Jul 9, 2019

ClobberXD commented Jul 9, 2019 •

edited

Loading

sfan5 commented Jul 9, 2019 •

edited

Loading

AKryukov92 commented Jul 9, 2019

Lejo1 commented Jul 9, 2019

ClobberXD commented Jul 9, 2019

AKryukov92 commented Jul 9, 2019 •

edited

Loading

ClobberXD commented Jul 11, 2019 •

edited

Loading

Lejo1 commented Jul 11, 2019 •

edited

Loading

ClobberXD commented Jul 24, 2019

paramat commented Jul 24, 2019 •

edited

Loading

SmallJoker commented Jul 25, 2019 •

edited

Loading

paramat commented Jul 25, 2019

AKryukov92 commented Jul 25, 2019

Lejo1 commented Jul 25, 2019 •

edited

Loading

sfan5 commented Jul 25, 2019

Lejo1 commented Jul 27, 2019 •

edited

Loading

ClobberXD commented Jul 27, 2019 •

edited

Loading

ClobberXD commented Aug 9, 2019

ClobberXD commented Aug 9, 2019

rubenwardy commented Aug 9, 2019

paramat commented Aug 10, 2019 •

edited

Loading

Lejo1 commented Aug 11, 2019

Lejo1 commented Aug 11, 2019

paramat commented Aug 11, 2019

ClobberXD commented Aug 17, 2019

ClobberXD Aug 17, 2019

Lejo1 commented Dec 20, 2019

Fix the player information version_string return value #8616

Fix the player information version_string return value #8616

Conversation

Lejo1 commented Jun 22, 2019 • edited by SmallJoker Loading

What for?

How to test

SmallJoker commented Jun 22, 2019

Lejo1 commented Jun 22, 2019 • edited Loading

AKryukov92 commented Jul 9, 2019

ClobberXD commented Jul 9, 2019 • edited Loading

sfan5 commented Jul 9, 2019 • edited Loading

AKryukov92 commented Jul 9, 2019

Lejo1 commented Jul 9, 2019

ClobberXD commented Jul 9, 2019

AKryukov92 commented Jul 9, 2019 • edited Loading

ClobberXD commented Jul 11, 2019 • edited Loading

Lejo1 commented Jul 11, 2019 • edited Loading

ClobberXD commented Jul 24, 2019

paramat commented Jul 24, 2019 • edited Loading

SmallJoker commented Jul 25, 2019 • edited Loading

paramat commented Jul 25, 2019

AKryukov92 commented Jul 25, 2019

Lejo1 commented Jul 25, 2019 • edited Loading

sfan5 commented Jul 25, 2019

Lejo1 commented Jul 27, 2019 • edited Loading

ClobberXD commented Jul 27, 2019 • edited Loading

ClobberXD commented Aug 9, 2019

ClobberXD commented Aug 9, 2019

rubenwardy commented Aug 9, 2019

paramat commented Aug 10, 2019 • edited Loading

Lejo1 commented Aug 11, 2019

Lejo1 commented Aug 11, 2019

paramat commented Aug 11, 2019

ClobberXD commented Aug 17, 2019

ClobberXD Aug 17, 2019

Choose a reason for hiding this comment

Lejo1 commented Dec 20, 2019

Lejo1 commented Jun 22, 2019 •

edited by SmallJoker

Loading

Lejo1 commented Jun 22, 2019 •

edited

Loading

ClobberXD commented Jul 9, 2019 •

edited

Loading

sfan5 commented Jul 9, 2019 •

edited

Loading

AKryukov92 commented Jul 9, 2019 •

edited

Loading

ClobberXD commented Jul 11, 2019 •

edited

Loading

Lejo1 commented Jul 11, 2019 •

edited

Loading

paramat commented Jul 24, 2019 •

edited

Loading

SmallJoker commented Jul 25, 2019 •

edited

Loading

Lejo1 commented Jul 25, 2019 •

edited

Loading

Lejo1 commented Jul 27, 2019 •

edited

Loading

ClobberXD commented Jul 27, 2019 •

edited

Loading

paramat commented Aug 10, 2019 •

edited

Loading