fix(task-coordinator): back under EIP-170 with a CI size gate (#201 Part A) + onlyCurrentGI on registerDINaggregator (#206) - #211
Conversation
…d add a CI size gate (InfiniteZeroFoundation#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 <noreply@anthropic.com>
…nfiniteZeroFoundation#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 <noreply@anthropic.com>
|
Reviewed against Claimed: Verified, exact: after a full via_ir build of PR head, both Claimed: the gate fails on unmodified Verified:
Claimed: folding the T1/T2 slash loops into Verified, beyond the PR: no existing test asserts
Each scenario logs every Claimed: the view-ABI change is exactly 12 removed getters plus Verified, exact: I compared ABI entry sets. The bundled
Claimed: Verified, break-then-fix: I removed Claimed: 481 forge tests pass, and pytest passes (309 in a torch-less venv). Verified: Did not hold up: the Minor, optional: Not independently re-verified: the per-change savings breakdown (528 / 360 / 212 B) and the two rejected candidates (5 B / 60 B). Checking them would take one via_ir build per variant, and the total I did reproduce (−1,088 B for commit 1, +9 B for commit 2) is what the gate depends on. Every checkable claim held up except the split §3.2 table in |
Files changed (13) — as of
|
| Field | Value |
|---|---|
| Change | Modified |
| Lines | +109/-83 |
| Diff (what exactly is in this PR) | Commit 1: • Folds slashAggregators' duplicated T1/T2 loops into _slashBatch(_GI, batchId, aggs, finalCID, minStakeAmt, s2Amount, bool t2).• Makes tier1Batches/tier2Batches and the ten t1*/t2* per-aggregator maps internal.• Adds the getAggregatorSubmission(gi, tier, batchId, aggregator) view.• Corrects the stale slashAggregators NatSpec.Commit 2: adds onlyCurrentGI(_GI) to registerDINaggregator. |
| Functionality — how & why | How: slashAggregators still computes minStakeAmt/s2Amount once, then calls _slashBatch per T1 batch and then for the T2 batch. _slashBatch picks the AGG_T{1,2}_* reason literals and the t{1,2}Submitted/t{1,2}SubmissionCID map from t2, then runs the same rules as before:• not revealed → slashPartial(s2Amount), skipped when s2Amount is 0;• revealed CID ≠ finalCID → full slash(minStakeAmt).getAggregatorSubmission branches on TierKind and returns committed/commitHash/submitted/cid, plus the vote count for the aggregator's own CID. onlyCurrentGI reverts TC_WrongGI before any registration state is read. Why: at 24,585 B the contract was 9 B over EIP-170, so nothing on develop could deploy to a real chain (issue No. 201 Part A). It is now 23,497 B. Separately, registerDINaggregator trusted the caller's _GI, so a validator could fill GI N+1's 300-slot aggregator list during GI N's window, or leak a concurrency slot on a released past GI (issue No. 206). |
Diff vs current develop HEAD |
None: untouched since the merge-base. |
| Recommended merge proposal | Merge as-is. Size verified at 23,497 B by both the gate and forge build --sizes. Slash events and stake deltas are identical to develop in all four T1/T2 × no-submission/bad-consensus branches (scratch test, see the verification comment). Break-then-fix: removing onlyCurrentGI fails the future/past-GI tests; ignoring tier in the view fails the T2 view test. |
| Actual merge proposal | Applied as-is, byte-identical to PR head. Size gate: 23,497 B runtime (1,079 B margin, warning band). Evidence on the merged tree: forge clean && forge build (via_ir) clean; size gate exit 0; forge test 481 passed / 0 failed / 0 skipped; pytest -m "not integration" 385 passed / 0 failed. 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 | +54/-358 |
| Diff (what exactly is in this PR) | The bundled ABI, regenerated with dump-abi --official. |
| Functionality — how & why | How: dincli falls back to this ABI when a manifest supplies no custom task_contracts. Why: without it, the six moved call sites would raise AttributeError on getAggregatorSubmission. |
Diff vs current develop HEAD |
None |
| Recommended merge proposal | Merge as-is. The ABI is identical (as a set) to the via_ir-built foundry/out artifact: exactly 12 getters removed and getAggregatorSubmission added. |
| Actual merge proposal | Applied as-is. dump-abi --official from the merged via_ir build reproduces it byte-for-byte. Evidence on the merged tree: forge clean && forge build (via_ir) clean; size gate exit 0; forge test 481 passed / 0 failed / 0 skipped; pytest -m "not integration" 385 passed / 0 failed. Uncommitted in the develop cwd, not committed or pushed. |
| Pending proposal | None |
| Local merge conflict | No |
| GitHub merge conflict | No |
dincli/cli/aggregator.py, dincli/cli/modelownerd/aggregation.py, tests/test_aggregator_commit_retry.py
| Field | Value |
|---|---|
| Change | Modified (all three) |
| Lines | +4/-4, +2/-2, +3/-2 |
| Diff (what exactly is in this PR) | The 4 + 2 call sites of t{1,2}SubmissionCID / t{1,2}Committed move to getAggregatorSubmission(...).call()[3] (cid) or [0] (committed). The retry-test mock answers getAggregatorSubmission with a 5-tuple. |
| Functionality — how & why | How: same reads, through the new view: • show-t1/t2-batches show the per-validator revealed CID;• aggregate-t1/t2's retry guard skips a batch already committed on chain, so the salt behind an existing commitment is never replaced.Why: the old getters no longer exist on the contract. |
Diff vs current develop HEAD |
None |
| Recommended merge proposal | Merge as-is. pytest -m "not integration" with torch: 385 passed. Optional nit: aggregation.py passes literal 0/1 where aggregator.py already has TIER1/TIER2. |
| Actual merge proposal | All three applied as-is, byte-identical to PR head. Evidence on the merged tree: forge clean && forge build (via_ir) clean; size gate exit 0; forge test 481 passed / 0 failed / 0 skipped; pytest -m "not integration" 385 passed / 0 failed. Uncommitted in the develop cwd, not committed or pushed. |
| Pending proposal | None |
| Local merge conflict | No |
| GitHub merge conflict | No |
foundry/test/AggregatorCommitReveal.t.sol, foundry/test/StakingEnforcement.t.sol
| Field | Value |
|---|---|
| Change | Modified (both) |
| Lines | +71/-4, +35/-0 |
| Diff (what exactly is in this PR) | AggregatorCommitReveal:• moves 2 existing assertions to the new view; • adds test_getAggregatorSubmission_t1_commitThenReveal / _t2_commitThenReveal, covering commit-only state, then revealed CID + votes, a never-committed aggregator, and the tier parameter being honoured.StakingEnforcement adds future / past / current GI tests for registerDINaggregator. |
| Functionality — how & why | How: the new tests read the view before and after reveal. The GI tests expectRevert(TC_WrongGI) and assert activeRegistrationCount is unchanged. Why: they pin the new view's semantics and the No. 206 fix. |
Diff vs current develop HEAD |
None |
| Recommended merge proposal | Merge as-is. The full suite gives 481/481. Break-then-fix: removing onlyCurrentGI fails the future/past-GI tests; ignoring tier in the view fails the T2 view test. |
| Actual merge proposal | Both applied as-is, byte-identical to PR head; their new tests pass in the full run. Evidence on the merged tree: forge clean && forge build (via_ir) clean; size gate exit 0; forge test 481 passed / 0 failed / 0 skipped; pytest -m "not integration" 385 passed / 0 failed. Uncommitted in the develop cwd, not committed or pushed. |
| Pending proposal | None |
| Local merge conflict | No |
| GitHub merge conflict | No |
.github/scripts/contract_size_gate.py, .github/workflows/ci.yml
| Field | Value |
|---|---|
| Change | New / Modified |
| Lines | +111/-0, +7/-0 |
| Diff (what exactly is in this PR) | A new gate script, plus a CI step between forge build and forge test. |
| Functionality — how & why | How: the script globs out/*.sol/*.json and keeps artifacts whose metadata.settings.compilationTarget is under src/ and whose deployedBytecode is non-empty. Unlinked-library placeholders count as 20 B. It fails below a 1,024 B runtime margin or above 49,152 B initcode, warns below 2,048 B, and fails closed with no artifacts. Why: nothing checked contract size before, and anvil.sh lifts the limit, which is how the 9 B overrun reached develop. It reads artifacts because forge build --sizes leaves DINTaskAuditor out. |
Diff vs current develop HEAD |
None |
| Recommended merge proposal | Merge as-is. Exit 1 with ::error … -9 B on develop's src, exit 0 with 2 warnings on PR head. The PR's own CI run shows the same output. |
| Actual merge proposal | Applied as-is (new script copied; ci.yml patch applied), byte-identical to PR head. On the merged build the gate exits 0 with 2 warnings (DINTaskCoordinator 1,079 B, DINTaskAuditor 2,009 B). Evidence on the merged tree: forge clean && forge build (via_ir) clean; size gate exit 0; forge test 481 passed / 0 failed / 0 skipped; pytest -m "not integration" 385 passed / 0 failed. Uncommitted in the develop cwd, not committed or pushed. |
| Pending proposal | None |
| Local merge conflict | No |
| GitHub merge conflict | No |
Documentation/technical/contracts/DINTaskCoordinator.md
| Field | Value |
|---|---|
| Change | Modified |
| Lines | +9/-5 |
| Diff (what exactly is in this PR) | Documents the following: • §3.2: the internal maps and getAggregatorSubmission;• §6.2: onlyCurrentGI;• §10 No. 1: from "over EIP-170" to "little room; CI gate"; • §10 No. 7: the slashAggregators NatSpec item dropped;• §10 No. 11: marked fixed; • two Change Log entries. |
| Functionality — how & why | How / why: keeps the code-reader doc in line with develop, per the Documentation/ convention. |
Diff vs current develop HEAD |
None |
| Recommended merge proposal | Needs a fix. The new §3.2 paragraph sits between two table rows, so the following five rows (tier1FinalizedAt … aggSeedBlock) render as a paragraph instead of table rows. GitHub's GFM renderer outputs 7 <tr> instead of 12. Move the paragraph below the table's last row. |
| Actual merge proposal | Applied, plus the pending fix: the §3.2 "The batch arrays and the ten t1*/t2* maps are internal…" paragraph now sits below the table's last row (aggSeedBlock) instead of mid-table. Text unchanged; the only difference from PR head is the moved paragraph. GitHub's GFM renderer now gives 12 <tr> (was 7). check_doc_links.py Documentation: all 258 links resolve. Uncommitted in the develop cwd, not committed or pushed. |
| Pending proposal | |
| Local merge conflict | No |
| GitHub merge conflict | No |
Developer/CONTRIBUTING.md, Documentation/technical/testing/dincli-testing-guide.md, foundry/anvil.sh
| Field | Value |
|---|---|
| Change | Modified (all three) |
| Lines | +5/-0, +1/-1, +4/-1 |
| Diff (what exactly is in this PR) | Records the size budget and the local check command in CONTRIBUTING. Rewords the testing-guide warning so it no longer says the coordinator is over the limit. Turns anvil.sh's bare commented flag into an explanation. |
| Functionality — how & why | How / why: tells contributors where the size line is enforced, now that the local anvil chain won't catch it. Comment-only in anvil.sh; the anvil flags are unchanged. |
Diff vs current develop HEAD |
None |
| Recommended merge proposal | Merge as-is. check_doc_links.py passes in CI (Docs job green). |
| Actual merge proposal | All three applied as-is, byte-identical to PR head. check_doc_links.py on Documentation (258) and Developer (242): all resolve. Evidence on the merged tree: forge clean && forge build (via_ir) clean; size gate exit 0; forge test 481 passed / 0 failed / 0 skipped; pytest -m "not integration" 385 passed / 0 failed. Uncommitted in the develop cwd, not committed or pushed. |
| Pending proposal | None |
| Local merge conflict | No |
| GitHub merge conflict | No |
Verification
All on the real via_ir profile:
forge test: 481 passed / 0 failed (39 suites).pytest -m "not integration"with torch: 385 passed.- Size gate: green with 2 warnings on PR head, red (−9 B) on
develop. - Slash-event equivalence vs
develop: identical. - Break-then-fix on the No. 206 guard and the view's tier branch.
- Bundled ABI: equal to the built artifact.
Full detail is in the verification comment above.
Local vs. GitHub agree: both report a clean merge with no conflicts.
Actual outcome — PR No. 211 merged + review fix applied (pushed)This supersedes the pre-merge review comment above with what actually happened when landing this PR on Two commits, both on
Files unchanged from the PR (12 of 13)
Files changed by
|
| File | What changed vs. this PR's merged version |
|---|---|
Documentation/technical/contracts/DINTaskCoordinator.md |
The §3.2 paragraph on the internal maps and getAggregatorSubmission moved from between two table rows to below the table's last row (aggSeedBlock). The text is unchanged. GitHub's GFM renderer now gives all 12 table rows (it was 7). |
Why the change
It's a review finding in this PR's own diff, not develop drift. The paragraph split the §3.2 table, so its last five rows rendered as plain text (see the verification comment).
Verification
On the pushed tree, 420d48d:
forge clean && forge build(via_ir) is clean.contract_size_gate.pyexits 0.DINTaskCoordinatoris 23,497 B (1,079 B margin) andDINTaskAuditor22,567 B (2,009 B), both in the warning band.forge test: 481 passed, 0 failed.pytest -m "not integration"with an empty HOME: 385 passed, 0 failed.dump-abi --officialfrom the merged build reproduces the bundled ABI byte-for-byte.- Doc links in
Documentation(258) andDeveloper(242) all resolve. - The local CI mirror on
420d48dpassed: docs, pytest, via_ir build, forge test and hardhat. - GitHub
pushrun ondevelop: green, including the newcontract size gatestep.
Local vs. GitHub agree: clean merge with no conflicts, as both predicted. Issue No. 206 is fixed by this PR. Issue No. 201 stays open for Part B (the slash reason for committed-but-unrevealed).
2 participants
Summary
PR 1 of task_021026_19, Parts A + B. It has two commits, per PR #209 Decision 2:
c96a063(Part A of Contract size headroom for all contracts (DINTaskCoordinator −9 B over EIP-170 with PR No. 197) + slash reason for committed-but-unrevealed across commit-reveal flows #201). BringsDINTaskCoordinatorback under EIP-170 inside the same contract, and adds a CI size gate. No state-changing function, event or storage slot changes.c3e585d(security: registerDINaggregator has no onlyCurrentGI — validators can pre-register for (and fill) a future GI's aggregator list #206). AddsonlyCurrentGItoregisterDINaggregator.Closes #206. This is Part A of #201; #201 Part B (the slash reason for committed-but-unrevealed) stays open.
Sizes
Runtime bytes,
via_ir, 200 runs, all from one clean build:DINTaskCoordinatordevelop@3208e89What each change saves, measured by removing it from the final commit-1 build:
slashAggregators' duplicated T1/T2 loops into_slashBatch(..., bool t2)t1*/t2*per-aggregator mapsinternal, plus a newgetAggregatorSubmissionviewtier1Batches/tier2Batchesauto-gettersinternal(getTier1Batch/getTier2Batchremain)The two extra candidates were measured as asked and not taken: a
FinalizedAtview saves only 5 B, and merging the tier-duplicated errors 60 B.DINTaskAuditoris unchanged at 22,567 B (2,009 B margin, in the warning band).Size gate (
.github/scripts/contract_size_gate.py)foundry/outartifact whose compilation target is undersrc/and whosedeployedBytecodeis non-empty. That's all 14 deployable contracts, includingDINTaskAuditor, whichforge build --sizesleaves out.forge build.develop:::error … DINTaskCoordinator runtime 24585 B leaves -9 B under EIP-170and exit 1.::warningforDINTaskCoordinator(1,079 B) andDINTaskAuditor(2,009 B).The thresholds are recorded in
CONTRIBUTING.md, andanvil.shnow explains that it lifts the limit locally.View-ABI change (Decision 4)
getAggregatorSubmission(gi, tier, batchId, aggregator)returns(committed, commitHash, submitted, cid, votes).votesis the vote count for the aggregator's own revealed CID, because the vote maps are keyed by CID, and it's 0 before the reveal. Tests cover both tiers, before and after reveal.dincli/cli/aggregator.py(4 sites) anddincli/cli/modelownerd/aggregation.py(2 sites)tests/test_aggregator_commit_retry.pyAggregatorCommitReveal.t.soldincli/abis/DINTaskCoordinator.jsonwas regenerated withdump-abi --official. The diff is exactly the 12 removed getters plusgetAggregatorSubmission.Also
slashAggregatorsNatSpec: no submission is slashed at the S2 fraction, not the fullminStake(). That removes it fromDINTaskCoordinator.md§10 No. 7.DINTaskCoordinator.sol(3 before, 3 after). An earlierbytes32("…")-ternary version added 4unsafe-typecastwarnings, so I replaced it with implicit literal assignment.DINTaskCoordinator.md§3.2 describes the internal maps and the new view; §6.2 now statesonlyCurrentGI.dincli-testing-guide.mdis updated too.Verification
forge clean && forge build && contract_size_gate.py && forge test: 481 passed, 0 failed (39 suites, includingUpgradeValidation.t.sol). Commit 1 alone gave 478/478.pytest -m "not integration": 309 passed. The torch-dependent files (test_cache_client_dp.py,test_resampling_policy.py,test_scoring.py) and the 2test_integration_markerchecks that collect them can't run in my local venv (no torch); unmodifieddevelopbehaves the same there. CI installs the full deps.check_doc_links.py Documentation/Developer: all links resolve.🤖 Generated with Claude Code