Skip to content

fix(foundry+dincli): commit-then-reveal for T1/T2 aggregation (issue #156 M-1, Part C) - #197

Merged
umeradl merged 6 commits into
InfiniteZeroFoundation:developfrom
Abidoyesimze:feat/issue-156-part-c-commit-reveal
Sep 30, 2026
Merged

umeradl merged 6 commits into
InfiniteZeroFoundation:developfrom
Abidoyesimze:feat/issue-156-part-c-commit-reveal

Conversation

@Abidoyesimze

@Abidoyesimze Abidoyesimze commented Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

PR 3 of 3 for task_240926_18, closing #156 — following up on Part A (#176/#186), Part B (#191, H-2 batch-assignment seed), and task_16's PR #171/#173. Fixes the aggregation half of #156's M-1 finding: submitT1Aggregation/submitT2Aggregation accepted a plaintext CID in a single transaction, so any aggregator who wasn't first to submit in their batch could read every prior submission from public state (t1SubmissionCID/t2SubmissionCID) and copy it instead of doing the actual aggregation work.

What changed

  • DINShared.sol: GIstates gains T1AggregationRevealStarted/T2AggregationRevealStarted, each inserted immediately after its commit-phase state (same lifecycle-position precedent LMSevaluationRevealStarted set — not appended). New commit-reveal custom errors mirroring PR feat(auditing): commit-then-reveal scoring, encrypted test-data keys, resampling policy #63's auditor-side error set.
  • DINTaskCoordinator.sol: submitT1Aggregation/submitT2Aggregation replaced by commitT1Aggregation/revealT1Aggregation and the T2 equivalents. startT1AggregationReveal/startT2AggregationReveal (model-owner-only) close the commit window and open the reveal window. finalizeT1Aggregation/finalizeT2Aggregation now gate on the reveal state instead of the commit state — their body is otherwise unchanged, since t1Submitted/t2Submitted and t1SubmissionCID/t2SubmissionCID are now written at reveal time only, so a committed-but-never-revealed aggregator simply never sets them and slashAggregators' existing "no submission" (S2) check needed no changes.
  • The hardening (deliberately past security: aggregator batch shuffle uses grindable blockhash + no commit-reveal on T1/T2 submissions (H-2/M-1 aggregation-side) #156's own proposal): the commit hash is keccak256(abi.encode(cid, salt, msg.sender, GI, tierKind, batchId)), not keccak256(cid, salt) as security: aggregator batch shuffle uses grindable blockhash + no commit-reveal on T1/T2 submissions (H-2/M-1 aggregation-side) #156's issue text proposes. Binding msg.sender (plus GI/tier/batchId) closes a replay path the unbound version leaves open: without it, an aggregator could copy a peer's commit hash verbatim and reveal the peer's (cid, salt) under their own address once the peer reveals — reproducing the exact free-riding commit-reveal exists to prevent, just one step removed from "copy the plaintext CID." See test_copyAttack_replayingPeerCommitHash_cannotReveal for the PoC.
  • PR feat(auditing): commit-then-reveal scoring, encrypted test-data keys, resampling policy #63's auditor-side commit hash has this identical weakness (keccak256(abi.encodePacked(score, vote, salt)), no sender bound in) and is not fixed here — opened #192 to track it separately, per the task's explicit scope note.
  • dincli: aggregator aggregate-t1/aggregate-t2 --submit now commit a hidden hash and cache (cid, salt) locally (never regenerated on retry, same pattern as auditor.py's commit-reveal). New aggregator reveal-t1/reveal-t2 commands read that cache and reveal once the model owner opens the reveal window via new model-owner aggregation T1/T2 start-reveal commands. ABI regenerated via the standard dump-abi process (same Python-3.10-sandbox caveat as PR fix(foundry+dincli): ungrindable batch-assignment seed (issue #156 H-2, Part B) #191 — the extraction logic was run directly since the CLI entrypoint needs 3.12; diff verified entry-for-entry against the actual artifact).
  • Tests: new foundry/test/AggregatorCommitReveal.t.sol (12 tests) covering the copy-attack PoC, reveal-window/hash-mismatch/double-commit/double-reveal reverts, a commit-without-reveal S2-slash PoC, and a full-GI walk asserting GIstate through both new reveal states. 8 other test files updated for the commit+reveal call shape (GasSimulation.t.sol's dedicated submit-gas benchmarks split into separate commit/reveal measurements, since that's now genuinely two transactions). tests/test_aggregator_commit_hash.py cross-checks the Python-side hash computation against golden vectors computed independently via cast abi-encode/cast keccak (not by comparing the implementation to itself) — this sandbox can't execute it directly (same Python 3.10 vs required 3.12 collection issue noted in PR fix(foundry+dincli): ungrindable batch-assignment seed (issue #156 H-2, Part B) #191), so I additionally ran the exact hash logic standalone against both golden vectors to confirm correctness.

Ordinal table (posted to PR #29 for the subgraph mapping)

Ordinal State
17 T1AggregationStarted (commit phase)
18 T1AggregationRevealStarted new
19 T1AggregationDone (was 18)
20 T2AggregationStarted (was 19, commit phase)
21 T2AggregationRevealStarted new
22 T2AggregationDone (was 20)
23 AuditorsSlashed (was 21)
24 AggregatorsSlashed (was 22)
25 GIended (was 23)

Two new events (T1AggregationCommitted/T2AggregationCommitted) added speculatively for subgraph commit-time visibility — flagged as removable in the PR #29 comment if not needed.

Contract size

Contract Before After Δ Margin after (24,576 B limit)
DINTaskCoordinator 21,342 B 23,242 B +1,900 B 1,334 B
DINTaskAuditor 22,406 B 22,406 B — unchanged (Part C doesn't touch this contract)

Note: this PR was branched directly off develop rather than stacked on #191 (Part B), since Part C's M-1 fix is functionally independent of Part B's H-2 fix — different functions in the same files, no shared state. Will rebase after #191 merges if needed.

Test plan

  • forge test --match-contract AggregatorCommitRevealTest -vv — 12/12 pass
  • forge test (full suite, after forge clean && forge build --build-info) — 416/416 pass, 34 suites
  • _agg_commit_hash verified against 2 golden vectors computed independently via cast abi-encode + cast keccak (tests/test_aggregator_commit_hash.py; couldn't execute directly in this sandbox — see note above, verified by running the identical logic standalone instead)

Closes #156.

Abidoyesimze and others added 5 commits September 29, 2026 01:05
submitT1Aggregation/submitT2Aggregation accepted a plaintext CID in a
single transaction, letting any aggregator who isn't first to submit
in their batch read every prior submission from public state and copy
it instead of doing the aggregation work (issue InfiniteZeroFoundation#156 M-1). Replace
with commitT1Aggregation/revealT1Aggregation and the T2 equivalents,
mirroring PR InfiniteZeroFoundation#63's auditor-side commit-reveal shape with one
deliberate hardening: the commit hash binds msg.sender (plus GI, tier,
batchId) --

    keccak256(abi.encode(cid, salt, msg.sender, GI, tierKind, batchId))

-- rather than issue InfiniteZeroFoundation#156's own keccak256(cid, salt) proposal. Without
the sender binding, a lazy aggregator could copy a peer's *commit
hash* itself and reveal the peer's (cid, salt) under their own name
once the peer reveals, reproducing the same free-riding this fix is
meant to close. PR InfiniteZeroFoundation#63's auditor-side hash has this identical
weakness and is deliberately left unfixed here -- tracked separately
in InfiniteZeroFoundation#192.

GIstates gains T1AggregationRevealStarted/T2AggregationRevealStarted,
inserted immediately after their corresponding commit-phase state
(same lifecycle-position precedent as LMSevaluationRevealStarted, not
appended). t1Submitted/t2Submitted and t1SubmissionCID/t2SubmissionCID
are now written at reveal time only; a committed-but-never-revealed
aggregator simply never sets them, so slashAggregators()'s existing
"no submission" (S2) check needed no changes to keep working.
finalizeT1Aggregation/finalizeT2Aggregation now gate on the reveal
state instead of the commit state; their body is otherwise unchanged.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
AggregatorCommitReveal.t.sol (new): the actual property M-1's fix is
about -- test_copyAttack_replayingPeerCommitHash_cannotReveal proves
an aggregator who copies a peer's commit hash verbatim cannot reveal
the peer's (cid, salt) under their own address, since the hash the
contract expects for their own reveal is computed with their own
address baked in. Plus reveal-before-window/without-commit/wrong-cid/
wrong-salt/double-commit/double-reveal reverts, finalize-before-reveal
gating, a commit-without-reveal S2-slash PoC, and a full-GI walk
through both new reveal states (asserting GIstate at each step).

The other 8 files gain commit+reveal (and, for the 0-T2-batch cases,
a startT2AggregationReveal call) ahead of what were direct
submitT1Aggregation/submitT2Aggregation calls, and
GasSimulation.t.sol's dedicated submit-gas benchmarks split into
separate commit/reveal measurements (revealT1Aggregation is what now
does the vote-counting write the old single-shot submit did).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
aggregator aggregate-t1/aggregate-t2 --submit now commits a hidden
CID hash instead of submitting the plaintext CID directly, caching
(cid, salt) locally the same way auditor.py's commit-reveal flow does
(never regenerated on retry). New aggregator reveal-t1/reveal-t2
commands read that cache and call revealT1Aggregation/
revealT2Aggregation once the model owner opens the reveal window via
the new model-owner aggregation T1/T2 start-reveal commands.
aggregation T1/T2 close's GIstate gate moves from *AggregationStarted
to *AggregationRevealStarted to match.

_agg_commit_hash computes keccak256(abi.encode(cid, salt, msg.sender,
GI, tierKind, batchId)) via eth_abi.encode (standard, non-packed ABI
encoding) + Web3.keccak, matching commitT1Aggregation/
commitT2Aggregation's NatSpec exactly -- verified against golden
hashes computed independently via `cast abi-encode` + `cast keccak`
(tests/test_aggregator_commit_hash.py), not by comparing the
implementation to itself.

cli/utils.py's states/stateDescription GIstates mirrors gain
T1AggregationRevealStarted/T2AggregationRevealStarted in lifecycle
position, and drop a stale duplicate append of
LMSevaluationRevealStarted that had been sitting at the end of both
lists since before that insertion precedent was established.

Regenerate dincli/abis/DINTaskCoordinator.json (full dump-abi regen)
for the new commit/reveal functions, storage getters, and events.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
model-workflow.md: walk through the new commit/reveal split for T1
and T2 aggregation, including the model-owner start-reveal step
between commit and close.

DINShared.md: update the GIstates ordinal table and lifecycle diagram
for T1AggregationRevealStarted/T2AggregationRevealStarted; note the
commit-then-reveal shape mirrors the existing LMS evaluation section.

foundry-src-security-review.md: mark M-1's aggregation-side half
fixed (PR number filled in once opened), and note the auditor-side
half (PR InfiniteZeroFoundation#63) is deliberately left unfixed, tracked in the new InfiniteZeroFoundation#192.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…gation-side fix

Follow-up to the previous commit's placeholder now that the PR exists.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@umeradl

umeradl commented Sep 29, 2026 •

Copy link
Copy Markdown
Member

Re-review (2026-09-30): b5d602f (reviewed) → b3ff1b9 (current HEAD)

The only new commit is b3ff1b9, which merges develop @ 4658bf8 (PR No. 191, No. 184, No. 196, No. 200). No functional change to the PR's own code. GitHub now reports MERGEABLE / CLEAN, and a local git merge-tree --write-tree against develop @ 4658bf8 agrees (exit 0).

Before b3ff1b9 was pushed, I resolved the DINShared.sol conflict independently in a separate worktree. My resolution matches the author's byte-for-byte (keep both error blocks: develop's seed-lock errors first, then this PR's commit-reveal errors). Diffing my merged tree against b3ff1b9 leaves only the author's two test-file fixes. Before those fixes I reproduced both problems the author describes: DisputeResolution.t.sol hit the solc via_ir ICE (Tag too large for reserved space, the only one of 25 test files and 2 deploy scripts that failed when compiled one at a time), and all 12 AggregatorCommitReveal.t.sol tests failed with TC_AuditSeedNotLocked(). Both fixes are correct. The DisputeResolution refactor keeps the "agg" / "dissentAgg" address prefixes, so derived addresses and test behaviour are unchanged.

Verified on b3ff1b9:

  • forge build (real via_ir profile): compiles, no ICE.
  • forge test after forge clean: 39 suites, 467 passed, 0 failed, 0 skipped. (The author's comment says 465/465; I get 467, and AggregatorCommitRevealTest is 12/12.)
  • pytest -m "not integration" (dincli resolved into the worktree): 378 passed, 133 deselected, 0 failed. (+13 vs. the first review, all from develop.)
  • Bundled dincli/abis/DINTaskCoordinator.json vs. my own via_ir foundry/out artifact: identical, 213 entries each. The auto-merge of both PRs' ABI additions is correct.
  • GIstates enum vs. dincli/cli/utils.py states / stateDescription: 26 / 26 / 26, position-for-position. PR No. 191 added no states, so the ordinals are the same as in the first review (and the PR No. 29 table).
  • CI on b3ff1b9: Docs / Python / Solidity / CI OK all green.

New finding

No. 6 (blocking): after the merge, DINTaskCoordinator exceeds EIP-170 by 9 bytes and cannot be deployed. forge build --sizes on b3ff1b9 gives runtime 24,585 B, margin −9 B, and the command exits 1 with Error: some contracts exceed the runtime size limit (EIP-170: 24576 bytes). develop @ 4658bf8 alone is 22,689 B (margin 1,887 B), and this PR adds about 1.9 KB. On its own, before the merge, it was 23,242 B. Each PR fitted separately, but together they don't. CI didn't catch it: ci.yml runs plain forge build / forge test with no --sizes, and Solidity CI is green on b3ff1b9; forge test doesn't enforce the limit (my suite results were identical with and without FOUNDRY_DISABLE_CODE_SIZE_LIMIT=true). The repo's own foundry/anvil.sh runs with --code-size-limit 4294967295, so the local devnet hides it too. It only shows up on Optimism Sepolia or any real chain, where the coordinator deploy reverts.

Suggested fix: the T1 and T2 commit/reveal/start-reveal functions (DINTaskCoordinator.sol L807–1041: commitT1Aggregation/commitT2Aggregation, revealT1Aggregation/revealT2Aggregation, startT1AggregationReveal/startT2AggregationReveal) are near-duplicates. Folding each pair's body into one internal function taking the tier (the per-tier mappings and errors can be picked by a branch) should recover well over 9 B, and leaves some headroom. Please post the forge build --sizes line for DINTaskCoordinator with the fix. Anything that recovers only a few bytes will break again on the next feature. (Separately, a forge build --sizes step in CI would have caught this; I'll raise that on our side, not in this PR.)

Status of earlier findings on b3ff1b9

  • No. 1 (blocking): still open. dincli/cli/aggregator.py is unchanged. After the merge the lines are T1 409–423 and T2 614–625: a fresh secrets.token_bytes(32) on every --submit, exit_on_failure=False, and an unconditional _save_agg_commit(...). The pre-existing auditor-side twin is now at dincli/cli/auditor.py 533–550 on develop.
  • No. 2 (should fix before/at merge): still open. Documentation/technical/contracts/DINTaskCoordinator.md L144–145 / L249 still document submitT1Aggregation/submitT2Aggregation. Documentation/public/roles/aggregators.md and getting-started.md still have no reveal-t1/reveal-t2 step (0 mentions).
  • No. 3 (non-blocking): still open. 9 bare vm.expectRevert() in AggregatorCommitReveal.t.sol. T2 negative paths are still covered only via T1.
  • No. 4 (design note): unchanged, still a No. 155 mechanism question.
  • No. 5 (coordination): resolved. PR No. 191 has landed and is merged into this branch. The enum ordinals are unchanged by it, so the ordinal table posted to PR No. 29 is still correct.

Still not mergeable: No. 6 (size) and No. 1 (salt overwrite) are both blocking. No. 2 should land with it.


Original review (at b5d602f) below, unchanged.

Reviewed against develop in an isolated worktree (PR head b5d602f, 5 commits, merge-base 7b66392; develop is 11 commits ahead at ea349d6 but none of them touch any of this PR's 19 files — no conflicts, confirmed by both git merge-tree --write-tree locally and GitHub's own mergeable: MERGEABLE / mergeStateStatus: CLEAN). Ran the actual commit-reveal flow, the full suites, and mutation checks rather than just reading the diff.

Claimed: full suite passes, 416/416 across 34 suites; AggregatorCommitRevealTest 12/12.

Verified — holds: forge build + forge test on the real via_ir = true profile (npm ci first): 34 suites, 416 passed, 0 failed, 0 skipped. pytest -m "not integration" against the PR tree (not the editable install from develop — confirmed dincli.__file__ resolved into the worktree): 365 passed, 133 deselected, 0 failed — so tests/test_aggregator_commit_hash.py, which the PR notes couldn't be executed in the author's sandbox, does actually run and pass under Python 3.12.

Claimed: the sender-bound commit hash keccak256(abi.encode(cid, salt, msg.sender, GI, tierKind, batchId)) closes the "replay a peer's commit hash, reveal their (cid, salt) under your own address" path, and test_copyAttack_replayingPeerCommitHash_cannotReveal is the PoC.

Verified — holds, and the test genuinely guards it: broke the mechanism on purpose — replaced msg.sender with address(0) in revealT1Aggregation's expected-hash computation (and in the test's _t1CommitHash helper so honest reveals still line up) — and re-ran the suite: test_copyAttack_replayingPeerCommitHash_cannotReveal → [FAIL: next call did not revert as expected], the other 11 still pass. Restored, it passes again. So that test isn't tautological; it fails exactly when the sender binding is removed.

Claimed: Python _agg_commit_hash golden vectors were computed independently via cast abi-encode + cast keccak.

Verified — holds: recomputed both vectors myself with cast keccak $(cast abi-encode "f(bytes32,bytes32,address,uint256,uint8,uint256)" …): 0xe3d4a8c4…53aa70 (Tier1, GI 1, batch 0) and 0x66a96e77…b59cf0 (Tier2, GI 7, batch 3) — both byte-identical to the test's expected values.

Claimed: GIstates gains two states inserted in lifecycle position, and dincli/cli/utils.py's states/stateDescription mirrors were updated to match (removing a stale duplicate LMSevaluationRevealStarted append).

Verified — holds: parsed the Solidity enum and both Python lists programmatically: 26 / 26 / 26 entries, states == enum position-for-position, and every stateDescription lines up with its state (index 18 T1AggregationRevealStarted, 21 T2AggregationRevealStarted, 25 GIended). On develop the lists were 25 long with a duplicate LMSevaluationRevealStarted at both 14 and the tail — the removal is correct. Also grepped foundry/src for any ordinal comparison on GIstate (<, >=, uint8(GIstate)) that the renumbering could silently break: none — every gate is an exact ==/!= on a named state, including DINTaskAuditor's two cross-contract reads (AuditorsBatchesCreated, T2AggregationDone).

Claimed: bundled dincli/abis/DINTaskCoordinator.json regenerated and verified entry-for-entry.

Verified — holds: set-compared the bundled ABI against foundry/out/DINTaskCoordinator.sol/DINTaskCoordinator.json from my own via_ir build: identical, 195 entries each; submitT1Aggregation/submitT2Aggregation gone, the six new functions present.

Claimed: DINTaskCoordinator 23,242 B, 1,334 B margin.

Verified — holds: forge build --sizes → runtime 23,242 B, margin 1,334 B.

Claimed: aggregate-t1/aggregate-t2 --submit cache (cid, salt) locally and the salt is "never regenerated on retry".

Does not hold — see No. 1 below.


Findings

No. 1 (blocking) — rerunning aggregate-t1/aggregate-t2 --submit overwrites the committed salt, so the aggregator's own reveal then fails and they get S2-slashed. In dincli/cli/aggregator.py (T1: lines 384–398, T2: 589–600), every --submit run does salt = secrets.token_bytes(32), sends commitT1Aggregation(...) with exit_on_failure=False, then calls _save_agg_commit(...) unconditionally. build_and_send_tx doesn't raise on a revert/failed estimate with exit_on_failure=False — it returns None (dincli/cli/utils.py, the three return None paths) — so the except never fires and the cache is overwritten either way. aggregate_t1 also has no "already committed" skip: it loops every batch the account is assigned to.

Concrete failure: an aggregator assigned to T1 batches 0 and 1 runs aggregate-t1 --submit; batch 0 commits, batch 1's tx fails (RPC hiccup, gas). They rerun the same command. Batch 0 is re-aggregated, a new salt is generated, commitT1Aggregation reverts with TC_T1AlreadyCommitted (returns None, no exception), and t1_gi_<GI>_batch_0.json is overwritten with the new salt. Their later reveal-t1 then reverts TC_T1RevealHashMismatch for batch 0, t1Submitted stays false, and slashAggregators hits them with AGG_T1_NO_SUBMISSION — for an honest, correct aggregation. Same shape on T2.

Suggested fix (both tiers): before aggregating/committing a batch, read t1Committed(GI, bid, account) / t2Committed(...) on-chain and skip it if already committed (leaving the cache untouched); and only call _save_agg_commit when build_and_send_tx returns a receipt (i.e. status == 1). A unit test with build_and_send_tx monkeypatched to return None should assert the existing cache file is unchanged.

(dincli/cli/auditor.py's commitAuditScore path on develop — lines 510–527 — has the identical unconditional-save pattern; that's pre-existing and out of this PR's scope, but worth a separate follow-up issue alongside No. 192.)

No. 2 (should fix before/at merge) — docs still describe the removed single-shot submit.

  • Documentation/technical/contracts/DINTaskCoordinator.md: the function tree (lines 144–145) and §7.4/§7.5 (function submitT1Aggregation(...) at line 249; §7.5 at line 279 says "Identical pattern to T1 … on submit") still document submitT1Aggregation/submitT2Aggregation with no mention of commit/reveal, startT1AggregationReveal/startT2AggregationReveal, the new errors, or the new t1CommitHash/t1Committed/t2CommitHash/t2Committed mappings. Per the repo's docs convention, Documentation/ describes what's in the code on develop.
  • Documentation/public/roles/aggregators.md (lines 78–122) and Documentation/public/getting-started.md (lines 331, 343) still present aggregate-t1/-t2 --submit as the complete aggregator step, with no reveal-t1/reveal-t2. An aggregator following either page commits and never reveals — and is S2-slashed. model-workflow.md was updated correctly; these two need the same.

No. 3 (non-blocking) — negative tests use bare vm.expectRevert(), so they don't pin which check fired. Second mutation in the same run: deleted if (!t1Committed[...]) revert TC_T1NoCommitFound(); from revealT1Aggregation entirely — test_reveal_withoutPriorCommit_reverts still passed (it now reverts on TC_T1RevealHashMismatch against the zero stored hash instead). Suggest vm.expectRevert(TC_T1NoCommitFound.selector) etc. throughout AggregatorCommitReveal.t.sol (the intended error is already in each comment). Also, the T2 negative paths (copy attack, hash mismatch, no-commit, double reveal) are only exercised on T1; T2 has happy-path coverage via the full-GI walk only.

No. 4 (design note, not a code defect — for the mechanism owners) — selective non-reveal is now an option that didn't exist before. Reveals land sequentially inside the reveal window, so an aggregator who sees peers' revealed CIDs diverge from their own can simply not reveal and take the S2 liveness slash (s2SlashFractionBps, default 3000 = 30% of minStake) instead of the full-minStake AGG_*_BAD_CONSENSUS slash they'd get for revealing a wrong CID. Under the old single-shot submit a wrong submission was irrevocable. This matches the auditor-side precedent (non-reveal = missed vote), so it's consistent, not a regression in this PR — but it's worth recording against the S1/S2 fraction decision in No. 155 (e.g. a distinct, heavier reason for committed-but-unrevealed vs. never-committed).

No. 5 (coordination note) — Part B (PR No. 191, H-2 seed) is still open and touches the same two contracts; whichever lands second needs a rebase and the ordinal table posted to PR No. 29 should be re-checked against the final enum. The PR's closing keyword for issue No. 156 only takes effect on merge to the default branch, so it won't close prematurely off develop, but No. 156 shouldn't be closed by hand until No. 191 is also in.

Not independently re-verified: the contract-side NatSpec/doc wording beyond spot-checks, and the gas-benchmark numbers in GasSimulation.t.sol (ran green; didn't compare figures against the pre-PR baseline).


The contract change itself is sound: the sender-bound hash is a real improvement over No. 156's own proposal, the copy-attack test provably guards it, non-reveal correctly falls through to the existing S2 path, the GI-state renumbering is consistent across Solidity, dincli, and docs with no ordinal comparisons to break, and everything builds and passes on the real via_ir profile. Not mergeable as-is because of No. 1 — it turns an ordinary retry into a slash of an honest aggregator, and contradicts the PR's own "never regenerated on retry" claim. No. 2 should land with it; No. 3 is a quick tightening; No. 4/No. 5 are notes.

@umeradl

umeradl commented Sep 29, 2026 •

Copy link
Copy Markdown
Member

Files changed (19) — as of b3ff1b9 (PR head; re-reviewed 2026-09-30, previously b5d602f)

Re-review b5d602f → b3ff1b9: the only new commit is the author's merge of develop @ 4658bf8 (PR No. 191/No. 184/No. 196/No. 200) into the branch. At b5d602f, develop had touched 17 of this PR's 19 files, and foundry/src/DINShared.sol conflicted (GitHub CONFLICTING/DIRTY; local git merge-tree --write-tree exit 1, that one path). b3ff1b9 resolves it. GitHub now: MERGEABLE / CLEAN, and a local git merge-tree --write-tree against develop @ 4658bf8 agrees (exit 0). Because the branch now contains develop, each block's "Diff vs current develop HEAD" is just this PR's own change. Details and the new No. 6 are in the verification comment's re-review section.

foundry/src/DINShared.sol

Field Value
Change Modified
Lines +52/-8
Diff (what exactly is in this PR) Inserts T1AggregationRevealStarted (18) after T1AggregationStarted and T2AggregationRevealStarted (21) after T2AggregationStarted in GIstates, renumbering every later ordinal (GIended 23 → 25). Rewords the TC_T1/T2AggregationNotStarted NatSpec to "commit phase". Adds 12 new errors: TC_T{1,2}RevealCannotBeStarted, …RevealPhaseNotOpen, …AlreadyCommitted, …EmptyCommitHash, …NoCommitFound, …RevealHashMismatch.
Functionality — how & why How: the two new states give the coordinator a distinct "reveals only" window between each tier's commit window and its …Done state; every contract gate on GIstate is an exact ==/!= on a named state (grepped — no ordinal comparisons in foundry/src, including DINTaskAuditor's two cross-contract reads), so the renumbering can't silently open or close any other phase. Why: issue No. 156 M-1 — a single-phase submit let a late aggregator read t1SubmissionCID from public state and copy it; a separate reveal state is what makes "commits close before anything is revealed" enforceable on-chain. Insertion (not append) follows the LMSevaluationRevealStarted precedent so the enum stays in lifecycle order.
Diff vs current develop HEAD Conflicted at b5d602f: develop (PR No. 191) and this PR each appended a new error block after TC_DisputeSeedAlreadyLocked(). b3ff1b9 keeps both, develop's seed-lock errors first, then this PR's commit-reveal errors. My own independent resolution is byte-identical. PR No. 191 added no GIstates, so the ordinals are unchanged (26 entries, GIended = 25).
Recommended merge proposal Merged as-is. Enum verified 26 entries and matched position-for-position against dincli/cli/utils.py's mirrors (see that file's block). Downstream: PR No. 191 landed first without adding states, so the ordinal table posted to PR No. 29 is still correct.
Actual merge proposal Applied as-is: PR diff 4658bf8..b3ff1b9 applied cleanly (byte-identical to the PR head; the PR No. 191 error-block conflict was already resolved in the author's merge). GIstates / dincli states / stateDescription re-checked in the merged tree: 26/26/26, position-for-position. Evidence on the final merged tree: forge clean && forge build (via_ir) clean; forge test 476 passed / 0 failed / 0 skipped, 39 suites; pytest -m "not integration" (empty HOME) 382 passed / 0 failed, 133 deselected; doc links: 228 checked, all resolve. Uncommitted in the develop cwd — not committed or pushed.
Pending proposal None
Local merge conflict No at b3ff1b9 (Yes at b5d602f, resolved by the author's merge)
GitHub merge conflict No at b3ff1b9 (was CONFLICTING at b5d602f)

foundry/src/DINTaskCoordinator.sol

Field Value
Change Modified
Lines +150/-18
Diff (what exactly is in this PR) Replaces submitT1Aggregation/submitT2Aggregation with commitT{1,2}Aggregation(GI, batchId, commitHash) + revealT{1,2}Aggregation(GI, batchId, cid, salt); adds owner-only startT{1,2}AggregationReveal; adds t{1,2}CommitHash / t{1,2}Committed mappings and T{1,2}AggregationCommitted events; finalizeT{1,2}Aggregation now gate on the reveal state.
Functionality — how & why How: commit checks state == …AggregationStarted, batch validity, isTier{1,2}Aggregator, isValidatorActive, non-zero hash, not already committed, then stores the hash. The owner flips to …RevealStarted. Reveal re-checks aggregator/active, requires a prior commit and no prior reveal, a non-zero CID, and keccak256(abi.encode(cid, salt, msg.sender, GI, TierKind, batchId)) == stored. Only then does it write t{1,2}Submitted / t{1,2}SubmissionCID and bump t{1,2}Votes, exactly as the old submit did. Finalize bodies are unchanged. A committed-but-unrevealed aggregator never sets …Submitted, so slashAggregators (read, unchanged) S2-slashes them via the existing "no submission" branch. Why: closes No. 156 M-1's copy-the-leader path. Binding msg.sender, GI, tier and batchId into the hash also closes the "replay a peer's commit hash, then reveal their (cid, salt)" variant that No. 156's own keccak256(cid, salt) proposal would leave open.
Diff vs current develop HEAD Merged develop @ 4658bf8 in b3ff1b9; develop had also changed this file (PR No. 191 seed-lock / PR No. 184 params), which auto-merged cleanly. The remaining diff vs develop is exactly this PR's change above.
Recommended merge proposal Needs a fix: verification comment No. 6 (blocking, new in re-review). After the merge the runtime is 24,585 B, 9 B over EIP-170 (forge build --sizes exits 1). develop alone is 22,689 B, and this PR adds about 1.9 KB. CI didn't catch it (plain forge build/forge test, no --sizes; forge test and anvil.sh don't enforce the limit). Fix: fold the near-duplicate T1/T2 commit…/reveal…/start…Reveal bodies (L807–1041) into tier-parameterised internal functions, and post the new size with real headroom. Otherwise the contract side is merged as-is. Mutation-checked: removing msg.sender from the reveal hash makes test_copyAttack_replayingPeerCommitHash_cannotReveal fail, so the binding is genuinely guarded. Size before the merge was 23,242 B (margin 1,334 B). Design note (verification comment No. 4): selective non-reveal now trades a full BAD_CONSENSUS slash for S2's 30%. This is consistent with the auditor-side precedent, so it's a No. 155 mechanism question, not a merge blocker.
Actual merge proposal Applied as-is: PR diff applied cleanly (byte-identical to the PR head). Size NOT reduced, by decision (Umer, 2026-09-30): runtime is 24,585 B, 9 B over EIP-170, accepted as a known exception for this merge. The reduction moves to issue No. 201 Part A, which is now a hard blocker for any deploy from develop, including DevNet 3.0 (recorded on issue No. 201). DINTaskAuditor is unchanged at 22,567 B (2,009 B margin). Evidence on the final merged tree: forge clean && forge build (via_ir) clean; forge test 476 passed / 0 failed / 0 skipped, 39 suites; pytest -m "not integration" (empty HOME) 382 passed / 0 failed, 133 deselected; doc links: 228 checked, all resolve. Uncommitted in the develop cwd — not committed or pushed.
Pending proposal Open, moved to issue No. 201: size under EIP-170 with ≥ 1,024 B headroom (No. 6, Part A, deploy blocker), and the committed-but-unrevealed slash reason (No. 4, Part B, mechanism-owner call alongside No. 155).
Local merge conflict No
GitHub merge conflict No

dincli/cli/aggregator.py

Field Value
Change Modified
Lines +148/-10
Diff (what exactly is in this PR) Adds TIER1/TIER2, a JSON commit store under <model_base_dir>/aggregations/commits/, and _agg_commit_hash (eth_abi encode + keccak). aggregate-t1/-t2 --submit now generate a fresh 32-byte salt and send commitT{1,2}Aggregation, then save (cid, salt). New reveal-t1 (optional --batch) and reveal-t2 commands load the cache and call revealT{1,2}Aggregation.
Functionality — how & why How: the commit run computes the hash client-side from the aggregated CID, a random salt, the account address, GI, tier and batch, and persists (cid, salt) to disk. Reveal reads it back after the owner opens the window, so the aggregation worker doesn't have to run twice. Why: the contract now only accepts a hidden hash at commit time, so the client must hold the preimage between the two phases. Golden vectors independently re-derived with cast match.
Diff vs current develop HEAD Merged develop @ 4658bf8 in b3ff1b9; develop had also changed this file (PR No. 191 seed-lock / PR No. 184 params), which auto-merged cleanly. The remaining diff vs develop is exactly this PR's change above.
Recommended merge proposal Needs a fix — verification comment No. 1 (blocking). Lines 409–423 (T1) and 614–625 (T2) at b3ff1b9 (384–398 / 589–600 at b5d602f) regenerate the salt on every --submit run and overwrite the cache unconditionally. build_and_send_tx(..., exit_on_failure=False) returns None on a revert rather than raising. So rerunning after a partial success reverts with TC_T1AlreadyCommitted and still clobbers the committed salt, and the later reveal fails with TC_T1RevealHashMismatch, which means an S2 slash of an honest aggregator. Fix: skip batches where t{1,2}Committed(GI, bid, account) is already true, and only save when a receipt is returned. Add a monkeypatched unit test asserting the cache is unchanged on a failed commit.
Actual merge proposal Applied PR diff + No. 1 fix. aggregate-t1/-t2 --submit now (a) skips a batch already committed on-chain (t1Committed/t2Committed) before re-aggregating, leaving its cached (cid, salt) untouched, and (b) writes the cache before sending the commit tx. Umer chose that over "save only on receipt", because build_and_send_tx returns None both on a revert and when the receipt wait fails for a tx that may still mine. Known residual: rerunning while the previous commit tx is still pending in the mempool. New tests/test_aggregator_commit_retry.py (4 cases, T1+T2): already-committed batch → cache unchanged, no tx, no worker run; failed send → cache existed at send time and hashes to exactly the sent commit. Evidence on the final merged tree: forge clean && forge build (via_ir) clean; forge test 476 passed / 0 failed / 0 skipped, 39 suites; pytest -m "not integration" (empty HOME) 382 passed / 0 failed, 133 deselected; doc links: 228 checked, all resolve. Uncommitted in the develop cwd — not committed or pushed.
Pending proposal Salt-overwrite fix (No. 1). Done: skip-if-committed + cache-before-send, with regression tests. Still open: the follow-up issue for the same pre-existing pattern in dincli/cli/auditor.py (L533–550) hasn't been opened yet. Separately, pre-existing and noticed during the fix: aggregate_t2 rebinds bid in its inner T1 loop, so T2 container/job names and the models path use the last T1 batch id (the commit uses i, so it's correct).
Local merge conflict No
GitHub merge conflict No

dincli/cli/modelownerd/aggregation.py

Field Value
Change Modified
Lines +68/-2
Diff (what exactly is in this PR) New model-owner aggregation T1 start-reveal / T2 start-reveal commands calling startT{1,2}AggregationReveal. T1 close / T2 close now require T{1,2}AggregationRevealStarted instead of …Started.
Functionality — how & why How: each command validates GI and the current state client-side (same validate_GIstate_ET_given_GIstate helper as the neighbouring commands), then sends the owner-only transaction. Why: gives the model owner the explicit phase step the contract now requires. Without it the GI stalls in the commit window, because finalize reverts with TC_NotReadyToFinalizeT{1,2}.
Diff vs current develop HEAD Merged develop @ 4658bf8 in b3ff1b9; develop had also changed this file (PR No. 191 seed-lock / PR No. 184 params), which auto-merged cleanly. The remaining diff vs develop is exactly this PR's change above.
Recommended merge proposal Merged as-is.
Actual merge proposal Applied as-is: PR diff applied cleanly (byte-identical to the PR head). Evidence on the final merged tree: forge clean && forge build (via_ir) clean; forge test 476 passed / 0 failed / 0 skipped, 39 suites; pytest -m "not integration" (empty HOME) 382 passed / 0 failed, 133 deselected; doc links: 228 checked, all resolve. Uncommitted in the develop cwd — not committed or pushed.
Pending proposal None
Local merge conflict No
GitHub merge conflict No

dincli/cli/utils.py

Field Value
Change Modified
Lines +13/-8
Diff (what exactly is in this PR) Inserts the two new reveal states (and descriptions) into the states / stateDescription positional mirrors. Removes the stale trailing duplicate LMSevaluationRevealStarted / "LM submissions evaluation reveal started".
Functionality — how & why How: GIstate_to_index and every state-name lookup in dincli index these lists by the raw on-chain ordinal. Why: without the insertion every state from 18 onward would display and validate as the wrong name, so for example T1 close would check the wrong state.
Diff vs current develop HEAD Merged develop @ 4658bf8 in b3ff1b9; develop had also changed this file (PR No. 191 seed-lock / PR No. 184 params), which auto-merged cleanly. The remaining diff vs develop is exactly this PR's change above.
Recommended merge proposal Merged as-is. I parsed the lists and the Solidity enum programmatically: 26/26/26 entries, states == enum position for position, and descriptions aligned. develop's lists were 25 long with the duplicate at both index 14 and the tail, so the removal is correct.
Actual merge proposal Applied as-is: PR diff applied cleanly (byte-identical to the PR head); 26/26/26 enum mirror check re-run in the merged tree. Evidence on the final merged tree: forge clean && forge build (via_ir) clean; forge test 476 passed / 0 failed / 0 skipped, 39 suites; pytest -m "not integration" (empty HOME) 382 passed / 0 failed, 133 deselected; doc links: 228 checked, all resolve. Uncommitted in the develop cwd — not committed or pushed.
Pending proposal None
Local merge conflict No
GitHub merge conflict No

dincli/abis/DINTaskCoordinator.json

Field Value
Change Modified
Lines +333/-13
Diff (what exactly is in this PR) Regenerated bundled ABI: the two submit… entries are removed; the six new functions, four new mappings' getters, two new events and 12 new errors are added.
Functionality — how & why How: dincli falls back to this bundled ABI when a manifest supplies no custom task_contracts ABI. Why: the new commands call functions that don't exist in the old ABI.
Diff vs current develop HEAD Merged develop @ 4658bf8 in b3ff1b9; develop had also changed this file (PR No. 191 seed-lock / PR No. 184 params), which auto-merged cleanly. The remaining diff vs develop is exactly this PR's change above.
Recommended merge proposal Merged as-is. I set-compared it with foundry/out/DINTaskCoordinator.sol/DINTaskCoordinator.json from my own via_ir build: identical, 195 entries each.
Actual merge proposal Regenerated with dincli system dump-abi from the clean via_ir build (not copied): +333/-13 vs develop, identical to foundry/out and to the PR head's copy (213/213 entries). Evidence on the final merged tree: forge clean && forge build (via_ir) clean; forge test 476 passed / 0 failed / 0 skipped, 39 suites; pytest -m "not integration" (empty HOME) 382 passed / 0 failed, 133 deselected; doc links: 228 checked, all resolve. Uncommitted in the develop cwd — not committed or pushed.
Pending proposal None
Local merge conflict No
GitHub merge conflict No

tests/test_aggregator_commit_hash.py

Field Value
Change New
Lines +70/-0
Diff (what exactly is in this PR) Four tests for _agg_commit_hash: T1 and T2 golden vectors, plus two checks that the hash varies with sender and with tier.
Functionality — how & why How: it compares Python's abi_encode + keccak output against fixed hex values produced outside the implementation. Why: any field-order or type mismatch with the Solidity side would make every reveal revert, which means every aggregator gets S2-slashed.
Diff vs current develop HEAD None — new file.
Recommended merge proposal Merged as-is. It runs under Python 3.12 (the PR notes the author's sandbox couldn't run it), and both vectors are byte-identical to my own cast abi-encode + cast keccak recomputation.
Actual merge proposal Applied as-is: new file copied from the PR head; its imports are unchanged by the aggregator.py fix. Evidence on the final merged tree: forge clean && forge build (via_ir) clean; forge test 476 passed / 0 failed / 0 skipped, 39 suites; pytest -m "not integration" (empty HOME) 382 passed / 0 failed, 133 deselected; doc links: 228 checked, all resolve. Uncommitted in the develop cwd — not committed or pushed.
Pending proposal Add the No. 1 regression test. Done: added as a separate file, tests/test_aggregator_commit_retry.py (see dincli/cli/aggregator.py), to keep the golden vectors unmixed with the heavily mocked retry tests.
Local merge conflict No
GitHub merge conflict No

foundry/test/AggregatorCommitReveal.t.sol

Field Value
Change New
Lines +516/-0
Diff (what exactly is in this PR) 12 tests: happy path, copy-attack PoC, commit twice, zero hash, reveal before the window opens, reveal with no commit, wrong CID or salt, reveal twice, finalize before the reveal window opens, commit-but-never-reveal leading to an S2 slash, and a full GI walked through both new states.
Functionality — how & why How: it drives a real deployed platform plus a task pair through the GI lifecycle with pranked aggregators. Why: it's the regression net for the new phase split and the sender binding.
Diff vs current develop HEAD None — new file.
Recommended merge proposal Merged, ideally with a tightening. Re-review: b3ff1b9 adds _lockAuditSeedNow/_lockAggSeedNow (roll past disputeSeedDelay + lockAuditSeed/lockAggSeed) before createAuditorsBatches/autoCreateTier1AndTier2. Without it, all 12 tests fail on the merged tree with TC_AuditSeedNotLocked() (reproduced). With it, 12/12 pass. Verification comment No. 3: every negative test uses a bare vm.expectRevert(). Deleting the TC_T1NoCommitFound check entirely left test_reveal_withoutPriorCommit_reverts still passing, because it hit TC_T1RevealHashMismatch instead. Pin the selectors (each intended error is already named in its test's comment). The T2 negative paths are only covered on T1.
Actual merge proposal Applied new file (incl. the author's seed-lock fix) + No. 3 fix: all 9 bare vm.expectRevert() pinned to the intended selector (each checked against the contract's check order), and 9 new T2 negative-path tests (copy attack, reveal before window, no commit, wrong CID/salt, double commit, zero hash, double reveal, early finalize, TC_OnlyOneTier2Batch on commit and reveal). Suite 12 → 21, all pass; no via_ir ICE. Evidence on the final merged tree: forge clean && forge build (via_ir) clean; forge test 476 passed / 0 failed / 0 skipped, 39 suites; pytest -m "not integration" (empty HOME) 382 passed / 0 failed, 133 deselected; doc links: 228 checked, all resolve. Uncommitted in the develop cwd — not committed or pushed.
Pending proposal Pin revert selectors, and add T2 negative-path tests. Done: 9 selectors pinned, 9 T2 tests added.
Local merge conflict No
GitHub merge conflict No

foundry/test/DisputeResolution.t.sol, GasSimulation.t.sol, LifecycleEvents.t.sol, PR146SlashingRegression.t.sol, RewardEngine.t.sol, SecurityFindings.t.sol, StakingEnforcement.t.sol, TreasuryForwarding.t.sol

Field Value
Change Modified (8 files)
Lines +59/-7, +97/-42, +105/-19, +16/-2, +28/-3, +27/-5, +14/-2, +31/-5
Diff (what exactly is in this PR) Call-shape migration: each submitT{1,2}Aggregation becomes commit… + startT{1,2}AggregationReveal + reveal… through local _commitT1/_commitT2-style helpers using the sender-bound hash. GasSimulation.t.sol splits the old single submit-gas benchmark into separate commit and reveal measurements (cold and warm). LifecycleEvents.t.sol asserts the new ordinals and events.
Functionality — how & why How: these are mechanical rewrites of the lifecycle drivers that existing suites use to reach aggregation and slashing states. Why: the old entry points no longer exist, so every suite that walks a GI past T1 had to change.
Diff vs current develop HEAD Merged develop @ 4658bf8 in b3ff1b9; develop had also changed this file (PR No. 191 seed-lock / PR No. 184 params), which auto-merged cleanly. The remaining diff vs develop is exactly this PR's change above.
Recommended merge proposal Merged as-is. Re-review: b3ff1b9 also factors DisputeResolution.t.sol's duplicated _runToT1Finalized/_runToT1FinalizedWithDissent setup into _setupToT1AggregationStarted(n, prefix). Without that, the merged file hits solc via_ir's ICE (Tag too large for reserved space, reproduced; it's the only file that does). The "agg"/"dissentAgg" prefixes are kept, so addresses are unchanged. The full suite at b3ff1b9 is 467/467 across 39 suites on the via_ir profile. I didn't compare the new gas figures against a pre-PR baseline.
Actual merge proposal Applied all 8 as-is: each PR diff applied cleanly and is byte-identical to the PR head (DisputeResolution includes the author's _setupToT1AggregationStarted ICE refactor). Advisory forge lint unsafe-typecast notes on new gas-measurement casts in GasSimulation.t.sol (test-only; CI lint doesn't gate). Evidence on the final merged tree: forge clean && forge build (via_ir) clean; forge test 476 passed / 0 failed / 0 skipped, 39 suites; pytest -m "not integration" (empty HOME) 382 passed / 0 failed, 133 deselected; doc links: 228 checked, all resolve. Uncommitted in the develop cwd — not committed or pushed.
Pending proposal None
Local merge conflict No
GitHub merge conflict No

Documentation/public/workflows/model-workflow.md, Documentation/technical/contracts/DINShared.md, Documentation/technical/audits/foundry-src-security-review.md

Field Value
Change Modified (3 files)
Lines +41/-6, +33/-25, +2/-0
Diff (what exactly is in this PR) model-workflow.md adds the commit, start-reveal and reveal-t1/reveal-t2 steps for both tiers. DINShared.md updates the state table, the lifecycle diagram and the ordinal note (18/21, GIended = 25). The audit doc marks M-1's aggregation side fixed, citing this PR and linking the auditor-side follow-up (issue No. 192).
Functionality — how & why How: documentation only. Why: Documentation/ describes code on develop, so the new phase step and ordinals have to be reflected.
Diff vs current develop HEAD Merged develop @ 4658bf8 in b3ff1b9; develop had also changed this file (PR No. 191 seed-lock / PR No. 184 params), which auto-merged cleanly. The remaining diff vs develop is exactly this PR's change above.
Recommended merge proposal Merged as-is. I spot-checked that the DINShared.md table and note match the enum. Missing siblings, verification comment No. 2: Documentation/technical/contracts/DINTaskCoordinator.md still documents submitT1Aggregation/submitT2Aggregation (tree at lines 144–145, §7.4, §7.5). Documentation/public/roles/aggregators.md and getting-started.md have no reveal step, so an aggregator following them gets S2-slashed.
Actual merge proposal Applied all 3 as-is (byte-identical to the PR head) + No. 2 fix in three sibling docs. Documentation/technical/contracts/DINTaskCoordinator.md: state tables, function tree, §7.4/§7.5 rewritten for commit → start-reveal → reveal with all errors, §8.2, §12 events, §13 M-1 row, §15 limitations; no submitT1/T2Aggregation left. Documentation/public/roles/aggregators.md: --submit = commit, new reveal-t1/reveal-t2 sections, a non-reveal warning, and a 7-step workflow. Documentation/public/getting-started.md: steps 7/8 include the reveals. Evidence on the final merged tree: forge clean && forge build (via_ir) clean; forge test 476 passed / 0 failed / 0 skipped, 39 suites; pytest -m "not integration" (empty HOME) 382 passed / 0 failed, 133 deselected; doc links: 228 checked, all resolve. Uncommitted in the develop cwd — not committed or pushed.
Pending proposal Update DINTaskCoordinator.md, roles/aggregators.md and getting-started.md for commit/reveal (No. 2). Done. Pre-existing, not changed: DINTaskCoordinator.md §8.2 still says a no-submission slash is a full minStake(), but since PR No. 184 it uses s2SlashFractionBps.
Local merge conflict No
GitHub merge conflict No

Verification

Re-review at b3ff1b9: forge build (via_ir) compiles; forge test after forge clean gives 467/467, 39 suites; pytest -m "not integration" gives 378 passed, 133 deselected. The bundled ABI is identical to the forge artifact (213/213), and the enum and dincli mirrors match 26/26/26. CI is green. But forge build --sizes fails: DINTaskCoordinator is 24,585 B, 9 B over EIP-170 (No. 6, blocking). Findings No. 1–No. 3 are still open; No. 5 is resolved.

At b5d602f (first review): 416/416 across 34 suites; pytest 365 passed, 133 deselected. The bundled ABI was identical to the forge artifact, and the golden vectors matched cast. The mutation checks and findings No. 1–No. 5 are in the verification comment above.

Local vs. GitHub agree: both report a clean merge at b3ff1b9 with no conflicted paths (git merge-tree --write-tree exit 0; MERGEABLE / CLEAN). At b5d602f both reported the DINShared.sol conflict.

Resolves the one real conflict (foundry/src/DINShared.sol -- both
sides independently added a disjoint block of new custom errors at
the same location; kept both blocks, Part B's seed-lock errors before
Part C's commit-reveal errors). Everything else auto-merged, including
foundry/src/DINTaskCoordinator.sol/DINTaskAuditor.sol, which now carry
develop's own PR No. 184 zero-rounding slash guards alongside this
branch's commit-reveal functions with no overlap.

Two fixes needed after resolving the merge, both because this branch's
code predates content that landed on develop while it was in flight:

- foundry/test/DisputeResolution.t.sol: merging in develop's own
  (identical) seed-lock fixture edits pushed this file's total
  via_ir complexity past the same ICE threshold hit once before
  (solc "Tag too large for reserved space"). _runToT1Finalized and
  _runToT1FinalizedWithDissent duplicated their entire setup path
  almost verbatim; factored the shared portion into
  _setupToT1AggregationStarted rather than duplicating it across both.

- foundry/test/AggregatorCommitReveal.t.sol: this file's fixture was
  written before issue InfiniteZeroFoundation#156 Part B's seed-lock gate existed on
  develop, so it called createAuditorsBatches/autoCreateTier1AndTier2
  without ever locking the required seed -- 12 tests failed with
  TC_AuditSeedNotLocked() once merged. Added the same
  _lockAuditSeedNow/_lockAggSeedNow helpers DisputeResolution.t.sol
  and others already use.

Verified on the merged tree: forge build (via_ir) clean, full
forge test 465/465 (one apparent failure in an isolated rerun was a
parallel-execution race in the FFI upgrade-validation tooling, not a
real failure -- confirmed by rerunning that suite alone), pytest
-m "not integration" at the same 187 passed / 16 pre-existing
environment-only failed baseline (py-cid version mismatch,
test_integration_marker -- unrelated to this branch).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@Abidoyesimze

Copy link
Copy Markdown
Collaborator Author

Merged latest `develop` in (b3ff1b9) to resolve the conflict flagged after #191 landed — this branch was cut before #191's seed-lock mechanism existed, so `foundry/src/DINShared.sol` had two independently-added error blocks at the same spot (kept both). Two things needed fixing once merged, both because this branch predates code that landed on `develop` while it was in flight:

  • `DisputeResolution.t.sol`: merging in `develop`'s own (identical) seed-lock fixture edits pushed this file's via_ir complexity past the same ICE threshold hit once before (`solc` "Tag too large for reserved space"). Factored the shared setup path out of `_runToT1Finalized`/`_runToT1FinalizedWithDissent` into one helper instead of the two fixtures duplicating it almost verbatim.
  • `AggregatorCommitReveal.t.sol`: this fixture predates fix(foundry+dincli): ungrindable batch-assignment seed (issue #156 H-2, Part B) #191's seed-lock gate, so it called `createAuditorsBatches`/`autoCreateTier1AndTier2` without ever locking the seed — 12 tests failed with `TC_AuditSeedNotLocked()` once merged. Added the same `_lockAuditSeedNow`/`_lockAggSeedNow` helpers the other files already use.

Verified on the merged tree: `forge build` (via_ir) clean, full `forge test` 465/465, `pytest -m "not integration"` at the same 187 passed / 16 pre-existing-environment-failed baseline. CI green (Docs/Python/Solidity/CI OK) on b3ff1b9.

@umeradl

umeradl commented Sep 30, 2026

Copy link
Copy Markdown
Member

Actual outcome: PR No. 197 merged + review fixes applied (pushed)

This supersedes the merge-proposal comment above with what actually landed on develop.

Two commits, both on origin/develop (4658bf8..e373c8d):

  1. d68f73a: a real --no-ff merge of this PR's head b3ff1b9 (merge commit, not squash), authored as Abidoyesimze. Its tree is exactly the PR head. There were no conflicts at merge time: the one real conflict (foundry/src/DINShared.sol vs PR No. 191) had already been resolved in the author's own b3ff1b9, and it matches my independent resolution. GitHub agrees: the PR shows MERGED.
  2. e373c8d: follow-up commit with the review fixes (findings No. 1, No. 2, No. 3), the same pattern as earlier follow-up commits.

Files unchanged from the PR (17 of 19)

foundry/src/DINShared.sol, foundry/src/DINTaskCoordinator.sol, dincli/cli/modelownerd/aggregation.py, dincli/cli/utils.py, dincli/abis/DINTaskCoordinator.json (regenerated from the merged via_ir build; identical to the PR's copy, 213/213 entries), tests/test_aggregator_commit_hash.py, foundry/test/DisputeResolution.t.sol, GasSimulation.t.sol, LifecycleEvents.t.sol, PR146SlashingRegression.t.sol, RewardEngine.t.sol, SecurityFindings.t.sol, StakingEnforcement.t.sol, TreasuryForwarding.t.sol, Documentation/public/workflows/model-workflow.md, Documentation/technical/contracts/DINShared.md, Documentation/technical/audits/foundry-src-security-review.md.

Files changed by e373c8d (2 of this PR's 19, plus 4 outside it)

File What changed vs. this PR's merged version
dincli/cli/aggregator.py No. 1: aggregate-t1/-t2 --submit skip a batch already committed on-chain (t1Committed/t2Committed) before re-aggregating, leaving its cached (cid, salt) untouched, and write the cache before sending the commit tx instead of after. build_and_send_tx returns None both on a revert and when the receipt wait fails for a tx that may still mine, so saving only on a receipt could strand a real commit without its preimage. Residual: a rerun while the previous commit tx is still pending in the mempool.
tests/test_aggregator_commit_retry.py (new) No. 1 regression tests (T1 + T2): an already-committed batch leaves the cache byte-identical, sends no tx and never runs the worker; a failed send still leaves a cache that hashes to exactly the commit that was sent.
foundry/test/AggregatorCommitReveal.t.sol No. 3: the 9 bare vm.expectRevert() calls are pinned to their intended selectors (each checked against the contract's check order), and 9 T2 negative-path tests are added (copy attack, reveal before window, no commit, wrong CID/salt, double commit, zero hash, double reveal, early finalize, TC_OnlyOneTier2Batch). 12 → 21 tests.
Documentation/technical/contracts/DINTaskCoordinator.md No. 2: now documents commit → start-reveal → reveal (state tables, function tree, §7.4/§7.5, §8.2, events, M-1 security row, limitations incl. selective non-reveal).
Documentation/public/roles/aggregators.md No. 2: --submit = commit; new reveal-t1/reveal-t2 sections; "never revealed = slashed" warning; 7-step workflow.
Documentation/public/getting-started.md No. 2: steps 7/8 include the reveals.

Why the follow-up commit

These aren't develop-drift deviations. The branch was already up to date with develop @ 4658bf8. They are the review's own open findings, applied at merge time: No. 1 (blocking: a retry overwrote the committed salt and got an honest aggregator S2-slashed), No. 2 (sibling docs still described the removed single-shot submit), and No. 3 (non-blocking test tightening).

Not fixed here: tracked separately

  • Contract size (No. 6): DINTaskCoordinator is 24,585 B runtime, 9 B over EIP-170. It was accepted as a known exception for this merge. The reduction is issue No. 201 Part A, a hard blocker for any deploy from develop, including DevNet 3.0 (decision).
  • Committed-but-unrevealed slash reason (No. 4): issue No. 201 Part B, a mechanism-owner call alongside No. 155.
  • Auditor-side twin of No. 1, and aggregate-t2's stale T1 batch id: issue No. 202.

Verification

On the merged tree, before commit (identical byte-for-byte to the pushed e373c8d):

  • forge clean && forge build (real via_ir): clean.
  • forge test: 476 passed, 0 failed, 0 skipped, 39 suites.
  • pytest -m "not integration" (empty HOME): 382 passed, 0 failed, 133 deselected.
  • Doc links: 228 checked, all resolve.

Pre-push local CI mirror on e373c8d (docs, pytest, via_ir build, forge test, hardhat compile/test): all ok. GitHub Actions push run on e373c8d: success.

Local vs. GitHub agree: the merge applied without conflicts, as predicted by the re-review; the PR shows MERGED.

umeradl pushed a commit that referenced this pull request Oct 5, 2026
… work after the T2 batch (#202)

Part 1 - auditor commit retry (auditor-side twin of PR #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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants