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

runtime/session_info: avoid heavy loop when introduced on a live chain #2099

Merged
merged 4 commits into from
Dec 9, 2020
Merged

runtime/session_info: avoid heavy loop when introduced on a live chain #2099

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1046e95
session_info: a heavy loop test
ordian Dec 9, 2020
db19ccb
session_info: fix a typo
ordian Dec 9, 2020
736bfdc
session_info: fix heavy loop
ordian Dec 9, 2020
162f851
session_info: crank the iterations all the way up
ordian Dec 9, 2020
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
31 changes: 28 additions & 3 deletions runtime/parachains/src/session_info.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,11 @@ impl<T: Config> Module<T> {
// update `EarliestStoredSession` based on `config.dispute_period`
EarliestStoredSession::set(new_earliest_stored_session);
// remove all entries from `Sessions` from the previous value up to the new value
for idx in old_earliest_stored_session..new_earliest_stored_session {
Sessions::remove(&idx);
// avoid a potentially heavy loop when introduced on a live chain
if old_earliest_stored_session != 0 || Sessions::get(0).is_some() {
for idx in old_earliest_stored_session..new_earliest_stored_session {
Sessions::remove(&idx);
}
}
// create a new entry in `Sessions` with information about the current session
let new_session_info = SessionInfo {
Expand Down Expand Up @@ -216,7 +219,7 @@ mod tests {
}

#[test]
fn session_pruning_is_based_on_dispute_deriod() {
fn session_pruning_is_based_on_dispute_period() {
new_test_ext(genesis_config()).execute_with(|| {
run_to_block(100, session_changes);
assert_eq!(EarliestStoredSession::get(), 9);
Expand Down Expand Up @@ -253,4 +256,26 @@ mod tests {
assert_eq!(session.needed_approvals, 42);
})
}

#[test]
fn session_pruning_avoids_heavy_loop() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't figure out how this test ensures that we don't call Sessions::remove for every n < start.

Copy link
Member Author

@ordian ordian Dec 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

My laptop froze when running this test without the fix (on 1046e95).
It calls SessionInfo::initializer_on_new_session(&notification) on a very high session index.

Copy link
Member Author

@ordian ordian Dec 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe relying on a cargo test timeout is not the most elegant way, but this test shouldn't normally fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could add an internal timeout with a few seconds' max time? Like wrap the whole closure in async_std::task::block_on(async move { ... }.timeout(Duration::from_secs(5)), using subsystem_util::TimeoutExt. It would mean adding async_std as a dev-dependency, but I don't think that should be a huge deal.

new_test_ext(genesis_config()).execute_with(|| {
let start = 1_000_000_000;
System::on_initialize(start);
System::set_block_number(start);

if let Some(notification) = new_session_every_block(start) {
Configuration::initializer_on_new_session(&notification.validators, &notification.queued);
SessionInfo::initializer_on_new_session(&notification);
}

Configuration::initializer_initialize(start);
SessionInfo::initializer_initialize(start);

assert_eq!(EarliestStoredSession::get(), start - 1);

run_to_block(start + 1, new_session_every_block);
assert_eq!(EarliestStoredSession::get(), start);
})
}
}