-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Erasure encoding availability #345
Erasure encoding availability #345
Conversation
* Modifications to availability store to keep chunks as well as reconstructed blocks and extrinsics. * Gossip messages containig signed erasure chunks. * Requesting eraure chunks with polkadot-specific messages. * Validation of erasure chunk messages.
@@ -101,6 +106,21 @@ impl GossipStatement { | |||
} | |||
} | |||
|
|||
/// A gossip message containing one erasure chunk of a candidate block. | |||
/// For each chunk of block erasure encoding one of this messages is constructed. |
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.
the grammar on this line doesn't sound correct
The PR is updated with implementation of most of the features described in #475:
There is though an irritating problem of not being able to use
Now, there is also this. Because of the move to |
/// A trait that provides a shim for the [`NetworkService`] trait. | ||
/// | ||
/// Currently it is not possible to use the networking code in the availability store | ||
/// core directly due to a number of loop dependencies it require: |
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'm hoping that further Substrate improvements allow us to declare a subprotocol directly in this crate.
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.
Reviewed the store changes so far, that's all. Looks OK although I would like to see much more documentation particularly of invariants.
availability-store/src/worker.rs
Outdated
|
||
let handle = thread::spawn(move || { | ||
let mut sender = self.sender.clone(); | ||
let mut runtime = LocalRuntime::new().expect("Could not create local runtime"); |
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 seems like it could fail. Panickers should be proven not to fail or removed.
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 just copies logic in attestation_service
, should we just fail with an error here before starting a thread and return that to the caller?
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.
If possible, that seems reasonable. Is the LocalRuntime
Send
? We could create and then send it if so
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.
No, only a handle to it is
} | ||
} | ||
|
||
self.inner.import_block(block, new_cache).map_err(Into::into) |
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 racing against the background worker handling the messages sent above, so import notifications might come out before the information is registered in the availability store. Not sure what to do about that
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 guess it's fine as long as we implement the network protocol in the end by getting notifications directly from the availability store.
/// | ||
/// This information is needed before the `add_candidates_in_relay_block` is called | ||
/// since that call forms the awaited frontier of chunks. | ||
/// In the current implementation this function is called in the `get_or_instantiate` at |
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 won't block the PR on this, but in general I'd prefer to avoid these kinds of long-distance expectations of how the code should be used. It's an assumption that can easily change without noticing, unlike things like "X is called before Y".
* make message-lane Event generic * cargo fmt --all * Update modules/message-lane/src/lib.rs Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com> Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
availability-store
modified to store block erasure chunks and reconstructblocks when the number of chunks at hand is enough.
make_available
.add_erasure_chunk
method. Among other thing it will check wether the chunk belongs to the Merkle tree of
the block encoding.
block_data
orextrinsic
are called by the user and the result is cached in storage.
candidates_finalized
purges chunks and whole blocks (if any) from the storage.network
extended to pass erasure chunks in the gossip and in polkadot-specificMessage
-s as well.gossip
each gossip message contains a chunk itself, theValidatorIndex
of the validator that has generated this message and it's signature.
checked_statements
stream inrouter
provides signed statements anderasure chunks. If the chunk belongs to the candidate we are not yet aware of,
it is deferred. Chunks are then imported with the
import_erasure_chunk
invalidation
.validation
is modified to gossip the chunks of the local collations to thenetwork:
chunks which are stored into the validator's availability store and also
handed over to the network
local_collation
to be gossiped to the peers.import_erasure_chunk
.