feat(exex): finalize ExEx WAL on new finalized block header #11174
Conversation
shekhirin
force-pushed
the
alexey/exex-manager-finalize-wal
branch
from
aa35526 to
56a643b
Compare
shekhirin
requested review from
mattsse,
Rjected,
onbjerg and
gakonst
as code owners
| fn empty_finalized_header_stream() -> ForkChoiceStream<SealedHeader> { | ||
| let (tx, rx) = watch::channel(None); | ||
| // Do not drop the sender, otherwise the receiver will always return an error | ||
| std::mem::forget(tx); |
There was a problem hiding this comment.
this leaks, but should be fine for testing
crates/exex/exex/src/manager.rs
Outdated
| @@ -71,6 +76,7 @@ impl ExExHandle { | |||
| node_head: Head, | |||
| provider: P, | |||
| executor: E, | |||
| wal: Wal, | |||
There was a problem hiding this comment.
why does every exex get a new wal?
doesn't this duplicate a ton of the same data?
this doesn't make much sense to me
There was a problem hiding this comment.
agreed, the notifications should be the same for all ExExes no matter how fast they are processing them. Moved it to the ExExManager
| } | ||
| // If there is a finalized header, finalize the WAL with it | ||
| if let Some(header) = last_finalized_header { | ||
| self.wal.finalize((header.number, header.hash()).into())?; |
There was a problem hiding this comment.
this should be fine because we're operating on the assumption that exex will never trail the finalized block, right?
There was a problem hiding this comment.
wdym trail? If you're worried about an ExEx being behind the finalized block, then it's fine because we have this data in the database anyway to backfill the ExEx in case it's behind.
| /// Write-Ahead Log for the [`ExExNotification`]s. | ||
| wal: Wal, |
There was a problem hiding this comment.
we should def document why this is even needed.
Finalizes the ExEx WAL when a new finalized header is received.
In addition to that, a small bug in the
Wal::commitwas fixed cb58a9eTested on Holesky:
datadir/exex/walwas created and is currently empty.Wal::commitis called, but since there's no notifications being committed to the WAL yet, it just early returns.