Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Step duration map configuration parameter ported from the POA Network fork #10902

Merged
merged 7 commits into from
Oct 28, 2019

Conversation

vkomenda
Copy link
Contributor

The original PR: poanetwork#152.

Summary: Instead of a single step duration, this PR allows specifying time intervals with given step durations.

@parity-cla-bot
Copy link

It looks like @vkomenda signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@ordian ordian added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jul 19, 2019
@ordian ordian added this to the 2.7 milestone Jul 19, 2019
@ordian ordian requested review from tomusdrw and andresilva July 19, 2019 08:20
};
let step_durations: BTreeMap<u64, u16> = match p.step_duration {
StepDuration::Single(u) => iter::once((0, map_step_duration(u))).collect(),
StepDuration::Transitions(tr) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we verify there is at least one transition?

Copy link
Contributor

@afck afck Oct 1, 2019

Choose a reason for hiding this comment

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

In fact, we should verify that there is a transition for time 0, i.e. an initial step duration.
Edit: We actualy do that, in the first line of AuthorityRound::new.

let mut prev_time = 0u64;
let next_step = self.load().checked_add(1)?;
for (time, dur) in self.durations.iter().skip(1) {
let step_diff = time.checked_add(prev_dur)?.checked_sub(1)?.checked_sub(prev_time)?.checked_div(prev_dur)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't we precompute timestamp->step conversion and store them in the BTreeMap instead of calculating it every time?

})
}

fn opt_duration_remaining(&self) -> Option<Duration> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be good to have a comment of what (and how) we actually try to achieve in this function. It's not super clear to me. I suspect we want to calculate the remaining duration of current step taking into account potential transitions.

let mut prev_step = 0u64;
let mut prev_time = 0u64;
let next_step = self.load().checked_add(1)?;
for (time, dur) in self.durations.iter().skip(1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this loop supposed to find the transition timestamp and duration of current step? If so I think it could be greately simplified (and optimized) by:

  1. Converting timestamps into step transitions + keeping the timestamps
  2. Bin searching in the step transitions to find current which transition we fall into and just use the pre-computed starting timestamp.

Instead of bin-searching I think we should rather start from the end, as that's a most likely case that we will fall into.
So to summarize, we need:
Vec<(StepTransition, TimestampTransition, StepDuration)>
Then you start from the end and check if next_step > StepTransition if it is you use TimestampTransition + (next_step - StepTransition) * StepDuration to compute the expected timestamp of the next step.

}
let time = prev_time.checked_add(next_step.checked_sub(prev_step)?.checked_mul(prev_dur)?)?;
let step_end = Duration::from_secs(time);
if step_end > now {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use saturating_sub instead of this if-else

Copy link
Contributor

@afck afck Oct 1, 2019

Choose a reason for hiding this comment

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

I think Duration has no saturating_sub. But we could convert now to seconds and back, if you prefer.
(Sorry, already obsolete.)

let mut prev_dur = u64::from(self.durations[&0]);
let mut prev_step = 0u64;
let mut prev_time = 0u64;
for (time, dur) in self.durations.range(..now).skip(1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This logic seems convoluted and duplicated, please at least refactor it to a single place.

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");
panic!("authority_round: step 0 duration is undefined")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we panic instead of returning a proper error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not a question I can answer because the panic is already there. I can change it to returning an error though.

/// Deliberately typed as u16 as too high of a value leads
/// to slow block issuance.
pub step_duration: u16,
/// Wait times (durations) are deliberately typed as `u16` since larger values lead to slow
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can still store u64 to avoid casting in the functions that use it and just validate when creating the params or the engine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we are already checking that the durations are in the range when receiving AuthorityRoundParams. I don't mind storing u64 in the map.

Copy link
Contributor

Choose a reason for hiding this comment

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

But then I wonder whether u16::MAX is a good upper bound. This has nothing to do with the number of bits and should just be a threshold above which step durations don't make sense anymore. u16::MAX corresponds to about 18 hours. Should we set the limit to 24 hours (86400 seconds)?

@@ -2387,7 +2448,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();
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need more tests for the actual transitions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a test. Please let me know if you require more.

@vkomenda vkomenda force-pushed the poanetwork-step-duration-map branch from c4716fa to fd14e77 Compare July 24, 2019 13:15
prev_step = *step;
prev_time = *time;
prev_dur = *dur;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe simpler:

let (prev_dur, prev_step, prev_time) = self.durations.iter()
    .take_while(|(_, step, _)| step < next_step)
    .last()
    .expect("durations cannot be empty")
    .clone();

(Also in opt_calibrate.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it's (step, _, _), which brings the question: do you think there should be a struct instead of the triple?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, sorry! But then it should also be:

let (prev_step, prev_dur, prev_time) = …

A struct would avoid that kind of confusion, yes, but it's also a bit more verbose. Your call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's (step, time, dur) and I dropped prev_ as redundant.

let (mut prev_step, mut prev_time, mut prev_dur) =
self.durations.first().expect("durations cannot be empty");
for (step, time, dur) in self.durations.iter().skip(1) {
if *time >= now {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this actually be strict? If time == now, I imagine we consider the new step to have started?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. == works better for Vec.

Copy link
Collaborator

@ordian ordian left a comment

Choose a reason for hiding this comment

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

just a few nits

ethcore/src/engines/authority_round/mod.rs Outdated Show resolved Hide resolved
ethcore/src/engines/authority_round/mod.rs Outdated Show resolved Hide resolved
ethcore/types/src/errors/engine_error.rs Outdated Show resolved Hide resolved
ethcore/src/engines/authority_round/mod.rs Outdated Show resolved Hide resolved
ethcore/src/engines/authority_round/mod.rs Outdated Show resolved Hide resolved
@vkomenda
Copy link
Contributor Author

Are you happy with the way comments have been addressed? Is there anything else that could be improved?

Copy link
Collaborator

@ordian ordian left a comment

Choose a reason for hiding this comment

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

LGTM

json/src/spec/step_duration.rs Outdated Show resolved Hide resolved
@@ -57,14 +58,19 @@ use unexpected::{Mismatch, OutOfBounds};

mod finality;

type TransitionStep = u64;
type TransitionTimestamp = u64;
type StepDuration = u64;
Copy link
Collaborator

Choose a reason for hiding this comment

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

was it changed to u64 to avoid casts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have triples of u64 which needed some explanation in https://github.com/paritytech/parity-ethereum/blob/7863f8381a3790a3e62c605b6fc27c6c6b7fc5fe/ethcore/src/engines/authority_round/mod.rs#L157.

@ordian Would you prefer a struct type instead of plain triples?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@vkomenda not sure if the fields would be named like transition_step, transition_timestamp, step_duration, it would be any clearer, but I don't have a strong preference here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vkomenda not sure if the fields would be named like transition_step, transition_timestamp, step_duration, it would be any clearer, but I don't have a strong preference here.

That might actually help to avoid confusing different fields when using the triple. I'll add that struct.

@vkomenda vkomenda force-pushed the poanetwork-step-duration-map branch from 7863f83 to 0295f43 Compare September 3, 2019 14:29
@afck afck force-pushed the poanetwork-step-duration-map branch from 5ebf3eb to 55d3d0d Compare October 1, 2019 13:41
@afck
Copy link
Contributor

afck commented Oct 1, 2019

I rebased on master.
Edit: I can't reproduce the test failure locally.

@afck afck force-pushed the poanetwork-step-duration-map branch from 55d3d0d to 6d2bf3c Compare October 8, 2019 09:27
@afck
Copy link
Contributor

afck commented Oct 8, 2019

Something seems to be wrong with CI.

ERROR: Job failed (system failure): Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running? (executor_docker.go:739:120s)

@dvdplm
Copy link
Collaborator

dvdplm commented Oct 8, 2019

@afck Most likely yes and caused by a docker-related change that is missing on this branch. If you merge in master it should be alright. Sorry for the hassle!

@afck afck force-pushed the poanetwork-step-duration-map branch from 6d2bf3c to 94830d7 Compare October 8, 2019 13:26
@varasev
Copy link
Contributor

varasev commented Oct 28, 2019

Seems all checks now have passed. Could we merge it then?

@dvdplm dvdplm merged commit e0e79fd into openethereum:master Oct 28, 2019
@afck afck deleted the poanetwork-step-duration-map branch October 28, 2019 13:56
dvdplm added a commit that referenced this pull request Oct 28, 2019
* master:
  [informant]: `MillisecondDuration` -> `as_millis()` (#11211)
  Step duration map configuration parameter ported from the POA Network fork (#10902)
  Upgrade jsonrpc to latest (#11206)
  [export hardcoded sync]: use debug for `H256` (#11204)
  Pause pruning while snapshotting (#11178)
dvdplm added a commit that referenced this pull request Nov 6, 2019
* master: (21 commits)
  ropsten #6631425 foundation #8798209 (#11201)
  Update list of bootnodes for xDai chain (#11236)
  ethcore/res: add mordor testnet configuration (#11200)
  [chain specs]: activate `Istanbul` on mainnet (#11228)
  [builtin]: support `multiple prices and activations` in chain spec (#11039)
  Insert explicit warning into the panic hook (#11225)
  Snapshot restoration overhaul (#11219)
  Fix docker centos build (#11226)
  retry on gitlab system failures (#11222)
  Update bootnodes. (#11203)
  Use provided usd-per-eth value if an endpoint is specified (#11209)
  Use a lock instead of atomics for snapshot Progress (#11197)
  [informant]: `MillisecondDuration` -> `as_millis()` (#11211)
  Step duration map configuration parameter ported from the POA Network fork (#10902)
  Upgrade jsonrpc to latest (#11206)
  [export hardcoded sync]: use debug for `H256` (#11204)
  Pause pruning while snapshotting (#11178)
  Type annotation for next_key() matching of json filter options (#11192)
  Crypto primitives removed from ethkey (#11174)
  Made ecrecover implementation trait public (#11188)
  ...
dvdplm added a commit that referenced this pull request Feb 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants