fix: resolve division-by-zero chain halt by amathxbt · Pull Request #299 · KiiChain/kiichain

amathxbt · 2026-03-19T23:05:35Z

Summary

This PR fixes 2 confirmed bugs found in the KiiChain codebase:

🔴 Fix 1: Division-by-Zero Chain Halt in `CalculateReward` (Closes #267)

Severity: Critical — Chain Halt

File: x/rewards/types/reward.go

Root Cause:
The CalculateReward function used Seconds() (which truncates to whole seconds) for its division check, but compared equality using nanosecond precision. When EndTime - LastReleaseTime is between 1–999 milliseconds:

Equal() returns false (they differ by sub-second)
Seconds() returns 0 (truncation)
Division by zero panic → all validators crash simultaneously → chain halt

Fix:
Replaced Seconds() with Nanoseconds() throughout CalculateReward to maintain full precision and prevent truncation-induced division by zero. When EndTime <= LastReleaseTime, the full remaining amount is now released immediately instead of returning an error.

// Before (vulnerable)
timeElapsed := math.NewInt(int64(timeElapsedStamp.Seconds())).BigInt()
totalDuration := math.NewInt(int64(totalDurationStamp.Seconds())).BigInt()

// After (fixed)
timeElapsedNs := math.NewInt(timeElapsedStamp.Nanoseconds()).BigInt()
totalDurationNsBig := math.NewInt(totalDurationStamp.Nanoseconds()).BigInt()

Also improved the guard check from Equal() to <= 0 nanoseconds for full precision safety.

Testing

The division-by-zero fix is verifiable with the PoC test from [BUG] Division by Zero in CalculateReward causes chain halt when duration < 1 second #267: go test -v -run TestCalculateReward ./x/rewards/types/...
All 8 table-driven test cases pass including sub-second duration regression and the EndTime == LastReleaseTime edge case

Related Issues

Closes [BUG] Division by Zero in CalculateReward causes chain halt when duration < 1 second #267 — Division by Zero in CalculateReward causes chain halt

coderabbitai · 2026-03-19T23:05:49Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

Walkthrough

Replaces second-precision time math in CalculateReward with nanosecond precision: compute durations via Sub(...).Nanoseconds(), use nanosecond values for the release proportion, and treat zero-or-negative total duration by returning the full remaining reward and nil error. Removed the fmt import. Tests were updated to expect successful releases for equal/short-duration schedules and a new sub-second (500ms) proportional-release case. A CHANGELOG entry documents the fix.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 1 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	Title mentions 'oracle weight parameter bug' but the actual diff only includes the division-by-zero fix in CalculateReward; the oracle/ballot.go fix was merged separately and is not in this PR.	Update title to 'fix: Resolve division-by-zero chain halt in CalculateReward' to accurately reflect only the changes included in this diff.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description check	✅ Passed	Description is related to the changeset and documents the division-by-zero fix in reward.go with detailed context, test cases, and related issues.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

🧹 Nitpick comments (2)

x/rewards/types/reward.go (1)

22-39: Add an explicit sub-second regression test (e.g., 500ms total duration).

Given issue #267, a test where EndTime - LastReleaseTime is in the 1–999ms range would lock in this fix and prevent reintroduction of second-truncation behavior.

Example test case shape (in x/rewards/types/reward_test.go)

+{
+    name:      "sub-second duration does not divide by zero",
+    blockTime: now.Add(250 * time.Millisecond),
+    schedule: types.ReleaseSchedule{
+        TotalAmount:     sdk.NewCoin(denom, math.NewInt(1000)),
+        ReleasedAmount:  sdk.NewCoin(denom, math.NewInt(0)),
+        LastReleaseTime: now,
+        EndTime:         now.Add(500 * time.Millisecond),
+        Active:          true,
+    },
+    expectedError: false,
+},

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@x/rewards/types/reward.go` around lines 22 - 39, Add a regression test in
x/rewards/types/reward_test.go that covers a sub-second total duration (e.g.,
set schedule.LastReleaseTime and schedule.EndTime 500ms apart) to ensure the
nanosecond-based math (use of
schedule.EndTime.Sub(schedule.LastReleaseTime).Nanoseconds(), totalDurationNs,
timeElapsedNs and the calculation of releaseProportion) prevents
division-by-zero from second-truncation; the test should simulate blockTime
between LastReleaseTime and EndTime, call the code path that computes
releaseProportion, and assert a valid non-zero proportion and no error when
totalDurationNs > 0.

x/oracle/types/ballot.go (1)

25-25: Add a regression test with distinct power and weight values.

The assignment fix is correct, but current coverage (e.g., power == weight) won’t catch this bug if it regresses. Please add a case like power=10, weight=0 in x/oracle/types/ballot_test.go.

Proposed test adjustment

 func TestNewClaim(t *testing.T) {
 	power := int64(10)
-	weight := int64(10)
+	weight := int64(0)
 	winCount := int64(0)
 	didVote := false
 	recipient := sdk.ValAddress([]byte("validator1"))

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@x/oracle/types/ballot.go` at line 25, Add a regression test in
x/oracle/types/ballot_test.go that constructs a Ballot using distinct power and
weight values (e.g., power=10, weight=0) and asserts that the Ballot.Weight
field (or method) equals the provided weight rather than power; locate the
Ballot constructor/struct in ballot.go (referencing the Weight field assignment)
and in the new test create the ballot, then assert both ballot.Power == 10 and
ballot.Weight == 0 to ensure the previous bug (using power instead of weight) is
covered and cannot regress.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@x/oracle/types/ballot.go`:
- Line 25: Add a regression test in x/oracle/types/ballot_test.go that
constructs a Ballot using distinct power and weight values (e.g., power=10,
weight=0) and asserts that the Ballot.Weight field (or method) equals the
provided weight rather than power; locate the Ballot constructor/struct in
ballot.go (referencing the Weight field assignment) and in the new test create
the ballot, then assert both ballot.Power == 10 and ballot.Weight == 0 to ensure
the previous bug (using power instead of weight) is covered and cannot regress.

In `@x/rewards/types/reward.go`:
- Around line 22-39: Add a regression test in x/rewards/types/reward_test.go
that covers a sub-second total duration (e.g., set schedule.LastReleaseTime and
schedule.EndTime 500ms apart) to ensure the nanosecond-based math (use of
schedule.EndTime.Sub(schedule.LastReleaseTime).Nanoseconds(), totalDurationNs,
timeElapsedNs and the calculation of releaseProportion) prevents
division-by-zero from second-truncation; the test should simulate blockTime
between LastReleaseTime and EndTime, call the code path that computes
releaseProportion, and assert a valid non-zero proportion and no error when
totalDurationNs > 0.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4de32c24-3172-4121-aae0-36cdce1a27dc

📥 Commits

Reviewing files that changed from the base of the PR and between b103c1c and 74a669b.

📒 Files selected for processing (2)

x/oracle/types/ballot.go
x/rewards/types/reward.go

Thaleszh

There are a couple changes needed, there is also a merge conflict with main that needs to be addressed

amathxbt · 2026-03-31T12:41:40Z

@Thaleszh done, good call. Changed the guard so that when totalDurationNs <= 0 we return remaining directly instead of erroring out. Makes much more sense semantically since if the schedule has already ended the node should just release whatever is left rather than halt on an error.

@adambalogh this is now pushed to the branch if you want to take a look.

amathxbt · 2026-03-31T12:41:41Z

@Thaleszh cleaned up. Removed the inline fix annotation comments from reward.go, the code is self-explanatory and the PR description covers the reasoning.

@adambalogh both commits are in, ready for another pass.

amathxbt · 2026-03-31T12:41:42Z

@Thaleszh good to know, stripped the trailing comment from the Weight assignment since the change itself is still present and correct as far as the code goes. If the whole hunk is a duplicate of what landed in another PR, let me know and I can drop it entirely from this one too.

@adambalogh flagging this in case you want to verify against the other PR before merging.

@Thaleszh

…nd precision to prevent division by zero Addresses reviewer feedback from @Thaleszh: - When EndTime <= LastReleaseTime the remaining reward is released immediately rather than returning an error - Removed inline fix-comment annotations from the code - Added sub-second duration regression test - Removed ballot.go change (already merged in a separate PR)

amathxbt · 2026-03-31T12:44:29Z

Hey @Thaleszh, thanks for the review! I've addressed all three of your points in the latest push:

1. Zero-duration guard now releases the full remaining reward

Changed the totalDurationNs <= 0 branch from returning an error to returning remaining, nil. So when EndTime equals (or is before) LastReleaseTime, the entire remaining balance goes out immediately instead of halting with an error. The test case that previously expected an error has been updated to verify it now returns the correct remaining amount.

2. Inline fix-comment annotations removed

Stripped all the // Fix: and // that causes... annotations from reward.go. The code is clean now with just the functional comments explaining what each step does.

3. ballot.go change dropped

Since the Weight field assignment was already corrected in a separate PR, I've rebased this branch directly on top of the current main. ballot.go no longer appears in the diff at all.

Also added a sub-second duration regression test (EndTime - LastReleaseTime = 500ms) to make sure the nanosecond path can't regress back to the old second-truncation behaviour.

@adambalogh happy to make any further adjustments if something still looks off.

codecov · 2026-03-31T14:30:26Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Thaleszh · 2026-03-31T14:34:01Z

@amathxbt Just missing changelog with fixes, under unreleased

…coverage CI check

amathxbt · 2026-03-31T14:50:20Z

@Thaleszh just pushed fixes for both CI warnings blocking this PR:

Fix 1 — Docstring Coverage (was 66.67%, needs ≥ 80%)

TestCalculateReward in reward_test.go was missing a doc comment. Added a full docstring covering all 8 test scenarios (exhausted schedules, linear proportional releases, sub-second nanosecond precision, and the EndTime == LastReleaseTime immediate-release edge case). Commit: 77ecf6b

Fix 2 — Out of Scope Changes warning

Updated the PR description to clarify that the ballot.go / issue #293 fix was already merged in a separate PR and is intentionally absent from this diff. The description now explicitly states this PR only addresses the CalculateReward division-by-zero bug (issue #267).

Both CI warnings should now clear on the next check run. Please let me know if anything else is needed before merge. Thanks!

coderabbitai · 2026-03-31T14:51:11Z

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check the current state of reward_test.go for the docstring
fd -e go reward_test | xargs grep -A 5 "TestCalculateReward" | head -30

# Check if CHANGELOG exists and has unreleased section
fd -iname "CHANGELOG*" | head -5
fd -iname "CHANGELOG*" | xargs grep -i "unreleased" -A 20 | head -40

Length of output: 1026

🏁 Script executed:

#!/bin/bash
# Find and check CHANGELOG
find . -maxdepth 2 -iname "CHANGELOG*" 2>/dev/null | head -5

# Check unreleased section
find . -maxdepth 2 -iname "CHANGELOG*" -exec grep -i "unreleased" -A 20 {} \; | head -50

Length of output: 765

@amathxbt Thanks for the updates! Here's a quick status check:

Fix 1 — Docstring ✅
The docstring for TestCalculateReward looks solid and comprehensive, covering all the relevant scenarios. That should clear the coverage warning.

Fix 2 — PR Description ✅
Noted — clarifying the ballot.go scope in the description makes sense.

Changelog ⚠️ Still Missing
Thaleszh flagged that a changelog entry is needed under ## Unreleased. Looking at the current CHANGELOG.md, the ## Unreleased → ### Fixed section does not yet have an entry for the CalculateReward division-by-zero fix (issue #267). You'll want to add something like:

## Unreleased

### Fixed

- Fix division-by-zero panic in `CalculateReward` when `EndTime - LastReleaseTime` is sub-second; switch time calculations to nanosecond precision and release remaining amount when total duration is zero or negative (x/rewards/types/reward.go, closes `#267`)

That should be the last remaining item before merge. Let me know once it's in!

…-by-zero fix (KiiChain#267)

amathxbt · 2026-03-31T14:53:33Z

@Thaleszh @coderabbitai — changelog entry added as requested. Pushed to CHANGELOG.md under ## Unreleased → ### Fixed:

- Fix division-by-zero chain halt in `CalculateReward` caused by sub-second schedule durations; replace `Seconds()` truncation with `Nanoseconds()` precision and release full remaining reward when `EndTime <= LastReleaseTime` ([#267](https://github.com/KiiChain/kiichain/issues/267))

Commit: 32601a8

The entry is inserted as the first item in the ### Fixed list so it's immediately visible at the top of the Unreleased section. All three CI warnings (docstring coverage, out-of-scope changes, and missing changelog) should now be resolved. Ready for final review! 🚀

coderabbitai · 2026-03-31T14:54:08Z

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check the CHANGELOG.md for the unreleased section and the new entry
find . -maxdepth 2 -iname "CHANGELOG*" | head -5
echo "---"
find . -maxdepth 2 -iname "CHANGELOG*" -exec grep -i "unreleased" -A 30 {} \; | head -50

Length of output: 1175

@amathxbt The changelog entry is confirmed ✅ — CHANGELOG.md now shows the entry at the top of ## Unreleased → ### Fixed:

"Fix division-by-zero chain halt in CalculateReward caused by sub-second schedule durations; replace Seconds() truncation with Nanoseconds() precision and release full remaining reward when EndTime <= LastReleaseTime (#267)"

All three previously flagged items are now resolved:

Docstring coverage ✅ — TestCalculateReward doc comment added
Out-of-scope changes ✅ — ballot.go clarified in PR description as already merged separately
Changelog entry ✅ — Entry added under ## Unreleased → ### Fixed

The PR looks good to go from my side. 🚀

🐇✨

) # Description Adds missing regression tests for changes done in #299 ## Type of change - [x] test (For updates on tests) # PR Checklist: Make sure each step was done: - [ ] Updated changelog with PR's intent - [x] Lint with `make lint-fix`

amathxbt requested a review from jhelison as a code owner March 19, 2026 23:05

coderabbitai Bot reviewed Mar 19, 2026

View reviewed changes

Thaleszh reviewed Mar 27, 2026

View reviewed changes

Comment thread x/rewards/types/reward.go Outdated

Comment thread x/rewards/types/reward.go Outdated

Comment thread x/oracle/types/ballot.go Outdated

amathxbt force-pushed the fix/critical-bugs-div-zero-weight-param branch from aa4ae8f to 1c6ba35 Compare March 31, 2026 12:44

docs(rewards): add docstring to TestCalculateReward to fix docstring …

77ecf6b

…coverage CI check

docs(changelog): add ## Unreleased entry for CalculateReward division…

32601a8

…-by-zero fix (KiiChain#267)

amathxbt requested a review from Thaleszh March 31, 2026 15:12

Thaleszh changed the title ~~fix(critical): Resolve division-by-zero chain halt & oracle weight parameter bug~~ fix: Resolve division-by-zero chain halt & oracle weight parameter bug Mar 31, 2026

Thaleszh approved these changes Mar 31, 2026

View reviewed changes

Thaleszh merged commit 2c96c69 into KiiChain:main Mar 31, 2026
10 checks passed

Thaleszh mentioned this pull request Apr 2, 2026

test: add regression tests for reward schedule under 1 second left #315

Merged

3 tasks

Thaleszh changed the title ~~fix: Resolve division-by-zero chain halt & oracle weight parameter bug~~ fix: Resolve division-by-zero chain halt Apr 2, 2026

Thaleszh changed the title ~~fix: Resolve division-by-zero chain halt~~ fix: resolve division-by-zero chain halt Apr 2, 2026

Conversation

amathxbt commented Mar 19, 2026 • edited by Thaleszh Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

🔴 Fix 1: Division-by-Zero Chain Halt in CalculateReward (Closes #267)

Testing

Related Issues

Uh oh!

coderabbitai Bot commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Estimated code review effort

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Thaleszh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amathxbt commented Mar 31, 2026

Uh oh!

amathxbt commented Mar 31, 2026

Uh oh!

amathxbt commented Mar 31, 2026

Uh oh!

amathxbt commented Mar 31, 2026

Uh oh!

codecov Bot commented Mar 31, 2026

Codecov Report

Uh oh!

Thaleszh commented Mar 31, 2026

Uh oh!

amathxbt commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Mar 31, 2026

Uh oh!

amathxbt commented Mar 31, 2026

Uh oh!

coderabbitai Bot commented Mar 31, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

amathxbt commented Mar 19, 2026 •

edited by Thaleszh

Loading

🔴 Fix 1: Division-by-Zero Chain Halt in `CalculateReward` (Closes #267)

coderabbitai Bot commented Mar 19, 2026 •

edited

Loading

amathxbt commented Mar 31, 2026 •

edited

Loading