From e0e79fdee0834109923afdf9f309330cf5eac1c1 Mon Sep 17 00:00:00 2001 From: Vladimir Komendantskiy Date: Mon, 28 Oct 2019 13:39:18 +0000 Subject: [PATCH] Step duration map configuration parameter ported from the POA Network fork (#10902) * step duration map configuration parameter ported from POA Network fork * step duration map refactoring * added a test of step duration change * refactoring of vector search; return Err instead of panicking * removed dead code and the Config engine error variant * doc correction * converted triples to struct StepDurationInfo --- ethcore/engines/authority-round/src/lib.rs | 280 ++++++++++++++++----- json/src/spec/authority_round.rs | 8 +- json/src/spec/mod.rs | 2 + json/src/spec/step_duration.rs | 36 +++ 4 files changed, 254 insertions(+), 72 deletions(-) create mode 100644 json/src/spec/step_duration.rs diff --git a/ethcore/engines/authority-round/src/lib.rs b/ethcore/engines/authority-round/src/lib.rs index fd3585fc7..54980e4f4 100644 --- a/ethcore/engines/authority-round/src/lib.rs +++ b/ethcore/engines/authority-round/src/lib.rs @@ -33,11 +33,12 @@ use std::collections::{BTreeMap, BTreeSet, HashSet}; use std::{cmp, fmt}; -use std::iter::FromIterator; +use std::iter::{self, FromIterator}; use std::ops::Deref; -use std::sync::atomic::{AtomicUsize, AtomicBool, Ordering as AtomicOrdering}; +use std::sync::atomic::{AtomicU64, AtomicBool, Ordering as AtomicOrdering}; use std::sync::{Weak, Arc}; use std::time::{UNIX_EPOCH, Duration}; +use std::u64; use client_traits::EngineClient; use engine::{Engine, ConstructedVerifier}; @@ -83,12 +84,13 @@ use self::finality::RollingFinality; /// `AuthorityRound` params. pub struct AuthorityRoundParams { - /// Time to wait before next block or authority switching, - /// in seconds. + /// A map defining intervals of blocks with the given times (in seconds) to wait before next + /// block or authority switching. The keys in the map are steps of starting blocks of those + /// periods. The entry at `0` should be defined. /// - /// Deliberately typed as u16 as too high of a value leads - /// to slow block issuance. - pub step_duration: u16, + /// Wait times (durations) are additionally required to be less than 65535 since larger values + /// lead to slow block issuance. + pub step_durations: BTreeMap, /// Starting step, pub start_step: Option, /// Valid validators. @@ -121,11 +123,27 @@ const U16_MAX: usize = ::std::u16::MAX as usize; impl From for AuthorityRoundParams { fn from(p: ethjson::spec::AuthorityRoundParams) -> Self { - let mut step_duration_usize: usize = p.step_duration.into(); - if step_duration_usize > U16_MAX { - step_duration_usize = U16_MAX; - warn!(target: "engine", "step_duration is too high ({}), setting it to {}", step_duration_usize, U16_MAX); - } + let map_step_duration = |u: ethjson::uint::Uint| { + let mut step_duration_usize: usize = u.into(); + if step_duration_usize == 0 { + panic!("AuthorityRoundParams: step duration cannot be 0"); + } + if step_duration_usize > U16_MAX { + warn!(target: "engine", "step duration is too high ({}), setting it to {}", step_duration_usize, U16_MAX); + step_duration_usize = U16_MAX; + } + step_duration_usize as u64 + }; + let step_durations: BTreeMap<_, _> = match p.step_duration { + ethjson::spec::StepDuration::Single(u) => + iter::once((0, map_step_duration(u))).collect(), + ethjson::spec::StepDuration::Transitions(tr) => { + if tr.is_empty() { + panic!("AuthorityRoundParams: step duration transitions cannot be empty"); + } + tr.into_iter().map(|(timestamp, u)| (timestamp.into(), map_step_duration(u))).collect() + } + }; let transition_block_num = p.block_reward_contract_transition.map_or(0, Into::into); let mut br_transitions: BTreeMap<_, _> = p.block_reward_contract_transitions .unwrap_or_default() @@ -151,7 +169,7 @@ impl From for AuthorityRoundParams { ); } AuthorityRoundParams { - step_duration: step_duration_usize as u16, + step_durations, validators: new_validator_set(p.validators), start_step: p.start_step.map(Into::into), validate_score_transition: p.validate_score_transition.map_or(0, Into::into), @@ -169,54 +187,89 @@ impl From for AuthorityRoundParams { } } -// Helper for managing the step. +/// A triple containing the first step number and the starting timestamp of the given step duration. +#[derive(Clone, Debug)] +struct StepDurationInfo { + transition_step: u64, + transition_timestamp: u64, + step_duration: u64, +} + +/// Helper for managing the step. #[derive(Debug)] struct Step { calibrate: bool, // whether calibration is enabled. - inner: AtomicUsize, - duration: u16, + inner: AtomicU64, + /// Planned durations of steps. + durations: Vec, } impl Step { - fn load(&self) -> u64 { self.inner.load(AtomicOrdering::SeqCst) as u64 } + fn load(&self) -> u64 { self.inner.load(AtomicOrdering::SeqCst) } + + /// Finds the remaining duration of the current step. Panics if there was a counter under- or + /// overflow. fn duration_remaining(&self) -> Duration { - let now = unix_now(); - let expected_seconds = self.load() - .checked_add(1) - .and_then(|ctr| ctr.checked_mul(self.duration as u64)) - .map(Duration::from_secs); - - match expected_seconds { - Some(step_end) if step_end > now => step_end - now, - Some(_) => Duration::from_secs(0), - None => { - let ctr = self.load(); - error!(target: "engine", "Step counter is too high: {}, aborting", ctr); - panic!("step counter is too high: {}", ctr) - }, - } - + self.opt_duration_remaining().unwrap_or_else(|| { + let ctr = self.load(); + error!(target: "engine", "Step counter under- or overflow: {}, aborting", ctr); + panic!("step counter under- or overflow: {}", ctr) + }) } + /// Finds the remaining duration of the current step. Returns `None` if there was a counter + /// under- or overflow. + fn opt_duration_remaining(&self) -> Option { + let next_step = self.load().checked_add(1)?; + let StepDurationInfo { transition_step, transition_timestamp, step_duration } = + self.durations.iter() + .take_while(|info| info.transition_step < next_step) + .last() + .expect("durations cannot be empty") + .clone(); + let next_time = transition_timestamp + .checked_add(next_step.checked_sub(transition_step)?.checked_mul(step_duration)?)?; + Some(Duration::from_secs(next_time.saturating_sub(unix_now().as_secs()))) + } + + /// Increments the step number. + /// + /// Panics if the new step number is `u64::MAX`. fn increment(&self) { - use std::usize; // fetch_add won't panic on overflow but will rather wrap // around, leading to zero as the step counter, which might // lead to unexpected situations, so it's better to shut down. - if self.inner.fetch_add(1, AtomicOrdering::SeqCst) == usize::MAX { - error!(target: "engine", "Step counter is too high: {}, aborting", usize::MAX); - panic!("step counter is too high: {}", usize::MAX); + if self.inner.fetch_add(1, AtomicOrdering::SeqCst) == u64::MAX { + error!(target: "engine", "Step counter is too high: {}, aborting", u64::MAX); + panic!("step counter is too high: {}", u64::MAX); } - } fn calibrate(&self) { if self.calibrate { - let new_step = unix_now().as_secs() / (self.duration as u64); - self.inner.store(new_step as usize, AtomicOrdering::SeqCst); + if self.opt_calibrate().is_none() { + let ctr = self.load(); + error!(target: "engine", "Step counter under- or overflow: {}, aborting", ctr); + panic!("step counter under- or overflow: {}", ctr) + } } } + /// Calibrates the AuRa step number according to the current time. + fn opt_calibrate(&self) -> Option<()> { + let now = unix_now().as_secs(); + let StepDurationInfo { transition_step, transition_timestamp, step_duration } = + self.durations.iter() + .take_while(|info| info.transition_timestamp < now) + .last() + .expect("durations cannot be empty") + .clone(); + let new_step = (now.checked_sub(transition_timestamp)? / step_duration) + .checked_add(transition_step)?; + self.inner.store(new_step, AtomicOrdering::SeqCst); + Some(()) + } + fn check_future(&self, given: u64) -> Result<(), Option>> { const REJECTED_STEP_DRIFT: u64 = 4; @@ -234,7 +287,9 @@ impl Step { Err(None) // wait a bit for blocks in near future } else if given > current { - let d = self.duration as u64; + let d = self.durations.iter().take_while(|info| info.transition_step <= current).last() + .expect("Duration map has at least a 0 entry.") + .step_duration; Err(Some(OutOfBounds { min: None, max: Some(d * current), @@ -730,23 +785,54 @@ impl<'a, A: ?Sized, B> Deref for CowLike<'a, A, B> where B: AsRef { impl AuthorityRound { /// Create a new instance of AuthorityRound engine. pub fn new(our_params: AuthorityRoundParams, machine: Machine) -> Result, Error> { - if our_params.step_duration == 0 { - error!(target: "engine", "Authority Round step duration can't be zero, aborting"); - panic!("authority_round: step duration can't be zero") + if !our_params.step_durations.contains_key(&0) { + error!(target: "engine", "Authority Round step 0 duration is undefined, aborting"); + return Err(Error::Engine(EngineError::Custom(String::from("step 0 duration is undefined")))); + } + if our_params.step_durations.values().any(|v| *v == 0) { + error!(target: "engine", "Authority Round step duration cannot be 0"); + return Err(Error::Engine(EngineError::Custom(String::from("step duration cannot be 0")))); } let should_timeout = our_params.start_step.is_none(); - let initial_step = our_params.start_step.unwrap_or_else(|| (unix_now().as_secs() / (our_params.step_duration as u64))); + let initial_step = our_params.start_step.unwrap_or(0); + + let mut durations = Vec::new(); + let mut prev_step = 0u64; + let mut prev_time = 0u64; + let mut prev_dur = our_params.step_durations[&0]; + durations.push(StepDurationInfo { + transition_step: prev_step, + transition_timestamp: prev_time, + step_duration: prev_dur + }); + for (time, dur) in our_params.step_durations.iter().skip(1) { + let (step, time) = next_step_time_duration( + StepDurationInfo{ + transition_step: prev_step, + transition_timestamp: prev_time, + step_duration: prev_dur, + }, *time) + .ok_or(BlockError::TimestampOverflow)?; + durations.push(StepDurationInfo { + transition_step: step, + transition_timestamp: time, + step_duration: *dur + }); + prev_step = step; + prev_time = time; + prev_dur = *dur; + } + + let step = Step { + inner: AtomicU64::new(initial_step), + calibrate: our_params.start_step.is_none(), + durations, + }; + step.calibrate(); let engine = Arc::new( AuthorityRound { transition_service: IoService::<()>::start()?, - step: Arc::new(PermissionedStep { - inner: Step { - inner: AtomicUsize::new(initial_step as usize), - calibrate: our_params.start_step.is_none(), - duration: our_params.step_duration, - }, - can_propose: AtomicBool::new(true), - }), + step: Arc::new(PermissionedStep { inner: step, can_propose: AtomicBool::new(true) }), client: Arc::new(RwLock::new(None)), signer: RwLock::new(None), validators: our_params.validators, @@ -994,8 +1080,10 @@ impl IoHandler<()> for TransitionHandler { } } - let next_run_at = AsMillis::as_millis(&self.step.inner.duration_remaining()) >> 2; - io.register_timer_once(ENGINE_TIMEOUT_TOKEN, Duration::from_millis(next_run_at)) + let next_run_at = Duration::from_millis( + AsMillis::as_millis(&self.step.inner.duration_remaining()) >> 2 + ); + io.register_timer_once(ENGINE_TIMEOUT_TOKEN, next_run_at) .unwrap_or_else(|e| warn!(target: "engine", "Failed to restart consensus step timer: {}.", e)) } } @@ -1694,12 +1782,27 @@ impl Engine for AuthorityRound { } } +/// A helper accumulator function mapping a step duration and a step duration transition timestamp +/// to the corresponding step number and the correct starting second of the step. +fn next_step_time_duration(info: StepDurationInfo, time: u64) -> Option<(u64, u64)> +{ + let step_diff = time.checked_add(info.step_duration)? + .checked_sub(1)? + .checked_sub(info.transition_timestamp)? + .checked_div(info.step_duration)?; + Some(( + info.transition_step.checked_add(step_diff)?, + step_diff.checked_mul(info.step_duration)?.checked_add(time)?, + )) +} + #[cfg(test)] mod tests { use std::collections::BTreeMap; use std::str::FromStr; use std::sync::Arc; - use std::sync::atomic::{AtomicUsize, Ordering as AtomicOrdering}; + use std::sync::atomic::{AtomicUsize, AtomicU64, Ordering as AtomicOrdering}; + use std::time::Duration; use keccak_hash::keccak; use accounts::AccountProvider; use ethereum_types::{Address, H520, H256, U256}; @@ -1726,13 +1829,16 @@ mod tests { use ethjson; use serde_json; - use super::{AuthorityRoundParams, AuthorityRound, EmptyStep, SealedEmptyStep, calculate_score}; + use super::{ + AuthorityRoundParams, AuthorityRound, EmptyStep, SealedEmptyStep, StepDurationInfo, + calculate_score, + }; fn build_aura(f: F) -> Arc where F: FnOnce(&mut AuthorityRoundParams), { let mut params = AuthorityRoundParams { - step_duration: 1, + step_durations: [(0, 1)].to_vec().into_iter().collect(), start_step: Some(1), validators: Box::new(TestSet::default()), validate_score_transition: 0, @@ -2061,29 +2167,67 @@ mod tests { use super::Step; let step = Step { calibrate: false, - inner: AtomicUsize::new(::std::usize::MAX), - duration: 1, + inner: AtomicU64::new(::std::u64::MAX), + durations: [StepDurationInfo { + transition_step: 0, + transition_timestamp: 0, + step_duration: 1, + }].to_vec().into_iter().collect(), }; step.increment(); } #[test] - #[should_panic(expected="counter is too high")] + #[should_panic(expected="step counter under- or overflow")] fn test_counter_duration_remaining_too_high() { use super::Step; let step = Step { calibrate: false, - inner: AtomicUsize::new(::std::usize::MAX), - duration: 1, + inner: AtomicU64::new(::std::u64::MAX), + durations: [StepDurationInfo { + transition_step: 0, + transition_timestamp: 0, + step_duration: 1, + }].to_vec().into_iter().collect(), }; step.duration_remaining(); } #[test] - #[should_panic(expected="authority_round: step duration can't be zero")] + fn test_change_step_duration() { + use super::Step; + use std::thread; + + let now = super::unix_now().as_secs(); + let step = Step { + calibrate: true, + inner: AtomicU64::new(::std::u64::MAX), + durations: [ + StepDurationInfo { transition_step: 0, transition_timestamp: 0, step_duration: 1 }, + StepDurationInfo { transition_step: now, transition_timestamp: now, step_duration: 2 }, + StepDurationInfo { transition_step: now + 1, transition_timestamp: now + 2, step_duration: 4 }, + ].to_vec().into_iter().collect(), + }; + // calibrated step `now` + step.calibrate(); + let duration_remaining = step.duration_remaining(); + assert_eq!(step.inner.load(AtomicOrdering::SeqCst), now); + assert!(duration_remaining <= Duration::from_secs(2)); + thread::sleep(duration_remaining); + step.increment(); + // calibrated step `now + 1` + step.calibrate(); + let duration_remaining = step.duration_remaining(); + assert_eq!(step.inner.load(AtomicOrdering::SeqCst), now + 1); + assert!(duration_remaining > Duration::from_secs(2)); + assert!(duration_remaining <= Duration::from_secs(4)); + } + + #[test] + #[should_panic(expected="called `Result::unwrap()` on an `Err` value: Engine(Custom(\"step duration cannot be 0\"))")] fn test_step_duration_zero() { build_aura(|params| { - params.step_duration = 0; + params.step_durations = [(0, 0)].to_vec().into_iter().collect(); }); } @@ -2473,7 +2617,7 @@ mod tests { #[test] fn test_empty_steps() { let engine = build_aura(|p| { - p.step_duration = 4; + p.step_durations = [(0, 4)].to_vec().into_iter().collect(); p.empty_steps_transition = 0; p.maximum_empty_steps = 0; }); @@ -2507,7 +2651,7 @@ mod tests { let (_spec, tap, accounts) = setup_empty_steps(); let engine = build_aura(|p| { p.validators = Box::new(SimpleList::new(accounts.clone())); - p.step_duration = 4; + p.step_durations = [(0, 4)].to_vec().into_iter().collect(); p.empty_steps_transition = 0; p.maximum_empty_steps = 0; }); @@ -2544,7 +2688,7 @@ mod tests { let (_spec, tap, accounts) = setup_empty_steps(); let engine = build_aura(|p| { p.validators = Box::new(SimpleList::new(accounts.clone())); - p.step_duration = 4; + p.step_durations = [(0, 4)].to_vec().into_iter().collect(); p.empty_steps_transition = 0; p.maximum_empty_steps = 0; }); diff --git a/json/src/spec/authority_round.rs b/json/src/spec/authority_round.rs index 0a31203f6..ba5ad63b3 100644 --- a/json/src/spec/authority_round.rs +++ b/json/src/spec/authority_round.rs @@ -41,7 +41,7 @@ use std::collections::BTreeMap; use crate::{bytes::Bytes, hash::Address, uint::Uint}; use serde::Deserialize; -use super::ValidatorSet; +use super::{StepDuration, ValidatorSet}; /// Authority params deserialization. #[derive(Debug, PartialEq, Deserialize)] @@ -49,7 +49,7 @@ use super::ValidatorSet; #[serde(rename_all = "camelCase")] pub struct AuthorityRoundParams { /// Block duration, in seconds. - pub step_duration: Uint, + pub step_duration: StepDuration, /// Valid authorities pub validators: ValidatorSet, /// Starting step. Determined automatically if not specified. @@ -107,7 +107,7 @@ pub struct AuthorityRound { #[cfg(test)] mod tests { - use super::{Address, Uint}; + use super::{Address, Uint, StepDuration}; use ethereum_types::{U256, H160}; use crate::spec::{validator_set::ValidatorSet, authority_round::AuthorityRound}; use std::str::FromStr; @@ -129,7 +129,7 @@ mod tests { }"#; let deserialized: AuthorityRound = serde_json::from_str(s).unwrap(); - assert_eq!(deserialized.params.step_duration, Uint(U256::from(0x02))); + assert_eq!(deserialized.params.step_duration, StepDuration::Single(Uint(U256::from(2)))); assert_eq!( deserialized.params.validators, ValidatorSet::List(vec![Address(H160::from_str("c6d9d2cd449a754c494264e1809c50e34d64562b").unwrap())]), diff --git a/json/src/spec/mod.rs b/json/src/spec/mod.rs index e2569c286..75b801455 100644 --- a/json/src/spec/mod.rs +++ b/json/src/spec/mod.rs @@ -32,6 +32,7 @@ pub mod null_engine; pub mod instant_seal; pub mod hardcoded_sync; pub mod clique; +pub mod step_duration; pub use self::account::Account; pub use self::builtin::{Builtin, Pricing, Linear}; @@ -49,3 +50,4 @@ pub use self::clique::{Clique, CliqueParams}; pub use self::null_engine::{NullEngine, NullEngineParams}; pub use self::instant_seal::{InstantSeal, InstantSealParams}; pub use self::hardcoded_sync::HardcodedSync; +pub use self::step_duration::StepDuration; diff --git a/json/src/spec/step_duration.rs b/json/src/spec/step_duration.rs new file mode 100644 index 000000000..628e2dd7d --- /dev/null +++ b/json/src/spec/step_duration.rs @@ -0,0 +1,36 @@ +// Copyright 2015-2019 Parity Technologies (UK) Ltd. +// This file is part of Parity. + +// Parity is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Parity is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Parity. If not, see . + +//! Step duration configuration parameter + +use std::collections::BTreeMap; + +use serde::Deserialize; + +use crate::uint::Uint; + +/// Step duration can be specified either as a `Uint` (in seconds), in which case it will be +/// constant, or as a list of pairs consisting of a timestamp of type `Uint` and a duration, in +/// which case the duration of a step will be determined by a mapping arising from that list. +#[derive(Debug, PartialEq, Deserialize)] +#[serde(deny_unknown_fields)] +#[serde(untagged)] +pub enum StepDuration { + /// Duration of all steps. + Single(Uint), + /// Step duration transitions: a mapping of timestamp to step durations. + Transitions(BTreeMap), +}