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 2 commits
Commits
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
27 changes: 15 additions & 12 deletions client/finality-grandpa-warp-sync/src/proof.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ impl<Block: BlockT> WarpSyncProof<Block> {
) -> Result<WarpSyncProof<Block>, HandleRequestError>
where
Backend: ClientBackend<Block>,
NumberFor<Block>: BlockNumberOps,
{
// TODO: cache best response (i.e. the one with lowest begin_number)
let blockchain = backend.blockchain();
Expand Down Expand Up @@ -130,18 +131,20 @@ impl<Block: BlockT> WarpSyncProof<Block> {
false
} else {
let latest_justification =
sc_finality_grandpa::best_justification(backend)?.filter(|justification| {
// the existing best justification must be for a block higher than the
// last authority set change. if we didn't prove any authority set
// change then we fallback to make sure it's higher or equal to the
// initial warp sync block.
let limit = proofs
.last()
.map(|proof| proof.justification.target().0 + One::one())
.unwrap_or(begin_number);

justification.target().0 >= limit
});
sc_finality_grandpa::best_justification::<_, Block::Header, _>(backend)?.filter(
|justification: &GrandpaJustification<Block>| {
// the existing best justification must be for a block higher than the
// last authority set change. if we didn't prove any authority set
// change then we fallback to make sure it's higher or equal to the
// initial warp sync block.
let limit = proofs
.last()
.map(|proof| proof.justification.target().0 + One::one())
.unwrap_or(begin_number);

justification.target().0 >= limit
},
);

if let Some(latest_justification) = latest_justification {
let header = blockchain.header(BlockId::Hash(latest_justification.target().1))?
Expand Down
39 changes: 30 additions & 9 deletions client/finality-grandpa/src/authorities.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ use sc_telemetry::{telemetry, TelemetryHandle, CONSENSUS_INFO};
use sp_finality_grandpa::{AuthorityId, AuthorityList};
use sc_consensus::shared_data::{SharedData, SharedDataLocked};

use crate::SetId;

use std::cmp::Ord;
use std::fmt::Debug;
use std::ops::Add;
Expand Down Expand Up @@ -684,6 +686,17 @@ impl<H, N: Add<Output=N> + Clone> PendingChange<H, N> {
#[derive(Debug, Encode, Decode, Clone, PartialEq)]
pub struct AuthoritySetChanges<N>(Vec<(u64, N)>);

/// The response when queuering for a the set id for a specific block. Either we get a set id

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// The response when queuering for a the set id for a specific block. Either we get a set id
/// The response when querying for a the set id for a specific block. Either we get a set id

/// together with a block number for the last block in the set, or that the requested block is in the
/// latest set.
#[derive(Debug, PartialEq)]
pub enum AuthoritySetChangeId<N> {
/// The requested block is in the latest set.
Latest,
/// Tuple containing the set id and the last block number of that set.
Set(SetId, N),
}

Comment on lines +692 to +702

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't really like this type, seems overly complicated for what we're doing here. Perhaps we should just drop the assumption that some data might not be available.

impl<N> From<Vec<(u64, N)>> for AuthoritySetChanges<N> {
fn from(changes: Vec<(u64, N)>) -> AuthoritySetChanges<N> {
AuthoritySetChanges(changes)
Expand All @@ -699,7 +712,11 @@ impl<N: Ord + Clone> AuthoritySetChanges<N> {
self.0.push((set_id, block_number));
}

pub(crate) fn get_set_id(&self, block_number: N) -> Option<(u64, N)> {
pub(crate) fn get_set_id(&self, block_number: N) -> Option<AuthoritySetChangeId<N>> {
if self.block_is_current_set(block_number.clone()).unwrap_or(false) {
return Some(AuthoritySetChangeId::Latest);
}

let idx = self.0
.binary_search_by_key(&block_number, |(_, n)| n.clone())
.unwrap_or_else(|b| b);
Expand All @@ -718,12 +735,16 @@ impl<N: Ord + Clone> AuthoritySetChanges<N> {
// that we are in the right set id.
return None;
}
Some((set_id, block_number))
Some(AuthoritySetChangeId::Set(set_id, block_number))
} else {
None
}
}

pub(crate) fn block_is_current_set(&self, block_number: N) -> Option<bool> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can just be moved above as it's not used elsewhere.

self.0.last().map(|last_auth_change| last_auth_change.1 < block_number)
}

/// Returns an iterator over all historical authority set changes starting at the given block
/// number (excluded). The iterator yields a tuple representing the set id and the block number
/// of the last block in that set.
Expand Down Expand Up @@ -1660,11 +1681,11 @@ mod tests {
authority_set_changes.append(1, 81);
authority_set_changes.append(2, 121);

assert_eq!(authority_set_changes.get_set_id(20), Some((0, 41)));
assert_eq!(authority_set_changes.get_set_id(40), Some((0, 41)));
assert_eq!(authority_set_changes.get_set_id(41), Some((0, 41)));
assert_eq!(authority_set_changes.get_set_id(42), Some((1, 81)));
assert_eq!(authority_set_changes.get_set_id(141), None);
assert_eq!(authority_set_changes.get_set_id(20), Some(AuthoritySetChangeId::Set(0, 41)));
assert_eq!(authority_set_changes.get_set_id(40), Some(AuthoritySetChangeId::Set(0, 41)));
assert_eq!(authority_set_changes.get_set_id(41), Some(AuthoritySetChangeId::Set(0, 41)));
assert_eq!(authority_set_changes.get_set_id(42), Some(AuthoritySetChangeId::Set(1, 81)));
assert_eq!(authority_set_changes.get_set_id(141), Some(AuthoritySetChangeId::Latest));
}

#[test]
Expand All @@ -1677,8 +1698,8 @@ mod tests {
assert_eq!(authority_set_changes.get_set_id(20), None);
assert_eq!(authority_set_changes.get_set_id(40), None);
assert_eq!(authority_set_changes.get_set_id(41), None);
assert_eq!(authority_set_changes.get_set_id(42), Some((3, 81)));
assert_eq!(authority_set_changes.get_set_id(141), None);
assert_eq!(authority_set_changes.get_set_id(42), Some(AuthoritySetChangeId::Set(3, 81)));
assert_eq!(authority_set_changes.get_set_id(141), Some(AuthoritySetChangeId::Latest));
}

#[test]
Expand Down
35 changes: 21 additions & 14 deletions client/finality-grandpa/src/aux_schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,19 @@ use fork_tree::ForkTree;
use sc_client_api::backend::AuxStore;
use sp_blockchain::{Error as ClientError, Result as ClientResult};
use sp_finality_grandpa::{AuthorityList, RoundNumber, SetId};
use sp_runtime::traits::{Block as BlockT, NumberFor};

use crate::authorities::{
AuthoritySet, AuthoritySetChanges, DelayKind, PendingChange, SharedAuthoritySet,
};
use crate::environment::{
CompletedRound, CompletedRounds, CurrentRounds, HasVoted, SharedVoterSetState, VoterSetState,
use sp_runtime::traits::{Block as BlockT, Header as HeaderT, NumberFor};

use crate::{
authorities::{
AuthoritySet, AuthoritySetChanges, DelayKind, PendingChange, SharedAuthoritySet,
},
environment::{
CompletedRound, CompletedRounds, CurrentRounds, HasVoted, SharedVoterSetState,
VoterSetState,
},
finality_proof::ProvableJustification,
NewAuthoritySet,
};
use crate::{GrandpaJustification, NewAuthoritySet};

const VERSION_KEY: &[u8] = b"grandpa_schema_version";
const SET_STATE_KEY: &[u8] = b"grandpa_completed_round";
Expand Down Expand Up @@ -500,26 +504,29 @@ where
/// We always keep around the justification for the best finalized block and overwrite it
/// as we finalize new blocks, this makes sure that we don't store useless justifications
/// but can always prove finality of the latest block.
pub(crate) fn update_best_justification<Block: BlockT, F, R>(
justification: &GrandpaJustification<Block>,
pub(crate) fn update_best_justification<Header, J, F, R>(
justification: &J,
write_aux: F,
) -> R
where
Header: HeaderT,
J: ProvableJustification<Header>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wanted to remove ProvableJustification as it is a bit useless right now. Tests can just create a proper GrandpaJustification.

F: FnOnce(&[(&'static [u8], &[u8])]) -> R,
{
let encoded_justification = justification.encode();
write_aux(&[(BEST_JUSTIFICATION, &encoded_justification[..])])
}

/// Fetch the justification for the latest block finalized by GRANDPA, if any.
pub fn best_justification<B, Block>(
pub fn best_justification<B, Header, J>(
backend: &B,
) -> ClientResult<Option<GrandpaJustification<Block>>>
) -> ClientResult<Option<J>>
where
B: AuxStore,
Block: BlockT,
Header: HeaderT,
J: ProvableJustification<Header>,
{
load_decode::<_, GrandpaJustification<Block>>(backend, BEST_JUSTIFICATION)
load_decode::<_, J>(backend, BEST_JUSTIFICATION)
}

/// Write voter set state.
Expand Down
3 changes: 2 additions & 1 deletion client/finality-grandpa/src/environment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1224,6 +1224,7 @@ where
Block: BlockT,
BE: Backend<Block>,
Client: crate::ClientForGrandpa<Block, BE>,
NumberFor<Block>: BlockNumberOps,
{
// NOTE: lock must be held through writing to DB to avoid race. this lock
// also implicitly synchronizes the check for last finalized number
Expand Down Expand Up @@ -1330,7 +1331,7 @@ where
"number" => ?number, "hash" => ?hash,
);

crate::aux_schema::update_best_justification(
crate::aux_schema::update_best_justification::<Block::Header, _, _, _>(
&justification,
|insert| apply_aux(import_op, insert, &[]),
)?;
Expand Down
Loading