fix(foundry+dincli): commit-then-reveal for T1/T2 aggregation (issue #156 M-1, Part C) - #197
Conversation
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>
Re-review (2026-09-30):
|
Files changed (19) — as of
|
| 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 | 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 | 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 | |
| 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 | DINTaskCoordinator.md, roles/aggregators.md and getting-started.md for commit/reveal (No. 2).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>
|
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:
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. |
Actual outcome: PR No. 197 merged + review fixes applied (pushed)This supersedes the merge-proposal comment above with what actually landed on Two commits, both on
Files unchanged from the PR (17 of 19)
Files changed by
|
| 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):
DINTaskCoordinatoris 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 fromdevelop, 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.
… 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>
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/submitT2Aggregationaccepted 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:GIstatesgainsT1AggregationRevealStarted/T2AggregationRevealStarted, each inserted immediately after its commit-phase state (same lifecycle-position precedentLMSevaluationRevealStartedset — 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/submitT2Aggregationreplaced bycommitT1Aggregation/revealT1Aggregationand the T2 equivalents.startT1AggregationReveal/startT2AggregationReveal(model-owner-only) close the commit window and open the reveal window.finalizeT1Aggregation/finalizeT2Aggregationnow gate on the reveal state instead of the commit state — their body is otherwise unchanged, sincet1Submitted/t2Submittedandt1SubmissionCID/t2SubmissionCIDare now written at reveal time only, so a committed-but-never-revealed aggregator simply never sets them andslashAggregators' existing "no submission" (S2) check needed no changes.keccak256(abi.encode(cid, salt, msg.sender, GI, tierKind, batchId)), notkeccak256(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. Bindingmsg.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." Seetest_copyAttack_replayingPeerCommitHash_cannotRevealfor the PoC.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 --submitnow commit a hidden hash and cache(cid, salt)locally (never regenerated on retry, same pattern asauditor.py's commit-reveal). Newaggregator reveal-t1/reveal-t2commands read that cache and reveal once the model owner opens the reveal window via newmodel-owner aggregation T1/T2 start-revealcommands. ABI regenerated via the standarddump-abiprocess (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).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 assertingGIstatethrough 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.pycross-checks the Python-side hash computation against golden vectors computed independently viacast 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)
T1AggregationStartedT1AggregationRevealStartedT1AggregationDoneT2AggregationStartedT2AggregationRevealStartedT2AggregationDoneAuditorsSlashedAggregatorsSlashedGIendedTwo new events (
T1AggregationCommitted/T2AggregationCommitted) added speculatively for subgraph commit-time visibility — flagged as removable in the PR #29 comment if not needed.Contract size
DINTaskCoordinatorDINTaskAuditorNote: this PR was branched directly off
developrather 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 passforge test(full suite, afterforge clean && forge build --build-info) — 416/416 pass, 34 suites_agg_commit_hashverified against 2 golden vectors computed independently viacast 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.