#45 review pass: correct three claims the fix's own comments got wrong

An adversarial review of the change returned NO BLOCKERS but caught three
statements that were wrong or unverifiable.  All three are corrections to my own
comments/docs, not behaviour changes -- the verified binary is unchanged.

1. The phantom-kill note had the ORDERING backwards.  It claimed the owner's
   record "overwrites it on the next record -- the phantom stops being visible".
   The engine's event priorities run the other way: an inbound update record is
   posted at UpdateEventPriority == MaxEventPriority and drained by
   ExecuteBackgroundTask, while the rerouted score message sits at
   EntityManagerEventPriority == DefaultEventPriority and is popped later -- so
   the correction usually lands FIRST and the phantom AFTER it.  The artifact is
   BOUNDED, not masked: on the killer's pod only, that victim's KILLS reads +1
   until the next record, i.e. <= one 2s heartbeat instead of "until the victim
   next scores or dies".  Swept in btplayer.cpp, KD_SCOREBOARD_PLAN.md and
   RECONCILE.md.

2. The corpus figures were not reproducible, and were mis-tiered.  I cited
   "125 of 125 SCORE type=2 over 255 node-logs" and "0 of 8800 DMG rows" -- true
   when measured, but the corpus is append-only and this fix's OWN verification
   runs grew it from ~255 to 425 files, so nobody can re-derive them.  Replaced
   everywhere with invariant RATIOS that hold at any corpus size, re-measured
   here:
     * 0 of 18818 DMG rows are inst=R (damage is applied master-side only)
     * 439 of 439 DEATH inst=R rows read killer=0:0 killdmg=0.000, against
       0 of 225 inst=M rows (sole lastInflictingID writer: mech.cpp:746)
   Also re-tagged T1 -> T2: log-corpus field evidence is T2 per the CLAUDE.md
   tier table.  Lesson recorded in the banner: cite ratios, not counts, for a
   corpus your own rigs append to.

3. The recordLength guard's justification was wrong (and the plan's risk 5 with
   it).  A short legacy record does NOT desync the stream -- both sides advance
   by the WRITER's stamped recordLength and no reader asserts a length.  The real
   hazard is a read PAST THE ALLOCATION: an inbound update message lives in a
   per-event heap block sized from messageLength, so on a TRAILING legacy record
   the two new fields would be read off the end of it.

Plus: the record struct's own field comment still said "deathCount -- the DEATHS
column", the exact wrong-field belief #45 removed, sitting on the wire struct.

Filed the review's other finding on #60: a SECOND decomp export gap, index
jumping FUN_0049fe80 (ends 0x49ffc8) -> FUN_004a1232, 4714 unexported bytes
containing Mech::TakeDamageMessageHandler @0x4a0230 -- the producer of every
score and death message in the game.  Verified independently against
functions_index.tsv before posting.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
arcattack
2026-07-25 22:15:06 -05:00
co-authored by Claude Opus 5
parent a52207d779
commit 48d47ef806
6 changed files with 93 additions and 52 deletions
+29 -16
View File
@@ -1822,8 +1822,8 @@ BitMap *BTPilotNameBitmap(void *pilot)
// (mech4.cpp) and its `friend` (btplayer.hpp) -- all three at once, because /FORCE
// would otherwise hide a straggler. It never executed (its call site sat inside the
// once-per-death TRANSITION, which a replicant never enters) and it could not have
// worked (a replicant victim carries no attribution: 0 of 8800 corpus DMG rows target
// a replicant). DEATHS is now replicated from the owning pod instead. Do not
// worked (a replicant victim carries no attribution: 0 of 18818 corpus DMG rows are
// inst=R, and 439 of 439 DEATH inst=R rows read killer=0:0). DEATHS is now replicated from the owning pod instead. Do not
// resurrect it: under replication it would be a SECOND writer of a replicated counter,
// and it incremented `Player::deathCount`, which the scoreboard no longer even reads.
@@ -1842,12 +1842,20 @@ BitMap *BTPilotNameBitmap(void *pilot)
// Sends for free: both counter writes already dirty the record --
// `ForceUpdate()` on the death path (:508) and on every score award (:829/:840).
//
// Side effect worth knowing: the binary's kill handler also bumps killCount on
// the VICTIM's player (the @0x4c0397/@0x4c03a3 wrong-column slip we reproduce
// faithfully). That partner increment always lands on a REPLICANT copy of the
// victim, so the owner's authoritative value now overwrites it on the next
// record -- the phantom kill stops being visible without touching the
// reconstructed handler.
// The binary's phantom partner increment, stated ACCURATELY. Its kill handler
// also bumps killCount on the VICTIM's player (the @0x4c0397/@0x4c03a3
// wrong-column slip, which we still reproduce byte-faithfully). That increment
// always lands on a REPLICANT copy of the victim, so the owner's record corrects
// it -- but NOT, as an earlier draft of this comment claimed, by arriving after
// it. The engine's event priorities run the other way: an inbound update record
// is posted at UpdateEventPriority == MaxEventPriority (INTEREST.cpp) and drained
// by ExecuteBackgroundTask, while the rerouted score message is posted at
// EntityManagerEventPriority == DefaultEventPriority (NTTMGR.cpp) and popped
// later, so the correction usually lands FIRST and the phantom lands after it.
// The artifact is therefore BOUNDED, not masked: on the killer's pod only, that
// victim's KILLS reads +1 until the next record -- at most one heartbeat (2s)
// rather than "until the victim next scores or dies". Deciding whether to drop
// the increment outright is the open fidelity question; see docs/RECONCILE.md.
//
void
BTPlayer::WriteUpdateRecord(Simulation__UpdateRecord *message, int update_model)
@@ -1885,12 +1893,16 @@ void
UpdateRecord *rec = (UpdateRecord *)message;
//
// MIXED-VERSION GUARD: a peer on a build without this extension writes a
// plain Player-sized record, and reading the two fields off the end would
// pick up whatever record was packed after it in the update stream and
// display it as a kill count. Trust the length the writer stamped. (The
// stream itself stays in sync either way -- both sides advance by the
// WRITER's recordLength, ENTITY.cpp:390 / SIMULATE.cpp:323.)
// MIXED-VERSION GUARD -- and the reason is NOT stream desync. Both sides
// advance by the WRITER's stamped recordLength (ENTITY.cpp:390 /
// SIMULATE.cpp:323) and no reader asserts a length, so an old 60-byte record
// read by this 68-byte reader keeps the stream perfectly in sync. The real
// hazard is a READ PAST THE ALLOCATION: an inbound update message is copied
// into a per-event heap block sized from messageLength (EntityManager's
// ReceiveNetworkPacket -> application->Post -> new long[(len+3)>>2]), so on a
// TRAILING legacy record these two fields would be read off the end of that
// block -- garbage displayed as a kill count, or worse. Trust the length the
// writer stamped.
//
if (rec->recordLength < sizeof(UpdateRecord))
{
@@ -2043,8 +2055,9 @@ void BTPostKillScore(Entity *victim, Scalar damage) // Step 7: KILL (+ MP deat
// tallies its own copies self-consistently". That was FALSE and it is what hid
// this bug: the reroute means exactly ONE node increments, no other node can
// observe the killer at all, and nothing carried the value back out -- so every
// remote pilot's KILLS read 0 on every other pod, all mission (measured: 125 of
// 125 `SCORE type=2` rows credit the logging node's own player). The counters are
// remote pilot's KILLS read 0 on every other pod, all mission (corpus invariant:
// 0 of 18818 DMG rows are inst=R, so no other node can even attribute the kill;
// every DEATH inst=R row reads killer=0:0). The counters are
// now replicated owner->replicant instead; see Read/WriteUpdateRecord above.
BTPlayer *killer_player = 0;
if (application->GetHostManager() != 0)