Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial support of executor gossip #219

Merged
merged 16 commits into from
Dec 31, 2021
Merged

Initial support of executor gossip #219

merged 16 commits into from
Dec 31, 2021

Conversation

liuchengxu
Copy link
Contributor

@liuchengxu liuchengxu commented Dec 22, 2021

The gossip message for bundle and execution receipt can be sent and
received as expected in the executor gossip network, the logic of message_expired
and message_allowed is not final but should be on the right track.

This PR will be squash merged.

Now the gossip message for bundle and execution receipt can be sent and
received, but still a lot of works to do.
`OpaqueExecutionReceipt` is introduced as `ExecutionReceipt` has both
concrete type(`primary_hash`) and generic type `Hash`, used by the
client submitting the external ER to runtime.
`GossipMessageHandler` has been changed from `async` to sync due to the
`Validator` trait of network-gossip is not async and it's non-trivial to
submit a patch to the upstream.
@liuchengxu liuchengxu marked this pull request as ready for review December 30, 2021 13:51
}

#[derive(Decode, Encode, TypeInfo, PartialEq, Eq, Clone, RuntimeDebug)]
pub struct OpaqueExecutionReceipt(Vec<u8>);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary? I think both nodes should understand ER fully and have it fully typed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that overseer doesn't understand this type as it does not know the Block::Hash type but that's how ExecutionReceipt is built.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we use a concrete type there then? It should be possible to transform generic type into this specific type and back if need be.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A TODO noted b4c5372.

I was also thinking about if OpaqueExecutionReceipt is really necessary but failed to make it compile, maybe just some Rust issues I haven't managed to fix, a TODO added anyway, will have a look later.

cumulus/client/executor-gossip/src/lib.rs Outdated Show resolved Hide resolved
cumulus/client/cirrus-executor/src/lib.rs Outdated Show resolved Hide resolved
cumulus/client/cirrus-executor/src/lib.rs Show resolved Hide resolved
// - gossip the bundle to the executor peers
// - OnBundleReceivedBySecondaryNode
// - OnBundleEquivocationProof(farmer only)
// - OnInvalidBundleProof(farmer only)
async fn produce_bundle(self, slot_info: ExecutorSlotInfo) -> Option<BundleResult> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here, can't we use &self?

cumulus/client/cirrus-executor/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines +43 to +46
/// Gossip engine messages topic.
fn topic<Block: BlockT>() -> Block::Hash {
<<Block::Header as HeaderT>::Hashing as HashT>::hash(b"executor")
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It only seems to be called in one place, so I'd just inline it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's indeed called once in executor-gossip/src/lib.rs but called a few times in executor-gossip/src/worker.rs.

cumulus/client/executor-gossip/src/lib.rs Show resolved Hide resolved
cumulus/client/executor-gossip/src/lib.rs Outdated Show resolved Hide resolved
cumulus/client/cirrus-executor/src/lib.rs Outdated Show resolved Hide resolved
nazar-pc
nazar-pc previously approved these changes Dec 30, 2021
@liuchengxu liuchengxu merged commit dbfcd24 into main Dec 31, 2021
@liuchengxu liuchengxu deleted the executor-gossip branch December 31, 2021 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants