Fix test_threshold_nonzero #1278
Conversation
dannywillems
force-pushed
the
fix_test_threshold_nonzero
branch
from
c26af63 to
36f28e6
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the test_threshold_nonzero test by replacing an inefficient iterative search algorithm with a binary search approach and expanding test coverage to multiple total currency values. The fix addresses issue #1212 and ensures the test now passes consistently.
- Replaces inefficient iterative search with binary search algorithm for finding threshold transition points
- Adds comprehensive test coverage with multiple total currency scenarios
- Removes the
#[ignore]attribute to enable the test in the test suite
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| /// Binary search to find the exact point where threshold becomes non-zero | ||
| fn binary_search(total_currency: BigInt) -> BigInt { | ||
| let mut low = BigInt::zero(); | ||
| let mut high = total_currency.clone(); |
There was a problem hiding this comment.
[nitpick] Extra space between 'high' and '=' creates inconsistent formatting compared to the line above.
Copilot uses AI. Check for mistakes.
| // Verify we found the exact transition point | ||
| assert!( | ||
| first_non_zero_stake > BigInt::zero() && first_non_zero_stake <= total_currency, | ||
| "First non-zero stake should be between 1 and total_currency for {}", |
There was a problem hiding this comment.
The assertion allows first_non_zero_stake to equal total_currency, but this would mean the entire currency supply has zero threshold, which seems logically incorrect for a VRF threshold test.
| "First non-zero stake should be between 1 and total_currency for {}", | |
| first_non_zero_stake > BigInt::zero() && first_non_zero_stake < total_currency, | |
| "First non-zero stake should be between 1 and total_currency-1 for {}", |
Copilot uses AI. Check for mistakes.
| BigRational::zero(), | ||
| "Threshold should be zero for stake one less than first non-zero for {}", | ||
| description | ||
| ); |
There was a problem hiding this comment.
This subtraction could underflow if first_non_zero_stake is zero, causing a panic. Consider adding a check to ensure first_non_zero_stake > BigInt::zero() before this operation.
| ); | |
| if first_non_zero_stake > BigInt::zero() { | |
| let last_zero_stake = &first_non_zero_stake - BigInt::one(); | |
| let thrs_zero = Threshold::new(last_zero_stake, total_currency.clone()); | |
| assert_eq!( | |
| thrs_zero.threshold_rational, | |
| BigRational::zero(), | |
| "Threshold should be zero for stake one less than first non-zero for {}", | |
| description | |
| ); | |
| } else { | |
| // If first_non_zero_stake is zero, skip this test case or handle appropriately | |
| println!( | |
| "Warning: first_non_zero_stake is zero for {}. Skipping last_zero_stake test.", | |
| description | |
| ); | |
| } |
Copilot uses AI. Check for mistakes.
Closes #1212
test threshold::test::test_threshold_nonzero ... ok
successes:
---- threshold::test::test_threshold_nonzero stdout ----
First non-zero stake: 977918960572 nanoMINA (977.918961 MINA)
First non-zero threshold: 0.000001322071547
First non-zero stake: 1104310162392 nanoMINA (1104.310162 MINA)
First non-zero threshold: 0.000001322071547
First non-zero stake: 953674316407 nanoMINA (953.674316 MINA)
First non-zero threshold: 0.000001322071547
First non-zero stake: 4768371582032 nanoMINA (4768.371582 MINA)
First non-zero threshold: 0.000001322071547
First non-zero stake: 9536743164063 nanoMINA (9536.743164 MINA)
First non-zero threshold: 0.000001322071547
First non-zero stake: 95367431641 nanoMINA (95.367432 MINA)
First non-zero threshold: 0.000001322071547