Add conviction on double-spend attempt #760
Conversation
WalkthroughAdds CrimeTypeDoubleSpend, implements banDoubleSpendAttempt to persist single-script DoubleSpend convictions, triggers that ban asynchronously from fraud handling (reactToFraud/service), and adds test helper and calls to pardon convictions in e2e tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Detector as Fraud Detector
participant Service as Application Service
participant Fraud as reactToFraud
participant Ban as banDoubleSpendAttempt
participant Repo as Convictions Repo
participant Log as Logger
Detector->>Service: vtxo flagged as Spent
activate Service
Service->>Fraud: reactToFraud(ctx, vtxo, ...)
note right of Service #f0f9ff: spawn goroutine for banDoubleSpendAttempt
Service-->>Ban: async banDoubleSpendAttempt(context.Background(), vtxo)
deactivate Service
activate Ban
Ban->>Ban: compute vtxo output script
alt script computed
Ban->>Repo: add conviction {type: DoubleSpend, single_script, duration}
alt persisted
Note right of Repo: Conviction stored
else failed
Ban->>Log: warn("failed to add DoubleSpend conviction")
end
else script error
Ban->>Log: warn("script computation error")
end
deactivate Ban
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (1)
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: 0
🧹 Nitpick comments (1)
internal/core/application/ban.go (1)
186-207: LGTM! Consider enriching the reason string for better traceability.The implementation correctly mirrors the existing ban pattern. The empty
RoundIDis appropriate since double-spend detection occurs via the scanner, outside of round execution.Optionally, you could include the vtxo outpoint in the reason for enhanced observability:
convictions = append(convictions, domain.NewScriptConviction( hex.EncodeToString(outputScript), domain.Crime{ Type: domain.CrimeTypeDoubleSpend, - Reason: "double spend attempt", + Reason: fmt.Sprintf("double spend attempt on vtxo %s", vtxo.Outpoint), }, &s.banDuration))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/core/application/ban.go(1 hunks)internal/core/application/service.go(1 hunks)internal/core/domain/conviction.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/core/application/ban.go (2)
internal/core/domain/vtxo.go (2)
Vtxo(38-53)Outpoint(15-18)internal/core/domain/conviction.go (4)
Conviction(50-58)NewScriptConviction(135-140)Crime(38-42)CrimeTypeDoubleSpend(21-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit tests
- GitHub Check: integration tests
- GitHub Check: Build and Scan
🔇 Additional comments (2)
internal/core/application/service.go (1)
2729-2731: LGTM! Double-spend ban integrated correctly.The new ban path follows the existing fraud reaction pattern at line 2732-2738, executing asynchronously alongside the established fraud handling logic.
internal/core/domain/conviction.go (1)
21-21: LGTM! New crime type properly defined.The
CrimeTypeDoubleSpendconstant and its string mapping follow the existing pattern correctly.Also applies to: 34-34
louisinger
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/core/application/service.go (1)
731-738: Consider usingcontext.Background()for fraud reaction.The fraud reaction is spawned asynchronously and may take time to complete. Using the
ctxparameter couples this operation to the scanner's context lifetime. Consider usingcontext.Background()to ensure fraud reactions complete independently, similar to the pattern infraud.golines 45-54.Based on learnings.
Apply this diff:
if vtxo.Spent { log.Infof("fraud detected on vtxo %s", vtxo.Outpoint.String()) go func() { - if err := s.reactToFraud(ctx, vtxo, mutx); err != nil { + if err := s.reactToFraud(context.Background(), vtxo, mutx); err != nil { log.WithError(err).Warnf( "failed to react to fraud for vtxo %s", vtxo.Outpoint.String(), ) } }() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/core/application/fraud.go(1 hunks)internal/core/application/service.go(1 hunks)test/e2e/e2e_test.go(3 hunks)test/e2e/utils_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-28T08:21:01.170Z
Learnt from: louisinger
PR: arkade-os/arkd#686
File: internal/core/application/fraud.go:47-61
Timestamp: 2025-08-28T08:21:01.170Z
Learning: In reactToFraud function in internal/core/application/fraud.go, the goroutine that waits for confirmation and schedules checkpoint sweep should use context.Background() instead of the request context, as this is intentional design to decouple the checkpoint sweep scheduling from the request lifetime.
Applied to files:
internal/core/application/fraud.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit tests
- GitHub Check: Build and Scan
- GitHub Check: integration tests
altafan
left a comment
altafan
left a comment
There was a problem hiding this comment.
@louisinger the tx can be put onchain by somebody else and we may end up banning an innocent? Maybe better to handle this case manually?
|
Since the code is here, let's ,make it configurable. Also ensure to write to logs and otel so that we can issue alerts through filters |
Co-authored-by: Pietralberto Mazza <18440657+altafan@users.noreply.github.com> Signed-off-by: louisinger <41042567+louisinger@users.noreply.github.com>
3 participants
Ban script trying to double-spend a VTXO.
@altafan @Kukks please review
Summary by CodeRabbit
New Features
Tests