From 1f16a6a41b973bbdd800ce07ac68c6055400a321 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= <123550+andresilva@users.noreply.github.com> Date: Sat, 12 Jun 2021 16:58:36 +0100 Subject: [PATCH] im-online: send heartbeats at a random period (#8819) * im-online: send heartbeats at a random period * support: use permill to represent session progress * im-online: increase probability of heartbeating with session progress * babe, session: fix tests * babe: fix test --- frame/babe/src/lib.rs | 6 +- frame/babe/src/tests.rs | 6 +- frame/im-online/src/lib.rs | 44 +++++++++---- frame/im-online/src/mock.rs | 6 +- frame/im-online/src/tests.rs | 86 +++++++++++++++++++++++++- frame/session/src/lib.rs | 8 +-- frame/session/src/tests.rs | 14 ++--- frame/support/src/traits/validation.rs | 6 +- 8 files changed, 140 insertions(+), 36 deletions(-) diff --git a/frame/babe/src/lib.rs b/frame/babe/src/lib.rs index a0a9e01eaa26c..6ec199925be17 100644 --- a/frame/babe/src/lib.rs +++ b/frame/babe/src/lib.rs @@ -31,7 +31,7 @@ use sp_application_crypto::Public; use sp_runtime::{ generic::DigestItem, traits::{IsMember, One, SaturatedConversion, Saturating, Zero}, - ConsensusEngineId, KeyTypeId, Percent, + ConsensusEngineId, KeyTypeId, Permill, }; use sp_session::{GetSessionNumber, GetValidatorCount}; use sp_std::prelude::*; @@ -848,11 +848,11 @@ impl frame_support::traits::EstimateNextSessionRotation (Option, Weight) { + fn estimate_current_session_progress(_now: T::BlockNumber) -> (Option, Weight) { let elapsed = CurrentSlot::::get().saturating_sub(Self::current_epoch_start()) + 1; ( - Some(Percent::from_rational( + Some(Permill::from_rational( *elapsed, T::EpochDuration::get(), )), diff --git a/frame/babe/src/tests.rs b/frame/babe/src/tests.rs index 6aa80e9697339..dfb398a4f4775 100644 --- a/frame/babe/src/tests.rs +++ b/frame/babe/src/tests.rs @@ -236,10 +236,10 @@ fn can_estimate_current_epoch_progress() { if Babe::estimate_next_session_rotation(i).0.unwrap() - 1 == i { assert_eq!( Babe::estimate_current_session_progress(i).0.unwrap(), - Percent::from_percent(100) + Permill::from_percent(100) ); } else { - assert!(Babe::estimate_current_session_progress(i).0.unwrap() < Percent::from_percent(100)); + assert!(Babe::estimate_current_session_progress(i).0.unwrap() < Permill::from_percent(100)); } } @@ -247,7 +247,7 @@ fn can_estimate_current_epoch_progress() { progress_to_block(4); assert_eq!( Babe::estimate_current_session_progress(4).0.unwrap(), - Percent::from_percent(33), + Permill::from_float(1.0 / 3.0), ); }) } diff --git a/frame/im-online/src/lib.rs b/frame/im-online/src/lib.rs index bddb286fad739..e132f7f929a06 100644 --- a/frame/im-online/src/lib.rs +++ b/frame/im-online/src/lib.rs @@ -81,8 +81,8 @@ use sp_std::prelude::*; use sp_std::convert::TryInto; use sp_runtime::{ offchain::storage::StorageValueRef, - traits::{AtLeast32BitUnsigned, Convert, Saturating}, - Perbill, Percent, RuntimeDebug, + traits::{AtLeast32BitUnsigned, Convert, Saturating, TrailingZeroInput}, + Perbill, Permill, PerThing, RuntimeDebug, SaturatedConversion, }; use sp_staking::{ SessionIndex, @@ -571,23 +571,46 @@ impl Pallet { pub(crate) fn send_heartbeats( block_number: T::BlockNumber, ) -> OffchainResult>> { - const HALF_SESSION: Percent = Percent::from_percent(50); + const START_HEARTBEAT_RANDOM_PERIOD: Permill = Permill::from_percent(10); + const START_HEARTBEAT_FINAL_PERIOD: Permill = Permill::from_percent(80); + + // this should give us a residual probability of 1/SESSION_LENGTH of sending an heartbeat, + // i.e. all heartbeats spread uniformly, over most of the session. as the session progresses + // the probability of sending an heartbeat starts to increase exponentially. + let random_choice = |progress: Permill| { + // given session progress `p` and session length `l` + // the threshold formula is: p^6 + 1/l + let session_length = T::NextSessionRotation::average_session_length(); + let residual = Permill::from_rational(1u32, session_length.saturated_into()); + let threshold: Permill = progress.saturating_pow(6).saturating_add(residual); + + let seed = sp_io::offchain::random_seed(); + let random = ::decode(&mut TrailingZeroInput::new(seed.as_ref())) + .expect("input is padded with zeroes; qed"); + let random = Permill::from_parts(random % Permill::ACCURACY); + + random <= threshold + }; - let too_early = if let (Some(progress), _) = + let should_heartbeat = if let (Some(progress), _) = T::NextSessionRotation::estimate_current_session_progress(block_number) { - // we try to get an estimate of the current session progress first since it - // should provide more accurate results and send the heartbeat if we're halfway - // through the session. - progress < HALF_SESSION + // we try to get an estimate of the current session progress first since it should + // provide more accurate results. we will start an early heartbeat period where we'll + // randomly pick whether to heartbeat. after 80% of the session has elapsed, if we + // haven't sent an heartbeat yet we'll send one unconditionally. the idea is to prevent + // all nodes from sending the heartbeats at the same block and causing a temporary (but + // deterministic) spike in transactions. + progress >= START_HEARTBEAT_FINAL_PERIOD + || progress >= START_HEARTBEAT_RANDOM_PERIOD && random_choice(progress) } else { // otherwise we fallback to using the block number calculated at the beginning // of the session that should roughly correspond to the middle of the session let heartbeat_after = >::get(); - block_number < heartbeat_after + block_number >= heartbeat_after }; - if too_early { + if !should_heartbeat { return Err(OffchainErr::TooEarly); } @@ -607,7 +630,6 @@ impl Pallet { ) } - fn send_single_heartbeat( authority_index: u32, key: T::AuthorityId, diff --git a/frame/im-online/src/mock.rs b/frame/im-online/src/mock.rs index 4f21012abc510..4bc976476a676 100644 --- a/frame/im-online/src/mock.rs +++ b/frame/im-online/src/mock.rs @@ -26,7 +26,7 @@ use pallet_session::historical as pallet_session_historical; use sp_core::H256; use sp_runtime::testing::{Header, TestXt, UintAuthorityId}; use sp_runtime::traits::{BlakeTwo256, ConvertInto, IdentityLookup}; -use sp_runtime::{Perbill, Percent}; +use sp_runtime::{Perbill, Permill}; use sp_staking::{ offence::{OffenceError, ReportOffence}, SessionIndex, @@ -182,7 +182,7 @@ impl pallet_authorship::Config for Runtime { } thread_local! { - pub static MOCK_CURRENT_SESSION_PROGRESS: RefCell>> = RefCell::new(None); + pub static MOCK_CURRENT_SESSION_PROGRESS: RefCell>> = RefCell::new(None); } thread_local! { @@ -199,7 +199,7 @@ impl frame_support::traits::EstimateNextSessionRotation for TestNextSession mock.unwrap_or(pallet_session::PeriodicSessions::::average_session_length()) } - fn estimate_current_session_progress(now: u64) -> (Option, Weight) { + fn estimate_current_session_progress(now: u64) -> (Option, Weight) { let (estimate, weight) = pallet_session::PeriodicSessions::::estimate_current_session_progress( now, diff --git a/frame/im-online/src/tests.rs b/frame/im-online/src/tests.rs index f100bd71c34f6..5fb8fd3a791e9 100644 --- a/frame/im-online/src/tests.rs +++ b/frame/im-online/src/tests.rs @@ -433,10 +433,92 @@ fn should_handle_non_linear_session_progress() { assert!(ImOnline::send_heartbeats(5).ok().is_some()); // if we have a valid current session progress then we'll heartbeat as soon - // as we're past 50% of the session regardless of the block number + // as we're past 80% of the session regardless of the block number MOCK_CURRENT_SESSION_PROGRESS - .with(|p| *p.borrow_mut() = Some(Some(Percent::from_percent(51)))); + .with(|p| *p.borrow_mut() = Some(Some(Permill::from_percent(81)))); assert!(ImOnline::send_heartbeats(2).ok().is_some()); }); } + +#[test] +fn test_does_not_heartbeat_early_in_the_session() { + let mut ext = new_test_ext(); + let (offchain, _state) = TestOffchainExt::new(); + let (pool, _) = TestTransactionPoolExt::new(); + ext.register_extension(OffchainDbExt::new(offchain.clone())); + ext.register_extension(OffchainWorkerExt::new(offchain)); + ext.register_extension(TransactionPoolExt::new(pool)); + + ext.execute_with(|| { + // mock current session progress as being 5%. we only randomly start + // heartbeating after 10% of the session has elapsed. + MOCK_CURRENT_SESSION_PROGRESS.with(|p| *p.borrow_mut() = Some(Some(Permill::from_float(0.05)))); + assert_eq!( + ImOnline::send_heartbeats(2).err(), + Some(OffchainErr::TooEarly), + ); + }); +} + +#[test] +fn test_probability_of_heartbeating_increases_with_session_progress() { + let mut ext = new_test_ext(); + let (offchain, state) = TestOffchainExt::new(); + let (pool, _) = TestTransactionPoolExt::new(); + ext.register_extension(OffchainDbExt::new(offchain.clone())); + ext.register_extension(OffchainWorkerExt::new(offchain)); + ext.register_extension(TransactionPoolExt::new(pool)); + + ext.execute_with(|| { + let set_test = |progress, random: f64| { + // the average session length is 100 blocks, therefore the residual + // probability of sending a heartbeat is 1% + MOCK_AVERAGE_SESSION_LENGTH.with(|p| *p.borrow_mut() = Some(100)); + MOCK_CURRENT_SESSION_PROGRESS.with(|p| *p.borrow_mut() = + Some(Some(Permill::from_float(progress)))); + + let mut seed = [0u8; 32]; + let encoded = ((random * Permill::ACCURACY as f64) as u32).encode(); + seed[0..4].copy_from_slice(&encoded); + state.write().seed = seed; + }; + + let assert_too_early = |progress, random| { + set_test(progress, random); + assert_eq!( + ImOnline::send_heartbeats(2).err(), + Some(OffchainErr::TooEarly), + ); + }; + + let assert_heartbeat_ok = |progress, random| { + set_test(progress, random); + assert!(ImOnline::send_heartbeats(2).ok().is_some()); + }; + + assert_too_early(0.05, 1.0); + + assert_too_early(0.1, 0.1); + assert_too_early(0.1, 0.011); + assert_heartbeat_ok(0.1, 0.010); + + assert_too_early(0.4, 0.015); + assert_heartbeat_ok(0.4, 0.014); + + assert_too_early(0.5, 0.026); + assert_heartbeat_ok(0.5, 0.025); + + assert_too_early(0.6, 0.057); + assert_heartbeat_ok(0.6, 0.056); + + assert_too_early(0.65, 0.086); + assert_heartbeat_ok(0.65, 0.085); + + assert_too_early(0.7, 0.13); + assert_heartbeat_ok(0.7, 0.12); + + assert_too_early(0.75, 0.19); + assert_heartbeat_ok(0.75, 0.18); + }); +} diff --git a/frame/session/src/lib.rs b/frame/session/src/lib.rs index 8574979ef2fea..547d29715d9c1 100644 --- a/frame/session/src/lib.rs +++ b/frame/session/src/lib.rs @@ -118,7 +118,7 @@ use sp_std::{prelude::*, marker::PhantomData, ops::{Sub, Rem}}; use codec::Decode; use sp_runtime::{ traits::{AtLeast32BitUnsigned, Convert, Member, One, OpaqueKeys, Zero}, - KeyTypeId, Perbill, Percent, RuntimeAppPublic, + KeyTypeId, Perbill, Permill, RuntimeAppPublic, }; use sp_staking::SessionIndex; use frame_support::{ @@ -168,7 +168,7 @@ impl< Period::get() } - fn estimate_current_session_progress(now: BlockNumber) -> (Option, Weight) { + fn estimate_current_session_progress(now: BlockNumber) -> (Option, Weight) { let offset = Offset::get(); let period = Period::get(); @@ -177,12 +177,12 @@ impl< // (0% is never returned). let progress = if now >= offset { let current = (now - offset) % period.clone() + One::one(); - Some(Percent::from_rational( + Some(Permill::from_rational( current.clone(), period.clone(), )) } else { - Some(Percent::from_rational( + Some(Permill::from_rational( now + One::one(), offset, )) diff --git a/frame/session/src/tests.rs b/frame/session/src/tests.rs index f48388b5a002c..a551e1a4a2612 100644 --- a/frame/session/src/tests.rs +++ b/frame/session/src/tests.rs @@ -274,11 +274,11 @@ fn periodic_session_works() { if P::estimate_next_session_rotation(i).0.unwrap() - 1 == i { assert_eq!( P::estimate_current_session_progress(i).0.unwrap(), - Percent::from_percent(100) + Permill::from_percent(100) ); } else { assert!( - P::estimate_current_session_progress(i).0.unwrap() < Percent::from_percent(100) + P::estimate_current_session_progress(i).0.unwrap() < Permill::from_percent(100) ); } } @@ -290,7 +290,7 @@ fn periodic_session_works() { assert_eq!(P::estimate_next_session_rotation(3u64).0.unwrap(), 3); assert_eq!( P::estimate_current_session_progress(3u64).0.unwrap(), - Percent::from_percent(10), + Permill::from_percent(10), ); for i in (1u64..10).map(|i| 3 + i) { @@ -302,11 +302,11 @@ fn periodic_session_works() { if P::estimate_next_session_rotation(i).0.unwrap() - 1 == i { assert_eq!( P::estimate_current_session_progress(i).0.unwrap(), - Percent::from_percent(100) + Permill::from_percent(100) ); } else { assert!( - P::estimate_current_session_progress(i).0.unwrap() < Percent::from_percent(100) + P::estimate_current_session_progress(i).0.unwrap() < Permill::from_percent(100) ); } } @@ -316,14 +316,14 @@ fn periodic_session_works() { assert_eq!(P::estimate_next_session_rotation(13u64).0.unwrap(), 23); assert_eq!( P::estimate_current_session_progress(13u64).0.unwrap(), - Percent::from_percent(10) + Permill::from_percent(10) ); assert!(!P::should_end_session(14u64)); assert_eq!(P::estimate_next_session_rotation(14u64).0.unwrap(), 23); assert_eq!( P::estimate_current_session_progress(14u64).0.unwrap(), - Percent::from_percent(20) + Permill::from_percent(20) ); } diff --git a/frame/support/src/traits/validation.rs b/frame/support/src/traits/validation.rs index 900be7bb8e7e2..d0583d6991fe6 100644 --- a/frame/support/src/traits/validation.rs +++ b/frame/support/src/traits/validation.rs @@ -20,7 +20,7 @@ use sp_std::prelude::*; use codec::{Codec, Decode}; use sp_runtime::traits::{Convert, Zero}; -use sp_runtime::{BoundToRuntimeAppPublic, ConsensusEngineId, Percent, RuntimeAppPublic}; +use sp_runtime::{BoundToRuntimeAppPublic, ConsensusEngineId, Permill, RuntimeAppPublic}; use sp_staking::SessionIndex; use crate::dispatch::Parameter; use crate::weights::Weight; @@ -126,7 +126,7 @@ pub trait EstimateNextSessionRotation { /// Return an estimate of the current session progress. /// /// None should be returned if the estimation fails to come to an answer. - fn estimate_current_session_progress(now: BlockNumber) -> (Option, Weight); + fn estimate_current_session_progress(now: BlockNumber) -> (Option, Weight); /// Return the block number at which the next session rotation is estimated to happen. /// @@ -139,7 +139,7 @@ impl EstimateNextSessionRotation for () { Zero::zero() } - fn estimate_current_session_progress(_: BlockNumber) -> (Option, Weight) { + fn estimate_current_session_progress(_: BlockNumber) -> (Option, Weight) { (None, Zero::zero()) }