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

approval-voting: populate session cache in advance #3954

Merged
11 commits merged into from
Sep 29, 2021
4 changes: 2 additions & 2 deletions node/core/approval-voting/src/import.rs
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ pub(crate) async fn handle_new_head(
}
};

match state.session_window.cache_session_info_for_head(ctx, head, &header).await {
match state.session_window.cache_session_info_for_head(ctx, head).await {
Err(e) => {
tracing::debug!(
target: LOG_TARGET,
Expand Down Expand Up @@ -1236,7 +1236,7 @@ pub(crate) mod tests {
h,
RuntimeApiRequest::SessionIndexForChild(c_tx),
)) => {
assert_eq!(h, parent_hash.clone());
assert_eq!(h, hash);
let _ = c_tx.send(Ok(session));
}
);
Expand Down
4 changes: 2 additions & 2 deletions node/core/approval-voting/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -786,8 +786,8 @@ async fn import_block(
RuntimeApiRequest::SessionIndexForChild(s_tx)
)
) => {
let hash = &hashes[number.saturating_sub(1) as usize];
assert_eq!(req_block_hash, hash.0.clone());
let hash = &hashes[number as usize];
assert_eq!(req_block_hash, hash.0);
s_tx.send(Ok(number.into())).unwrap();
}
);
Expand Down
17 changes: 1 addition & 16 deletions node/core/dispute-coordinator/src/real/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -478,22 +478,7 @@ async fn handle_new_activations(
new_activations: impl IntoIterator<Item = Hash>,
) -> Result<(), Error> {
for new_leaf in new_activations {
let block_header = {
let (tx, rx) = oneshot::channel();

ctx.send_message(ChainApiMessage::BlockHeader(new_leaf, tx)).await;

match rx.await?? {
None => continue,
Some(header) => header,
}
};

match state
.rolling_session_window
.cache_session_info_for_head(ctx, new_leaf, &block_header)
.await
{
match state.rolling_session_window.cache_session_info_for_head(ctx, new_leaf).await {
Err(e) => {
tracing::warn!(
target: LOG_TARGET,
Expand Down
3 changes: 2 additions & 1 deletion node/primitives/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,8 @@ pub const POV_BOMB_LIMIT: usize = (MAX_POV_SIZE * 4u32) as usize;
/// On Polkadot this is 1 day, and on Kusama it's 6 hours.
///
/// Number of sessions we want to consider in disputes.
pub const DISPUTE_WINDOW: SessionIndex = 6;
/// + 1 for the child's session.
pub const DISPUTE_WINDOW: SessionIndex = 6 + 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this change is necessary.

  1. BABE session changes occur at slots, so at particular points in time.
  2. The parachains runtime defers all session changes by a single block in order to have predictability of the validator-set.
  3. Therefore, if we've imported one block which has SessionIndexForChild(b) > SessionIndex(b), any blocks constructed by honest validators with accurate clocks after b will have SessionIndexForChild(b') >= SessionIndexForChild(b).
  4. What this means is that the change-set of being more aggressive with advancing the rolling session window will lead us to evict data at most 1 block-time (6 seconds) earlier than we could without the change. As this is dwarfed by the size of the window overall (hundreds or thousands of blocks), it doesn't justify a parameter tweak.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, but this param should be fetched from runtime anyway (#3227) and this change doesn't harm.


/// The cumulative weight of a block in a fork-choice rule.
pub type BlockWeight = u32;
Expand Down
21 changes: 8 additions & 13 deletions node/subsystem-util/src/rolling_session_window.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
//! This is useful for consensus components which need to stay up-to-date about recent sessions but don't
//! care about the state of particular blocks.

use polkadot_primitives::v1::{Hash, Header, SessionIndex, SessionInfo};
use polkadot_primitives::v1::{Hash, SessionIndex, SessionInfo};

use futures::channel::oneshot;
use polkadot_node_subsystem::{
Expand Down Expand Up @@ -131,7 +131,7 @@ impl RollingSessionWindow {
}

/// When inspecting a new import notification, updates the session info cache to match
/// the session of the imported block.
/// the session of the imported block's child.
///
/// this only needs to be called on heads where we are directly notified about import, as sessions do
/// not change often and import notifications are expected to be typically increasing in session number.
Expand All @@ -141,7 +141,6 @@ impl RollingSessionWindow {
&mut self,
ctx: &mut (impl SubsystemContext + overseer::SubsystemContext),
block_hash: Hash,
block_header: &Header,
) -> Result<SessionWindowUpdate, SessionsUnavailable> {
if self.window_size == 0 {
return Ok(SessionWindowUpdate::Unchanged)
Expand All @@ -150,11 +149,9 @@ impl RollingSessionWindow {
let session_index = {
let (s_tx, s_rx) = oneshot::channel();

// The genesis is guaranteed to be at the beginning of the session and its parent state
// is non-existent. Therefore if we're at the genesis, we request using its state and
// not the parent.
// We're requesting session index of a child to populate the cache in advance.
ctx.send_message(RuntimeApiMessage::Request(
if block_header.number == 0 { block_hash } else { block_header.parent_hash },
block_hash,
RuntimeApiRequest::SessionIndexForChild(s_tx),
))
.await;
Expand Down Expand Up @@ -289,6 +286,7 @@ mod tests {
use assert_matches::assert_matches;
use polkadot_node_subsystem::messages::{AllMessages, AvailabilityRecoveryMessage};
use polkadot_node_subsystem_test_helpers::make_subsystem_context;
use polkadot_primitives::v1::Header;
use sp_core::testing::TaskExecutor;

const TEST_WINDOW_SIZE: SessionIndex = 6;
Expand Down Expand Up @@ -329,9 +327,8 @@ mod tests {
let hash = header.hash();

let test_fut = {
let header = header.clone();
Box::pin(async move {
window.cache_session_info_for_head(&mut ctx, hash, &header).await.unwrap();
window.cache_session_info_for_head(&mut ctx, hash).await.unwrap();

assert_eq!(window.earliest_session, Some(expected_start_session));
assert_eq!(
Expand Down Expand Up @@ -497,9 +494,8 @@ mod tests {
let hash = header.hash();

let test_fut = {
let header = header.clone();
Box::pin(async move {
let res = window.cache_session_info_for_head(&mut ctx, hash, &header).await;
let res = window.cache_session_info_for_head(&mut ctx, hash).await;

assert!(res.is_err());
})
Expand Down Expand Up @@ -559,9 +555,8 @@ mod tests {
let hash = header.hash();

let test_fut = {
let header = header.clone();
Box::pin(async move {
window.cache_session_info_for_head(&mut ctx, hash, &header).await.unwrap();
window.cache_session_info_for_head(&mut ctx, hash).await.unwrap();

assert_eq!(window.earliest_session, Some(session));
assert_eq!(window.session_info, vec![dummy_session_info(session)]);
Expand Down