Repository navigation
Treat a spend we could not read as unknown, not as a refund - #214
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 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 |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Review — #214
The fix is correct and the bug description matches the code. Detailed notes below.
Protocol correctness ✓
The three-state PreimageEvidence enum is the right model. The pre-fix code had one conflation:
SpendIsVisibleAsyncreturned true →sawASpend = true→FindAsyncreturned null (either parse failure or genuine no-preimage) → verdictReturned
Those two null paths are not equivalent. A PSBT that could not be parsed (witness not yet published, or indexer serving a partial blob) looks identical to a genuine refund witness at the old call site. The new code only advances to Returned when evidence == Absent, which requires at least one Candidates iteration to have run — i.e., a witness was actually there and walked.
The readASpend flag is also now set after the evidence check, not before LookupAsync is called (as sawASpend was). Under the old code, sawASpend was set as soon as SpendIsVisibleAsync returned true, so a FindAsync exception on the second fetch would still have produced Returned. The new code is strictly safer.
Tests ✓
SpentWithNoProvenPreimage_IsReturned (LockupFateReaderTests.cs:73) — correctly migrated to spendPsbt: ReadableSpendOf(). Under the new semantics the old "not-a-psbt" blob makes LookupAsync return Unreadable, which now maps to Unknown, not Returned. The test had to be updated, and it was.
ASpendWhoseWitnessCannotBeReadYet_IsUnknownRatherThanReturned (LockupFateReaderTests.cs:131) — exercises exactly the bug scenario: visible transaction, unreadable PSBT ("not-a-psbt"), expected verdict Unknown.
ReadableSpendOf (LockupFateReaderTests.cs:144) — the constructed PSBT has PrevOut = (bbbb…, 0) which matches Vtxo().TransactionId/TransactionOutputIndex, so the input.PrevOut != lockup guard passes. The FinalScriptWitness carries a single 32-byte zero push; SHA256(0x00…00) ≠ "0000…0000", so Matches returns false and evidence is Absent. Construction is correct.
InternalsVisibleTo is already wired for NArk.Tests (.csproj line 30), so LookupAsync and PreimageEvidence are reachable for direct unit tests if desired later.
Doc comment misplacement — nit
LockupFateReaderTests.cs, the block added above ASpendWhoseWitnessCannotBeReadYet_IsUnknownRatherThanReturned:
/// <summary>A PSBT spending the fixture's lockup, carrying a witness that proves nothing.</summary>
// The bug this guards: …
[Test]
public async Task ASpendWhoseWitnessCannotBeReadYet_IsUnknownRatherThanReturned()The <summary> describes ReadableSpendOf, not the test method. ReadableSpendOf itself has no doc comment. The summary on the test is also slightly misleading — the test's point is an unreadable PSBT, not "a witness that proves nothing." The // comment below is the real explanation. Suggest moving the <summary> to ReadableSpendOf and dropping it from the test, or replacing it with a summary that matches the test's intent.
Pre-existing items (not introduced here, worth tracking)
Double-fetch per VTXO (LockupFate.cs:157–163): SpendIsVisibleAsync calls GetVirtualTxsAsync([txid]) and then LookupAsync calls it again for the same txid. Two network round-trips per VTXO in the spend loop. Not a correctness problem, but a follow-up optimisation (LookupAsync could accept the already-fetched list, or SpendIsVisibleAsync could be folded into it).
No Claimed integration test in LockupFateReaderTests: The suite has no test that threads a valid preimage all the way through LockupFateReader.ReadAsync and asserts LockupFate.Claimed. The lower-level SwapPreimageReaderTests covers the preimage-found path, so this is not a blind spot for the reader itself, but end-to-end coverage through the fate reader would be good to add.
Cross-repo
LookupAsync and PreimageEvidence are internal. FindAsync signature is unchanged. No impact on consumers in other SDK repos.
Bottom line: the logic fix is correct, the critical test update is correct, and coverage of the new Unknown path is in place. The doc comment placement is a minor cleanup item. No protocol concerns.
…t tests arkade-os#214 made LockupFateReader call a lockup Returned only when the spend carried a witness to inspect; a spend with none reads as Unknown. The SpendOf helpers built a bare PSBT, so five tests stopped seeing a refund.
* fix: remove ECXonly comparer and sweepDestination from tests merged from an old pr * test: give a preimage-less spend a readable witness in the swap intent tests #214 made LockupFateReader call a lockup Returned only when the spend carried a witness to inspect; a spend with none reads as Unknown. The SpendOf helpers built a bare PSBT, so five tests stopped seeing a refund.
3 participants
A claim whose witness had not been published yet read exactly like a refund: the transaction was visible, the preimage search found nothing in it, and
Returnedis terminal — so a Lightning payout was reported failed after the payee had been paid.Returnednow needs a witness it could actually read; anything else isUnknownand gets re-read next pass. Confirmed by replaying the real checkpoint from that swap: the reader finds the preimage in it.Note for review: the caller that turns this verdict into a swap status,
SwapSpendVerdict, lands in #215. This is correct on its own, but its effect on a payout's reported state only shows once that merges.