RollingSessionWindow cleanup (#7204)

* Replace `RollingSessionWindow` with `RuntimeInfo` - initial commit

* Fix tests in import

* Fix the rest of the tests

* Remove dead code

* Fix todos

* Simplify session caching

* Comments for `SessionInfoProvider`

* Separate `SessionInfoProvider` from `State`

* `cache_session_info_for_head` becomes freestanding function

* Remove unneeded `mut` usage

* fn session_info -> fn get_session_info() to avoid name clashes. The function also tries to initialize `SessionInfoProvider`

* Fix SessionInfo retrieval

* Code cleanup

* Don't wrap `SessionInfoProvider` in an `Option`

* Remove `earliest_session()`

* Remove pre-caching -> wip

* Fix some tests and code cleanup

* Fix all tests

* Fixes in tests

* Fix comments, variable names and small style changes

* Fix a warning

* impl From<SessionWindowSize> for NonZeroUsize

* Fix logging for `get_session_info` - remove redundant logs and decrease log level to DEBUG

* Code review feedback

* Storage migration removing `COL_SESSION_WINDOW_DATA` from parachains db

* Remove `col_session_data` usages

* Storage migration clearing columns w/o removing them

* Remove session data column usages from `approval-voting` and `dispute-coordinator` tests

* Add some test cases from `RollingSessionWindow` to `dispute-coordinator` tests

* Fix formatting in initialized.rs

* Fix a corner case in `SessionInfo` caching for `dispute-coordinator`

* Remove `RollingSessionWindow` ;(

* Revert "Fix formatting in initialized.rs"

This reverts commit 0f94664.

* v2 to v3 migration drops `COL_DISPUTE_COORDINATOR_DATA` instead of clearing it

* Fix `NUM_COLUMNS` in `approval-voting`

* Use `columns::v3::NUM_COLUMNS` when opening db

* Update node/service/src/parachains_db/upgrade.rs

Co-authored-by: Andrei Sandu <[email protected]>

* Don't write in `COL_DISPUTE_COORDINATOR_DATA` for `test_rocksdb_migrate_2_to_3`

* Fix `NUM+COLUMNS` in approval_voting

* Fix formatting

* Fix columns usage

* Clarification comments about the different db versions

---------

Co-authored-by: Andrei Sandu <[email protected]>

node/core/approval-voting/src/approval_db/v1/mod.rs

@@ -15,6 +15,12 @@
 // along with Polkadot.  If not, see <http://www.gnu.org/licenses/>.
 
 //! Version 1 of the DB schema.
+//!
+//! Note that the version here differs from the actual version of the parachains
+//! database (check `CURRENT_VERSION` in `node/service/src/parachains_db/upgrade.rs`).
+//! The code in this module implements the way approval voting works with
+//! its data in the database. Any breaking changes here will still
+//! require a db migration (check `node/service/src/parachains_db/upgrade.rs`).
 
 use parity_scale_codec::{Decode, Encode};
 use polkadot_node_primitives::approval::{AssignmentCert, DelayTranche};
@@ -154,8 +160,6 @@ pub type Bitfield = BitVec<u8, BitOrderLsb0>;
 pub struct Config {
 	/// The column family in the database where data is stored.
 	pub col_approval_data: u32,
-	/// The column of the database where rolling session window data is stored.
-	pub col_session_data: u32,
 }
 
 /// Details pertaining to our assignment on a block.

node/core/approval-voting/src/approval_db/v1/tests.rs

@@ -28,12 +28,10 @@ use std::{collections::HashMap, sync::Arc};
 use ::test_helpers::{dummy_candidate_receipt, dummy_candidate_receipt_bad_sig, dummy_hash};
 
 const DATA_COL: u32 = 0;
-const SESSION_DATA_COL: u32 = 1;
 
-const NUM_COLUMNS: u32 = 2;
+const NUM_COLUMNS: u32 = 1;
 
-const TEST_CONFIG: Config =
-	Config { col_approval_data: DATA_COL, col_session_data: SESSION_DATA_COL };
+const TEST_CONFIG: Config = Config { col_approval_data: DATA_COL };
 
 fn make_db() -> (DbBackend, Arc<dyn Database>) {
 	let db = kvdb_memorydb::create(NUM_COLUMNS);

node/core/approval-voting/src/import.rs

@@ -609,12 +609,10 @@ pub(crate) mod tests {
 	use crate::{approval_db::v1::Config as DatabaseConfig, criteria, BlockEntry};
 
 	const DATA_COL: u32 = 0;
-	const SESSION_DATA_COL: u32 = 1;
 
-	const NUM_COLUMNS: u32 = 2;
+	const NUM_COLUMNS: u32 = 1;
 
-	const TEST_CONFIG: DatabaseConfig =
-		DatabaseConfig { col_approval_data: DATA_COL, col_session_data: SESSION_DATA_COL };
+	const TEST_CONFIG: DatabaseConfig = DatabaseConfig { col_approval_data: DATA_COL };
 	#[derive(Default)]
 	struct MockClock;
 

node/core/approval-voting/src/lib.rs

@@ -116,8 +116,6 @@ const LOG_TARGET: &str = "parachain::approval-voting";
 pub struct Config {
 	/// The column family in the DB where approval-voting data is stored.
 	pub col_approval_data: u32,
-	/// The of the DB where rolling session info is stored.
-	pub col_session_data: u32,
 	/// The slot duration of the consensus algorithm, in milliseconds. Should be evenly
 	/// divisible by 500.
 	pub slot_duration_millis: u64,
@@ -357,10 +355,7 @@ impl ApprovalVotingSubsystem {
 			keystore,
 			slot_duration_millis: config.slot_duration_millis,
 			db,
-			db_config: DatabaseConfig {
-				col_approval_data: config.col_approval_data,
-				col_session_data: config.col_session_data,
-			},
+			db_config: DatabaseConfig { col_approval_data: config.col_approval_data },
 			mode: Mode::Syncing(sync_oracle),
 			metrics,
 		}
@@ -369,10 +364,8 @@ impl ApprovalVotingSubsystem {
 	/// Revert to the block corresponding to the specified `hash`.
 	/// The operation is not allowed for blocks older than the last finalized one.
 	pub fn revert_to(&self, hash: Hash) -> Result<(), SubsystemError> {
-		let config = approval_db::v1::Config {
-			col_approval_data: self.db_config.col_approval_data,
-			col_session_data: self.db_config.col_session_data,
-		};
+		let config =
+			approval_db::v1::Config { col_approval_data: self.db_config.col_approval_data };
 		let mut backend = approval_db::v1::DbBackend::new(self.db.clone(), config);
 		let mut overlay = OverlayedBackend::new(&backend);
 

node/core/approval-voting/src/tests.rs

@@ -14,8 +14,6 @@
 // You should have received a copy of the GNU General Public License
 // along with Polkadot.  If not, see <http://www.gnu.org/licenses/>.
 
-use crate::tests::test_constants::TEST_CONFIG;
-
 use super::*;
 use polkadot_node_primitives::{
 	approval::{
@@ -115,12 +113,10 @@ fn make_sync_oracle(val: bool) -> (Box<dyn SyncOracle + Send>, TestSyncOracleHan
 pub mod test_constants {
 	use crate::approval_db::v1::Config as DatabaseConfig;
 	const DATA_COL: u32 = 0;
-	const SESSION_DATA_COL: u32 = 1;
 
-	pub(crate) const NUM_COLUMNS: u32 = 2;
+	pub(crate) const NUM_COLUMNS: u32 = 1;
 
-	pub(crate) const TEST_CONFIG: DatabaseConfig =
-		DatabaseConfig { col_approval_data: DATA_COL, col_session_data: SESSION_DATA_COL };
+	pub(crate) const TEST_CONFIG: DatabaseConfig = DatabaseConfig { col_approval_data: DATA_COL };
 }
 
 struct MockSupportsParachains;
@@ -493,7 +489,6 @@ fn test_harness<T: Future<Output = VirtualOverseer>>(
 			Config {
 				col_approval_data: test_constants::TEST_CONFIG.col_approval_data,
 				slot_duration_millis: SLOT_DURATION_MILLIS,
-				col_session_data: TEST_CONFIG.col_session_data,
 			},
 			Arc::new(db),
 			Arc::new(keystore),

node/core/dispute-coordinator/src/db/v1.rs

@@ -15,6 +15,12 @@
 // along with Polkadot.  If not, see <http://www.gnu.org/licenses/>.
 
 //! `V1` database for the dispute coordinator.
+//!
+//! Note that the version here differs from the actual version of the parachains
+//! database (check `CURRENT_VERSION` in `node/service/src/parachains_db/upgrade.rs`).
+//! The code in this module implements the way dispute coordinator works with
+//! the dispute data in the database. Any breaking changes here will still
+//! require a db migration (check `node/service/src/parachains_db/upgrade.rs`).
 
 use polkadot_node_primitives::DisputeStatus;
 use polkadot_node_subsystem_util::database::{DBTransaction, Database};
@@ -206,8 +212,6 @@ fn candidate_votes_session_prefix(session: SessionIndex) -> [u8; 15 + 4] {
 pub struct ColumnConfiguration {
 	/// The column in the key-value DB where data is stored.
 	pub col_dispute_data: u32,
-	/// The column in the key-value DB where session data is stored.
-	pub col_session_data: u32,
 }
 
 /// Tracked votes on candidates, for the purposes of dispute resolution.
@@ -378,7 +382,7 @@ mod tests {
 		let db = kvdb_memorydb::create(1);
 		let db = polkadot_node_subsystem_util::database::kvdb_impl::DbAdapter::new(db, &[0]);
 		let store = Arc::new(db);
-		let config = ColumnConfiguration { col_dispute_data: 0, col_session_data: 1 };
+		let config = ColumnConfiguration { col_dispute_data: 0 };
 		DbBackend::new(store, config, Metrics::default())
 	}
 

node/core/dispute-coordinator/src/initialized.rs

@@ -305,13 +305,12 @@ impl Initialized {
 				Ok(session_idx)
 					if self.gaps_in_cache || session_idx > self.highest_session_seen =>
 				{
-					// If error has occurred during last session caching - fetch the whole window
-					// Otherwise - cache only the new sessions
-					let lower_bound = if self.gaps_in_cache {
-						session_idx.saturating_sub(DISPUTE_WINDOW.get() - 1)
-					} else {
-						self.highest_session_seen + 1
-					};
+					// Fetch the last `DISPUTE_WINDOW` number of sessions unless there are no gaps in
+					// cache and we are not missing too many `SessionInfo`s
+					let mut lower_bound = session_idx.saturating_sub(DISPUTE_WINDOW.get() - 1);
+					if !self.gaps_in_cache && self.highest_session_seen > lower_bound {
+						lower_bound = self.highest_session_seen + 1
+					}
 
 					// There is a new session. Perform a dummy fetch to cache it.
 					for idx in lower_bound..=session_idx {

node/core/dispute-coordinator/src/lib.rs

@@ -127,16 +127,11 @@ pub struct DisputeCoordinatorSubsystem {
 pub struct Config {
 	/// The data column in the store to use for dispute data.
 	pub col_dispute_data: u32,
-	/// The data column in the store to use for session data.
-	pub col_session_data: u32,
 }
 
 impl Config {
 	fn column_config(&self) -> db::v1::ColumnConfiguration {
-		db::v1::ColumnConfiguration {
-			col_dispute_data: self.col_dispute_data,
-			col_session_data: self.col_session_data,
-		}
+		db::v1::ColumnConfiguration { col_dispute_data: self.col_dispute_data }
 	}
 }
 

node/core/dispute-coordinator/src/tests.rs

@@ -33,6 +33,7 @@ use polkadot_node_subsystem_util::database::Database;
 
 use polkadot_node_primitives::{
 	DisputeMessage, DisputeStatus, SignedDisputeStatement, SignedFullStatement, Statement,
+	DISPUTE_WINDOW,
 };
 use polkadot_node_subsystem::{
 	messages::{
@@ -214,9 +215,9 @@ impl Default for TestState {
 			make_keystore(vec![Sr25519Keyring::Alice.to_seed()].into_iter()).into();
 
 		let db = kvdb_memorydb::create(1);
-		let db = polkadot_node_subsystem_util::database::kvdb_impl::DbAdapter::new(db, &[]);
+		let db = polkadot_node_subsystem_util::database::kvdb_impl::DbAdapter::new(db, &[0]);
 		let db = Arc::new(db);
-		let config = Config { col_dispute_data: 0, col_session_data: 1 };
+		let config = Config { col_dispute_data: 0 };
 
 		let genesis_header = Header {
 			parent_hash: Hash::zero(),
@@ -330,9 +331,11 @@ impl TestState {
 					assert_eq!(h, block_hash);
 					let _ = tx.send(Ok(session));
 
+					let first_expected_session = session.saturating_sub(DISPUTE_WINDOW.get() - 1);
+
 					// Queries for session caching - see `handle_startup`
 					if self.known_session.is_none() {
-						for i in 0..=session {
+						for i in first_expected_session..=session {
 							assert_matches!(
 								overseer_recv(virtual_overseer).await,
 								AllMessages::RuntimeApi(RuntimeApiMessage::Request(
@@ -3393,3 +3396,174 @@ fn informs_chain_selection_when_dispute_concluded_against() {
 		})
 	});
 }
+
+// On startup `SessionInfo` cache should be populated
+#[test]
+fn session_info_caching_on_startup_works() {
+	test_harness(|mut test_state, mut virtual_overseer| {
+		Box::pin(async move {
+			let session = 1;
+
+			test_state.handle_resume_sync(&mut virtual_overseer, session).await;
+
+			test_state
+		})
+	});
+}
+
+// Underflow means that no more than `DISPUTE_WINDOW` sessions should be fetched on startup
+#[test]
+fn session_info_caching_doesnt_underflow() {
+	test_harness(|mut test_state, mut virtual_overseer| {
+		Box::pin(async move {
+			let session = DISPUTE_WINDOW.get() + 1;
+
+			test_state.handle_resume_sync(&mut virtual_overseer, session).await;
+
+			test_state
+		})
+	});
+}
+
+// Cached `SessionInfo` shouldn't be re-requested from the runtime
+#[test]
+fn session_info_is_requested_only_once() {
+	test_harness(|mut test_state, mut virtual_overseer| {
+		Box::pin(async move {
+			let session = 1;
+
+			test_state.handle_resume_sync(&mut virtual_overseer, session).await;
+
+			// This leaf activation shouldn't fetch `SessionInfo` because the session is already cached
+			test_state
+				.activate_leaf_at_session(
+					&mut virtual_overseer,
+					session,
+					3,
+					vec![make_candidate_included_event(make_valid_candidate_receipt())],
+				)
+				.await;
+
+			// This leaf activation should fetch `SessionInfo` because the session is new
+			test_state
+				.activate_leaf_at_session(
+					&mut virtual_overseer,
+					session + 1,
+					4,
+					vec![make_candidate_included_event(make_valid_candidate_receipt())],
+				)
+				.await;
+
+			assert_matches!(
+				virtual_overseer.recv().await,
+				AllMessages::RuntimeApi(RuntimeApiMessage::Request(
+					_,
+					RuntimeApiRequest::SessionInfo(session_index, tx),
+				)) => {
+					assert_eq!(session_index, 2);
+					let _ = tx.send(Ok(Some(test_state.session_info())));
+				}
+			);
+			test_state
+		})
+	});
+}
+
+// Big jump means the new session we see with a leaf update is at least a `DISPUTE_WINDOW` bigger than
+// the already known one. In this case The whole `DISPUTE_WINDOW` should be fetched.
+#[test]
+fn session_info_big_jump_works() {
+	test_harness(|mut test_state, mut virtual_overseer| {
+		Box::pin(async move {
+			let session_on_startup = 1;
+
+			test_state.handle_resume_sync(&mut virtual_overseer, session_on_startup).await;
+
+			// This leaf activation shouldn't fetch `SessionInfo` because the session is already cached
+			test_state
+				.activate_leaf_at_session(
+					&mut virtual_overseer,
+					session_on_startup,
+					3,
+					vec![make_candidate_included_event(make_valid_candidate_receipt())],
+				)
+				.await;
+
+			let session_after_jump = session_on_startup + DISPUTE_WINDOW.get() + 10;
+			// This leaf activation should cache all missing `SessionInfo`s
+			test_state
+				.activate_leaf_at_session(
+					&mut virtual_overseer,
+					session_after_jump,
+					4,
+					vec![make_candidate_included_event(make_valid_candidate_receipt())],
+				)
+				.await;
+
+			let first_expected_session =
+				session_after_jump.saturating_sub(DISPUTE_WINDOW.get() - 1);
+			for expected_idx in first_expected_session..=session_after_jump {
+				assert_matches!(
+					virtual_overseer.recv().await,
+					AllMessages::RuntimeApi(RuntimeApiMessage::Request(
+						_,
+						RuntimeApiRequest::SessionInfo(session_index, tx),
+					)) => {
+						assert_eq!(session_index, expected_idx);
+						let _ = tx.send(Ok(Some(test_state.session_info())));
+					}
+				);
+			}
+			test_state
+		})
+	});
+}
+
+// Small jump means the new session we see with a leaf update is at less than last known one + `DISPUTE_WINDOW`. In this
+// case fetching should start from last known one + 1.
+#[test]
+fn session_info_small_jump_works() {
+	test_harness(|mut test_state, mut virtual_overseer| {
+		Box::pin(async move {
+			let session_on_startup = 1;
+
+			test_state.handle_resume_sync(&mut virtual_overseer, session_on_startup).await;
+
+			// This leaf activation shouldn't fetch `SessionInfo` because the session is already cached
+			test_state
+				.activate_leaf_at_session(
+					&mut virtual_overseer,
+					session_on_startup,
+					3,
+					vec![make_candidate_included_event(make_valid_candidate_receipt())],
+				)
+				.await;
+
+			let session_after_jump = session_on_startup + DISPUTE_WINDOW.get() - 1;
+			// This leaf activation should cache all missing `SessionInfo`s
+			test_state
+				.activate_leaf_at_session(
+					&mut virtual_overseer,
+					session_after_jump,
+					4,
+					vec![make_candidate_included_event(make_valid_candidate_receipt())],
+				)
+				.await;
+
+			let first_expected_session = session_on_startup + 1;
+			for expected_idx in first_expected_session..=session_after_jump {
+				assert_matches!(
+					virtual_overseer.recv().await,
+					AllMessages::RuntimeApi(RuntimeApiMessage::Request(
+						_,
+						RuntimeApiRequest::SessionInfo(session_index, tx),
+					)) => {
+						assert_eq!(session_index, expected_idx);
+						let _ = tx.send(Ok(Some(test_state.session_info())));
+					}
+				);
+			}
+			test_state
+		})
+	});
+}