diff --git a/substrate/client/finality-grandpa/Cargo.toml b/substrate/client/finality-grandpa/Cargo.toml
index 7b2e58b8be9a673f8e720c3e5e1855addbfbf997..7cd3548a7628cb3f84004af7e211ee5e994f06b9 100644
--- a/substrate/client/finality-grandpa/Cargo.toml
+++ b/substrate/client/finality-grandpa/Cargo.toml
@@ -21,7 +21,6 @@ futures-timer = "3.0.1"
log = "0.4.8"
parking_lot = "0.10.0"
rand = "0.7.2"
-assert_matches = "1.3.0"
parity-scale-codec = { version = "1.3.4", features = ["derive"] }
sp-application-crypto = { version = "2.0.0-rc5", path = "../../primitives/application-crypto" }
sp-arithmetic = { version = "2.0.0-rc5", path = "../../primitives/arithmetic" }
@@ -47,6 +46,7 @@ finality-grandpa = { version = "0.12.3", features = ["derive-codec"] }
pin-project = "0.4.6"
[dev-dependencies]
+assert_matches = "1.3.0"
finality-grandpa = { version = "0.12.3", features = ["derive-codec", "test-helpers"] }
sc-network = { version = "0.8.0-rc5", path = "../network" }
sc-network-test = { version = "0.8.0-rc5", path = "../network/test" }
diff --git a/substrate/client/finality-grandpa/src/environment.rs b/substrate/client/finality-grandpa/src/environment.rs
index 0cfab13a6fa1135159518d5992c45e1e40b9ef91..ca47e5e2cc4c54c280b9036d5bc31fee31287a68 100644
--- a/substrate/client/finality-grandpa/src/environment.rs
+++ b/substrate/client/finality-grandpa/src/environment.rs
@@ -930,7 +930,12 @@ where
// remove the round from live rounds and start tracking the next round
let mut current_rounds = current_rounds.clone();
current_rounds.remove(&round);
- current_rounds.insert(round + 1, HasVoted::No);
+
+ // NOTE: this condition should always hold as GRANDPA rounds are always
+ // started in increasing order, still it's better to play it safe.
+ if !current_rounds.contains_key(&(round + 1)) {
+ current_rounds.insert(round + 1, HasVoted::No);
+ }
let set_state = VoterSetState::<Block>::Live {
completed_rounds,
diff --git a/substrate/client/finality-grandpa/src/tests.rs b/substrate/client/finality-grandpa/src/tests.rs
index e2b9671f04df37ad5900937607c8b4507dddecf1..e2cdd7653a64f7927f52a80941eb8a4a8285a8a1 100644
--- a/substrate/client/finality-grandpa/src/tests.rs
+++ b/substrate/client/finality-grandpa/src/tests.rs
@@ -19,10 +19,11 @@
//! Tests and test helpers for GRANDPA.
use super::*;
+use assert_matches::assert_matches;
use environment::HasVoted;
use sc_network_test::{
- Block, Hash, TestNetFactory, BlockImportAdapter, Peer,
- PeersClient, PassThroughVerifier, PeersFullClient,
+ Block, BlockImportAdapter, Hash, PassThroughVerifier, Peer, PeersClient, PeersFullClient,
+ TestClient, TestNetFactory,
};
use sc_network::config::{ProtocolConfig, BoxFinalityProofRequestBuilder};
use parking_lot::Mutex;
@@ -53,16 +54,9 @@ use consensus_changes::ConsensusChanges;
use sc_block_builder::BlockBuilderProvider;
use sc_consensus::LongestChain;
-type PeerData =
- Mutex<
- Option<
- LinkHalf<
- Block,
- PeersFullClient,
- LongestChain<substrate_test_runtime_client::Backend, Block>
- >
- >
- >;
+type TestLinkHalf =
+ LinkHalf<Block, PeersFullClient, LongestChain<substrate_test_runtime_client::Backend, Block>>;
+type PeerData = Mutex<Option<TestLinkHalf>>;
type GrandpaPeer = Peer<PeerData>;
struct GrandpaTestNet {
@@ -1519,10 +1513,67 @@ fn voter_catches_up_to_latest_round_when_behind() {
);
}
+type TestEnvironment<N, VR> = Environment<
+ substrate_test_runtime_client::Backend,
+ Block,
+ TestClient,
+ N,
+ LongestChain<substrate_test_runtime_client::Backend, Block>,
+ VR,
+>;
+
+fn test_environment<N, VR>(
+ link: &TestLinkHalf,
+ keystore: Option<BareCryptoStorePtr>,
+ network_service: N,
+ voting_rule: VR,
+) -> TestEnvironment<N, VR>
+where
+ N: NetworkT<Block>,
+ VR: VotingRule<Block, TestClient>,
+{
+ let PersistentData {
+ ref authority_set,
+ ref consensus_changes,
+ ref set_state,
+ ..
+ } = link.persistent_data;
+
+ let config = Config {
+ gossip_duration: TEST_GOSSIP_DURATION,
+ justification_period: 32,
+ keystore,
+ name: None,
+ is_authority: true,
+ observer_enabled: true,
+ };
+
+ let network = NetworkBridge::new(
+ network_service.clone(),
+ config.clone(),
+ set_state.clone(),
+ None,
+ );
+
+ Environment {
+ authority_set: authority_set.clone(),
+ config: config.clone(),
+ consensus_changes: consensus_changes.clone(),
+ client: link.client.clone(),
+ select_chain: link.select_chain.clone(),
+ set_id: authority_set.set_id(),
+ voter_set_state: set_state.clone(),
+ voters: Arc::new(authority_set.current_authorities()),
+ network,
+ voting_rule,
+ metrics: None,
+ _phantom: PhantomData,
+ }
+}
+
#[test]
fn grandpa_environment_respects_voting_rules() {
use finality_grandpa::Chain;
- use sc_network_test::TestClient;
let peers = &[Ed25519Keyring::Alice];
let voters = make_ids(peers);
@@ -1532,63 +1583,28 @@ fn grandpa_environment_respects_voting_rules() {
let network_service = peer.network_service().clone();
let link = peer.data.lock().take().unwrap();
- // create a voter environment with a given voting rule
- let environment = |voting_rule: Box<dyn VotingRule<Block, TestClient>>| {
- let PersistentData {
- ref authority_set,
- ref consensus_changes,
- ref set_state,
- ..
- } = link.persistent_data;
-
- let config = Config {
- gossip_duration: TEST_GOSSIP_DURATION,
- justification_period: 32,
- keystore: None,
- name: None,
- is_authority: true,
- observer_enabled: true,
- };
-
- let network = NetworkBridge::new(
- network_service.clone(),
- config.clone(),
- set_state.clone(),
- None,
- );
-
- Environment {
- authority_set: authority_set.clone(),
- config: config.clone(),
- consensus_changes: consensus_changes.clone(),
- client: link.client.clone(),
- select_chain: link.select_chain.clone(),
- set_id: authority_set.set_id(),
- voter_set_state: set_state.clone(),
- voters: Arc::new(authority_set.current_authorities()),
- network,
- voting_rule,
- metrics: None,
- _phantom: PhantomData,
- }
- };
-
// add 21 blocks
peer.push_blocks(21, false);
// create an environment with no voting rule restrictions
- let unrestricted_env = environment(Box::new(()));
+ let unrestricted_env = test_environment(&link, None, network_service.clone(), ());
// another with 3/4 unfinalized chain voting rule restriction
- let three_quarters_env = environment(Box::new(
- voting_rule::ThreeQuartersOfTheUnfinalizedChain
- ));
+ let three_quarters_env = test_environment(
+ &link,
+ None,
+ network_service.clone(),
+ voting_rule::ThreeQuartersOfTheUnfinalizedChain,
+ );
// and another restricted with the default voting rules: i.e. 3/4 rule and
// always below best block
- let default_env = environment(Box::new(
- VotingRulesBuilder::default().build()
- ));
+ let default_env = test_environment(
+ &link,
+ None,
+ network_service.clone(),
+ VotingRulesBuilder::default().build(),
+ );
// the unrestricted environment should just return the best block
assert_eq!(
@@ -1648,6 +1664,70 @@ fn grandpa_environment_respects_voting_rules() {
);
}
+#[test]
+fn grandpa_environment_never_overwrites_round_voter_state() {
+ use finality_grandpa::voter::Environment;
+
+ let peers = &[Ed25519Keyring::Alice];
+ let voters = make_ids(peers);
+
+ let mut net = GrandpaTestNet::new(TestApi::new(voters), 1);
+ let peer = net.peer(0);
+ let network_service = peer.network_service().clone();
+ let link = peer.data.lock().take().unwrap();
+
+ let (keystore, _keystore_path) = create_keystore(peers[0]);
+ let environment = test_environment(&link, Some(keystore), network_service.clone(), ());
+
+ let round_state = || finality_grandpa::round::State::genesis(Default::default());
+ let base = || Default::default();
+ let historical_votes = || finality_grandpa::HistoricalVotes::new();
+
+ let get_current_round = |n| {
+ let current_rounds = environment
+ .voter_set_state
+ .read()
+ .with_current_round(n)
+ .map(|(_, current_rounds)| current_rounds.clone())
+ .ok()?;
+
+ Some(current_rounds.get(&n).unwrap().clone())
+ };
+
+ // round 2 should not be tracked
+ assert_eq!(get_current_round(2), None);
+
+ // after completing round 1 we should start tracking round 2
+ environment
+ .completed(1, round_state(), base(), &historical_votes())
+ .unwrap();
+
+ assert_eq!(get_current_round(2).unwrap(), HasVoted::No);
+
+ let info = peer.client().info();
+
+ let prevote = finality_grandpa::Prevote {
+ target_hash: info.best_hash,
+ target_number: info.best_number,
+ };
+
+ // we prevote for round 2 which should lead to us updating the voter state
+ environment.prevoted(2, prevote.clone()).unwrap();
+
+ let has_voted = get_current_round(2).unwrap();
+
+ assert_matches!(has_voted, HasVoted::Yes(_, _));
+ assert_eq!(*has_voted.prevote().unwrap(), prevote);
+
+ // if we report round 1 as completed again we should not overwrite the
+ // voter state for round 2
+ environment
+ .completed(1, round_state(), base(), &historical_votes())
+ .unwrap();
+
+ assert_matches!(get_current_round(2).unwrap(), HasVoted::Yes(_, _));
+}
+
#[test]
fn imports_justification_for_regular_blocks_on_import() {
// NOTE: this is a regression test since initially we would only import