dincli: auditor commit retry overwrites the committed salt (auditor-side twin of PR No. 197 No. 1) + aggregate-t2 uses a stale T1 batch id #202

Summary

Two dincli bugs found while landing PR No. 197 (merge-proposal comment, dincli/cli/aggregator.py row). Both are older than PR No. 197 and out of its scope. Line numbers are for develop once PR No. 197 has landed.

Part 1: auditor commit retry overwrites the committed salt (slash risk)

This is the auditor-side copy of the bug PR No. 197 fixed for aggregators (review finding No. 1).

dincli auditor lms-evaluation evaluate --submit (dincli/cli/auditor.py, evaluate_lms, L530–553) does the following for every assigned LM on every run:

1. generates a fresh salt = secrets.token_bytes(32) (L533);
2. sends commitAuditScore(GI, batchId, modelIndex, commitHash) with exit_on_failure=False;
3. unconditionally calls _save_commit(...) (L550), overwriting <model dir>/audits/commits/gi_<GI>_batch_<b>_lm_<m>.json.

build_and_send_tx(..., exit_on_failure=False) returns None on a failed gas estimate, a reverted tx, or a failed receipt wait; it doesn't raise. So the except never fires and the cache is overwritten either way. There's also no "already committed" check, so a rerun re-evaluates and re-commits every assigned LM.

Failure scenario: an auditor assigned to LMs 0 and 1 runs evaluate --submit. LM 0's commit lands, and LM 1's tx fails (RPC hiccup, gas). They rerun. LM 0 gets a new salt, commitAuditScore reverts with TA_AlreadyCommitted (and returns None), and LM 0's cache is overwritten with the new salt. The later reveal for LM 0 then fails the hash check, the vote is never revealed, and slashAuditors hits them with AUD_NO_VOTE (S1) for an honest, correct evaluation.

Fix: mirror PR No. 197's aggregator fix (dincli/cli/aggregator.py aggregate_t1/aggregate_t2):

• before evaluating an LM, read hasCommittedLM(GI, batchId, account, modelIndex) on DINTaskAuditor and skip it if it's already committed, leaving its cache untouched;
• write the cache before sending the commit tx, not after. Save-only-on-receipt would lose the preimage when the receipt wait fails for a tx that still mines, because build_and_send_tx returns None in that case too;
• add tests like tests/test_aggregator_commit_retry.py: an already-committed LM leaves the cache byte-identical and sends no tx; on a failed send, the cache existed at send time and hashes to exactly the commit that was sent.

This is independent of No. 192, which binds msg.sender into the auditor commit hash. If No. 192 lands first, the test's hash helper must follow the new preimage.

Part 2: aggregate-t2 uses a stale T1 batch id for its worker job

In aggregate_t2 (dincli/cli/aggregator.py), the T2 batch loop unpacks (bid, aggregators, finalized, cid) = getTier2Batch(GI, i) (L525). The inner loop that collects the T1 final CIDs then rebinds bid:

for j in range(t1_batches_count):                                  # L545
    (bid, val, idxs, fin, cid) = ...getTier1Batch(curr_GI, j).call()   # L547

After that loop, bid holds the last T1 batch's id, not the T2 batch id, and it's used for:

• aggregator_models_path = .../"T2"/str(bid)/"models" (L578);
• the worker job name aggregator_t2_gi_{GI}_batch_{bid} (L586);
• the container name din-worker-aggregator-t2-model-…-batch-{bid} (L599);
• the log messages after the loop.

The on-chain commit and the commit cache use i, so the committed CID is correct. But the T2 working directory, job and container are named after an unrelated T1 batch. They can collide with that T1 batch's naming conventions and are misleading when debugging.

Fix: rename the inner loop's unpacked variables (e.g. (t1_bid, _, _, _, t1_cid)), and use the T2 id consistently (bid from getTier2Batch, which is always 0, or i). A unit test can assert the job/container name passed to write_worker_job/run_worker_container carries the T2 batch id when tier1BatchCount > 1.

(Minor, same function: if batch_id and batch_id >= t2_batches_count / if batch_id: use truthiness, so --batch 0 is treated as "no batch given". Harmless today because T2 has exactly one batch, but worth switching to is not None, like aggregate_t1, while in there.)

Related

PR No. 197 (aggregator-side fix and tests), No. 192 (auditor commit hash sender binding), No. 156 (M-1), No. 201 (Part B: slash reason for committed-but-unrevealed).