Skip to content

[P1] Int overflow #1222

Open @tcharchian opened 2026-05-21 22:30 UTC 8 comments Updated 2026-10-02 15:11 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 (8)

@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.

@zpoken commented 2026-10-01 10:39 UTC

@tcharchian, following your request, here is the detailed proposal: - Proposal: https://github.com/zpoken/gonka/blob/zpoken/int-overflow-proposal/int-overflow-proposal.md - Community discussion: https://github.com/gonka-ai/gonka/discussions/1886

In short: rules by execution context with code examples, a shared stdlib-only safemath package with SDK/decimal adapters, and a CI check (gosec G115 + nolintlint, new lines only) for chain, devshard, decentralized-api and common.

@olegsuhoparov @Mayveskii, the proposal coordinates with #1379 and #1017 rather than replacing them; your feedback is very welcome.

@olegsuhoparov commented 2026-10-02 15:11 UTC

Thanks @zpoken. I have the smaller #1379 patch ready locally on upgrade-v0.2.16: checked payout conversions and weight-sum overflow handling, with boundary and atomicity tests. The full inference-chain suite, including keeper, and the API -short tests passed. I haven't run Docker builds or the full integration tests, and the patch isn't pushed yet.

The proposal linked in https://github.com/gonka-ai/gonka/discussions/1886 puts chain behavior changes after v0.2.16. Could the maintainers confirm which base and release #1379 should target before I update the PR?

Also, would you prefer keeping the small keeper-local helpers for now, or adopting the shared safemath package once it's available? I'd keep the patch limited to the remaining claim-path guards.

🔄 Auto-synced from Issue #1222 every hour.