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

[umeradl]

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).

[umeradl]

Done by PR No. 214, merged into develop as d2d2213, with the review's ROADMAP follow-up in e491f0b (outcome comment, green develop push run). Closing by hand, because a merge into develop doesn't trigger closing keywords (main is the default branch).

Checked on origin/develop @ e491f0b:

• Part 1 (auditor commit retry):
  • evaluate_lms checks hasCommittedLM(curr_GI, batch_id, account, model_index) with --submit and skips an already-committed LM before evaluating it, drawing a salt or touching its cache (dincli/cli/auditor.py:438).
  • _save_commit(...) now runs before build_and_send_tx(...) (auditor.py:568-569).
  • Tests in tests/test_auditor_commit_retry.py:
    • test_already_committed_lm_is_skipped_and_cache_untouched and test_all_committed_sends_nothing: the cache stays byte-identical and no tx is sent.
    • test_cache_written_before_send_and_matches_sent_commit: the cache exists at send time and hashes via _audit_commit_hash (No. 192's sender-bound preimage) to exactly the commit that was sent.
• Part 2 (aggregate-t2 batch id):
  • The inner T1 loop unpacks into (_, _, _, _, t1_final_cid) (dincli/cli/aggregator.py:550), so bid from getTier2Batch (:525) names the T2 models path (:581), the worker job (:589) and the container (:602).
  • The minor point is also done: both batch_id checks use is not None (:516, :521).
  • tests/test_aggregator_t2_batch_id.py asserts that the job and container names carry the T2 batch id, for both no --batch and --batch 0, with more than one T1 batch. It also keeps the out-of-range rejection.
• task_021026_19 Part D deliverables: D1 and D2 with their tests, and pytest -m "not integration" green: 395 passed locally, and green in CI on e491f0b.
• Docs: DINTaskAuditor.md §13 No. 9 and DINTaskCoordinator.md §10 No. 10 mark the bugs fixed. The auditors.md --submit row describes safe reruns. The stale No. 202 sentence is gone from Developer/ROADMAP.md.

Out of scope, not tracked in an issue yet: the PR review's optional hardening for evaluate_lms, aggregate_t1 and aggregate_t2. It would reuse an existing cache entry's preimage for a slot that isn't committed yet, which closes the rerun window while a commit tx is still pending.