From 283c66fcabb0b03f24d126e91398468e1d654b24 Mon Sep 17 00:00:00 2001 From: Abidoyesimze Date: Thu, 15 Oct 2026 10:47:08 +0100 Subject: [PATCH 1/2] gas(contracts): cache repeated storage/external reads in hot paths (issue #249) Mechanical caching across validator registration, finalize/slash loops, and validator-status sync, plus two string requires converted to custom errors: - registerDINaggregator/registerDINAuditor: share a single getStake() read between the floor and cap checks instead of up to two. - _activeAggregatorPool/_activeAuditorPool: cache each isValidatorActive() result from the counting pass for reuse in the fill pass, instead of re-querying it. - finalizeT1Aggregation, slashAggregators/_slashBatch, finalizeEvaluation, slashAuditors: cache batchId/array lengths that are loop-invariant but were re-read from storage every iteration. - DinValidatorStake._syncValidatorStatus: cache status/activeStake instead of reading each twice. - releaseGIRegistrationSlots: TC_GINotEnded/TC_SlotsAlreadyReleased custom errors replace the two string requires (adds no bytecode elsewhere since it's a one-off path); updated the double-release test and added a not-ended-revert test; regenerated the DINTaskCoordinator ABI. DINTaskCoordinator runtime size shrinks (23,497 -> 23,302 B; margin 1,079 -> 1,274 B under EIP-170) despite finding 2's extra bool[] pass, since the custom errors and other caching outweigh it. DINTaskAuditor isn't under the same size pressure and grows by 22 B (21,290 -> 21,312 B; margin 3,264 B, well above the 2,048 B warn line). Co-Authored-By: Claude Sonnet 5 --- dincli/abis/DINTaskCoordinator.json | 10 ++++ foundry/src/DINShared.sol | 5 ++ foundry/src/DINTaskAuditor.sol | 65 ++++++++++++++---------- foundry/src/DINTaskCoordinator.sol | 73 ++++++++++++++++----------- foundry/src/DinValidatorStake.sol | 12 +++-- foundry/test/StakingEnforcement.t.sol | 13 ++++- 6 files changed, 118 insertions(+), 60 deletions(-) diff --git a/dincli/abis/DINTaskCoordinator.json b/dincli/abis/DINTaskCoordinator.json index 20af9c1e..ce67af33 100644 --- a/dincli/abis/DINTaskCoordinator.json +++ b/dincli/abis/DINTaskCoordinator.json @@ -2437,6 +2437,11 @@ "name": "TC_GICannotBeStarted", "inputs": [] }, + { + "type": "error", + "name": "TC_GINotEnded", + "inputs": [] + }, { "type": "error", "name": "TC_GIRewardPoolNotFunded", @@ -2577,6 +2582,11 @@ "name": "TC_RevealCannotBeStarted", "inputs": [] }, + { + "type": "error", + "name": "TC_SlotsAlreadyReleased", + "inputs": [] + }, { "type": "error", "name": "TC_StakeBelowModelFloor", diff --git a/foundry/src/DINShared.sol b/foundry/src/DINShared.sol index a27f1e81..d00174bc 100644 --- a/foundry/src/DINShared.sol +++ b/foundry/src/DINShared.sol @@ -349,6 +349,11 @@ error TC_AggregatorNotActive(); error TC_FailedToSlashAuditors(); /// @dev The upcoming GI's reward pool has not been funded via depositRewards. error TC_GIRewardPoolNotFunded(); +/// @dev releaseGIRegistrationSlots was called for a GI that has not ended yet +/// (not < current GI, and not == current GI with state GIended). +error TC_GINotEnded(); +/// @dev releaseGIRegistrationSlots was already called for this GI (idempotent guard). +error TC_SlotsAlreadyReleased(); // ───────────────────────────────────────────────────────────────────────────── // Custom errors — rewards (task_210726_6 §3, DINTaskAuditor) diff --git a/foundry/src/DINTaskAuditor.sol b/foundry/src/DINTaskAuditor.sol index 342222a0..dd16c818 100644 --- a/foundry/src/DINTaskAuditor.sol +++ b/foundry/src/DINTaskAuditor.sol @@ -689,16 +689,19 @@ contract DINTaskAuditor is Ownable, ReentrancyGuardTransient { // Per-model stake floor: enforced when the model owner has set a non-zero bound. uint256 floorMin = dinvalidatorStakeContract.getModelStakeMin(modelId); - if (floorMin > 0 && dinvalidatorStakeContract.getStake(msg.sender) < floorMin) - revert TA_StakeBelowModelFloor(); - // Concurrent-registration cap: enforced when the DAO has set a non-zero value. uint256 capPerUnit = dinvalidatorStakeContract.maxConcurrentRegistrationsPerStakeUnit(); - if (capPerUnit > 0) { - uint256 maxAllowed = (dinvalidatorStakeContract.getStake(msg.sender) / - dinvalidatorStakeContract.minStake()) * capPerUnit; - if (dinvalidatorStakeContract.activeRegistrationCount(msg.sender) >= maxAllowed) - revert TA_ConcurrentRegistrationCapReached(); + if (floorMin > 0 || capPerUnit > 0) { + // Single getStake() read shared by both checks below (issue #249 finding 1). + uint256 stake = dinvalidatorStakeContract.getStake(msg.sender); + if (floorMin > 0 && stake < floorMin) + revert TA_StakeBelowModelFloor(); + if (capPerUnit > 0) { + uint256 maxAllowed = (stake / + dinvalidatorStakeContract.minStake()) * capPerUnit; + if (dinvalidatorStakeContract.activeRegistrationCount(msg.sender) >= maxAllowed) + revert TA_ConcurrentRegistrationCapReached(); + } } dinAuditors[_GI].push(msg.sender); @@ -794,22 +797,25 @@ contract DINTaskAuditor is Ownable, ReentrancyGuardTransient { uint _GI ) internal view returns (address[] memory activePool) { address[] storage registeredPool = dinAuditors[_GI]; + uint len = registeredPool.length; + // issue #249 finding 2: cache each isValidatorActive result from the + // counting pass instead of re-querying it in the fill pass below. + bool[] memory active = new bool[](len); uint activeCount; - for (uint i = 0; i < registeredPool.length; i++) { - if ( - dinvalidatorStakeContract.isValidatorActive(registeredPool[i]) - ) { + for (uint i = 0; i < len; i++) { + bool isActive = dinvalidatorStakeContract.isValidatorActive(registeredPool[i]); + active[i] = isActive; + if (isActive) { activeCount++; } } activePool = new address[](activeCount); uint ptr; - for (uint i = 0; i < registeredPool.length; i++) { - address auditor = registeredPool[i]; - if (dinvalidatorStakeContract.isValidatorActive(auditor)) { - activePool[ptr++] = auditor; + for (uint i = 0; i < len; i++) { + if (active[i]) { + activePool[ptr++] = registeredPool[i]; } } } @@ -1227,9 +1233,14 @@ contract DINTaskAuditor is Ownable, ReentrancyGuardTransient { for (uint b = 0; b < batches; b++) { AuditBatch storage batch = auditBatches[_GI][b]; + // issue #249 finding 3: both lengths are loop-invariant per + // batch; cache instead of re-reading storage on every iteration + // (modelIndexes.length) or per-model (auditors.length x3). + uint modelIndexCount = batch.modelIndexes.length; + uint auditorCount = batch.auditors.length; // For each model assigned to this batch - for (uint m = 0; m < batch.modelIndexes.length; m++) { + for (uint m = 0; m < modelIndexCount; m++) { uint modelIndex = batch.modelIndexes[m]; if (modelIndex >= submissions.length) { // Defensive: skip if bad index @@ -1246,15 +1257,11 @@ contract DINTaskAuditor is Ownable, ReentrancyGuardTransient { // Collect scores from auditors who actually voted. Bounded by // batch.auditors.length (params.auditorsPerBatch, demo: 3, // spec: up to 10), so an in-memory sort here is cheap. - uint256[] memory votedScores = new uint256[]( - batch.auditors.length - ); - address[] memory votedAuditors = new address[]( - batch.auditors.length - ); + uint256[] memory votedScores = new uint256[](auditorCount); + address[] memory votedAuditors = new address[](auditorCount); uint votes; - for (uint a = 0; a < batch.auditors.length; a++) { + for (uint a = 0; a < auditorCount; a++) { address auditor = batch.auditors[a]; if (hasAuditedLM[_GI][b][auditor][modelIndex]) { votedScores[votes] = auditScores[_GI][b][auditor][ @@ -1355,12 +1362,18 @@ contract DINTaskAuditor is Ownable, ReentrancyGuardTransient { for (uint b = 0; b < batchCount; b++) { AuditBatch storage batch = auditBatches[_GI][b]; - for (uint a = 0; a < batch.auditors.length; a++) { + // issue #249 finding 3: both lengths are loop-invariant per + // batch; cache instead of re-reading storage on every iteration + // of the loops they bound (modelIndexes.length was re-read up to + // auditorCount x modelIndexCount times without this). + uint auditorCount = batch.auditors.length; + uint modelIndexCount = batch.modelIndexes.length; + for (uint a = 0; a < auditorCount; a++) { address auditor = batch.auditors[a]; bool missedVote = false; bool exceededDeviation = false; - for (uint m = 0; m < batch.modelIndexes.length; m++) { + for (uint m = 0; m < modelIndexCount; m++) { uint modelIndex = batch.modelIndexes[m]; if (!hasAuditedLM[_GI][b][auditor][modelIndex]) { missedVote = true; diff --git a/foundry/src/DINTaskCoordinator.sol b/foundry/src/DINTaskCoordinator.sol index d3675535..07cacafd 100644 --- a/foundry/src/DINTaskCoordinator.sol +++ b/foundry/src/DINTaskCoordinator.sol @@ -412,17 +412,20 @@ contract DINTaskCoordinator is Ownable, ReentrancyGuardTransient { // Per-model stake floor: enforced when the model owner has set a non-zero bound. uint256 floorMin = dinvalidatorStakeContract.getModelStakeMin(modelId); - if (floorMin > 0 && dinvalidatorStakeContract.getStake(msg.sender) < floorMin) - revert TC_StakeBelowModelFloor(); - // Concurrent-registration cap: enforced when the DAO has set a non-zero value. // Formula: each MIN_STAKE unit of stake allows capPerUnit concurrent registrations. uint256 capPerUnit = dinvalidatorStakeContract.maxConcurrentRegistrationsPerStakeUnit(); - if (capPerUnit > 0) { - uint256 maxAllowed = (dinvalidatorStakeContract.getStake(msg.sender) / - dinvalidatorStakeContract.minStake()) * capPerUnit; - if (dinvalidatorStakeContract.activeRegistrationCount(msg.sender) >= maxAllowed) - revert TC_ConcurrentRegistrationCapReached(); + if (floorMin > 0 || capPerUnit > 0) { + // Single getStake() read shared by both checks below (issue #249 finding 1). + uint256 stake = dinvalidatorStakeContract.getStake(msg.sender); + if (floorMin > 0 && stake < floorMin) + revert TC_StakeBelowModelFloor(); + if (capPerUnit > 0) { + uint256 maxAllowed = (stake / + dinvalidatorStakeContract.minStake()) * capPerUnit; + if (dinvalidatorStakeContract.activeRegistrationCount(msg.sender) >= maxAllowed) + revert TC_ConcurrentRegistrationCapReached(); + } } // Add to list and mark as registered @@ -693,22 +696,25 @@ contract DINTaskCoordinator is Ownable, ReentrancyGuardTransient { uint _GI ) internal view returns (address[] memory activePool) { address[] storage registeredPool = dinAggregators[_GI]; + uint len = registeredPool.length; + // issue #249 finding 2: cache each isValidatorActive result from the + // counting pass instead of re-querying it in the fill pass below. + bool[] memory active = new bool[](len); uint activeCount; - for (uint i = 0; i < registeredPool.length; i++) { - if ( - dinvalidatorStakeContract.isValidatorActive(registeredPool[i]) - ) { + for (uint i = 0; i < len; i++) { + bool isActive = dinvalidatorStakeContract.isValidatorActive(registeredPool[i]); + active[i] = isActive; + if (isActive) { 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; + for (uint i = 0; i < len; i++) { + if (active[i]) { + activePool[ptr++] = registeredPool[i]; } } } @@ -944,21 +950,26 @@ contract DINTaskCoordinator is Ownable, ReentrancyGuardTransient { revert TC_NotReadyToFinalizeT1(); Tier1Batch[] storage batches = tier1Batches[_GI]; + uint batchCount = batches.length; - for (uint i = 0; i < batches.length; i++) { + for (uint i = 0; i < batchCount; i++) { Tier1Batch storage b = batches[i]; + // issue #249 finding 3: batchId/aggregators.length are + // loop-invariant per batch; cache instead of re-reading storage. + uint batchId = b.batchId; + uint aggregatorCount = b.aggregators.length; // Determine the CID with the most votes bytes32 winningCID = ""; uint maxVotes = 0; uint submissionCount = 0; - for (uint j = 0; j < b.aggregators.length; j++) { + for (uint j = 0; j < aggregatorCount; j++) { address aggregator = b.aggregators[j]; - if (t1Submitted[_GI][b.batchId][aggregator]) { + if (t1Submitted[_GI][batchId][aggregator]) { submissionCount++; - bytes32 cid = t1SubmissionCID[_GI][b.batchId][aggregator]; - uint votes = t1Votes[_GI][b.batchId][cid]; + bytes32 cid = t1SubmissionCID[_GI][batchId][aggregator]; + uint votes = t1Votes[_GI][batchId][cid]; if (votes > maxVotes) { maxVotes = votes; winningCID = cid; @@ -981,8 +992,8 @@ contract DINTaskCoordinator is Ownable, ReentrancyGuardTransient { revert TC_InsufficientSubmissions(); b.finalized = true; b.finalCID = winningCID; - tier1FinalizedAt[_GI][b.batchId] = uint64(block.timestamp); - emit T1BatchFinalized(_GI, b.batchId, winningCID); + tier1FinalizedAt[_GI][batchId] = uint64(block.timestamp); + emit T1BatchFinalized(_GI, batchId, winningCID); } _setGIstate(GIstates.T1AggregationDone); @@ -1164,12 +1175,14 @@ contract DINTaskCoordinator is Ownable, ReentrancyGuardTransient { uint256 s2Amount = (minStakeAmt * s2SlashFractionBps) / 10_000; Tier1Batch[] storage t1batches = tier1Batches[_GI]; - for (uint i = 0; i < t1batches.length; i++) { + uint t1Count = t1batches.length; + for (uint i = 0; i < t1Count; i++) { Tier1Batch storage b = t1batches[i]; _slashBatch(_GI, b.batchId, b.aggregators, b.finalCID, minStakeAmt, s2Amount, false); } Tier2Batch[] storage t2batches = tier2Batches[_GI]; - for (uint i = 0; i < t2batches.length; i++) { + uint t2Count = t2batches.length; + for (uint i = 0; i < t2Count; i++) { Tier2Batch storage b = t2batches[i]; _slashBatch(_GI, b.batchId, b.aggregators, b.finalCID, minStakeAmt, s2Amount, true); } @@ -1201,7 +1214,8 @@ contract DINTaskCoordinator is Ownable, ReentrancyGuardTransient { noSubmission = "AGG_T2_NO_SUBMISSION"; badConsensus = "AGG_T2_BAD_CONSENSUS"; } - for (uint j = 0; j < aggs.length; j++) { + uint aggCount = aggs.length; + for (uint j = 0; j < aggCount; j++) { address aggregator = aggs[j]; bool submitted = t2 ? t2Submitted[_GI][batchId][aggregator] @@ -1287,12 +1301,13 @@ contract DINTaskCoordinator is Ownable, ReentrancyGuardTransient { function releaseGIRegistrationSlots(uint _GI) external onlyOwner { bool giEnded = (_GI < GI) || (_GI == GI && GIstate == GIstates.GIended); - require(giEnded, "GI has not ended"); - require(!registrationSlotsReleased[_GI], "slots already released"); + if (!giEnded) revert TC_GINotEnded(); + if (registrationSlotsReleased[_GI]) revert TC_SlotsAlreadyReleased(); registrationSlotsReleased[_GI] = true; address[] storage aggs = dinAggregators[_GI]; - for (uint256 i = 0; i < aggs.length; i++) { + uint256 aggCount = aggs.length; + for (uint256 i = 0; i < aggCount; i++) { dinvalidatorStakeContract.decrementActiveRegistration(aggs[i]); } dinTaskAuditorContract.decrementAuditorRegistrations(_GI); diff --git a/foundry/src/DinValidatorStake.sol b/foundry/src/DinValidatorStake.sol index 12ce88bf..ba7371d9 100644 --- a/foundry/src/DinValidatorStake.sol +++ b/foundry/src/DinValidatorStake.sol @@ -714,22 +714,26 @@ contract DinValidatorStake is } function _syncValidatorStatus(ValidatorInfo storage validator) internal { - if (validator.status == ValidatorStatus.Blacklisted) { + // issue #249 finding 4: cache status/activeStake instead of + // re-reading each from storage. + ValidatorStatus status = validator.status; + if (status == ValidatorStatus.Blacklisted) { return; } if ( - validator.status == ValidatorStatus.Jailed && + status == ValidatorStatus.Jailed && validator.jailedUntil > block.timestamp ) { return; } + uint256 activeStake = validator.activeStake; if (validator.pendingWithdrawals > 0) { validator.status = ValidatorStatus.Exiting; - } else if (validator.activeStake >= MIN_STAKE) { + } else if (activeStake >= MIN_STAKE) { validator.status = ValidatorStatus.Active; - } else if (validator.activeStake > 0) { + } else if (activeStake > 0) { validator.status = ValidatorStatus.Exiting; } else { validator.status = ValidatorStatus.None; diff --git a/foundry/test/StakingEnforcement.t.sol b/foundry/test/StakingEnforcement.t.sol index 38cf069d..bbdad273 100644 --- a/foundry/test/StakingEnforcement.t.sol +++ b/foundry/test/StakingEnforcement.t.sol @@ -22,6 +22,8 @@ import { TC_StakeBelowModelFloor, TC_ConcurrentRegistrationCapReached, TC_WrongGI, + TC_GINotEnded, + TC_SlotsAlreadyReleased, TA_StakeBelowModelFloor, TA_ConcurrentRegistrationCapReached, TA_DualRoleNotAllowed @@ -482,11 +484,20 @@ contract StakingEnforcementTest is Test { vm.startPrank(modelOwner); tc.releaseGIRegistrationSlots(1); - vm.expectRevert(bytes("slots already released")); + vm.expectRevert(TC_SlotsAlreadyReleased.selector); tc.releaseGIRegistrationSlots(1); vm.stopPrank(); } + function test_releaseGIRegistrationSlots_revertsWhenGINotEnded() public { + // GI 1 is still in progress (never driven to GIended) -- neither + // "_GI < current GI" nor "_GI == current GI && GIstate == GIended" + // holds, so the call must revert rather than release slots early. + vm.prank(modelOwner); + vm.expectRevert(TC_GINotEnded.selector); + tc.releaseGIRegistrationSlots(1); + } + // ── one address can't hold both roles in a GI (issue No. 180) ──────────── /// @dev An aggregator for GI 1 can't also register as an auditor for GI 1. From bec5dd0cbfc7c07ddc4c05054ffe7fb57c4d3a43 Mon Sep 17 00:00:00 2001 From: Abidoyesimze Date: Mon, 2 Nov 2026 18:39:54 +0100 Subject: [PATCH 2/2] gas(DINTaskCoordinator): cache batchId/aggregators.length in finalizeT2Aggregation Issue #249 finding 3 named finalizeT2Aggregation alongside finalizeT1Aggregation as the "same shape" ("same shape" meaning the same loop-invariant batchId/aggregators.length re-reads), but the original PR only cached finalizeT1Aggregation. Apply the identical caching here: batches.length once before the loop, b.batchId/b.aggregators.length once per batch, matching finalizeT1Aggregation's comment and structure exactly. Gas (forge test --gas-report, AggregatorCommitRevealTest + LifecycleEventsTest + StakingEnforcementTest, 9 finalizeT2Aggregation calls): avg 130060 -> 129671 (-389), max 206468 -> 205761 (-707). Size (forge build --sizes / contract_size_gate.py): DINTaskCoordinator runtime 23302 B -> 23295 B (margin 1274 B -> 1281 B), still `warn` not `fail`. DINTaskAuditor unaffected (21312 B, untouched by this change). forge test: 517 passed, 0 failed. Co-Authored-By: Claude Sonnet 5 --- foundry/src/DINTaskCoordinator.sol | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/foundry/src/DINTaskCoordinator.sol b/foundry/src/DINTaskCoordinator.sol index ae981ace..44a54b58 100644 --- a/foundry/src/DINTaskCoordinator.sol +++ b/foundry/src/DINTaskCoordinator.sol @@ -1105,21 +1105,26 @@ contract DINTaskCoordinator is Ownable, ReentrancyGuardTransient { revert TC_NotReadyToFinalizeT2(); Tier2Batch[] storage batches = tier2Batches[_GI]; + uint batchCount = batches.length; - for (uint i = 0; i < batches.length; i++) { + for (uint i = 0; i < batchCount; i++) { Tier2Batch storage b = batches[i]; + // issue #249 finding 3: batchId/aggregators.length are + // loop-invariant per batch; cache instead of re-reading storage. + uint batchId = b.batchId; + uint aggregatorCount = b.aggregators.length; // Determine the CID with the most votes bytes32 winningCID = ""; uint maxVotes = 0; uint submissionCount = 0; - for (uint j = 0; j < b.aggregators.length; j++) { + for (uint j = 0; j < aggregatorCount; j++) { address aggregator = b.aggregators[j]; - if (t2Submitted[_GI][b.batchId][aggregator]) { + if (t2Submitted[_GI][batchId][aggregator]) { submissionCount++; - bytes32 cid = t2SubmissionCID[_GI][b.batchId][aggregator]; - uint votes = t2Votes[_GI][b.batchId][cid]; + bytes32 cid = t2SubmissionCID[_GI][batchId][aggregator]; + uint votes = t2Votes[_GI][batchId][cid]; if (votes > maxVotes) { maxVotes = votes; winningCID = cid; @@ -1140,7 +1145,7 @@ contract DINTaskCoordinator is Ownable, ReentrancyGuardTransient { revert TC_InsufficientSubmissions(); b.finalized = true; b.finalCID = winningCID; - tier2FinalizedAt[_GI][b.batchId] = uint64(block.timestamp); + tier2FinalizedAt[_GI][batchId] = uint64(block.timestamp); emit T2Finalized(_GI, winningCID); }