Skip to content

fix(consensus): remove double record_block(0) at epoch boundary#764

Merged
github-actions[bot] merged 1 commit into
mainfrom
fix/epoch-boundary-double-record-block
Jun 2, 2026
Merged

fix(consensus): remove double record_block(0) at epoch boundary#764
github-actions[bot] merged 1 commit into
mainfrom
fix/epoch-boundary-double-record-block

Conversation

@satyakwok
Copy link
Copy Markdown
Collaborator

@satyakwok satyakwok commented Jun 2, 2026

Summary

CodeRabbit finding on PR #763 — valid.

run_epoch_bookkeeping called self.epoch_manager.record_block(0) before pushing the finished epoch to history. But every call site already invoked epoch_manager.record_block(reward) before entering this method. The extra record_block(0) incremented total_blocks_produced twice for every boundary block, so epoch history carried an inflated block count (+1 per rollover).

Fix: remove the redundant call. The caller's record_block(reward) is sufficient.

What's not fixed

The second CodeRabbit finding (process_unbonding drops releases on transfer failure) is pre-existing behavior, not introduced by PR #763. Fixing it requires changing StakeRegistry::process_unbonding return semantics — deferred to a dedicated PR.

Test plan

  • cargo check -p sentrix-core -D warnings clean

Summary by CodeRabbit

  • Chores
    • Optimized epoch bookkeeping process by removing an unnecessary operation during epoch finalization.

…n_epoch_bookkeeping

Callers (main.rs BFT-finalize, libp2p gossip + batch-sync) already call
epoch_manager.record_block(reward) before run_epoch_bookkeeping. The
extra record_block(0) inside the method incremented total_blocks_produced
twice for every boundary block, inflating epoch history by 1 block per
rollover. Reported by CodeRabbit on PR #763.
@github-actions github-actions Bot enabled auto-merge (squash) June 2, 2026 09:42
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 2, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 04c22612-830b-4ddb-82aa-cefe075e0301

📥 Commits

Reviewing files that changed from the base of the PR and between 69188da and dbb6e4c.

📒 Files selected for processing (1)
  • crates/sentrix-core/src/blockchain.rs
💤 Files with no reviewable changes (1)
  • crates/sentrix-core/src/blockchain.rs

📝 Walkthrough

Walkthrough

This PR removes a single call to epoch_manager.record_block(0) from the Blockchain::run_epoch_bookkeeping method. The call was positioned between computing the total staked balance and finalizing the current epoch by cloning it into history and advancing to the next epoch. After this change, the epoch finalization sequence proceeds directly from stake computation to epoch persistence without the intermediate record operation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • sentrix-labs/sentrix#763: Both PRs modify epoch-boundary logic inside Blockchain::run_epoch_bookkeeping in the same file, with this PR adjusting the epoch finalization sequence while the related PR introduces or calls the epoch-bookkeeping routine from sync/finalize paths.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description addresses the summary requirement but lacks most of the template structure (Scope, Checks, Linked issue, Deploy impact sections missing). Complete the PR description by filling out all required template sections including Scope checkboxes, Checks checklist, Linked issue reference, and Deploy impact section.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: removing a redundant record_block(0) call at the epoch boundary.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/epoch-boundary-double-record-block

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.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 2, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@satyakwok satyakwok self-assigned this Jun 2, 2026
@github-actions github-actions Bot merged commit 7654cac into main Jun 2, 2026
18 checks passed
@satyakwok satyakwok deleted the fix/epoch-boundary-double-record-block branch June 2, 2026 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant