Breath is now handled server side. Changing this behaviour required some modifications to core:
* Ignore TOSERVER_BREATH package, marking it as obsolete
* Clients doesn't send the breath to server anymore
* Use PlayerSAO pointer instead of peer_id in Server::SendPlayerBreath to prevent a useless lookup (little perf gain)
* drop a useless static_cast in emergePlayer
Remake the light_decode_table.
The table starts out without pre-filled in values since those
are always discarded by the code apparently. We calculate a
pseudo curve with gamma power function, and then apply a new
adjustment table.
The adjustment table is setup to make the default gamma of 2.2
look decent: not too dark at light level 3 or so, but too dark
at 1 and below to be playable. The curve is much smoother than
before and looks reasonable at the whole range, offering a
pleasant decay of light levels away from lights.
The `display_gamma` setting now actually does something logical:
the game is darker at values below 2.2, and brighter at values
above 2.2. At 3.0, the game is very bright, but still has a good
light scale. At 1.1 or so, the bottom 5 light levels are virtually
black, but you can still see enough detail at light levels 7-8,
so the range and spread is adequate.
I must add that my monitor is somewhat dark to begin with, since
I have a `hc` screen that doesn't dynamic range colors or try to
pull up `black` pixels for me (it is tuned for accurate color and
light levels), so this should look even better on more dynamic
display tunings.
* Calculate maximum interact distance from wielded tool
* New "interacted_while_dead" cheat_type for the Lua API
* Disallow dropping items while dead
* Move player to spawn before resurrecting them
Currently, lua does a regular exit() after a lua panic, which can make
a problem hard to debug. Invoking FATAL_ERROR() instead will print
some useful information, and abort() minetest, so that a debugger can
be used to analyze the situation.
Trying to resolve a path with RemoveRelativePathComponents that can't
be resolved without leaving leading parent components (e.g. "../worlds/foo"
or "bar/../../worlds/foo") will fail. To work around this, we leave
the relative components and simply remove the trailing components one
at a time, and bail out when we find a parent component. This will
still fail for paths like "worlds/foo/noexist/../auth.txt" (the path
before the last parent component must not exist), but this is fine
since you won't be able to open a file with a path like that anyways
(the O.S. will determine that the path doesn't exist.
Try `cat /a/../etc/passwd`).
As mgv7 is now the default mapgen i re-checked its tunnel width on request,
discovered they needed to be wider, and have made this change.
This commit widens the identical 3D noise tunnels in the other mapgens in
exactly the same way.
Fixes#4878
Also remove an artificial viewing range reduction that
(presumably) was added to compensate for miscomputed
viewing ranges, and that doesn't seem to be needed any
more (thanks to lhofhansl).
Schematics can already be placed with a param2 value, but not
simple 1-node plant decorations of the simple type.
This adds a `param2` field to the simple deco type that is
checked to be between 0 and 255, and put to the placed node
at mapgen.
This can be used to put a degrotate value in, or e.g. a fill
value for leveltype nodes, or a place_param2 value at mapgen
placement, or vary the shape of meshoptions plantlike drawtype.
Prevent waterfalls from falling down or streams from flowing away when the
source node is in an unloaded block - Nodes near a CONTENT_IGNORE node will
be interpreted as if the ignored node is a liquid node that just supports
the current state of the nodes in question.
To avoid smaller biomes when extra biomes are added to MTGame.
The addition of bushes in MTGame grasslands makes wood resources easier
to find and less distant, so slightly larger biomes are now acceptable,
but also desirable to encourage travel and create more sense of adventure.
Floatland base terrain underside was too thin, causing excessive water
leakage through tunnels under lakes, now make it thicker.
Floatland mountain terrain had a rim 1 node thick which made it bare
stone, now make it 2 nodes thick to merge with the floatland base
terrain rim and to have a layer of biome material.
Make mountain terrain more exponentially shaped by altering the
exponent.
Remove unnecessary and potentially ugly MYMAX() applied to
n_base_height.
This combats the problem of sending the hundreds of
"creative" / "armor" or whatever detached invs that
exist on popular servers to each and every player
on join or on change of said invs.
Previously, paths like ./worlds would be resolved to /worlds since the
leading dot was considered just as irrelevant as a dot in the middle of
the path.
Position, velocity and acceleration vectors of particles are rotated
by the yaw of the parent object so that they are truly relative to it.
Clarify new attached particle spawner behavior in lua_api.txt.
Active objects that are attached to other objects are not safe
from deletion. As a result, the parent object may have a reference
to an id of a child's that no longer exists.
If at some point an attempt is made to manipulate the child,
enviromment->getActiveObject(child-id) returns NULL. Using the
NULL pointer causes the crash...
This is a slightly modified and cleaned up version of #3774 by RealBadAngel.
By sofar: Remove color change (just make it lighter) and some minor cleanups.
The client would not compute the distance from the camera to
to a mapblock correctly. The result was that blocks that were in
view (i.e. not beyond the fog limit) would not be rendered.
With the improved distance computation, a range adjustment that
existed in clientiface.cpp is no longer required.
The problem was seen while using the mobf mod package.
The problem happens when the server serializes entity attachments.
Sometimes, such attachments no longer exist. The serialization code
skips those. However, the total number of attachments was serialized
earlier. Therefore the client expects more than it gets, and logs a
serialization error.
PlayerSAO::disconnected() function was historical and remove the link between SAO and RemotePlayer session. With previous attributes linked to RemotePlayer saving was working. But now attributes are read from SAO not RemotePlayer and the current serialize function verify SAO exists to save the player attributes.
Because PlayerSAO::disconnected marks playersao for removal, only mark playerSAO for removal and let PlayerSAO::removingFromEnvironment do the correct saving behaviour and all the disconnection process instead of doing a partial removal and let the server loop doing the RemotePlayer cleanup and remove some saved attributes...
* Create UnitSAO, a common part between PlayerSAO & LuaEntitySAO
* Move breath to PlayerSAO & LocalPlayer
* Migrate m_yaw from (Remote)Player & LuaEntitySAO to UnitSAO
* Migrate m_yaw from Player to LocalPlayer for client
* Move some functions outside of player class to PlayerSAO/RemotePlayer or LocalPlayer depending on which class needs it
* Move pitch to LocalPlayer & PlayerSAO
* Move m_position from Player to LocalPlayer
* Move camera_barely_in_ceiling to LocalPlayer as it's used only there
* use PlayerSAO::m_base_position for Server side positions
* remove a unused variable
* ServerActiveObject::setPos now uses const ref
* use ServerEnv::loadPlayer unconditionnaly as it creates RemotePlayer only if it's not already loaded
* Move hp from Player to LocalPlayer
* Move m_hp from LuaEntitySAO to UnitSAO
* Use m_hp from PlayerSAO/UnitSAO instead of RemotePlayer
This commit rewrites the procedure that is responsible for light
updating.
this commit
-provides iterative solutions for unlighting and light spreading
-introduces a new priority queue-like container for the iteration
-creates per-node MapBlock caching to reduce retrieving MapBlocks from
the map
-calculates with map block positions and in-block relative node
coordinates
-skips light updating if it is not necessary since the node's new light
will be the same as its old light was
Flash alpha maximum is reduced from 180 to 127 to avoid player blindness
in combat. Flash alpha minimum is unchanged.
The 'damage_flash' value is now limited to max alpha, to avoid multiple
hits creating a huge value that causes flash to stay at maximum alpha
for a long period. Now alpha always starts to fade immediately after
taking damage.
Both problems can be seen in Minetest let's play videos.
Simplify and optimise some code.
Move static object storage force-delete message from errorstream to
warningstream.
Increase 'max objects per block' setting to 64.
Add missing spaces in warning code.
Modification of the emergeblocks internal state was not protected
by a lock, causing a race condition.
This can be reproduced by repeatedly running emergeblocks for an
already-generated section of the map (with multiple emerge threads).
* Move client list to ServerEnvironment and use RemotePlayer members instead of Player
* ClientEnvironment only use setLocalPlayer to specify the current player
* Remove ClientEnvironment dead code on player list (in fact other players are CAO not Player objects)
* Drop LocalPlayer::getPlayer(xxx) functions which aren't used.
* Improve a little bit performance by using const ref list for ClientEnvironment::getPlayerNames() & Client::getConnectedPlayerNames()
* Drop isLocal() function from (Local)Player which is not needed anymore because of previous changes
This change permits to cleanup shared client list which is very old code.
ClientEnvironment doesn't use player list anymore, it only contains the local player, as addPlayer is only called from Client constructor client side.
Clients are only CAO on client side, this cleanup permit to remove confusion about player list.
* remove IGameDef from Player class, only LocalPlayer has it now
* move many attributes/functions only used by LocalPlayer from Player to LocalPlayer
* move many attributes/functions only used by RemotePlayer from Player to RemotePlayer
* make some functions const
* hudGetHotbarSelectedImage now returns const ref
* RemotePlayer getHotbarSelectedImage now returns const ref
* various code style fixes
* ClientEnvironment now uses UNORDERED MAP for active objects
* Use RemotePlayer and LocalPlayer everywhere it's possible
* Minor code style fixes
* Drop Client::getBreath() unused function
* Server/Client Environments now have an helper to cast Player object in the right type to use it
* Server: use RemotePlayer everywhere and remove previous added casts
* Client: use LocalPlayer where needed
* Environment: remove unused functions (getPlayers(), getRandomConnectedPlayer(), getNearestConnectedPlayer())
* LocalPlayer take ownership of maxHudId as it's the only caller
* RemotePlayer take ownership of day night ratio as it's the only user
* Pass getPlayerControl as const reference to prevent object copy on each call (perf improvement in ObjectRef::l_get_player_control call)
* getPlayerSAO is now only RemotePlayer call
* get/setHotbarItemCount is now RemotePlayer owned
* Server: Use RemotePlayer instead of Player object on concerned call to properly fix the object type
* PlayerSAO now uses RemotePlayer instead of Player because it's only server side
* ObjectRef::getplayer also returns RemotePlayer as it's linked with PlayerSAO
The initial problem was that mutex_auto_lock.h tries to use std::unique_lock<std::mutex>
despite mutex.h not using C++11's std::mutex on Windows. The problem here is the mismatch
between C++11 usage conditions of the two headers. This commit moves the decision logic
to threads.h and makes sure mutex.h, mutex_auto_lock.h and event.h all use the same features.
This is part 2 for 5f084cd98d7b3326b51320455364337539710efd
Other improvements:
* Use the defined ItemGroupList when used
* make Client::checkPrivilege const
* inline some trivial functions
* Add ActiveObjectMap typedef
* Add SettingsEntries typedef
'getSpawnLevelAtPoint()' did not account for disabled mountains, it
was possible to be spawned in mid-air where a mountain surface would
have been.
Avoid check for river area if rivers are disabled.
Add 'core.LIGHT_MAX = 14' to builtin/game/constants.lua with the intention
to replace misplaced 'default.LIGHT_MAX = 14' in Minetest Game.
Add comment in light.h requiring the constant be changed in both places.
Add lighting bug warning to note in lua_api.txt.
There are hundreds of mod uses of 15 which causes a lighting bug.
Inverts the given channels of the base image.
Mode may contain the characters "r", "g", "b", "a".
Only the channels that are mentioned in the mode string will be inverted.
Rebased and tuned version of Calinou's original pull request.
'm_view_bobbing_speed' controls the frequency of view bobbing,
hand bobbing and footsteps, it was limited to a maximum of 40 (walking
frequency) so did not increase if player speed was modified by a
'speed buff', a sprinting mod or modified in .conf or advanced
settngs.
This commit raises the limit to 70 which is suitable for sprinting.
Adds several new ways that the plantlike drawtype mesh can be changed.
This requires paramtype2 = "meshoptions" to be set in the node
definition. The drawtype for these nodes should be "plantlike".
These modifications are all done using param2. This field is now
a complex bitfield that allows some or more of the combinations to
be chosen, and the mesh draw code will choose the options based as
neeeded for each plantlike node.
bit layout:
bits 0, 1 and 2 (values 0x1 through 0x7) are for choosing the plant
mesh shape:
0 - ordinary plantlike plant ("x" shaped)
1 - ordinary plant, but rotated 45 degrees ("+" shaped)
2 - a plant with 3 faces ("*" shaped)
3 - a plant with 4 faces ("#" shaped)
4 - a plant with 4 faces ("#" shaped, leaning outwards)
5 through 7 are unused and reserved for future mesh shapes.
bit 3 (0x8) causes the plant to be randomly offset in the x,z
plane. The plant should fall within the 1x1x1 nodebox if regularly
sized.
bit 4 (0x10) causes the plant mesh to grow by sqrt(2), and will cause
the plant mesh to fill out 1x1x1, and appear slightly larger. Texture
makers will want to make their plant texture 23x16 pixels to have the
best visual fit in 1x1x1 size.
bit 5 (0x20) causes each face of the plant to have a slight negative
Y offset in position, descending up to 0.125 downwards into the node
below. Because this is per face, this causes the plant model to be
less symmetric.
bit 6 (0x40) through bit 7 (0x80) are unused and reserved for
future use.
!(https://youtu.be/qWuI664krsI)
Disable the ability to connect to old servers by default to
improve password security.
If people still want to connect to old (0.4.12 and earlier)
servers, they can flip the send_pre_v25_init setting.
Add the ability to detect if we've tried to connect
to a server which only supports the pre v25 init protocol,
and show an apropriate error message. Most times the error
will already be catched at the serverlist level, the
detection mechanism only acts as last resort, because the
"Connection timed out" error message that would be shown
otherwise would be very confusing.
Automatic "fixing" of this condition is not desired,
as it would allow for downgrade attacks.
As already 161 of the 167 servers on the serverlist
support the new srp based auth protocol (> 96%),
the breakage should be minimal.
Follow up of commit
af30183124d40a969040d7de4b3a487feec466e4 "Add option to not send pre v25 init packet"
Also change the pessimistic assumption of masterlist
server versions to optimistic, in order to avoid buggy
behaviour (favourites not in the serverlist would be
denied to connect to, etc).
Fixes#4446: a syntax error that happens if something else (such as
<iomanip> or <locale> in certain libstdc++ versions) includes
<libintl.h> later, which contains a function declaration for gettext
that gets mangled by the macro.
See the added comment in gettext.h and the discussion in #4446/#4452
for details.
If compiling according to a C++ version before C++11, then define
std::to_string ourselves.
Add a to_wstring version as well
As std::to_string() for floating point types uses %.6f as floating
point format converter, instead of %G, it needs special care.
To preserve ftos() behavior (which is expected to use the %G format
converter), it no longer uses to_string().
If a mod is disabled, or upgraded without sufficient backward compatibility,
then entities it has put into the world become unknown, and continue moving
around, but are completely unrecognisable.
This change allows the player to see their type, and therefore which mod is
or was responsible.
previously function used tonumber which returned float
this caused errors in large numbers and resulted in
obj-def-handlers being invalid when retrived from lua tables in c
* Move included json code to jsoncpp subdirectory
This is needed to avoid having to specify the minetest src directory
as a system include when fixing the json includes.
* Fix json includes
They used "", so that the compiler searches the project's directory
first. The result was that when compiling with a system jsoncpp,
the project's own version of json.h was still included, instead of
the system version.
The includes now use <>, so a system location, or one specified with
'-Ilocation' is searched only.
* Fix for jsoncpp deprecated function warning
When compiling with a newer version of jsoncpp (and
ENABLE_SYSTEM_JSONCPP=true), jsoncpp emits a warning
about a deprecated function that minetest uses.
This fallback to std::map & std::set for older compilers
Use UNORDERED_SET as an example in decoration and ore biome sets
Use UNORDERED_MAP as an example in nameidmapping
Fix height check comparison from '>=' to '>'.
Fix getHeight() for schematic decorations to account for
'deco place center y' flag and for how normal placement
sinks schematic 1 node into the ground.
Jungletrees were not being placed at y = 46, y = 47 despite
having an acceptable 16 nodes of height above ground surface.
When the 'flat' and 'trees' flags were moved into mgv6_spflags they
were left in mg_flags in an attempt to support old mgv6 worlds. However
their appearence in mg_flags causes confusion, also, later, old-world
support was found to be broken for mgv6 worlds with 'notrees'.
This commit cleans up the mess and comes a month after a thread warning
of the change, and explaining the required action, was posted in the
news subforum. Only old mgv6 worlds with 'flat' or 'notrees' are
affected, a small minority of worlds, the required action being
correctly setting these flags in mgv6_spflags.
Disable a section of the 'map settings manager' unit test which is to
be changed as it is causing problems for pull requests.
l_request_insecure_environment didn't ignore all whitespace in the
secure.trusted_mods config option.
Replaces std::remove with std::remove_if and the isspace function.
Since the creation of minetest, it had no window icon on X11.
Now we have one.
The misc/minetest-xorg-icon-128.png file is a rendering of the
misc/minetest.svg file with inkscape, created with something like:
inkscape -z -e misc/minetest-xorg-icon-128.png -w 128 -h 128 misc/minetest.svg
The "testStartStopWait" unit test is unreliably failing on mac,
for some time already. See bug #3786.
Having the unittest fail unreliably doesn't help anybody but mostly
inhibits the main feature of travis builds: to test PRs for regressions.
Therefore, disable the specific unit test for until bug #3786
is fixed.
The source used a hodge-podge of different combinations of different
macros to check for linux: 'linux', '__linux', '__linux__'.
As '__linux__' is standard (Posix), and the others are not, the source
now uniformly uses __linux__. If either linux or __linux are defined,
it is made sure that __linux__ is defined as well.
It seems that various different devices register as
Joysticks on linux, and their presence has caused
lots of confusion amongst minetest users.
Therefore, disable the joystick feature.
Closes#4261Closes#4221
This commit refactors the majority of the Mapgen settings system.
- MapgenParams is now owned by MapSettingsManager, itself a part of ServerMap,
instead of the EmergeManager.
- New Script API functions added:
core.get_mapgen_setting
core.get_mapgen_setting_noiseparams,
core.set_mapgen_setting, and
core.set_mapgen_setting_noiseparams.
- minetest.get/set_mapgen_params are deprecated by the above new functions.
- It is now possible to view and modify any arbitrary mapgen setting from a mod,
rather than the base MapgenParams structure.
- MapgenSpecificParams has been removed.
- Move mapgen creation logic out of EmergeManager and into Mapgen
- Internally represent mapgen type as an enum value, instead of a string
- Remove the need for a MapgenFactory per mapgen
Reduce spread from 96 to primes 61 and 67 (either side of 64)
Prime spreads help to keep 3D noise periodic features unaligned
'cave width' 0.2 to preserve tunnel width
Reduce octaves to 3 to improve network structure
- Generalise node names to c_wall and c_alt_wall
- Remove 'mossratio' and instead disable alt_wall loop if
c_alt_wall == CONTENT_IGNORE
- Use one generalised 3D noise for alternative wall nodes and in
mgv6 create moss distribution similar to the previous
- Rename rarity noise to density noise and enable the option of multiple
dungeons per chunk determined by the value. Recreate previous distribution
- Add parameters for min and max rooms per dungeon
- Add dungeon y limits
- Integrate river water properly
Generalisation is needed now that we have sandstone and desert stone
dungeons by default and can choose any node for alternative structure.
The current code is based around cobble dungeons with mossycobble
alternative nodes, the 2 noises controlling the alternative nodes
are based on wetness.
Enabling multiple dungeons per chunk with definable number of rooms
allows the option of very dense and complex underground structures
that could interconnect to create megastructures.
Y limits are added to be consistent with other mapgen elements, and
enable locaton of dungeon or megastructure realms as part of our
'stacked realms' philosophy.
Add new colours 'skycolour_bright_night', 'bgcolour_bright_night'
and enable these between sunset end and sunrise start
Night sky has same hue as day sky but is darker and more saturated
Night horizon haze (bgcolour) is slightly less saturated and
slightly brighter than night sky, to be consistent with daytime
horizon haze
Mgvalleys: Remove riverbed sand placement from base terrain generation
Riverbed material placement moved to MapgenBasic::generateBiomes()
Document fields and add note that the biome API is still unstable
- Adds only ~100 nodes per chunk to trans_liquid with similar processing time
- Adds liquid nodes themselves instead of potential solid nodes below them
- CONTENT_IGNORE nodes are interpreted as if they continue their neighborhood
- This allows liquid columns to span multiple chunks without being interrupted
- NOTE: Expects an one-node border in generation chunk without liquid changes
Previously, rivers were sometimes blocked by vertical walls
of mountain terrain due to river carving being disabled
when base terrain height was below water_level - 16
Remove now unused base terrain heightmap created in
generateTerrain()
Gives a convenient way to check a player's password.
This entirely bypasses the SRP protocol, so should be used
with great care.
This function is not intended to be used
in-game, but solely by external protocols, where no
authentication of the minetest engine is provided, and
also only for protocols, in which the user already gives the
server the plaintext password.
Examples for good use are the classical http form, or irc,
an example for a bad use is a password change dialog inside
formspec.
Users should be aware that they lose the advantages of the SRP
protocol if they enter their passwords for servers outside the
normal entry box, like in in-game formspec menus,
or through irc /msg s,
This patch also fixes an auth.h mistake which has mixed up the
order of params inside the decode_srp_verifier_and_salt function.
Zeno-: Added errorstream message for invalid format when I committed
Adds the particle option `collision_removal = bool`
Some particles are hard to use right now since they either go through
solid blocks (without collision detection), and with collision
detection enabled they (e.g. raindrops) would just stop dead on the
floor and sit there until they expire, or worse, scrape along a wall
or ceiling.
We can solve the problem by adding a boolean flag that tells the
particle to be removed if it ever collides with something. This will
make it easier to add rain that doesn't fall through your roof or stick
on the top of it. Or clouds and smoke that don't go through trees.
Particles that collide with this flag are marked expired
unconditionally, causing them to be treated like normal expired
particles and cleaned up normally.
Documentation is adjusted accordingly.
An added bonus of this patch is that particles can potentially collide
many times with nodes, and this reduces the amount of collisions to 1
(max), which may end up reducing particle load on the client.
This commit moves noise calculation to the functions where the noise is
actually required, increasing the separation of concerns and level of
interdependency for each mapgen method. Valleys Mapgen is left unmodified.
- Convert instances of numeric literal doubles to floats
- Move dswitchint to a local variable now that being a member is unnecessary
- Improve const correctness
- Indentation fixes
MSVC and GCC evaluate parameters in right-to-left order, whereas Clang
evaluates in left-to-right order, and of course, an optimization could
leave the order of evaluation completely indeterminate.
This commit fixes all instances of the error by explicitly assigning the
results of expressions that use PseudoRandom::next() or range() to their
respective vector components.
The right-to-left evaluation behavior is preserved since Clang is much less
commonly used to compile Minetest than GCC and MSVC combined, and would
therefore cause the least harm.
BiomeGen defines an interface that, given a set of BiomeParams, computes biomes
for a given area using the algorithm implemented by that specific BiomeGen.
This abstracts away the old system where each mapgen supplied the noises
required for biome generation.
We can remove the function in MtNativeActivity now
as it serves precisely that purpose: to tell irrlicht
that we handled the esc key.
TODO for later:
* Perhaps try to find a more performant container than KeyList
Fixes a bug where packet reordering made the server give the
client two peer ids instead of one. This in turn confused
reliable packet sending and made connecting to the server fail.
The client usually sends three packets at init: one "dummy"
packet consisting of two 0 bytes, and the init packet as well as
its legacy counterpart. The last one can be turned off since commit
af30183124d40a969040d7de4b3a487feec466e4, but this is of lower
relevance for the bug. The relevant part here is that network
packet reorder (which is a normal occurence) can make the packets
reach the server in different order.
If reorder puts the dummy packet further behind, the following
would happen before the patch:
1. The server will get one of the init packets on channel 1 and
assign the client a peer id, as the packet will have zero as
peer id.
2. The server sends a CONTROLTYPE_SET_PEER_ID packet to inform
the client of the peer id.
3. The next packet from the client will contain the peer id set by
the server.
4. The server sets the m_has_sent_with_id member for the client's
peer structure to true.
5. Now the dummy packet arrives. It has a peer id of zero, therefore
the server searches whether it already has a peer id for the
address the packet was sent from. The search fails because
m_has_sent_with_id was set to true and the server only searched
for peers with m_has_sent_with_id set to false.
6. In a working setup, the server would assign the dummy packet to
the correct peer id. However the server instead now assigns a
second peer id and peer structure to the peer, and assign the
packet to that new peer.
7. In order to inform the peer of its peer id, the server sends a
CONTROLTYPE_SET_PEER_ID command packet, reliably, to the peer.
This packet uses the new peer id.
8. The client sends an ack to that packet, not with the new peer id
but with the peer id sent in 2.
9. This packet reaches the server, but it drops the ACK as the peer
id does not map to any un-ACK-ed packets with that seqnum. The
same time, the server still waits for an ACK with the new peer
id, which of course won't come. This causes the server to
periodically re-try sending that packet, and the client ACKing it
each time.
Steps 7-9 cause annoyances and erroneous output, but don't cause
the connection failure itself.
The actual mistake that causes the connection failure happens in 6:
The server does not assign the dummy packet to the correct peer, but
to a newly created one.
Therefore, all further packets sent by the client on channel 0 are
now buffered by the server as it waits for the dummy packet to reach
the peer, which of course doesn't happen as the server assigned
that packet to the second peer it created for the client.
This makes the connection code indefinitely buffer the
TOSERVER_CLIENT_READY packet, not passing it to higher level code,
which stalls the continuation of the further init process
indefinitely and causes the actual bug.
Maybe this can be caused by reordered init packets as well, the only
studied case was where network has reliably reordered the dummy
packet to get sent after the init packets.
The patch fixes the bug by not ignoring peers where
m_has_sent_with_id has been set anymore. The other changes of the
patch are just cleanups of unused methods and fields and additional
explanatory comments.
One could think of alternate ways to fix the bug:
* The client could simply take the new peer id and continue
communicating with that. This is however worse than the fix as
it requires the peer id set command to be sent reliably (which
currently happens, but it cant be changed anymore). Also, such a
change would require both server and client to be patched in order
for the bug to be fixed, as right now the client ignores peer id
set commands after the peer id is different from
PEER_ID_INEXISTENT and the server requires modification too to
change the peer id internally.
And, most importantly, right now we guarantee higher level server
code that the peer id for a certain peer does not change. This
guarantee would have to be broken, and it would require much
larger changes to the server than this patch means.
* One could stop sending the dummy packet. One may be unsure whether
this is a good idea, as the meaning of the dummy packet is not
known (it might be there for something important), and as it is
possible that the init packets may cause this problem as well
(although it may be possible too that they can't cause this).
Thanks to @auouymous who had originally reported this bug and who
has helped patiently in finding its cause.
