From 981c3a57f96f7ef44c9d07af73583d51151f5007 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9=20Silva?= <andre.beat@gmail.com>
Date: Sun, 27 Oct 2019 11:54:08 +0000
Subject: [PATCH] grandpa: always try to import available block justifcation
(#3928)
* grandpa: always try to import justifications in blocks
* grandpa: export useful types
* grandpa: add test for justification import on regular blocks
* grandpa: expand comment in test
---
.../finality-grandpa/src/communication/mod.rs | 2 +-
substrate/core/finality-grandpa/src/import.rs | 14 ++--
.../finality-grandpa/src/justification.rs | 2 +-
substrate/core/finality-grandpa/src/lib.rs | 1 +
substrate/core/finality-grandpa/src/tests.rs | 81 +++++++++++++++++++
5 files changed, 90 insertions(+), 10 deletions(-)
diff --git a/substrate/core/finality-grandpa/src/communication/mod.rs b/substrate/core/finality-grandpa/src/communication/mod.rs
index 6f43b1106a5..9a6a40fb8d2 100644
--- a/substrate/core/finality-grandpa/src/communication/mod.rs
+++ b/substrate/core/finality-grandpa/src/communication/mod.rs
@@ -683,7 +683,7 @@ impl<B: BlockT, N: Network<B>> Clone for NetworkBridge<B, N> {
}
}
-fn localized_payload<E: Encode>(round: RoundNumber, set_id: SetIdNumber, message: &E) -> Vec<u8> {
+pub(crate) fn localized_payload<E: Encode>(round: RoundNumber, set_id: SetIdNumber, message: &E) -> Vec<u8> {
(message, round, set_id).encode()
}
diff --git a/substrate/core/finality-grandpa/src/import.rs b/substrate/core/finality-grandpa/src/import.rs
index 758f6f18dbb..8fbe0791e8c 100644
--- a/substrate/core/finality-grandpa/src/import.rs
+++ b/substrate/core/finality-grandpa/src/import.rs
@@ -465,17 +465,15 @@ impl<B, E, Block: BlockT<Hash=H256>, RA, SC> BlockImport<Block>
_ => {},
}
- if !needs_justification && !enacts_consensus_change {
- return Ok(ImportResult::Imported(imported_aux));
- }
-
match justification {
Some(justification) => {
self.import_justification(hash, number, justification, needs_justification).unwrap_or_else(|err| {
- debug!(target: "finality", "Imported block #{} that enacts authority set change with \
- invalid justification: {:?}, requesting justification from peers.", number, err);
- imported_aux.bad_justification = true;
- imported_aux.needs_justification = true;
+ if needs_justification || enacts_consensus_change {
+ debug!(target: "finality", "Imported block #{} that enacts authority set change with \
+ invalid justification: {:?}, requesting justification from peers.", number, err);
+ imported_aux.bad_justification = true;
+ imported_aux.needs_justification = true;
+ }
});
},
None => {
diff --git a/substrate/core/finality-grandpa/src/justification.rs b/substrate/core/finality-grandpa/src/justification.rs
index b4de8ff0586..f5965df3e12 100644
--- a/substrate/core/finality-grandpa/src/justification.rs
+++ b/substrate/core/finality-grandpa/src/justification.rs
@@ -39,7 +39,7 @@ use crate::communication;
/// This is meant to be stored in the db and passed around the network to other
/// nodes, and are used by syncing nodes to prove authority set handoffs.
#[derive(Encode, Decode)]
-pub(crate) struct GrandpaJustification<Block: BlockT> {
+pub struct GrandpaJustification<Block: BlockT> {
round: u64,
pub(crate) commit: Commit<Block>,
votes_ancestries: Vec<Block::Header>,
diff --git a/substrate/core/finality-grandpa/src/lib.rs b/substrate/core/finality-grandpa/src/lib.rs
index 63eddfd3f33..68019dc8eb2 100644
--- a/substrate/core/finality-grandpa/src/lib.rs
+++ b/substrate/core/finality-grandpa/src/lib.rs
@@ -96,6 +96,7 @@ mod voting_rule;
pub use communication::Network;
pub use finality_proof::FinalityProofProvider;
+pub use justification::GrandpaJustification;
pub use light_import::light_block_import;
pub use observer::run_grandpa_observer;
pub use voting_rule::{
diff --git a/substrate/core/finality-grandpa/src/tests.rs b/substrate/core/finality-grandpa/src/tests.rs
index 8c0047e38bd..2767c14b274 100644
--- a/substrate/core/finality-grandpa/src/tests.rs
+++ b/substrate/core/finality-grandpa/src/tests.rs
@@ -1627,3 +1627,84 @@ fn grandpa_environment_respects_voting_rules() {
19,
);
}
+
+#[test]
+fn imports_justification_for_regular_blocks_on_import() {
+ // NOTE: this is a regression test since initially we would only import
+ // justifications for authority change blocks, and would discard any
+ // existing justification otherwise.
+ let peers = &[Ed25519Keyring::Alice];
+ let voters = make_ids(peers);
+ let api = TestApi::new(voters);
+ let mut net = GrandpaTestNet::new(api.clone(), 1);
+
+ let client = net.peer(0).client().clone();
+ let (mut block_import, ..) = net.make_block_import(client.clone());
+
+ let full_client = client.as_full().expect("only full clients are used in test");
+ let builder = full_client.new_block_at(&BlockId::Number(0), Default::default()).unwrap();
+ let block = builder.bake().unwrap();
+
+ let block_hash = block.hash();
+
+ // create a valid justification, with one precommit targeting the block
+ let justification = {
+ let round = 1;
+ let set_id = 0;
+
+ let precommit = grandpa::Precommit {
+ target_hash: block_hash,
+ target_number: *block.header.number(),
+ };
+
+ let msg = grandpa::Message::Precommit(precommit.clone());
+ let encoded = communication::localized_payload(round, set_id, &msg);
+ let signature = peers[0].sign(&encoded[..]).into();
+
+ let precommit = grandpa::SignedPrecommit {
+ precommit,
+ signature,
+ id: peers[0].public().into(),
+ };
+
+ let commit = grandpa::Commit {
+ target_hash: block_hash,
+ target_number: *block.header.number(),
+ precommits: vec![precommit],
+ };
+
+ GrandpaJustification::from_commit(
+ &full_client,
+ round,
+ commit,
+ ).unwrap()
+ };
+
+ // we import the block with justification attached
+ let block = BlockImportParams {
+ origin: BlockOrigin::File,
+ header: block.header,
+ justification: Some(justification.encode()),
+ post_digests: Vec::new(),
+ body: Some(block.extrinsics),
+ finalized: false,
+ auxiliary: Vec::new(),
+ fork_choice: ForkChoiceStrategy::LongestChain,
+ };
+
+ assert_eq!(
+ block_import.import_block(block, HashMap::new()).unwrap(),
+ ImportResult::Imported(ImportedAux {
+ needs_justification: false,
+ clear_justification_requests: false,
+ bad_justification: false,
+ is_new_best: true,
+ ..Default::default()
+ }),
+ );
+
+ // the justification should be imported and available from the client
+ assert!(
+ client.justification(&BlockId::Hash(block_hash)).unwrap().is_some(),
+ );
+}
--
GitLab