Skip to content

[P1] Int overflow #1222

Open @tcharchian opened 2026-05-21 22:30 UTC 6 comments Updated 2026-07-22 05:01 UTC
Priority: Medium nice-to-have

The goal of this is to have in place after this a standard way of handling possible overflows, have it implemented consistently across the entire codebase and to have a check (preferably a static check, an AI persona if necessary as a backup) that flags anything that doesn't use the established pattern

💬 Comments (6)

@olegsuhoparov commented 2026-06-30 13:00 UTC

Opened a surgical first PR against main: #1379.

It ports the already-accepted #1100/#1101 overflow fixes to main and adds two small guards for payout uint64->int64 conversion and validation totalWeight accumulation.

I intentionally left broad static analysis and #1017 supply-cap semantics out of scope so this remains reviewable.

@Mayveskii commented 2026-07-07 10:08 UTC

Hi @olegsuhoparov

Wanted to flag that #1017 (Bitcoin reward supply-cap overflow guards) has been aligned with the overflow-handling direction from this meta-issue.

Changes on branch fix/bitcoin-rewards-supply-cap-overflow (commit 9e4ebf74e):

  • Added a reusable checkedAddUint64 helper in inference-chain/x/inference/keeper/bitcoin_rewards.go, placed next to the checkedAddInt64 pattern introduced in #1379 .
  • Replaced the inline math.MaxUint64 overflow checks in the supply-cap reduction loop and in CalculateParticipantBitcoinRewards with the new helper.
  • Upgraded the overflow guard logs from Warn to Error.
  • Added explicit unit tests for the overflow path:
    • TestCheckedAddUint64
    • TestGetBitcoinSettleAmounts_SupplyCapReductionNoWrap

Since #1379 explicitly left the #1017 supply-cap semantics out of scope, #1017 adopts the same helper/logging/test pattern independently rather than conflicting with #1379.

@olegsuhoparov commented 2026-07-08 08:51 UTC

Thanks for aligning #1017 with the overflow-handling direction from #1379.

That split makes sense to me: #1379 intentionally stays focused on settlement/claim/validation overflow paths and leaves the Bitcoin supply-cap semantics to a separate PR.

One thing to keep in mind is merge order: #1379 changes the CalculateParticipantBitcoinRewards fallback from MaxUint64 to MaxInt64, so if #1379 lands first, #1017 will likely need a small rebase/adaptation around the fallback-path rationale and tests.

I would also double-check that the new #1017 tests assert the overflow guard path directly, not only the final totalRewarded <= remainingSupply invariant. That would make the defense-in-depth coverage clearer for reviewers.

@Mayveskii commented 2026-07-08 09:53 UTC

re

Have a good time, Oleg! Direct overflow guard test - added in the latest commit 76d0ab6d0 on fix/bitcoin-rewards-supply-cap-overflow: - TestApplyProportionalSupplyCapReduction_OverflowGuard feeds RewardCoins = MaxUint64 for two participants and asserts the guard fires, total Distributed saturates at MaxUint64, and remaining rewards are zeroed. The log shows the Error-level guard trigger. Merge order — agreed. If #1379 lands first, I'll rebase #1017 and align the CalculateParticipantBitcoinRewards fallback path with the MaxInt64 cap introduced there.

I'll keep waiting maintainers reaction and proto lifecycle move.

@tcharchian commented 2026-07-20 22:20 UTC

First, prepare a detailed proposal outlining what is planned, why it should be done, the expected outcomes, and the rationale behind the proposed approach. Share the proposal with the community and obtain validation before proceeding with implementation.

@everyone @Mayveskii @olegsuhoparov

@Mayveskii commented 2026-07-22 05:01 UTC

re

Got it.

🔄 Auto-synced from Issue #1222 every hour.