From 25984d8dc8368de2ded95a3633ec35d326b28dad Mon Sep 17 00:00:00 2001
From: Robert Habermeier <rphmeier@gmail.com>
Date: Sat, 27 Oct 2018 16:18:01 +0200
Subject: [PATCH] add some tests for authority set logic
---
substrate/core/finality-grandpa/Cargo.toml | 2 -
.../core/finality-grandpa/src/authorities.rs | 167 ++++++++++++++++--
substrate/core/finality-grandpa/src/lib.rs | 12 +-
3 files changed, 154 insertions(+), 27 deletions(-)
diff --git a/substrate/core/finality-grandpa/Cargo.toml b/substrate/core/finality-grandpa/Cargo.toml
index 34c0e99b631..afff3ad5cba 100644
--- a/substrate/core/finality-grandpa/Cargo.toml
+++ b/substrate/core/finality-grandpa/Cargo.toml
@@ -11,7 +11,6 @@ sr-primitives = { path = "../sr-primitives" }
substrate-consensus-common = { path = "../consensus/common" }
substrate-primitives = { path = "../primitives" }
substrate-client = { path = "../client" }
-substrate-network = { path = "../network" }
log = "0.4"
parking_lot = "0.4"
tokio = "0.1.7"
@@ -23,5 +22,4 @@ features = ["derive-codec"]
[dev-dependencies]
substrate-network = { path = "../network", features = ["test-helpers"] }
-parking_lot = "0.4"
substrate-keyring = { path = "../keyring" }
diff --git a/substrate/core/finality-grandpa/src/authorities.rs b/substrate/core/finality-grandpa/src/authorities.rs
index cbe53ceb3db..4e435f54209 100644
--- a/substrate/core/finality-grandpa/src/authorities.rs
+++ b/substrate/core/finality-grandpa/src/authorities.rs
@@ -53,6 +53,11 @@ impl<H, N> SharedAuthoritySet<H, N> {
{
f(&*self.inner.read())
}
+
+ /// Execute a closure with the inner set mutably.
+ pub(crate) fn with_mut<F, U>(&self, f: F) -> U where F: FnOnce(&mut AuthoritySet<H, N>) -> U {
+ f(&mut *self.inner.write())
+ }
}
impl<H: Eq, N> SharedAuthoritySet<H, N>
@@ -60,17 +65,7 @@ impl<H: Eq, N> SharedAuthoritySet<H, N>
{
/// Note an upcoming pending transition.
pub(crate) fn add_pending_change(&self, pending: PendingChange<H, N>) {
- // ordered first by effective number and then by signal-block number.
- let mut inner = self.inner.write();
- let key = (pending.effective_number(), pending.canon_height.clone());
- let idx = inner.pending_changes
- .binary_search_by_key(&key, |change| (
- change.effective_number(),
- change.canon_height.clone(),
- ))
- .unwrap_or_else(|i| i);
-
- inner.pending_changes.insert(idx, pending);
+ self.inner.write().add_pending_change(pending)
}
/// Get the earliest limit-block number, if any.
@@ -82,11 +77,6 @@ impl<H: Eq, N> SharedAuthoritySet<H, N>
pub(crate) fn set_id(&self) -> u64 {
self.inner.read().set_id
}
-
- /// Execute a closure with the inner set mutably.
- pub(crate) fn with_mut<F, U>(&self, f: F) -> U where F: FnOnce(&mut AuthoritySet<H, N>) -> U {
- f(&mut *self.inner.write())
- }
}
impl<H, N> From<AuthoritySet<H, N>> for SharedAuthoritySet<H, N> {
@@ -118,6 +108,19 @@ impl<H, N> AuthoritySet<H, N> {
impl<H: Eq, N> AuthoritySet<H, N>
where N: Add<Output=N> + Ord + Clone + Debug,
{
+ /// Note an upcoming pending transition.
+ pub(crate) fn add_pending_change(&mut self, pending: PendingChange<H, N>) {
+ // ordered first by effective number and then by signal-block number.
+ let key = (pending.effective_number(), pending.canon_height.clone());
+ let idx = self.pending_changes
+ .binary_search_by_key(&key, |change| (
+ change.effective_number(),
+ change.canon_height.clone(),
+ ))
+ .unwrap_or_else(|i| i);
+
+ self.pending_changes.insert(idx, pending);
+ }
/// Get the earliest limit-block number, if any.
pub(crate) fn current_limit(&self) -> Option<N> {
self.pending_changes.get(0).map(|change| change.effective_number().clone())
@@ -173,7 +176,7 @@ impl<H: Eq, N> AuthoritySet<H, N>
///
/// This will be applied when the announcing block is at some depth within
/// the finalized chain.
-#[derive(Debug, Clone, Encode, Decode)]
+#[derive(Debug, Clone, Encode, Decode, PartialEq)]
pub(crate) struct PendingChange<H, N> {
/// The new authorities and weights to apply.
pub(crate) next_authorities: Vec<(AuthorityId, u64)>,
@@ -192,3 +195,133 @@ impl<H, N: Add<Output=N> + Clone> PendingChange<H, N> {
self.canon_height.clone() + self.finalization_depth.clone()
}
}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+
+ #[test]
+ fn changes_sorted_in_correct_order() {
+ let mut authorities = AuthoritySet {
+ current_authorities: Vec::new(),
+ set_id: 0,
+ pending_changes: Vec::new(),
+ };
+
+ let change_a = PendingChange {
+ next_authorities: Vec::new(),
+ finalization_depth: 10,
+ canon_height: 5,
+ canon_hash: "hash_a",
+ };
+
+ let change_b = PendingChange {
+ next_authorities: Vec::new(),
+ finalization_depth: 0,
+ canon_height: 16,
+ canon_hash: "hash_b",
+ };
+
+ let change_c = PendingChange {
+ next_authorities: Vec::new(),
+ finalization_depth: 5,
+ canon_height: 10,
+ canon_hash: "hash_c",
+ };
+
+ authorities.add_pending_change(change_a.clone());
+ authorities.add_pending_change(change_b.clone());
+ authorities.add_pending_change(change_c.clone());
+
+ assert_eq!(authorities.pending_changes, vec![change_a, change_c, change_b]);
+ }
+
+ #[test]
+ fn apply_change() {
+ let mut authorities = AuthoritySet {
+ current_authorities: Vec::new(),
+ set_id: 0,
+ pending_changes: Vec::new(),
+ };
+
+ let set_a = vec![([1; 32].into(), 5)];
+ let set_b = vec![([2; 32].into(), 5)];
+
+ let change_a = PendingChange {
+ next_authorities: set_a.clone(),
+ finalization_depth: 10,
+ canon_height: 5,
+ canon_hash: "hash_a",
+ };
+
+ let change_b = PendingChange {
+ next_authorities: set_b.clone(),
+ finalization_depth: 10,
+ canon_height: 5,
+ canon_hash: "hash_b",
+ };
+
+ authorities.add_pending_change(change_a.clone());
+ authorities.add_pending_change(change_b.clone());
+
+ authorities.apply_changes::<_, ()>(10, |_| panic!()).unwrap();
+ assert!(authorities.current_authorities.is_empty());
+
+ authorities.apply_changes::<_, ()>(
+ 15,
+ |n| if n == 5 { Ok("hash_a") } else { panic!() }
+ ).unwrap();
+
+ assert_eq!(authorities.current_authorities, set_a);
+ assert_eq!(authorities.set_id, 1);
+ }
+
+ #[test]
+ fn apply_many_changes_at_once() {
+ let mut authorities = AuthoritySet {
+ current_authorities: Vec::new(),
+ set_id: 0,
+ pending_changes: Vec::new(),
+ };
+
+ let set_a = vec![([1; 32].into(), 5)];
+ let set_b = vec![([2; 32].into(), 5)];
+ let set_c = vec![([3; 32].into(), 5)];
+
+ let change_a = PendingChange {
+ next_authorities: set_a.clone(),
+ finalization_depth: 10,
+ canon_height: 5,
+ canon_hash: "hash_a",
+ };
+
+ // will be ignored because it was signalled when change_a still pending.
+ let change_b = PendingChange {
+ next_authorities: set_b.clone(),
+ finalization_depth: 10,
+ canon_height: 15,
+ canon_hash: "hash_b",
+ };
+
+ let change_c = PendingChange {
+ next_authorities: set_c.clone(),
+ finalization_depth: 10,
+ canon_height: 16,
+ canon_hash: "hash_c",
+ };
+
+ authorities.add_pending_change(change_a.clone());
+ authorities.add_pending_change(change_b.clone());
+ authorities.add_pending_change(change_c.clone());
+
+ authorities.apply_changes(26, |n| match n {
+ 5 => Ok("hash_a"),
+ 15 => Ok("hash_b"),
+ 16 => Ok("hash_c"),
+ _ => Err(()),
+ }).unwrap();
+
+ assert_eq!(authorities.current_authorities, set_c);
+ assert_eq!(authorities.set_id, 2); // has been bumped only twice
+ }
+}
diff --git a/substrate/core/finality-grandpa/src/lib.rs b/substrate/core/finality-grandpa/src/lib.rs
index 7c12be4a8a8..f5379a8113a 100644
--- a/substrate/core/finality-grandpa/src/lib.rs
+++ b/substrate/core/finality-grandpa/src/lib.rs
@@ -24,7 +24,6 @@ extern crate substrate_client as client;
extern crate sr_primitives as runtime_primitives;
extern crate substrate_consensus_common as consensus_common;
extern crate substrate_primitives;
-extern crate substrate_network as network;
extern crate tokio;
extern crate parking_lot;
extern crate parity_codec as codec;
@@ -35,9 +34,6 @@ extern crate log;
#[cfg(test)]
extern crate substrate_network as network;
-#[cfg(test)]
-extern crate parking_lot;
-
#[cfg(test)]
extern crate substrate_keyring as keyring;
@@ -871,10 +867,10 @@ mod tests {
.take_while(|n| Ok(n.header.number() < &20))
.for_each(move |_| Ok(()))
);
- let voter = run_grandpa(
+ let (voter, _) = run_grandpa(
Config {
gossip_duration: TEST_GOSSIP_DURATION,
- voters: voters.clone(),
+ genesis_voters: voters.clone(),
local_key: Some(Arc::new(key.clone().into())),
},
client,
@@ -930,10 +926,10 @@ mod tests {
.take_while(|n| Ok(n.header.number() < &20))
.for_each(move |_| Ok(()))
);
- let voter = run_grandpa(
+ let (voter, _) = run_grandpa(
Config {
gossip_duration: TEST_GOSSIP_DURATION,
- voters: voters.keys().cloned().collect(),
+ genesis_voters: voters.keys().cloned().collect(),
local_key,
},
client,
--
GitLab