Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix mandatory headers scanning in on-demand relay #1306

Merged
merged 5 commits into from
Feb 2, 2022
Merged

Fix mandatory headers scanning in on-demand relay #1306

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a6b1302
more logging in on-demand headerss
svyatonik Feb 1, 2022
3181f9a
remove `maximal_headers_difference` concept from on-demand-relay
svyatonik Feb 2, 2022
4dc6149
another leftover from previous on-demand version
svyatonik Feb 2, 2022
fa659f3
removed extra log
svyatonik Feb 2, 2022
06ee2da
fmt
svyatonik Feb 2, 2022
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
16 changes: 0 additions & 16 deletions relays/bin-substrate/src/cli/relay_headers_and_messages.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,10 +133,6 @@ macro_rules! select_bridge {
type LeftAccountIdConverter = bp_millau::AccountIdConverter;
type RightAccountIdConverter = bp_rialto::AccountIdConverter;

const MAX_MISSING_LEFT_HEADERS_AT_RIGHT: bp_millau::BlockNumber =
bp_millau::SESSION_LENGTH;
const MAX_MISSING_RIGHT_HEADERS_AT_LEFT: bp_rialto::BlockNumber =
bp_rialto::SESSION_LENGTH;
const LEFT_RUNTIME_VERSION: Option<sp_version::RuntimeVersion> =
Some(millau_runtime::VERSION);
const RIGHT_RUNTIME_VERSION: Option<sp_version::RuntimeVersion> =
Expand Down Expand Up @@ -185,11 +181,6 @@ macro_rules! select_bridge {
type LeftAccountIdConverter = bp_rococo::AccountIdConverter;
type RightAccountIdConverter = bp_wococo::AccountIdConverter;

const MAX_MISSING_LEFT_HEADERS_AT_RIGHT: bp_rococo::BlockNumber =
bp_rococo::SESSION_LENGTH;
const MAX_MISSING_RIGHT_HEADERS_AT_LEFT: bp_wococo::BlockNumber =
bp_wococo::SESSION_LENGTH;

const LEFT_RUNTIME_VERSION: Option<sp_version::RuntimeVersion> =
Some(bp_rococo::VERSION);
const RIGHT_RUNTIME_VERSION: Option<sp_version::RuntimeVersion> =
Expand Down Expand Up @@ -268,11 +259,6 @@ macro_rules! select_bridge {
type LeftAccountIdConverter = bp_kusama::AccountIdConverter;
type RightAccountIdConverter = bp_polkadot::AccountIdConverter;

const MAX_MISSING_LEFT_HEADERS_AT_RIGHT: bp_kusama::BlockNumber =
bp_kusama::SESSION_LENGTH;
const MAX_MISSING_RIGHT_HEADERS_AT_LEFT: bp_polkadot::BlockNumber =
bp_polkadot::SESSION_LENGTH;

const LEFT_RUNTIME_VERSION: Option<sp_version::RuntimeVersion> =
Some(bp_kusama::VERSION);
const RIGHT_RUNTIME_VERSION: Option<sp_version::RuntimeVersion> =
Expand Down Expand Up @@ -543,14 +529,12 @@ impl RelayHeadersAndMessages {
left_client.clone(),
right_client.clone(),
left_to_right_transaction_params,
MAX_MISSING_LEFT_HEADERS_AT_RIGHT,
params.shared.only_mandatory_headers,
);
let right_to_left_on_demand_headers = OnDemandHeadersRelay::new::<RightToLeftFinality>(
right_client.clone(),
left_client.clone(),
right_to_left_transaction_params,
MAX_MISSING_RIGHT_HEADERS_AT_LEFT,
params.shared.only_mandatory_headers,
);

Expand Down
92 changes: 44 additions & 48 deletions relays/lib-substrate-relay/src/on_demand_headers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

use async_std::sync::{Arc, Mutex};
use futures::{select, FutureExt};
use num_traits::{CheckedSub, One, Zero};
use num_traits::{One, Zero};

use finality_relay::{FinalitySyncParams, SourceHeader, TargetClient as FinalityTargetClient};
use relay_substrate_client::{
Expand Down Expand Up @@ -55,7 +55,6 @@ impl<SourceChain: Chain> OnDemandHeadersRelay<SourceChain> {
source_client: Client<P::SourceChain>,
target_client: Client<P::TargetChain>,
target_transaction_params: TransactionParams<AccountKeyPairOf<P::TransactionSignScheme>>,
maximal_headers_difference: BlockNumberOf<P::SourceChain>,
only_mandatory_headers: bool,
) -> Self
where
Expand All @@ -73,7 +72,6 @@ impl<SourceChain: Chain> OnDemandHeadersRelay<SourceChain> {
source_client,
target_client,
target_transaction_params,
maximal_headers_difference,
only_mandatory_headers,
required_header_number,
)
Expand Down Expand Up @@ -105,7 +103,6 @@ async fn background_task<P: SubstrateFinalitySyncPipeline>(
source_client: Client<P::SourceChain>,
target_client: Client<P::TargetChain>,
target_transaction_params: TransactionParams<AccountKeyPairOf<P::TransactionSignScheme>>,
maximal_headers_difference: BlockNumberOf<P::SourceChain>,
only_mandatory_headers: bool,
required_header_number: RequiredHeaderNumberRef<P::SourceChain>,
) where
Expand Down Expand Up @@ -165,15 +162,28 @@ async fn background_task<P: SubstrateFinalitySyncPipeline>(
}

// submit mandatory header if some headers are missing
let best_finalized_source_header_at_source_fmt =
format!("{:?}", best_finalized_source_header_at_source);
let best_finalized_source_header_at_target_fmt =
format!("{:?}", best_finalized_source_header_at_target);
let required_header_number_value = *required_header_number.lock().await;
let mandatory_scan_range = mandatory_headers_scan_range::<P::SourceChain>(
best_finalized_source_header_at_source.ok(),
best_finalized_source_header_at_target.ok(),
maximal_headers_difference,
&required_header_number,
required_header_number_value,
)
.await;

log::trace!(
target: "bridge",
"Mandatory headers scan range in {}: ({:?}, {:?}, {:?}) -> {:?}",
relay_task_name,
required_header_number_value,
best_finalized_source_header_at_source_fmt,
best_finalized_source_header_at_target_fmt,
mandatory_scan_range,
);

if let Some(mandatory_scan_range) = mandatory_scan_range {
let relay_mandatory_header_result = relay_mandatory_header_from_range(
&finality_source,
Expand Down Expand Up @@ -227,6 +237,25 @@ async fn background_task<P: SubstrateFinalitySyncPipeline>(

// start/restart relay
if restart_relay {
let stall_timeout = relay_substrate_client::transaction_stall_timeout(
target_transactions_mortality,
P::TargetChain::AVERAGE_BLOCK_INTERVAL,
STALL_TIMEOUT,
);

log::info!(
target: "bridge",
"Starting {} relay\n\t\
Only mandatory headers: {}\n\t\
Tx mortality: {:?} (~{}m)\n\t\
Stall timeout: {:?}",
relay_task_name,
only_mandatory_headers,
target_transactions_mortality,
stall_timeout.as_secs_f64() / 60.0f64,
stall_timeout,
);

finality_relay_task.set(
finality_relay::run(
finality_source.clone(),
Expand All @@ -237,11 +266,7 @@ async fn background_task<P: SubstrateFinalitySyncPipeline>(
P::TargetChain::AVERAGE_BLOCK_INTERVAL,
),
recent_finality_proofs_limit: RECENT_FINALITY_PROOFS_LIMIT,
stall_timeout: relay_substrate_client::transaction_stall_timeout(
target_transactions_mortality,
P::TargetChain::AVERAGE_BLOCK_INTERVAL,
STALL_TIMEOUT,
),
stall_timeout,
only_mandatory_headers,
},
MetricsParams::disabled(),
Expand All @@ -260,11 +285,8 @@ async fn background_task<P: SubstrateFinalitySyncPipeline>(
async fn mandatory_headers_scan_range<C: Chain>(
best_finalized_source_header_at_source: Option<C::BlockNumber>,
best_finalized_source_header_at_target: Option<C::BlockNumber>,
maximal_headers_difference: C::BlockNumber,
required_header_number: &RequiredHeaderNumberRef<C>,
required_header_number: BlockNumberOf<C>,
) -> Option<(C::BlockNumber, C::BlockNumber)> {
let required_header_number = *required_header_number.lock().await;

// if we have been unable to read header number from the target, then let's assume
// that it is the same as required header number. Otherwise we risk submitting
// unneeded transactions
Expand All @@ -276,23 +298,8 @@ async fn mandatory_headers_scan_range<C: Chain>(
let best_finalized_source_header_at_source =
best_finalized_source_header_at_source.unwrap_or(best_finalized_source_header_at_target);

// if there are too many source headers missing from the target node, sync mandatory
// headers to target
//
// why do we need that? When complex headers+messages relay is used, it'll normally only relay
// headers when there are undelivered messages/confirmations. But security model of the
// `pallet-bridge-grandpa` module relies on the fact that headers are synced in real-time and
// that it'll see authorities-change header before unbonding period will end for previous
// authorities set.
let current_headers_difference = best_finalized_source_header_at_source
.checked_sub(&best_finalized_source_header_at_target)
.unwrap_or_else(Zero::zero);
if current_headers_difference <= maximal_headers_difference {
return None
}

// if relay is already asked to sync headers, don't do anything yet
if required_header_number > best_finalized_source_header_at_target {
// if relay is already asked to sync more headers than we have at source, don't do anything yet
if required_header_number >= best_finalized_source_header_at_source {
return None
}

Expand Down Expand Up @@ -423,29 +430,18 @@ mod tests {
const AT_TARGET: Option<bp_rococo::BlockNumber> = Some(1);

#[async_std::test]
async fn mandatory_headers_scan_range_selects_range_if_too_many_headers_are_missing() {
async fn mandatory_headers_scan_range_selects_range_if_some_headers_are_missing() {
assert_eq!(
mandatory_headers_scan_range::<TestChain>(
AT_SOURCE,
AT_TARGET,
5,
&Arc::new(Mutex::new(0))
)
.await,
mandatory_headers_scan_range::<TestChain>(AT_SOURCE, AT_TARGET, 0,).await,
Some((AT_TARGET.unwrap() + 1, AT_SOURCE.unwrap())),
);
}

#[async_std::test]
async fn mandatory_headers_scan_range_selects_nothing_if_enough_headers_are_relayed() {
async fn mandatory_headers_scan_range_selects_nothing_if_already_queued() {
assert_eq!(
mandatory_headers_scan_range::<TestChain>(
AT_SOURCE,
AT_TARGET,
10,
&Arc::new(Mutex::new(0))
)
.await,
mandatory_headers_scan_range::<TestChain>(AT_SOURCE, AT_TARGET, AT_SOURCE.unwrap(),)
.await,
None,
);
}
Expand Down