docs(node-rewards): say why the synced day is the furthest, not the earliest

[pietrodimarco-dfinity]

What

A comment on the last_day_synced computation in update_subnets_metrics explaining why it takes the furthest day any subnet reached rather than the earliest. Comments only — no behaviour change.

Why

Taking the maximum across subnets reads like an oversight. A security scan and a Copilot review both flagged it as one within a day, and nothing in the code said otherwise. Both suggested the minimum instead.

The minimum would be actively harmful. It holds last_day_synced back to whichever subnet reported least, so validate_reward_period rejects the whole reward period — and then no provider is paid for those days, including the ones with nodes in the subnet that fell behind. A subnet that stayed down would freeze minting indefinitely. See #11423, which implemented exactly that and was closed for this reason.

The comment records three things, so the next reader does not have to rediscover them:

1. Why the maximum. A halted or unreachable subnet is a protocol-level event that node providers do not control, so it must not withhold their rewards. The reward calculation is built to match: a node with no metrics for a day counts as unassigned, which for a provider with a healthy fleet is rewarded in full.
2. Why subnets disagree at all. node_metrics_history withholds the running (current-day) snapshot, and a day's snapshot is only closed once a later block arrives — so a halted or stalled subnet answers with fewer days than a healthy one, and one that stalled before closing its first snapshot answers with nothing.
3. What actually guards completeness, since it is not this line: the all-or-nothing check on failed calls just above. A failed call means data exists that we could not fetch — our own staleness — and stops the sync. A successful call returns every snapshot the subnet has closed, so whatever is still missing does not yet exist.

It also records one accepted gap: a subnet that has not rolled over into the new day when the sync runs reports one day short, with its real data arriving minutes later, so a mint landing in that window pays full rewards for a day whose metrics did exist. That is a timing artefact rather than the protocol-fault case, and too narrow to justify a gate that can stall minting network-wide.

Testing

Comments only. cargo fmt --check clean and the crate compiles; the added lines stay inside the 100-column limit (rustfmt does not reflow comments).

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

The new comments overstate completeness guarantees and omit the all-empty sentinel behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Documents why subnet metric synchronization uses the furthest reported day.

Changes:

• Clarifies the return value and rationale for maximum-day synchronization.
• Documents lagging-subnet and reward implications.

File summaries

File	Description
rs/node_rewards/canister/src/metrics.rs	Adds synchronization rationale and edge-case commentary.

Review details

Suppressed comments (1)

rs/node_rewards/canister/src/metrics.rs:201

• This overstates the guarantees on both paths. A failed management call does not prove that data exists, and successful subnet results/cursors are still persisted before the function returns Err (as partial_failures_are_handled_correctly demonstrates). Also, the endpoint exposes only the 60 snapshots still retained, so after a long outage success does not prove that every historical closed snapshot was fetched. Describe this check as guarding advancement of the global synced day rather than completeness.

            // Completeness is guarded by the all-or-nothing check above rather than by this: a
            // call that *fails* means data exists that we could not fetch — our own staleness — and
            // stops the sync without advancing anything. A call that *succeeds* returns every
            // snapshot the subnet has closed, so whatever is still missing does not yet exist.

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.