feat: Emit IntentDisruptedEvent on batch failure to misbehaving clients#1049
feat: Emit IntentDisruptedEvent on batch failure to misbehaving clients#1049Dunsin-cyber wants to merge 1 commit into
Conversation
WalkthroughThis PR introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/core/application/service.go (1)
3160-3169:⚠️ Potential issue | 🟡 MinorCache failure silently disables IntentDisrupted routing.
If
GetSelectedIntentsfails,registeredIntentswill be nil andbanForfeitCollectionTimeoutwill execute without intent context, which means the disrupting participant cannot be identified by their VTXO outpoint topics and won't receiveIntentDisruptedEvent(the very signal this PR introduces). The round still fails correctly, but the user-facing benefit of the feature is silently lost on this code path.Consider either:
- Failing the round more loudly here (alert / error log instead of warn) so the operator notices, or
- Refactoring so the ban goroutine retrieves intents itself and surfaces the lookup failure consistently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/application/service.go` around lines 3160 - 3169, Currently we pass possibly-nil registeredIntents into banForfeitCollectionTimeout which silently loses IntentDisrupted routing on GetSelectedIntents failure; change the flow so banForfeitCollectionTimeout fetches intents itself (call s.cache.Intents().GetSelectedIntents inside banForfeitCollectionTimeout using the same ctx and roundId), handle and log any error there at error level (log.WithError(err).Error(...)) and exit the goroutine if the lookup fails, and remove the external dependency on registeredIntents from the call site (i.e., stop passing registeredIntents from this block); ensure any logic that emits IntentDisruptedEvent only runs when the fetched intents are non-nil/valid.
🧹 Nitpick comments (3)
pkg/client-lib/client/client.go (1)
112-116: Consider documentingIdsemantics (round id).
Idhere is the round id (per the proto definitionstring id = 1; // round id), but the struct also hasIntentId, which can confuse callers. A short doc comment (or renaming to e.g.RoundId) would make the intent unambiguous. Renaming would be a breaking change to the public client API, so a doc comment is safer.📝 Suggested doc comment
-type IntentDisruptedEvent struct { - Id string - IntentId string - Reason string -} +type IntentDisruptedEvent struct { + // Id is the round id that the disrupted intent was associated with. + Id string + IntentId string + Reason string +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/client-lib/client/client.go` around lines 112 - 116, The field naming in IntentDisruptedEvent is ambiguous: add a short doc comment above the IntentDisruptedEvent type explaining that Id is the round id (per proto: "string id = 1; // round id") and that IntentId is the intent identifier so callers aren’t confused; reference the IntentDisruptedEvent type and its fields (Id, IntentId) in the comment rather than renaming the field to avoid breaking the public API.pkg/client-lib/batch_session_handler.go (1)
183-189: Handler's nil return is silently replaced by a synthetic error.If
OnIntentDisruptedreturnsnil, this code overwrites it withfmt.Errorf("intent disrupted: %s", e.Reason)soJoinBatchSessionalways exits with a non-nil error. This is inconsistent withOnBatchFailedjust above (Lines 177-182), which honors anilreturn and lets the loop continue.If the design is "always exit on IntentDisrupted, regardless of handler outcome," that's fine — but the handler contract becomes confusing because the return value is effectively ignored when
nil. Two cleaner options:Option A — exit with handler's error verbatim (let handler decide)
case client.IntentDisruptedEvent: e := event.(client.IntentDisruptedEvent) - err := eventsHandler.OnIntentDisrupted(ctx, e) - if err == nil { - err = fmt.Errorf("intent disrupted: %s", e.Reason) - } - return "", "", -1, nil, nil, err + return "", "", -1, nil, nil, eventsHandler.OnIntentDisrupted(ctx, e)Option B — keep current semantics but document on the interface
Add a comment on
BatchEventsHandler.OnIntentDisrupted(Line 58) noting that the session will always terminate after this callback and that anilreturn will be wrapped into a generic error. This sets implementer expectations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/client-lib/batch_session_handler.go` around lines 183 - 189, The code currently overwrites a nil return from eventsHandler.OnIntentDisrupted by creating a synthetic error, causing JoinBatchSession to always exit with an error; change the logic in the IntentDisruptedEvent case (handling client.IntentDisruptedEvent and calling eventsHandler.OnIntentDisrupted) to return the handler's error verbatim (i.e., remove the fmt.Errorf wrapping and the conditional that replaces nil with a new error) so a nil from OnIntentDisrupted is honored, or alternatively if you intend to always exit document this behavior on the BatchEventsHandler.OnIntentDisrupted interface (but do not silently replace the handler's return value in JoinBatchSession).internal/core/application/ban.go (1)
96-110: Per-intent dedup looks correct; note ordering inconsistency vs forfeit path.Deduplicating notifications per
intent.IdvianotifiedIntentsis correct — multiple cosigner mappings cannot retrigger the event for the same intent within a single call.Minor inconsistency: here the event is emitted before
Convictions().Add(...)(line 115), while inbanForfeitCollectionTimeoutthe conviction is persisted first (line 200) and the event is emitted afterward. Not necessarily a defect — the event is informational for the offending participant — but if downstream consumers ever correlateIntentDisruptedwith stored convictions, the ordering will produce different observable states between the two paths. Consider aligning the order (persist conviction first, then emit) for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/application/ban.go` around lines 96 - 110, The IntentDisrupted event is emitted before the conviction is persisted here, causing an ordering mismatch with banForfeitCollectionTimeout which calls Convictions().Add(...) before emitting; change the sequence in this block so you call Convictions().Add(...) (using the same conviction creation logic) and persist the conviction first, then compute topics via intentTopics and send the IntentDisrupted event on s.eventsCh, keeping the per-intent dedup via notifiedIntents unchanged (refer to notifiedIntents, IntentDisrupted, Convictions().Add, intentTopics, eventsCh and the banForfeitCollectionTimeout flow to mirror the ordering).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/test/e2e/e2e_test.go`:
- Around line 5002-5070: Bob's handlers for JoinBatchSession should mirror
Alice's so Bob doesn't exit early: add an onBatchFailed handler (no-op that
returns nil to keep waiting) into the customBatchEventsHandler passed to
JoinBatchSession for bobStream (the same place where onIntentDisrupted sets
bobDisruptedEvent), stop swallowing bobErr (remove `_ = bobErr`) and add
assertions similar to Alice's (e.g., require.Error/require.NotNil on
bobErr/bobDisruptedEvent as appropriate) so any leaked IntentDisrupted to Bob is
detected.
- Around line 4991-5060: Bob's customBatchEventsHandler must explicitly set
onBatchFailed to return nil so the default behavior (which returns an error and
ends the session) doesn't terminate Bob's JoinBatchSession before
IntentDisrupted is handled; update the bob-side handler (the
customBatchEventsHandler passed into arksdk.JoinBatchSession with bobStream and
referenced as bobErr) to include onBatchFailed: func(ctx context.Context, event
client.BatchFailedEvent) error { return nil } just like Alice's handler.
---
Outside diff comments:
In `@internal/core/application/service.go`:
- Around line 3160-3169: Currently we pass possibly-nil registeredIntents into
banForfeitCollectionTimeout which silently loses IntentDisrupted routing on
GetSelectedIntents failure; change the flow so banForfeitCollectionTimeout
fetches intents itself (call s.cache.Intents().GetSelectedIntents inside
banForfeitCollectionTimeout using the same ctx and roundId), handle and log any
error there at error level (log.WithError(err).Error(...)) and exit the
goroutine if the lookup fails, and remove the external dependency on
registeredIntents from the call site (i.e., stop passing registeredIntents from
this block); ensure any logic that emits IntentDisruptedEvent only runs when the
fetched intents are non-nil/valid.
---
Nitpick comments:
In `@internal/core/application/ban.go`:
- Around line 96-110: The IntentDisrupted event is emitted before the conviction
is persisted here, causing an ordering mismatch with banForfeitCollectionTimeout
which calls Convictions().Add(...) before emitting; change the sequence in this
block so you call Convictions().Add(...) (using the same conviction creation
logic) and persist the conviction first, then compute topics via intentTopics
and send the IntentDisrupted event on s.eventsCh, keeping the per-intent dedup
via notifiedIntents unchanged (refer to notifiedIntents, IntentDisrupted,
Convictions().Add, intentTopics, eventsCh and the banForfeitCollectionTimeout
flow to mirror the ordering).
In `@pkg/client-lib/batch_session_handler.go`:
- Around line 183-189: The code currently overwrites a nil return from
eventsHandler.OnIntentDisrupted by creating a synthetic error, causing
JoinBatchSession to always exit with an error; change the logic in the
IntentDisruptedEvent case (handling client.IntentDisruptedEvent and calling
eventsHandler.OnIntentDisrupted) to return the handler's error verbatim (i.e.,
remove the fmt.Errorf wrapping and the conditional that replaces nil with a new
error) so a nil from OnIntentDisrupted is honored, or alternatively if you
intend to always exit document this behavior on the
BatchEventsHandler.OnIntentDisrupted interface (but do not silently replace the
handler's return value in JoinBatchSession).
In `@pkg/client-lib/client/client.go`:
- Around line 112-116: The field naming in IntentDisruptedEvent is ambiguous:
add a short doc comment above the IntentDisruptedEvent type explaining that Id
is the round id (per proto: "string id = 1; // round id") and that IntentId is
the intent identifier so callers aren’t confused; reference the
IntentDisruptedEvent type and its fields (Id, IntentId) in the comment rather
than renaming the field to avoid breaking the public API.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 556a0bd4-47a6-44ff-a65e-bc43d062ef51
⛔ Files ignored due to path filters (3)
api-spec/protobuf/gen/ark/v1/indexer.pb.rgw.gois excluded by!**/gen/**api-spec/protobuf/gen/ark/v1/service.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/ark/v1/types.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (14)
api-spec/openapi/swagger/ark/v1/service.openapi.jsonapi-spec/openapi/swagger/ark/v1/types.openapi.jsonapi-spec/protobuf/ark/v1/service.protoapi-spec/protobuf/ark/v1/types.protointernal/core/application/ban.gointernal/core/application/service.gointernal/core/application/service_event.gointernal/core/domain/events_repo.gointernal/interface/grpc/handlers/arkservice.gointernal/test/e2e/delegate_utils_test.gointernal/test/e2e/e2e_test.gopkg/client-lib/batch_session_handler.gopkg/client-lib/client/client.gopkg/client-lib/client/grpc/types.go
1 participant
fixes #899
input signatures)
Summary by CodeRabbit
New Features
Tests