Conversation
Codecov Report❌ Patch coverage is ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Greptile SummaryThis PR implements finality-based event syncing for the execution client, transitioning from a follow-distance approach to using the execution layer's finalized blocks after a configurable fork epoch. Key Changes:
Implementation Details:
The implementation is clean, well-tested, and follows project conventions. Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
Start([Event Sync Request]) --> GetHead[Get current head block]
GetHead --> CalcEpoch[Calculate current epoch from block timestamp]
CalcEpoch --> CheckFork{Current epoch >= FinalityConsensus?}
CheckFork -->|No - Pre-fork| CheckDistance{Current block >= follow distance?}
CheckDistance -->|No| NothingToSync[Return ErrNothingToSync]
CheckDistance -->|Yes| CalcPreFork[toBlock = currentBlock - followDistance]
CheckFork -->|Yes - Post-fork| GetFinalized[Get finalized block via rpc.FinalizedBlockNumber]
GetFinalized --> CalcPostFork[toBlock = finalized block number]
CalcPreFork --> CheckRange{toBlock >= fromBlock?}
CalcPostFork --> CheckRange
CheckRange -->|No| NothingToSync
CheckRange -->|Yes| FetchLogs[Fetch logs in batches from fromBlock to toBlock]
FetchLogs --> Return([Return logs channel])
NothingToSync --> End([End])
Return --> End
style CheckFork fill:#e1f5ff
style GetFinalized fill:#d4edda
style CalcPreFork fill:#fff3cd
style FetchLogs fill:#f8d7da
Last reviewed commit: cb2e0dc |
|
As mentioned in the SIP comments, we are still debating whether this feature will need a fork or not. Also, we are discussing adding a "special treatment" for some events like exits/removals to allow a nice UX. So I think this pends some discussion with spec (CC: @GalRogozinski @MatheusFranco99 ). |
Yes, I wanted the first version to be ported from the original PR as it is, just with minor changes. I tested it on stage, and it worked fine. I have also been thinking about the fork, and I don't see the reason why we'd need it. So, I want to remove the fork code, but I decided to keep it until the discussion on it has been finalized. I'm currently working on the special treatment of |
GalRogozinski
left a comment
There was a problem hiding this comment.
There could be issues where views differ of registered validators. So some operators may send messages that will be rejected by others. This can cause temporary scoring issues...
I think ValidatorExits may have issues, but it is not so important since this can be activated via EL
|
Overall I like moving event sync from 8 EL blocks to CL finality, but we need to be sure that this has no impact on duty loss, or signing anything on behalf of exited validators. Also, I wanted to clarify something about the fork transition.
|
Supersedes #2184
SIP: ssvlabs/SIPs#67