From c96a063c1c1c97d03b65ec26233ee03acb96882e Mon Sep 17 00:00:00 2001 From: umermjd11 Date: Fri, 2 Oct 2026 13:26:35 +0500 Subject: [PATCH 1/2] fix(task-coordinator): bring DINTaskCoordinator back under EIP-170 and add a CI size gate (#201 Part A) DINTaskCoordinator was 24,585 B runtime, 9 B over EIP-170, so nothing on develop could deploy to a real chain. It is now 23,488 B (1,088 B margin), with no change to state-changing functions, events or storage: - fold slashAggregators' duplicated T1/T2 loops into _slashBatch (-528 B); same reasons, S2 partial / full bad-consensus slashes and events - make the ten t1*/t2* per-aggregator maps internal and add getAggregatorSubmission(gi, tier, batchId, aggregator) -> (committed, commitHash, submitted, cid, votes) (-360 B); votes is the count for the aggregator's own revealed CID - make tier1Batches/tier2Batches internal; getTier1Batch/getTier2Batch already cover them (-212 B) View-ABI change: dincli (aggregator.py, modelownerd/aggregation.py), the retry pytest mock and AggregatorCommitReveal.t.sol move to getAggregatorSubmission; bundled dincli/abis/DINTaskCoordinator.json regenerated. New tests cover the view for both tiers. Adds .github/scripts/contract_size_gate.py, run in CI after forge build. It reads every foundry/src artifact directly (forge build --sizes omits DINTaskAuditor), fails below a 1,024 B runtime margin or above EIP-3860 initcode, and warns below 2,048 B. Thresholds recorded in CONTRIBUTING.md; anvil.sh documents that it lifts the limit locally. Also corrects the stale slashAggregators NatSpec (no-submission is the S2 fraction). Co-Authored-By: Claude Opus 5.5 --- .github/scripts/contract_size_gate.py | 111 +++++ .github/workflows/ci.yml | 7 + Developer/CONTRIBUTING.md | 5 + .../technical/contracts/DINTaskCoordinator.md | 9 +- .../technical/testing/dincli-testing-guide.md | 2 +- dincli/abis/DINTaskCoordinator.json | 412 +++--------------- dincli/cli/aggregator.py | 8 +- dincli/cli/modelownerd/aggregation.py | 4 +- foundry/anvil.sh | 5 +- foundry/src/DINTaskCoordinator.sol | 190 ++++---- foundry/test/AggregatorCommitReveal.t.sol | 75 +++- tests/test_aggregator_commit_retry.py | 5 +- 12 files changed, 376 insertions(+), 457 deletions(-) create mode 100644 .github/scripts/contract_size_gate.py diff --git a/.github/scripts/contract_size_gate.py b/.github/scripts/contract_size_gate.py new file mode 100644 index 00000000..7c68c3e2 --- /dev/null +++ b/.github/scripts/contract_size_gate.py @@ -0,0 +1,111 @@ +#!/usr/bin/env python3 +"""Enforce an EIP-170 / EIP-3860 size budget on every deployable foundry/src contract. + +Run from foundry/ after `forge build` (issue #201 Part A): + + python ../.github/scripts/contract_size_gate.py [--out out] + +Reads every foundry/out/.sol/.json artifact directly, rather than +`forge build --sizes`, whose table leaves DINTaskAuditor out. An artifact is +checked when its metadata compilationTarget is under src/ and its +deployedBytecode is non-empty (interfaces and abstract contracts have none; +test and script contracts aren't under src/). Linked libraries would show +unresolved `__$...$__` placeholders; none exist today, and the gate counts them +at their 20-byte linked size. + +Fails when any runtime margin under EIP-170 is below FAIL_MARGIN, or any initcode +exceeds EIP-3860. Warns, without failing, when a runtime margin is below +WARN_MARGIN, so a contract approaching the budget shows up in every PR's log. +Fails closed if no artifacts are found. +""" +from __future__ import annotations + +import argparse +import json +import re +import sys +from pathlib import Path + +EIP170 = 24_576 # max runtime bytecode (bytes) +EIP3860 = 49_152 # max initcode (bytes) +FAIL_MARGIN = 1_024 # required runtime headroom under EIP170 (issue #201, PR #209 Decision 1) +WARN_MARGIN = 2_048 # headroom below which CI warns + +PLACEHOLDER = re.compile(r"__\$[0-9a-fA-F]{34}\$__") + + +def code_size(obj: str) -> int: + hexstr = obj[2:] if obj.startswith("0x") else obj + # Each unlinked-library placeholder is 40 hex chars, the size of the address it becomes. + hexstr = PLACEHOLDER.sub("0" * 40, hexstr) + return len(hexstr) // 2 + + +def compilation_target(artifact: dict) -> str | None: + metadata = artifact.get("metadata") or {} + if isinstance(metadata, str): + try: + metadata = json.loads(metadata) + except json.JSONDecodeError: + return None + target = (metadata.get("settings") or {}).get("compilationTarget") or {} + return next(iter(target), None) + + +def collect(out_dir: Path) -> list[tuple[str, str, int, int]]: + rows = [] + for path in sorted(out_dir.glob("*.sol/*.json")): + try: + artifact = json.loads(path.read_text()) + except (OSError, json.JSONDecodeError) as exc: + print(f"::error::cannot read {path}: {exc}") + sys.exit(1) + source = compilation_target(artifact) + if not source or not source.startswith("src/"): + continue + runtime = code_size((artifact.get("deployedBytecode") or {}).get("object", "")) + if runtime == 0: + continue + initcode = code_size((artifact.get("bytecode") or {}).get("object", "")) + rows.append((path.stem, source, runtime, initcode)) + return rows + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + parser.add_argument("--out", default="out", help="forge artifact directory (default: out)") + args = parser.parse_args() + + out_dir = Path(args.out) + rows = collect(out_dir) + if not rows: + print(f"::error::no deployable src/ artifacts found under {out_dir}/ -- run `forge build` first") + return 1 + + print(f"EIP-170 runtime limit {EIP170} B (fail below {FAIL_MARGIN} B margin, warn below {WARN_MARGIN} B); " + f"EIP-3860 initcode limit {EIP3860} B") + print(f"{'contract':<28} {'runtime':>8} {'margin':>8} {'initcode':>9} status") + failed = False + for name, source, runtime, initcode in sorted(rows, key=lambda r: EIP170 - r[2]): + margin = EIP170 - runtime + status = "ok" + if margin < FAIL_MARGIN: + status = "FAIL" + failed = True + print(f"::error file=foundry/{source}::{name} runtime {runtime} B leaves {margin} B under EIP-170; " + f"the budget requires >= {FAIL_MARGIN} B") + elif margin < WARN_MARGIN: + status = "warn" + print(f"::warning file=foundry/{source}::{name} runtime {runtime} B leaves only {margin} B under " + f"EIP-170 (warn below {WARN_MARGIN} B)") + if initcode > EIP3860: + status = "FAIL" + failed = True + print(f"::error file=foundry/{source}::{name} initcode {initcode} B exceeds EIP-3860 ({EIP3860} B)") + print(f"{name:<28} {runtime:>8} {margin:>8} {initcode:>9} {status}") + + return 1 if failed else 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fbe5cb22..cfbe8f68 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -61,6 +61,13 @@ jobs: working-directory: foundry run: forge build + # EIP-170/EIP-3860 budget for every foundry/src contract: fails below a + # 1,024 B runtime margin, warns below 2,048 B. Reads artifacts directly + # because `forge build --sizes` omits DINTaskAuditor. Issue #201 Part A. + - name: contract size gate + working-directory: foundry + run: python3 ../.github/scripts/contract_size_gate.py + - name: forge test working-directory: foundry run: forge test diff --git a/Developer/CONTRIBUTING.md b/Developer/CONTRIBUTING.md index 4117205c..72103436 100644 --- a/Developer/CONTRIBUTING.md +++ b/Developer/CONTRIBUTING.md @@ -47,6 +47,11 @@ For issue-specific contributor packets, review questions, and curated reading li dincli system dump-abi --artifact foundry/out/.sol/.json --output dincli/abis --official ``` `dincli/abis/*.json` is the only ABI dincli has for the platform contracts (`DinCoordinator`, `DinToken`, `DinValidatorStake`, `DINModelRegistry`, `DinFeeRouter`), and the fallback for `DINTaskCoordinator`/`DINTaskAuditor` when a model's manifest doesn't supply custom `task_contracts` artifacts. A stale bundle means dincli calls functions that no longer exist (`AttributeError`) or decodes events/structs against the wrong layout. No network or wallet needed to run it. See [issue #177](https://github.com/InfiniteZeroFoundation/DevNet/issues/177) for what drifting looks like when this is skipped. +- **Contract size budget.** Every deployable `foundry/src` contract must keep at least **1,024 B** of runtime headroom under EIP-170's 24,576 B. Initcode must stay under EIP-3860's 49,152 B. CI enforces both with `.github/scripts/contract_size_gate.py`, and warns when a contract's margin drops below **2,048 B**. Both thresholds are constants at the top of the script (issue #201). Check locally after `forge build`: + ``` + cd foundry && python3 ../.github/scripts/contract_size_gate.py + ``` + `foundry/anvil.sh` lifts the limit on the local chain, so a local deploy is not a size check. A PR that pushes a contract under the 1,024 B fail line needs a size reduction in the same PR. Don't split a contract without raising it with a maintainer first. ## Code Standards diff --git a/Documentation/technical/contracts/DINTaskCoordinator.md b/Documentation/technical/contracts/DINTaskCoordinator.md index f7033ac2..e10b9b92 100644 --- a/Documentation/technical/contracts/DINTaskCoordinator.md +++ b/Documentation/technical/contracts/DINTaskCoordinator.md @@ -55,10 +55,12 @@ Auditor registration, client submissions, scoring and rewards live in the paired |----------|-------------| | `dinAggregators[gi]`, `isDINAggregator[gi][addr]` | Registration list and flag | | `registrationSlotsReleased[gi]` | Guard for `releaseGIRegistrationSlots` | -| `tier1Batches[gi]`, `tier2Batches[gi]` | Batch arrays (`Tier1Batch` / `Tier2Batch`); T2 always has at most one batch, id 0 | +| `tier1Batches[gi]`, `tier2Batches[gi]` | Batch arrays (`Tier1Batch` / `Tier2Batch`); T2 always has at most one batch, id 0. Internal: read through `getTier1Batch` / `getTier2Batch` | | `isTier1Aggregator`, `isTier2Aggregator` | Batch assignment flags (not public) | | `t1CommitHash`, `t1Committed` (and `t2*`) | Commit hash and commit flag per `[gi][batchId][aggregator]` | | `t1SubmissionCID`, `t1Submitted`, `t1Votes` (and `t2*`) | Revealed CID, revealed flag and per-CID vote count. Written only by the reveal functions; `t*Submitted` stays `false` for an aggregator who committed but never revealed | + +The batch arrays and the ten `t1*`/`t2*` maps are `internal`, to keep the contract under EIP-170 (issue #201 Part A). Read per-aggregator state through `getAggregatorSubmission(gi, tier, batchId, aggregator)`, which returns `(committed, commitHash, submitted, cid, votes)`. `votes` is the vote count of that aggregator's own revealed CID, because the vote maps are keyed by CID. It is 0 before the reveal. A vote count for an arbitrary CID is no longer readable from outside the contract. | `tier1FinalizedAt`, `tier2FinalizedAt` | Finalization timestamps (dispute window start) | | `tier2Score[gi]` | Owner-recorded quality score for the T2 model | | `aggregatorWeight[gi][addr]`, `totalAggregatorWeight[gi]` | Reward weights (one unit per aggregator per finalized batch) | @@ -268,13 +270,13 @@ DINTaskAuditor → DINTaskCoordinator: GI, GIstate, aggregatorWeight (at claim t Earlier findings from the [foundry/src security review](../audits/foundry-src-security-review.md) should be read alongside these. Several are now fixed: unbounded registration (capped at 300), zero-CID collision (`TC_ZeroCID`), missing quorum (`TC_InsufficientSubmissions`), grindable shuffle (H-2, §6.3) and copy-the-leader aggregation (M-1, §6.5). -- **No. 1 — Runtime bytecode is over the EIP-170 limit.** With commit-reveal aggregation merged, this contract compiles to 24,585 bytes, 9 over the 24,576-byte limit, so it cannot be deployed to Optimism Sepolia or any other real chain from `develop`. `foundry/anvil.sh` starts anvil with `--code-size-limit 4294967295`, and neither `forge test` nor CI checks contract size, so the local devnet and a green test run do not show the problem. Tracked in issue #201 (Part A), which also sets a size budget for every contract. +- **No. 1 — Little room under EIP-170.** Issue #201 Part A brought this contract back under the 24,576-byte limit. It folded the T1/T2 slash loops into `_slashBatch`, and replaced the public batch and per-aggregator getters with `getAggregatorSubmission`. CI now runs `.github/scripts/contract_size_gate.py`. The gate fails any `foundry/src` contract with less than 1,024 bytes of margin, and warns below 2,048 bytes. This contract sits in the warning band, so the next feature added here may need another size reduction first. `foundry/anvil.sh` still lifts the limit locally, so a local deploy is not a size check; the CI gate is. - **No. 2 — Selective non-reveal is cheaper than being wrong.** Reveals land one at a time. A committed aggregator who sees that peers' CIDs differ from their own can withhold the reveal and take the S2 liveness slash (30% of `minStake` by default) instead of the full-`minStake` bad-consensus slash. The same trade-off exists for auditors. Whether committed-but-unrevealed gets its own reason code and fraction is open in issue #201 (Part B). - **No. 3 — `expireDispute` can slash an idle fresh subgroup that had no on-chain way to act.** The fresh subgroup cannot submit a recomputation to this contract, so the only thing preventing the timeout slash is the owner calling `settleRecomputation` in time. If the owner does nothing, anyone can slash three uninvolved validators a full `minStake` each. - **No. 4 — `modelId` is fixed at construction, but assigned at registry approval.** The registry assigns the ID only when it approves the request, which requires this contract to already exist. The deployer must predict the ID. A wrong guess silently applies another model's stake floor (or none). - **No. 5 — Plurality with three aggregators.** Two colluding aggregators in a batch win the vote; nothing verifies the aggregated model itself. The dispute path (§7) is the only recourse, and the owner adjudicates it. - **No. 6 — No recovery from a stalled GI.** If a batch never reaches the reveal quorum, `finalizeT1Aggregation` / `finalizeT2Aggregation` keep reverting and there is no owner path to skip the batch or abort the GI. -- **No. 7 — Stale NatSpec:** `slashAggregators` says the slash is always `minStake()` (no-reveal is now the S2 fraction). Comments around the dispute scaffold still say `DinTreasury` "doesn't exist on develop yet" (forfeitures are already forwarded). Several parameters are described as "DAO-settable"; they are `onlyOwner`, i.e. set by the model owner. +- **No. 7 — Stale NatSpec:** comments around the dispute scaffold still say `DinTreasury` "doesn't exist on develop yet" (forfeitures are already forwarded). Several parameters are described as "DAO-settable"; they are `onlyOwner`, i.e. set by the model owner. - **No. 8 — Leftovers:** `networkFeeFloor` is stored but not enforced. `setTestDataAssignedFlag` gates nothing: evaluation can start without test data being assigned. `releaseGIRegistrationSlots` uses string `require` messages, unlike the rest of the contract. - **No. 9 — Not upgradeable:** a bug in a model's task contracts requires redeploying them and re-registering the model. - **No. 10 — dincli lags this contract:** `dincli model-owner deploy task-coordinator` still calls the older one-argument constructor (no `modelId`). `dincli aggregator aggregate-t2` names its working directory, worker job and container after the last T1 batch id, not the T2 batch id (issue #202, Part 2); the on-chain commit is unaffected. @@ -297,3 +299,4 @@ Earlier findings from the [foundry/src security review](../audits/foundry-src-se - `modelId` constructor argument. - Locked batch-assignment seeds for auditor and T1/T2 batches (`lockAuditSeed`, `lockAggSeed`; issue #156 H-2, PR #191). - Commit-then-reveal T1/T2 aggregation with a sender-bound commit hash (`commitT*Aggregation`, `revealT*Aggregation`, `startT*AggregationReveal`; issue #156 M-1, PR #197). Adds states `T1AggregationRevealStarted` (18) and `T2AggregationRevealStarted` (21), shifting later ordinals. +- Back under EIP-170 (issue #201 Part A). The `slashAggregators` T1/T2 loops are folded into `_slashBatch`, with unchanged slashes and events. `tier1Batches` / `tier2Batches` and the ten `t1*`/`t2*` per-aggregator maps are now `internal`, and the new `getAggregatorSubmission` view replaces their getters. This is a view-ABI change; dincli and the bundled ABI were updated with it. diff --git a/Documentation/technical/testing/dincli-testing-guide.md b/Documentation/technical/testing/dincli-testing-guide.md index 831fdfd5..de926f45 100644 --- a/Documentation/technical/testing/dincli-testing-guide.md +++ b/Documentation/technical/testing/dincli-testing-guide.md @@ -10,7 +10,7 @@ The chain backend and platform deploy script are chosen by chain ID 1337 on `http://127.0.0.1:8545`. > [!WARNING] -> `foundry/anvil.sh` starts Anvil with `--code-size-limit 4294967295`, so the local chain accepts contracts above the 24,576-byte EIP-170 limit. `DINTaskCoordinator` is currently over that limit (issue #201), so a green local run does not show that a contract can be deployed to a real chain. +> `foundry/anvil.sh` starts Anvil with `--code-size-limit 4294967295`, so the local chain accepts contracts above the 24,576-byte EIP-170 limit. A green local run therefore does not show that a contract can be deployed to a real chain. CI enforces the limit instead, with `.github/scripts/contract_size_gate.py` (issue #201). --- diff --git a/dincli/abis/DINTaskCoordinator.json b/dincli/abis/DINTaskCoordinator.json index 967a44d9..20af9c1e 100644 --- a/dincli/abis/DINTaskCoordinator.json +++ b/dincli/abis/DINTaskCoordinator.json @@ -590,6 +590,60 @@ ], "stateMutability": "view" }, + { + "type": "function", + "name": "getAggregatorSubmission", + "inputs": [ + { + "name": "_GI", + "type": "uint256", + "internalType": "uint256" + }, + { + "name": "tier", + "type": "uint8", + "internalType": "enum DINTaskCoordinator.TierKind" + }, + { + "name": "_batchId", + "type": "uint256", + "internalType": "uint256" + }, + { + "name": "aggregator", + "type": "address", + "internalType": "address" + } + ], + "outputs": [ + { + "name": "committed", + "type": "bool", + "internalType": "bool" + }, + { + "name": "commitHash", + "type": "bytes32", + "internalType": "bytes32" + }, + { + "name": "submitted", + "type": "bool", + "internalType": "bool" + }, + { + "name": "cid", + "type": "bytes32", + "internalType": "bytes32" + }, + { + "name": "votes", + "type": "uint256", + "internalType": "uint256" + } + ], + "stateMutability": "view" + }, { "type": "function", "name": "getDINtaskAggregators", @@ -1387,296 +1441,6 @@ "outputs": [], "stateMutability": "nonpayable" }, - { - "type": "function", - "name": "t1CommitHash", - "inputs": [ - { - "name": "", - "type": "uint256", - "internalType": "uint256" - }, - { - "name": "", - "type": "uint256", - "internalType": "uint256" - }, - { - "name": "", - "type": "address", - "internalType": "address" - } - ], - "outputs": [ - { - "name": "", - "type": "bytes32", - "internalType": "bytes32" - } - ], - "stateMutability": "view" - }, - { - "type": "function", - "name": "t1Committed", - "inputs": [ - { - "name": "", - "type": "uint256", - "internalType": "uint256" - }, - { - "name": "", - "type": "uint256", - "internalType": "uint256" - }, - { - "name": "", - "type": "address", - "internalType": "address" - } - ], - "outputs": [ - { - "name": "", - "type": "bool", - "internalType": "bool" - } - ], - "stateMutability": "view" - }, - { - "type": "function", - "name": "t1SubmissionCID", - "inputs": [ - { - "name": "", - "type": "uint256", - "internalType": "uint256" - }, - { - "name": "", - "type": "uint256", - "internalType": "uint256" - }, - { - "name": "", - "type": "address", - "internalType": "address" - } - ], - "outputs": [ - { - "name": "", - "type": "bytes32", - "internalType": "bytes32" - } - ], - "stateMutability": "view" - }, - { - "type": "function", - "name": "t1Submitted", - "inputs": [ - { - "name": "", - "type": "uint256", - "internalType": "uint256" - }, - { - "name": "", - "type": "uint256", - "internalType": "uint256" - }, - { - "name": "", - "type": "address", - "internalType": "address" - } - ], - "outputs": [ - { - "name": "", - "type": "bool", - "internalType": "bool" - } - ], - "stateMutability": "view" - }, - { - "type": "function", - "name": "t1Votes", - "inputs": [ - { - "name": "", - "type": "uint256", - "internalType": "uint256" - }, - { - "name": "", - "type": "uint256", - "internalType": "uint256" - }, - { - "name": "", - "type": "bytes32", - "internalType": "bytes32" - } - ], - "outputs": [ - { - "name": "", - "type": "uint256", - "internalType": "uint256" - } - ], - "stateMutability": "view" - }, - { - "type": "function", - "name": "t2CommitHash", - "inputs": [ - { - "name": "", - "type": "uint256", - "internalType": "uint256" - }, - { - "name": "", - "type": "uint256", - "internalType": "uint256" - }, - { - "name": "", - "type": "address", - "internalType": "address" - } - ], - "outputs": [ - { - "name": "", - "type": "bytes32", - "internalType": "bytes32" - } - ], - "stateMutability": "view" - }, - { - "type": "function", - "name": "t2Committed", - "inputs": [ - { - "name": "", - "type": "uint256", - "internalType": "uint256" - }, - { - "name": "", - "type": "uint256", - "internalType": "uint256" - }, - { - "name": "", - "type": "address", - "internalType": "address" - } - ], - "outputs": [ - { - "name": "", - "type": "bool", - "internalType": "bool" - } - ], - "stateMutability": "view" - }, - { - "type": "function", - "name": "t2SubmissionCID", - "inputs": [ - { - "name": "", - "type": "uint256", - "internalType": "uint256" - }, - { - "name": "", - "type": "uint256", - "internalType": "uint256" - }, - { - "name": "", - "type": "address", - "internalType": "address" - } - ], - "outputs": [ - { - "name": "", - "type": "bytes32", - "internalType": "bytes32" - } - ], - "stateMutability": "view" - }, - { - "type": "function", - "name": "t2Submitted", - "inputs": [ - { - "name": "", - "type": "uint256", - "internalType": "uint256" - }, - { - "name": "", - "type": "uint256", - "internalType": "uint256" - }, - { - "name": "", - "type": "address", - "internalType": "address" - } - ], - "outputs": [ - { - "name": "", - "type": "bool", - "internalType": "bool" - } - ], - "stateMutability": "view" - }, - { - "type": "function", - "name": "t2Votes", - "inputs": [ - { - "name": "", - "type": "uint256", - "internalType": "uint256" - }, - { - "name": "", - "type": "uint256", - "internalType": "uint256" - }, - { - "name": "", - "type": "bytes32", - "internalType": "bytes32" - } - ], - "outputs": [ - { - "name": "", - "type": "uint256", - "internalType": "uint256" - } - ], - "stateMutability": "view" - }, { "type": "function", "name": "tier1BatchCount", @@ -1696,40 +1460,6 @@ ], "stateMutability": "view" }, - { - "type": "function", - "name": "tier1Batches", - "inputs": [ - { - "name": "", - "type": "uint256", - "internalType": "uint256" - }, - { - "name": "", - "type": "uint256", - "internalType": "uint256" - } - ], - "outputs": [ - { - "name": "batchId", - "type": "uint256", - "internalType": "uint256" - }, - { - "name": "finalized", - "type": "bool", - "internalType": "bool" - }, - { - "name": "finalCID", - "type": "bytes32", - "internalType": "bytes32" - } - ], - "stateMutability": "view" - }, { "type": "function", "name": "tier1FinalizedAt", @@ -1754,40 +1484,6 @@ ], "stateMutability": "view" }, - { - "type": "function", - "name": "tier2Batches", - "inputs": [ - { - "name": "", - "type": "uint256", - "internalType": "uint256" - }, - { - "name": "", - "type": "uint256", - "internalType": "uint256" - } - ], - "outputs": [ - { - "name": "batchId", - "type": "uint256", - "internalType": "uint256" - }, - { - "name": "finalized", - "type": "bool", - "internalType": "bool" - }, - { - "name": "finalCID", - "type": "bytes32", - "internalType": "bytes32" - } - ], - "stateMutability": "view" - }, { "type": "function", "name": "tier2FinalizedAt", diff --git a/dincli/cli/aggregator.py b/dincli/cli/aggregator.py index 647e0668..8b76214f 100644 --- a/dincli/cli/aggregator.py +++ b/dincli/cli/aggregator.py @@ -210,7 +210,7 @@ def show_t1_batches( if detailed: time.sleep(0.2) - submission_cid = taskCoordinator_contract.functions.t1SubmissionCID(curr_GI, bid, validator).call() + submission_cid = taskCoordinator_contract.functions.getAggregatorSubmission(curr_GI, TIER1, bid, validator).call()[3] submission_cid_str = get_cid_from_bytes32(submission_cid.hex()) if submission_cid and any(submission_cid) else "" table.add_row(str(bid), ", ".join(map(str, model_idxs)), str(finalized), cid_str, submission_cid_str) found_batches = True @@ -262,7 +262,7 @@ def show_t2_batches( table.add_row(str(bid), str(finalized), cid_str) else: time.sleep(0.2) - submission_cid = taskCoordinator_contract.functions.t2SubmissionCID(curr_GI, bid, validator).call() + submission_cid = taskCoordinator_contract.functions.getAggregatorSubmission(curr_GI, TIER2, bid, validator).call()[3] submission_cid_str = get_cid_from_bytes32(submission_cid.hex()) if submission_cid and any(submission_cid) else "" table.add_row(str(bid), str(finalized), cid_str, submission_cid_str) found_batches = True @@ -319,7 +319,7 @@ def aggregate_t1( # A retry must never replace the salt behind an existing on-chain # commitment -- the reveal would then fail TC_T1RevealHashMismatch and # the aggregator be S2-slashed. Skip before re-aggregating. - if submit and taskCoordinator_contract.functions.t1Committed(curr_GI, bid, account.address).call(): + if submit and taskCoordinator_contract.functions.getAggregatorSubmission(curr_GI, TIER1, bid, account.address).call()[0]: console.print(f"[yellow]T1 batch {bid} already committed by {account.address}; skipping (reveal with `dincli aggregator reveal-t1`).[/yellow]") continue @@ -529,7 +529,7 @@ def aggregate_t2( # Same retry guard as aggregate-t1: never replace the salt behind an # existing on-chain commitment. - if submit and taskCoordinator_contract.functions.t2Committed(curr_GI, i, account.address).call(): + if submit and taskCoordinator_contract.functions.getAggregatorSubmission(curr_GI, TIER2, i, account.address).call()[0]: found_batch = True console.print(f"[yellow]T2 batch {i} already committed by {account.address}; skipping (reveal with `dincli aggregator reveal-t2`).[/yellow]") continue diff --git a/dincli/cli/modelownerd/aggregation.py b/dincli/cli/modelownerd/aggregation.py index c59f2101..73343297 100644 --- a/dincli/cli/modelownerd/aggregation.py +++ b/dincli/cli/modelownerd/aggregation.py @@ -106,7 +106,7 @@ def show_t1_batches( final_cid = get_cid_from_bytes32(final_cid_raw.hex()) if final_cid_raw and final_cid_raw != bytes(32) else "Pending" for validator in validators: - submitted_cid_raw = task_coordinator_Contract.functions.t1SubmissionCID(ref_gi, bid, validator).call() + submitted_cid_raw = task_coordinator_Contract.functions.getAggregatorSubmission(ref_gi, 0, bid, validator).call()[3] # 0 = TierKind.Tier1; [3] = revealed cid submitted_cid = get_cid_from_bytes32(submitted_cid_raw.hex()) if submitted_cid_raw and submitted_cid_raw != bytes(32) else "None" idxs_display = ", ".join(map(str, model_idxs)) detailed_table.add_row(str(bid), validator, submitted_cid, idxs_display, final_cid) @@ -151,7 +151,7 @@ def show_t2_batches( else: submitted_parts = [] for v in validators: - sub_raw = task_coordinator_Contract.functions.t2SubmissionCID(ref_gi, bid, v).call() + sub_raw = task_coordinator_Contract.functions.getAggregatorSubmission(ref_gi, 1, bid, v).call()[3] # 1 = TierKind.Tier2 submitted_parts.append(get_cid_from_bytes32(sub_raw.hex()) if sub_raw and sub_raw != bytes(32) else "") submitted_cid_display = "\n".join(submitted_parts) table.add_row(str(bid), val_display, submitted_cid_display, str(finalized), cid) diff --git a/foundry/anvil.sh b/foundry/anvil.sh index c33bff19..e2630de4 100755 --- a/foundry/anvil.sh +++ b/foundry/anvil.sh @@ -6,7 +6,10 @@ anvil \ --code-size-limit 4294967295 \ --block-time 2 \ --mnemonic "test test test test test test test test test test test junk" -# --code-size-limit 4294967295 +# --code-size-limit 4294967295: lifts the EIP-170 24,576-byte runtime +# limit, so local deploys accept contracts a real chain would reject. +# CI enforces the limit instead: .github/scripts/contract_size_gate.py +# fails any foundry/src contract with < 1,024 B margin (issue #201). # --block-time 2: without a block-time, anvil only mines when a tx # arrives, so an idle chain sits at block 0 forever. dincli's # ensure_batch_seed_locked (issue #156 H-2) polls w3.eth.block_number diff --git a/foundry/src/DINTaskCoordinator.sol b/foundry/src/DINTaskCoordinator.sol index b7475de3..f746e470 100644 --- a/foundry/src/DINTaskCoordinator.sol +++ b/foundry/src/DINTaskCoordinator.sol @@ -50,7 +50,11 @@ contract DINTaskCoordinator is Ownable, ReentrancyGuardTransient { bytes32 finalCID; // Majority‐agreed CID } - mapping(uint => Tier1Batch[]) public tier1Batches; + // tier1Batches/tier2Batches and the t1*/t2* per-aggregator maps below are + // internal to keep this contract under EIP-170 (issue #201 Part A): read + // batches via getTier1Batch/getTier2Batch and per-aggregator commit/reveal + // state via getAggregatorSubmission. + mapping(uint => Tier1Batch[]) internal tier1Batches; mapping(uint => mapping(uint => mapping(address => bool))) isTier1Aggregator; // Audit & voting maps GI ➜ batchId ➜ validator ➜ … @@ -60,12 +64,12 @@ contract DINTaskCoordinator is Ownable, ReentrancyGuardTransient { // what slashAggregators()'s existing "no submission" (S2) check already // reads, so that loop needed no changes for the commit-reveal split. mapping(uint => mapping(uint => mapping(address => bytes32))) - public t1SubmissionCID; + internal t1SubmissionCID; mapping(uint => mapping(uint => mapping(address => bool))) - public t1Submitted; - mapping(uint => mapping(uint => mapping(bytes32 => uint))) public t1Votes; // CID ➜ votes - mapping(uint => mapping(uint => mapping(address => bytes32))) public t1CommitHash; - mapping(uint => mapping(uint => mapping(address => bool))) public t1Committed; + internal t1Submitted; + mapping(uint => mapping(uint => mapping(bytes32 => uint))) internal t1Votes; // CID ➜ votes + mapping(uint => mapping(uint => mapping(address => bytes32))) internal t1CommitHash; + mapping(uint => mapping(uint => mapping(address => bool))) internal t1Committed; struct Tier2Batch { uint batchId; @@ -74,17 +78,17 @@ contract DINTaskCoordinator is Ownable, ReentrancyGuardTransient { bytes32 finalCID; } - mapping(uint => Tier2Batch[]) public tier2Batches; + mapping(uint => Tier2Batch[]) internal tier2Batches; mapping(uint => mapping(uint => mapping(address => bool))) isTier2Aggregator; mapping(uint => uint) public tier2Score; mapping(uint => mapping(uint => mapping(address => bytes32))) - public t2SubmissionCID; + internal t2SubmissionCID; mapping(uint => mapping(uint => mapping(address => bool))) - public t2Submitted; - mapping(uint => mapping(uint => mapping(bytes32 => uint))) public t2Votes; - mapping(uint => mapping(uint => mapping(address => bytes32))) public t2CommitHash; - mapping(uint => mapping(uint => mapping(address => bool))) public t2Committed; + internal t2Submitted; + mapping(uint => mapping(uint => mapping(bytes32 => uint))) internal t2Votes; + mapping(uint => mapping(uint => mapping(address => bytes32))) internal t2CommitHash; + mapping(uint => mapping(uint => mapping(address => bool))) internal t2Committed; /// @notice Per-aggregator count of finalized T1/T2 batches they were /// assigned to in a GI, one increment per (aggregator, @@ -778,6 +782,46 @@ contract DINTaskCoordinator is Ownable, ReentrancyGuardTransient { return (b.batchId, b.aggregators, b.finalized, b.finalCID); } + /// @notice One aggregator's commit-then-reveal state for a T1 or T2 batch. + /// @dev Replaces the former public t1*/t2* getters (issue #201 Part A). + /// `votes` is the vote count of this aggregator's own revealed CID + /// (t1Votes/t2Votes are keyed by CID, not by aggregator). Before a + /// reveal `cid` is zero, and the zero CID never has votes (reveal + /// rejects it with TC_ZeroCID), so `votes` is 0 until the reveal. + /// @param _GI GI index. + /// @param tier Tier1 or Tier2. + /// @param _batchId Batch index (always 0 for Tier2). + /// @param aggregator Aggregator address. + /// @return committed True once the aggregator has committed. + /// @return commitHash The stored commit hash. + /// @return submitted True once the aggregator has revealed. + /// @return cid The revealed CID (zero before reveal). + /// @return votes Votes for `cid` in this batch (zero before reveal). + function getAggregatorSubmission( + uint _GI, + TierKind tier, + uint _batchId, + address aggregator + ) + external + view + returns (bool committed, bytes32 commitHash, bool submitted, bytes32 cid, uint votes) + { + if (tier == TierKind.Tier1) { + committed = t1Committed[_GI][_batchId][aggregator]; + commitHash = t1CommitHash[_GI][_batchId][aggregator]; + submitted = t1Submitted[_GI][_batchId][aggregator]; + cid = t1SubmissionCID[_GI][_batchId][aggregator]; + votes = t1Votes[_GI][_batchId][cid]; + } else { + committed = t2Committed[_GI][_batchId][aggregator]; + commitHash = t2CommitHash[_GI][_batchId][aggregator]; + submitted = t2Submitted[_GI][_batchId][aggregator]; + cid = t2SubmissionCID[_GI][_batchId][aggregator]; + votes = t2Votes[_GI][_batchId][cid]; + } + } + /// @notice Transitions GI state to T1AggregationStarted, opening the /// Tier-1 commit window for assigned aggregators /// (commitT1Aggregation). @@ -1105,8 +1149,11 @@ contract DINTaskCoordinator is Ownable, ReentrancyGuardTransient { /// @notice Slashes aggregators in both Tier-1 and Tier-2 batches that failed /// to submit or submitted a CID that did not match the consensus. - /// @dev Slash amount equals minStake() at call time. Each affected aggregator - /// emits an AggregatorSlashed event with the actual amount deducted. + /// @dev No submission (including committed but never revealed) is an S2 + /// liveness fault slashed at s2SlashFractionBps of minStake(); a CID + /// that differs from the batch's finalCID is slashed the full + /// minStake(). Each affected aggregator emits an AggregatorSlashed + /// event with the actual amount deducted. See _slashBatch. /// @param _GI Current GI index. function slashAggregators(uint _GI) external onlyOwner onlyCurrentGI(_GI) { if (GIstate != GIstates.AuditorsSlashed) @@ -1116,85 +1163,64 @@ contract DINTaskCoordinator is Ownable, ReentrancyGuardTransient { // S2: partial fraction for liveness fault (no submission); BAD_CONSENSUS keeps full. uint256 s2Amount = (minStakeAmt * s2SlashFractionBps) / 10_000; - // 1. Tier 1 batches Tier1Batch[] storage t1batches = tier1Batches[_GI]; for (uint i = 0; i < t1batches.length; i++) { Tier1Batch storage b = t1batches[i]; - for (uint j = 0; j < b.aggregators.length; j++) { - address aggregator = b.aggregators[j]; - - bool submitted = t1Submitted[_GI][b.batchId][aggregator]; - if (!submitted) { - // S2 liveness fault: partial slash + S5/S6 tracking. - // Skip when rounding reduces s2Amount to 0 — slashPartial - // reverts InvalidSlashAmount on zero, bricking the GI. - uint256 actualSlashed = s2Amount > 0 - ? dinvalidatorStakeContract.slashPartial( - aggregator, - s2Amount, - "AGG_T1_NO_SUBMISSION", - _GI - ) - : 0; - emit AggregatorSlashed(_GI, b.batchId, aggregator, "AGG_T1_NO_SUBMISSION", s2Amount, actualSlashed); - // No S6 recordNoParticipation here: slashPartial above already - // penalises this missed submission (S2, escalating to S5 on - // repeat). Also firing S6 on the same event could slash more - // than MIN_STAKE in one event (S2/S5 + S6 stacking). - } else { - bytes32 cid = t1SubmissionCID[_GI][b.batchId][aggregator]; - if (cid != b.finalCID) { - // BAD_CONSENSUS: full-severity slash (active incorrect submission). - uint256 actualSlashed = dinvalidatorStakeContract.slash( - aggregator, - minStakeAmt, - "AGG_T1_BAD_CONSENSUS" - ); - emit AggregatorSlashed(_GI, b.batchId, aggregator, "AGG_T1_BAD_CONSENSUS", minStakeAmt, actualSlashed); - } - } - } + _slashBatch(_GI, b.batchId, b.aggregators, b.finalCID, minStakeAmt, s2Amount, false); } - - // 2. Tier 2 batches Tier2Batch[] storage t2batches = tier2Batches[_GI]; for (uint i = 0; i < t2batches.length; i++) { Tier2Batch storage b = t2batches[i]; - for (uint j = 0; j < b.aggregators.length; j++) { - address aggregator = b.aggregators[j]; + _slashBatch(_GI, b.batchId, b.aggregators, b.finalCID, minStakeAmt, s2Amount, true); + } - bool submitted = t2Submitted[_GI][b.batchId][aggregator]; - if (!submitted) { - // S2 liveness fault: partial slash + S5/S6 tracking. - // Skip when rounding reduces s2Amount to 0 (same guard as T1). - uint256 actualSlashed = s2Amount > 0 - ? dinvalidatorStakeContract.slashPartial( - aggregator, - s2Amount, - "AGG_T2_NO_SUBMISSION", - _GI - ) - : 0; - emit AggregatorSlashed(_GI, b.batchId, aggregator, "AGG_T2_NO_SUBMISSION", s2Amount, actualSlashed); - // No S6 recordNoParticipation here: same rationale as the T1 - // branch above (S2/S5 already covers this event; avoids - // stacking past MIN_STAKE). - } else { - bytes32 cid = t2SubmissionCID[_GI][b.batchId][aggregator]; - if (cid != b.finalCID) { - // BAD_CONSENSUS: full-severity slash (active incorrect submission). - uint256 actualSlashed = dinvalidatorStakeContract.slash( - aggregator, - minStakeAmt, - "AGG_T2_BAD_CONSENSUS" - ); - emit AggregatorSlashed(_GI, b.batchId, aggregator, "AGG_T2_BAD_CONSENSUS", minStakeAmt, actualSlashed); - } + _setGIstate(GIstates.AggregatorsSlashed); + } + + /// @dev Slashes one T1 or T2 batch's aggregators for slashAggregators + /// (one body for both tiers keeps this contract under EIP-170, issue + /// #201 Part A). No submission is an S2 liveness fault: partial slash + /// plus S5/S6 tracking, skipped when rounding makes s2Amount 0 + /// (slashPartial reverts InvalidSlashAmount on zero, bricking the GI). + /// No S6 recordNoParticipation here: slashPartial already penalises + /// the missed submission, and also firing S6 could slash more than + /// MIN_STAKE in one event. A submitted CID that differs from the + /// batch's finalCID is BAD_CONSENSUS: full-severity slash. + function _slashBatch( + uint _GI, + uint batchId, + address[] storage aggs, + bytes32 finalCID, + uint256 minStakeAmt, + uint256 s2Amount, + bool t2 + ) internal { + bytes32 noSubmission = "AGG_T1_NO_SUBMISSION"; + bytes32 badConsensus = "AGG_T1_BAD_CONSENSUS"; + if (t2) { + noSubmission = "AGG_T2_NO_SUBMISSION"; + badConsensus = "AGG_T2_BAD_CONSENSUS"; + } + for (uint j = 0; j < aggs.length; j++) { + address aggregator = aggs[j]; + bool submitted = t2 + ? t2Submitted[_GI][batchId][aggregator] + : t1Submitted[_GI][batchId][aggregator]; + if (!submitted) { + uint256 actualSlashed = s2Amount > 0 + ? dinvalidatorStakeContract.slashPartial(aggregator, s2Amount, noSubmission, _GI) + : 0; + emit AggregatorSlashed(_GI, batchId, aggregator, noSubmission, s2Amount, actualSlashed); + } else { + bytes32 cid = t2 + ? t2SubmissionCID[_GI][batchId][aggregator] + : t1SubmissionCID[_GI][batchId][aggregator]; + if (cid != finalCID) { + uint256 actualSlashed = dinvalidatorStakeContract.slash(aggregator, minStakeAmt, badConsensus); + emit AggregatorSlashed(_GI, batchId, aggregator, badConsensus, minStakeAmt, actualSlashed); } } } - - _setGIstate(GIstates.AggregatorsSlashed); } /// @notice Records the Tier-2 aggregation quality score for the current GI. diff --git a/foundry/test/AggregatorCommitReveal.t.sol b/foundry/test/AggregatorCommitReveal.t.sol index 52ad0813..a4785779 100644 --- a/foundry/test/AggregatorCommitReveal.t.sol +++ b/foundry/test/AggregatorCommitReveal.t.sol @@ -296,8 +296,46 @@ contract AggregatorCommitRevealTest is Test { _openT1RevealPhase(); _revealT1(t1aggs[0], 0, CID_A); - assertTrue(tc.t1Submitted(1, 0, t1aggs[0])); - assertEq(tc.t1SubmissionCID(1, 0, t1aggs[0]), CID_A); + (, , bool submitted, bytes32 cid, ) = tc.getAggregatorSubmission(1, DINTaskCoordinator.TierKind.Tier1, 0, t1aggs[0]); + assertTrue(submitted); + assertEq(cid, CID_A); + } + + /// @dev getAggregatorSubmission (issue #201 Part A) replaces the former + /// public t1*/t2* getters: committed-only state before reveal, then + /// the revealed CID and that CID's vote count. + function test_getAggregatorSubmission_t1_commitThenReveal() public { + _runToT1AggregationStarted(); + (, address[] memory t1aggs, , , ) = tc.getTier1Batch(1, 0); + _commitT1(t1aggs[0], 0, CID_A); + _commitT1(t1aggs[1], 0, CID_A); + + (bool committed, bytes32 commitHash, bool submitted, bytes32 cid, uint votes) = + tc.getAggregatorSubmission(1, DINTaskCoordinator.TierKind.Tier1, 0, t1aggs[0]); + assertTrue(committed); + assertEq(commitHash, _t1CommitHash(t1aggs[0], CID_A, TEST_SALT, 0)); + assertFalse(submitted); + assertEq(cid, bytes32(0)); + assertEq(votes, 0); + + _openT1RevealPhase(); + _revealT1(t1aggs[0], 0, CID_A); + _revealT1(t1aggs[1], 0, CID_A); + + (committed, , submitted, cid, votes) = + tc.getAggregatorSubmission(1, DINTaskCoordinator.TierKind.Tier1, 0, t1aggs[0]); + assertTrue(committed); + assertTrue(submitted); + assertEq(cid, CID_A); + assertEq(votes, 2); + + // An assigned aggregator who never committed reads all-zero. + (committed, commitHash, submitted, cid, votes) = + tc.getAggregatorSubmission(1, DINTaskCoordinator.TierKind.Tier1, 0, t1aggs[2]); + assertFalse(committed); + assertEq(commitHash, bytes32(0)); + assertFalse(submitted); + assertEq(votes, 0); } /// @dev issue #156 M-1's actual hardening: the commit hash binds @@ -473,6 +511,31 @@ contract AggregatorCommitRevealTest is Test { (, t2aggs, , ) = tc.getTier2Batch(1, 0); } + function test_getAggregatorSubmission_t2_commitThenReveal() public { + address[] memory t2aggs = _runToT2AggregationStarted(); + _commitT2(t2aggs[0], CID_B); + + (bool committed, bytes32 commitHash, bool submitted, , uint votes) = + tc.getAggregatorSubmission(1, DINTaskCoordinator.TierKind.Tier2, 0, t2aggs[0]); + assertTrue(committed); + assertEq(commitHash, _t2CommitHash(t2aggs[0], CID_B, TEST_SALT)); + assertFalse(submitted); + assertEq(votes, 0); + + _openT2RevealPhase(); + _revealT2(t2aggs[0], CID_B); + + bytes32 cid; + (, , submitted, cid, votes) = tc.getAggregatorSubmission(1, DINTaskCoordinator.TierKind.Tier2, 0, t2aggs[0]); + assertTrue(submitted); + assertEq(cid, CID_B); + assertEq(votes, 1); + + // Tier is honoured: the same address/batch under Tier1 reads the T1 state. + (, , , cid, ) = tc.getAggregatorSubmission(1, DINTaskCoordinator.TierKind.Tier1, 0, t2aggs[0]); + assertTrue(cid != CID_B); + } + function test_t2_copyAttack_replayingPeerCommitHash_cannotReveal() public { address[] memory t2aggs = _runToT2AggregationStarted(); bytes32 honestCommitHash = _t2CommitHash(t2aggs[0], CID_B, TEST_SALT); @@ -596,8 +659,12 @@ contract AggregatorCommitRevealTest is Test { vm.prank(modelOwner); tc.finalizeT1Aggregation(1); - assertTrue(tc.t1Submitted(1, 0, t1aggs[0])); - assertFalse(tc.t1Submitted(1, 0, t1aggs[2]), "committed but never revealed -- excluded like a non-participant"); + (, , bool revealed0, , ) = tc.getAggregatorSubmission(1, DINTaskCoordinator.TierKind.Tier1, 0, t1aggs[0]); + (bool committed2, , bool revealed2, , ) = + tc.getAggregatorSubmission(1, DINTaskCoordinator.TierKind.Tier1, 0, t1aggs[2]); + assertTrue(revealed0); + assertTrue(committed2); + assertFalse(revealed2, "committed but never revealed -- excluded like a non-participant"); // Run T2 to completion too (the fixture's other 3 aggregators form a // real T2 batch, not a trivial empty one) so slashAuditors/ diff --git a/tests/test_aggregator_commit_retry.py b/tests/test_aggregator_commit_retry.py index 6c4d7962..7d30e156 100644 --- a/tests/test_aggregator_commit_retry.py +++ b/tests/test_aggregator_commit_retry.py @@ -50,8 +50,9 @@ def getter(name): return lambda gi, i: _Call((0, [ACCOUNT], [], False, b"\x00" * 32)) if name == "getTier2Batch": return lambda gi, i: _Call((0, [ACCOUNT], False, b"\x00" * 32)) - if name in ("t1Committed", "t2Committed"): - return lambda gi, bid, who: _Call(state["committed"]) + if name == "getAggregatorSubmission": + # (committed, commitHash, submitted, cid, votes) + return lambda gi, tier, bid, who: _Call((state["committed"], b"\x00" * 32, False, b"\x00" * 32, 0)) if name in ("commitT1Aggregation", "commitT2Aggregation"): def _commit(*args): state["commits"].append((name, args)) From c3e585d1490a359babfe4864f0b78c8a630484b8 Mon Sep 17 00:00:00 2001 From: umermjd11 Date: Fri, 2 Oct 2026 13:39:04 +0500 Subject: [PATCH 2/2] fix(task-coordinator): pin registerDINaggregator to the current GI (#206) registerDINaggregator(_GI) checked GIstate but used the caller's _GI for every read and write, so during GI N's registration window a validator could register for GI N+1 (filling its 300-slot aggregator list before it opened, and so every T1/T2 batch) or for a released past GI (leaking a concurrent-registration slot). Add onlyCurrentGI, matching DINTaskAuditor.registerDINAuditor. Tests: future GI and past GI revert with TC_WrongGI and consume no registration slot; the current GI still registers. +9 B runtime (23,497 B, 1,079 B margin; size gate green). ABI unchanged. Co-Authored-By: Claude Opus 5.5 --- .../technical/contracts/DINTaskCoordinator.md | 5 +-- foundry/src/DINTaskCoordinator.sol | 2 +- foundry/test/StakingEnforcement.t.sol | 35 +++++++++++++++++++ 3 files changed, 39 insertions(+), 3 deletions(-) diff --git a/Documentation/technical/contracts/DINTaskCoordinator.md b/Documentation/technical/contracts/DINTaskCoordinator.md index e10b9b92..9c743178 100644 --- a/Documentation/technical/contracts/DINTaskCoordinator.md +++ b/Documentation/technical/contracts/DINTaskCoordinator.md @@ -155,7 +155,7 @@ The three reveal-opening calls (`startLMsubmissionsEvaluationReveal`, `startT1Ag Requires `_GI == GI + 1` and a funded reward pool: `dinTaskAuditorContract.giRewardPool(_GI) > 0`, otherwise `TC_GIRewardPoolNotFunded`. The two-argument overload also calls `updatePassScore`. Then `GI++`. ### 6.2 `registerDINaggregator` -In `DINaggregatorsRegistrationStarted` only. There is no `onlyCurrentGI` modifier: the checks and the registration list use the `_GI` the caller passes. Checks in order: `isValidatorActive` (`TC_AggregatorNotActive`), not already registered, fewer than 300 registered (`TC_RegistrationCapReached`), stake ≥ `getModelStakeMin(modelId)` when non-zero (`TC_StakeBelowModelFloor`), and — when `maxConcurrentRegistrationsPerStakeUnit > 0` — `activeRegistrationCount < (stake / minStake) × cap` (`TC_ConcurrentRegistrationCapReached`). Then records the aggregator, calls `incrementActiveRegistration`, emits `DINValidatorRegistered`. +In `DINaggregatorsRegistrationStarted` only, and `onlyCurrentGI`: `_GI` must be the current GI (`TC_WrongGI`), as for `DINTaskAuditor.registerDINAuditor` (issue #206). Checks in order: `isValidatorActive` (`TC_AggregatorNotActive`), not already registered, fewer than 300 registered (`TC_RegistrationCapReached`), stake ≥ `getModelStakeMin(modelId)` when non-zero (`TC_StakeBelowModelFloor`), and — when `maxConcurrentRegistrationsPerStakeUnit > 0` — `activeRegistrationCount < (stake / minStake) × cap` (`TC_ConcurrentRegistrationCapReached`). Then records the aggregator, calls `incrementActiveRegistration`, emits `DINValidatorRegistered`. ### 6.3 Batch-Assignment Seed Lock @@ -280,7 +280,7 @@ Earlier findings from the [foundry/src security review](../audits/foundry-src-se - **No. 8 — Leftovers:** `networkFeeFloor` is stored but not enforced. `setTestDataAssignedFlag` gates nothing: evaluation can start without test data being assigned. `releaseGIRegistrationSlots` uses string `require` messages, unlike the rest of the contract. - **No. 9 — Not upgradeable:** a bug in a model's task contracts requires redeploying them and re-registering the model. - **No. 10 — dincli lags this contract:** `dincli model-owner deploy task-coordinator` still calls the older one-argument constructor (no `modelId`). `dincli aggregator aggregate-t2` names its working directory, worker job and container after the last T1 batch id, not the T2 batch id (issue #202, Part 2); the on-chain commit is unaffected. -- **No. 11 — `registerDINaggregator` doesn't check the GI.** Unlike `DINTaskAuditor.registerDINAuditor`, it has no `onlyCurrentGI`: while any GI's registration window is open, a validator can register for a future GI (§6.2). Up to 300 addresses can fill GI N+1's list during GI N's window, which locks out honest registrants and hands the attacker every T1/T2 batch. Registering for an already-released past GI leaks the caller's own concurrent-registration slot. dincli always passes the current GI. Tracked in issue #206; the fix (add `onlyCurrentGI`) has to fit within the EIP-170 budget from No. 1. +- **No. 11 — Fixed: `registerDINaggregator` now checks the GI.** It used to have no `onlyCurrentGI`, so during GI N's window a validator could register for GI N+1. Up to 300 addresses could fill that list, which locked out honest registrants and handed the attacker every T1/T2 batch. Registering for a released past GI also leaked the caller's own slot. Fixed by adding `onlyCurrentGI` (issue #206); it cost 9 bytes inside the No. 1 budget. --- @@ -300,3 +300,4 @@ Earlier findings from the [foundry/src security review](../audits/foundry-src-se - Locked batch-assignment seeds for auditor and T1/T2 batches (`lockAuditSeed`, `lockAggSeed`; issue #156 H-2, PR #191). - Commit-then-reveal T1/T2 aggregation with a sender-bound commit hash (`commitT*Aggregation`, `revealT*Aggregation`, `startT*AggregationReveal`; issue #156 M-1, PR #197). Adds states `T1AggregationRevealStarted` (18) and `T2AggregationRevealStarted` (21), shifting later ordinals. - Back under EIP-170 (issue #201 Part A). The `slashAggregators` T1/T2 loops are folded into `_slashBatch`, with unchanged slashes and events. `tier1Batches` / `tier2Batches` and the ten `t1*`/`t2*` per-aggregator maps are now `internal`, and the new `getAggregatorSubmission` view replaces their getters. This is a view-ABI change; dincli and the bundled ABI were updated with it. +- `registerDINaggregator` gains `onlyCurrentGI`: registering for any GI other than the current one reverts with `TC_WrongGI` (issue #206). diff --git a/foundry/src/DINTaskCoordinator.sol b/foundry/src/DINTaskCoordinator.sol index f746e470..d3675535 100644 --- a/foundry/src/DINTaskCoordinator.sol +++ b/foundry/src/DINTaskCoordinator.sol @@ -398,7 +398,7 @@ contract DINTaskCoordinator is Ownable, ReentrancyGuardTransient { /// @notice Registers the caller as an aggregator for the current GI. /// @dev Caller must be an active validator; duplicate registrations revert. /// @param _GI Current GI index. - function registerDINaggregator(uint _GI) public { + function registerDINaggregator(uint _GI) public onlyCurrentGI(_GI) { if (GIstate != GIstates.DINaggregatorsRegistrationStarted) revert TC_AggregatorsRegistrationNotOpen(); diff --git a/foundry/test/StakingEnforcement.t.sol b/foundry/test/StakingEnforcement.t.sol index 85a8fbed..4f705b30 100644 --- a/foundry/test/StakingEnforcement.t.sol +++ b/foundry/test/StakingEnforcement.t.sol @@ -21,6 +21,7 @@ import {GIstates} from "../src/DINShared.sol"; import { TC_StakeBelowModelFloor, TC_ConcurrentRegistrationCapReached, + TC_WrongGI, TA_StakeBelowModelFloor, TA_ConcurrentRegistrationCapReached } from "../src/DINShared.sol"; @@ -288,6 +289,40 @@ contract StakingEnforcementTest is Test { assertEq(stake.activeRegistrationCount(agg1), 1); } + // ── registerDINaggregator is pinned to the current GI (issue #206) ─────── + + /// @dev Without onlyCurrentGI, a validator could register for GI N+1 during + /// GI N's window and fill its aggregator list before it opened. + function test_registerDINaggregator_futureGI_reverts() public { + _stake(agg1, MIN_STAKE_AMOUNT); + _advanceToAggregatorRegistration(); + vm.prank(agg1); + vm.expectRevert(TC_WrongGI.selector); + tc.registerDINaggregator(2); + assertEq(stake.activeRegistrationCount(agg1), 0); + } + + /// @dev A past GI (here GI 0 while GI 1 is open) would otherwise leak a + /// concurrent-registration slot that releaseGIRegistrationSlots never frees. + function test_registerDINaggregator_pastGI_reverts() public { + _stake(agg1, MIN_STAKE_AMOUNT); + _advanceToAggregatorRegistration(); + vm.prank(agg1); + vm.expectRevert(TC_WrongGI.selector); + tc.registerDINaggregator(0); + assertEq(stake.activeRegistrationCount(agg1), 0); + } + + function test_registerDINaggregator_currentGI_registers() public { + _stake(agg1, MIN_STAKE_AMOUNT); + _advanceToAggregatorRegistration(); + vm.prank(agg1); + tc.registerDINaggregator(1); + assertTrue(tc.isDINAggregator(1, agg1)); + assertFalse(tc.isDINAggregator(2, agg1)); + assertEq(stake.activeRegistrationCount(agg1), 1); + } + function test_activeCount_incrementsOnAuditorRegister() public { _stake(aud1, MIN_STAKE_AMOUNT); _advanceToAuditorRegistration();