Conversation
📝 WalkthroughWalkthroughThis change refactors chain block-add and rollback flows to separate event construction from publication. A new internal method, addBlockInternal, performs add-block logic and returns events; AddBlock calls it and publishes immediately if an event bus exists, while AddBlocks collects per-block events and publishes them after commit. Rollback logic was simplified to publish events inline and return errors directly; previously used locked-helper deferred-publication patterns were removed. Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
chain/chain.go (2)
214-243:AddBlockscallsAddBlockwhich re-acquires locks per block — repeated lock/unlock overhead in a tight loop.Each iteration calls
c.AddBlockwhich acquires and releases bothc.mutexandc.manager.mutexper block (up to 50 blocks per batch). PreviouslyaddBlockLockedavoided this overhead. While functionally correct, this introduces unnecessary contention and performance cost for bulk inserts. Consider an internaladdBlockLockedvariant that assumes the caller holds the locks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@chain/chain.go` around lines 214 - 243, AddBlocks currently calls AddBlock for each block which re-acquires c.mutex and c.manager.mutex per block; implement and use a new internal addBlockLocked (or reuse existing addBlockLocked) that assumes both c.mutex and c.manager.mutex are already held so the batch loop can hold the locks once per txn. Modify AddBlocks to lock c.mutex and c.manager.mutex (in the same order used elsewhere) before the batch txn, call addBlockLocked inside the txn for each tmpBlock, and release the locks after the batch completes; ensure AddBlock remains unchanged for single-block callers and keep error handling identical.
271-276: Sentinel errors returned without wrapping context.Lines 271 and 274 return early from the header-scanning loop. Line 274 returns
models.ErrBlockNotFoundwithout any context about what was being looked up. Per coding guidelines, errors should be wrapped with context usingfmt.Errorfwith%w.Suggested fix
if header.SlotNumber() < point.Slot { - return models.ErrBlockNotFound + return fmt.Errorf( + "rollback point not found in headers: %w", + models.ErrBlockNotFound, + ) }As per coding guidelines: "Wrap errors with context using fmt.Errorf with %w verb".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@chain/chain.go` around lines 271 - 276, The loop that scans headers returns the sentinel models.ErrBlockNotFound with no context; replace that bare return with a wrapped error using fmt.Errorf and %w so callers get useful context. Concretely, in the header-scanning logic (referencing header.SlotNumber() and point.Slot), change the return models.ErrBlockNotFound to something like fmt.Errorf("block not found for slot %d (point=%v): %w", point.Slot, point, models.ErrBlockNotFound) so the error includes which slot/point was being looked up and preserves the sentinel via %w.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@chain/chain.go`:
- Around line 198-211: AddBlocks currently calls AddBlock which synchronously
publishes ChainBlockEvent via c.eventBus.Publish while still inside txn.Do (and
holding c.mutex), causing subscribers to see rolled-back data and risking
deadlocks; modify AddBlocks/AddBlock so events are emitted only after a
successful txn.Do commit: either (1) change AddBlock to append events to a local
slice (e.g., []ChainBlockEvent) instead of calling c.eventBus.Publish, have
AddBlocks collect these events for each block and after txn.Do returns nil
iterate and call c.eventBus.Publish for each event, or (2) refactor by adding an
internal addWithoutPublish method that performs DB writes/validation (used by
AddBlocks inside txn.Do) and keep the public AddBlock to call addWithoutPublish
then publish; ensure c.mutex is not held during publish and keep references to
ChainBlockEvent, eventBus.Publish, AddBlocks, AddBlock, txn.Do in your changes.
---
Duplicate comments:
In `@chain/chain.go`:
- Around line 373-401: The rollback/fork event Publish calls (c.eventBus.Publish
with ChainUpdateEventType/ChainForkEventType) are executed while both c.mutex
and c.manager.mutex are held, which can deadlock if Publish is synchronous;
change the code to not hold those locks while dispatching events by capturing
the event payloads (e.g., build ChainRollbackEvent and ChainForkEvent values or
a small events slice) inside the locked section and then perform
c.eventBus.Publish calls after releasing the locks (or dispatch asynchronously),
ensuring you reference and avoid holding c.mutex and c.manager.mutex during the
actual Publish calls.
---
Nitpick comments:
In `@chain/chain.go`:
- Around line 214-243: AddBlocks currently calls AddBlock for each block which
re-acquires c.mutex and c.manager.mutex per block; implement and use a new
internal addBlockLocked (or reuse existing addBlockLocked) that assumes both
c.mutex and c.manager.mutex are already held so the batch loop can hold the
locks once per txn. Modify AddBlocks to lock c.mutex and c.manager.mutex (in the
same order used elsewhere) before the batch txn, call addBlockLocked inside the
txn for each tmpBlock, and release the locks after the batch completes; ensure
AddBlock remains unchanged for single-block callers and keep error handling
identical.
- Around line 271-276: The loop that scans headers returns the sentinel
models.ErrBlockNotFound with no context; replace that bare return with a wrapped
error using fmt.Errorf and %w so callers get useful context. Concretely, in the
header-scanning logic (referencing header.SlotNumber() and point.Slot), change
the return models.ErrBlockNotFound to something like fmt.Errorf("block not found
for slot %d (point=%v): %w", point.Slot, point, models.ErrBlockNotFound) so the
error includes which slot/point was being looked up and preserves the sentinel
via %w.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="chain/chain.go">
<violation number="1" location="chain/chain.go:231">
P1: Events are published before the transaction commits. `AddBlocks` calls `AddBlock` inside a `txn.Do` callback, but `AddBlock` now publishes events immediately. If the transaction fails after some blocks succeed, subscribers will have already been notified about blocks that were never persisted. The previous code deferred event publication until after the transaction committed successfully.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
wolf31o2
force-pushed
the
refactor/chain-events
branch
from
f0b892e to
e98cf2e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
chain/chain.go (2)
228-271: Batched event publication after commit — correctly addresses prior review concern.The
pendingEvents[:0]reset on retry (line 249) and post-commit publication loop (lines 262-267) are both correct. This ensures subscribers never observe rolled-back data.One minor guideline note: the error at line 260 is returned without wrapping context.
Suggested wrap
if err != nil { - return err + return fmt.Errorf("add blocks batch: %w", err) }As per coding guidelines, "Wrap errors with context using fmt.Errorf with %w verb".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@chain/chain.go` around lines 228 - 271, In Chain.AddBlocks, wrap the error returned from txn.Do with contextual information before returning (e.g., include batchOffset/batchSize and operation context) using fmt.Errorf and %w so callers get wrapped error traces; update the return in the if err != nil branch after txn.Do to return a formatted error and add fmt to imports if needed.
299-303: Unwrapped sentinel errors in header-rollback path.Lines 299–303 return
nilormodels.ErrBlockNotFoundwithout rollback-context wrapping. Similarly, line 336 returnsErrRollbackExceedsSecurityParambare. Wrapping with%wpreserveserrors.Is()semantics while adding call-site context for debugging.Example wraps
- return models.ErrBlockNotFound + return fmt.Errorf("rollback point not found in headers: %w", models.ErrBlockNotFound)- return ErrRollbackExceedsSecurityParam + return fmt.Errorf( + "rollback depth %d exceeds K=%d: %w", + forkDepth, securityParam, ErrRollbackExceedsSecurityParam, + )As per coding guidelines, "Wrap errors with context using fmt.Errorf with %w verb".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@chain/chain.go` around lines 299 - 303, In the header-rollback path, don't return sentinel errors bare—wrap them with context using fmt.Errorf("%s: %w", ...) so callers retain errors.Is() behavior; specifically, replace returns of models.ErrBlockNotFound (when header.SlotNumber() < point.Slot) with a wrapped error that includes the slot/point details (e.g., "header rollback: header slot X < point slot Y") and likewise wrap ErrRollbackExceedsSecurityParam with context (e.g., "rollback exceeds security param at slot X: %w"); use header.SlotNumber(), point.Slot, models.ErrBlockNotFound and ErrRollbackExceedsSecurityParam as anchors when constructing the fmt.Errorf wraps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@chain/chain.go`:
- Around line 228-271: In Chain.AddBlocks, wrap the error returned from txn.Do
with contextual information before returning (e.g., include
batchOffset/batchSize and operation context) using fmt.Errorf and %w so callers
get wrapped error traces; update the return in the if err != nil branch after
txn.Do to return a formatted error and add fmt to imports if needed.
- Around line 299-303: In the header-rollback path, don't return sentinel errors
bare—wrap them with context using fmt.Errorf("%s: %w", ...) so callers retain
errors.Is() behavior; specifically, replace returns of models.ErrBlockNotFound
(when header.SlotNumber() < point.Slot) with a wrapped error that includes the
slot/point details (e.g., "header rollback: header slot X < point slot Y") and
likewise wrap ErrRollbackExceedsSecurityParam with context (e.g., "rollback
exceeds security param at slot X: %w"); use header.SlotNumber(), point.Slot,
models.ErrBlockNotFound and ErrRollbackExceedsSecurityParam as anchors when
constructing the fmt.Errorf wraps.
|
@cubic-dev-ai review |
@wolf31o2 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="chain/chain.go">
<violation number="1" location="chain/chain.go:404">
P1: Rollback now publishes events while holding Chain and Manager locks, which can deadlock if subscribers call back into chain/manager state. Defer event publication until after locks are released (as in the previous pattern).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
wolf31o2
force-pushed
the
refactor/chain-events
branch
from
e98cf2e to
eaf8053
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
chain/chain.go (1)
262-267: Inconsistent event type source betweenAddBlocksandRollback.Line 265 hardcodes
ChainUpdateEventType, whileRollback(line 285) usesevt.Type. Both are correct today sinceaddBlockInternalalways producesChainUpdateEventType, but usingevt.Typeis more resilient to future changes (e.g., ifaddBlockInternalever returns different event types).Suggested diff
for _, evt := range pendingEvents { - c.eventBus.Publish(ChainUpdateEventType, evt) + c.eventBus.Publish(evt.Type, evt) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@chain/chain.go` around lines 262 - 267, The commit path currently hardcodes ChainUpdateEventType when publishing pendingEvents (in the AddBlocks/transaction commit block), while the Rollback path uses each event's own Type; change the publish call to use evt.Type instead of ChainUpdateEventType so eventBus.Publish(ChainUpdateEventType, evt) becomes eventBus.Publish(evt.Type, evt), referencing the pendingEvents slice and c.eventBus.Publish to keep behavior consistent and resilient if addBlockInternal returns other event types in the future.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@chain/chain.go`:
- Around line 262-267: The commit path currently hardcodes ChainUpdateEventType
when publishing pendingEvents (in the AddBlocks/transaction commit block), while
the Rollback path uses each event's own Type; change the publish call to use
evt.Type instead of ChainUpdateEventType so
eventBus.Publish(ChainUpdateEventType, evt) becomes eventBus.Publish(evt.Type,
evt), referencing the pendingEvents slice and c.eventBus.Publish to keep
behavior consistent and resilient if addBlockInternal returns other event types
in the future.
|
@cubic-dev-ai review |
@wolf31o2 I have started the AI code review. It will take a few minutes to complete. |
1 participant
Summary by cubic
Refactored chain event flow: AddBlock publishes immediately after locks release, AddBlocks defers publishing until the batch transaction commits, and Rollback publishes its events after locks release. Simplifies the code and standardizes event handling.
Written for commit eaf8053. Summary will update on new commits.
Summary by CodeRabbit