feat: unilateral exit support

[Kukks]

Summary

• Virtual Tx Storage: Per-tx normalized storage model for VTXO exit data. Sibling VTXOs sharing internal tree nodes deduplicate automatically. Supports Lite (txids only) and Full (txids + hex) modes.
• Unilateral Exit Service: Full exit state machine — broadcasts chain of virtual txs root-to-leaf via 1p1c package relay, waits CSV timelock, then claims funds on-chain using the UnilateralPath tapscript.
• Exit Watchtower: Monitors blockchain for partial tree broadcasts and alerts when auto-exit is needed to protect funds from previous owners in the chain.
• P2A CPFP Builder: Builds v3 CPFP child transactions spending P2A anchor outputs (both BIP 431 standard and Ark protocol marker).
• IOnchainBroadcaster: Interface with NBXplorer (Bitcoin Core RPC) and Esplora (REST API) implementations for tx broadcast, package relay, status queries, and fee estimation.
• Event Integration: Auto-fetches exit data on VTXO receive, auto-prunes on VTXO spend.

New files (36 changed, +2577 lines)

Abstractions: IOnchainBroadcaster, IVirtualTxStorage, IExitSessionStorage, ExitSession, VirtualTx, VtxoBranch
Core Services: VirtualTxService, UnilateralExitService, ExitWatchtowerService, P2ACpfpBuilder
Transport: GetVtxoChainAsync, GetVirtualTxsAsync, GetVtxoTreeAsync (gRPC + REST)
Storage: VirtualTxEntity, VtxoBranchEntity, ExitSessionEntity (EF Core)
Tests: 12 new unit tests for VirtualTxService and P2ACpfpBuilder

Test plan

• All 255 unit tests pass (dotnet test NArk.Tests)
• dotnet build NArk.Core — 0 errors
• dotnet build NArk.Storage.EfCore — 0 errors
• E2E tests with nigiri (future PR)
• Integration test with arkd v0.9 (future PR)

[arkanaai]

🔍 Arkana PR Review — feat: unilateral exit support

Scope: +2577 lines across 36 files. Adds full unilateral exit pipeline: virtual tx storage, exit state machine, watchtower, CPFP builder, transport bindings (gRPC + REST), EF Core storage, and 12 unit tests.

Architecture ✅

Clean separation of concerns — abstractions, core services, transport, storage, tests all properly layered. DI registration via AddUnilateralExit() and AddExitWatchtowerBackgroundService() is ergonomic. Event-driven auto-fetch on VTXO receive and auto-prune on spend is a smart design.

🔴 Bug: Hardcoded Network.Main in Broadcasting

UnilateralExitService.cs line ~186:

var tx = Transaction.Parse(vtx.Hex, Network.Main); // Network doesn't matter for parsing

The comment says "Network doesn't matter for parsing" — this is mostly true for raw hex parsing, but it's misleading and fragile. If any downstream code in the same method depends on the parsed tx's network context, this will silently produce wrong results on testnet/signet. The service already calls transport.GetServerInfoAsync() in other methods — should use serverInfo.Network consistently.

🟡 CPFP Not Wired in Broadcasting Path

UnilateralExitService.cs ~190-198:

// Build CPFP child and broadcast as package
// For now, broadcast the parent alone — CPFP child building requires
// wallet UTXOs which will be wired through the fee funding interface
var success = await broadcaster.BroadcastAsync(tx, ct);

The P2ACpfpBuilder is fully implemented and tested but never used in the actual exit flow. For v3 transactions, bare parent broadcast may fail if the parent's fee rate is below mempool min. This is the core use case for 1p1c package relay.

Recommendation: At minimum, document which future PR will wire this, or add a TODO reference. For v3-only virtual txs, this is a correctness concern, not just an optimization.

🟡 Watchtower Detects But Doesn't Act

ExitWatchtowerService.cs ~94-100:

logger?.LogWarning(
    "Auto-exit needed for VTXO {Outpoint} (wallet={WalletId}). " +
    "Call StartExitAsync with a claim address to protect these funds.",
    vtxo.OutPoint, contract.WalletIdentifier);

The watchtower detects partial tree broadcasts (where an adversary may be racing to claim via a prior-owner's CSV path) but only logs. In a real attack scenario, the time window matters — logging without acting could mean lost funds. Understand this is a phased rollout, but this should be clearly documented as a known limitation.

🟢 Protocol Correctness

• Branch ordering (position 0 = root → leaf): Correct for root-to-leaf broadcasting.
• CSV delay check: Uses block height comparison — correct for BIP 68 relative timelocks.
• Commitment type filtering: Correctly excluded from virtual tx chain (already on-chain).
• Claim tx retry: Session transitions back to Claimable if claim tx drops from mempool — solid resilience.
• P2A detection: Handles both BIP 431 standard (OP_1) and Ark protocol marker (OP_1 PUSH2 "Ns") — correct.

🟢 Security

• Exit session state machine prevents double-starts (checks existing session per VTXO).
• VtxoVout stored as uint in records but int in EF Core entities — safe given Bitcoin output indices, but worth noting for consistency.
• Orphan cleanup in PruneForSpentVtxoAsync correctly uses exclusion clause to avoid deleting virtual txs still referenced by sibling VTXOs.
• Claim transaction correctly sets CSV sequence on input.
• P2TR signature uses TaprootSigHash.Default — correct for keypath spend.

🟢 Tests

12 tests covering:

• P2A anchor detection (BIP 431, Ark marker, none)
• CPFP child construction (v3, inputs, signing)
• Virtual tx service (Full/Lite modes, dedup, pruning, hex population)

Good coverage of the core logic. The mock-based approach with NSubstitute is clean.

Minor Notes

1. EsploraOnchainBroadcaster — GetTxStatusAsync returns InMempool: true when tx is not confirmed but found. This assumes the Esplora status endpoint returns a response for mempool txs. Verify this matches Esplora's actual API behavior (some implementations return 404 for mempool-only txs).

2. Fee estimation fallback in both broadcasters defaults to 2 sat/vB — reasonable for now but should be configurable for mainnet use where fee spikes can make 2 sat/vB too low.

3. PostBatchVirtualTxFetchHandler — the 2-second Task.Delay is a code smell. Consider a more robust synchronization mechanism (e.g., event-based trigger after VTXO polling completes).

Summary

Category	Rating
Architecture	✅ Excellent
Protocol correctness	✅ Solid
Security	✅ Good
Test coverage	✅ Good
Completeness	🟡 CPFP + watchtower not wired
Bug	🔴 Hardcoded Network.Main

Overall: Strong foundation for unilateral exit. The Network.Main should be fixed before merge. The CPFP and watchtower gaps are acceptable as phased work if tracked.

---

Review by Arkana 🔍 — PR Reviewer Agent

[arkanaai]

🔍 Arkana PR Review — feat: unilateral exit support

Massive PR (+2673 lines, 37 files). Clean architecture with proper abstraction boundaries. Here's the analysis:

---

✅ What looks solid

• State machine design: ExitSessionState progression (Broadcasting → AwaitingCsvDelay → Claimable → Claiming → Completed) with proper failure handling and rebroadcast logic when claim tx drops from mempool.
• P2A CPFP signing: BuildCpfpChild correctly uses TaprootSigHash.Default on the fee input, committing to all inputs/outputs. P2A anchor input correctly has empty witness.
• Watchtower optimization: Checking root tx first before scanning the full branch is correct — if root isn't on-chain, nothing downstream can be.
• Virtual tx deduplication: Per-tx storage with branch references means sibling VTXOs sharing internal tree nodes naturally share data. Orphan cleanup on prune is the right approach.
• Event integration: Auto-fetch on batch complete, auto-prune on spend. Clean lifecycle management.
• Test coverage: 12 unit tests for VirtualTxService and P2ACpfpBuilder covering key paths (Full/Lite modes, branch caching, commitment filtering, hex population).

---

⚠️ Potential issues

1. Concurrency race in PruneForSpentVtxoAsync (EfCoreVirtualTxStorage.cs)
If two sibling VTXOs are spent simultaneously, both prune operations could see references from the other and skip orphan cleanup, OR both see zero references and attempt double-delete. The remove + orphan-check + save isn't atomic across concurrent calls. Consider wrapping in a serializable transaction or using a single SQL DELETE with a subquery.

2. txs/package endpoint doesn't exist in standard Esplora (EsploraOnchainBroadcaster.cs)
Standard mempool.space/Esplora REST API doesn't expose a package relay endpoint. BroadcastPackageAsync will silently fail on all standard Esplora instances. Should either document the requirement for a custom Esplora extension, or fall back to sequential broadcast.

3. submitpackage requires Bitcoin Core 28.0+ (NBXplorerOnchainBroadcaster.cs)
The RPC command was introduced in Core 28. On older nodes this will throw. Consider checking node version or catching the "unknown command" error and falling back to sequential broadcast.

4. Fragile timing in PostBatchVirtualTxFetchHandler

await Task.Delay(TimeSpan.FromSeconds(2), cancellationToken);

This 2-second wait for VTXO polling to complete is a timing assumption. If polling takes longer (network latency, large batch), the handler won't find the new VTXOs. Consider using a more deterministic signal, or retrying with backoff if no VTXOs found.

5. N+1 queries in UpsertVirtualTxsAsync (EfCoreVirtualTxStorage.cs)
Iterates through txs with individual FindAsync calls. For a VTXO with a deep tree (many virtual txs), this generates N+1 queries. Consider using ExecuteUpdateAsync or batching the lookup with Where(e => txids.Contains(e.Txid)).

6. No retry backoff or max attempts in ProgressBroadcastingAsync
If a broadcast fails, it returns and waits for the next poll cycle with no backoff. A permanently-failing tx would retry every 60 seconds forever. Consider adding a retry counter to ExitSession and failing after a threshold.

7. int vs uint for VtxoVout (EfCoreExitSessionStorage.cs, EfCoreVirtualTxStorage.cs)
The EF Core entities store vout as int but the domain model uses uint. Technically this could overflow for vout > 2^31, though in practice Bitcoin vouts won't hit this. Still, the (int) casts should be documented as intentional.

---

🔐 Security observations

• Fee estimation fallback of 2 sat/vB: For time-sensitive unilateral exits (racing a CSV timeout from a previous owner), 2 sat/vB could be dangerously low. Consider making the fallback configurable or using a higher minimum for exit-critical broadcasts.
• Watchtower scans all unspent VTXOs: CheckAndRespondAsync iterates every unspent VTXO and makes RPC calls for each. For wallets with many VTXOs, this could be slow and could hit rate limits. Consider maintaining a watchlist of VTXOs with stored branches instead of scanning all.
• FeeCoin.SigningKey lifetime: Private key is passed via record. Callers need to ensure proper key disposal/zeroing — worth a comment on the interface.
• Claim address derivation: Using a boarding contract address for claims is clever (keeps funds in the Ark ecosystem), but ensure the derived address doesn't reuse a key that's already committed in another VTXO tree.

---

💡 Suggestions (non-blocking)

• Consider adding a MaxRetries field to ExitSession and ExitWatchtowerOptions
• The README examples are clear and well-structured 👍
• GetVtxoChainAsync and GetVirtualTxsAsync in the gRPC transport properly handle pagination — good
• For the REST transport, expires_at parsing via long.Parse() could throw on malformed data — consider TryParse

---

Overall this is well-architected and covers the critical exit path thoroughly. The main concerns are the Esplora package relay assumption and the concurrency race in pruning. Everything else is refinement-level.

[arkanaai]

🔍 Arkana Review — e1cf189

Summary

Major feature PR: adds full unilateral exit support to the .NET SDK. Includes virtual tx storage (per-tx normalized model), exit state machine (Broadcasting → AwaitingCsvDelay → Claimable → Claiming → Completed), exit watchtower for partial tree broadcast detection, P2A CPFP builder for v3 package relay, and dual broadcaster implementations (NBXplorer/Esplora). +2577 lines across 36 files.

✅ Looks Good

• Architecture is clean. Clear separation: abstractions (IOnchainBroadcaster, IVirtualTxStorage, IExitSessionStorage, IFeeWallet), core services, transport, storage.
• Exit state machine is well-designed. Five states with clear transitions. ProgressExitsAsync() processes each state independently, making the flow resumable and crash-safe.
• VirtualTxService handles Lite/Full modes correctly. Deduplication via HasBranchAsync check, on-demand hex population for Lite→Full upgrade.
• P2A CPFP builder handles both BIP 431 standard and Ark protocol marker (OP_1 PUSH2 "Ns"). Good dual detection.
• NBXplorer broadcaster tries submitpackage first, falls back to sequential for Bitcoin Core < 28. Graceful degradation.
• Esplora broadcaster correctly notes lack of package relay endpoint and falls back to sequential.
• Watchtower optimization: checks root tx first, skips if not on-chain. Avoids N queries per VTXO.
• Event integration (PostBatchVirtualTxFetchHandler, PostSpendVirtualTxPruneHandler) — clean lifecycle management.
• MinExitWorthAmount threshold (default 1000 sats) — good cost-awareness, avoids storing exit data for dust VTXOs.

🟡 Observations

1. UnilateralExitService.ProgressBroadcastingAsync catches all exceptions and calls FailSession — but some failures (e.g., transient network errors) may be recoverable. Consider whether FailSession (state=Failed, terminal) is too aggressive for transient errors. The retry counter helps, but once MaxBroadcastRetries (10) is hit, the session is permanently failed with no recovery path.

2. ExitWatchtowerService.CheckVtxoAsync queries GetTxStatusAsync for root and leaf txids every poll cycle for every unspent VTXO. With many VTXOs this could be expensive. Consider caching or only checking VTXOs whose round is near expiry.

3. P2ACpfpBuilder.BuildCpfpChild hardcodes estimatedChildVsize = 155 in both the builder and the caller (BroadcastWithCpfpAsync). If the actual vsize differs significantly (e.g., different change script type), fees may be under/overpaid. Consider computing this from the actual built transaction.

4. PostBatchVirtualTxFetchHandler uses exponential backoff (2s, 4s, 8s, 16s = 30s total) waiting for VTXOs to appear after batch. This is fine for background processing, but the comment should note that this delay is expected and not a performance bug.

5. EsploraOnchainBroadcaster.BroadcastPackageAsync returns true even if the child fails — the comment explains why (parent accepted = valid outcome), which is correct for CPFP, but callers should be aware that true does not mean both txs were accepted.

6. GrpcClientTransport.GetVtxoChainAsync pagination: the while loop condition response.Page.Next != response.Page.Total — verify this matches the arkd indexer API contract. If Next == Total means "no more pages", this is correct.

Security

• ✅ Unilateral exit path integrity — the UnilateralPath tapscript is correctly extracted from ArkPaymentContract/ArkBoardingContract and used with proper control block.
• ✅ CSV timelock enforcement — session only transitions to Claimable after chainTime.Height >= confirmHeight + csvDelay.
• ✅ Signing uses wallet provider — no hardcoded keys, proper descriptor-based signing via IWalletProvider.GetSignerAsync.
• ✅ Fee wallet is optional — CPFP is best-effort, falls back to direct broadcast.
• ⚠️ FeeCoin record exposes SigningKey (private key) — this is necessary for signing but ensure FeeCoin instances are never serialized/logged. The record's ToString() will include the key by default.
• ⚠️ Claim address derivation via DeriveClaimAddressAsync — the watchtower auto-starts exits and derives its own claim address. Verify this address is controlled by the wallet owner and not an attacker-controllable path.

Cross-Repo Impact

• This mirrors functionality that exists in the TS SDK (ts-sdk) and arkd (Go). The state machine design differs slightly (ts-sdk uses VtxoManager + inline renewal vs. this explicit session-based approach). Worth ensuring the exit path behavior is consistent across SDKs.
• The gRPC transport additions (GetVtxoChainAsync, GetVirtualTxsAsync, GetVtxoTreeAsync) depend on arkd indexer API — verify these endpoints exist in the arkd version being targeted.

Good foundational work for a critical safety feature. The unilateral exit guarantee is what makes Ark non-custodial — getting this right matters.

[arkanaai]

Review — New Commit bdaec075 (Address second round of feedback)

Reviewed by: Arkana (PR Reviewer Agent)

This commit addresses findings from the previous review round. All four changes are solid:

FeeCoin ToString override

SigningKey no longer leaks via default record ToString(). Closes the serialization/logging vector flagged in the prior review.

Transient error handling

HttpRequestException/TimeoutException/TaskCanceledException now log+retry on next poll instead of permanently failing sessions. Combined with MaxBroadcastRetries (10), gives robust error categorization.

Watchtower early branch check

HasBranchAsync moved into the loop before CheckVtxoAsync — skips RPC calls for VTXOs without stored exit data. Reduces unnecessary blockchain queries per poll cycle.

CPFP vsize correction

Two-pass build (estimate → build → measure → rebuild if >10 vbyte delta) prevents fee underpayment when estimated 155 vbyte child doesn't match reality.

Minor note: The corrected fee rate reconstruction creates FeeRate from total_fee / total_size, which yields the same sat/vB as the original rate — the rebuild really adjusts the change output based on true child vsize. Correct approach.

Verdict: LGTM ✅

All previous review concerns addressed. PR is in solid shape for merge. E2E and integration tests correctly deferred to follow-up PRs.

[Kukks]

@arkanaai what happens if we dont have enough onchain funds to complete unilateral exit? Is the unilateral exit system aware that only confirmed utxos can be used in a package? and that it has essential step and wait with every broadcast?

[arkanaai]

Three questions, addressing each:

1. No on-chain funds for CPFP

BroadcastWithCpfpAsync falls back to direct broadcast if SelectFeeUtxoAsync returns null:

var feeCoin = await feeWallet.SelectFeeUtxoAsync(estimatedTotalFee, ct);
if (feeCoin is null)
{
    logger?.LogWarning("No fee UTXO available for CPFP, falling back to direct broadcast");
    return await broadcaster.BroadcastAsync(tx, ct);
}

If the virtual tx's fee rate is below mempool minimum (likely for v3/TRUC txs that depend on package relay), this direct broadcast will fail. The retry counter increments until MaxBroadcastRetries (10) is hit, at which point the session is permanently Failed. Exit is stuck — no user alert, no recovery path.

This is a correctness concern: a user with no on-chain fee funds could silently fail their exit.

2. Confirmed UTXOs only

IFeeWallet documents this requirement:

"Selects a confirmed on-chain UTXO"

But it's only a documentation contract — there's no validation that SelectFeeUtxoAsync actually returns a confirmed UTXO. The package relay rules (1p1c) require the fee input to be confirmed; if an implementor returns an unconfirmed UTXO, submitpackage will fail at the node level. This should be enforced or at least explicitly documented as an implementor invariant.

3. Sequential confirmation between tree txs

Yes, ProgressBroadcastingAsync correctly handles this — it stops when a tx is in mempool and returns, resuming from NextTxIndex on the next poll cycle:

if (status.InMempool)
{
    await UpdateSession(session with { NextTxIndex = i, ... });
    return; // wait for confirmation before broadcasting next tx
}

Each tx in the tree spends the previous tx's output, so the InMempool → wait → confirmed → continue pattern is correct.

[Kukks]

@arkanaai iirc you cannot submit a tx that has a 0 amount p2a "directly", it is not standard policy and would be rejected i guess. What are the defaults with this PR? How do you provide onchain funds to fund the exits? Review deeply these nuances

[arkanaai]

You're correct. A v3 tx with a 0-value P2A anchor and zero/low fee will be rejected by mempool policy when broadcast directly — it's below min relay fee rate and non-standard without package relay context.

Defaults in this PR:

feeWallet is optional (nullable constructor param, defaults to null). When null, BroadcastWithCpfpAsync skips CPFP and calls broadcaster.BroadcastAsync(tx) directly:

if (anchor is null || feeWallet is null)
    return await broadcaster.BroadcastAsync(tx, ct);

So by default — no fee wallet configured — every virtual tx broadcast attempt will hit the node, get rejected (below min relay fee / non-standard), return false, increment RetryCount, and eventually hit MaxBroadcastRetries (10) → Failed session. The unilateral exit silently never happens.

How to provide on-chain funds:

Callers must implement IFeeWallet and inject it:

new UnilateralExitService(
    transport, broadcaster, exitSessionStorage, virtualTxStorage,
    virtualTxService, contractStorage, chainTime,
    feeWallet: myFeeWalletImpl  // ← caller provides this
);

No default implementation is included in the PR — the doc comment points to BTCPay (use store wallet) or standalone apps (dedicated fee key). Neither is implemented here.

The problem: IFeeWallet being optional with a silent fallback to direct broadcast is dangerous for this use case. For a safety-critical exit path, "no fee wallet" should arguably throw at registration time or at least warn loudly that exits will fail, not silently retry to death.

[arkanaai]

🔍 Arkana Review — New Commits ae5354d0..9038a8da (E2E tests)

Four commits adding E2E coverage for the unilateral exit pipeline. Tests themselves are well-structured — good state-machine assertions, proper timeout handling via [CancelAfter], idempotency check, CSV delay maturation check. The iterative debugging across commits (c7d9e99a → 6e20c8f4 → 9038a8da) is transparent and the final state works.

However, the test iteration uncovered a production bug that needs to be fixed in this PR or tracked explicitly.

---

🔴 Production Bug: PostBatchVtxoPollingHandler misses VTXOs on inactive contracts

File: NArk.Core/Events/PostBatchVtxoPollingHandler.cs:50

var contracts = await contractStorage.GetContracts(
    walletIds: [walletId],
    isActive: true,   // ← filters out inactive contracts
    cancellationToken: cancellationToken);

Commit 9038a8da's own message documents the problem clearly:

"SimpleIntentScheduler derives the SendToSelf output contract as Inactive [...] so the SDK's PostBatchVtxoPollingHandler — which filters isActive=true — was skipping the script the new VTXO actually lands on."

The test works around this by replacing the handler entirely with an InlineEventHandler that polls ALL contracts. But the production code still has this bug — any user of the SDK using the standard DI registration (AddArkServices) will hit the same issue: after a batch, the new offchain VTXO won't be detected by the post-batch handler, and the user's balance won't update until the next routine poll cycle (or never, if the contract stays inactive).

This is not just a cosmetic issue — if the VTXO isn't detected, it can't be spent or exited, and could be silently lost to a round expiry.

Fix: Either:

• (a) Remove the isActive: true filter in PostBatchVtxoPollingHandler (poll all wallet contracts), or
• (b) Have SimpleIntentScheduler derive the SendToSelf contract as Active, or
• (c) Pass both active and newly-derived contract scripts from the batch event itself

🟡 Tests bypass the production handler — production path untested

Because the inline handler replaces PostBatchVtxoPollingHandler, the actual production code path (PostBatchVtxoPollingHandler.HandleAsync) is never exercised in E2E. Once the above bug is fixed, the E2E should use the real handler to validate the fix.

🟡 Kukks' open questions not addressed in code

The maintainer raised two protocol-level concerns in the PR comments:

1. 0-value P2A direct broadcast rejection: On non-regtest networks, feeWallet: null (the default) means every exit silently retries to death. This is documented in my earlier review and your response, but no code change was made (e.g., throwing at registration, logging a startup warning, or defaulting IFeeWallet to required).

2. Confirmed UTXO enforcement: IFeeWallet.SelectFeeUtxoAsync documents "confirmed" but doesn't enforce it. An implementor returning an unconfirmed UTXO will cause submitpackage to fail at the node.

These should either be addressed in code or explicitly tracked as follow-up issues with links.

✅ What's good

• Test structure: SettleAVtxoAsync helper is clean — boards, settles, wires exit deps, returns a disposable. Good pattern for other E2E suites to follow.
• State machine coverage: Broadcasting → AwaitingCsvDelay transition, CSV delay enforcement (no premature advance), idempotent start — these are the right tests to write.
• Timeout discipline: [CancelAfter(180_000)] / [CancelAfter(240_000)] with CancellationToken threading through all async calls. Proper.
• Event-driven VTXO detection: Using VtxosChanged event + TCS instead of polling is the right approach. The triple-TCS synchronization (newSuccessBatch → batchPolledTcs → settledVtxoTcs) correctly sequences the async pipeline.
• Diagnostic output: TestContext.WriteLine with state/retry/fail info on each step — will make CI failures debuggable.

Minor

• UnilateralExitTests.cs:327: The 500ms delay comment says "enough on regtest" — consider making this configurable or using the same BatchSuccessPollingDelay option that the production handler uses, so the test mirrors prod behavior.
• ExitTestSetup doesn't dispose the ServiceProvider — minor leak in test context, but sp (the built ServiceProvider) is never disposed. The IAsyncDisposable[] only covers the background services.

---

Verdict: Request changes — the isActive: true filter bug in PostBatchVtxoPollingHandler is a real production issue exposed by the test iteration. Fix it (or explicitly track it) before merge. The E2E tests are otherwise solid work.

— Arkana 🔍

[arkanaai]

🔍 Arkana Review — New Commits 9038a8da..0ee7084e

Eight commits: PSBT parsing fix, whole-chain-with-types storage, opt-in auto-fetch refactor, commitment-row skip in broadcasting, E2E test hardening. All previous findings addressed. Approving with minor notes below.

---

✅ PSBT Parsing Fix — Correct (5b916fec, 5f1d1c0d)

ParseVirtualTx now parses as PSBT and lifts FinalScriptWitness/FinalScriptSig directly — the right approach given arkd's encoding. The null-guard on each PSBT input field is correct defensive coding.

✅ Whole-Chain Storage with ChainedTxType — Clean (2dad1633)

Storing the full chain including the commitment anchor is the right call — consumers can walk back to the on-chain root without re-querying the indexer. The type enum in NArk.Abstractions (not Transport.Models) is correctly placed for cross-layer access.

✅ Commitment Skip in Broadcasting (06cad189)

Both StartExitAsync (hex-null check) and ProgressBroadcastingAsync (loop skip) correctly handle commitment rows. The Unspecified type falls through to the strict path (treated as needing hex) — fail-loud is the right default for a protocol-critical flow.

✅ VtxoChainAutoFetchService — Well-Designed (2dad1633)

The move from event-handler-with-retry-loop to Channel<T>-backed background service is a significant improvement. SingleReader = true is correct for the sequential processing pattern. UnboundedChannel is acceptable here — VTXO arrival rate is naturally bounded by batch frequency.

---

🟡 Minor: EnsureHexPopulatedAsync Omits Type in Update Records

VirtualTxService.cs ~line 145:

updates.Add(new VirtualTx(txids[i], hexList[i], missingHex[i].ExpiresAt));

This defaults Type to Unspecified. The upsert logic in EfCoreVirtualTxStorage guards against overwriting with Unspecified, so this won't clobber existing types today. But it's fragile — if the upsert guard is ever changed, this silently resets types. Should be:

updates.Add(new VirtualTx(txids[i], hexList[i], missingHex[i].ExpiresAt, missingHex[i].Type));

🟡 Missing EF Core Migration for Type Column

VirtualTxEntity gained a Type property with HasConversion<int>() and HasDefaultValue(ChainedTxType.Unspecified). No migration was added. Consumers using relational DBs will hit a runtime column-not-found error until they generate one. At minimum, document this as a required migration step in the PR description, or ship the migration.

🟡 Broadcasting Blockers Are Tracked?

The [Ignore] annotations on two E2E tests document two distinct arkd-side issues:

1. Tree-tx PSBTs from GetVirtualTxs lack FinalScriptWitness → empty witnesses → mempool rejection
2. First tree-tx is v3 (TRUC) but parent is non-v3 → TRUC-violation

These block the entire broadcasting path on non-regtest. Are these tracked as arkd issues? Without a fix on the server side, unilateral exit is inoperable end-to-end.

---

Summary

The new commits are solid. The PSBT parsing fix is a real correctness improvement, whole-chain storage is the right architecture, and the opt-in auto-fetch is cleaner than the old event handler. The two minor code items (Type propagation, missing migration) are low-risk. The broadcasting blockers are server-side and correctly deferred.

Approve — pending human sign-off on the protocol-critical exit path (this is an L2 unilateral exit pipeline).

— Arkana 🔍

[arkanaai]

🔍 Arkana Review — New Commit 0ee7084e..2494bd6f

— Arkana 🔍

🚨 CRITICAL: Wrong WitScript constructor — malformed witness (protocol-critical)

NArk.Core/Services/UnilateralExitService.cs:656

tx.Inputs[i].WitScript = new WitScript(sig.ToBytes());

This uses the WitScript(byte[]) constructor which interprets the bytes as serialized witness data (varint count + length-prefixed stack items), NOT as a single witness stack push.

Every other taproot key-path spend in the codebase correctly uses:

new WitScript(new[] { sig.ToBytes() }, true)

Evidence:

• P2ACpfpBuilder.cs:94 — new WitScript(new[] { sig.ToBytes() }, true)
• ChainSwapMusigSession.cs:104,212 — new WitScript(new[] { finalSig.ToBytes() }, true)
• UnilateralExitService.cs:493-495 (claim tx, same file!) — new WitScript([sig.ToBytes(), ...], true)

Fix:

tx.Inputs[i].WitScript = new WitScript(new[] { sig.ToBytes() }, true);

Without this fix, the tree-tx witness will be malformed and Bitcoin Core will reject the broadcast with a script verification failure. This is the exit path for user funds — if it doesn't work, users cannot unilaterally exit.

---

⚠️ Tests un-ignored but TRUC-violation still unresolved

The XML doc on ProgressExits_AdvancesFromBroadcastingToAwaitingCsvDelay (line 145-148) still documents a known TRUC-violation issue:

The first tree-tx is v3 (TRUC) but its parent is non-v3, tripping TRUC-violation.

The [Ignore] attribute was removed, but this commit only fixes the witness assembly — it does NOT address the v3/TRUC relay policy issue. Either:

1. The TRUC issue was already fixed elsewhere (if so, remove the stale XML doc), or
2. These tests will still fail on Bitcoin Core 28+ nodes with TRUC enforcement, in which case keep [Ignore] until the tree-tx version or commitment-tx version is resolved upstream.

Which is it? The test remarks are now contradictory with the test being enabled.

---

Summary

The witness fix approach is correct in spirit (lifting TaprootKeySignature for Tree txs matches the ts-sdk reference at unroll.ts:183-189), but the wrong constructor makes it produce garbage on the wire. One-line fix, then this is good.

Reviewed by: Arkana (PR Reviewer Agent)

[arkanaai]

🔍 Arkana Review — New Commit 0ee7084e..2494bd6f

— Arkana 🔍

🚨 CRITICAL: Wrong WitScript constructor — malformed witness (protocol-critical)

NArk.Core/Services/UnilateralExitService.cs:656

tx.Inputs[i].WitScript = new WitScript(sig.ToBytes());

This uses the WitScript(byte[]) constructor which interprets the bytes as serialized witness data (varint count + length-prefixed stack items), NOT as a single witness stack push.

Every other taproot key-path spend in the codebase correctly uses:

new WitScript(new[] { sig.ToBytes() }, true)

Evidence:

• P2ACpfpBuilder.cs:94 — new WitScript(new[] { sig.ToBytes() }, true)
• ChainSwapMusigSession.cs:104,212 — new WitScript(new[] { finalSig.ToBytes() }, true)
• UnilateralExitService.cs:493-495 (claim tx, same file!) — new WitScript([sig.ToBytes(), ...], true)

Fix:

tx.Inputs[i].WitScript = new WitScript(new[] { sig.ToBytes() }, true);

Without this fix, the tree-tx witness will be malformed and Bitcoin Core will reject the broadcast with a script verification failure. This is the exit path for user funds — if it doesn't work, users cannot unilaterally exit.

---

⚠️ Tests un-ignored but TRUC-violation still unresolved

The XML doc on ProgressExits_AdvancesFromBroadcastingToAwaitingCsvDelay (line 145-148) still documents a known TRUC-violation issue:

The first tree-tx is v3 (TRUC) but its parent is non-v3, tripping TRUC-violation.

The [Ignore] attribute was removed, but this commit only fixes the witness assembly — it does NOT address the v3/TRUC relay policy issue. Either:

1. The TRUC issue was already fixed elsewhere (if so, remove the stale XML doc), or
2. These tests will still fail on Bitcoin Core 28+ nodes with TRUC enforcement, in which case keep [Ignore] until the tree-tx version or commitment-tx version is resolved upstream.

Which is it? The test remarks are now contradictory with the test being enabled.

---

Summary

The witness fix approach is correct in spirit (lifting TaprootKeySignature for Tree txs matches the ts-sdk reference at unroll.ts:183-189), but the wrong constructor makes it produce garbage on the wire. One-line fix, then this is good.

Reviewed by: Arkana (PR Reviewer Agent)

[Kukks]

Pushed two commits to address the rebase + opt-in storage concern.

82e1769 — Merge origin/master into feat/unilateral-exit. Resolves the two transport conflicts that PR #90 introduced (additive-on-additive: both PRs added methods to IClientTransport and CachingClientTransport). Order kept as: GetPendingTx* (PR #90) → GetVtxoChain* / GetVirtualTxs* / GetVtxoTree* (this PR). Also adds the three passthroughs to CrashAfterSubmitTransport in PendingArkTransactionRecoveryTests so the test decorator still compiles against the wider interface.

70a4f6c — Two correctness/opt-in fixes:

1. EF entity registration is now opt-in. Previously ConfigureArkEntities unconditionally registered VirtualTxEntity / VtxoBranchEntity / ExitSessionEntity, so every EF Core consumer got three exit tables in their migration even if they never called AddUnilateralExit. Moved them into a new ConfigureArkExitEntities() extension matching the pattern PR feat: payment tracking improvements #75 set with ConfigureArkPaymentEntities(). The sample wallet's WalletDbContext was updated to call it so its behaviour is unchanged.

2. VirtualTxOptions.DefaultMode now defaults to Lite (was Full). Full triggers two arkd round-trips per VTXO arrival — GetVtxoChain plus GetVirtualTxs for every off-chain row — which scales with chain depth and dominates wallet throughput on a busy receive path. Most VTXOs never exit unilaterally; Lite stores txids + expiry only and defers the hex fetch until UnilateralExitService.StartExitAsync is actually called. Callers with strict offline-exit requirements (e.g. must broadcast without an indexer reachable) can still set Full via configureVirtualTx.

361/361 unit tests pass locally. CI in flight.

[arkanaai]

🔍 Arkana Review — New Commits b6dda81f..32cc19e6

— Arkana 🔍

Reviewed 3 non-merge commits since last review:

✅ eda81d2e — fix(test): decode UnilateralExit Sequence properly in CSV-delay test

Correct fix. BIP68 time-based sequences (bit 22 set) produce raw .Value in the millions — mining that many regtest blocks was wrecking bitcoind/LND/Boltz for the rest of the suite. Now gates on LockType: height-based mines LockHeight+2, time-based early-returns after validating the don't-advance-early assertion. Pragmatic; the time-based CSV maturation via setmocktime+MTP is correctly deferred as separate work.

✅ 70a4f6cc — fix(exit): make EF entity registration opt-in + default to Lite mode

Two good changes:

1. Opt-in ConfigureArkExitEntities() — follows the existing ConfigureArkPaymentEntities pattern exactly. Consumers who never call AddUnilateralExit no longer get three unwanted migration tables. Clean.

2. Default VirtualTxMode.Lite — correct default. Full mode does 2 arkd round-trips per VTXO arrival and most VTXOs never exit unilaterally. Lite defers hex fetch to StartExitAsync. Docs clearly explain the trade-off and the opt-in path for offline-exit requirements.

Minor note (non-blocking): All three Configure* methods instantiate independent ArkStorageOptions. If a consumer passes configure: o => o.Schema = "foo" to ConfigureArkEntities but forgets to pass it to ConfigureArkExitEntities, exit tables land in the default schema while core tables are in "foo". This is a pre-existing pattern from ConfigureArkPaymentEntities (not a regression), but worth a doc note or a future refactor to a shared options builder.

✅ 32cc19e6 — fix(tests): TestDbContext opts in to exit entities

Trivially correct — mirrors the sample wallet's WalletDbContext change from the prior commit.

Verdict

All three commits are clean. No protocol-critical concerns. No security issues. No cross-repo breakage (the ConfigureArkExitEntities is additive and opt-in). LGTM.