-
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.
| } | ||
|
|
||
| /// 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.
rphmeier
left a 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.
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 <[email protected]> Co-authored-by: Hernando Castano <[email protected]>
availability-storemodified to store block erasure chunks and reconstructblocks when the number of chunks at hand is enough.
make_available.add_erasure_chunkmethod. Among other thing it will check wether the chunk belongs to the Merkle tree of
the block encoding.
block_dataorextrinsicare called by the user and the result is cached in storage.
candidates_finalizedpurges chunks and whole blocks (if any) from the storage.networkextended to pass erasure chunks in the gossip and in polkadot-specificMessage-s as well.gossipeach gossip message contains a chunk itself, theValidatorIndexof the validator that has generated this message and it's signature.
checked_statementsstream inrouterprovides 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_chunkinvalidation.validationis 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_collationto be gossiped to the peers.import_erasure_chunk.