Skip to content

Commit

Permalink
grandpa: pass the actual best block to voting rules (paritytech#12477)
Browse files Browse the repository at this point in the history 
* grandpa: pass the actual best block to voting rules

* grandpa: add test for checking best header is passed to voting rule
  • Loading branch information
@andresilva @ark0f
andresilva authored and ark0f committed Feb 27, 2023
1 parent 0b5e912 commit fb63dd7
Show file tree
Hide file tree
Showing 2 changed files with 207 additions and 76 deletions.
131 changes: 69 additions & 62 deletions client/finality-grandpa/src/environment.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1174,7 +1174,7 @@ where
let base_header = match client.header(block)? {
Some(h) => h,
None => {
debug!(
warn!(
target: LOG_TARGET,
"Encountered error finding best chain containing {:?}: couldn't find base block",
block,
Expand All @@ -1194,75 +1194,82 @@ where
"Finding best chain containing block {:?} with number limit {:?}", block, limit
);

let result = match select_chain.finality_target(block, None).await {
Ok(best_hash) => {
let best_header = client
.header(best_hash)?
.expect("Header known to exist after `finality_target` call; qed");

// check if our vote is currently being limited due to a pending change
let limit = limit.filter(|limit| limit < best_header.number());

let (base_header, best_header, target_header) = if let Some(target_number) = limit {
let mut target_header = best_header.clone();

// walk backwards until we find the target block
loop {
if *target_header.number() < target_number {
unreachable!(
"we are traversing backwards from a known block; \
blocks are stored contiguously; \
qed"
);
}

if *target_header.number() == target_number {
break
}

target_header = client
.header(*target_header.parent_hash())?
.expect("Header known to exist after `finality_target` call; qed");
}

(base_header, best_header, target_header)
} else {
// otherwise just use the given best as the target
(base_header, best_header.clone(), best_header)
};
let mut target_header = match select_chain.finality_target(block, None).await {
Ok(target_hash) => client
.header(target_hash)?
.expect("Header known to exist after `finality_target` call; qed"),
Err(err) => {
warn!(
target: LOG_TARGET,
"Encountered error finding best chain containing {:?}: couldn't find target block: {}",
block,
err,
);

// restrict vote according to the given voting rule, if the
// voting rule doesn't restrict the vote then we keep the
// previous target.
//
// note that we pass the original `best_header`, i.e. before the
// authority set limit filter, which can be considered a
// mandatory/implicit voting rule.
//
// we also make sure that the restricted vote is higher than the
// round base (i.e. last finalized), otherwise the value
// returned by the given voting rule is ignored and the original
// target is used instead.
voting_rule
.restrict_vote(client.clone(), &base_header, &best_header, &target_header)
.await
.filter(|(_, restricted_number)| {
// we can only restrict votes within the interval [base, target]
restricted_number >= base_header.number() &&
restricted_number < target_header.number()
})
.or_else(|| Some((target_header.hash(), *target_header.number())))
return Ok(None)
},
Err(e) => {
};

// NOTE: this is purposefully done after `finality_target` to prevent a case
// where in-between these two requests there is a block import and
// `finality_target` returns something higher than `best_chain`.
let best_header = match select_chain.best_chain().await {
Ok(best_header) => best_header,
Err(err) => {
warn!(
target: LOG_TARGET,
"Encountered error finding best chain containing {:?}: {}", block, e
"Encountered error finding best chain containing {:?}: couldn't find best block: {}",
block,
err,
);
None

return Ok(None)
},
};

Ok(result)
if target_header.number() > best_header.number() {
return Err(Error::Safety(
"SelectChain returned a finality target higher than its best block".into(),
))
}

// check if our vote is currently being limited due to a pending change,
// in which case we will restrict our target header to the given limit
if let Some(target_number) = limit.filter(|limit| limit < target_header.number()) {
// walk backwards until we find the target block
loop {
if *target_header.number() < target_number {
unreachable!(
"we are traversing backwards from a known block; \
blocks are stored contiguously; \
qed"
);
}

if *target_header.number() == target_number {
break
}

target_header = client
.header(*target_header.parent_hash())?
.expect("Header known to exist after `finality_target` call; qed");
}
}

// restrict vote according to the given voting rule, if the voting rule
// doesn't restrict the vote then we keep the previous target.
//
// we also make sure that the restricted vote is higher than the round base
// (i.e. last finalized), otherwise the value returned by the given voting
// rule is ignored and the original target is used instead.
Ok(voting_rule
.restrict_vote(client.clone(), &base_header, &best_header, &target_header)
.await
.filter(|(_, restricted_number)| {
// we can only restrict votes within the interval [base, target]
restricted_number >= base_header.number() && restricted_number < target_header.number()
})
.or_else(|| Some((target_header.hash(), *target_header.number()))))
}

/// Finalize the given block and apply any authority set changes. If an
Expand Down
152 changes: 138 additions & 14 deletions client/finality-grandpa/src/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

use super::*;
use assert_matches::assert_matches;
use async_trait::async_trait;
use environment::HasVoted;
use futures_timer::Delay;
use parking_lot::{Mutex, RwLock};
Expand All @@ -33,8 +34,7 @@ use sc_network_test::{
PeersFullClient, TestClient, TestNetFactory,
};
use sp_api::{ApiRef, ProvideRuntimeApi};
use sp_blockchain::Result;
use sp_consensus::BlockOrigin;
use sp_consensus::{BlockOrigin, Error as ConsensusError, SelectChain};
use sp_core::H256;
use sp_finality_grandpa::{
AuthorityList, EquivocationProof, GrandpaApi, OpaqueKeyOwnershipProof, GRANDPA_ENGINE_ID,
Expand Down Expand Up @@ -200,11 +200,50 @@ sp_api::mock_impl_runtime_apis! {
}

impl GenesisAuthoritySetProvider<Block> for TestApi {
fn get(&self) -> Result<AuthorityList> {
fn get(&self) -> sp_blockchain::Result<AuthorityList> {
Ok(self.genesis_authorities.clone())
}
}

/// A mock `SelectChain` that allows the user to set the return values for each
/// method. After the `SelectChain` methods are called the pending value is
/// discarded and another call to set new values must be performed.
#[derive(Clone, Default)]
struct MockSelectChain {
leaves: Arc<Mutex<Option<Vec<Hash>>>>,
best_chain: Arc<Mutex<Option<<Block as BlockT>::Header>>>,
finality_target: Arc<Mutex<Option<Hash>>>,
}

impl MockSelectChain {
fn set_best_chain(&self, best: <Block as BlockT>::Header) {
*self.best_chain.lock() = Some(best);
}

fn set_finality_target(&self, target: Hash) {
*self.finality_target.lock() = Some(target);
}
}

#[async_trait]
impl SelectChain<Block> for MockSelectChain {
async fn leaves(&self) -> Result<Vec<Hash>, ConsensusError> {
Ok(self.leaves.lock().take().unwrap())
}

async fn best_chain(&self) -> Result<<Block as BlockT>::Header, ConsensusError> {
Ok(self.best_chain.lock().take().unwrap())
}

async fn finality_target(
&self,
_target_hash: Hash,
_maybe_max_number: Option<NumberFor<Block>>,
) -> Result<Hash, ConsensusError> {
Ok(self.finality_target.lock().take().unwrap())
}
}

const TEST_GOSSIP_DURATION: Duration = Duration::from_millis(500);

fn make_ids(keys: &[Ed25519Keyring]) -> AuthorityList {
Expand Down Expand Up @@ -1291,21 +1330,16 @@ async fn voter_catches_up_to_latest_round_when_behind() {
future::select(test, drive_to_completion).await;
}

type TestEnvironment<N, VR> = Environment<
substrate_test_runtime_client::Backend,
Block,
TestClient,
N,
LongestChain<substrate_test_runtime_client::Backend, Block>,
VR,
>;
type TestEnvironment<N, SC, VR> =
Environment<substrate_test_runtime_client::Backend, Block, TestClient, N, SC, VR>;

fn test_environment<N, VR>(
fn test_environment_with_select_chain<N, VR, SC>(
link: &TestLinkHalf,
keystore: Option<SyncCryptoStorePtr>,
network_service: N,
select_chain: SC,
voting_rule: VR,
) -> TestEnvironment<N, VR>
) -> TestEnvironment<N, SC, VR>
where
N: NetworkT<Block>,
VR: VotingRule<Block, TestClient>,
Expand All @@ -1330,7 +1364,7 @@ where
authority_set: authority_set.clone(),
config: config.clone(),
client: link.client.clone(),
select_chain: link.select_chain.clone(),
select_chain,
set_id: authority_set.set_id(),
voter_set_state: set_state.clone(),
voters: Arc::new(authority_set.current_authorities()),
Expand All @@ -1343,6 +1377,25 @@ where
}
}

fn test_environment<N, VR>(
link: &TestLinkHalf,
keystore: Option<SyncCryptoStorePtr>,
network_service: N,
voting_rule: VR,
) -> TestEnvironment<N, LongestChain<substrate_test_runtime_client::Backend, Block>, VR>
where
N: NetworkT<Block>,
VR: VotingRule<Block, TestClient>,
{
test_environment_with_select_chain(
link,
keystore,
network_service,
link.select_chain.clone(),
voting_rule,
)
}

#[tokio::test]
async fn grandpa_environment_respects_voting_rules() {
use finality_grandpa::voter::Environment;
Expand Down Expand Up @@ -1455,6 +1508,77 @@ async fn grandpa_environment_respects_voting_rules() {
);
}

#[tokio::test]
async fn grandpa_environment_passes_actual_best_block_to_voting_rules() {
// NOTE: this is a "regression" test since initially we were not passing the
// best block to the voting rules
use finality_grandpa::voter::Environment;

let peers = &[Ed25519Keyring::Alice];
let voters = make_ids(peers);

let mut net = GrandpaTestNet::new(TestApi::new(voters), 1, 0);
let peer = net.peer(0);
let network_service = peer.network_service().clone();
let link = peer.data.lock().take().unwrap();
let client = peer.client().as_client().clone();
let select_chain = MockSelectChain::default();

// add 42 blocks
peer.push_blocks(42, false);

// create an environment with a voting rule that always restricts votes to
// before the best block by 5 blocks
let env = test_environment_with_select_chain(
&link,
None,
network_service.clone(),
select_chain.clone(),
voting_rule::BeforeBestBlockBy(5),
);

// both best block and finality target are pointing to the same latest block,
// therefore we must restrict our vote on top of the given target (#21)
let hashof21 = client.expect_block_hash_from_id(&BlockId::Number(21)).unwrap();
select_chain.set_best_chain(client.expect_header(hashof21).unwrap());
select_chain.set_finality_target(client.expect_header(hashof21).unwrap().hash());

assert_eq!(
env.best_chain_containing(peer.client().info().finalized_hash)
.await
.unwrap()
.unwrap()
.1,
16,
);

// the returned finality target is already 11 blocks from the best block,
// therefore there should be no further restriction by the voting rule
let hashof10 = client.expect_block_hash_from_id(&BlockId::Number(10)).unwrap();
select_chain.set_best_chain(client.expect_header(hashof21).unwrap());
select_chain.set_finality_target(client.expect_header(hashof10).unwrap().hash());

assert_eq!(
env.best_chain_containing(peer.client().info().finalized_hash)
.await
.unwrap()
.unwrap()
.1,
10,
);

// returning a finality target that's higher than the best block is an
// inconsistent state that should be handled
let hashof42 = client.expect_block_hash_from_id(&BlockId::Number(42)).unwrap();
select_chain.set_best_chain(client.expect_header(hashof21).unwrap());
select_chain.set_finality_target(client.expect_header(hashof42).unwrap().hash());

assert_matches!(
env.best_chain_containing(peer.client().info().finalized_hash).await,
Err(CommandOrError::Error(Error::Safety(_)))
);
}

#[tokio::test]
async fn grandpa_environment_never_overwrites_round_voter_state() {
use finality_grandpa::voter::Environment;
Expand Down

0 comments on commit fb63dd7

Please sign in to comment.