Skip to content

fix(cli): keep auditor commit salts across retries; name aggregate-t2 work after the T2 batch (#202) - #214

Merged
umeradl merged 1 commit into
InfiniteZeroFoundation:developfrom
umermjd11:fix/auditor-commit-retry-and-t2-batch-id
Oct 5, 2026
Merged

fix(cli): keep auditor commit salts across retries; name aggregate-t2 work after the T2 batch (#202)#214
umeradl merged 1 commit into
InfiniteZeroFoundation:developfrom
umermjd11:fix/auditor-commit-retry-and-t2-batch-id

Conversation

@umermjd11

@umermjd11 umermjd11 commented Oct 2, 2026

Copy link
Copy Markdown
Collaborator

Summary

PR 3 of task_021026_19, Part D. This is dincli only, with no contract changes. Closes #202.

It builds on #211 (getAggregatorSubmission) and #212 (_audit_commit_hash), both merged.

D1. Auditor commit retry (issue #202 Part 1)

The bug: dincli auditor lms-evaluation evaluate --submit drew a fresh salt for every LM on every run, and wrote the commit cache unconditionally after the send.

Suppose LM 0's commit lands and LM 1's fails, and the auditor reruns. LM 0 gets a new salt, commitAuditScore reverts TA_AlreadyCommitted (build_and_send_tx returns None), and LM 0's cache is overwritten anyway. Its reveal then fails TA_RevealHashMismatch, and slashAuditors hits an honest auditor with AUD_NO_VOTE (S1).

The fix mirrors PR #197's aggregator fix in evaluate_lms:

  • An LM with hasCommittedLM(gi, batchId, account, modelIndex) already set is skipped before re-evaluating, and its cache is left untouched.
  • The cache is written before the commit tx is sent. build_and_send_tx(..., exit_on_failure=False) returns None both on a revert and on a failed receipt wait for a tx that still mines.

D2. aggregate-t2 batch id (issue #202 Part 2)

The bug: aggregate_t2 read bid from getTier2Batch, then rebound it while looping over T1 batches. After the loop, bid held the last T1 batch's id. That id named the T2 models path, the worker job, the container and the log lines. The on-chain commit used i and was correct.

The fix:

  • The inner loop now unpacks into (_, _, _, _, t1_final_cid).
  • --batch checks use is not None, matching aggregate_t1. Before, --batch 0 was treated as no batch.

Tests

  • tests/test_auditor_commit_retry.py, mirroring tests/test_aggregator_commit_retry.py:
    • LM 0 committed, LM 1 not: LM 0's cache stays byte-identical. Only LM 1 is evaluated (one worker run) and committed.
    • All LMs committed: no tx and no worker run.
    • Failed send: the cache existed at send time for each LM, and _audit_commit_hash of the cached (score, vote, salt) equals the hash actually sent.
  • tests/test_aggregator_t2_batch_id.py, with 3 T1 batches:
    • The job is named aggregator_t2_gi_1_batch_0, the container ends -batch-0, and the models path is .../T2/0/models.
    • bid passed to the worker is 0, and all 3 T1 CIDs are collected.
    • Both cases run with and without --batch 0.
    • --batch 1 is still rejected.
  • The tests catch the bugs: run against the old auditor.py/aggregator.py, all 3 auditor tests and both T2 naming cases fail. Only the out-of-range check passes there, because that behaviour is unchanged.

Docs

  • DINTaskAuditor.md §13 No. 9 and DINTaskCoordinator.md §10 No. 10 are marked fixed.
  • The --submit row in auditors.md now describes the commit and the safe rerun, like aggregators.md already does.

Verification

  • pytest -m "not integration": 319 passed, including the 6 new tests. The torch-dependent files and the 2 test_integration_marker checks that collect them can't run in my local venv (no torch), the same as on develop. CI installs the full deps.
  • check_doc_links.py Documentation: all links resolve.
  • No Solidity changes, so forge results are unchanged.

🤖 Generated with Claude Code

@umermjd11 @claude
fix(cli): keep auditor commit salts across retries; name aggregate-t2…
25dcaa7 
… work after the T2 batch (InfiniteZeroFoundation#202)

Part 1 - auditor commit retry (auditor-side twin of PR InfiniteZeroFoundation#197's aggregator
fix). `auditor lms-evaluation evaluate --submit` drew a fresh salt for
every LM on every run and wrote the commit cache unconditionally after
the send. A rerun after a partial failure overwrote the salt behind an
LM already committed on-chain, so its reveal failed TA_RevealHashMismatch
and the auditor was S1-slashed for an honest vote. Now:
- an LM with hasCommittedLM set is skipped before re-evaluating, leaving
  its cache untouched;
- the cache is written before the commit tx is sent, because
  build_and_send_tx returns None both on a revert and when the receipt
  wait fails for a tx that still mines.

Part 2 - aggregate_t2 rebound `bid` while collecting T1 final CIDs, so
the T2 models path, worker job and container were named after the last
T1 batch. The inner loop now unpacks into distinct names, and --batch
uses `is not None` (0 was treated as "no batch").

Tests: tests/test_auditor_commit_retry.py (skip-if-committed, all
committed sends nothing, cache-before-send matches the sent hash) and
tests/test_aggregator_t2_batch_id.py (T2 naming with 3 T1 batches, with
and without --batch 0). All new tests fail against the old code.
Docs: DINTaskAuditor.md No. 9 and DINTaskCoordinator.md No. 10 marked
fixed; auditors.md --submit row describes commit + safe re-run.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@umermjd11
umermjd11 requested a review from umeradl October 2, 2026 23:44
@umermjd11 umermjd11 mentioned this pull request Oct 3, 2026
Merged
@umeradl

umeradl commented Oct 3, 2026

Copy link
Copy Markdown
Member

Reviewed against develop in an isolated worktree at PR head 25dcaa7 (1 commit, merge-base 7785acf). The branch applies cleanly on top of current develop tip 4ec42d4: git merge-tree --write-tree exits 0 locally, and GitHub reports mergeable: MERGEABLE, mergeStateStatus: CLEAN. The only commit develop gained since the merge-base touches .claude/skills only. I ran the old and new code against the new tests, and reverted each sub-fix on its own.

Claimed: the 6 new tests pass, and against the old auditor.py/aggregator.py all 3 auditor tests and both T2 naming cases fail; only the out-of-range check passes.

Verified — exact match: at the PR head, tests/test_auditor_commit_retry.py + tests/test_aggregator_t2_batch_id.py → 6 passed. Swapping in the merge-base versions of dincli/cli/auditor.py and dincli/cli/aggregator.py → 5 failed, 1 passed. The one pass is test_batch_out_of_range_still_rejected.

Going further — which test guards which change: I reverted each sub-fix on its own:

Reverted sub-fix Result
hasCommittedLM skip guard in evaluate_lms 2 failed (…already_committed_lm_is_skipped_and_cache_untouched, …all_committed_sends_nothing)
_save_commit moved back to after build_and_send_tx 1 failed (…cache_written_before_send_and_matches_sent_commit)
T1 loop unpacking back into bid 2 failed (both T2 naming parametrisations)
batch_id is not None → batch_id (truthiness), both sites in aggregate_t2 0 failed, see below

The is not None change can't fail a test today, and that is expected rather than a gap. The contract creates exactly one T2 batch (tier2Batches[_GI].push() with batchId = 0), and aggregate_t2 hard-codes t2_batches_count = 1. So --batch 0 and no --batch walk the same single batch under either check. The change brings aggregate_t2 in line with aggregate_t1, and it starts to matter if more than one T2 batch ever exists.

Claimed: hasCommittedLM(gi, batchId, account, modelIndex) is the right on-chain check.

Verified: foundry/src/DINTaskAuditor.sol declares hasCommittedLM as a public mapping, set in commitAuditScore and checked in revealAuditScore. The bundled dincli/abis/DINTaskAuditor.json exposes hasCommittedLM(uint256,uint256,address,uint256), which matches the call's argument order. The guard sits after the --lmi filter and after found_any = True, so a fully committed batch doesn't print the misleading "No matching assigned auditor batches found."

Claimed: the fix mirrors PR No. 197's aggregator fix.

Verified: the cache-before-send block and its rationale follow aggregate_t1/aggregate_t2 in dincli/cli/aggregator.py almost line for line (_save_agg_commit immediately before build_and_send_tx(..., exit_on_failure=False)).

Claimed: pytest -m "not integration": 319 passed (venv without torch).

Verified — 395 passed in a venv with torch installed, at the PR head and on a trial merge into develop 4ec42d4. The difference is the torch-dependent files that the PR's venv couldn't collect, as the PR says. GitHub CI on the PR head: Solidity / Python / Docs / CI OK all pass.

Claimed: all doc links resolve.

Verified: .github/scripts/check_doc_links.py Documentation → 258 links, all resolve. git diff --check 7785acf 25dcaa7 is clean.

Not independently re-verified: an end-to-end commit/reveal against a live chain. The tests drive evaluate_lms/aggregate_t2 with mocked contracts and workers. That matches how tests/test_aggregator_commit_retry.py covers the aggregator twin.

Notes (non-blocking):

  • Developer/ROADMAP.md line 22 is now stale. It still says "dincli's auditor commit retry can still lose a committed salt (No. 202)". This PR doesn't touch it, so it remains after merge. It should be removed or marked fixed alongside this merge.
  • A narrow pending-tx window remains, as on the aggregator side. The guard reads hasCommittedLM from chain state. Suppose a first attempt's receipt wait fails while its tx is still in the mempool, and the auditor reruns before it mines. The guard won't see the commit yet, the rerun draws a fresh salt and overwrites the cache, and if the first tx then mines, the cached preimage no longer matches it. aggregate_t1/aggregate_t2 have the same window after PR No. 197, so this PR is consistent with the accepted design. A possible later hardening for both roles: when a cache entry already exists for an uncommitted slot, reuse its (score, vote, salt) instead of drawing a new salt. Then whichever attempt mines matches the cached preimage.

Every checkable claim held up. The bug fixes are each pinned by a test that fails when the fix is reverted. The one untestable change (is not None) is a consistency fix that has no observable effect while there is only one T2 batch. Looks good to merge. The only follow-up I'd fold into the merge is the stale Developer/ROADMAP.md line.

@umeradl

umeradl commented Oct 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Files changed (7) — as of 25dcaa7 (PR head)

Diffed against merge-base 7785acf (develop). develop has moved 1 commit since (4ec42d4, .claude/skills only), which doesn't overlap with this PR's files. GitHub agrees: mergeable: MERGEABLE, mergeStateStatus: CLEAN. A local git merge-tree --write-tree dry run is clean (exit 0).

dincli/cli/auditor.py

Field Value
Change Modified
Lines +16/-3
Diff (what exactly is in this PR) In evaluate_lms: with --submit, any LM for which hasCommittedLM(curr_GI, batch_id, account, model_index) is true is skipped before evaluation (yellow notice pointing at reveal). _save_commit(...) moves from after build_and_send_tx(...) to immediately before it.
Functionality — how & why How: the skip runs after the --lmi filter and found_any = True, so no worker run, salt or cache write happens for a slot already committed on-chain. For uncommitted slots, the (score, vote, salt) preimage is on disk before the tx is signed and sent. Why: a rerun after a partial failure used to draw a new salt for an LM already committed and overwrite its cache. Its reveal then failed TA_RevealHashMismatch, and slashAuditors S1-slashed an honest auditor for AUD_NO_VOTE. Saving only after the send could also strand a mined commit without its preimage, because build_and_send_tx returns None on a failed receipt wait too. Break-then-fix: reverting the guard fails 2 tests; reverting the ordering fails 1.
Diff vs current develop HEAD None — untouched by develop since merge-base
Recommended merge proposal Merge as-is.
Actual merge proposal Applied as-is, byte-identical to PR head. Merged in d2d2213. The optional cache-reuse hardening isn't in this merge. Evidence on the merged tree (develop @ 4ec42d4 + this PR): forge build (via_ir) clean; forge test 484 passed / 0 failed / 0 skipped; pytest -m "not integration" 395 passed / 0 failed (389 before + the 6 new tests).
Pending proposal Optional later hardening, shared with aggregate_t1/aggregate_t2: reuse an existing cache entry's (score, vote, salt) for an uncommitted slot, which closes the pending-tx rerun window. Not needed for this PR.
Local merge conflict No
GitHub merge conflict No

dincli/cli/aggregator.py

Field Value
Change Modified
Lines +7/-4
Diff (what exactly is in this PR) In aggregate_t2: the T1-collection loop unpacks getTier1Batch into (_, _, _, _, t1_final_cid) instead of rebinding bid. Both --batch checks use batch_id is not None instead of truthiness.
Functionality — how & why How: bid now keeps the T2 batch id read from getTier2Batch, so the models path (.../T2/<bid>/models), worker job (aggregator_t2_gi_<gi>_batch_<bid>), container name and log lines all name the T2 batch. Why: after the loop, bid held the last T1 batch's id, so T2 work was filed under an unrelated batch. The on-chain commit already used i and was unaffected. The is not None change matches aggregate_t1. It has no observable effect today, because the contract creates exactly one T2 batch and dincli hard-codes t2_batches_count = 1, which is why no test can catch it. Break-then-fix: reverting the unpacking fails both naming tests.
Diff vs current develop HEAD None — untouched by develop since merge-base
Recommended merge proposal Merge as-is.
Actual merge proposal Applied as-is, byte-identical to PR head. Merged in d2d2213. Evidence on the merged tree (develop @ 4ec42d4 + this PR): forge build (via_ir) clean; forge test 484 passed / 0 failed / 0 skipped; pytest -m "not integration" 395 passed / 0 failed (389 before + the 6 new tests).
Pending proposal None
Local merge conflict No
GitHub merge conflict No

tests/test_auditor_commit_retry.py, tests/test_aggregator_t2_batch_id.py

Field Value
Change New (2 files)
Lines +187/-0, +105/-0
Diff (what exactly is in this PR) Auditor: an LM already committed is skipped with its cache byte-identical; an all-committed run sends nothing and runs no worker; with a failed send, the cache exists at send time and the _audit_commit_hash of the cached preimage equals the hash sent. T2: 3 T1 batches, run with and without --batch 0; asserts the job, container and models path name batch 0 and that all 3 T1 CIDs are collected; --batch 1 is still rejected.
Functionality — how & why How: mocked contracts, worker and build_and_send_tx, in the style of tests/test_aggregator_commit_retry.py. Unmarked, so they run in CI's not integration job. Why: they pin both issue No. 202 bugs. Against the old source, 5 of 6 fail; the out-of-range check passes because that behaviour is unchanged.
Diff vs current develop HEAD None — new files
Recommended merge proposal Merge as-is.
Actual merge proposal Both applied as-is, byte-identical to PR head; all 6 tests pass (6 passed run on its own and in the full suite). Merged in d2d2213. Evidence on the merged tree (develop @ 4ec42d4 + this PR): forge build (via_ir) clean; forge test 484 passed / 0 failed / 0 skipped; pytest -m "not integration" 395 passed / 0 failed (389 before + the 6 new tests).
Pending proposal None
Local merge conflict No
GitHub merge conflict No

Documentation/technical/contracts/DINTaskAuditor.md, Documentation/technical/contracts/DINTaskCoordinator.md, Documentation/public/roles/auditors.md

Field Value
Change Modified (3 files)
Lines +1/-1 each
Diff (what exactly is in this PR) DINTaskAuditor.md §13 No. 9 is rewritten as Fixed (skip-if-committed + cache-before-send, rerun safe). DINTaskCoordinator.md §10 No. 10 moves the aggregate-t2 naming bug into a "fixed in issue No. 202 Part 2" parenthetical, and keeps the constructor-lag item. The auditors.md --submit row now describes commit-then-reveal, the local preimage cache and safe reruns.
Functionality — how & why How: documentation only. Why: it keeps Documentation/ describing develop as it is: the "don't rerun" warning is no longer needed. Link check: 258 links, all resolve.
Diff vs current develop HEAD None — untouched by develop since merge-base
Recommended merge proposal Merge as-is, plus one doc fix outside the PR: Developer/ROADMAP.md line 22 still says "dincli's auditor commit retry can still lose a committed salt (No. 202)". Drop or mark fixed at merge time.
Actual merge proposal All three applied as-is, byte-identical to PR head, in d2d2213. The out-of-PR fix is in e491f0b: the stale No. 202 sentence is dropped from Developer/ROADMAP.md line 22. Doc links check passed in the local CI mirror on e491f0b.
Pending proposal Developer/ROADMAP.md line 22 stale No. 202 sentence (not in this PR).
Local merge conflict No
GitHub merge conflict No

Verification

Full detail is in the verification comment above. In short: 6 of 6 new tests pass, and 5 of 6 fail against the old source. Each bug-fix sub-change is caught when reverted on its own. pytest -m "not integration" gives 395 passed at the PR head and on a trial merge with develop 4ec42d4. Doc links: 258/258 resolve. git diff --check is clean. GitHub CI is green.

Local vs. GitHub agree: both report a clean merge; no file overlap with the 1 commit develop gained since the merge-base.

@umeradl
umeradl merged commit d2d2213 into InfiniteZeroFoundation:develop Oct 5, 2026
4 checks passed
@umeradl

umeradl commented Oct 5, 2026

Copy link
Copy Markdown
Member

Actual outcome — PR No. 214 merged + review follow-up applied (pushed)

This supersedes the pre-merge review comment above with what actually happened when landing this PR on develop.

Two commits, both on origin/develop:

  1. d2d2213 — real merge of this PR (merge commit, not squash) at its head 25dcaa7, authored as umermjd11, on top of develop 4ec42d4. No conflicts, as predicted. GitHub agrees: PR shows MERGED.
  2. e491f0b — follow-up commit with the out-of-PR doc fix from the review.

Files unchanged from the PR (7 of 7)

All 7 files are byte-identical to 25dcaa7:

  • dincli/cli/auditor.py, dincli/cli/aggregator.py
  • tests/test_auditor_commit_retry.py, tests/test_aggregator_t2_batch_id.py
  • Documentation/technical/contracts/DINTaskAuditor.md, Documentation/technical/contracts/DINTaskCoordinator.md, Documentation/public/roles/auditors.md

Outside the PR, changed by e491f0b (1 file)

File What changed
Developer/ROADMAP.md Line 22 drops "dincli's auditor commit retry can still lose a committed salt (No. 202)", which this PR fixes. The rest of the line is unchanged.

Why the change

It's the review's recommended doc fix, not develop drift. Nothing in this PR needed a functional fix.

Verification

On the merged tree (develop 4ec42d4 + this PR):

  • forge build (via_ir) is clean, and forge test gives 484 passed, 0 failed (same as the 4ec42d4 baseline).
  • pytest -m "not integration": 395 passed, 0 failed. That's the baseline 389 plus the 6 new tests. The 2 new files also pass on their own (6 passed).
  • The local CI mirror on e491f0b passed: doc links and pytest with an empty HOME. Solidity was skipped because nothing under foundry/, hardhat/ or .github/ changed.
  • GitHub push run on develop: green (Docs, Python, Solidity, CI OK).

Local vs. GitHub agree: clean merge with no conflicts, as both predicted. Issue No. 202 is fixed by this PR.

@umeradl umeradl mentioned this pull request Oct 5, 2026
Closed
@umermjd11 umermjd11 mentioned this pull request Oct 5, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@umeradl umeradl Awaiting requested review from umeradl

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@umermjd11 @umeradl