Skip to content

fix: code review — dead code, security, logic flaws#103

Merged
wcatz merged 1 commit intomasterfrom
fix/code-review-cleanup
Mar 13, 2026
Merged

fix: code review — dead code, security, logic flaws#103
wcatz merged 1 commit intomasterfrom
fix/code-review-cleanup

Conversation

@wcatz
Copy link
Copy Markdown
Owner

@wcatz wcatz commented Mar 13, 2026

Summary

  • Remove dead image/ticker fields and fix misplaced doc comments
  • Security: WebSocket clients are now receive-only (no message relay); fix RWMutex lock upgrade race in handleMessages
  • Logic: handleRollBackward now deletes orphaned blocks and recomputes epoch nonce; fix context cancel leak in cmdStake; deduplicate cmdLeaderlog into shared computeAndReplyLeaderlog helper

Test plan

  • go build clean
  • go vet clean
  • All 69 tests pass

Summary by CodeRabbit

  • New Features

    • Improved chain rollback handling with automatic block cleanup and VRF state recomputation to ensure consistency.
  • Improvements

    • Increased leaderlog computation timeout from 5 to 15 minutes for longer-running operations.
    • Enhanced WebSocket client management with better error handling and automatic cleanup of disconnected clients.

- Remove dead `image` and `ticker` fields from Indexer struct
- Fix WebSocket message relay: clients are now receive-only
- Fix RWMutex lock upgrade race in handleMessages
- Fix handleRollBackward: delete orphaned blocks + recompute nonce
- Fix cmdStake context cancel leak (defer koiosCancel)
- Deduplicate cmdLeaderlog into computeAndReplyLeaderlog helper
- Fix misplaced truncHash/formatNumber doc comments
- Add DeleteBlocksAfterSlot to Store interface + both implementations
@wcatz wcatz merged commit 1d25155 into master Mar 13, 2026
3 checks passed
@wcatz wcatz deleted the fix/code-review-cleanup branch March 13, 2026 13:26
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 13, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5b5ef47e-2fb0-446c-a9a4-cea9ba1865f8

📥 Commits

Reviewing files that changed from the base of the PR and between c1d90c0 and 7316243.

📒 Files selected for processing (4)
  • commands.go
  • db.go
  • main.go
  • store.go

📝 Walkthrough

Walkthrough

The pull request refactors the leaderlog command handler by extracting computation logic into a helper function, adds block deletion capability to the storage layer, enhances rollback handling with block cleanup and epoch nonce recomputation, and redesigns WebSocket broadcasting as a dedicated receive-only pattern. Additionally, unused fields are removed from the Indexer struct, and operation timeouts are increased.

Changes

Cohort / File(s) Summary
Leaderlog Command Refactoring
commands.go
Extracts leaderlog computation into computeAndReplyLeaderlog helper function, increases operation timeout from 5 to 15 minutes, improves Koios cancellation with proper defer pattern, updates message delivery to target provided chat parameter instead of original message context.
Database API Enhancement
db.go, store.go
Adds new public DeleteBlocksAfterSlot method to Store interface and both PgStore and SqliteStore implementations to delete blocks beyond a given slot with row count reporting.
Rollback and WebSocket Refactoring
main.go
Implements live rollback cleanup via block deletion and epoch nonce recomputation, refactors WebSocket to receive-only client mode with dedicated broadcaster loop, removes unused Indexer struct fields (image, ticker), improves client disconnect handling with batched removal.

Sequence Diagram

sequenceDiagram
    participant WS as WebSocket Handler
    participant BC as Broadcaster Loop
    participant Clients as Connected Clients
    participant Ch as Broadcast Channel

    WS->>WS: Accept client connection
    WS->>Clients: Add to clients set
    WS->>WS: Launch receive loop (discard messages)
    
    Note over BC: Dedicated broadcaster goroutine
    BC->>Ch: Wait for BlockEvent
    Ch-->>BC: BlockEvent received
    
    BC->>Clients: Iterate all clients
    BC->>Clients: Send event to each client
    
    alt Broadcast Success
        Clients-->>BC: Message sent ✓
    else Broadcast Fails
        BC->>BC: Mark client as failed
        BC->>Clients: Remove from set (batched)
    end
    
    BC->>Ch: Wait for next event
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

Poem

🐰 A rabbit's ode to refactored streams:

Blocks are trimmed when rollback dreams,
Broadcasting flows in dedicated streams,
Leaderlog helpers extract with grace,
Timeouts stretched—more time and space! ✨

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/code-review-cleanup
📝 Coding Plan
  • Generate coding plan for human review comments

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant