gas: redundant SLOADs/external calls in the highest-traffic task-contract and staking paths

Summary

Five safe, behavior-preserving gas optimizations found during a fresh pass over foundry/src/ (checked against develop @ 6e361cb), all in the repo's highest-call-frequency paths (every registration, every GI finalize/slash, every stake/unstake/slash). None change the public ABI, storage layout, or any revert/event behavior — each just caches a value that's already being read/computed and reuses it instead of re-reading. Batched into one issue since they're the same category of fix (cache-before-reuse) repeated across a few files, rather than filing five near-identical issues.

This followed a scan explicitly looking for a different angle than the security/correctness review already done on this codebase (C-1's gas-DoS, the EIP-170 size-reduction passes) — this is runtime gas-per-call, not bytecode size or unbounded-loop risk. No new unbounded-loop gas-DoS was found beyond the already-tracked C-1/BL-10 class.

Findings

1. Duplicate `getStake(msg.sender)` call in every aggregator/auditor registration

foundry/src/DINTaskCoordinator.sol:414 and :422 (registerDINaggregator), foundry/src/DINTaskAuditor.sol (registerDINAuditor, identical shape) — dinvalidatorStakeContract.getStake(msg.sender) is called once for the per-model stake-floor check and again for the concurrent-registration-cap check:

uint256 floorMin = dinvalidatorStakeContract.getModelStakeMin(modelId);
if (floorMin > 0 && dinvalidatorStakeContract.getStake(msg.sender) < floorMin)   // call #1
    revert TC_StakeBelowModelFloor();

uint256 capPerUnit = dinvalidatorStakeContract.maxConcurrentRegistrationsPerStakeUnit();
if (capPerUnit > 0) {
    uint256 maxAllowed = (dinvalidatorStakeContract.getStake(msg.sender) /   // call #2, same value
        dinvalidatorStakeContract.minStake()) * capPerUnit;
    ...

Fix: read uint256 stake = dinvalidatorStakeContract.getStake(msg.sender); once, reuse in both branches. Saves a full external STATICCALL (~150-700 gas) on every registration once both DAO knobs are non-zero (both default to 0 today, so the saving is currently latent, but the fix is free and unconditionally safe to apply now).

2. `_activeAggregatorPool`/`_activeAuditorPool` call `isValidatorActive` twice per registrant

foundry/src/DINTaskCoordinator.sol:692-714, foundry/src/DINTaskAuditor.sol:793-815 — the classic "count then fill" two-pass pattern for sizing a memory array re-queries isValidatorActive(registeredPool[i]) for the same address in both passes:

for (uint i = 0; i < registeredPool.length; i++) {
    if (dinvalidatorStakeContract.isValidatorActive(registeredPool[i])) { activeCount++; }
}
activePool = new address[](activeCount);
uint ptr;
for (uint i = 0; i < registeredPool.length; i++) {
    address validator = registeredPool[i];
    if (dinvalidatorStakeContract.isValidatorActive(validator)) { activePool[ptr++] = validator; }  // re-queries
}

Fix: store each result in a bool[] memory active during the first pass, read from that in the second instead of re-calling. Saves one external CALL per active registrant — called once per GI by autoCreateTier1AndTier2/createAuditorsBatches, over up to 300 registrants (MAX_REGISTERED_AGGREGATORS/MAX_REGISTERED_AUDITORS), so potentially tens of thousands of gas per GI at scale.

3. `array.length`/struct-field re-read every loop iteration in the finalize/slash functions

Several spots re-read a storage array's .length or a struct field every iteration of a loop where neither changes during the loop:

DINTaskCoordinator.sol: finalizeT1Aggregation (batches.length at the outer loop, b.aggregators.length + b.batchId re-read in the inner loop), finalizeT2Aggregation (same shape), slashAggregators (t1batches.length/t2batches.length)
DINTaskAuditor.sol: finalizeEvaluation (batch.modelIndexes.length/batch.auditors.length re-read per inner iteration, plus twice more for two new array allocations sized from the same field), slashAuditors (batch.auditors.length/batch.modelIndexes.length re-read per inner iteration)

Example (finalizeT1Aggregation):

for (uint i = 0; i < batches.length; i++) {
    Tier1Batch storage b = batches[i];
    for (uint j = 0; j < b.aggregators.length; j++) {        // storage SLOAD every iteration
        address aggregator = b.aggregators[j];
        if (t1Submitted[_GI][b.batchId][aggregator]) {        // b.batchId re-read multiple times too

Fix: cache uint256 aggLen = b.aggregators.length; and uint256 batchId = b.batchId; once per outer iteration, and the outer .length once before the loop. Each avoided re-read is a warm SLOAD (~100 gas); for a 3-aggregator batch, b.batchId alone is read ~9x instead of once. These are exactly the functions C-1 already flagged for O(n) asymptotic cost (pagination is still the real fix for that) — this doesn't change the shape, just shaves a constant factor off every call today, called every single GI.

4. `_syncValidatorStatus` reads `status` and `activeStake` twice each

foundry/src/DinValidatorStake.sol:716-737 — validator.status is checked twice (both reads of the same packed slot, no intervening write) and validator.activeStake is read twice in a mutually-exclusive else if chain:

function _syncValidatorStatus(ValidatorInfo storage validator) internal {
    if (validator.status == ValidatorStatus.Blacklisted) { return; }                      // read #1
    if (validator.status == ValidatorStatus.Jailed && validator.jailedUntil > block.timestamp) { return; }  // read #2
    if (validator.pendingWithdrawals > 0) { validator.status = ValidatorStatus.Exiting; }
    else if (validator.activeStake >= MIN_STAKE) { validator.status = ValidatorStatus.Active; }  // read #1
    else if (validator.activeStake > 0) { validator.status = ValidatorStatus.Exiting; }          // read #2
    else { validator.status = ValidatorStatus.None; }
}

Fix: cache both into locals at the top, reuse. This is the most-called internal helper in the contract — invoked from stake, unstake, claimUnstaked, reactivate, unblacklistValidator, and (via _applySlash) every slash/slashPartial/recordNoParticipation call. ~100 gas (one warm SLOAD) per avoided extra read, on every stake and every slash.

5. One stray `require(..., "string")` instead of a custom error

foundry/src/DINTaskCoordinator.sol:1290-1291 (releaseGIRegistrationSlots) — every other revert path in both task contracts uses a custom error; this function is the one holdout:

require(giEnded, "GI has not ended");
require(!registrationSlotsReleased[_GI], "slots already released");

Fix: add TC_GINotEnded()/TC_SlotsAlreadyReleased() custom errors and swap. Small (string-revert overhead only hits the revert path, not the happy path), but free and consistent with the rest of the file.

Considered, not recommending without a decision

Three struct-packing opportunities (DINTaskAuditor.LMSubmission, DinEmission.EmissionState, DINModelRegistry.ModelRequest) could save a storage slot each by reordering/shrinking fields, but all three structs sit behind a public array/mapping whose auto-generated getter encodes fields in declared order (or type, for EmissionState) — so the fix would change the public ABI's output shape, not just internal layout. LMSubmission is the highest-value one (called every submitLocalModel, every client, every GI — a full SSTORE's worth, ~20k+ gas, if it drops from 4 slots to 3), but making lmSubmissions internal and hand-writing a reassembling getter is real added complexity. Not filing as actionable work; noting here in case a maintainer wants to make that call deliberately.

Recommendation

All five numbered findings are mechanical, zero-behavior-change read-caching — safe to implement directly. No storage layout, ABI, event, or revert-selector changes beyond #5 (which only changes the revert error type, already the norm everywhere else in these files).

Approval review: Approved with amendments

Verified against origin/develop @ 6e361cb.

Claims

Claim	Result	Evidence
1. `getStake(msg.sender)` called twice in `registerDINaggregator` / `registerDINAuditor`	✅ (line ref ⚠️ off by one)	`DINTaskCoordinator.sol:415` and `:422` (`:414` is the `getModelStakeMin` read); `DINTaskAuditor.sol:692`, `:698`
2. `_activeAggregatorPool` / `_activeAuditorPool` re-query `isValidatorActive` in the fill pass	✅	`DINTaskCoordinator.sol:692-714`, `DINTaskAuditor.sol:793`; sole callers `TC:591`, `TA:880`
3. `.length` / `b.batchId` re-read per iteration in the finalize/slash loops	✅	`finalizeT1Aggregation` inner loop reads `b.batchId` in `t1Submitted`/`t1SubmissionCID`/`t1Votes`; `slashAggregators` `TC:1167`, `:1172`; `finalizeEvaluation` `TA:1232-1257`; `slashAuditors` `TA:1358`, `:1363`
4. `_syncValidatorStatus` reads `status` and `activeStake` twice	✅	`DinValidatorStake.sol:716-737`; called from 6 sites (`:225`, `:376`, `:401`, `:432`, `:536`, `:651`)
5. `releaseGIRegistrationSlots` is the one string-`require` holdout	✅	`DINTaskCoordinator.sol:1290-1291`; no `require(` anywhere in `DINTaskAuditor.sol`
No ABI/storage/event change beyond finding 5	✅	All are local-variable caching. Finding 5 adds two error selectors to the ABI
Per-finding gas figures ("~150-700", "tens of thousands per GI")	⚠️	Estimates, not measured. Fine for an issue, but the PR should show measured numbers

Merit

Worth doing. Each change is mechanical caching, and four of the five sit on paths that run every GI or every stake/slash. Tracing it further turned up three things the issue doesn't cover. First, DINTaskCoordinator is already over EIP-170 on develop (No. 201 Part A blocks any deploy). Gas caching can move bytecode size either way: the bool[] memory second pass in finding 2 adds code, while finding 5 removes two revert strings. Second, finding 5 breaks an existing test that matches the string. Third, open PR No. 253 removes both callers of the finding-2 helpers.

Amendments

Size gate. Post forge build --sizes (via_ir profile) for DINTaskCoordinator and DINTaskAuditor before/after in the PR (--sizes omits DINTaskAuditor; read its size from out/ deployedBytecode). DINTaskCoordinator must not grow. If a finding increases its size, drop that finding for the coordinator.
Finding 2 vs PR No. 253. No. 253 (position-stable seat assignment) replaces both _activeAggregatorPool(_GI) and _activeAuditorPool(_GI) calls. If No. 253 merges first, skip finding 2. If any helper is left with no callers, delete it instead of optimising it.
Finding 5. Declare TC_GINotEnded / TC_SlotsAlreadyReleased in foundry/src/DINShared.sol with the other TC_* errors. Update foundry/test/StakingEnforcement.t.sol:485 (vm.expectRevert(bytes("slots already released"))) to the selector, and add a test for the not-ended path if none exists. Regenerate dincli/abis/DINTaskCoordinator.json.
Evidence. Include a forge snapshot --diff (or --gas-report before/after) covering registration, finalizeT1Aggregation, finalizeEvaluation, slashAuditors, and stake/slash, so the savings are measured rather than estimated.
Leave the struct-packing items out, as the issue already says. They change getter output shape, and storage layout is still a running document (Documentation/technical/storage_layout.md).

Conflicts

PR No. 253: DINTaskAuditor.sol, DINTaskCoordinator.sol (same batch-creation functions; see amendment 2)
PR No. 236: DinValidatorStake.sol
PR No. 218: DINTaskAuditor.sol, DinValidatorStake.sol, StakingEnforcement.t.sol
PR No. 72, No. 31, No. 32 (long-lived): touch the same contracts/test

gas(DINTaskCoordinator): cache batchId/aggregators.length in finalize…

@robertocarlous

Done by PR No. 259, merged into develop as 1d3b484. Finding 5 landed earlier, through PR No. 287 (e62713f). Closes #249 closed this issue automatically at merge, so this is the peer check that MAINTAINING No. 5.2 requires afterwards. CI on the develop push: run 42258275515, Solidity forge test 526 passed, 0 failed, size gate green.

Checked on origin/develop @ 1d3b484:

Finding 1 (single getStake): registerDINaggregator and registerDINAuditor each read uint256 stake = dinvalidatorStakeContract.getStake(msg.sender) once, inside if (floorMin > 0 || capPerUnit > 0), and use it for both the floor check and the cap check.
Finding 2 (isValidatorActive once per registrant): _activeAggregatorPool (DINTaskCoordinator.sol:699) and _activeAuditorPool (DINTaskAuditor.sol:794) store the counting pass's results in a bool[] memory active and fill from it.
Finding 3 (loop invariants): finalizeT1Aggregation and finalizeT2Aggregation cache batches.length, b.batchId and b.aggregators.length. slashAggregators caches t1batches.length and t2batches.length. finalizeEvaluation and slashAuditors cache auditBatches[_GI].length, batch.modelIndexes.length and batch.auditors.length, and the votedScores/votedAuditors allocations use the cached count.
Finding 4 (_syncValidatorStatus): DinValidatorStake.sol reads status and activeStake into locals once each.
Finding 5 (custom errors): releaseGIRegistrationSlots reverts with TC_GINotEnded() and TC_SlotsAlreadyReleased() (DINTaskCoordinator.sol:1313-1314). There's no require( left in either task contract.
Amendment 1 (size gate): DINTaskCoordinator didn't grow. CI's size gate on 1d3b484 reports 23,393 B (1,183 B margin), down from 23,507 B on ae7e8d6, and it's still warn, not fail. DINTaskAuditor is 21,319 B (ok).
Amendment 2 (vs PR No. 253): PR No. 253 is still open, so finding 2 applies. Both helpers still have callers (DINTaskCoordinator.sol:598, DINTaskAuditor.sol:884). Whichever of the two lands second deletes the helpers, as already asked on PR No. 253.
Amendment 3 (finding 5 details): both errors are declared in foundry/src/DINShared.sol:315,317. StakingEnforcement.t.sol:489 now expects TC_SlotsAlreadyReleased.selector, and :499 adds the not-ended case with TC_GINotEnded.selector. dincli/abis/DINTaskCoordinator.json contains both errors (:2437, :2582).
Amendment 4 (measured gas): the PR body has a before/after forge test --gas-report table for finalizeT1Aggregation, finalizeT2Aggregation, slashAggregators, slashAuditors, releaseGIRegistrationSlots, registration and stake/slash. Registration and stake/slash measure flat. The existing suites never enable both registration knobs, and via-IR may already merge the duplicate reads. finalizeEvaluation has no row. I flagged that as non-blocking in the Nov 5 re-review of PR No. 259, and it's accepted as is. The change is plain caching of loop invariants, verified above.
Amendment 5 (no struct packing): PR No. 259 changes no struct definition (git diff 1207bf9 1d3b484 -- foundry/src has no struct lines).

The struct-packing options from "Considered, not recommending" remain unfiled, as both the issue and amendment 5 intended. The PR merged without the peer approval that MAINTAINING No. 4.2 requires. That's recorded on PR No. 259, where @robertocarlous is doing the review after the fact. If that review turns up a problem, I'll reopen this issue.

gas(contracts): cache repeated storage/external reads in hot paths (i…

Navigation Menu

Repository navigation

gas: redundant SLOADs/external calls in the highest-traffic task-contract and staking paths #249

Summary

Findings

1. Duplicate `getStake(msg.sender)` call in every aggregator/auditor registration

2. `_activeAggregatorPool`/`_activeAuditorPool` call `isValidatorActive` twice per registrant

3. `array.length`/struct-field re-read every loop iteration in the finalize/slash functions

4. `_syncValidatorStatus` reads `status` and `activeStake` twice each

5. One stray `require(..., "string")` instead of a custom error

Considered, not recommending without a decision

Recommendation

Approval review: Approved with amendments

Claims

Merit

Amendments

Conflicts

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

gas: redundant SLOADs/external calls in the highest-traffic task-contract and staking paths #249

Description

Summary

Findings

1. Duplicate getStake(msg.sender) call in every aggregator/auditor registration

2. _activeAggregatorPool/_activeAuditorPool call isValidatorActive twice per registrant

3. array.length/struct-field re-read every loop iteration in the finalize/slash functions

4. _syncValidatorStatus reads status and activeStake twice each

5. One stray require(..., "string") instead of a custom error

Considered, not recommending without a decision

Recommendation

Activity

umeradl commented on Oct 15, 2026

Approval review: Approved with amendments

Claims

Merit

Amendments

Conflicts

umeradl commented on Nov 6, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

1. Duplicate `getStake(msg.sender)` call in every aggregator/auditor registration

2. `_activeAggregatorPool`/`_activeAuditorPool` call `isValidatorActive` twice per registrant

3. `array.length`/struct-field re-read every loop iteration in the finalize/slash functions

4. `_syncValidatorStatus` reads `status` and `activeStake` twice each

5. One stray `require(..., "string")` instead of a custom error