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

feat(exex, node): check ExEx head against the node head #10539

Closed
wants to merge 4 commits into from

Conversation

shekhirin
Copy link
Collaborator

@shekhirin shekhirin commented Aug 26, 2024

Ref #10489

@shekhirin shekhirin added C-enhancement New feature or request A-exex Execution Extensions labels Aug 26, 2024
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

I think they're two ways to go about this
a) like this pr does, return context on launch

or a different install process that not only receives the ExEx context but also a way to communicate this to the manager, imo this would be the more elegant approach.
for example, during install, you create a subscription by providing the ExExHead as an argument, so that

/// Channel used to send [`ExExEvent`]s to the rest of the node.
///
/// # Important
///
/// The exex should emit a `FinishedHeight` whenever a processed block is safe to prune.
/// Additionally, the exex can pre-emptively emit a `FinishedHeight` event to specify what
/// blocks to receive notifications for.
pub events: UnboundedSender<ExExEvent>,
/// Channel to receive [`ExExNotification`]s.
///
/// # Important
///
/// Once a an [`ExExNotification`] is sent over the channel, it is considered delivered by the
/// node.
pub notifications: Receiver<ExExNotification>,

is no longer created beforehand but the subscriptions will be created specific to the ExExHead

@@ -437,7 +437,7 @@ where
pub fn install_exex<F, R, E>(self, exex_id: impl Into<String>, exex: F) -> Self
where
F: FnOnce(ExExContext<NodeAdapter<T, CB::Components>>) -> R + Send + 'static,
R: Future<Output = eyre::Result<E>> + Send,
R: Future<Output = eyre::Result<(Option<BlockHash>, E)>> + Send,
Copy link
Collaborator

Choose a reason for hiding this comment

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

why does this take just the block hash?

Comment on lines +56 to +60
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub struct ExExHead {
pub number: BlockNumber,
pub hash: BlockHash,
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

this needs proper docs,

do we think we will eventually require additional info from the exex?
if so we could perhaps change this to

struct ExExInfo {
   head: Option<ExExHead>
}

@shekhirin
Copy link
Collaborator Author

Going with the second approach in another PR

@shekhirin shekhirin closed this Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-exex Execution Extensions C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants