Skip to content

Fix test_threshold_nonzero #1278

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
cyberbono3 wants to merge 1 commit into from
Closed

Fix test_threshold_nonzero #1278

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 63 additions & 40 deletions vrf/src/threshold.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,56 +225,79 @@ mod test {

use super::*;

// TODO: move to regular fns, rework step
fn first_non_zero(stake: BigInt, total_currency: BigInt, step: BigInt) -> BigInt {
let ten = BigInt::from_str("10").unwrap();
let mut stake = stake;
if step == BigInt::zero() {
stake + BigInt::one()
} else {
loop {
let thrs = Threshold::new(stake.clone(), total_currency.clone());

if thrs.threshold_rational != BigRational::zero() {
println!("stake: {stake} nanoMINA");
return first_non_zero(stake - step.clone(), total_currency, step / ten);
#[test]
fn test_threshold_nonzero() {
/// 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();
Copy link

Copilot AI Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Extra space between 'high' and '=' creates inconsistent formatting compared to the line above.

Copilot uses AI. Check for mistakes.
while low < high {
let mid: BigInt = (&low + &high) / 2;
let thrs = Threshold::new(mid.clone(), total_currency.clone());

if thrs.threshold_rational == BigRational::zero() {
low = mid + BigInt::one();
} else {
high = mid.clone();
}
stake += step.clone();
}
low
}
}

#[test]
#[ignore]
fn test_threshold_nonzero() {
// let total_currency = BigInt::from_str("1157953132840039233").unwrap();
// let initial_stake = BigInt::zero();
// let initial_step = BigInt::from_str("10000000000000000000").unwrap();

let total_currency = BigInt::from_str("1025422352000001000").unwrap();
let initial_stake = BigInt::zero();
let initial_step = BigInt::from_str("10000000000000000000").unwrap();

let first_non_zero_nanomina =
first_non_zero(initial_stake, total_currency.clone(), initial_step);

let last_zero = first_non_zero_nanomina.clone() - BigInt::one();
// Different total currency values to test
let test_cases = vec![
("1025422352000001000", "Test case 1"), // Original value
("1157953132840039233", "Test case 2"), // Another value from comments
("1000000000000000000", "1 billion MINA"), // 1 billion MINA
("5000000000000000000", "5 billion MINA"), // 5 billion MINA
("10000000000000000000", "10 billion MINA"), // 10 billion MINA
("100000000000000000", "100 million MINA"), // 100 million MINA
];

for (total_currency_str, description) in test_cases {
let total_currency = BigInt::from_str(total_currency_str).unwrap();

let first_non_zero_stake = binary_search(total_currency.clone());

// 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 {}",
Copy link

Copilot AI Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
"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.
description
);

let thrs_zero = Threshold::new(last_zero, total_currency.clone());
assert_eq!(thrs_zero.threshold_rational, BigRational::zero());
// Verify the transition point
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
);
Copy link

Copilot AI Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
);
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.

let thrs_first = Threshold::new(first_non_zero_nanomina.clone(), total_currency);
assert!(thrs_first.threshold_rational > BigRational::zero());
let thrs_first = Threshold::new(first_non_zero_stake.clone(), total_currency.clone());
assert!(
thrs_first.threshold_rational > BigRational::zero(),
"Threshold should be non-zero for first non-zero stake for {}",
description
);

let first_non_zero_mina = first_non_zero_nanomina.to_f64().unwrap() / 1_000_000_000.0;
// Convert to MINA for display
let first_non_zero_mina = first_non_zero_stake.to_f64().unwrap() / 1_000_000_000.0;

println!("First non zero stake: {first_non_zero_mina} MINA");
println!(
"First non zero threshold: {}",
thrs_first.threshold_rational.to_f64().unwrap()
);
println!(
"First non-zero stake: {} nanoMINA ({:.6} MINA)",
first_non_zero_stake, first_non_zero_mina
);
println!(
"First non-zero threshold: {:.15}",
thrs_first.threshold_rational.to_f64().unwrap()
);
}
}


#[test]
#[ignore]
fn test_threshold_increase() {
Expand Down