feat: finality calculator#6785
Merged
hanabi1224 merged 5 commits intomainfrom Mar 25, 2026
Merged
Conversation
hanabi1224
requested review from
LesnyRumcajs and
sudo-shashank
and removed request for
a team
Contributor
WalkthroughAdds the FRC-0089 EC finality calculator: new calculator module (Poisson + Skellam math), Skellam PMF helper, comprehensive tests, Cargo dependency additions, and integration into the Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Calc as "calc_validator_prob"
participant Poisson as "Poisson Helpers"
participant Skellam as "Skellam PMF"
participant Normalize as "Truncate & Normalize"
participant Aggregate as "Cumulative Aggregation"
Client->>Calc: call(chain, finality, blocks_per_epoch, byzantine_fraction, current_epoch, target_epoch)
activate Calc
Calc->>Poisson: compute L (adversarial lead) and B (settlement window) distributions
Poisson-->>Calc: L, B arrays
Calc->>Skellam: compute M (future advantage) via Skellam PMF
Skellam-->>Calc: M array
Calc->>Normalize: truncate & renormalize L, B, M distributions
Normalize-->>Calc: normalized distributions
Calc->>Aggregate: nested cumulative sums / convolution-like aggregation
Aggregate-->>Calc: upper-bound reorg probability (clamped to ≤1.0)
Calc-->>Client: Result<f64> (probability)
deactivate Calc
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/chain/ec_finality/calculator/mod.rs`:
- Around line 223-269: The bisection using (mut low, mut high) with
calc_validator_prob is unsound because calc_validator_prob(depth) is not
strictly monotone; replace the binary search in this function with a forward
linear scan from low = BISECT_LOW (clamped by current_epoch) up to high =
BISECT_HIGH.min(current_epoch), inclusive, calling calc_validator_prob for each
depth (using current_epoch - depth) and return the first depth where prob <
guarantee, otherwise return -1; also remove the early return on low >= high and
ensure the single-depth case (low == high) is checked rather than rejected.
- Around line 62-64: The code truncates blocks_per_epoch by using an integer
cast when computing max_k_b, shrinking support for non-integer block rates;
update the computation to perform the arithmetic in f64 and use ceil() before
casting to usize (e.g., compute ((current_epoch - target_epoch) as f64 *
blocks_per_epoch).ceil() as usize) and apply the same ceil-based fix at the
other identical location where blocks_per_epoch is cast to an integer; keep the
variable names (max_k_b, current_epoch, target_epoch, blocks_per_epoch) to
locate and replace the truncating cast.
In `@src/chain/ec_finality/calculator/skellam.rs`:
- Around line 20-25: The guard in skellam_pmf incorrectly treats mu1==0 or
mu2==0 as invalid; instead handle degenerate Poisson branches explicitly in
skellam_pmf (and the similar logic around lines 95-100) so only negative or
non-finite mus return NaN. Concretely: keep the NaN checks for mu1 or mu2 being
negative or non-finite, but add branches: if mu1==0.0 && mu2==0.0 return 1.0 if
k==0.0 else 0.0; if mu1==0.0 compute the Poisson PMF for N2 with n = -k (integer
check) and return that probability; if mu2==0.0 compute the Poisson PMF for N1
with n = k (integer check) and return that probability. Use or add a helper
poisson_pmf(n: u64, mu: f64) to avoid duplicating formulas and ensure
integer-ness of k before mapping to n.
In `@src/chain/ec_finality/mod.rs`:
- Line 4: The calculator submodule (calculator) is declared but its API isn't
exposed; update this module to publicly re-export the calculator API (e.g.,
expose the module and/or its public symbols from ec_finality by adding a pub use
or pub mod re-export for calculator and selectively pub use the
functions/constants you need such as the calculator types/constants and
functions referenced) and also export the ec_finality module from the parent
chain module (add a public re-export of ec_finality or its key symbols in the
parent module so external code can reach calculator via
ec_finality::calculator); ensure you reference the existing module name
calculator and the enclosing module ec_finality when adding the re-exports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2b369bdb-6006-4463-9220-15b20ac58dc3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.config/forest.dicCargo.tomlsrc/chain/ec_finality/calculator/mod.rssrc/chain/ec_finality/calculator/skellam.rssrc/chain/ec_finality/calculator/tests.rssrc/chain/ec_finality/mod.rssrc/chain/mod.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/chain/ec_finality/calculator/mod.rs`:
- Line 60: Rename the constant NELIGIBLE_THRESHOLD to NEGLIGIBLE_THRESHOLD in
the declaration and update every usage in this module to the new name (replace
all occurrences of NELIGIBLE_THRESHOLD with NEGLIGIBLE_THRESHOLD) so the
identifier spelling is correct and references compile; ensure any unit tests or
internal matches that reference the old name are updated as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f3556281-c34d-4dd8-b25f-9e60b6bd5189
📒 Files selected for processing (1)
src/chain/ec_finality/calculator/mod.rs
Member
|
@hanabi1224 I think it should be relatively easy to cover the remaining LoC with tests; those are some edge cases, but easily constructible. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/chain/ec_finality/calculator/tests.rs (1)
223-227: Add zero-length chain case to complete short-input coverage.The short-chain parameterization currently starts at length 1. Adding length 0 would guard against accidental empty-slice regressions in
find_threshold_depth.Proposed test tweak
#[rstest] +#[case(0)] #[case(4)] #[case(3)] #[case(2)] #[case(1)] fn test_find_threshold_depth_too_short_chain(#[case] chain_len: usize) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chain/ec_finality/calculator/tests.rs` around lines 223 - 227, Add a zero-length case to the parameterized short-chain test by adding #[case(0)] to the test_find_threshold_depth_too_short_chain parameterization so the test invokes find_threshold_depth with chain_len == 0; update any test setup inside test_find_threshold_depth_too_short_chain that constructs the slice/vector for the chain (used by find_threshold_depth) to correctly create an empty slice when chain_len is 0 so the test asserts the expected behavior for empty input.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/chain/ec_finality/calculator/tests.rs`:
- Around line 223-227: Add a zero-length case to the parameterized short-chain
test by adding #[case(0)] to the test_find_threshold_depth_too_short_chain
parameterization so the test invokes find_threshold_depth with chain_len == 0;
update any test setup inside test_find_threshold_depth_too_short_chain that
constructs the slice/vector for the chain (used by find_threshold_depth) to
correctly create an empty slice when chain_len is 0 so the test asserts the
expected behavior for empty input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1def13c3-df20-416d-990b-96afa74494ce
📒 Files selected for processing (2)
src/chain/ec_finality/calculator/skellam.rssrc/chain/ec_finality/calculator/tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/chain/ec_finality/calculator/skellam.rs
LesnyRumcajs
approved these changes
Mar 25, 2026
Member
LesnyRumcajs
left a comment
LesnyRumcajs
left a comment
There was a problem hiding this comment.
I believe in this math and do not believe in God. In both cases, I might be wrong.
Contributor
Author
@LesnyRumcajs all edge cases are now covered. 4 left missing lines are workaround for slice indexing clippy warnings, which cannot be tested as they are infallible |
This was referenced Apr 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #6767
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
New Features
Tests
Chores