-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Availability store subsystem #1404
Availability store subsystem #1404
Conversation
@ordian This is the only subsystem that does storage, and all storage updates are atomic. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few nits, but overall, looks good to me.
node/core/av-store/src/lib.rs
Outdated
// along with Polkadot. If not, see <http://www.gnu.org/licenses/>. | ||
|
||
//! Implements a `AvailabilityStoreSubsystem`. | ||
//! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing empty comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be expanded a bit (:
node/core/av-store/src/lib.rs
Outdated
use AvailabilityStoreMessage::*; | ||
match msg { | ||
QueryAvailableData(hash, tx) => { | ||
let _ = tx.send(available_data(db, &hash).map(|d| d.data)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it safe to swallow send errors? Why not just propagate them?
node/subsystem/src/util.rs
Outdated
Ok(Signal(BlockFinalized(_))) => { | ||
return false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok(Signal(BlockFinalized(_))) => { | |
return false; | |
} | |
Ok(Signal(BlockFinalized(_))) => {} |
We can just let the operation fall through in this case.
let mut db_config = DatabaseConfig::with_columns(columns::NUM_COLUMNS); | ||
|
||
if let Some(cache_size) = config.cache_size { | ||
let mut memory_budget = HashMap::new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let mut memory_budget = HashMap::new(); | |
let mut memory_budget = HashMap::with_capacity(columns::NUM_COLUMNS); |
let mut memory_budget = HashMap::new(); | ||
|
||
for i in 0..columns::NUM_COLUMNS { | ||
memory_budget.insert(i, cache_size / columns::NUM_COLUMNS as usize); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or
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>(); |
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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not entirely convinced this is a good idea, should we not rather warn and return a None?
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(..) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, changed the naming of the var; you see here the Option
is chunk
var and we are draining chunks
which are Vec
.
futures::pin_mut!(test_fut); | ||
futures::pin_mut!(subsystem); | ||
|
||
executor::block_on(future::select(test_fut, subsystem)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is way simpler than I anticipated! 👍
@@ -60,13 +60,16 @@ Messages to and from the availability store. | |||
```rust | |||
enum AvailabilityStoreMessage { | |||
/// Query the PoV of a candidate by hash. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it a bit unclear what AvailableData
here means? The description still refers to the PoV
, a bit of elaboration would be helpful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's defined in availability.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing big here, looks good! Mostly a code check, not a functional review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some comments about kvdb
usage
} | ||
|
||
#[cfg(test)] | ||
fn new_in_memory(inner: Arc<dyn KeyValueDB>) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why doesn't it create in_memory
db inside the function, I don't see the store used outside?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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
No description provided.