feat(eth): ethereum sync finality support - #2184
Conversation
…ity and consistency refactor(execution_client.go): rename heads channel to headersCh for better naming refactor(execution_client.go): update variable names for improved readability refactor(execution_client.go): update log messages for consistency and clarity
…n_client_test.go, multi_client.go, multi_client_test.go, options.go): remove follow distance configuration option as it is no longer needed [skip ci]
…ated logic to simplify the codebase and improve maintainability
…plify code refactor(defaults.go): remove reconnection interval constants as they are no longer used refactor(execution_client.go): remove reconnection interval fields to streamline struct refactor(execution_client_test.go): remove reconnection interval option from test refactor(multi_client.go): remove reconnection interval options to enhance clarity refactor(multi_client_test.go): remove reconnection interval checks from test refactor(options.go): remove reconnection interval options as they are no longer used
…ity and consistency
…tion logs to verify logs are received only after blocks are finalized and reorgs before finalization do not affect the final result
… clarity and consistency feat(common_test.go): add MineAndFinalize method to ensure finality by mining blocks feat(eth_e2e_test.go): replace CloseFollowDistance calls with MineAndFinalize for block finality
…e 32 for
better readability and maintainability
refactor(execution_client): update usage of finality distance constant to use
DefaultFinalityDistance constant for consistency and clarity
test(execution_client_test): update finalize function to use DefaultFinalityDistance
constant instead of hardcoded value for improved maintainability [skip ci]
There was a problem hiding this comment.
Pull Request Overview
This PR implements Ethereum sync finality support by replacing the previous follow-distance mechanism with finalized block retrieval while also removing reconnection-related options. Key changes include:
- Removal of the follow distance and reconnection options from the execution client and multi-client APIs.
- Replacement of block retrieval logic with finalized block support throughout the execution client.
- Update of test helpers and CLI commands to align with the new finality approach.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| eth/executionclient/options.go | Removed follow distance and reconnection options. |
| eth/executionclient/multi_client_test.go | Removed tests for follow distance and reconnection intervals. |
| eth/executionclient/multi_client.go | Removed follow distance and reconnection options from the multi-client. |
| eth/executionclient/execution_client.go | Reworked block retrieval and log streaming to use finalized blocks. |
| eth/executionclient/defaults.go | Replaced follow distance defaults with a finality distance constant. |
| eth/eventhandler/event_handler_test.go | Updated client construction to remove obsolete follow distance option. |
| eth/ethtest/eth_e2e_test.go | Updated tests to use MineAndFinalize instead of CloseFollowDistance. |
| eth/ethtest/common_test.go | Renamed follow distance helpers to finality blocks helpers. |
| cli/operator/node.go | Removed follow distance and reconnection options in node startup. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…adability and use it to calculate DefaultFinalityDistance for better maintainability
test(IsFinalityActive): add test cases to cover different scenarios for finality check based on the current epoch and finality fork epoch to determine if finality is active Cover cases where finality is disabled, active, and inactive Ensure the function returns the expected boolean value for each scenario
…ity fork epoch in NewMulti function feat(options.go): add WithFollowDistance and WithFinalityForkEpoch options to set follow distance and finality fork epoch respectively
…Epoch(1) option to client creation for improved testing accuracy
|
@kchojn now that #2307 was merged, I think we need to address #2307 (comment) here as well |
thanks @iurii-ssv |
# Conflicts: # eth/executionclient/execution_client.go # networkconfig/hoodi-stage.go # networkconfig/hoodi.go # networkconfig/mainnet.go # networkconfig/sepolia.go # networkconfig/ssv.go # networkconfig/ssv_test.go
|
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 30 days if there are no updates. Please comment if you would like to keep it open. |
|
Closing this pull request as it has been inactive for 30 days after being marked stale. You are welcome to reopen it if you wish to continue. |
Greptile Summary
PR Description Notes:
Important Files Changed
Confidence score: 3/5
Sequence DiagramsequenceDiagram
participant User
participant Node as SSV Node
participant ExecutionClient as Execution Client
participant EventSyncer as Event Syncer
participant EventHandler as Event Handler
participant BeaconClient as Beacon Client
User->>Node: "start-node"
Note over Node: Load configuration and setup
Node->>ExecutionClient: "New()"
Node->>BeaconClient: "New()"
Node->>EventHandler: "New()"
Node->>EventSyncer: "New()"
Note over ExecutionClient,EventSyncer: Determine sync approach
ExecutionClient->>ExecutionClient: "IsFinalizedFork()"
alt Finality Fork Active
ExecutionClient->>ExecutionClient: "getFinalizedBlock()"
else Pre-Fork
ExecutionClient->>ExecutionClient: "Use follow distance"
end
Node->>EventSyncer: "SyncHistory()"
EventSyncer->>ExecutionClient: "FetchHistoricalLogs()"
ExecutionClient->>EventHandler: "HandleBlockEventsStream()"
EventHandler-->>EventSyncer: "lastProcessedBlock"
Node->>EventSyncer: "SyncOngoing()"
EventSyncer->>ExecutionClient: "StreamLogs()"
loop Ongoing Event Processing
ExecutionClient->>ExecutionClient: "HeaderByNumber()"
alt Finality Fork Active
ExecutionClient->>ExecutionClient: "getFinalizedBlock()"
Note over ExecutionClient: Process up to finalized block
else Pre-Fork
Note over ExecutionClient: Process with follow distance
end
ExecutionClient->>EventHandler: "HandleBlockEventsStream()"
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (1)
-
eth/executionclient/execution_client.go, line 405-457 (link)logic: The healthy() method duplicates the health invalidation check from the Healthy() method above (lines 411-414). This creates redundant logic that could be inconsistent if one is updated without the other.
30 files reviewed, 8 comments
Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format
|
|
||
| func TestSSVForks_ComputeActiveFork(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| forks := SSVForks{ | ||
| {Name: "Fork1", Epoch: 100}, | ||
| {Name: "Fork2", Epoch: 200}, | ||
| } | ||
|
|
||
| testCases := []struct { | ||
| name string | ||
| epoch phase0.Epoch | ||
| expectedFork string | ||
| expectNilFork bool | ||
| }{ | ||
| { | ||
| name: "before first fork", | ||
| epoch: 50, | ||
| expectNilFork: true, | ||
| }, | ||
| { | ||
| name: "at first fork", | ||
| epoch: 100, | ||
| expectedFork: "Fork1", | ||
| }, | ||
| { | ||
| name: "between first and second fork", | ||
| epoch: 150, | ||
| expectedFork: "Fork1", | ||
| }, | ||
| { | ||
| name: "at second fork", | ||
| epoch: 200, | ||
| expectedFork: "Fork2", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range testCases { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| result := forks.ComputeActiveFork(tt.epoch) | ||
| if tt.expectNilFork { | ||
| require.Nil(t, result) | ||
| } else { | ||
| require.NotNil(t, result) | ||
| require.Equal(t, tt.expectedFork, result.Name) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
style: TestSSVForks_ComputeActiveFork duplicates TestSSVForks_ActiveFork test cases exactly - consider removing or differentiating the test scenarios if these methods have different behaviors. Should ComputeActiveFork and ActiveFork methods have different test scenarios to validate their distinct behaviors?
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: networkconfig/ssv_forks_test.go
Line: 94:144
Comment:
**style:** TestSSVForks_ComputeActiveFork duplicates TestSSVForks_ActiveFork test cases exactly - consider removing or differentiating the test scenarios if these methods have different behaviors. Should ComputeActiveFork and ActiveFork methods have different test scenarios to validate their distinct behaviors?
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| Name: "Finality Consensus", | ||
| Epoch: 100, // TODO: MaxEpoch |
There was a problem hiding this comment.
style: Consider using math.MaxUint64 for not-yet-defined epochs to follow Ethereum conventions
| Name: "Finality Consensus", | |
| Epoch: 100, // TODO: MaxEpoch | |
| Name: "Finality Consensus", | |
| Epoch: math.MaxUint64, // TODO: MaxEpoch |
Context Used: Rule from dashboard - Use math.MaxUint64 for not-yet-defined epochs to follow Ethereum conventions, rather than custom v... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: networkconfig/hoodi.go
Line: 34:35
Comment:
**style:** Consider using `math.MaxUint64` for not-yet-defined epochs to follow Ethereum conventions
```suggestion
Name: "Finality Consensus",
Epoch: math.MaxUint64, // TODO: MaxEpoch
```
**Context Used:** Rule from `dashboard` - Use `math.MaxUint64` for not-yet-defined epochs to follow Ethereum conventions, rather than custom v... ([source](https://app.greptile.com/review/custom-context?memory=c39ddeef-318a-487d-bc00-7bcaedbc8e56))
How can I resolve this? If you propose a fix, please make it concise.| }, | ||
| { | ||
| Name: "Finality Consensus", | ||
| Epoch: MaxEpoch, |
There was a problem hiding this comment.
style: Finality Consensus fork is set to MaxEpoch without TODO comment - should this also have a TODO indicating it needs proper epoch configuration? Is the Finality Consensus fork intentionally set to MaxEpoch indefinitely, or should it have a TODO comment like the Gas Limit 36M fork?
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: networkconfig/mainnet.go
Line: 44:44
Comment:
**style:** Finality Consensus fork is set to MaxEpoch without TODO comment - should this also have a TODO indicating it needs proper epoch configuration? Is the Finality Consensus fork intentionally set to MaxEpoch indefinitely, or should it have a TODO comment like the Gas Limit 36M fork?
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| t.Run("ValidatorExited incorrect event public key", func(t *testing.T) { | ||
| pk := validatorData1.masterPubKey.Serialize() | ||
| // Corrupt the public key | ||
| pk[len(pk)-1] ^= 1 | ||
|
|
||
| _, err = boundContract.ExitValidator( | ||
| auth, | ||
| pk, | ||
| []uint64{1, 2, 3, 4}, | ||
| ) | ||
| require.NoError(t, err) | ||
|
|
||
| sim.Commit() | ||
| for i := 0; i < 64; i++ { | ||
| sim.Commit() | ||
| } | ||
|
|
||
| block := getBlockWithLogs(logs) | ||
| require.NotEmpty(t, block.Logs) | ||
| require.Equal(t, ethcommon.HexToHash("0xb4b20ffb2eb1f020be3df600b2287914f50c07003526d3a9d89a9dd12351828c"), block.Logs[0].Topics[0]) | ||
|
|
||
| eventsCh := make(chan executionclient.BlockLogs) | ||
| go func() { | ||
| defer close(eventsCh) | ||
| eventsCh <- block | ||
| }() | ||
| lastProcessedBlock, err := eh.HandleBlockEventsStream(ctx, eventsCh, false) | ||
|
|
||
| require.Equal(t, block.BlockNumber, lastProcessedBlock) | ||
| require.NoError(t, err) | ||
| }) |
There was a problem hiding this comment.
style: duplicate test case - this test is identical to the earlier test at lines 602-632
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: eth/eventhandler/event_handler_test.go
Line: 715:745
Comment:
**style:** duplicate test case - this test is identical to the earlier test at lines 602-632
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| // ActiveFork returns the active fork at the given epoch. | ||
| func (f SSVForks) ActiveFork(epoch phase0.Epoch) *SSVFork { | ||
| for i := len(f) - 1; i >= 0; i-- { | ||
| if f[i].Epoch <= epoch { | ||
| return f[i] | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // ComputeActiveFork returns the active fork for the given epoch | ||
| func (f SSVForks) ComputeActiveFork(epoch phase0.Epoch) *SSVFork { | ||
| // Search from the end to find the most recent active fork | ||
| for i := len(f) - 1; i >= 0; i-- { | ||
| if epoch >= f[i].Epoch { | ||
| return f[i] | ||
| } | ||
| } | ||
| // If no active fork is found, return nil | ||
| return nil |
There was a problem hiding this comment.
style: ActiveFork and ComputeActiveFork implement nearly identical logic with only a subtle difference in comparison operators (≤ vs ≥). Consider removing one method or documenting the semantic difference between them. Is there a semantic difference between ActiveFork and ComputeActiveFork that justifies having both methods?
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: networkconfig/ssv_forks.go
Line: 48:67
Comment:
**style:** ActiveFork and ComputeActiveFork implement nearly identical logic with only a subtle difference in comparison operators (≤ vs ≥). Consider removing one method or documenting the semantic difference between them. Is there a semantic difference between ActiveFork and ComputeActiveFork that justifies having both methods?
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| } else { | ||
| e.followDistance = executionclient.DefaultFollowDistance | ||
| e.networkConfig.SSVConfig.Forks.Forks[1].Epoch = phase0.Epoch(1000000) | ||
|
|
There was a problem hiding this comment.
style: Extra empty line should be removed for consistency with code formatting.
| e.networkConfig.SSVConfig.Forks.Forks[1].Epoch = phase0.Epoch(1000000) |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: eth/ethtest/common_test.go
Line: 111:111
Comment:
**style:** Extra empty line should be removed for consistency with code formatting.
```suggestion
e.networkConfig.SSVConfig.Forks.Forks[1].Epoch = phase0.Epoch(1000000)
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| testNetwork := networkconfig.TestNetwork | ||
| testNetwork.GenesisTime = time.Now().Add(-1 * time.Minute) // Recent genesis | ||
| testNetwork.SSVConfig.Forks.Forks[1].Epoch = 10000 // High epoch to ensure pre-fork |
There was a problem hiding this comment.
style: Direct mutation of testNetwork could cause issues if TestNetwork is a shared instance. Consider using a copy constructor or deep copy to avoid side effects.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: eth/executionclient/execution_client_test.go
Line: 225:227
Comment:
**style:** Direct mutation of testNetwork could cause issues if TestNetwork is a shared instance. Consider using a copy constructor or deep copy to avoid side effects.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
|
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 30 days if there are no updates. Please comment if you would like to keep it open. |
|
superseded by #2695 |
TODO: