bugfix(network): Sort command list fast path by sort number

[CryoTheRenegade]

• Closes NetCommandList::addMessage sorts messages by id using different functions #2795

Changed the cached NetCommandList::addMessage insertion path to use getSortNumber(), matching the full list traversal.

For normal commands this is equivalent to getID(). ACK commands override it with the ID of the command being acknowledged, so using it consistently prevents incorrect ACK ordering.

[Skyaero42]

Starting to look better, but still a few points of attention and some formatting

[Skyaero42]

I don't think this is correct. if candidate->getNetCommandType() < reference->getNetCommandType() it is not supposed to return false, but to move to the next if statement.

I think it should be

if (candidate->getNetCommandType() > reference->getNetCommandType())
{
  return true;
}

if (candidate->getPlayerID() > reference->getPlayerID())
{
  return true;
}

return isCommandIdNewer(candidate->getsortNumber(), msg->reference->getsortNumber());

[CryoTheRenegade]

This is lexicographic: type → player → sort number. A lower type must return false; otherwise (type=1, player=7) could incorrectly sort after (type=2, player=0).

[Skyaero42]

I need a second opinion on this from someone else. You may be correct, but its very different from the original if-statement as well as your original fix (which only replaced id with sortednumber)

[bobtista]

Second opinion :) Cryo’s comparator matches the ordering used by the full-scan path.
That loop only compares sort numbers while the type and player ID are both equal. In other words, the existing ordering is already lexicographic: compare type first, then player, then sort number. The cached fast path has to produce the same insertion point.
The proposed independent conditions preserve the shape of the original || expression, but that shape is the defect being corrected. It allows a lower-type command with a higher player ID to be treated as “after,” so the cached path can disagree with the full scan.
A short truth table in the PR description - type lower/higher, then equal type with player lower/higher, then both equal would document this without requiring new test infrastructure.

[Caball009]

I'm a bit concerned this may affect retail compatibility. Please test this change in a multiplayer match. You can use two local instances, one with the change and one without.

[CryoTheRenegade]

I'm a bit concerned this may affect retail compatibility. Please test this change in a multiplayer match. You can use two local instances, one with the change and one without.

Tested with latest weekly as control, no issues seen (had 2 instances of tracy in the background and nothing weird happened, but im not very good at playing against myself so i didnt stress it out too much :/)

[xezon]

The title says that this is a bug fix. What is the bug that can be observed in the game before this fix?

[CryoTheRenegade]

The cached fast path could order network commands differently from the full scan. Peers can then consume commands in different orders, causing multiplayer desync. This makes both paths use the same ordering: type, player, then sort number

[Caball009]

Would you mind writing test code that forces a mismatch because of this bug? FWIW I did something similar for a similar PR here: https://www.github.com/TheSuperHackers/GeneralsGameCode/pull/2736/changes/f386e5fb030acf148f7d038c6c2a09808ab7425d