From b708f8c6349ba3ce4d38d2dd59d4c8e2afc1dfa6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Thu, 23 Apr 2020 18:09:31 +0100 Subject: [PATCH 1/6] babe: remove error fallbacks from threshold calculation --- client/consensus/babe/src/authorship.rs | 35 +++++++++++++++++++------ 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/client/consensus/babe/src/authorship.rs b/client/consensus/babe/src/authorship.rs index 074e582bff252..d3f96ba99ade6 100644 --- a/client/consensus/babe/src/authorship.rs +++ b/client/consensus/babe/src/authorship.rs @@ -47,14 +47,33 @@ pub(super) fn calculate_primary_threshold( authorities[authority_index].1 as f64 / authorities.iter().map(|(_, weight)| weight).sum::() as f64; - let calc = || { - let p = BigRational::from_float(1f64 - (1f64 - c).powf(theta))?; - let numer = p.numer().to_biguint()?; - let denom = p.denom().to_biguint()?; - ((BigUint::one() << 128) * numer / denom).to_u128() - }; - - calc().unwrap_or(u128::max_value()) + assert!(theta > 0, "authority with weight 0."); + + let p = BigRational::from_float(1f64 - (1f64 - c).powf(theta)).expect( + "returns None when the given value is not finite; \ + c is a configuration parameter defined in (0, 1]; \ + theta must be > 0 if the given authority's weight is > 0; \ + theta represents the validator's relative weight defined in (0, 1]; \ + powf will always return values in (0, 1] given both the \ + base and exponent are in that domain; \ + qed."; + ); + + let numer = p.numer().to_biguint().expect( + "returns None when the given value is negative; \ + p is defined as `1 - n` where n is defined in (0, 1]; + p must be a value in (0, 1]; \ + qed." + ); + + let denom = p.denom().to_biguint().expect( + "returns None when the given value is negative; \ + p is defined as `1 - n` where n is defined in (0, 1]; + p must be a value in (0, 1]; \ + qed." + ); + + ((BigUint::one() << 128) * numer / denom).to_u128() } /// Returns true if the given VRF output is lower than the given threshold, From 7111f3180d2a7e555e297dc16451146251d87150 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Thu, 23 Apr 2020 18:15:11 +0100 Subject: [PATCH 2/6] babe: fix indent --- client/consensus/babe/src/authorship.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/client/consensus/babe/src/authorship.rs b/client/consensus/babe/src/authorship.rs index d3f96ba99ade6..4918169bf118a 100644 --- a/client/consensus/babe/src/authorship.rs +++ b/client/consensus/babe/src/authorship.rs @@ -61,16 +61,16 @@ pub(super) fn calculate_primary_threshold( let numer = p.numer().to_biguint().expect( "returns None when the given value is negative; \ - p is defined as `1 - n` where n is defined in (0, 1]; - p must be a value in (0, 1]; \ - qed." + p is defined as `1 - n` where n is defined in (0, 1]; \ + p must be a value in (0, 1]; \ + qed." ); let denom = p.denom().to_biguint().expect( "returns None when the given value is negative; \ - p is defined as `1 - n` where n is defined in (0, 1]; - p must be a value in (0, 1]; \ - qed." + p is defined as `1 - n` where n is defined in (0, 1]; \ + p must be a value in (0, 1]; \ + qed." ); ((BigUint::one() << 128) * numer / denom).to_u128() From e129d382b33bc46f3da262585dbc5e351b76f47b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Thu, 23 Apr 2020 18:18:02 +0100 Subject: [PATCH 3/6] babe: fix domain --- client/consensus/babe/src/authorship.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client/consensus/babe/src/authorship.rs b/client/consensus/babe/src/authorship.rs index 4918169bf118a..30db5f4be177b 100644 --- a/client/consensus/babe/src/authorship.rs +++ b/client/consensus/babe/src/authorship.rs @@ -62,14 +62,14 @@ pub(super) fn calculate_primary_threshold( let numer = p.numer().to_biguint().expect( "returns None when the given value is negative; \ p is defined as `1 - n` where n is defined in (0, 1]; \ - p must be a value in (0, 1]; \ + p must be a value in [0, 1); \ qed." ); let denom = p.denom().to_biguint().expect( "returns None when the given value is negative; \ p is defined as `1 - n` where n is defined in (0, 1]; \ - p must be a value in (0, 1]; \ + p must be a value in [0, 1); \ qed." ); From 3323d8842620891c5acf9db139a15999b1abbf11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Thu, 23 Apr 2020 19:03:04 +0100 Subject: [PATCH 4/6] babe: fix compilation --- client/consensus/babe/src/authorship.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/client/consensus/babe/src/authorship.rs b/client/consensus/babe/src/authorship.rs index 30db5f4be177b..b72503b52b237 100644 --- a/client/consensus/babe/src/authorship.rs +++ b/client/consensus/babe/src/authorship.rs @@ -47,7 +47,7 @@ pub(super) fn calculate_primary_threshold( authorities[authority_index].1 as f64 / authorities.iter().map(|(_, weight)| weight).sum::() as f64; - assert!(theta > 0, "authority with weight 0."); + assert!(theta > 0.0, "authority with weight 0."); let p = BigRational::from_float(1f64 - (1f64 - c).powf(theta)).expect( "returns None when the given value is not finite; \ @@ -56,7 +56,7 @@ pub(super) fn calculate_primary_threshold( theta represents the validator's relative weight defined in (0, 1]; \ powf will always return values in (0, 1] given both the \ base and exponent are in that domain; \ - qed."; + qed.", ); let numer = p.numer().to_biguint().expect( @@ -73,7 +73,13 @@ pub(super) fn calculate_primary_threshold( qed." ); - ((BigUint::one() << 128) * numer / denom).to_u128() + ((BigUint::one() << 128) * numer / denom).to_u128().expect( + "returns None if the underlying value cannot be represented with 128 bits; \ + we start with 1 << 128 which is one unit more than can be represented with 128 bits; \ + we multiple by p which is defined in [0, 1); \ + the result must be lower than 1 << 128 and representable with 128 bits; \ + qed.", + ) } /// Returns true if the given VRF output is lower than the given threshold, From 7060562ed3bcad97f6568fbf0e57d9abf2fb26f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Thu, 23 Apr 2020 19:11:31 +0100 Subject: [PATCH 5/6] babe: improve expect --- client/consensus/babe/src/authorship.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client/consensus/babe/src/authorship.rs b/client/consensus/babe/src/authorship.rs index b72503b52b237..3e7ff4cdd2aa0 100644 --- a/client/consensus/babe/src/authorship.rs +++ b/client/consensus/babe/src/authorship.rs @@ -75,9 +75,9 @@ pub(super) fn calculate_primary_threshold( ((BigUint::one() << 128) * numer / denom).to_u128().expect( "returns None if the underlying value cannot be represented with 128 bits; \ - we start with 1 << 128 which is one unit more than can be represented with 128 bits; \ + we start with 2^128 which is one more than can be represented with 128 bits; \ we multiple by p which is defined in [0, 1); \ - the result must be lower than 1 << 128 and representable with 128 bits; \ + the result must be lower than 2^128 by at least one and thus representable with 128 bits; \ qed.", ) } From 21d452a7591f72377aa04a2bfe346d636d07173b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Thu, 23 Apr 2020 20:29:43 +0100 Subject: [PATCH 6/6] babe: add one more note about p's limit --- client/consensus/babe/src/authorship.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/client/consensus/babe/src/authorship.rs b/client/consensus/babe/src/authorship.rs index 3e7ff4cdd2aa0..cdd4e656788db 100644 --- a/client/consensus/babe/src/authorship.rs +++ b/client/consensus/babe/src/authorship.rs @@ -49,6 +49,11 @@ pub(super) fn calculate_primary_threshold( assert!(theta > 0.0, "authority with weight 0."); + // NOTE: in the equation `p = 1 - (1 - c)^theta` the value of `p` is always + // capped by `c`. For all pratical purposes `c` should always be set to a + // value < 0.5, as such in the computations below we should never be near + // edge cases like `0.999999`. + let p = BigRational::from_float(1f64 - (1f64 - c).powf(theta)).expect( "returns None when the given value is not finite; \ c is a configuration parameter defined in (0, 1]; \