Mineclonia not starting in Minetest 5.5-dev due to faulty find_nodes_in_area() bugfix in Minetest #183

New Issue

erlehmann · 2021-11-28T00:18:16+01:00

erlehmann commented

2021-11-28 00:18:16 +01:00

What happened?

In response to the crash from #165 the Minetest core developers changed minetest.find_nodes_in_area() to ignore areas outside of coordinates that they think the map generator can actually generate, which is ±31000. Note that in vanilla Minetest the visible map can actually extend beyond that point.

Despite a lot of warnings from my side, they merged the faulty bugfix – one that prevents the crash, but introduces numerous subtle other problems due to using the wrong boundary, it is trivial for mods, players or map imports to place nodes outside the generated maps.

The PR #169 includes a self test at game startup that tests if minetest.find_nodes_in_area() can actually find all nodes that the test places. The faulty bugfix for minetest.find_nodes_in_area() makes an assertion in that test case fail.

What did I expect?

I expected Minetest core developers to actually know how their engine works at the map boundaries.

I expected Minetest core developers to take my warnings seriously.

Apparently I was way too optimistic.

How to get it to happen

Enter a Mineclonia world withMineclonia commit 4b9094ddc2 in Minetest commit 87ab97da2.
Note that the game does not start due to a failed assertion.

Environment

Mineclonia commit 4b9094ddc2
Minetest commit 87ab97da2

##### What happened? In response to the crash from https://git.minetest.land/Mineclonia/Mineclonia/issues/165 the Minetest core developers changed `minetest.find_nodes_in_area()` to ignore areas outside of coordinates that *they think* the map generator can actually generate, which is ±31000. Note that in vanilla Minetest the visible map can actually extend beyond that point. Despite a lot of warnings from my side, they merged the faulty bugfix – one that prevents the crash, but introduces numerous subtle other problems due to using the wrong boundary, it is trivial for mods, players or map imports to place nodes outside the generated maps. The PR https://git.minetest.land/Mineclonia/Mineclonia/pulls/169 includes a self test at game startup that tests if `minetest.find_nodes_in_area()` can actually find all nodes that the test places. The faulty bugfix for `minetest.find_nodes_in_area()` makes an assertion in that test case fail. ##### What did I expect? I expected Minetest core developers to actually know how their engine works at the map boundaries. I expected Minetest core developers to take my warnings seriously. Apparently I was way too optimistic. ##### How to get it to happen 1. Enter a Mineclonia world withMineclonia commit 4b9094ddc20b77ff134184612de8b49b2de18c05 in Minetest commit 87ab97da2. 2. Note that the game does not start due to a failed assertion. ##### Environment Mineclonia commit 4b9094ddc20b77ff134184612de8b49b2de18c05 Minetest commit 87ab97da2

erlehmann added the

bug

unconfirmed

labels 2021-11-28 00:18:16 +01:00

erlehmann commented

2021-11-28 00:24:29 +01:00

TL;DR: The self test fails due to the Minetest engine now being buggy around coordinates with x/y/z > ±31000. Basically, nodes can exist in those coordinates, but they will not be found by minetest.find_nodes_in_area().

TL;DR: The self test fails due to the Minetest engine now being buggy around coordinates with x/y/z > ±31000. Basically, nodes can exist in those coordinates, but they will not be found by `minetest.find_nodes_in_area()`.

cora commented

2021-11-28 11:48:35 +01:00

this is kind of funny ngl.

cora referenced this issue

2021-12-14 00:40:09 +01:00

MISC/mcl_selftests: Add a setting to opt-in for selftests #213

cora commented

2021-12-14 00:46:14 +01:00

TL;DR: The self test fails due to the Minetest engine now being buggy around coordinates with x/y/z > ±31000. Basically, nodes can exist in those coordinates, but they will not be found by minetest.find_nodes_in_area().

While i suppose you're right about nodes being able to exist there, you have to see that there is no evidence of find_nodes_in_area not working there being a problem – having the game crash on startup however: kind of a problem (evidence: the existence of this issue) If this proves anything it's that these tests should be opt-in. ( #213 )

> TL;DR: The self test fails due to the Minetest engine now being buggy around coordinates with x/y/z > ±31000. Basically, nodes can exist in those coordinates, but they will not be found by `minetest.find_nodes_in_area()`. While i suppose you're right about nodes being able to exist there, you have to see that there is no evidence of find_nodes_in_area not working there being a problem – having the game crash on startup however: kind of a problem (evidence: the existence of this issue) If this proves anything it's that these tests should be opt-in. ( #213 )

erlehmann commented

2021-12-14 10:07:30 +01:00

While i suppose you're right about nodes being able to exist there, you have to see that there is no evidence of find_nodes_in_area not working there being a problem

Lots of code in Mineclonia places nodes and then uses minetest.find_nodes_in_area() to find those exact nodes – which is what the test tries to do too.

Among the mechanics that use minetest.find_nodes_in_area() and could possible interact with coordinates beyond 31000 are:

being on fire
lava cooling
dirt spread
vine spread
ice walker enchantment
sugar cane growth
bone meal apply
dragon egg teleportation
farmland becoming wet farmland or becoming dirt
fire spread through fire
fire sound
mob spawners
mushroom spread
nether portals
potions
glass panes connection
structure placement in the map generator (desert well, igloo, fossil, witch hut, ice spikes in mapgen v6, jungle trees)
fixup code in the map generator (fixes for double plants, color of grass blocks, snow fixes)
nether map generation
structure placement in the map generator using /spawnstruct (witch hut, end shrine & end portal, desert temple)
village generator
finding a safe spawn point (admittedly, this is a bit of a stretch)

I am confident that at least some of those will create opportunities for griefers to crash servers in hard to diagnose ways or maybe just create natural sources of lag or crashes, in cases where someone is careless enough to actually run Mineclonia on a Minetest 5.5-dev server.

Are you confident that none of those will be a problem?

having the game crash on startup however: kind of a problem

Do not shoot the messenger. I think it is perfectly fine to not start the game with an engine version that breaks over 20 mechanics. IIRC no one – you and me included – assumed that the Minetest devs would realistically break the function.

I propose to fix it in the engine. The comment near that assertion also asks to complain to the Minetest engine devs in case the test ever fails. Have you done that or am I the only one so far?

> While i suppose you're right about nodes being able to exist there, you have to see that there is no evidence of find_nodes_in_area not working there being a problem Lots of code in Mineclonia places nodes and then uses `minetest.find_nodes_in_area()` to find those exact nodes – which is what the test tries to do too. Among the mechanics that use `minetest.find_nodes_in_area()` and could possible interact with coordinates beyond 31000 are: * being on fire * lava cooling * dirt spread * vine spread * ice walker enchantment * sugar cane growth * bone meal apply * dragon egg teleportation * farmland becoming wet farmland or becoming dirt * fire spread through fire * fire sound * mob spawners * mushroom spread * nether portals * potions * glass panes connection * structure placement in the map generator (desert well, igloo, fossil, witch hut, ice spikes in mapgen v6, jungle trees) * fixup code in the map generator (fixes for double plants, color of grass blocks, snow fixes) * nether map generation * structure placement in the map generator using `/spawnstruct` (witch hut, end shrine & end portal, desert temple) * village generator * finding a safe spawn point (admittedly, this is a bit of a stretch) I am confident that at least some of those will create opportunities for griefers to crash servers in hard to diagnose ways or maybe just create natural sources of lag or crashes, in cases where someone is careless enough to actually run Mineclonia on a Minetest 5.5-dev server. Are you confident that *none* of those will be a problem? > having the game crash on startup however: kind of a problem Do not shoot the messenger. I think it is perfectly fine to not start the game with an engine version that breaks over 20 mechanics. IIRC no one – you and me included – assumed that the Minetest devs would realistically break the function. I propose to fix it in the engine. The comment near that assertion also asks to complain to the Minetest engine devs in case the test ever fails. Have you done that or am I the only one so far?

erlehmann commented

2021-12-14 10:39:23 +01:00

I decided to investigate the lava cooling. It seems that flowing lava touching water beyond x=31000 may be a problem (crash).

Note that the map generator can actually do this on its own.

I decided to investigate the lava cooling. It seems that flowing lava touching water beyond x=31000 may be a problem (crash). Note that the map generator can actually do this on its own.

cora commented

2021-12-14 11:11:44 +01:00

well the tests make it worse

cora commented

2021-12-14 11:14:16 +01:00

I suppose you just want to keep it broken then because you're so right.

cora commented

2021-12-14 11:16:04 +01:00

Among the things not working due to this:

EVERYTHING

Among the things not working due to this: EVERYTHING

LizzyFleckenstein03 commented

2021-12-14 13:17:38 +01:00

"with focus on stability (less crashes)..."

erlehmann commented

2021-12-14 13:51:36 +01:00

@cora and me chatted about the issue this morning. To reiterate my talking points for others:

I put in the assertion to prevent the game from starting in a known to be buggy state. If the game is started with a buggy engine, it will exhibit buggy behaviour at the map edge in the over 20 features I listed above. Only after I did this did sfan5 actually change Minetest, against my multiple warnings that his solution is wrong. I was not aware that among Minetest core devs “fixing” signed integer overflow is usually done by coming up with a poorly thought-out ad-hoc solution. I know more nowadays – in particular that celeron55 seems to think that checking for signed integer overflow makes software too slow (IMO this is bullshit).
I agree the current solution is bad. In particular, the assertion does not even have an error message! Only by looking at the source code it becomes clear that the problem lies with the engine placing nodes it can not find.
The affected versions of Minetest 5.5-dev have minetest.find_nodes_in_area() not working correctly for about 15% of possible v3s16 coordinates – I calculated this as ( ( 2¹⁶ )³ - ( 31000×2 )³ ) / ( ( 2¹⁶ )³ ). From the set of coordinates that do not work correctly, only 0,05% are wrong in all cases (i.e. the Minetest map generator can and will place nodes there) – I calculated this as ( ( 31006×2 )³ - ( 31000×2 )³ ) / ( 31000×2 )³. Unfortunately, those positions consist of a shell around the map and it is trivial to trigger buggy behaviour (just show up near the map border).
Nevertheless, for debugging purposes it is a good idea to turn tests off. I think self tests should be opt-out though, not opt-in.
I think it is my responsibility to fix the crash (arguably, I introduced it) and I have a plan for doing so, consisting of several steps:

I plan to convince others that the default for tests should be that checks are opt-out, not opt-in. If someone wants to run a known-to-be-buggy engine release, they should make a conscious effort to do so. So far, only developers and people who wanted to test unreleased Minetest engine versions had a need for disabling the tests: I had a need for this, sfan5 had a need for this, apparently @cora and @EliasFleckenstein03 have a need for this too.
I plan to propose an error message to the assertion. This is orthogonal to everything else. @cora had complained about my assertions not having any error messages and I brushed it aside. That was a mistake.
I plan to make a PR for the Minetest engine to change the limits to the limits of v3s16. This is a very simple endeavour and its success mostly depends on if I find any core devs that know how to fix a signed integer overflow.
If the previous step fails, I plan to reimplement the buggy parts of minetest.find_nodes_in_area() using a voxelmanip, so that the test can stay as it is.
If the previous step fails, I plan to change the test so that it does not fail – and open bugs against every feature that does not work anymore near the map border. I would like to avoid this.

@cora and me chatted about the issue this morning. To reiterate my talking points for others: * **I put in the assertion to prevent the game from starting in a known to be buggy state.** If the game is started with a buggy engine, it will exhibit buggy behaviour at the map edge in the over 20 features I listed above. Only after I did this did sfan5 actually change Minetest, against my multiple warnings that his solution is wrong. I was not aware that among Minetest core devs “fixing” signed integer overflow is usually done by coming up with a poorly thought-out ad-hoc solution. I know more nowadays – in particular that celeron55 seems to think that checking for signed integer overflow makes software too slow (IMO this is bullshit). * I agree the current solution is bad. In particular, the assertion does not even have an error message! Only by looking at the source code it becomes clear that the problem lies with the engine placing nodes it can not find. * The affected versions of Minetest 5.5-dev have `minetest.find_nodes_in_area()` not working correctly for about 15% of possible `v3s16` coordinates – I calculated this as `( ( 2¹⁶ )³ - ( 31000×2 )³ ) / ( ( 2¹⁶ )³ )`. From the set of coordinates that do not work correctly, only 0,05% are wrong in all cases (i.e. the Minetest map generator can and will place nodes there) – I calculated this as ( ( 31006×2 )³ - ( 31000×2 )³ ) / ( 31000×2 )³. Unfortunately, those positions consist of a shell around the map and it is trivial to trigger buggy behaviour (just show up near the map border). * Nevertheless, for debugging purposes it is a good idea to turn tests off. I think self tests should be opt-out though, not opt-in. * I think it is my responsibility to fix the crash (arguably, I introduced it) and I have a plan for doing so, consisting of several steps: 1. I plan to convince others that the default for tests should be that checks are opt-out, not opt-in. If someone wants to run a known-to-be-buggy engine release, they should make a conscious effort to do so. So far, only developers and people who wanted to test unreleased Minetest engine versions had a need for disabling the tests: I had a need for this, sfan5 had a need for this, apparently @cora and @EliasFleckenstein03 have a need for this too. 2. I plan to propose an error message to the assertion. This is orthogonal to everything else. @cora had complained about my assertions not having any error messages and I brushed it aside. That was a mistake. 3. I plan to make a PR for the Minetest engine to change the limits to the limits of v3s16. This is a very simple endeavour and its success mostly depends on if I find any core devs that know how to fix a signed integer overflow. 4. If the previous step fails, I plan to reimplement the buggy parts of `minetest.find_nodes_in_area()` using a voxelmanip, so that the test can stay as it is. 5. If the previous step fails, I plan to change the test so that it does not fail – and open bugs against every feature that does not work anymore near the map border. I would like to avoid this.

erlehmann changed title from ~~Mineclonia not starting due to faulty find_nodes_in_area() bugfix in Minetest~~ to Mineclonia not starting in Minetest 5.5-dev due to faulty find_nodes_in_area() bugfix in Minetest

2021-12-14 14:47:10 +01:00

erlehmann commented

2021-12-14 15:28:00 +01:00

I have sent Minetest a patch: https://github.com/minetest/minetest/issues/11828#issuecomment-993594718

From 1ab3d4f6153b9ddf713206c201e0330ae8216ac3 Mon Sep 17 00:00:00 2001
From: Nils Dagsson Moskopp <nils@dieweltistgarnichtso.net>
Date: Tue, 14 Dec 2021 14:06:31 +0100
Subject: [PATCH] Fix minetest.find_nodes_in_area() coord clamping
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Previously, minetest.find_nodes_in_area() was affected by a signed
integer overflow, which was fixed by clamping the area in which the
function works to MAX_MAP_GENERATION_LIMIT & -MAX_MAP_GENERATION_LIMIT.

The problem with that approach is that nodes can exist and even be generated
past MAX_MAP_GENERATION_LIMIT in vanilla Minetest – which, despite the name,
does not specify exactly where the map generator stops working. Therefore,
the bug fix created an unknown amount of other bugs near the map border.

At minimum, those bugs affect the outer mapblocks, where nodes past
MAX_MAP_GENERATION_LIMIT can be generated. Ironically, the first thing
broken by the faulty bug fix was a test case belonging to a workaround that
prevents minetest.find_nodes_in_area() being called with coordinates out of
s16 bounds in the Mineclonia game, thus avoiding signed integer overflow.

Using INT16_MIN+1 & INT16_MAX-1 makes minetest.find_nodes_in_area()
work for all coordinates that worked before the faulty fix, avoiding a
situation where Minetest can place a node in the map, but then not find
it afterwards using minetest_find_nodes_in_area().
---
 src/script/lua_api/l_env.cpp | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/script/lua_api/l_env.cpp b/src/script/lua_api/l_env.cpp
index 18ee3a521..ccb18d992 100644
--- a/src/script/lua_api/l_env.cpp
+++ b/src/script/lua_api/l_env.cpp
@@ -888,8 +888,9 @@ static void checkArea(v3s16 &minp, v3s16 &maxp)
 		throw LuaError("Area volume exceeds allowed value of 4096000");
 	}

-	// Clamp to map range to avoid problems
-#define CLAMP(arg) core::clamp(arg, (s16)-MAX_MAP_GENERATION_LIMIT, (s16)MAX_MAP_GENERATION_LIMIT)
+	// Clamp to almost s16 range to avoid problems
+	// Clamping to exactly s16 range hangs Minetest
+#define CLAMP(arg) core::clamp(arg, (s16)(INT16_MIN+1), (s16)(INT16_MAX-1))
 	minp = v3s16(CLAMP(minp.X), CLAMP(minp.Y), CLAMP(minp.Z));
 	maxp = v3s16(CLAMP(maxp.X), CLAMP(maxp.Y), CLAMP(maxp.Z));
 #undef CLAMP
-- 
2.20.1

I have sent Minetest a patch: https://github.com/minetest/minetest/issues/11828#issuecomment-993594718 ``` From 1ab3d4f6153b9ddf713206c201e0330ae8216ac3 Mon Sep 17 00:00:00 2001 From: Nils Dagsson Moskopp <nils@dieweltistgarnichtso.net> Date: Tue, 14 Dec 2021 14:06:31 +0100 Subject: [PATCH] Fix minetest.find_nodes_in_area() coord clamping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, minetest.find_nodes_in_area() was affected by a signed integer overflow, which was fixed by clamping the area in which the function works to MAX_MAP_GENERATION_LIMIT & -MAX_MAP_GENERATION_LIMIT. The problem with that approach is that nodes can exist and even be generated past MAX_MAP_GENERATION_LIMIT in vanilla Minetest – which, despite the name, does not specify exactly where the map generator stops working. Therefore, the bug fix created an unknown amount of other bugs near the map border. At minimum, those bugs affect the outer mapblocks, where nodes past MAX_MAP_GENERATION_LIMIT can be generated. Ironically, the first thing broken by the faulty bug fix was a test case belonging to a workaround that prevents minetest.find_nodes_in_area() being called with coordinates out of s16 bounds in the Mineclonia game, thus avoiding signed integer overflow. Using INT16_MIN+1 & INT16_MAX-1 makes minetest.find_nodes_in_area() work for all coordinates that worked before the faulty fix, avoiding a situation where Minetest can place a node in the map, but then not find it afterwards using minetest_find_nodes_in_area(). --- src/script/lua_api/l_env.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/script/lua_api/l_env.cpp b/src/script/lua_api/l_env.cpp index 18ee3a521..ccb18d992 100644 --- a/src/script/lua_api/l_env.cpp +++ b/src/script/lua_api/l_env.cpp @@ -888,8 +888,9 @@ static void checkArea(v3s16 &minp, v3s16 &maxp) throw LuaError("Area volume exceeds allowed value of 4096000"); } - // Clamp to map range to avoid problems -#define CLAMP(arg) core::clamp(arg, (s16)-MAX_MAP_GENERATION_LIMIT, (s16)MAX_MAP_GENERATION_LIMIT) + // Clamp to almost s16 range to avoid problems + // Clamping to exactly s16 range hangs Minetest +#define CLAMP(arg) core::clamp(arg, (s16)(INT16_MIN+1), (s16)(INT16_MAX-1)) minp = v3s16(CLAMP(minp.X), CLAMP(minp.Y), CLAMP(minp.Z)); maxp = v3s16(CLAMP(maxp.X), CLAMP(maxp.Y), CLAMP(maxp.Z)); #undef CLAMP -- 2.20.1 ```

LizzyFleckenstein03 commented

2021-12-14 16:13:39 +01:00

I really don't want to make a controversy about a problem like this which had minor consequences. But I think it shows several attitudes that I think are damaging to the MineClone2/Mineclonia community and that I want to address by using this case as an example.

The criticism is not limited to @erlehmann only.

Before I start my rant however, I want to point out that I think we all agree that sfan5's pull request was faulty and the find_nodes_in_area is currently in a bugged state. Nevertheless, I think that what you did here was wrong.

The main problem here is taking responsibility for something that we can't possibly control. What a selftest like this is essentially trying to do is solve engine issues. MineClone2/Mineclonia cannot and should not attempt to solve (or prevent) engine problems, especially not problems in unreleased engine versions.

If the engine is buggy, that's the engine's problem. There are a lot of engine bugs that Mineclonia does not test at all for, and it is impossible to make the game only start whenever the engine is in a buggy state.

Minetest has > 1k issues open, 268 of them are bugs. The only logical consequence here is to not allow Mineclonia to run until there are no engine bugs.

Taking responsibility for engine issues is malicious in several ways:

It gives a false illusion to users: users may believe that something (engine stability) is ensured by our work (MineClone2/Mineclonia), which our work is simply incapable of ensuring.
It creates the idea of us game developers actually being responsible for engine stability. This is discouraging for anyone who contributes to MineClone2/Mineclonia, since they now have to fear being held responsible for problems that are caused by a piece of code (Minetest) that they are not in control of.
Developers might feel motivated to try to fix something (using game code) that they can't properly solve, resulting in a lot of work (Take #198 or #200 as example) for something that, if fixed in the right place, takes a single line of code. This is highly discouraging.

The second problem is the motivation behind your actions. As already mentioned, it is impossible to make Mineclonia reliably fail to start with a buggy engine. In some cases however, the impact of the engine bug might be so big (e.g. permanent loss of worlds etc.) that it is justified to take whatever actions are needed to prevent harm.

HOWEVER, it can hardly be said that this is the case here. The creation of the problematic test was at least partly motivated by the will to discredit the Minetest developers for applying a patch that you thought was wrong.

The engine patch is not directly related to Mineclonia. It affects Mineclonia servers as well as minetest_game servers and basically any Minetest content that uses find_nodes_in_area.

Your patch does not benefit Mineclonia users, and your patch harms them because their game does not start.

A malicious patch was applied for egoistical reasons, and you were dishonest about your intentions.

Being egoistic as developers is something that we often like to deny, but I don't think it's good if we're dishonest with each other in the "aftermath", so I'll make the first step and name a few times I myself have not been neutral:

I have patched dupes tactially in the past (During the war with the empire on Clamity, I patched the anvil dupes in MineClone2).
When working on enchantments, I originally forgot the privs for /forceenchant intentionally, but Wuzzy noticed it and fixed it without a comment.
Recently, when the removal of "fapple" speed was discussed, I mainly just wanted to keep the speed effect because I personally liked it that way (and I felt like it improved the game for other people as well). It may not exactly have been egoistic, but certainly egocentric ("I like it, so other people will like it too").

Everyone is egoistic or egocentric once in a while, and while I don't want to justify what I did, applying patches that are entirely harmful on top of that is crossing a bounary.

I believe that your motivation might partly have also been to protect the users. But in that case, such a patch would still have been an exaggerated reaction to the partially buggy state of the engine. Note that your patch could have possibly affected a lot of worlds that are not vulnerable to the engine bug at all (e.g. singleplayer worlds). Enforcing a selftest that knowingly fails, even with a good intend is harmful because:

It takes away the user's freedom to decide to run the game anyway if their priorities differ from yours (this is being changed by adding an option)
If there is a test that always fails and the game runs perfectly fine if it is disabled, it teaches the users that tests are useless and that it's fine to just disable them.

Reasons on why I still think that you were (partly) egoistially motivated:
There is not a single known fatal problem with the clamping issue, and you are evidently skilled at finding concrete ways to exploit faulty code. As someone who always wants proof before taking action, you claiming that there is going to be problems is not very credible if you can't name an example.
You might argue that "there exists an issue unless proven otherwise", but when there are two issues to compare to each other, it does not make sense to prefer breaking game startup (proven fatal issue) over possible other fatal issues (speculation), especially if the probability of a permanent issue (e.g. wold corruption) is somewhat low.
There are far more severe issues in the engine that can lead to breakage; take https://github.com/minetest/minetest/issues/11742 for example. Yet, no selftest was introduced for these.
Your claim that you would not have expected the minetest devs to "break the engine in such ways" is invalid and I don't see why you actually thought we'd believe that. You made a test about it, so of course you were expecting the test to eventually trigger. You KNEW about sfan5's PR, it was the motivation to make the test in the first place. If you would have not considered the possibility of the PR being merged, you wouldn't have made the test in the first place.
When the PR got merged, which you noticed, you did not disable the test or anything. You were fully aware of very likely breaking the game by making such a test. And you were also aware that you exaggerated the consequences of the engine breakage that would have resulted otherwise.

Conclusion

If you, @erlehmann want to take responsibility for engine problems, I highly encourage you to fork Minetest. You have done this in the past as well. I offer my help in contributing and maintaining that fork. Ngl, sfan5's attitude is fucked.
But please, don't harm the community we both care about by trying to fix engine problems with game code. This also applies to cora and me, who took part in the recent anticheat microhype. Let's fix the problems where they should be fixed.

And erlehmann, I think that you should try to be more honest and neutral. This is not the first time that I think you have tried to discourage someone or made unfair or dishonest claims (for example I don't think that MineClone2/Mineclonia comparison is a topic that you deal with neutrally). I think you are a really cool person and I like hanging out with you on IRC - I also really appreciate the work that you are doing for the MineClone2/Mineclonia community. Therefore I feel bad about publicly pointing out what I think was a mistake of yours, but I think it had to be addressed.

I really don't want to make a controversy about a problem like this which had minor consequences. But I think it shows several attitudes that I think are damaging to the MineClone2/Mineclonia community and that I want to address by using this case as an example. The criticism is not limited to @erlehmann only. Before I start my rant however, I want to point out that I think we all agree that sfan5's pull request was faulty and the find_nodes_in_area is currently in a bugged state. Nevertheless, I think that what you did here was wrong. The main problem here is **taking responsibility for something that we can't possibly control.** What a selftest like this is essentially trying to do is solve engine issues. MineClone2/Mineclonia cannot and should not attempt to solve (or prevent) engine problems, _especially_ not problems in unreleased engine versions. If the engine is buggy, that's the engine's problem. There are a lot of engine bugs that Mineclonia does not test at all for, and it is impossible to make the game only start whenever the engine is in a buggy state. Minetest has > 1k issues open, 268 of them are bugs. The only logical consequence here is to not allow Mineclonia to run until there are no engine bugs. Taking responsibility for engine issues is malicious in several ways: 1. It gives a false illusion to users: users may believe that something (engine stability) is ensured by our work (MineClone2/Mineclonia), which our work is simply incapable of ensuring. 2. It creates the idea of us game developers actually being responsible for engine stability. This is discouraging for anyone who contributes to MineClone2/Mineclonia, since they now have to fear being held responsible for problems that are caused by a piece of code (Minetest) that they are not in control of. 3. Developers might feel motivated to try to fix something (using game code) that they can't properly solve, resulting in a lot of work (Take #198 or #200 as example) for something that, if fixed in the right place, takes a single line of code. This is highly discouraging. The second problem is **the motivation behind your actions.** As already mentioned, it is impossible to make Mineclonia reliably fail to start with a buggy engine. In some cases however, the impact of the engine bug might be so big (e.g. permanent loss of worlds etc.) that it is justified to take whatever actions are needed to prevent harm. _HOWEVER_, it can hardly be said that this is the case here. The creation of the problematic test was at least partly motivated by the will to discredit the Minetest developers for applying a patch that you thought was wrong. The engine patch is not directly related to Mineclonia. It affects Mineclonia servers as well as minetest_game servers and basically any Minetest content that uses find_nodes_in_area. Your patch does not benefit Mineclonia users, and your patch harms them because their game does not start. **A malicious patch was applied for egoistical reasons, and you were dishonest about your intentions.** Being egoistic as developers is something that we often like to deny, but I don't think it's good if we're dishonest with each other in the "aftermath", so I'll make the first step and name a few times I myself have not been neutral: - I have patched dupes tactially in the past (During the war with the empire on Clamity, I patched the anvil dupes in MineClone2). - When working on enchantments, I originally forgot the privs for /forceenchant intentionally, but Wuzzy noticed it and fixed it without a comment. - Recently, when the removal of "fapple" speed was discussed, I mainly just wanted to keep the speed effect because I personally liked it that way (and I felt like it improved the game for other people as well). It may not exactly have been egoistic, but certainly egocentric ("I like it, so other people will like it too"). Everyone is egoistic or egocentric once in a while, and while I don't want to justify what I did, applying patches that are entirely harmful on top of that is crossing a bounary. I believe that your motivation might partly have also been to protect the users. But in that case, such a patch would still have been an exaggerated reaction to the partially buggy state of the engine. Note that your patch could have possibly affected a lot of worlds that are not vulnerable to the engine bug at all (e.g. singleplayer worlds). Enforcing a selftest that knowingly fails, even with a good intend is harmful because: 1. It takes away the user's freedom to decide to run the game anyway if their priorities differ from yours (this is being changed by adding an option) 2. If there is a test that always fails and the game runs perfectly fine if it is disabled, it teaches the users that tests are useless and that it's fine to just disable them. Reasons on why I still think that you were (partly) egoistially motivated: There is not a single known fatal problem with the clamping issue, and you are evidently skilled at finding concrete ways to exploit faulty code. As someone who always wants proof before taking action, you claiming that there is going to be problems is not very credible if you can't name an example. You might argue that "there exists an issue unless proven otherwise", but when there are two issues to compare to each other, it does not make sense to prefer breaking game startup (proven fatal issue) over possible other fatal issues (speculation), especially if the probability of a permanent issue (e.g. wold corruption) is somewhat low. There are far more severe issues in the engine that can lead to breakage; take https://github.com/minetest/minetest/issues/11742 for example. Yet, no selftest was introduced for these. Your claim that you would not have expected the minetest devs to "break the engine in such ways" is invalid and I don't see why you actually thought we'd believe that. You made a test about it, so of course you were expecting the test to eventually trigger. You KNEW about sfan5's PR, it was the motivation to make the test in the first place. If you would have not considered the possibility of the PR being merged, you wouldn't have made the test in the first place. When the PR got merged, which you noticed, you did not disable the test or anything. You were fully aware of very likely breaking the game by making such a test. And you were also aware that you exaggerated the consequences of the engine breakage that would have resulted otherwise. ### Conclusion If you, @erlehmann want to take responsibility for engine problems, I highly encourage you to fork Minetest. You have done this in the past as well. I offer my help in contributing and maintaining that fork. Ngl, sfan5's attitude is fucked. But please, don't harm the community we both care about by trying to fix engine problems with game code. This also applies to cora and me, who took part in the recent anticheat microhype. Let's fix the problems where they should be fixed. And erlehmann, I think that you should try to be more honest and neutral. This is not the first time that I think you have tried to discourage someone or made unfair or dishonest claims (for example I don't think that MineClone2/Mineclonia comparison is a topic that you deal with neutrally). I think you are a really cool person and I like hanging out with you on IRC - I also really appreciate the work that you are doing for the MineClone2/Mineclonia community. Therefore I feel bad about publicly pointing out what I think was a mistake of yours, but I think it had to be addressed.

LizzyFleckenstein03 commented

2021-12-14 16:29:47 +01:00

I have sent Minetest a patch: https://github.com/minetest/minetest/issues/11828#issuecomment-993594718

The minetest contribution guidelines explicitly state that you have to make a Pull Request.

I made one for you, hope you're ok with that. https://github.com/minetest/minetest/pull/11858

> I have sent Minetest a patch: https://github.com/minetest/minetest/issues/11828#issuecomment-993594718 The minetest contribution guidelines explicitly state that you have to make a Pull Request. I made one for you, hope you're ok with that. https://github.com/minetest/minetest/pull/11858

erlehmann commented

2021-12-14 19:45:57 +01:00

The main problem here is taking responsibility for something that we can't possibly control.

I agree. Mostly I wanted to avoid having to deal with a ton of buggy behaviour.

That failed assertion probably should have been some kind of annoying debug message.

There is not a single known fatal problem with the clamping issue, and you are evidently skilled at finding concrete ways to exploit faulty code. As someone who always wants proof before taking action, you claiming that there is going to be problems is not very credible if you can't name an example.

I am actually not very skilled in finding concrete ways of exploiting code – otherwise I would have turned a few of the UB bugs in Minetest into an inofficial client-sent server-mod interface. Greping the archive of contentdb for minetest\.find_nodes_in_area reveals a bunch of mods that now have problems.

Quoting myself from https://github.com/minetest/minetest/pull/11858#issuecomment-993855389

The bugs introduced by the change fall mostly into three categories:

Lua map generator code and placement code often contains functions to find nodes and replace them. I think Nether generation in MCL* has a function to replace stone with Nether rack and water with lava. Now if an area with x>31000 is emerged the stone generates in the wrong dimension and that water may flow into adjacent map blocks if I am not mistaken – in a hell dimension, in which no water should exist. I suspect it only takes one big cave or a flying machine or a player with an TNT cannon who blows up nodes in a normally-invisible mapblock to encounter this behaviour.

Naive code that loops if it can not find something it has just placed. This is by far the worst thing that can happen and the result will look about as stupid as this: https://github.com/minetest/minetest/issues/11839
You can say that this would be a mod bug and you would be right in a way, but at least for nodes used to work.

Code places a node and then does not account for a node not being found without looping. but leaving behind a node that should have been replaced. This is kind of a mix of the previous two cases. It can be abused if a game contains structure nodes that are intended to “unpack” into a schematic, if the structure node can never unpack. (Players could obtain the structure node.)

I have also seen code that naturally assumes that there is air around if you go higher and I do not remember exactly, but I think it will get into a nasty infinite loop when it can not ever find any air.

> The main problem here is taking responsibility for something that we can't possibly control. I agree. Mostly I wanted to avoid having to deal with a ton of buggy behaviour. That failed assertion probably should have been some kind of annoying debug message. > There is not a single known fatal problem with the clamping issue, and you are evidently skilled at finding concrete ways to exploit faulty code. As someone who always wants proof before taking action, you claiming that there is going to be problems is not very credible if you can't name an example. I am actually not very skilled in finding concrete ways of exploiting code – otherwise I would have turned a few of the UB bugs in Minetest into an inofficial client-sent server-mod interface. Greping the archive of contentdb for `minetest\.find_nodes_in_area` reveals a bunch of mods that now have problems. Quoting myself from https://github.com/minetest/minetest/pull/11858#issuecomment-993855389 > The bugs introduced by the change fall mostly into three categories: > > 1. Lua map generator code and placement code often contains functions to find nodes and replace them. I think Nether generation in MCL* has a function to replace stone with Nether rack and water with lava. Now if an area with x>31000 is emerged the stone generates in the wrong dimension and that water may flow into adjacent map blocks if I am not mistaken – in a hell dimension, in which no water should exist. I suspect it only takes one big cave or a flying machine or a player with an TNT cannon who blows up nodes in a normally-invisible mapblock to encounter this behaviour. > > 2. Naive code that loops if it can not find something it has just placed. This is by far the worst thing that can happen and the result will look about as stupid as this: https://github.com/minetest/minetest/issues/11839 You can say that this would be a mod bug and you would be right in a way, but at least for nodes used to work. > > 3. Code places a node and then does not account for a node not being found without looping. but leaving behind a node that should have been replaced. This is kind of a mix of the previous two cases. It can be abused if a game contains structure nodes that are intended to “unpack” into a schematic, if the structure node can never unpack. (Players could obtain the structure node.) > > I have also seen code that naturally assumes that there is air around if you go higher and I do not remember exactly, but I think it will get into a nasty infinite loop when it can not ever find any air.

erlehmann commented

2021-12-14 19:47:26 +01:00

I will address the rest of your comments later. I think it is a very good post though, thank you.

❤️ 1

erlehmann commented

2021-12-14 19:50:47 +01:00

The minetest contribution guidelines explicitly state that you have to make a Pull Request.

I made one for you, hope you're ok with that. https://github.com/minetest/minetest/pull/11858

I am totally ok with you making it. The reason I did not do it right away: I was told in the past that very small changes (like changing a constant, I guess) do not require two approvals, so I thought maybe I could get my shit past the radar if anyone in the core dev team agreed with me.

> The minetest contribution guidelines explicitly state that you have to make a Pull Request. > > I made one for you, hope you're ok with that. https://github.com/minetest/minetest/pull/11858 I am totally ok with you making it. The reason I did not do it right away: I was told in the past that very small changes (like changing a constant, I guess) do not require two approvals, so I thought maybe I could get my shit past the radar if *anyone* in the core dev team agreed with me.

erlehmann referenced this issue

2021-12-15 13:27:50 +01:00

MISC/mcl_selftests: Do not crash if minetest.find_nodes_in_area() lies #214

cora commented

2021-12-20 23:48:30 +01:00

fixed by #214

cora closed this issue

2021-12-20 23:48:30 +01:00

This repo is archived. You cannot comment on issues.