Availability store subsystem by montekki · Pull Request #1404 · paritytech/polkadot

montekki · 2020-07-14T16:23:02Z

No description provided.

node/subsystem/src/messages.rs

roadmap/implementers-guide/src/types/overseer-protocol.md

node/core/av-store/src/lib.rs

rphmeier · 2020-07-22T16:14:36Z

@ordian This is the only subsystem that does storage, and all storage updates are atomic.

coriolinus

A few nits, but overall, looks good to me.

coriolinus · 2020-07-24T08:15:33Z

node/core/av-store/src/lib.rs

+// along with Polkadot.  If not, see <http://www.gnu.org/licenses/>.
+
+//! Implements a `AvailabilityStoreSubsystem`.
+//!


Trailing empty comment.

Should be expanded a bit (:

coriolinus · 2020-07-24T08:36:55Z

node/core/av-store/src/lib.rs

+	use AvailabilityStoreMessage::*;
+	match msg {
+		QueryAvailableData(hash, tx) => {
+			let _ = tx.send(available_data(db, &hash).map(|d| d.data));


Is it safe to swallow send errors? Why not just propagate them?

coriolinus · 2020-07-24T08:49:05Z

node/subsystem/src/util.rs

+			Ok(Signal(BlockFinalized(_))) => {
+				return false;
+			}


Suggested change

Ok(Signal(BlockFinalized(_))) => {

return false;

}

Ok(Signal(BlockFinalized(_))) => {}

We can just let the operation fall through in this case.

drahnr · 2020-07-24T09:37:50Z

node/core/av-store/src/lib.rs

+		let mut db_config = DatabaseConfig::with_columns(columns::NUM_COLUMNS);
+
+		if let Some(cache_size) = config.cache_size {
+			let mut memory_budget = HashMap::new();


Suggested change

let mut memory_budget = HashMap::new();

let mut memory_budget = HashMap::with_capacity(columns::NUM_COLUMNS);

drahnr · 2020-07-24T09:38:33Z

node/core/av-store/src/lib.rs

+			let mut memory_budget = HashMap::new();
+
+			for i in 0..columns::NUM_COLUMNS {
+				memory_budget.insert(i, cache_size / columns::NUM_COLUMNS as usize);
+			}


or

Suggested change

let mut memory_budget = HashMap::new();

for i in 0..columns::NUM_COLUMNS {

memory_budget.insert(i, cache_size / columns::NUM_COLUMNS as usize);

}

let val = cache_size / columns::NUM_COLUMNS;

let memory_budget = 0..columns::NUM_COLUMNS.into_iter().map(|i| (i, val) ).collect::<HashMap>();

drahnr · 2020-07-24T09:45:12Z

node/core/av-store/src/lib.rs

+fn query_inner<D: Decode>(db: &Arc<dyn KeyValueDB>, column: u32, key: &[u8]) -> Option<D> {
+	match db.get(column, key) {
+		Ok(Some(raw)) => {
+			let res = D::decode(&mut &raw[..]).expect("all stored data serialized correctly; qed");


Not entirely convinced this is a good idea, should we not rather warn and return a None?

drahnr · 2020-07-24T09:47:58Z

node/core/av-store/src/lib.rs

+	if let Some(data) = available_data(db, candidate_hash) {
+		let mut chunks = get_chunks(&data.data, data.n_validators as usize)?;
+		let chunk = chunks.get(index as usize).cloned();
+		for chunk in chunks.drain(..) {


This is a bit cryptic, it uses IntoIterator of Option and the for loop really only does one iteration.

I .map() or .and_then() might be more idiomatic, or did you mean to avoid the closure?

yeah, changed the naming of the var; you see here the Option is chunk var and we are draining chunks which are Vec.

drahnr · 2020-07-24T09:49:27Z

node/core/av-store/src/lib.rs

+		futures::pin_mut!(test_fut);
+		futures::pin_mut!(subsystem);
+
+		executor::block_on(future::select(test_fut, subsystem));


this is way simpler than I anticipated! 👍

drahnr · 2020-07-24T09:53:14Z

roadmap/implementers-guide/src/types/overseer-protocol.md

@@ -60,13 +60,16 @@ Messages to and from the availability store.
 ```rust
 enum AvailabilityStoreMessage {
 	/// Query the PoV of a candidate by hash.


I find it a bit unclear what AvailableData here means? The description still refers to the PoV, a bit of elaboration would be helpful

that's defined in availability.md

drahnr

Nothing big here, looks good! Mostly a code check, not a functional review.

ordian

some comments about kvdb usage

node/core/av-store/src/lib.rs

ordian · 2020-07-24T12:25:45Z

node/core/av-store/src/lib.rs

+	}
+
+	#[cfg(test)]
+	fn new_in_memory(inner: Arc<dyn KeyValueDB>) -> Self {


why doesn't it create in_memory db inside the function, I don't see the store used outside?

This is used in testing so the main idea is that it could be useful to have a handle to the db outside of the store so the test could look into it directly; though it is not used at the moment

rphmeier · 2020-07-24T16:08:37Z

Needs a rebase. There are 3 PRs worth of commits in here.

* Apply suggestions from #1364 code review - use CoreState, not CoreOccupied - query for availability chunks, not the whole PoV - create a stub `fn availability_cores` * link to issue documenting unimplemented * implement get_availability_cores by adding a new runtime api request * back out an unrelated change properly part of #1404 * av-store: handle QueryChunkAvailability * simplify QueryDataAvailability * remove extraneous whitespace * compact primitive imports

montekki added A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes labels Jul 14, 2020

montekki changed the title ~~Fs availability store subsystem~~ Availability store subsystem Jul 15, 2020

montekki added C1-low PR touches the given topic and has a low impact on builders. A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jul 21, 2020

montekki marked this pull request as ready for review July 21, 2020 18:13

montekki requested a review from rphmeier July 21, 2020 18:13