Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
0fe90f7
core: make block justification optional
andresilva Nov 23, 2018
c9fdc56
runtime: update wasm binaries
andresilva Nov 23, 2018
cafa497
core: optionally pass justification on finalize_block
andresilva Nov 24, 2018
1fe5554
finality-grandpa: add channel to trigger authority set changes
andresilva Nov 26, 2018
8489541
finality-grandpa: move finalize_block to free function
andresilva Nov 27, 2018
208f097
finality-grandpa: add GrandpaOracle for auth set liveness checking
andresilva Nov 27, 2018
0c2912e
finality-grandpa: store justification on finalized transition blocks
andresilva Nov 28, 2018
b085e2e
finality-grandpa: check justification on authority set change blocks
andresilva Nov 28, 2018
fc284f7
finality-grandpa: poll grandpa liveness oracle every 10 seconds
andresilva Nov 29, 2018
4a90ba7
finality-grandpa: spawn grandpa oracle in service setup
andresilva Nov 30, 2018
0afa153
core: support multiple subscriptions per consensus gossip topic
andresilva Nov 30, 2018
4739e32
finality-grandpa: create and verify justifications
andresilva Nov 30, 2018
e4d5616
finality-grandpa: update to local branch of grandpa
andresilva Nov 30, 2018
f5ce910
finality-grandpa: update to finality-grandpa v0.5.0
andresilva Dec 2, 2018
711208b
finality-grandpa: move grandpa oracle code
andresilva Dec 2, 2018
cc46a45
finality-grandpa: fix canonality check
andresilva Dec 2, 2018
bac890e
finality-grandpa: clean up error handling
andresilva Dec 2, 2018
debd3b6
finality-grandpa: fix canonical_at_height
andresilva Dec 2, 2018
128eee0
finality-grandpa: fix tests
andresilva Dec 2, 2018
252817c
runtime: update wasm binaries
andresilva Dec 2, 2018
33392f5
core: add tests for finalizing block with justification
andresilva Dec 4, 2018
51eb2c5
finality-grandpa: improve validation of justifications
andresilva Dec 4, 2018
4a95dc3
core: remove unused IncompleteJustification block import error
andresilva Dec 4, 2018
101cba6
core: test multiple subscribers for same consensus gossip topic
andresilva Dec 4, 2018
a9e659b
Revert "finality-grandpa: improve validation of justifications"
andresilva Dec 5, 2018
d708849
finality-grandpa: fix commit validation
andresilva Dec 5, 2018
3ee13e0
Merge branch 'master' into andre/grandpa-handoff-justification
andresilva Dec 5, 2018
3f5732f
finality-grandpa: fix commit ancestry validation
andresilva Dec 5, 2018
1318a79
finality-grandpa: use grandpa v0.5.1
andresilva Dec 5, 2018
e5c593c
finality-grandpa: add docs
andresilva Dec 5, 2018
3a501ba
finality-grandpa: fix failing test
andresilva Dec 5, 2018
1e759d3
finality-grandpa: only allow a pending authority set change per fork
andresilva Dec 6, 2018
8c1981f
finality-grandpa: fix validator set transition test
andresilva Dec 7, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
finality-grandpa: create and verify justifications
  • Loading branch information
andresilva committed Nov 30, 2018
commit 4739e321aaf85e63d445884bcc8cd23737d93252
5 changes: 3 additions & 2 deletions core/consensus/aura/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,7 @@ impl<B: Block, C, E> Verifier<B> for AuraVerifier<C, E> where
&self,
origin: BlockOrigin,
header: B::Header,
_justification: Option<Justification>,
justification: Option<Justification>,
body: Option<Vec<B::Extrinsic>>
) -> Result<(ImportBlock<B>, Option<Vec<AuthorityId>>), String> {
let slot_now = slot_now(self.config.slot_duration)
Expand All @@ -389,13 +389,14 @@ impl<B: Block, C, E> Verifier<B> for AuraVerifier<C, E> where
debug!(target: "aura", "Checked {:?}; importing.", pre_header);

extra_verification.into_future().wait()?;

let import_block = ImportBlock {
origin,
header: pre_header,
justification: None,
post_digests: vec![item],
body,
finalized: false,
justification,
auxiliary: Vec::new(),
};

Expand Down
2 changes: 1 addition & 1 deletion core/consensus/common/src/block_import.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use runtime_primitives::Justification;
use std::borrow::Cow;

/// Block import result.
#[derive(Debug)]
#[derive(Debug, PartialEq, Eq)]
pub enum ImportResult {
/// Added to the import queue.
Queued,
Expand Down
2 changes: 1 addition & 1 deletion core/finality-grandpa/src/communication.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ fn localized_payload<E: Encode>(round: u64, set_id: u64, message: &E) -> Vec<u8>
}

// check a message.
fn check_message_sig<Block: BlockT>(
pub(crate) fn check_message_sig<Block: BlockT>(
message: &Message<Block>,
id: &AuthorityId,
signature: &ed25519::Signature,
Expand Down
222 changes: 150 additions & 72 deletions core/finality-grandpa/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ use grandpa::{voter, round::State as RoundState, Equivocation, BlockNumberOps};
use network::{Service as NetworkService, ExHashT};
use network::consensus_gossip::{ConsensusMessage};
use parking_lot::Mutex;
use std::collections::HashMap;
use std::collections::{HashMap, HashSet};
use std::fmt;
use std::sync::Arc;
use std::time::{Instant, Duration};
Expand Down Expand Up @@ -551,10 +551,83 @@ impl<B, E, Block: BlockT<Hash=H256>, N, RA> voter::Environment<Block::Hash, Numb

#[derive(Encode, Decode)]
struct GrandpaJustification<Block: BlockT> {
round: u64,
commit: Commit<Block>,
ancestry: Vec<Block::Header>,
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe rename to vote_ancestries or something similar?

}

impl<Block: BlockT<Hash=H256>> GrandpaJustification<Block> {
// called by voter, commit is not validated it is assumed to be well-formed
fn from_commit<B, E, RA>(
client: &Client<B, E, Block, RA>,
round: u64,
commit: Commit<Block>,
) -> Result<GrandpaJustification<Block>, Error> where
B: Backend<Block, Blake2Hasher>,
E: CallExecutor<Block, Blake2Hasher> + Send + Sync,
RA: Send + Sync,
{
let mut ancestry_hashes = HashSet::new();
let mut ancestry = Vec::new();
for signed in commit.precommits.iter() {
let mut current_hash = signed.precommit.target_hash.clone();
loop {
if current_hash == commit.target_hash { break; }
// FIXME: error with commit referencing unknown block
let current_header = client.backend().blockchain().header(BlockId::Hash(current_hash)).unwrap().unwrap();
if *current_header.number() <= commit.target_number { break; }

let parent_hash = current_header.parent_hash().clone();
if ancestry_hashes.insert(current_hash) {
ancestry.push(current_header);
}
current_hash = parent_hash;
}
}

Ok(GrandpaJustification {
round, commit, ancestry
})
}

// commit decoded as block justification must be verified and validated
fn decode_and_verify(
encoded: Vec<u8>,
set_id: u64,
) -> Result<GrandpaJustification<Block>, Error> {
// FIXME
let justification = GrandpaJustification::decode(&mut &*encoded).unwrap();

let ancestry: HashMap<_, _> = justification.ancestry
.iter()
.cloned()
.map(|h: Block::Header| (h.hash(), h))
.collect();

for signed in justification.commit.precommits.iter() {
// FIXME: handle error
communication::check_message_sig::<Block>(
&grandpa::Message::Precommit(signed.precommit.clone()),
&signed.id,
&signed.signature,
justification.round,
set_id,
).unwrap();

let mut current_hash = signed.precommit.target_hash.clone();
loop {
if current_hash == justification.commit.target_hash { break; }
// FIXME: error with commit referencing unknown block
let current_header = ancestry.get(&current_hash).unwrap();
if *current_header.number() <= justification.commit.target_number { break; }
current_hash = current_header.parent_hash().clone();
}
}

Ok(justification)
}
}

enum JustificationOrCommit<Block: BlockT> {
Justification(GrandpaJustification<Block>),
Commit(Commit<Block>),
Expand Down Expand Up @@ -639,8 +712,14 @@ fn finalize_block<B, Block: BlockT<Hash=H256>, E, RA>(
JustificationOrCommit::Justification(justification) => Some(justification.encode()),
JustificationOrCommit::Commit(commit) =>
if status.new_set_block.is_some() {
// FIXME: produce ancestry proof
let justification: GrandpaJustification<Block> = GrandpaJustification { commit, ancestry: vec![] };
// FIXME: must update `finality-grandpa` to pass along round number with commit
let round = 0;
let justification = GrandpaJustification::from_commit(
client,
round,
commit,
)?;

Some(justification.encode())
} else {
None
Expand Down Expand Up @@ -707,94 +786,93 @@ impl<B, E, Block: BlockT<Hash=H256>, RA, PRA> BlockImport<Block>
{
use authorities::PendingChange;

let enacts_change = self.authority_set.inner().read().enacts_change(*block.header.number(), |canon_number| {
self.inner.block_hash_from_id(&BlockId::number(canon_number))
.map(|h| h.expect("given number always less than block to finalize number; \
block to finalize number less or equal than best block number; \
thus there is a block with that number in the canon chain already; qed"))
})?;
// we don't want to finalize on `inner.import_block`
let justification = block.justification.take();
Copy link
Contributor

Choose a reason for hiding this comment

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

does this comment mean that import_block automatically finalizes when justification is Some?

The cloning and hashing within this import_block implementation could add up but perhaps this is premature optimization.

Copy link
Contributor Author

@andresilva andresilva Dec 3, 2018

Choose a reason for hiding this comment

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

Yes, that is the assumption. It only actually finalizes if finalized = true, but the contract of client.import_block has changed to: if a justification is provided, then the block must be finalized (this should maybe be statically typed instead of using different parameters for finalized and the justification).

let number = block.header.number().clone();
let hash = block.post_header().hash();
let parent_hash = *block.header.parent_hash();
let digest = block.header.digest().clone();
let is_live = self.authority_set_oracle.is_live();
let justification_provided = block.justification.is_some();

// a pending change is enacted by the given block, if the current
// grandpa authority set isn't live anymore the provided `ImportBlock`
// should include a justification for finalizing the block.
if enacts_change && !is_live && !justification_provided {
return Err(ClientErrorKind::BadJustification(
"missing justification for block that enacts authority set changes".to_string()
).into());
let import_result = self.inner.import_block(block, new_authorities)?;
if import_result != ImportResult::Queued {
return Ok(import_result);
}

let maybe_change = self.api.runtime_api().grandpa_pending_change(
&BlockId::hash(*block.header.parent_hash()),
&block.header.digest().clone(),
&BlockId::hash(parent_hash),
&digest,
)?;

// when we update the authorities, we need to hold the lock
// until the block is written to prevent a race if we need to restore
// the old authority set on error.
let just_in_case = maybe_change.map(|change| {
let hash = block.post_header().hash();
let number = block.header.number().clone();

if let Some(change) = maybe_change {
let mut authorities = self.authority_set.inner().write();
let old_set = authorities.clone();
authorities.add_pending_change(PendingChange {
next_authorities: change.next_authorities,
finalization_depth: change.delay,
canon_height: number,
canon_hash: hash,
});

block.auxiliary.push((AUTHORITY_SET_KEY.to_vec(), Some(authorities.encode())));
(old_set, authorities)
});

let hash = block.header.hash();
let number = *block.header.number();
let justification = block.justification.clone();
let encoded = authorities.encode();
self.inner.backend().insert_aux(&[(AUTHORITY_SET_KEY, &encoded[..])], &[])?;
};

let import_result = self.inner.import_block(block, new_authorities).map_err(|e| {
if let Some((old_set, mut authorities)) = just_in_case {
debug!(target: "afg", "Restoring old set after block import error: {:?}", e);
*authorities = old_set;
}
e
// FIXME: the closure may panic
let enacts_change = self.authority_set.inner().read().enacts_change(number, |canon_number| {
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably out of scope, but still: the change that is checked here (and applied later in finalize_block call) contains the same set of authorities as in the new_authorities set? Or those are/could be different sets?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also seems that if importing block with justification assumes finalization of the block (that's what @rphmeier asked above, iiuc), then imo we don't need new_authorities and authorities cache at all - we just need to refuse light-import of blocks which contain DigestItem::as_authorities_change and no justification. Could you, please, verify me here, @rphmeier ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@svyatonik yep, I believe that for GRANDPA we can indeed take that kind of approach. We may still want the authorities cache for other consensus algorithms.

Copy link
Contributor

Choose a reason for hiding this comment

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

although this is a bit of future work, because it would require tracking the authorship validator changes in GRANDPA and saving justifications for those too upon finality.

this is a little bit harder than the GRANDPA set changes because we may have a commit for block N+K where a set change happened in block N; unlike GRANDPA-set changes where honest voters can never vote beyond N. so constructing the justification may become inefficient without a good way to prove ancestry far back in the chain.

self.inner.block_hash_from_id(&BlockId::number(canon_number))
.map(|h| h.expect("given number always less than block to finalize number; \
block to finalize number less or equal than best block number; \
thus there is a block with that number in the canon chain already; qed"))
})?;

if let Some(justification) = justification {
// FIXME: validate justification
let justification = GrandpaJustification::decode(&mut &*justification).unwrap();

let result = finalize_block(
&*self.inner,
&self.authority_set,
hash,
number,
justification.into(),
);

match result {
Ok(_) if enacts_change => {
unreachable!("returns Ok when no authority set change should be enacted; \
verified previously that finalizing the current block enacts a change; \
qed;");
},
Err(ExitOrError::AuthoritiesChanged(new)) => {
if let Err(_) = self.authority_set_change.unbounded_send(new) {
return Err(ClientErrorKind::Backend(
"imported and finalized change block but grandpa voter is no longer running".to_string()
).into());
// a pending change is enacted by the given block, if the current
// grandpa authority set isn't live anymore the provided `ImportBlock`
// should include a justification for finalizing the block.
match justification {
Some(justification) => {
if enacts_change && !is_live {
// FIXME: handle error
let justification = GrandpaJustification::decode_and_verify(
justification,
self.authority_set.set_id(),
).unwrap();

let result = finalize_block(
&*self.inner,
&self.authority_set,
hash,
number,
justification.into(),
);

match result {
Ok(_) => {
unreachable!("returns Ok when no authority set change should be enacted; \
verified previously that finalizing the current block enacts a change; \
qed;");
},
Err(ExitOrError::AuthoritiesChanged(new)) => {
debug!(target: "finality", "Imported justified block #{} that enacts authority set change, signalling voter.", number);
if let Err(_) = self.authority_set_change.unbounded_send(new) {
return Err(ClientErrorKind::Backend(
"imported and finalized change block but grandpa voter is no longer running".to_string()
).into());
}
},
Err(ExitOrError::Error(_)) => {
return Err(ClientErrorKind::Backend(
"imported change block but failed to finalize it, node may be in an inconsistent state".to_string()
).into());
},
}
},
Err(ExitOrError::Error(_)) if enacts_change && !is_live => {
// FIXME: improve this
return Err(ClientErrorKind::Backend(
"imported change block but failed to finalize it, node may be in an inconsistent state".to_string()
).into());
},
_ => {},
}
}
},
None if enacts_change && !is_live => {
return Err(ClientErrorKind::BadJustification(
"missing justification for block that enacts authority set change".to_string()
).into());
},
_ => {}
Copy link
Contributor

Choose a reason for hiding this comment

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

we may want something here for writing to the SharedAuthoritySet to note that we imported a block to the chain which should have enacted the authority set chain (this may well be a set of heads).

Actual code that periodically revisits this and re-requests justifications can be done in a follow-up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed by making sure AuthoritySet only imports one pending change per fork.

}

Ok(import_result)
Expand Down