Fix transposed killer and victim in SMSG_PARTYKILLLOG - #80
Open
MadMaxMangos wants to merge 4 commits into
Open
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 7 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Killing a mob produced the combat-log line "<mob> killed you". The call site
passes the roles correctly -- Unit.cpp hands BuildPartyKillLog(player_tap,
pVictim) -- so the transposition was inside the builder.
The wire layout was never wrong. Reader sub_6F2FE4 decodes two packed GUIDs into
adjacent buffers, slot A at this+16..23 and slot B at this+24..31, with mask order
B7 B2 A1 B4 A2 A5 B3 B1 B0 A3 A0 A4 B6 A7 B5 A6
and byte order, from its "*(this + N) ^=" sequence, offsets
24 29 16 18 31 30 25 28 20 17 26 22 19 21 23 27 = B0 B5 A0 A2 B7 B6 B1 B4
A4 A1 B2 A6 A3 A5 A7 B3
Our interleaving matches both, position for position. Only the role assignment
was reversed: killer occupied slot A when the client shows slot B as the killer.
Which slot is the killer is NOT established by the binary here, and the old
comment claimed otherwise: "terminal sub_841B83 treats them as credited killer
then victim". There is no sub_841B83 anywhere in the 18414 export. That citation
was to a function that does not exist, and it is very likely how the wrong roles
came to be justified. The vtable the message constructor installs
(off_D6AB30 -> sub_6C4038) is a shared generic slot and does not resolve it
either. The roles are now set from observed client behaviour and the comment says
so, with a note not to restore the fabricated reference.
Both existing checks had locked the defect in rather than catching it:
- mop_compact_packets pinned exact bytes for one killer/victim pair, but the
expected bytes had been generated from the builder, so it asserted the
builder against itself and could not tell the two GUIDs apart.
- mop_compact_packets_source pinned "WriteGuidMask<7, 2>(victim)" under the
message "does not match reader sub_6F2FE4", conflating the ORDER (which the
reader does fix) with the ROLE (which it does not).
The unit test now decodes the packet using the mask and byte orders read out of
the reader itself, not out of the builder, and asserts which slot holds the
killer. Verified non-circular: transposing the builder fails it on
"slotB == killerGuid" and "slotA == victimGuid". The source gate now pins the
killer and its message separates the two claims.
New arm party_kill_roles_transposed, verified failing. The two pre-existing
party-kill arms quoted the old text and were updated; they had gone dead, which
this gate has no guard to report.
Full suite 1151/1151.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…fter all
Documentation only. The wire change in the previous commit is unaffected and
correct; what was wrong was the reason given for it.
That commit set killer/victim from observed client behaviour, having concluded
that the earlier "terminal sub_841B83" citation was fabricated because no such
symbol appears in the export. The address was always right. IDA defines no
function at 0x841B83 -- sub_841A86 ends at .text:00841B82 and the next proc is
sub_841DF7 -- because the only caller is a computed call, so no sub_841B83 SYMBOL
was ever created. The code is there.
Two search mistakes produced the wrong conclusion, and both are recorded in the
comment so they are not repeated. Wow.exe.c is pseudocode and contains defined
functions only, so a name missing there says nothing about the bytes. Wow.exe.asm
carries no address column at all, so grepping it for 0x841B83 cannot succeed
regardless. Wow.exe.lst has the address column; the code is at line 2111029.
The chain now cited:
sub_659694 case 176 (0xB0) builds via sub_706C0A, then calls sub_6C4FBA
sub_6C4FBA reaches its handler by computed call:
0xCE6A6758 + 0xD283DFE6 - 0xA06A2BBB = 0x00841B83
that handler passes slot B ([eax+18h]) and slot A ([eax+10h]) to sub_8413FA,
which routes B to event+0x18 and A to event+0x30
sub_840352 pushes those as COMBAT_LOG_EVENT sourceGUID and destGUID
subevent 0x2B = 43, and off_F58AD0[43] is "PARTY_KILL"
So slot B is the killer, which is what the builder already does. Corroborated
independently: the terminal resolves slot A through the path that sets the twelfth
PARTY_KILL argument, unconsciousOnDeath -- a property of the unit that died.
Also fixes Opcodes_reference.h, which still read "sub_6F2FE4 reads killer/victim
GUIDs". That is killer-first, the pre-fix order, so the branch contradicted itself
as pushed. The same stale line exists in the difficulty worktree and will be
corrected there separately.
off_D6AB30 is { sub_6C4038, nullsub_2, nullsub_2, sub_708A54, sub_7677FF }, not
just the generic slot 0 the previous comment described. Slot 3 tail-calls
sub_6F2B7B, a schema-identical second copy of the reader -- it corroborates the
layout without resolving the roles.
Full suite 1151/1151.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…g the evidence Three review findings, all correct. The test did not exercise mask ORDER at all. Both GUIDs in the existing case have eight nonzero bytes, so all 16 mask bits are 1 and the mask is FF FF under any permutation of the writes -- the case pinned byte order and the slot roles, and left kMaskOrder entirely unconstrained. A sparse case is added with DISJOINT nonzero positions: killer 0x00005C0000FF00A1 present at bytes 0, 2 and 5, and victim 0x00B2003E00000C00 at 1, 4 and 6. The mask is then 6 set bits in 16 specific places, so a permuted mask order lands presence on the wrong byte of the wrong slot and neither GUID reconstructs. Packet size is asserted at 8: two mask bytes plus six present bytes. party_kill_roles_transposed was not a transposition. It swapped only the first mask write, which this gate's own source assertion rejects immediately, and its comment credited mop_compact_packets with catching it. It now performs a full swap of every killer/victim write, and the comment says plainly that the source assertion is what fires first, with the semantic decode as the independent check that would still catch it if these assertions were relaxed. The swap is file-wide, which is safe only because all 17 (killer)/(victim) occurrences in Unit.h belong to this one builder -- noted at the arm, along with what to do if that stops being true. Two comments claimed live client observation, including a specific in-game string. The symptom was noticed in live play on 18414, but the fix has not been live-verified and I cannot substantiate the quoted message. The gate's error text now leads with the binary chain -- dispatch case 176 -> sub_706C0A -> sub_6C4FBA -> computed call to .text:00841B83 -> sub_840352, subevent 0x2B PARTY_KILL, event+0x18 = sourceGUID from slot B -- and records that a wrong-way-round combat log prompted the look without claiming any particular string. The reviewer independently confirmed the computed-call arithmetic: 0x00841B83 after the 32-bit wrap sits immediately after the preceding function's retn, opens with a standard prologue and reaches its own return, and a whole-tree search found only the one sender. Build clean, suite 1153/1153, gate baseline plus 83 arms all failing as intended with none dead.
…supported quote The sparse case did not pin kMaskOrder uniquely, which the re-review demonstrated concretely: two positions that are absent in the sparse case and present in the dense one have identical presence in both, so exchanging them in the mask order is invisible. 60 such pairs exist -- swapping victim <2, 5> for <5, 2> leaves both packets byte-identical. Four more cases close that. Each of the 16 (slot, byte) positions gets a 4-bit presence SIGNATURE: its id is slot * 8 + byteIndex, and in case k it is present iff bit k of that id is set. No two positions share a signature, so exchanging any two changes the decode in at least one case -- presence lands on the wrong position and the absent side reconstructs as zero. Byte values are 0x10 + id, so every present byte is nonzero, which the builder requires to mark it present, and distinct, which keeps kByteOrder constrained at the same time. Also removed the last unsupported evidence claim: the test still quoted a specific in-game string as what caught the transposition. The gate message was corrected in the previous commit but this one was missed. A combat log the wrong way round in live play prompted the look; the roles are settled from the client binary, and no particular string is claimed. Build clean, suite 1153/1153.
MadMaxMangos
force-pushed
the
fix/mop-party-kill-log-roles
branch
from
August 4, 2026 17:02
dce3edb to
d5b4526
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes transposed killer and victim in
SMSG_PARTYKILLLOG0x048A, so party and raid kills arecredited to the right unit in the combat log.
The change is purely semantic
Both the old and new versions emit exactly the same mask order, matching the client
reader
sub_6F2FE4:Master maps slot B → victim; this branch maps slot B → killer. The packets are
structurally identical — only the role assignment differs. No amount of inspecting packet
shape distinguishes them.
Evidence for the roles
Dispatch
sub_659694case 176 (0xB0) builds the message viasub_706C0A, which installsoff_D6AB30and reaches its terminal through a computed call resolving to.text:00841B83.That handler routes slot B to
event+0x18and slot A toevent+0x30, whichsub_840352pushes as
COMBAT_LOG_EVENTsourceGUID and destGUID. Subevent0x2B= 43, andoff_F58AD0[43]is"PARTY_KILL".Independently corroborated: the terminal resolves slot A through the path setting the twelfth
PARTY_KILLargument,unconsciousOnDeath— a property of the unit that died.Caveat worth stating plainly
The fixtures cannot adjudicate this. They are writer-inverses, so they agree with whichever
role assignment the writer uses. The commit history here also reversed twice before settling.
This wants one live confirmation: kill something in a group and read the combat log.
Landing notes
Rebased from 2026-07-30. Dropped
mop_compact_packets_source_test.cmakeand itsforeachmutation block — master deleted every
*_source_test.cmakeina4a7fd7e6. Took the branch'sOpcodes_reference.hprovenance line, which names the terminal accurately.Verification
mop_compact_packets_testbuilds and passes.