feat: High availabilty via RAFT #2836
Conversation
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2836 +/- ##
==========================================
- Coverage 64.76% 62.25% -2.51%
==========================================
Files 81 88 +7
Lines 7350 8063 +713
==========================================
+ Hits 4760 5020 +260
- Misses 2049 2467 +418
- Partials 541 576 +35
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
* main: build(deps): Bump the go_modules group across 2 directories with 3 updates (#2846) build(deps): Bump github.com/dvsekhvalnov/jose2go from 1.7.0 to 1.8.0 in /test/e2e (#2851) build(deps): Bump github.com/consensys/gnark-crypto from 0.18.0 to 0.18.1 in /test/e2e (#2844) build(deps): Bump github.com/cometbft/cometbft from 0.38.17 to 0.38.19 in /test/e2e (#2843) build(deps): Bump github.com/dvsekhvalnov/jose2go from 1.6.0 to 1.7.0 in /test/e2e (#2845)
alpe
changed the title
(cherry picked from commit c44cd77e665f6d5d463295c6ed61c59a56d88db3)
|
@claude review again |
|
Claude finished @alpe's task —— View job Code Review: High Availability via RAFTI've completed a comprehensive review of this RAFT-based high availability implementation. This is a significant feature adding distributed consensus for failover support. 1. Code Quality & ArchitectureExcellent Design Choices
Architectural Concernspkg/raft/node.go:211 - JSON serialization bottleneck data, err := json.Marshal(state) // todo:use protobuf
pkg/raft/node.go:274-276 - Insufficient documentation func (n *Node) SetApplyCallback(ch chan<- RaftApplyMsg) {
n.fsm.applyCh = ch
}
node/failover.go - Naming confusion
block/internal/syncing/raft_retriever.go:0% - Zero test coverage
2. Error Handling & ResilienceStrong Points
Critical Issuespkg/raft/election.go:96-104 - Double-signing prevention logic if !raftSynced || !runnable.IsSynced(d.node.GetState()) {
d.logger.Info().Msg("became leader, but not synced. Pass on leadership")
if err := d.node.leadershipTransfer(); err != nil && !errors.Is(err, raft.ErrNotLeader) {
return err // ⚠️ Terminates the election loop entirely
}
continue
}
block/internal/executing/executor.go:452 - No rollback on RAFT broadcast failure if err := e.raftNode.Broadcast(e.ctx, raftState); err != nil {
return fmt.Errorf("failed to propose block to raft: %w", err)
}
if err := batch.Commit(); err != nil { // ⚠️ Committed to local store even if RAFT fails
return fmt.Errorf("failed to commit batch: %w", err)
}
block/internal/syncing/syncer.go:181 - Incomplete catch-up check return s.headerStore.Store().Height() > currentHeight ||
s.dataStore.Store().Height() > currentHeight ||
s.raftRetriever != nil && s.raftRetriever.Height() > currentHeight
3. SecurityConcernsblock/internal/syncing/raft_retriever.go:112-114 - Weak proposer validation if err := assertExpectedProposer(r.genesis, header.ProposerAddress); err != nil {
r.logger.Debug().Err(err).Msg("unexpected proposer")
return nil // ⚠️ Silently ignores blocks from wrong proposer
}
pkg/raft/node.go:285 - No validation in FSM.Apply func (f *FSM) Apply(log *raft.Log) interface{} {
var state RaftBlockState
if err := json.Unmarshal(log.Data, &state); err != nil {
f.logger.Error().Err(err).Msg("unmarshal block state")
return err
}
if err := f.state.Load().assertValid(state); err != nil { // ⚠️ Only basic validation
return err
}
Secrets Management - No issues found
4. Performance & Resource EfficiencyBottleneckspkg/raft/node.go:211 - JSON overhead (repeated from Architecture)
block/internal/syncing/syncer.go:355 - Aggressive polling ticker := time.NewTicker(10 * time.Millisecond)
node/failover.go:187-231 - Shutdown complexity
Resource Leakspkg/raft/node.go:92-95 - No cleanup on error r, err := raft.NewRaft(raftConfig, fsm, logStore, stableStore, snapshotStore, transport)
if err != nil {
return nil, fmt.Errorf("create raft: %w", err)
}
5. Testing & ReliabilityTest Coverage Issues (41.34% patch coverage)Major gaps:
Positive Testing
Missing Test Scenarios
6. Documentation & Developer ExperienceStrengths
GapsConfiguration defaults unclear
Migration path undocumented
Operational runbooks missing
7. Specific Code Review Commentspkg/raft/election.go:95-104 (CRITICAL)Double-signing prevention stops node entirely on transfer failure if err := d.node.leadershipTransfer(); err != nil && !errors.Is(err, raft.ErrNotLeader) {
// the leadership transfer can fail due to no suitable leader. Better stop than double sign on old state
return err // ❌ Terminates node
}Recommendation: Change to: if err := d.node.leadershipTransfer(); err != nil {
d.logger.Warn().Err(err).Msg("leadership transfer failed, will retry on next election")
// Continue participating in elections
}
continueblock/internal/executing/executor.go:434-460 (HIGH)Commit order creates inconsistency risk Current flow:
If step 1 fails but step 2 succeeds, leader has uncommitted block locally. Recommendation:
block/internal/syncing/raft_retriever.go:99-128 (MEDIUM)No test coverage for critical follower path This code consumes blocks from RAFT leader. Zero tests means:
Recommendation: Add unit tests covering: func TestRaftRetrieverConsumeBlock(t *testing.T) {
// Test invalid header
// Test unexpected proposer
// Test event processing failure
// Test context cancellation
}node/full.go:216-220 (LOW)Initialization order unclear if e.raftNode != nil {
// ensure node is fully synced before producing any blocks
if raftState := e.raftNode.GetState(); raftState.Height != 0 && raftState.Height != state.LastBlockHeight {
return fmt.Errorf("invalid state: node is not synced with the chain: raft %d != %d state", raftState.Height, state.LastBlockHeight)
}
}Question: When does this check trigger? On first start raftState.Height is 0, so check passes. On restart after failover, heights should match. When would they differ? Summary & RecommendationsCritical (Must Fix Before Merge)
High Priority (Fix Soon After Merge)
Medium Priority (Technical Debt)
Low Priority (Nice to Have)
Overall AssessmentThis is a solid foundational implementation of RAFT-based HA. The architecture is clean, the integration is thoughtful, and the E2E test validates the core failover scenario. However, there are critical correctness issues around error handling and commit ordering that could cause cluster divergence or node crashes in production. The low test coverage (41%) and zero coverage on follower sync paths is concerning for a consensus system. Recommendation: Address the 4 critical issues before merging. The performance optimization (JSON→protobuf) is essential for the stated 100ms block time goal but could be done in a follow-up PR with benchmarks. Great work overall - this is a complex feature and the implementation shows careful consideration of the distributed systems challenges. With the fixes above, this will be production-ready. |
julienrbrt
left a comment
julienrbrt
left a comment
There was a problem hiding this comment.
first walkthrough, I'll go read about github.com/hashicorp/raft and come back to review election.go and node.go
| return nil | ||
| } | ||
|
|
||
| // Height returns the current height stored |
There was a problem hiding this comment.
Why do we need to know the height of the p2p (go header) store? (I am still reviewing, this may get clearer). We can have the app height from the evolve store.
There was a problem hiding this comment.
When the node switches from sync to aggregator mode, the internal state is key to prevent double signing.
The Syncer has a isCatchingUpState method now that checks the stores for any height > current.
it is called within the leader election loop to transfer leadership in case it is not fully synced, yet.
| } | ||
|
|
||
| // SetApplyCallback sets a callback to be called when log entries are applied | ||
| func (n *Node) SetApplyCallback(ch chan<- RaftApplyMsg) { |
There was a problem hiding this comment.
nit: what is this for? the go doc is very light
There was a problem hiding this comment.
The channel is passed by the syncer to receive first level state updates from within the raft cluster. This should be the fastest communication channel available.
| }() | ||
|
|
||
| // Check raft leadership if raft is enabled | ||
| if e.raftNode != nil && !e.raftNode.IsLeader() { |
There was a problem hiding this comment.
unrelated: i wonder how this will play with different sequencers.
In #2797 you can get to that path without node key (to sign). I suppose we'll need to add a condition for based sequencing.
There was a problem hiding this comment.
yes, I was only preparing for single sequencer. Base would not work with raft as there are no aggregators.
| leaderFactory := func() (raftpkg.Runnable, error) { | ||
| logger.Info().Msg("Starting aggregator-MODE") | ||
| nodeConfig.Node.Aggregator = true | ||
| nodeConfig.P2P.Peers = "" // peers are not supported in aggregator mode |
There was a problem hiding this comment.
not sure I understand this. is the aggregator broadcasting to no one?
There was a problem hiding this comment.
the aggregator is required to broadcast to at least one node part of a larger mesh other wise p2p will not work
There was a problem hiding this comment.
This is more who calls whom. The aggregator gets called not otherwise. Starting all nodes with p2p-peer setup makes sense though. When a ha cluster is setup, the raft leader gets the aggregator role and I clear the peers when the p2p stack is restarted.
There is an error thrown somewhere when peers are not empty.
node/full.go
Outdated
| func initRaftNode(nodeConfig config.Config, logger zerolog.Logger) (*raftpkg.Node, error) { | ||
| raftDir := nodeConfig.Raft.RaftDir | ||
| if raftDir == "" { | ||
| raftDir = filepath.Join(nodeConfig.RootDir, "raft") |
There was a problem hiding this comment.
nit: we should be using DefaultConfig() value if empty.
| bc *block.Components | ||
| } | ||
|
|
||
| func newSyncMode( |
There was a problem hiding this comment.
nit: i was a tiny bit confused this was moved here instead of full.go
There was a problem hiding this comment.
These are the constructors. Naming could be better, I guess.
| } | ||
| return setupFailoverState(nodeConfig, nodeKey, database, genesis, logger, mainKV, rktStore, blockComponentsFn, raftNode) | ||
| } | ||
| func newAggregatorMode( |
| return fmt.Errorf("not leader") | ||
| } | ||
|
|
||
| data, err := json.Marshal(state) // todo:use protobuf |
There was a problem hiding this comment.
we should migrate to protobuf here. json will cause overhead, at 100ms we need to minimise it as much as possible
* main: chore: fix some comments (#2874) chore: bump node in evm-single (#2875) refactor(syncer,cache): use compare and swap loop and add comments (#2873) refactor: use state da height as well (#2872) refactor: retrieve highest da height in cache (#2870) chore: change from event count to start and end height (#2871)
## Overview Speed up cache write/loads via parallel execution. Pulled from #2836
## Overview Minor updates to make it easier to trace errors Extracted from #2836
* main: chore: remove extra github action yml file (#2882) fix(execution/evm): verify payload status (#2863) feat: fetch included da height from store (#2880) chore: better output on errors (#2879) refactor!: create da client and split cache interface (#2878) chore!: rename `evm-single` and `grpc-single` (#2839) build(deps): Bump golang.org/x/crypto from 0.42.0 to 0.45.0 in /tools/da-debug in the go_modules group across 1 directory (#2876) chore: parallel cache de/serialization (#2868) chore: bump blob size (#2877)
Implement failover via RAFT