Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Bypass chain-selection subsystem until disputes are enabled. #3676

Merged
merged 13 commits into from
Aug 20, 2021
67 changes: 41 additions & 26 deletions node/service/src/relay_chain_selection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -178,10 +178,15 @@ where
target_hash: Hash,
maybe_max_number: Option<BlockNumber>,
) -> Result<Option<Hash>, ConsensusError> {
let longest_chain_best =
self.fallback.finality_target(target_hash, maybe_max_number).await?;

if self.selection.overseer.is_disconnected() {
return self.fallback.finality_target(target_hash, maybe_max_number).await
return Ok(longest_chain_best)
}
self.selection.finality_target(target_hash, maybe_max_number).await
self.selection
.finality_target_with_fallback(target_hash, longest_chain_best, maybe_max_number)
.await
}
}

Expand Down Expand Up @@ -268,8 +273,7 @@ impl OverseerHandleT for Handle {
}
}

#[async_trait::async_trait]
impl<B, OH> SelectChain<PolkadotBlock> for SelectRelayChain<B, OH>
impl<B, OH> SelectRelayChain<B, OH>
where
B: HeaderProviderProvider<PolkadotBlock>,
OH: OverseerHandleT,
Expand Down Expand Up @@ -313,14 +317,15 @@ where
///
/// It will also constrain the chain to only chains which are fully
/// approved, and chains which contain no disputes.
async fn finality_target(
pub(crate) async fn finality_target_with_fallback(
&self,
target_hash: Hash,
best_leaf: Option<Hash>,
maybe_max_number: Option<BlockNumber>,
) -> Result<Option<Hash>, ConsensusError> {
let mut overseer = self.overseer.clone();

let subchain_head = {
let subchain_head = if cfg!(feature = "disputes") {
let (tx, rx) = oneshot::channel();
overseer
.send_msg(
Expand All @@ -339,6 +344,11 @@ where
None => return Ok(Some(target_hash)),
Some(best) => best,
}
} else {
match best_leaf {
None => return Ok(Some(target_hash)),
Some(best_leaf) => best_leaf,
}
drahnr marked this conversation as resolved.
Show resolved Hide resolved
};

let target_number = self.block_number(target_hash)?;
Expand Down Expand Up @@ -419,27 +429,32 @@ where
let lag = initial_leaf_number.saturating_sub(subchain_number);
self.metrics.note_approval_checking_finality_lag(lag);

// 3. Constrain according to disputes:
let (tx, rx) = oneshot::channel();
overseer
.send_msg(
DisputeCoordinatorMessage::DetermineUndisputedChain {
base_number: target_number,
block_descriptions: subchain_block_descriptions,
tx,
},
std::any::type_name::<Self>(),
)
.await;
let (subchain_number, subchain_head) = rx
.await
.map_err(Error::OverseerDisconnected)
.map_err(|e| ConsensusError::Other(Box::new(e)))?
.unwrap_or_else(|| (subchain_number, subchain_head));
let lag = if cfg!(feature = "disputes") {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

// 3. Constrain according to disputes:
let (tx, rx) = oneshot::channel();
overseer
.send_msg(
DisputeCoordinatorMessage::DetermineUndisputedChain {
base_number: target_number,
block_descriptions: subchain_block_descriptions,
tx,
},
std::any::type_name::<Self>(),
)
.await;
let (subchain_number, _subchain_head) = rx
.await
.map_err(Error::OverseerDisconnected)
.map_err(|e| ConsensusError::Other(Box::new(e)))?
.unwrap_or_else(|| (subchain_number, subchain_head));

// The the total lag accounting for disputes.
let lag_disputes = initial_leaf_number.saturating_sub(subchain_number);
self.metrics.note_disputes_finality_lag(lag_disputes);
// The the total lag accounting for disputes.
let lag_disputes = initial_leaf_number.saturating_sub(subchain_number);
self.metrics.note_disputes_finality_lag(lag_disputes);
lag_disputes
} else {
lag
};

// 4. Apply the maximum safeguard to the finality lag.
if lag > MAX_FINALITY_LAG {
Copy link
Member

@shawntabrizi shawntabrizi Aug 20, 2021

Choose a reason for hiding this comment

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

im reviewing without context of all the surrounding code, but the behavior here before and after is different.

This check before was always checking against lag, and with the new code, could check against lag_disputes with the feature flag.

Was this a bug before? or a bug now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Bug before, addressed here. Approval and disputes both apply constraints to the finalizable chain and previously the disputes lag wasn't accounted for in the 'max' check.

Expand Down
14 changes: 12 additions & 2 deletions node/service/src/tests.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#![allow(dead_code)]
eskimor marked this conversation as resolved.
Show resolved Hide resolved
// Copyright 2021 Parity Technologies (UK) Ltd.
// This file is part of Polkadot.

Expand Down Expand Up @@ -35,7 +36,6 @@ use std::{
use assert_matches::assert_matches;
use std::{sync::Arc, time::Duration};

use consensus_common::SelectChain;
use futures::{channel::oneshot, prelude::*};
use polkadot_primitives::v1::{Block, BlockNumber, Hash, Header};
use polkadot_subsystem::messages::{
Expand Down Expand Up @@ -88,7 +88,10 @@ fn test_harness<T: Future<Output = VirtualOverseer>>(

let target_hash = case_vars.target_block.clone();
let selection_process = async move {
let best = select_relay_chain.finality_target(target_hash, None).await.unwrap();
let best = select_relay_chain
.finality_target_with_fallback(target_hash, Some(target_hash), None)
.await
.unwrap();
finality_target_tx.send(best).unwrap();
()
};
Expand Down Expand Up @@ -722,36 +725,43 @@ fn chain_6() -> CaseVars {
}
}

#[cfg(feature = "disputes")]
#[test]
fn chain_sel_0() {
run_specialized_test_w_harness(chain_0);
}

#[cfg(feature = "disputes")]
#[test]
fn chain_sel_1() {
run_specialized_test_w_harness(chain_1);
}

#[cfg(feature = "disputes")]
#[test]
fn chain_sel_2() {
run_specialized_test_w_harness(chain_2);
}

#[cfg(feature = "disputes")]
#[test]
fn chain_sel_3() {
run_specialized_test_w_harness(chain_3);
}

#[cfg(feature = "disputes")]
#[test]
fn chain_sel_4_target_hash_value_not_contained() {
run_specialized_test_w_harness(chain_4);
}

#[cfg(feature = "disputes")]
#[test]
fn chain_sel_5_best_is_target_hash() {
run_specialized_test_w_harness(chain_5);
}

#[cfg(feature = "disputes")]
#[test]
fn chain_sel_6_approval_lag() {
run_specialized_test_w_harness(chain_6);
Expand Down