feat(exex): backfill on subscription with head #10787
Conversation
crates/exex/exex/src/manager.rs
Outdated
| notifications: self.notifications, | ||
| head, | ||
| } | ||
| fn with_head(self, head: ExExHead) -> eyre::Result<ExExNotificationsWithHead<Node>> { |
There was a problem hiding this comment.
If L230 TODO should remove?
shekhirin
force-pushed
the
alexey/exex-notifications-backfill
branch
from
e3b7761
to
c48a07c
Compare
shekhirin
added
C-enhancement
crates/exex/exex/src/manager.rs
Outdated
| node_head: Head, | ||
| components: Node, | ||
| notifications: Receiver<ExExNotification>, | ||
| } |
There was a problem hiding this comment.
this needs a few docs now, especially node_head, because we now have two different types of heads
crates/exex/exex/src/manager.rs
Outdated
| Ok(notifications) | ||
| } | ||
|
|
||
| fn heal(&mut self, node_head: Head) -> eyre::Result<()> { |
There was a problem hiding this comment.
hmm, unsure what heal means in this context.
this synchronizes the streams, right?
There was a problem hiding this comment.
yeah, including handling of backfill / unwind & backfill
There was a problem hiding this comment.
changed to synchronize() and added an extensive doc comment
crates/exex/exex/src/manager.rs
Outdated
| /// Whether to wait for the node head to be at the same height as the ExEx head, and then | ||
| /// call the [`Self::heal`]. | ||
| wait_for_head: bool, |
There was a problem hiding this comment.
hmm, this naming is slightly confusing, because not immediately obvious which head this references.
I assume we can derive the same functionality by using Option<Notifaction> which is the most recent notification we received from the stream and compare the exexhead against this, or maybe as an additional field
There was a problem hiding this comment.
renamed the field
I assume we can derive the same functionality by using
Option<Notifaction>which is the most recent notification we received from the stream and compare the exexhead against this, or maybe as an additional field
we can do that but unsure what's the benefit because we will set this field to Some(notification) inside the Stream::poll_next and immediately use it to start healing process, if needed
crates/exex/exex/src/manager.rs
Outdated
| // If we are waiting for the node head to be at the same height as the ExEx head, | ||
| // then we need to check if the ExEx is on the canonical chain. To do this, we need | ||
| // to get the block at the ExEx head's height from new chain, and compare its hash | ||
| // to the ExEx head's hash. | ||
| if let Some((block, tip)) = chain_and_tip.as_ref().and_then(|(chain, tip)| { | ||
| chain.blocks().get(&this.head.block.number).zip(Some(tip)) | ||
| }) { | ||
| if block.hash() == this.head.block.hash { | ||
| // ExEx is on the canonical chain, proceed with the notification | ||
| this.wait_for_head = false; | ||
| } else { |
There was a problem hiding this comment.
this section is a bit hard to follow, unsure I fully understand this yet
There was a problem hiding this comment.
restructured it a bit and added more comments
crates/exex/exex/src/manager.rs
Outdated
| let tip = this | ||
| .components | ||
| .provider() | ||
| .sealed_header(*tip)? |
There was a problem hiding this comment.
isn't tip part of the Chain? then we don't need to fetch it?
There was a problem hiding this comment.
not necessarily, in case of a revert notification, we take chain.first().number - 1 as a tip
shekhirin
force-pushed
the
alexey/exex-notifications-backfill
branch
from
67e9f66
to
accdfe0
Compare
shekhirin
force-pushed
the
alexey/exex-notifications-backfill
branch
from
a086475
to
3712522
Compare
shekhirin
requested review from
joshieDo,
Rjected,
onbjerg and
gakonst
as code owners
| #[allow(dead_code)] | ||
| components: Node, | ||
| pub struct ExExNotificationsWithHead<P, E> { | ||
| node_head: Head, |
There was a problem hiding this comment.
does this need to be head, or is NumHash sufficient?
There was a problem hiding this comment.
same reasoning as #10787 (comment) imo
Closes #10571