Skip to content

fix: verify input VTXOs are spent before breaking poll loop#35

Merged
Kukks merged 5 commits intomasterfrom
fix/post-spend-vtxo-polling
Mar 16, 2026
Merged

fix: verify input VTXOs are spent before breaking poll loop#35
Kukks merged 5 commits intomasterfrom
fix/post-spend-vtxo-polling

Conversation

@Kukks
Copy link
Collaborator

@Kukks Kukks commented Mar 13, 2026

Summary

  • PostSpendVtxoPollingHandler broke out of the retry loop as soon as any VTXO was returned (found > 0), even if the input VTXOs weren't yet marked as spent by arkd's indexer
  • This caused swept VTXOs to remain "unspent" in local storage after auto-sweep (fixes Fix vtxos status if Auto-sweep is enabled ArkLabsHQ/btcpay-arkade#30)
  • Now verifies all input VTXOs have SpentByTransactionId set before breaking the poll loop
  • Added GetVtxosByOutpoints helper to VtxoSynchronizationService for the spent-state check

Test plan

  • Run E2E tests to verify sweep tests pass
  • Verify VTXOs page shows swept VTXOs as spent after auto-sweep

PostSpendVtxoPollingHandler broke out of the retry loop as soon as any
VTXO was returned (found > 0). When arkd returned the new output VTXOs
before marking the input VTXOs as spent, the handler exited early and
the input VTXOs remained "unspent" in local storage.

Now the handler checks that all input VTXOs have SpentByTransactionId
set before breaking the loop, ensuring the local state accurately
reflects the spend.
Kukks added 3 commits March 13, 2026 15:15
The handler now verifies input VTXOs are marked as spent before breaking
the poll loop. Update the test mock to return a spent input VTXO from
local storage so the retry loop exits on the first attempt.
Inject IVtxoStorage into PostSpendVtxoPollingHandler and use its
GetVtxos(outpoints:, includeSpent:) filter directly instead of going
through a wrapper on VtxoSynchronizationService. Removes the
GetVtxosByOutpoints helper that was only used here.
…d polling

Instead of querying local IVtxoStorage for spent state, query arkd directly
via the new GetVtxosByOutpoints transport method with spent_only=true.
This avoids relying on local storage which may lag behind arkd's indexer.
@arkanaai
Copy link

arkanaai bot commented Mar 14, 2026

[Arkana Review — dotnet-sdk PR #35: fix: verify input VTXOs are spent before breaking poll loop]

Reviewed at: 2026-03-14 14:00 UTC


Summary

Fixes a race condition in PostSpendVtxoPollingHandler: the handler previously broke out of the retry loop as soon as local storage showed any matching VTXOs found. The fix adds a second check — querying arkd directly via transport.GetVtxosByOutpoints(..., spentOnly: true) — and only breaks when arkd confirms all input outpoints are marked spent.


What looks good

Correct diagnosis: The bug is real — local storage upsert can complete before arkd's indexer processes the spend event, causing the loop to exit prematurely and leaving the wallet in an inconsistent state.

Fix approach: Querying arkd with spentOnly: true for the input outpoints is the right authoritative check. Local storage is a cache; arkd is the source of truth.

Chunked pagination in GetVtxosByOutpoints: The gRPC implementation chunks at 1000 outpoints and handles pagination (Page.Next != Page.Total). Correct pattern for the arkd indexer RPC.

CachingClientTransport passthrough: The method correctly delegates to _inner without caching — appropriate since this is a spent-status check that must be fresh.

Logging improvement: The new log line at "inputs spent on arkd" gives operators visibility into the retry loop progress.


Questions / potential issues

1. spentCount >= inputOutpoints.Count — strict equality vs >=

Using >= is fine and defensive. However, if arkd returns more spent outpoints than inputOutpoints.Count (unlikely given the query is by specific outpoints), this would still break. Cosmetic — == would be slightly more expressive of intent, but >= is safer.

2. Pagination assumption in GetVtxosByOutpoints

The pagination loop while (response is null || response.Page.Next != response.Page.Total) assumes Page.Total is the total number of pages (not total items). Confirming this matches the arkd IndexerPageResponse semantics — if Next and Total are both page indices, this is correct. If Total is total item count, the condition is wrong. Worth a comment.

3. Cancellation token threading

cancellationToken.ThrowIfCancellationRequested() is checked at the top of the pagination loop, but not between individual yield return statements. For large result sets, cancellation granularity is coarse. Low priority for the typical small number of input outpoints in a VTXO spend.

4. expiresAt heuristic (year >= 2025)

In the new GetVtxosByOutpoints implementation, the expiresAt parsing uses maybeExpiresAt.Year >= 2025 to distinguish a real timestamp from a block height. This is the same heuristic used elsewhere in the transport layer. It will break after year ~2106 when block heights exceed the 2025 timestamp threshold — known issue, acceptable for now but worth tracking.

5. spentOnly filter behavior on arkd

When spentOnly: true, does arkd return the outpoint only if it is spent in a committed (on-chain confirmed) round, or also if it is spent in a pending batch? If pending batches count as spent, the check could give a false positive during batch preparation. If only confirmed spends count, the check is strictly correct. Worth confirming the arkd indexer semantics.


Nits

  • The NNark submodule bump is included — presumably this is a dependency update. Worth mentioning in the PR description what changed in the submodule.
  • Removed IContractStorage contractStorage from constructor and replaced with IClientTransport transport — this is a DI change that callers need to update. Any registered service composition needs to be updated (presumably done in the Blazor/WASM sample).

Verdict

Solid fix for a real correctness issue. The spent-status check via arkd transport is the right approach. Points 2 and 5 are the most important to confirm before merge.

— Arkana

@arkanaai
Copy link

arkanaai bot commented Mar 14, 2026

🔍 Arkana Review — dotnet-sdk#35: fix: verify input VTXOs are spent before breaking poll loop

Scope: VTXO polling handler + new GetVtxosByOutpoints transport method across gRPC and REST clients.


Problem

PostSpendVtxoPollingHandler previously broke out of the retry loop as soon as any VTXO was found (found > 0), but this didn't guarantee the input VTXOs were marked as spent by arkd's indexer. This caused stale "unspent" display in the wallet after auto-sweeps.

Fix Analysis

1. PostSpendVtxoPollingHandler changes ✅

The new logic after found > 0:

await foreach (var _ in transport.GetVtxosByOutpoints(inputOutpoints, spentOnly: true, cancellationToken))
    spentCount++;
if (spentCount >= inputOutpoints.Count)
    break;

Queries arkd directly with spentOnly: true to confirm all input outpoints are marked spent before breaking. Correct approach — avoids relying on local storage cache which was the root cause.

Dependency change from IContractStorageIClientTransport is clean.

2. IClientTransport.GetVtxosByOutpoints interface ✅

Clean addition with proper doc comment. spentOnly maps directly to arkd's indexer API.

3. gRPC implementation ✅

  • Chunking by 1000 — sensible batch size
  • Pagination via Page.Index/Page.Next/Page.Total — matches existing patterns
  • SpentOnly correctly propagated

⚠️ Minor: The expiresAt heuristic (maybeExpiresAt.Year >= 2025) is duplicated from existing code. Consider extracting a shared ParseArkVtxo helper in a follow-up to avoid drift.

4. REST implementation ✅

Same chunking/pagination pattern as gRPC. Reuses existing ParseVtxo() helper — good.

5. CachingClientTransport passthrough ✅

Simple delegation — correct for outpoint-specific spent queries that shouldn't be cached.

6. Tests ✅

Updated test correctly mocks GetVtxosByOutpoints with a spent input VTXO and verifies the handler breaks after the first attempt when both conditions are met (VTXOs found + inputs spent).


Security Review

  • Double-spend vector: This fix improves protection. Previously, the handler could report success before arkd confirmed inputs were spent. Now it verifies spent state server-side.
  • No key handling changes — transport methods are read-only queries.
  • No exit path impact — wallet-side polling only.

Cross-repo

This is the SDK side of ArkLabsHQ/btcpay-arkade#32 (issue #30). The submodule bump in btcpay-arkade points to this commit. Both should merge together.

Nits

  • Missing newline at end of GrpcClientTransport.Vtxo.cs
  • gRPC VTXO parsing could reuse a shared helper to avoid drift with GetVtxoByScriptsAsSnapshot parsing

Verdict

Looks good. Correctly addresses the VTXO polling race by verifying spent state directly with arkd before breaking the retry loop. Clean implementation across both transport layers with proper test coverage.

— Arkana

Copy link

@arkanaai arkanaai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Arkana Review — arkade-os/dotnet-sdk#35

Summary: Fixes a race condition where PostSpendVtxoPollingHandler broke out of the retry loop as soon as any VTXO was found (found > 0), without verifying that input VTXOs were actually marked as spent by arkd's indexer. This caused swept VTXOs to remain "unspent" in local storage after auto-sweep.

✅ What looks good

  1. Correct diagnosis. The old code broke on found > 0 — which only meant some script matched, not that the spend was indexed. Verifying spent status of inputs via transport.GetVtxosByOutpoints(..., spentOnly: true) is the right fix.

  2. New GetVtxosByOutpoints API. Clean addition to IClientTransport with implementations in both gRPC and REST transports. The chunking (1000 per batch) and pagination logic mirrors the existing GetVtxoByScriptsAsSnapshot pattern — consistent.

  3. Dependency change. Swapping IContractStorageIClientTransport in the handler constructor is correct — the handler no longer needs local contract storage, it queries arkd directly for spent state. This removes a layer of indirection that was the source of the bug.

  4. Test coverage. Updated VtxoPollingHandlerTests properly mocks the new GetVtxosByOutpoints call and verifies the two-phase check (scripts first, then spent verification).

⚠️ Minor observations

  1. Partial spent detection. If only some inputs are marked spent (e.g., spentCount < inputOutpoints.Count), the handler continues retrying, which is correct. But if arkd never marks all inputs as spent within 5 attempts (e.g., indexer bug), we silently exit the loop. The existing logging handles this adequately for debugging, but worth noting.

  2. gRPC request fields. In GrpcClientTransport.Vtxo.cs, the new request sets SpendableOnly = false, RecoverableOnly = false, PendingOnly = false, Before = 0, After = 0. These defaults look correct for a raw outpoint lookup with spent-only filter.

  3. REST transport query string. Using HttpUtility.ParseQueryString and adding multiple outpoints keys for the same parameter is correct for repeated query params in .NET — just confirming the arkd REST API accepts this format (it should per standard HTTP query encoding).

🔒 Security

  • No key material handling changes.
  • No new attack surface — the transport call is read-only (GET on indexer).
  • Fixes a correctness issue that could lead to double-spend vectors if local storage showed VTXOs as unspent when they were actually spent on-chain.

Verdict: Clean, well-scoped fix. LGTM.

@Kukks Kukks merged commit 9aa3789 into master Mar 16, 2026
6 checks passed
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.

Fix vtxos status if Auto-sweep is enabled

1 participant