Skip to content

task_100926_11.md

Latest commit

 

History

History
146 lines (101 loc) · 19.3 KB

task_100926_11.md

File metadata and controls

146 lines (101 loc) · 19.3 KB
Raw

Task: Reward-Settlement Reconciliation — Align PR #131 with #134, Aggregator-Weight Fix, Docs + Gas Benchmark (Issues #126, #127)

ID: task_100926_11 Assigned to: Similoluwa Abidoye (@Abidoyesimze) Created: 2026-09-10 Status: Closed Repo: https://github.com/InfiniteZeroFoundation/DevNet Base branch: develop — spec written against commit 18166f7 (2026-09-10); this task is blocked on PR #134 (Robbert) merging first (see §0), so re-pull before starting regardless of when you begin. Roadmap ref: P3/P4, BL-10 (Developer/design/DESIGN_DECISIONS.md DD-4), continuation of #121 Tracking: #126, #127 · builds on PR #131 (your own, to be updated in place, not resubmitted) · depends on PR #134 landing on develop first

Context

You opened #124-#127 as a 4-part sequenced breakdown of #121 (DD-4 Option A), and opened PR #131 for #124's part of it. In parallel, Robbert independently built the same Option A design end-to-end in PR #134, which closes #121 directly. Both PRs touch the same reward-settlement code in DINTaskAuditor.sol/DINTaskCoordinator.sol. The full comparison is below (§ Issue/PR status audit) — short version: PR #134 is the one that merges to develop. #124 is already closed as duplicate. This task is what's left once that's settled: a genuine bug in PR #134's aggregator handling that PR #131 already had right, plus the docs (#126) and test (#127) follow-ups that survive regardless of which implementation won.

This is not a redo of #124/#125's scope — that's done, via #134. Don't re-implement client/auditor accumulation; it already exists on develop once #134 merges.

Issue/PR status audit (2026-09-10)

Similoluwa opened #124-#127 as a 4-part sequenced breakdown of #121 (BL-10, "rewards: settleRewards O(n) settlement doubles the C-1 gas-DoS surface"), proposed in a comment on #121 and written up as DD-4 in Developer/design/DESIGN_DECISIONS.md. He explicitly flagged wanting sign-off on Option A before starting #124's code. In parallel, Robbert opened PR #134, which closes #121 directly with a single end-to-end implementation of the same Option A design — built off Umer's original comment on #121, apparently without visibility into Similoluwa's 4-issue breakdown. Both PR #131 (Similoluwa, closes #124) and PR #134 (Robbert, closes #121) are open, unmerged, and touch the same reward-settlement code path with different internal variable names.

Similoluwa's issues (#124-#127)

Issue Title State Assignee Real / still pending?
#124 Accumulate settlement totals incrementally (Part 1/3) CLOSED (2026-09-10, not planned) none Closed as duplicate of PR #134's scope — see comment.
#125 Collapse settleRewards to O(1) + claimReward(gi) (Part 2/3) OPEN Similoluwa (@Abidoyesimze) Superseded by PR #134's scope, but PR #131 stays open — plan revised 2026-09-10. Rather than close PR #131 outright, once #134 merges to develop: rebase PR #131 onto it, adopt #134's naming/shape (giTotalApprovedScore, giTotalAuditWeight, auditorGIWeight, GIRewardSnapshot, claimReward(gi)), drop the now-duplicate client/auditor accumulation, and keep/port PR #131's more-correct aggregator-weight logic (see below). Comment posted on PR #131 telling Similoluwa to start this once task_11 is approved. Whether #125 itself gets formally closed (mirroring #124) is Umer's call to make separately once #134 merges — not part of this task's scope.
#126 Update MECHANISM_DESIGN.md §5 for two-step settle/claim (Part 3/3, docs) OPEN none Still real — retarget to describe PR #134's shipped code (not #125's design, now superseded). In scope for this task.
#127 Rewrite RewardEngine.t.sol for O(1) flow + spec-scale gas benchmark (Part 3/3, tests) OPEN none Mostly redundant, one piece still real — PR #134 already updated RewardEngine.t.sol for the two-step flow (24/24 pass + 3 new revert tests), covering most of #127. The spec-scale (10-50 validators / 100-500 clients) gas benchmark against PR #134's merged code, proving endGI is now bounded, is not shown in PR #134 and is still open. In scope for this task.

Robbert's issues (#78, #114, #115, #120, #121, #122) vs. his PRs (#116, #117, #129, #133, #134)

Issue Title State Addressed by PR state
#78 Simulation: model validator gas costs to size ETH network fee OPEN none yet —
#114 import-deployments silently drops dinTreasury/dinFeeRouter + proxy admins (BL-8) OPEN PR #116 (Closes #114) OPEN, unmerged
#115 Encrypted test-data key array is positional, not address-keyed OPEN none — still unaddressed; relevant to task_240826_10/#40 continuation, not this task
#120 Nonce collision on concurrent tx submission (BL-1) OPEN PR #129 (Closes #120) OPEN, unmerged
#121 settleRewards unbounded loop doubles C-1 gas-DoS surface (BL-10) OPEN PR #134 (Closes #121) OPEN, unmerged — this task's blocking dependency
#122 Dispute bond DIN vs. ETH denomination undocumented (BL-12) OPEN PR #133 (Closes #122) OPEN, unmerged (docs-only, no contract change)
— — — PR #117 — fixes SlashingInvariants.t.sol's 3 failing tests (pre-existing backlog item from PR #66, not tied to a numbered issue above) OPEN, unmerged

Conflict resolution (Umer, 2026-09-10)

For #121 vs. #124/#125/#127 and any other case in this audit where Robbert and Similoluwa independently implemented the same fix: merge Robbert's PR; Similoluwa's overlapping issue/PR is redundant. Applied here:

  • PR #134 (Robbert) merges for #121. #124 is closed as duplicate/superseded.
  • #126 and the gas-benchmark half of #127 survive — they're docs/tests gaps that exist regardless of which implementation won, just need to be retargeted at PR #134's shipped shape rather than #125's. These are this task's scope.
  • Residual note, not blocking the decision: PR #131/#124's design tracked aggregatorWeight incrementally (O(1) at claim time for aggregators), while PR #134 only marks isRewardableAggregator and still enumerates aggregators at settlement — narrower on the aggregator side than what #121 originally asked for, and (see below) actually loses funds. This is now this task's third piece of scope, carried via an updated PR #131 rather than a fresh issue.

Deep-dive: PR #131 vs PR #134 diffs (checked out at /home/umerm/tempdir/DIN/PRs/PRreview_131 and PRreview_134)

Read both diffs against develop in full (not just the PR descriptions). Confirms the redundancy call above and surfaces one concrete, non-cosmetic finding worth carrying forward:

PR #131's aggregatorWeight/totalAggregatorWeight accumulation is more correct than PR #134's isRewardableAggregator boolean, and PR #134's version loses funds.

  • DINTaskCoordinator._collectFinalizedBatchAggregators (untouched by PR #134 — confirmed via diff, only the docstring above it changed) is explicitly documented as building "one entry per (aggregator, finalized batch) pair" — "an aggregator who completed both a T1 batch and the T2 batch appears twice, earning proportionally more weight" (task_210726_6 §3's spec, restated verbatim in the function's own NatSpec). This is deliberate, documented, pre-existing behavior, not something #121 asked to change.
  • PR #131 preserves this exactly: it increments aggregatorWeight[gi][aggregator]++ and totalAggregatorWeight[gi]++ once per (aggregator, finalized-batch) pair inside finalizeT1Aggregation/finalizeT2Aggregation — an aggregator active in both T1 and T2 ends up with weight 2, same as the pre-#124 _settleAggregatorRewards's behavior via duplicate array entries.
  • PR #134 instead does, in settleRewards: count = rewardableAggregators.length (still duplicate-counted, since _collectFinalizedBatchAggregators is unchanged), aggregatorShare = aggregatorPool / count, then isRewardableAggregator[gi][addr] = true for each entry — a boolean write, idempotent on the second occurrence. claimReward(gi) then pays each rewardable aggregator exactly one aggregatorShare, regardless of how many finalized batches they're credited for.
  • Concrete failure: T1 finalizes with [agg1, agg2, agg3], T2 finalizes with [agg1] (agg1 did both) → rewardableAggregators = [agg1, agg2, agg3, agg1], count = 4, aggregatorShare = aggregatorPool / 4. agg1 should earn 2 units (correct, proportional design) but claimReward only ever adds one aggregatorShare for agg1 — the other 1/4 of aggregatorPool is never credited to anyone, anywhere. It isn't misrouted to the treasury either (treasury share is computed once at settleRewards time from the bps split, independent of this). It's simply stranded in the contract, permanently unclaimable — a real loss-of-funds bug, not just an underpayment.
  • Neither PR #134's own tests nor PR #131's exercise an aggregator finalizing both a T1 and T2 batch in the same GI (both use the shared _runFullHonestGI fixture: one T1 batch, no T2), so this doesn't show up in either test suite as-is.

Resolution — revised 2026-09-10: rather than close PR #131 and file a fresh follow-up issue, keep PR #131 open and update it in place once #134 merges to develop:

  1. Rebase PR #131's branch onto develop (post-#134-merge).
  2. Adopt #134's shape/naming (giTotalApprovedScore, giTotalAuditWeight, auditorGIWeight, GIRewardSnapshot, claimReward(gi)) and drop PR #131's now-duplicate client/auditor accumulation.
  3. Keep/port PR #131's aggregatorWeight/totalAggregatorWeight per-(aggregator, finalized-batch) counter into #134's claimReward, replacing isRewardableAggregator (bool) — pay (aggregatorPool * aggregatorWeight[gi][addr]) / totalAggregatorWeight[gi] instead of a flat per-address share.
  4. Add a test with an aggregator finalizing both a T1 and the T2 batch in one GI to lock in the fix (neither PR currently covers this).

This is now flagged to Similoluwa via a comment on PR #131 — he can start on it once task_11 is approved (candidate scope item, alongside #126 and #127's gas benchmark).

§0. Blocking dependency — read before starting anything

Do not start until PR #134 has merged to develop. Everything below is written against giTotalApprovedScore, giTotalAuditWeight, auditorGIWeight, GIRewardSnapshot, and claimReward(gi) — names and shapes that only exist once #134 lands. Starting earlier means rebasing your work a second time for no reason.

While you wait: it's fine to read #134's diff and this task's audit above, and to sketch the MECHANISM_DESIGN.md §5 rewrite (§2 below) against #134's description, since the shipped shape shouldn't change materially between now and merge. Don't write contract code against it yet.

Two other things worth knowing before you touch these files, independent of #134:

  • PR #133 (Robbert, open, docs-only) also touches DINTaskCoordinator.sol — a NatSpec comment on disputeBond, unrelated region to reward settlement, low collision risk, but re-pull if it's landed by the time you start.
  • The develop working tree also has in-flight encrypted-test-data work (task_240826_10 continuation) touching DINTaskAuditor.sol/DINShared.sol for an unrelated feature (auditor test-data encryption, not rewards). Different functions, low collision risk, but re-pull and re-run forge test right before opening your PR regardless, per usual practice.

Scope for this task

1. Update PR #131 to match PR #134 + fix the aggregator-weight bug

Once #134 is on develop:

  • Rebase feat/reward-settlement-accumulation (PR #131's branch) onto current develop.
  • Remove PR #131's client/auditor accumulation additions (totalApprovedScore, auditorWeight/totalAuditorWeight in DINTaskAuditor.sol) — #134 already ships equivalent logic under different names (giTotalApprovedScore, auditorGIWeight/giTotalAuditWeight). Don't carry duplicate storage forward.
  • In DINTaskCoordinator.sol, keep PR #131's aggregatorWeight/totalAggregatorWeight mappings and their accumulation inside finalizeT1Aggregation/finalizeT2Aggregation (one increment per (aggregator, finalized-batch) pair, exactly as PR #131 already has it — no change needed to that part).
  • In DINTaskAuditor.sol, replace #134's isRewardableAggregator mapping and the aggregatorShare = aggregatorPool / count flat-split logic in settleRewards/claimReward with a weighted split: claimReward's aggregator branch becomes (snap.aggregatorPool * aggregatorWeight[gi][msg.sender]) / totalAggregatorWeight[gi] (pull totalAggregatorWeight[gi] from the coordinator the same way settleRewards already reads rewardableAggregators today — check whether that needs a new cross-contract read or can be passed in at settle time, and note whichever you pick in the PR description).
  • settleRewards no longer needs the count/aggregatorShare computation or the rewardableAggregators loop at all once aggregator weight is tracked incrementally — this is the same shape #134 already gave clients/auditors, now extended to aggregators. Confirm _collectFinalizedBatchAggregators in DINTaskCoordinator.sol can be deleted entirely once nothing reads its output, or flag if something else still depends on it.

2. Developer/design/MECHANISM_DESIGN.md §5 update (closes #126)

Rewrite the "Settlement & claims" subsection to describe what's actually shipped after (1) above, not the old push-at-settlement model:

  • Settlement (endGI/settleRewards) computes and stores per-role pool totals + the aggregator-weight snapshot — O(1), no per-participant loop for any of clients, auditors, or aggregators.
  • Each participant calls claimReward(gi) once per GI they're owed from, computing their own share on demand from totals accumulated incrementally during evaluation/aggregation.
  • claimRewards() remains the final withdrawal step, unchanged.
  • Update the "Open decisions" list if aggregator batch-size scaling or pool minimum denomination are affected as a side effect of the weighted-aggregator-share change — check before assuming no impact.

3. Spec-scale gas benchmark for endGI (remaining piece of #127)

RewardEngine.t.sol already has the two-step-flow test coverage from #134 (24/24 passing) plus whatever (1) above adds for the aggregator fix. What's still missing:

  • A gas benchmark at spec scale (10-50 validators / 100-500 clients), same measured-marginal-cost-plus-explicit-extrapolation methodology as SecurityFindings.t.sol's test_gas_finalizeEvaluation_and_slashAuditors_atScale — proving endGI's incremental cost is now bounded regardless of participant count. This is the number that actually closes out #121/#127's original ask; nothing currently in either PR measures it.
  • Confirm the conservation invariant still holds under the new aggregator math: sum of all eventually-claimed amounts (clients + auditors + aggregators) + treasury share ≤ deposited pool for a GI, fuzzed across participant counts — extend the existing rounding-dust fuzz coverage from feature/reward-engine rather than writing a new one from scratch if it's already close to this shape.

Explicitly out of scope

  • Re-implementing client/auditor accumulation — already done by #134.
  • #115 (encrypted test-data key ordering) and #78 (gas-cost simulation for the ETH network fee) — Robbert's open items, unrelated to this task's reward-settlement scope.
  • Any change to the reward role-split bps (60/20/15/5) or the aggregator batch-size-scaling open question in MECHANISM_DESIGN §5/§9 — document what exists, don't relitigate the numbers.

Deliverables

  • PR #131 rebased onto develop post-#134-merge, client/auditor duplication removed
  • aggregatorWeight/totalAggregatorWeight accumulation kept in DINTaskCoordinator.sol (from PR #131, unchanged)
  • DINTaskAuditor.sol's settleRewards/claimReward updated to use weighted aggregator shares instead of isRewardableAggregator flat split
  • New test: an aggregator finalizing both a T1 batch and the T2 batch in the same GI earns proportionally more, and no pool balance is left permanently stranded
  • Developer/design/MECHANISM_DESIGN.md §5 rewritten for the shipped two-step settle/claim flow, including the weighted-aggregator-share behavior
  • Spec-scale (10-50 validators / 100-500 clients) gas benchmark test for endGI, matching SecurityFindings.t.sol's methodology
  • Conservation invariant (claimed + treasury ≤ deposited pool) confirmed under the new aggregator math, fuzzed
  • forge build && forge test green, full suite
  • PR description explains what changed vs. the original PR #131, references #126, #127, and this task file

Submission

One PR is the right shape for this — continue on PR #131's existing branch rather than opening a new one; it already carries the relevant history and review context, and the scope here (aggregator-weight fix + its test, docs update, gas benchmark) is small and tightly coupled to the same code path. Don't split the docs update or the gas benchmark into separate PRs.

PR description should reference #126 and #127, explain the aggregator-weight bug found in #134 and how this PR fixes it, and note that #124 is already closed and #125 is expected to be closed separately (Umer's call) once this PR's aggregator fix supersedes what was left of it.

Notes

  • Wait for #134 to merge before starting contract work (§0) — this is a hard blocker, not a suggestion.
  • If #134's shipped shape differs materially from what's described in this task (naming, snapshot struct fields, etc.) by the time it merges, adapt to what's actually on develop — this spec is written against the PR #134 diff as of 2026-09-10, not a guarantee of the final merged form.
  • Flag any change to DINTaskAuditor.sol/DINTaskCoordinator.sol for Umer's review given cryptoeconomic sensitivity, same as task_210726_6.