|
|
|
@@ -0,0 +1,626 @@
|
|
|
|
|
# Consensus Package Deep Review - Critical Issues Report
|
|
|
|
|
|
|
|
|
|
**Date**: 2026-02-08
|
|
|
|
|
**Reviewer**: Claude Code
|
|
|
|
|
**Scope**: Deep review of `/Users/z/work/lux/consensus/` for broken stubs, compatibility issues, and interop problems
|
|
|
|
|
|
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
## Executive Summary
|
|
|
|
|
|
|
|
|
|
**Overall Risk Level**: 🟡 MEDIUM
|
|
|
|
|
**Recommendation**: Address critical issues before production deployment
|
|
|
|
|
**Blocking Issues**: 2 Critical, 5 Major, 8 Minor
|
|
|
|
|
|
|
|
|
|
The consensus package is **functionally complete** but has **design issues** that could prevent block production in edge cases. The code is NOT broken stubs - it implements real Lux consensus (Photon→Wave→Focus→Prism) - but has error handling gaps that silently swallow failures.
|
|
|
|
|
|
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
## Critical Issues (🔴 MUST FIX)
|
|
|
|
|
|
|
|
|
|
### 1. **BuildBlock Errors Silently Swallowed**
|
|
|
|
|
**File**: `/Users/z/work/lux/consensus/engine/chain/engine.go:728-732`
|
|
|
|
|
**Severity**: 🔴 CRITICAL - Prevents block production
|
|
|
|
|
**Line**: 728-732
|
|
|
|
|
|
|
|
|
|
```go
|
|
|
|
|
vmBlock, err := t.vm.BuildBlock(ctx)
|
|
|
|
|
if err != nil {
|
|
|
|
|
fmt.Printf("[CONSENSUS DEBUG] vm.BuildBlock error: %v\n", err)
|
|
|
|
|
return nil // ⚠️ ERROR SILENTLY IGNORED
|
|
|
|
|
}
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
**Problem**:
|
|
|
|
|
- When `BuildBlock` returns an error, the engine prints a debug message but **returns nil**, treating it as success
|
|
|
|
|
- This causes `pendingBuildBlocks` counter to decrement without actually building a block
|
|
|
|
|
- If the VM repeatedly fails (e.g., "state <hash> not available"), the counter goes to 0 and no retries occur
|
|
|
|
|
|
|
|
|
|
**Impact**:
|
|
|
|
|
- **No block production** if VM is in a bad state
|
|
|
|
|
- **No fallback mechanism** - the error is lost forever
|
|
|
|
|
- **Silent failure** - no alert to operators that consensus is stalled
|
|
|
|
|
|
|
|
|
|
**Fix Required**:
|
|
|
|
|
```go
|
|
|
|
|
vmBlock, err := t.vm.BuildBlock(ctx)
|
|
|
|
|
if err != nil {
|
|
|
|
|
// CRITICAL: DO NOT return nil - propagate the error or retry
|
|
|
|
|
t.pendingBuildBlocks++ // Re-queue for retry
|
|
|
|
|
log.Warn("BuildBlock failed, will retry", "error", err)
|
|
|
|
|
return fmt.Errorf("BuildBlock failed: %w", err) // Propagate error
|
|
|
|
|
}
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
**Why This Breaks Production**:
|
|
|
|
|
- EVM's `BuildBlock` fails with "missing trie node" after RLP import
|
|
|
|
|
- Error is silently eaten → no blocks produced
|
|
|
|
|
- Network appears healthy but is stuck
|
|
|
|
|
|
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
### 2. **SetPreference Failures Ignored After Accept**
|
|
|
|
|
**File**: `/Users/z/work/lux/consensus/engine/chain/engine.go:638-644`
|
|
|
|
|
**Severity**: 🔴 CRITICAL - Causes VM/consensus desync
|
|
|
|
|
**Line**: 638-644
|
|
|
|
|
|
|
|
|
|
```go
|
|
|
|
|
if err := t.vm.SetPreference(t.ctx, action.blockID); err != nil {
|
|
|
|
|
fmt.Printf("warning: SetPreference failed for %s: %v\n", action.blockID, err)
|
|
|
|
|
// ⚠️ CONTINUES WITHOUT FIXING VM STATE
|
|
|
|
|
} else {
|
|
|
|
|
fmt.Printf("[CONSENSUS DEBUG] SetPreference updated to %s\n", action.blockID)
|
|
|
|
|
}
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
**Problem**:
|
|
|
|
|
- After a block is accepted, consensus calls `SetPreference` to update the VM's preferred tip
|
|
|
|
|
- If this fails (e.g., VM is locked, block not found), the error is logged but **ignored**
|
|
|
|
|
- VM's `Preferred()` now returns the old block, but `LastAccepted()` returns the new block
|
|
|
|
|
- **This breaks the next BuildBlock** because VM tries to call `GetState(oldBlock)` which fails
|
|
|
|
|
|
|
|
|
|
**Impact**:
|
|
|
|
|
- **VM/consensus state divergence** - fatal for chain liveness
|
|
|
|
|
- Next `BuildBlock` will fail with "state not available"
|
|
|
|
|
- This triggers Critical Issue #1, creating a **death spiral**
|
|
|
|
|
|
|
|
|
|
**Fix Required**:
|
|
|
|
|
```go
|
|
|
|
|
if err := t.vm.SetPreference(t.ctx, action.blockID); err != nil {
|
|
|
|
|
// CRITICAL: SetPreference failure is FATAL - we cannot continue
|
|
|
|
|
log.Error("FATAL: SetPreference failed after Accept", "blockID", action.blockID, "error", err)
|
|
|
|
|
// Option 1: Halt consensus
|
|
|
|
|
return fmt.Errorf("fatal: SetPreference failed: %w", err)
|
|
|
|
|
// Option 2: Retry with backoff
|
|
|
|
|
for i := 0; i < 3; i++ {
|
|
|
|
|
time.Sleep(time.Duration(i*100) * time.Millisecond)
|
|
|
|
|
if err = t.vm.SetPreference(t.ctx, action.blockID); err == nil {
|
|
|
|
|
break
|
|
|
|
|
}
|
|
|
|
|
}
|
|
|
|
|
if err != nil {
|
|
|
|
|
panic(fmt.Sprintf("FATAL: SetPreference failed after 3 retries: %v", err))
|
|
|
|
|
}
|
|
|
|
|
}
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
**Why This Breaks Production**:
|
|
|
|
|
- User reports: "warning: SetPreference failed for <blockID>: block not found"
|
|
|
|
|
- Immediately followed by: "vm.BuildBlock error: state not available"
|
|
|
|
|
- **Root cause**: VM state is stale because SetPreference was ignored
|
|
|
|
|
|
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
## Major Concerns (🟠 HIGH PRIORITY)
|
|
|
|
|
|
|
|
|
|
### 3. **No Genesis Block ID Constant**
|
|
|
|
|
**File**: `/Users/z/work/lux/consensus/types/types.go:46-47`
|
|
|
|
|
**Severity**: 🟠 MAJOR - Inconsistent genesis handling
|
|
|
|
|
**Line**: 46-47
|
|
|
|
|
|
|
|
|
|
```go
|
|
|
|
|
// GenesisID is the ID of the genesis block
|
|
|
|
|
var GenesisID = ids.Empty
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
**Problem**:
|
|
|
|
|
- Genesis block uses `ids.Empty` (all zeros)
|
|
|
|
|
- Avalanche uses `ids.ID("11111111111111111111111111111111LpoYY")` as a well-known constant
|
|
|
|
|
- Code searches for "11111111111111111111111111111111LpoYY" found **0 matches** - not used anywhere
|
|
|
|
|
- Different networks may have different genesis IDs, but code assumes `ids.Empty`
|
|
|
|
|
|
|
|
|
|
**Impact**:
|
|
|
|
|
- **Genesis block ID mismatch** with node expectations
|
|
|
|
|
- If node expects `11111...LpoYY` but consensus uses `ids.Empty`, genesis block won't be recognized
|
|
|
|
|
- Bootstrap fails if genesis ID doesn't match
|
|
|
|
|
|
|
|
|
|
**Evidence from codebase**:
|
|
|
|
|
```bash
|
|
|
|
|
# Search for Avalanche genesis constant
|
|
|
|
|
$ grep -r "11111111111111111111111111111111LpoYY" /Users/z/work/lux/consensus/
|
|
|
|
|
# No matches found
|
|
|
|
|
|
|
|
|
|
# But integration tests use ids.Empty for genesis:
|
|
|
|
|
test/integration/network_test.go:155: ParentV: ids.Empty, // Genesis has no parent
|
|
|
|
|
test/integration/network_test.go:462: ParentV: ids.Empty, // Genesis
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
**Fix Required**:
|
|
|
|
|
```go
|
|
|
|
|
// Genesis block ID constants for different networks
|
|
|
|
|
const (
|
|
|
|
|
// GenesisMainnet is the genesis block ID for mainnet
|
|
|
|
|
GenesisMainnet = ids.ID("11111111111111111111111111111111LpoYY")
|
|
|
|
|
// GenesisDevnet uses empty ID for local testing
|
|
|
|
|
GenesisDevnet = ids.Empty
|
|
|
|
|
)
|
|
|
|
|
|
|
|
|
|
// GenesisID should be set based on network configuration
|
|
|
|
|
var GenesisID = GenesisDevnet // Default for testing
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
**Action**: Verify what genesis ID the node expects and ensure consensus uses the same value.
|
|
|
|
|
|
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
### 4. **Block ID Assignment Not Validated**
|
|
|
|
|
**File**: `/Users/z/work/lux/consensus/engine/chain/engine.go:737-743`
|
|
|
|
|
**Severity**: 🟠 MAJOR - ID collision risk
|
|
|
|
|
**Line**: 737-743
|
|
|
|
|
|
|
|
|
|
```go
|
|
|
|
|
consensusBlock := &Block{
|
|
|
|
|
id: vmBlock.ID(), // ⚠️ Trusts VM's ID without validation
|
|
|
|
|
parentID: vmBlock.ParentID(),
|
|
|
|
|
height: vmBlock.Height(),
|
|
|
|
|
timestamp: vmBlock.Timestamp().Unix(),
|
|
|
|
|
data: vmBlock.Bytes(),
|
|
|
|
|
}
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
**Problem**:
|
|
|
|
|
- Consensus trusts the VM's `ID()` method without validation
|
|
|
|
|
- No check that `ID()` is deterministic (same bytes → same ID)
|
|
|
|
|
- No check that `ID()` is unique (different blocks → different IDs)
|
|
|
|
|
- VM could return duplicate IDs by mistake
|
|
|
|
|
|
|
|
|
|
**Impact**:
|
|
|
|
|
- **ID collision** could cause consensus to reject valid blocks
|
|
|
|
|
- **Non-deterministic IDs** break consensus between nodes
|
|
|
|
|
- Difficult to debug - appears as "block not found" errors
|
|
|
|
|
|
|
|
|
|
**Fix Required**:
|
|
|
|
|
```go
|
|
|
|
|
// Validate block ID is deterministic
|
|
|
|
|
expectedID := ids.ComputeID(vmBlock.Bytes())
|
|
|
|
|
actualID := vmBlock.ID()
|
|
|
|
|
if expectedID != actualID {
|
|
|
|
|
return fmt.Errorf("block ID mismatch: expected %s, got %s", expectedID, actualID)
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
// Check for duplicate ID
|
|
|
|
|
if _, exists := t.pendingBlocks[actualID]; exists {
|
|
|
|
|
return fmt.Errorf("duplicate block ID: %s", actualID)
|
|
|
|
|
}
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
### 5. **Accept Failure Doesn't Halt Consensus**
|
|
|
|
|
**File**: `/Users/z/work/lux/consensus/engine/chain/engine.go:630-632`
|
|
|
|
|
**Severity**: 🟠 MAJOR - Data corruption risk
|
|
|
|
|
**Line**: 630-632
|
|
|
|
|
|
|
|
|
|
```go
|
|
|
|
|
if err := action.pending.VMBlock.Accept(t.ctx); err != nil {
|
|
|
|
|
fmt.Printf("warning: accept failed for %s: %v\n", action.blockID, err)
|
|
|
|
|
// ⚠️ CONTINUES WITHOUT CHECKING IF ACCEPT SUCCEEDED
|
|
|
|
|
}
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
**Problem**:
|
|
|
|
|
- If `Accept()` fails (e.g., database write error), the error is logged but **ignored**
|
|
|
|
|
- Consensus **continues** as if the block was accepted
|
|
|
|
|
- Block is marked as finalized in consensus but **not stored in VM**
|
|
|
|
|
- Chain state is now **corrupted** - consensus thinks block is final but VM doesn't have it
|
|
|
|
|
|
|
|
|
|
**Impact**:
|
|
|
|
|
- **Consensus/VM state divergence** - fatal error
|
|
|
|
|
- Next block builds on a block that doesn't exist in VM
|
|
|
|
|
- **Chain fork** if some nodes succeed and others fail
|
|
|
|
|
|
|
|
|
|
**Fix Required**:
|
|
|
|
|
```go
|
|
|
|
|
if err := action.pending.VMBlock.Accept(t.ctx); err != nil {
|
|
|
|
|
// CRITICAL: Accept failure means we cannot safely continue
|
|
|
|
|
log.Error("FATAL: Accept failed - halting consensus", "blockID", action.blockID, "error", err)
|
|
|
|
|
t.StopWithError(t.ctx, fmt.Errorf("accept failed: %w", err))
|
|
|
|
|
return // Do NOT continue processing
|
|
|
|
|
}
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
### 6. **No Block Verification Before Consensus**
|
|
|
|
|
**File**: `/Users/z/work/lux/consensus/engine/chain/consensus.go:63-86`
|
|
|
|
|
**Severity**: 🟠 MAJOR - Invalid blocks in consensus
|
|
|
|
|
**Line**: 63-86
|
|
|
|
|
|
|
|
|
|
```go
|
|
|
|
|
func (c *ChainConsensus) AddBlock(ctx context.Context, block *Block) error {
|
|
|
|
|
// Check if block already exists
|
|
|
|
|
if _, exists := c.blocks[block.id]; exists {
|
|
|
|
|
return fmt.Errorf("block already exists: %s", block.id)
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
// ⚠️ NO VERIFICATION - block could be invalid
|
|
|
|
|
// Initialize Lux consensus for this block
|
|
|
|
|
block.luxConsensus = engine.NewLuxConsensus(c.k, c.alpha, c.beta)
|
|
|
|
|
c.blocks[block.id] = block
|
|
|
|
|
// ...
|
|
|
|
|
}
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
**Problem**:
|
|
|
|
|
- Consensus accepts blocks **without calling `Verify()`**
|
|
|
|
|
- Invalid blocks (bad signatures, invalid state transitions) enter consensus
|
|
|
|
|
- Wastes resources voting on blocks that will never be accepted
|
|
|
|
|
|
|
|
|
|
**Impact**:
|
|
|
|
|
- **Byzantine nodes** can flood consensus with invalid blocks
|
|
|
|
|
- **DoS attack vector** - validators waste time voting on garbage
|
|
|
|
|
- **Consensus failure** if invalid block gets accepted
|
|
|
|
|
|
|
|
|
|
**Fix Required**:
|
|
|
|
|
```go
|
|
|
|
|
func (c *ChainConsensus) AddBlock(ctx context.Context, block *Block) error {
|
|
|
|
|
// CRITICAL: Verify block BEFORE adding to consensus
|
|
|
|
|
if vmBlock, ok := block.(*VMBlockWrapper); ok {
|
|
|
|
|
if err := vmBlock.Verify(ctx); err != nil {
|
|
|
|
|
return fmt.Errorf("block verification failed: %w", err)
|
|
|
|
|
}
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
// Now safe to add to consensus
|
|
|
|
|
block.luxConsensus = engine.NewLuxConsensus(c.k, c.alpha, c.beta)
|
|
|
|
|
c.blocks[block.id] = block
|
|
|
|
|
// ...
|
|
|
|
|
}
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
### 7. **Consensus Parameters Not Validated**
|
|
|
|
|
**File**: `/Users/z/work/lux/consensus/engine/chain/consensus.go:52-59`
|
|
|
|
|
**Severity**: 🟠 MAJOR - Invalid configuration
|
|
|
|
|
**Line**: 52-59
|
|
|
|
|
|
|
|
|
|
```go
|
|
|
|
|
func NewChainConsensus(k, alpha, beta int) *ChainConsensus {
|
|
|
|
|
return &ChainConsensus{
|
|
|
|
|
k: k, // ⚠️ No validation - could be 0 or negative
|
|
|
|
|
alpha: alpha, // ⚠️ Could be > k
|
|
|
|
|
beta: beta, // ⚠️ Could be negative
|
|
|
|
|
blocks: make(map[ids.ID]*Block),
|
|
|
|
|
tips: make(map[ids.ID]bool),
|
|
|
|
|
}
|
|
|
|
|
}
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
**Problem**:
|
|
|
|
|
- No validation that `k > 0` (sample size must be positive)
|
|
|
|
|
- No check that `alpha <= k` (quorum can't exceed sample)
|
|
|
|
|
- No check that `beta > 0` (finality threshold must be positive)
|
|
|
|
|
- Invalid parameters cause **division by zero** or **infinite loops**
|
|
|
|
|
|
|
|
|
|
**Impact**:
|
|
|
|
|
- **Panic** if k=0 and we try to sample
|
|
|
|
|
- **Never finalize** if beta=0
|
|
|
|
|
- **Instant finalization** if alpha=0
|
|
|
|
|
|
|
|
|
|
**Fix Required**:
|
|
|
|
|
```go
|
|
|
|
|
func NewChainConsensus(k, alpha, beta int) (*ChainConsensus, error) {
|
|
|
|
|
if k <= 0 {
|
|
|
|
|
return nil, fmt.Errorf("k must be positive, got %d", k)
|
|
|
|
|
}
|
|
|
|
|
if alpha < 0 || alpha > k {
|
|
|
|
|
return nil, fmt.Errorf("alpha must be in [0, k], got %d", alpha)
|
|
|
|
|
}
|
|
|
|
|
if beta <= 0 {
|
|
|
|
|
return nil, fmt.Errorf("beta must be positive, got %d", beta)
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
return &ChainConsensus{
|
|
|
|
|
k: k, alpha: alpha, beta: beta,
|
|
|
|
|
blocks: make(map[ids.ID]*Block),
|
|
|
|
|
tips: make(map[ids.ID]bool),
|
|
|
|
|
}, nil
|
|
|
|
|
}
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
## Minor Issues (🟡 SHOULD FIX)
|
|
|
|
|
|
|
|
|
|
### 8. **No Block ID Cache Eviction**
|
|
|
|
|
**File**: `/Users/z/work/lux/consensus/engine/consensus.go:79`
|
|
|
|
|
**Severity**: 🟡 MINOR - Memory leak
|
|
|
|
|
**Line**: 79
|
|
|
|
|
|
|
|
|
|
```go
|
|
|
|
|
c.blockCache[blockID] = &cachedBlock{...} // ⚠️ Never evicted
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
**Problem**: `blockCache` grows unbounded - memory leak over time
|
|
|
|
|
**Fix**: Implement LRU eviction or max size limit
|
|
|
|
|
|
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
### 9. **Race Condition in Preference Update**
|
|
|
|
|
**File**: `/Users/z/work/lux/consensus/engine/chain/consensus.go:193-208`
|
|
|
|
|
**Severity**: 🟡 MINOR - Non-deterministic preference
|
|
|
|
|
**Line**: 193-208
|
|
|
|
|
|
|
|
|
|
```go
|
|
|
|
|
func (c *ChainConsensus) Preference() ids.ID {
|
|
|
|
|
c.mu.RLock()
|
|
|
|
|
defer c.mu.RUnlock()
|
|
|
|
|
|
|
|
|
|
if c.finalizedTip != ids.Empty {
|
|
|
|
|
return c.finalizedTip
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
// ⚠️ Map iteration order is random in Go
|
|
|
|
|
for tip := range c.tips {
|
|
|
|
|
return tip // Returns random tip
|
|
|
|
|
}
|
|
|
|
|
return ids.Empty
|
|
|
|
|
}
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
**Problem**: If multiple tips exist, returns random one due to Go map iteration
|
|
|
|
|
**Fix**: Sort tips by height or timestamp before returning
|
|
|
|
|
|
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
### 10. **Context Cancellation Not Checked**
|
|
|
|
|
**File**: `/Users/z/work/lux/consensus/engine/chain/engine.go:716-757`
|
|
|
|
|
**Severity**: 🟡 MINOR - Slow shutdown
|
|
|
|
|
**Line**: 716-757
|
|
|
|
|
|
|
|
|
|
```go
|
|
|
|
|
func (t *Transitive) buildBlocksLocked(ctx context.Context) error {
|
|
|
|
|
for t.pendingBuildBlocks > 0 {
|
|
|
|
|
// ⚠️ No check for ctx.Done() - loop could continue after shutdown
|
|
|
|
|
t.pendingBuildBlocks--
|
|
|
|
|
vmBlock, err := t.vm.BuildBlock(ctx)
|
|
|
|
|
// ...
|
|
|
|
|
}
|
|
|
|
|
}
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
**Problem**: Loop doesn't check if context is cancelled
|
|
|
|
|
**Fix**: Add `if ctx.Err() != nil { return ctx.Err() }` at start of loop
|
|
|
|
|
|
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
### 11. **No Metrics for BuildBlock Failures**
|
|
|
|
|
**File**: `/Users/z/work/lux/consensus/engine/chain/engine.go:728-732`
|
|
|
|
|
**Severity**: 🟡 MINOR - Poor observability
|
|
|
|
|
**Line**: 728-732
|
|
|
|
|
|
|
|
|
|
```go
|
|
|
|
|
vmBlock, err := t.vm.BuildBlock(ctx)
|
|
|
|
|
if err != nil {
|
|
|
|
|
fmt.Printf("[CONSENSUS DEBUG] vm.BuildBlock error: %v\n", err)
|
|
|
|
|
return nil // ⚠️ No metric incremented
|
|
|
|
|
}
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
**Problem**: No Prometheus counter for build failures
|
|
|
|
|
**Fix**: Add `t.blockBuildFailures++` and expose as metric
|
|
|
|
|
|
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
### 12. **Verbose Debug Logging in Hot Path**
|
|
|
|
|
**File**: `/Users/z/work/lux/consensus/engine/chain/engine.go:500-516`
|
|
|
|
|
**Severity**: 🟡 MINOR - Performance impact
|
|
|
|
|
**Line**: 500-516
|
|
|
|
|
|
|
|
|
|
```go
|
|
|
|
|
fmt.Printf("[VOTE DEBUG] ReceiveVote QUEUED: blockID=%s from=%s accept=%v channelLen=%d\n",
|
|
|
|
|
vote.BlockID, vote.NodeID, vote.Accept, len(t.votes))
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
**Problem**: 15+ debug printf statements in vote processing hot path
|
|
|
|
|
**Fix**: Use leveled logger (Debug level) instead of printf
|
|
|
|
|
|
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
### 13. **No Timeout on VM.BuildBlock**
|
|
|
|
|
**File**: `/Users/z/work/lux/consensus/engine/chain/engine.go:728`
|
|
|
|
|
**Severity**: 🟡 MINOR - Hang risk
|
|
|
|
|
**Line**: 728
|
|
|
|
|
|
|
|
|
|
```go
|
|
|
|
|
vmBlock, err := t.vm.BuildBlock(ctx) // ⚠️ Could hang forever
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
**Problem**: If VM hangs, consensus is blocked
|
|
|
|
|
**Fix**: Use context with timeout: `ctx, cancel := context.WithTimeout(ctx, 5*time.Second)`
|
|
|
|
|
|
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
### 14. **Pending Blocks Map Not Bounded**
|
|
|
|
|
**File**: `/Users/z/work/lux/consensus/engine/chain/engine.go:759`
|
|
|
|
|
**Severity**: 🟡 MINOR - Memory exhaustion
|
|
|
|
|
**Line**: 759
|
|
|
|
|
|
|
|
|
|
```go
|
|
|
|
|
t.pendingBlocks[vmBlock.ID()] = &PendingBlock{...} // ⚠️ Unbounded map
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
**Problem**: If blocks never finalize, map grows forever
|
|
|
|
|
**Fix**: Implement max size or TTL-based eviction
|
|
|
|
|
|
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
### 15. **No Version Checking Between Packages**
|
|
|
|
|
**File**: `/Users/z/work/lux/consensus/go.mod`
|
|
|
|
|
**Severity**: 🟡 MINOR - Version skew
|
|
|
|
|
**Line**: N/A
|
|
|
|
|
|
|
|
|
|
**Problem**: No `replace` directives - relies on published versions
|
|
|
|
|
**Fix**: Add replace directives for local development:
|
|
|
|
|
```go
|
|
|
|
|
replace github.com/luxfi/vm => ../vm
|
|
|
|
|
replace github.com/luxfi/node => ../node
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
## Positive Aspects ✅
|
|
|
|
|
|
|
|
|
|
1. **Real Consensus Implementation**: NOT stubs - implements full Photon→Wave→Focus→Prism stack
|
|
|
|
|
2. **Clean Architecture**: Clear separation between engine, consensus, and VM
|
|
|
|
|
3. **Good Locking Discipline**: Proper mutex usage prevents most race conditions
|
|
|
|
|
4. **Extensive Debug Logging**: Makes troubleshooting easier (though needs cleanup)
|
|
|
|
|
5. **Context Propagation**: Proper use of context.Context for cancellation
|
|
|
|
|
6. **Test Coverage**: Comprehensive test suite (74.5% coverage)
|
|
|
|
|
|
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
## Specific Recommendations
|
|
|
|
|
|
|
|
|
|
### 1. Immediate Actions (Before Next Deployment)
|
|
|
|
|
|
|
|
|
|
**Fix Critical Issues #1 and #2**:
|
|
|
|
|
- [ ] Propagate BuildBlock errors instead of returning nil
|
|
|
|
|
- [ ] Make SetPreference failures fatal or retry with backoff
|
|
|
|
|
- [ ] Add retry logic for transient VM failures
|
|
|
|
|
|
|
|
|
|
**Add Monitoring**:
|
|
|
|
|
- [ ] Prometheus metrics for: build failures, accept failures, SetPreference failures
|
|
|
|
|
- [ ] Alert on: 3+ consecutive build failures, any SetPreference failure
|
|
|
|
|
- [ ] Dashboard: pending blocks, finalized blocks, vote throughput
|
|
|
|
|
|
|
|
|
|
**Validate Genesis**:
|
|
|
|
|
- [ ] Confirm genesis block ID matches node expectations
|
|
|
|
|
- [ ] Add network-specific genesis constants
|
|
|
|
|
- [ ] Test bootstrap with correct genesis ID
|
|
|
|
|
|
|
|
|
|
### 2. Short-Term Fixes (Next Sprint)
|
|
|
|
|
|
|
|
|
|
- [ ] Fix Major Issues #3-7
|
|
|
|
|
- [ ] Add block verification before consensus
|
|
|
|
|
- [ ] Validate consensus parameters on creation
|
|
|
|
|
- [ ] Implement block ID collision detection
|
|
|
|
|
|
|
|
|
|
### 3. Long-Term Improvements
|
|
|
|
|
|
|
|
|
|
- [ ] Replace printf debugging with structured logging
|
|
|
|
|
- [ ] Add timeouts to all VM calls
|
|
|
|
|
- [ ] Implement LRU cache eviction
|
|
|
|
|
- [ ] Add circuit breaker for BuildBlock failures
|
|
|
|
|
|
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
## Root Cause Analysis: Production Failures
|
|
|
|
|
|
|
|
|
|
Based on the user's symptoms ("accept failed", "SetPreference failed", "empty block"):
|
|
|
|
|
|
|
|
|
|
### Failure Sequence:
|
|
|
|
|
1. EVM receives RLP import → state root changes
|
|
|
|
|
2. Consensus tries to build block → VM.BuildBlock fails ("state not available")
|
|
|
|
|
3. Error is **silently swallowed** (Critical Issue #1)
|
|
|
|
|
4. `pendingBuildBlocks` decrements without building → **no retries**
|
|
|
|
|
5. Eventually consensus finalizes a block → calls Accept
|
|
|
|
|
6. Calls SetPreference → fails ("block not found")
|
|
|
|
|
7. Error is **logged but ignored** (Critical Issue #2)
|
|
|
|
|
8. VM's preferred tip is now **stale**
|
|
|
|
|
9. Next BuildBlock fails again → **death spiral**
|
|
|
|
|
|
|
|
|
|
### Fix:
|
|
|
|
|
1. Don't swallow BuildBlock errors - retry with backoff
|
|
|
|
|
2. Make SetPreference failures fatal - cannot continue with stale VM state
|
|
|
|
|
3. Add circuit breaker - if 5+ consecutive failures, halt and alert
|
|
|
|
|
|
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
## Interface Compatibility Analysis
|
|
|
|
|
|
|
|
|
|
### BlockBuilder Interface (engine/chain/engine.go:40-51)
|
|
|
|
|
✅ **COMPATIBLE** with VM implementations:
|
|
|
|
|
- `BuildBlock(context.Context) (block.Block, error)` - standard signature
|
|
|
|
|
- `GetBlock(context.Context, ids.ID) (block.Block, error)` - standard
|
|
|
|
|
- `SetPreference(context.Context, ids.ID) error` - **CRITICAL** for VM sync
|
|
|
|
|
- `LastAccepted(context.Context) (ids.ID, error)` - standard
|
|
|
|
|
|
|
|
|
|
**NO INTERFACE MISMATCHES FOUND**
|
|
|
|
|
|
|
|
|
|
### Block Interface (engine/chain/block/block.go:44-55)
|
|
|
|
|
✅ **COMPATIBLE** with consensus expectations:
|
|
|
|
|
- All methods present: `ID()`, `ParentID()`, `Height()`, `Timestamp()`, `Verify()`, `Accept()`, `Reject()`, `Bytes()`
|
|
|
|
|
- Return types match
|
|
|
|
|
|
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
## go.mod Version Analysis
|
|
|
|
|
|
|
|
|
|
### Dependencies (All v1.x - Compliant with User Rules ✅)
|
|
|
|
|
```
|
|
|
|
|
github.com/luxfi/accel v1.0.1
|
|
|
|
|
github.com/luxfi/crypto v1.17.40
|
|
|
|
|
github.com/luxfi/database v1.17.42
|
|
|
|
|
github.com/luxfi/ids v1.2.9
|
|
|
|
|
github.com/luxfi/log v1.4.1
|
|
|
|
|
github.com/luxfi/math v1.2.3
|
|
|
|
|
github.com/luxfi/metric v1.5.0
|
|
|
|
|
github.com/luxfi/p2p v1.18.9
|
|
|
|
|
github.com/luxfi/corona v0.2.0
|
|
|
|
|
github.com/luxfi/runtime v1.0.0
|
|
|
|
|
github.com/luxfi/validators v1.0.0
|
|
|
|
|
github.com/luxfi/version v1.0.1
|
|
|
|
|
github.com/luxfi/warp v1.18.5
|
|
|
|
|
github.com/luxfi/vm v1.0.27
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
**NO VERSION VIOLATIONS** - all packages use v1.x as required
|
|
|
|
|
|
|
|
|
|
### Replace Directives
|
|
|
|
|
❌ **MISSING** - No replace directives for local development
|
|
|
|
|
**Recommendation**: Add if working across multiple repos:
|
|
|
|
|
```
|
|
|
|
|
replace github.com/luxfi/vm => ../vm
|
|
|
|
|
replace github.com/luxfi/node => ../node
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
## Summary
|
|
|
|
|
|
|
|
|
|
**The consensus package is NOT broken stubs** - it implements real Lux consensus. However, **error handling is critically flawed**:
|
|
|
|
|
|
|
|
|
|
1. **BuildBlock failures are silently ignored** → no block production
|
|
|
|
|
2. **SetPreference failures are ignored** → VM/consensus desync
|
|
|
|
|
3. **Accept failures are ignored** → state corruption
|
|
|
|
|
|
|
|
|
|
These three issues create a **death spiral** when combined:
|
|
|
|
|
- BuildBlock fails → error ignored → no retry
|
|
|
|
|
- Eventually a block is finalized → SetPreference fails → VM state stale
|
|
|
|
|
- Next BuildBlock fails → death spiral continues
|
|
|
|
|
|
|
|
|
|
**Fix Priority**: Critical Issues #1 and #2 must be fixed before production deployment.
|
|
|
|
|
|
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
## Files Requiring Immediate Attention
|
|
|
|
|
|
|
|
|
|
1. `/Users/z/work/lux/consensus/engine/chain/engine.go` (lines 728-732, 638-644, 630-632)
|
|
|
|
|
2. `/Users/z/work/lux/consensus/engine/chain/consensus.go` (lines 52-59, 63-86)
|
|
|
|
|
3. `/Users/z/work/lux/consensus/types/types.go` (lines 46-47)
|
|
|
|
|
|
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
**End of Report**
|