Watch batches instead of vtxos pkscript#854
Watch batches instead of vtxos pkscript#854louisinger wants to merge 7 commits intoarkade-os:masterfrom
Conversation
WalkthroughRefactors watching from per-vtxo to root-tx/script-based watching; adds a watcher that processes scanner notifications; introduces UnwatchAllScripts across API, scanner, NBXplorer and wallet; adds GetChildrenTxs repository APIs and implementations; renames VtxoWithValue→OutpointWithValue; updates tests and gRPC handlers. Changes
Sequence Diagram(s)sequenceDiagram
participant Scanner
participant Watcher
participant Repo as DB/Repo
participant Wallet
participant Sweeper
Scanner->>Watcher: Notification(map[outpoint]OutpointWithValue)
rect `#F0F8FF`
Watcher->>Repo: GetVtxosByOutpoint(outpoint)
Repo-->>Watcher: vtxo or nil
end
alt vtxo found (has ArkTxid)
rect `#EBF5FF`
Watcher->>Watcher: parse Ark PSBT -> extract scripts
Watcher->>Wallet: WatchScripts(scripts)
Wallet-->>Watcher: OK
end
else batch root (no vtxo, vout==0)
rect `#FFF5EB`
Watcher->>Repo: GetChildrenTxs(rootTxid)
Repo-->>Watcher: child txids
Watcher->>Wallet: WatchScripts(childScripts)
Wallet-->>Watcher: OK
end
end
alt vtxo on-chain / state transitions
rect `#FFECEC`
Watcher->>Repo: MarkUnrolled / UnscheduleSweeperInputs
par
Watcher->>Sweeper: reactToFraud(vtxo) (async)
and
Watcher->>Repo: Update DB state (async)
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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
♻️ Duplicate comments (1)
internal/infrastructure/db/postgres/sqlc/queries/query.sql.go (1)
162-193: SelectChildrenTxs wrapper mirrors the SQL definitionThis is the standard sqlc-generated wrapper around the
SelectChildrenTxsquery already reviewed ininternal/infrastructure/db/postgres/sqlc/query.sql; query text and scan logic are consistent.
🧹 Nitpick comments (7)
pkg/ark-lib/tree/tx_tree.go (1)
38-66: Avoid returning address of range variable inFlatTxTree.RootThe current implementation returns
&nodefrom afor _, node := range cloop. While it works here due to the early return, this pattern is fragile and commonly flagged by linters because the range variable is reused each iteration.You can keep the same behavior but use indices to avoid the pattern:
func (c FlatTxTree) Root() *TxTreeNode { - if len(c) == 1 { - return &c[0] - } - - // the root is the node not being a child of another one - allchildren := make(map[string]struct{}) - for _, node := range c { - for _, child := range node.Children { - allchildren[child] = struct{}{} - } - } - - for _, node := range c { - if _, ok := allchildren[node.Txid]; !ok { - return &node - } - } + if len(c) == 1 { + return &c[0] + } + + // the root is the node not being a child of another one + allchildren := make(map[string]struct{}) + for _, node := range c { + for _, child := range node.Children { + allchildren[child] = struct{}{} + } + } + + for i := range c { + if _, ok := allchildren[c[i].Txid]; !ok { + return &c[i] + } + } return nil }
RootTxiddelegating toRoot()then stays unchanged.pkg/arkd-wallet-btcwallet/interface/grpc/handlers/wallet_handler.go (1)
100-104: Clarify UnwatchAllScripts behavior for btcwallet backendThis RPC is exposed in protobuf/OpenAPI but here just returns a generic “not implemented” error, which surfaces as an UNKNOWN gRPC status. Consider either:
- Wiring it through to an actual walletSvc/scanner implementation when available, or
- At minimum, returning a proper gRPC
codes.Unimplementedstatus so clients can distinguish “not supported” from real failures, e.g.:func (h *WalletServiceHandler) UnwatchAllScripts( - ctx context.Context, request *arkwalletv1.UnwatchAllScriptsRequest, + ctx context.Context, _ *arkwalletv1.UnwatchAllScriptsRequest, ) (*arkwalletv1.UnwatchAllScriptsResponse, error) { - return nil, fmt.Errorf("not implemented") + return nil, status.Error(codes.Unimplemented, "UnwatchAllScripts not supported by btcwallet backend") }internal/infrastructure/db/badger/ark_repo.go (1)
231-263: GetChildrenTxs / findInVtxoTree work but can be simplifiedThe traversal logic is sound and returns the serialized child txs for a given node, consistent with the SQL backends. A couple of minor refinements you might consider:
findInVtxoTreeonly ever returns “not found” as an error; using an error for control flow makes callers treat parent- vs child-lookups differently. A simpler signature likefunc findInVtxoTree(tree tree.FlatTxTree, txid string) *tree.TxTreeNodewith nil meaning “not found” would make the intent clearer and avoid swallowing errors in one place and surfacing them in another.- Inside
findInVtxoTree, returning&tree[i](after iterating by index) instead of&nodeavoids the common “pointer to range variable” pattern and makes it obvious the pointer refers directly to the underlying slice element.- The parameter name
treeshadows the importedtreepackage; renaming it (e.g.flatTree) would improve readability.These are all optional; the current behavior is functionally correct for typical tree sizes.
Also applies to: 564-571
internal/test/e2e/e2e_test.go (1)
2366-2619: Consider extracting shared test setup to reduce duplication.The setup logic (lines 2366-2619) is nearly identical to the "cltv vtxo script" test above. While duplication in e2e tests is sometimes acceptable for clarity, you might consider extracting the common CLTV vtxo setup and offchain transfer logic into a helper function that returns the necessary state (alice, bobWallet, arkClient, initialTreeVtxo, bobTxid, etc.). This would reduce maintenance burden when the shared logic needs updates.
internal/core/application/watcher.go (2)
26-39: Unbounded goroutine spawning for notification processing.Each notification spawns a new goroutine per outpoint without any concurrency limit. Under high notification volume, this could lead to resource exhaustion.
Consider using a worker pool or semaphore to limit concurrent processing:
+const maxConcurrentNotifications = 100 + func (s *service) listenToScannerNotifications(ctx context.Context) { ch := s.scanner.GetNotificationChannel(ctx) + sem := make(chan struct{}, maxConcurrentNotifications) go func() { for { select { case <-ctx.Done(): return case notifications := <-ch: for _, notification := range notifications { for _, outpoint := range notification { + sem <- struct{}{} go func() { defer func() { + <-sem if r := recover(); r != nil { log.WithError(fmt.Errorf("panic: %v", r)). Error("panic while processing notification") } }()
137-160: Missing panic recovery in background goroutines.The goroutines spawned in
onVtxoOnchainlack panic recovery, unlike the parent notification handler. A panic in any of these goroutines would crash the process.if vtxo.Preconfirmed { go func() { + defer func() { + if r := recover(); r != nil { + log.WithError(fmt.Errorf("panic: %v", r)). + Error("panic while processing preconfirmed vtxo") + } + }() txs, err := s.repoManager.Rounds().GetTxsWithTxids(ctx, []string{vtxo.Txid})Apply similar recovery to the other goroutines at lines 164 and 179.
internal/core/application/service.go (1)
3449-3477: Potential N+1 query pattern when fetching children transactions.For each sweepable output,
GetChildrenTxsis called individually. Under high round counts with many sweepable outputs, this could result in many database queries.Consider batching if performance becomes an issue, though this is startup-only code so the impact may be acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
api-spec/protobuf/gen/ark/v1/indexer.pb.rgw.gois excluded by!**/gen/**api-spec/protobuf/gen/arkwallet/v1/bitcoin_wallet.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/arkwallet/v1/bitcoin_wallet.pb.rgw.gois excluded by!**/gen/**api-spec/protobuf/gen/arkwallet/v1/bitcoin_wallet_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (26)
api-spec/openapi/swagger/arkwallet/v1/bitcoin_wallet.openapi.json(2 hunks)api-spec/protobuf/arkwallet/v1/bitcoin_wallet.proto(2 hunks)internal/core/application/fraud.go(1 hunks)internal/core/application/service.go(5 hunks)internal/core/application/watcher.go(1 hunks)internal/core/domain/offchain_tx.go(0 hunks)internal/core/domain/round_repo.go(1 hunks)internal/core/ports/scanner.go(1 hunks)internal/infrastructure/db/badger/ark_repo.go(2 hunks)internal/infrastructure/db/postgres/round_repo.go(1 hunks)internal/infrastructure/db/postgres/sqlc/queries/query.sql.go(1 hunks)internal/infrastructure/db/postgres/sqlc/query.sql(1 hunks)internal/infrastructure/db/service_test.go(2 hunks)internal/infrastructure/db/sqlite/round_repo.go(1 hunks)internal/infrastructure/db/sqlite/sqlc/queries/query.sql.go(1 hunks)internal/infrastructure/db/sqlite/sqlc/query.sql(1 hunks)internal/infrastructure/tx-builder/covenantless/mocks_test.go(1 hunks)internal/infrastructure/wallet/wallet_client.go(3 hunks)internal/test/e2e/e2e_test.go(1 hunks)pkg/ark-lib/tree/tx_tree.go(2 hunks)pkg/arkd-wallet-btcwallet/interface/grpc/handlers/wallet_handler.go(1 hunks)pkg/arkd-wallet/core/application/scanner/service.go(1 hunks)pkg/arkd-wallet/core/application/types.go(1 hunks)pkg/arkd-wallet/core/infrastructure/nbxplorer/service.go(1 hunks)pkg/arkd-wallet/core/ports/nbxplorer.go(1 hunks)pkg/arkd-wallet/interface/grpc/handlers/wallet_handler.go(1 hunks)
💤 Files with no reviewable changes (1)
- internal/core/domain/offchain_tx.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-28T08:21:01.170Z
Learnt from: louisinger
Repo: arkade-os/arkd PR: 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
📚 Learning: 2025-08-19T10:58:41.042Z
Learnt from: louisinger
Repo: arkade-os/arkd PR: 691
File: internal/core/application/service.go:557-562
Timestamp: 2025-08-19T10:58:41.042Z
Learning: In the arkd SubmitOffchainTx method, using the checkpoint PSBT input's tapscript (forfeit path) for the VtxoInput.Tapscript field is the correct behavior, not a bug as initially thought. The system correctly handles the relationship between checkpoint inputs and Ark transaction inputs.
Applied to files:
internal/test/e2e/e2e_test.go
🧬 Code graph analysis (11)
internal/infrastructure/db/service_test.go (4)
pkg/ark-lib/tree/tx_tree.go (1)
FlatTxTree(36-36)internal/infrastructure/db/badger/ark_repo.go (1)
Tx(437-440)internal/infrastructure/db/postgres/sqlc/queries/models.go (1)
Tx(178-185)internal/infrastructure/db/sqlite/sqlc/queries/models.go (1)
Tx(165-172)
pkg/arkd-wallet/interface/grpc/handlers/wallet_handler.go (2)
pkg/arkd-wallet-btcwallet/interface/grpc/handlers/wallet_handler.go (1)
WalletServiceHandler(15-17)api-spec/protobuf/gen/arkwallet/v1/bitcoin_wallet.pb.go (6)
UnwatchAllScriptsRequest(2554-2558)UnwatchAllScriptsRequest(2571-2571)UnwatchAllScriptsRequest(2586-2588)UnwatchAllScriptsResponse(2590-2594)UnwatchAllScriptsResponse(2607-2607)UnwatchAllScriptsResponse(2622-2624)
pkg/arkd-wallet-btcwallet/interface/grpc/handlers/wallet_handler.go (2)
pkg/arkd-wallet/interface/grpc/handlers/wallet_handler.go (1)
WalletServiceHandler(17-20)api-spec/protobuf/gen/arkwallet/v1/bitcoin_wallet.pb.go (6)
UnwatchAllScriptsRequest(2554-2558)UnwatchAllScriptsRequest(2571-2571)UnwatchAllScriptsRequest(2586-2588)UnwatchAllScriptsResponse(2590-2594)UnwatchAllScriptsResponse(2607-2607)UnwatchAllScriptsResponse(2622-2624)
internal/infrastructure/db/sqlite/sqlc/queries/query.sql.go (2)
internal/infrastructure/db/postgres/sqlc/queries/db.go (1)
Queries(23-25)internal/infrastructure/db/sqlite/sqlc/queries/db.go (1)
Queries(23-25)
internal/infrastructure/db/postgres/sqlc/queries/query.sql.go (2)
internal/infrastructure/db/postgres/sqlc/queries/db.go (1)
Queries(23-25)internal/infrastructure/db/sqlite/sqlc/queries/db.go (1)
Queries(23-25)
internal/core/ports/scanner.go (2)
internal/core/application/types.go (1)
Outpoint(177-177)api-spec/protobuf/gen/ark/v1/types.pb.go (3)
Outpoint(24-30)Outpoint(43-43)Outpoint(58-60)
internal/infrastructure/tx-builder/covenantless/mocks_test.go (1)
internal/core/ports/scanner.go (1)
OutpointWithValue(9-12)
internal/core/application/watcher.go (4)
pkg/errors/errors.go (1)
Error(39-46)internal/core/ports/alerts.go (1)
ArkTx(9-9)pkg/ark-lib/script/script.go (1)
IsSubDustScript(126-130)internal/core/application/types.go (1)
VOut(131-131)
internal/infrastructure/db/badger/ark_repo.go (2)
internal/core/domain/round.go (1)
RoundFinalizationStage(14-14)pkg/ark-lib/tree/tx_tree.go (2)
FlatTxTree(36-36)TxTreeNode(26-32)
internal/infrastructure/wallet/wallet_client.go (2)
api-spec/protobuf/gen/arkwallet/v1/bitcoin_wallet.pb.go (3)
UnwatchAllScriptsRequest(2554-2558)UnwatchAllScriptsRequest(2571-2571)UnwatchAllScriptsRequest(2586-2588)internal/core/ports/scanner.go (1)
OutpointWithValue(9-12)
internal/core/application/service.go (4)
pkg/ark-lib/tree/tx_tree.go (2)
TxTreeNode(26-32)NewTxTree(79-143)internal/infrastructure/db/badger/ark_repo.go (1)
Tx(437-440)pkg/arkd-wallet-btcwallet/core/psbt.go (1)
ANCHOR_PKSCRIPT(17-19)pkg/ark-lib/script/script.go (1)
IsSubDustScript(126-130)
⏰ 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: Build and Scan
- GitHub Check: unit tests
- GitHub Check: integration tests
🔇 Additional comments (28)
pkg/arkd-wallet/core/ports/nbxplorer.go (1)
55-59: Interface extension for bulk unwatch looks consistentAdding
UnwatchAllAddresses(ctx context.Context) errorcleanly extends the nbxplorer backend API and matches the new scanner and service methods; no issues from an interface-design standpoint.internal/infrastructure/db/sqlite/round_repo.go (1)
456-458: Thin delegation forGetChildrenTxslooks goodDelegating
GetChildrenTxsdirectly tor.querier.SelectChildrenTxsmatches the RoundRepository interface and keeps the repository layer simple; no issues here.pkg/arkd-wallet/core/application/types.go (1)
52-63: BlockchainScannerUnwatchAllScriptsinterface is consistentAdding
UnwatchAllScripts(ctx context.Context) errorfits naturally alongsideWatchScriptsandUnwatchScripts, and matches the scanner implementation that delegates to nbxplorer; the interface change itself looks solid.internal/infrastructure/db/postgres/round_repo.go (1)
425-427: PostgresGetChildrenTxsmirrors sqlite implementation correctlyThe Postgres repository’s
GetChildrenTxsdelegates straight toSelectChildrenTxs, matching the interface and sqlite backend; implementation is appropriately minimal.pkg/arkd-wallet/core/application/scanner/service.go (1)
111-113: Scanner bulk-unwatch wiring is straightforward
UnwatchAllScriptscleanly delegates tonbxplorer.UnwatchAllAddresses, which is exactly what we want for a bulk-unwatch operation; once the nbxplorer groupID handling is fixed, this looks good.internal/core/domain/round_repo.go (1)
9-28: RoundRepository interface extension forGetChildrenTxsis coherentAdding
GetChildrenTxs(ctx context.Context, txid string) ([]string, error)aligns with the rest of the repository API and is already implemented for sqlite and Postgres; the interface change itself looks good.internal/infrastructure/db/postgres/sqlc/query.sql (1)
238-245: SelectChildrenTxs JSONB usage and semantics look consistentThe query correctly resolves direct children from the
childrenJSONB field and returns theirtxpayloads (not txids), which is consistent with how other tx-fetching queries return serialized transactions.internal/infrastructure/db/sqlite/sqlc/query.sql (1)
243-250: SQLite SelectChildrenTxs matches Postgres behaviorThis SQLite query mirrors the Postgres implementation, using
json_eachoverchildrento fetch the direct children’stxpayloads for a given tree txid; looks good.internal/infrastructure/db/sqlite/sqlc/queries/query.sql.go (1)
160-191: SelectChildrenTxs wrapper is correctQuery text matches the SQL definition and the method iterates/collects rows with standard error handling; no issues spotted.
internal/core/application/fraud.go (1)
30-57: reactToFraud signature cleanup looks fineDropping the unused mutex parameter simplifies the method surface without changing its internal fraud-handling logic; assuming call sites are updated (as in the rest of the PR), this looks good.
api-spec/openapi/swagger/arkwallet/v1/bitcoin_wallet.openapi.json (2)
1102-1141: New /v1/wallet/unwatch-all-scripts endpoint is wired correctlyThe OpenAPI path correctly maps WalletService_UnwatchAllScripts to a POST endpoint with the expected request/response wiring and standard error schema, consistent with the existing wallet API style.
1904-1911: UnwatchAllScripts request/response schemas are coherentThe empty
UnwatchAllScriptsRequest/UnwatchAllScriptsResponseschemas match the RPC shape for a parameter-less “bulk unwatch” call; no issues here. Just be aware that withrequired: trueon the requestBody, clients should send an empty JSON object{}.api-spec/protobuf/arkwallet/v1/bitcoin_wallet.proto (1)
180-185: LGTM - New bulk unwatch RPC follows established patterns.The
UnwatchAllScriptsRPC definition and its corresponding request/response messages are consistent with the existingWatchScriptsandUnwatchScriptspatterns. Using POST for this state-changing operation is appropriate.Also applies to: 387-389
internal/core/ports/scanner.go (1)
9-18: LGTM - Interface changes are well-structured.The rename from
VtxoWithValuetoOutpointWithValueis semantically more accurate for the intended purpose. The newUnwatchAllScriptsmethod appropriately extends theBlockchainScannerinterface for bulk unwatch operations, and the channel type update is consistent with the type rename.internal/infrastructure/tx-builder/covenantless/mocks_test.go (1)
249-264: LGTM - Mock implementations correctly updated.The
UnwatchAllScriptsmock follows the established pattern in the file, and theGetNotificationChannelmock properly uses the renamedports.OutpointWithValuetype with consistent type assertions.pkg/arkd-wallet/interface/grpc/handlers/wallet_handler.go (1)
121-128: LGTM - Handler implementation follows established patterns.The
UnwatchAllScriptshandler correctly delegates to the scanner service and returns an appropriate empty response on success. The implementation is consistent with the adjacentUnwatchScriptshandler.internal/infrastructure/db/service_test.go (2)
43-118: LGTM - Deterministic tree structure enables reliable testing.The predefined txid strings and expanded vtxoTree with explicit parent-child relationships provide a solid foundation for testing
GetChildrenTxs. The tree structure is clear and the relationships are well-defined.
584-620: LGTM - Comprehensive test coverage for GetChildrenTxs.The test cases thoroughly cover the three key scenarios:
- Node with children - verifies returned transactions match expected children
- Node without children - verifies empty result
- Non-existent txid - verifies empty result without error
The verification logic correctly maps child txids to their transactions and validates the returned data.
internal/infrastructure/wallet/wallet_client.go (2)
112-115: LGTM - UnwatchAllScripts follows established client patterns.The implementation correctly delegates to the gRPC client with the appropriate request type.
125-167: LGTM - GetNotificationChannel properly updated for OutpointWithValue.The channel type, map construction, and struct initialization are all correctly updated. The field mapping from the proto
VtxoWithKey(Txid, Vout, Value) toOutpointWithValue(Outpoint{Txid, VOut}, Value) is accurate.internal/test/e2e/e2e_test.go (1)
2366-2680: Test validates important post-restart fraud detection scenario.This test adds crucial coverage for ensuring fraud detection continues to work after arkd restarts. The test structure correctly:
- Sets up a CLTV vtxo and performs an offchain transfer
- Restarts arkd (line 2622)
- Verifies bob's vtxo is still available post-restart (lines 2627-2639)
- Triggers fraud by broadcasting redeem transactions
- Verifies the server detects and handles the fraud correctly
internal/core/application/watcher.go (2)
106-112: Potential race condition with missing children transactions.If a batch outpoint notification arrives before children transactions are persisted to the database, this returns an error. Consider whether a retry mechanism or graceful handling would be more appropriate.
txs, err := s.repoManager.Rounds().GetChildrenTxs(ctx, outpoint.Txid) if err != nil { return err } if len(txs) == 0 { - return fmt.Errorf("no children txs found for batch outpoint %s", outpoint.String()) + log.Warnf("no children txs found for batch outpoint %s, may need retry", outpoint.String()) + return nil }Could this scenario occur where children transactions aren't yet stored when the batch notification arrives?
177-186: Fraud reaction failure is silently logged as warning.Fraud detection is a critical security operation. If
reactToFraudfails, only a warning is logged and execution continues. Consider whether fraud handling requires stronger guarantees (e.g., retry, alerting, or persistent queue).Is the current warning-level logging sufficient for fraud handling failures, or should this trigger alerts or be persisted for retry?
internal/core/application/service.go (5)
290-316: LGTM!The root-transaction based watching logic is well-structured. Using
context.Background()for the async watch operation ensures it completes independently of the caller's context. The filtering of anchor and sub-dust scripts is consistent with the PR's approach.
368-371: LGTM!Proper initialization order:
restoreWatchingScriptsensures watches are restored beforelistenToScannerNotificationsstarts processing new notifications.
400-402: LGTM!Proper cleanup during shutdown. Using
context.Background()is correct here since the service context is already cancelled at this point.
2288-2305: LGTM!Good defensive checks to avoid unnecessary cache operations with empty identifiers. This prevents potential edge cases during round cleanup.
3480-3489: Inconsistent error handling in fallback path.When
GetVtxoPubKeysByCommitmentTxidfails (lines 3481-3484), the error is returned, causing the entire restoration to fail. This differs from other errors in the loop which log warnings and continue. Consider whether this should also continue with a warning for consistency:// else, fallback to the tapkeys of the vtxos tapKeys, err := s.repoManager.Vtxos().GetVtxoPubKeysByCommitmentTxid(ctx, commitmentTxId, 0) if err != nil { - return err + log.WithError(err).Warnf("failed to get vtxo pubkeys for commitment %s", commitmentTxId) + continue }Is returning the error here intentional to ensure critical vtxo scripts are always watched, or should this be consistent with other error handling?
| if len(sweepableOutputs) > 0 { | ||
| for _, outputs := range sweepableOutputs { | ||
| for _, output := range outputs { | ||
| childrenTxs, err := s.repoManager.Rounds(). |
There was a problem hiding this comment.
why do we have to source them from the repo if we already have the whole vtxo tree here (L3430)?
There was a problem hiding this comment.
GetChildrenTxs also handle the case the leaf is onchain (so children is not in the tree).
btw this logic is a "workaround" and is not efficient. It should be replaced by the proper "onchain" state of batches persisted in DB
| } | ||
|
|
||
| // UnwatchAllAddresses removes all addresses from group via DELETE /v1/groups/{groupID}/children/delete endpoint. | ||
| func (n *nbxplorer) UnwatchAllAddresses(ctx context.Context) error { |
There was a problem hiding this comment.
@Kukks can you double-check the endpoints used here to watch/unwatch?
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
internal/test/e2e/e2e_test.go (1)
2368-2469: Fraud‑after‑restart e2e flow is solid; consider minor naming/DRY nits.The new
"with arkd restart"subtest correctly mirrors the"default vtxo script"scenario, adds arestartArkd()in between, and then re‑validates that the spent vtxo cannot be reclaimed (no locked on‑chain balance). For readability and consistency, you might (optionally) rename it to"after arkd restart"as previously suggested and/or factor the common setup/unroll logic into a small helper shared by both subtests.pkg/arkd-wallet/core/infrastructure/nbxplorer/service.go (1)
634-647: Update the comment to match the correct endpoint.Good catch on clearing the
groupIDafter deletion (line 645)—this properly addresses the previous concern about subsequentWatchAddressescalls.However, the comment at line 634 is incorrect. The endpoint is
/v1/groups/{groupID}, not/v1/groups/{groupID}/children/delete. The code correctly uses the standard NBXplorer delete group endpoint. Update the comment:-// UnwatchAllAddresses removes all addresses from group via DELETE /v1/groups/{groupID}/children/delete endpoint. +// UnwatchAllAddresses removes all addresses by deleting the group via DELETE /v1/groups/{groupID} endpoint. func (n *nbxplorer) UnwatchAllAddresses(ctx context.Context) error {
🧹 Nitpick comments (3)
internal/core/domain/round_repo.go (1)
9-28: Clarify whatGetChildrenTxsreturns (txids vs serialized txs).Given
GetTxsWithTxidsalready returns serialized transactions, the newGetChildrenTxslikely returnstxpayloads too, but the name can be read as “children txids”. Consider a short interface comment (or a more explicit name) to document that the result slice contains raw transactions, not ids.internal/infrastructure/tx-builder/covenantless/mocks_test.go (1)
252-265: Mock updates forUnwatchAllScriptsandGetNotificationChannellook correct.The new
UnwatchAllScriptshook and the updated notification channel type (OutpointWithValue) are wired consistently with the other mock methods and should drop into existing tests cleanly, as long as expectations return a channel of the exact mapped type.internal/core/application/service.go (1)
3601-3609: Fallback to tapkey watching could be inefficient for large batches.When
findSweepableOutputsreturns empty (lines 3557-3567), the code falls back to watching individual vtxo tapkeys. While this provides a safety net, it could:
- Watch a large number of scripts if the batch has many vtxos
- Partially defeat the PR's goal of reducing watched scripts if
findSweepableOutputshas false negativesConsider adding metrics or logging to track how often this fallback is used, which would help identify if
findSweepableOutputsneeds improvement.Example enhancement:
// else, fallback to the tapkeys of the vtxos + log.Debugf("no sweepable outputs found for round %s, falling back to watching %d vtxo tapkeys", commitmentTxId, len(tapKeys)) tapKeys, err := s.repoManager.Vtxos().GetVtxoPubKeysByCommitmentTxid(ctx, commitmentTxId, 0)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
api-spec/protobuf/gen/ark/v1/indexer.pb.rgw.gois excluded by!**/gen/**
📒 Files selected for processing (15)
internal/core/application/fraud.gointernal/core/application/service.gointernal/core/application/sweeper.gointernal/core/application/watcher.gointernal/core/domain/round_repo.gointernal/core/ports/scanner.gointernal/infrastructure/db/postgres/sqlc/queries/query.sql.gointernal/infrastructure/db/postgres/sqlc/query.sqlinternal/infrastructure/db/service_test.gointernal/infrastructure/db/sqlite/sqlc/queries/query.sql.gointernal/infrastructure/db/sqlite/sqlc/query.sqlinternal/infrastructure/tx-builder/covenantless/mocks_test.gointernal/infrastructure/wallet/wallet_client.gointernal/test/e2e/e2e_test.gopkg/arkd-wallet/core/infrastructure/nbxplorer/service.go
✅ Files skipped from review due to trivial changes (1)
- internal/core/application/sweeper.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/infrastructure/wallet/wallet_client.go
- internal/infrastructure/db/sqlite/sqlc/query.sql
- internal/infrastructure/db/postgres/sqlc/queries/query.sql.go
- internal/core/application/watcher.go
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: louisinger
Repo: arkade-os/arkd PR: 854
File: internal/core/application/watcher.go:114-126
Timestamp: 2025-12-04T09:04:11.726Z
Learning: In arkd's watcher logic, batch outputs cannot contain subdust scripts by design, so the IsSubDustScript check is only necessary when processing vtxo outputs (where ark transactions may include subdust scripts) but not when processing batch outpoints.
📚 Learning: 2025-08-28T08:21:01.170Z
Learnt from: louisinger
Repo: arkade-os/arkd PR: 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
📚 Learning: 2025-08-19T10:58:41.042Z
Learnt from: louisinger
Repo: arkade-os/arkd PR: 691
File: internal/core/application/service.go:557-562
Timestamp: 2025-08-19T10:58:41.042Z
Learning: In the arkd SubmitOffchainTx method, using the checkpoint PSBT input's tapscript (forfeit path) for the VtxoInput.Tapscript field is the correct behavior, not a bug as initially thought. The system correctly handles the relationship between checkpoint inputs and Ark transaction inputs.
Applied to files:
internal/test/e2e/e2e_test.go
🧬 Code graph analysis (6)
internal/infrastructure/db/sqlite/sqlc/queries/query.sql.go (1)
internal/infrastructure/db/sqlite/sqlc/queries/db.go (1)
Queries(23-25)
internal/core/ports/scanner.go (2)
internal/core/application/types.go (1)
Outpoint(180-180)api-spec/protobuf/gen/ark/v1/types.pb.go (3)
Outpoint(24-30)Outpoint(43-43)Outpoint(58-60)
internal/infrastructure/db/service_test.go (4)
pkg/ark-lib/tree/tx_tree.go (1)
FlatTxTree(36-36)internal/infrastructure/db/badger/ark_repo.go (1)
Tx(437-440)internal/infrastructure/db/postgres/sqlc/queries/models.go (1)
Tx(178-185)internal/infrastructure/db/sqlite/sqlc/queries/models.go (1)
Tx(165-172)
internal/infrastructure/tx-builder/covenantless/mocks_test.go (2)
pkg/errors/errors.go (1)
Error(39-46)internal/core/ports/scanner.go (1)
OutpointWithValue(9-12)
internal/core/application/service.go (3)
pkg/ark-lib/tree/tx_tree.go (2)
TxTreeNode(26-32)NewTxTree(79-143)pkg/arkd-wallet-btcwallet/core/psbt.go (1)
ANCHOR_PKSCRIPT(17-19)pkg/ark-lib/script/script.go (1)
IsSubDustScript(126-130)
internal/test/e2e/e2e_test.go (1)
internal/core/application/admin.go (1)
Receiver(744-748)
⏰ 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 (8)
internal/infrastructure/db/postgres/sqlc/query.sql (1)
238-245:SelectChildrenTxsquery shape looks correct and consistent with JSON schema.Using
jsonb_path_query_array(children, '$.*')to extract all child txids and joining back ontx.txidis sound and scoped to tree rows; this should behave correctly for empty/missingchildrenas well. LGTM.internal/core/application/fraud.go (1)
24-70: Confirm concurrency assumptions after removing the per-call mutex.With
reactToFraudnow lock‑free, multiple fraud detections for the same vtxo could, in principle, race to broadcast/schedule the same checkpoint or forfeit flow. If upper layers already dedupe per‑vtxo fraud handling, this simplification is fine; otherwise, we may still need per‑vtxo coordination (e.g. map of in‑flight reactions). The continued use ofcontext.Background()inside the goroutine remains aligned with the earlier design to decouple checkpoint sweep scheduling from request lifetime.
Based on learnings, …internal/infrastructure/db/sqlite/sqlc/queries/query.sql.go (1)
160-191: SQLiteSelectChildrenTxsimplementation matches the Postgres behavior.The JSON traversal via
json_each(t2.children)andWHERE t1.txid IN (subquery)correctly pulls child transactions for a tree node, and the iterator boilerplate/row handling is standard sqlc style. This should keep Postgres/SQLite semantics aligned forGetChildrenTxs.internal/core/ports/scanner.go (1)
9-23: Scanner port changes align with batch‑script watching objectives.Generalizing to
OutpointWithValueand addingUnwatchAllScriptsonBlockchainScanner, with notifications carryingmap[string][]OutpointWithValue, fits the new “watch batches not all vtxos” model. Just ensure all implementations (wallet client, nbxplorer adapter, tests) have been updated to the new method and type signatures.internal/infrastructure/db/service_test.go (2)
42-118: ExpandedvtxoTreefixture gives good coverage for child‑lookup logic.Defining a stable tree (root, intermediate, and explicit leaf nodes) instead of fully random children makes the upcoming
GetChildrenTxsassertions much more meaningful across all backends (badger, sqlite, postgres) without introducing flakiness.
583-619:GetChildrenTxstests exercise the key cases and validate content, not just count.The new checks cover nodes with children, without children, and missing parents, and they verify that returned tx strings correspond exactly to the expected child nodes in
finalizedRound.VtxoTreeirrespective of ordering. This should catch backend discrepancies effectively.internal/core/application/service.go (2)
403-405: LGTM!The bulk unwatch call during shutdown is appropriate. Logging errors without blocking shutdown is the correct approach for cleanup logic.
293-319: Pass service context to the watcher goroutine to prevent leaks.The goroutine spawned here uses
context.Background()forscanner.WatchScripts(line 312), which means it won't be canceled when the service stops. This could cause the goroutine to leak or attempt operations on a stopped scanner during shutdown.🔎 Proposed fix to propagate service context
- go func(rootNode *tree.TxTreeNode) { + go func(ctx context.Context, rootNode *tree.TxTreeNode) { rootPtx, err := psbt.NewFromRawBytes(strings.NewReader(rootNode.Tx), true) if err != nil { log.WithError(err).Warn("failed to parse root tx") return } scripts := make([]string, 0, len(rootPtx.UnsignedTx.TxOut)) for _, out := range rootPtx.UnsignedTx.TxOut { if bytes.Equal(out.PkScript, txutils.ANCHOR_PKSCRIPT) { continue } if script.IsSubDustScript(out.PkScript) { continue } scripts = append(scripts, hex.EncodeToString(out.PkScript)) } if len(scripts) > 0 { - if err := svc.scanner.WatchScripts(context.Background(), scripts); err != nil { + if err := svc.scanner.WatchScripts(ctx, scripts); err != nil { log.WithError(err).Warn("failed to watch scripts") return } log.Debugf("watching %d scripts", len(scripts)) } - }(root) + }(context.Background(), root)⛔ Skipped due to learnings
Learnt from: louisinger Repo: arkade-os/arkd PR: 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.
| func (s *service) restoreWatchingScripts() error { | ||
| ctx := context.Background() | ||
|
|
There was a problem hiding this comment.
Use service context instead of creating a new background context.
Creating a new context.Background() bypasses the service's cancellation mechanism. If the service is stopping, this method should respect that and exit early rather than continuing to process rounds and watch scripts.
🔎 Proposed fix to use service context
func (s *service) restoreWatchingScripts() error {
- ctx := context.Background()
-
+ ctx := s.ctx
commitmentTxIds, err := s.repoManager.Rounds().GetSweepableRounds(ctx)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *service) restoreWatchingScripts() error { | |
| ctx := context.Background() | |
| func (s *service) restoreWatchingScripts() error { | |
| ctx := s.ctx | |
| commitmentTxIds, err := s.repoManager.Rounds().GetSweepableRounds(ctx) |
🤖 Prompt for AI Agents
In internal/core/application/service.go around lines 3534 to 3536, the function
creates a new context with context.Background(), bypassing the service
cancellation; replace that with the service's context (e.g., use s.ctx or the
service's Context() accessor) so the method observes service shutdown, and
ensure any long-running loops or waits check ctx.Done() and return early when
cancelled.
| if len(sweepableOutputs) > 0 { | ||
| for _, outputs := range sweepableOutputs { | ||
| for _, output := range outputs { | ||
| childrenTxs, err := s.repoManager.Rounds(). | ||
| GetChildrenTxs(ctx, output.Txid) | ||
| if err != nil { | ||
| log.WithError(err).Warn("failed to get children txs") | ||
| continue | ||
| } | ||
| for _, childTx := range childrenTxs { | ||
| ptx, err := psbt.NewFromRawBytes(strings.NewReader(childTx), true) | ||
| if err != nil { | ||
| log.WithError(err).Warn("failed to parse child tx") | ||
| continue | ||
| } | ||
| for _, output := range ptx.UnsignedTx.TxOut { | ||
| if bytes.Equal(output.PkScript, txutils.ANCHOR_PKSCRIPT) { | ||
| continue | ||
| } | ||
| if script.IsSubDustScript(output.PkScript) { | ||
| continue | ||
| } | ||
| scripts = append(scripts, hex.EncodeToString(output.PkScript)) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Complex error handling may silently skip watching important scripts.
The nested loop structure with multiple continue statements on error could silently fail to watch scripts for sweepable outputs without clear visibility. If GetChildrenTxs fails or child tx parsing fails, those outputs won't be watched, but only a warning is logged.
Consider:
- Accumulating errors and returning them at the end
- Tracking which commitment txs had failures
- Using more structured logging to indicate which sweepable outputs weren't watched
Example of the concern:
// If this fails, we continue to next output
childrenTxs, err := s.repoManager.Rounds().GetChildrenTxs(ctx, output.Txid)
if err != nil {
log.WithError(err).Warn("failed to get children txs")
continue // Silently skip this output
}This could result in incomplete watching coverage for unrolled batches.
🤖 Prompt for AI Agents
In internal/core/application/service.go around lines 3570 to 3596, the nested
loops swallow failures from GetChildrenTxs and PSBT parsing with only warn logs
which can silently skip watching scripts; change the flow to record per-output
failures and aggregate errors: when GetChildrenTxs or psbt.NewFromRawBytes
fails, log a structured message that includes the parent output.Txid (and
childTx id where applicable), append that Txid to a "failedOutputs" list and the
error to an error accumulator, continue processing other outputs, and after the
loops return or surface a combined error (or attach it to the function result)
so callers know which commitment txs were not fully watched; ensure the normal
script collection still proceeds for successful items.
2 participants
This PR reworks the watching logic. Instead of subscribing to any vtxo pkscript, arkd watches only batch output script. If unrolled, the children will be watched.
It reduces drastically the number of scripts watched by nbxplorer.
it closes #851
@altafan @Kukks please review
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.