Conversation
3126187 to
a58c3fd
Compare
|
Hey guys, it seems that rgb-lib intentionally does not support arbitrary script receives as an architectural boundary — or at least that’s what I gathered from the context back in February. See the second page of this file for more details: An escrow solution was implemented in txalkan/rgb-lightning-node@157190a — with support from these rgb-lib PRs: UTEXO-Protocol/rgb-lib-old#4 & UTEXO-Protocol/rgb-lib-old#5 |
|
@txalkan is right here: |
CI failed because security_gaps/shared_cosigner were untracked and inspect_rgb_transfer had an unclosed function. Co-authored-by: Cursor <cursoragent@cursor.com>
Review: HTLC preparationExposing Blocking - build/lint failures 1. 2. Once (1) is fixed, the new helpers in 3. Debris in 4. Correctness / safety 5. 6. An externally built PSBT loses its input metadata. 7. Reachable panic across the FFI boundary. Same line - 8. 9. Wrong error variant for a bad asset id. Quality 10. 11. Same in the bindings: you added 12. 13. Scope 14. The description leads with 15. The multisig security-gap helpers and Test coverage 16. None of the new entry points has a test - not Summary The FFI plumbing itself is straightforward. (1)-(4) are blocking - the branch does not build or lint. The ones I would most want resolved before merge are (5) and (6): both concern |
Co-authored-by: Cursor <cursoragent@cursor.com>
Parties only hold references, so end their borrows with a block before dropping wallets. Co-authored-by: Cursor <cursoragent@cursor.com>
🌗 Pull Request OverviewThis PR introduces HTLC (submarine swap) preparation features for RGB asset flows. It adds new Rust-only wallet methods for coloring PSBTs with explicit outpoint sets, fetching/accepting RGB consignments by recipient ID, and inspecting contract assignments on arbitrary outpoints. It also adds UniFFI bindings for these methods and includes regression tests documenting known multisig PSBT validation gaps and shared-cosigner wallet isolation. Reviewed Changes Show a summary per file
📋 Review Findings📄
|
|
@mirvaisdia-nitka I still have my comments unaddressed and AI report as well |
c8bc87f to
4c8fd5d
Compare
Jainakin
left a comment
There was a problem hiding this comment.
I re-reviewed the current head 4c8fd5d6, all changed files, every existing review thread, and the related receive, accounting, backup and persistence paths.
The PSBT map preservation, omitted-allocation rejection, witness pinning, path validation, broadcast-before-apply guard and durable marker are meaningful improvements. I am still requesting changes because the operation is not crash-consistent across RGB stock, operation files, SQLite and VSS.
The blocking issues are:
- Stock is persisted and the runtime lock released before recovery evidence is written.
- Required payload files are not durably published or integrity-bound.
- Abort and bulk expiry can split SQL, metadata and VSS state.
- The required
witness_receiveaccounting path is not implemented or tested end to end. - External broadcasting leaves an ambiguous rollback window.
- A committed operation cannot be adopted after response loss.
- The new UniFFI path exposes a caller-triggerable UDA panic.
Before merge, the PR also needs Windows-safe payload paths, fail-closed handling for missing PSBTs, operation retention/pruning, removal of serial indexer calls from bulk failure, and explicit documentation of the public API compatibility changes.
Local formatting, Clippy, no-default-feature tests and UniFFI tests pass, and the GitHub matrix is green. I also built a targeted witness_receive integration test, but the repository Docker harness stalled while starting services, so that end-to-end path remains unverified.
Given the size and upstream design guidance, I recommend separating PSBT safety primitives, the durable HTLC lifecycle, and bindings into independently reviewable changes.
| Ok(()) | ||
| } | ||
|
|
||
| /// Abort a prepared HTLC operation before apply (tx never broadcast). |
There was a problem hiding this comment.
Never broadcast cannot be established from a single current indexer lookup. Broadcast response loss, indexer lag, mempool eviction or later rebroadcast can make the transaction appear absent during abort and visible afterward, after inputs have already been credited back. Please persist an explicit broadcast lifecycle such as NotAttempted, Attempted, Observed and Ambiguous, and prohibit automatic rollback for attempted or ambiguous operations. Alternatively, make broadcasting part of the library-owned operation.
There was a problem hiding this comment.
Agreed — one indexer lookup can’t prove the tx was never broadcast. Lag, lost broadcast response, mempool drop, or a later rebroadcast can make it missing at abort and show up after we’ve already rolled the inputs back.
Broadcast stays with the caller in this PR; htlc_abort is only for the “we never sent it” case, and that’s a caller promise, not something the library can verify yet.
Follow-up: either an explicit broadcast lifecycle (NotAttempted / Attempted / Observed / Ambiguous) and no automatic rollback once it’s been attempted or is ambiguous, or the library owns broadcast. Not in this PR.
There was a problem hiding this comment.
Review update at 0f30018: Still open, with a narrower requested fix. I accept the documented never-broadcast caller precondition for explicit abort and am not asking for a general broadcast journal here. Automatic expiry is different: an externally broadcast transaction may still have a Prepared/Initiated operation until apply completes. Expiry plus a temporary indexer not-found can fail/release it, after which apply rejects the Failed state when the transaction becomes visible. Please prevent automatic failure/release of these externally managed prepared operations based solely on expiry and current transaction absence. Making the expiry explicit does not establish that broadcasting never occurred.
Consolidated scope correction.
Earlier assessment at b213f76 (superseded where noted above)
Thanks for making the caller-promise limitation explicit. There is still an automatic path that bypasses that promise at b213f76: psbt_op_prepare defaults to a 24-hour expiration, and the bulk expiry sweep fails an Initiated prepare batch when get_tx_confirmations returns None. It has no durable evidence that the caller never attempted broadcast. A transaction can have been broadcast, become temporarily absent, then reappear after the batch was failed and allocations released; apply then refuses the Failed batch.
Indexer errors now stay fail-closed, which is good, but a successful 'not found' response is not the same as NotAttempted. Please introduce durable pre-broadcast handoff/attempt evidence and quarantine ambiguous attempts, or remove automatic expiry/release for externally broadcast operations until that lifecycle exists. Cover response loss, indexer lag and eviction/reappearance. Keeping broadcast with the caller is fine; interpreting absence as proof of never-broadcast is the unsafe part.
58fa4eb to
7aa2540
Compare
7aa2540 to
fa6782b
Compare
|
|
I completed the requested review of the separated follow-up in #94 at exact head #94 does close several findings from my earlier review: required payloads are hash-bound and durably replaced, consignment paths are Windows-safe, missing PSBTs fail closed, the UDA panic is now a typed error, and the foreign-escrow The crash-consistency review on this PR must remain open, however. The separated patch still allows generic The exact-head formatting and no-default-features Clippy gates currently fail, and no GitHub checks are registered for #94's stacked base. I left the actionable findings on #94 so this already-large PR does not accumulate another implementation layer. |
The prepare/apply/abort lifecycle is not HTLC-specific: any externally built PSBT (timelock escrows, other scripts) goes through it. Rename the public API, FFI surface, on-disk layout (psbt_ops/, foreign_inputs.json) and tests. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Without an expiration a prepared operation could keep its inputs reserved forever; apply PSBT_OP_DEFAULT_EXPIRATION_SECS when the caller passes none. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
28aca7c to
b213f76
Compare
There was a problem hiding this comment.
Scope correction at 0f30018: the merge requirements below have been narrowed after comparison with the UTEXO base and RGB-Tools upstream. See the updated assessment. Fixed findings are acknowledged there; deferred recovery work is not a blanket blocker for #65. The remaining change request concerns automatic expiry of externally broadcast prepared operations.
Historical review at b213f76 (superseded where noted above)
Re-reviewed at b213f76, including the update pushed during this review, the earlier human/AI reviews, and the separate recovery work in #94. I cannot approve this head for production yet.
There are real improvements here: foreign inputs are no longer invented as wallet TXOs; the normal witness_receive + refresh path has a single accounting owner; missing persisted PSBTs fail closed; the UDA unwrap is replaced with a typed error; legacy output-index semantics and PSBT maps are covered; and the latest update makes repeat apply succeed and validates native SegWit inputs and the recipient's colored keychain. I am not repeating those as unresolved implementation defects.
Remaining correctness/recovery gates
- Stock consumption, consumed markers, SQL and operation metadata still do not form a recoverable persistence boundary. The injected failure occurs after both markers, not between stock persistence and marker publication.
- An absent indexer transaction still permits releasing externally broadcast inputs. The default 24-hour expiration makes this an automatic path, not merely an explicit caller promise that broadcasting was never attempted.
- Required payloads are ordinary mutable writes without a versioned, integrity-bound manifest. Durable metadata is not sufficient to make the payloads durable or mutually consistent.
- Aborting now repairs an already-failed batch on an explicit retry, but reconciliation still returns Prepared for Prepared metadata paired with Failed SQL. Bulk expiry and the intermediate auto-backup snapshot can produce this state.
- A successful prepare whose response is lost still has no discover/adopt API. In the string-based UniFFI call the caller may never receive the colored PSBT or its final txid, so lookup by that final txid alone is not a complete recovery contract.
- Rejected duplicate preparation changes existing transfer artifacts, including the marker that decides which consume API owns the operation. See the new inline reproduction.
- The documented receive helper imports into RGB stock but does not itself finish the ordinary witness_receive SQL/balance/history lifecycle. The new accounting test deliberately uses refresh instead of this helper; the supported caller flow needs to be explicit and tested.
- Per-asset filenames still contain the colon from the RGB contract ID and fail on Windows.
The separate scope of #94 is reasonable, but it is still open and targets htlc_preparation; its changes are not in this head. These are therefore unresolved production gates for the combined feature, not fixes already delivered by #65. Please either stack the required corrections into the release candidate or keep this API explicitly unavailable for production until the dependent recovery work is integrated and tested. I am not asking to fold unrelated recovery architecture into this diff.
Verification
- Current-head no-default-feature library tests: 56 passed.
- Current-head UniFFI library tests: 5 passed; these are conversion/validation tests, not a generated-Go end-to-end test.
- Formatting check passed.
- Four isolated characterization probes reproduced the current behavior using disposable wallets: stale Failed/Prepared reconciliation, duplicate-prepare marker mutation, abort accepting an invalid Online handle when no linked batch exists, and malformed native Rust PSBT output maps panicking. The first two are directly relevant to recovery. The last two are smaller API-hardening items; the malformed-struct panic is not a claim that malformed serialized input gets through UniFFI's PSBT parser.
- The duplicate-write probe uses the real persistence helper/SQLite/filesystem with a minimal fascia fixture. It is not presented as a funded HTLC round trip.
- No new full Docker/Go/OS-kill/Windows runtime test was run locally in this review. Current-head GitHub integration CI was still running when checked; passing build jobs must not be treated as proof of these failure boundaries.
The inline comments and replies distinguish fresh findings from earlier threads that remain unresolved, and include the specific regression cases needed. No production files or dependency pins were changed for this review.
99e9dbf to
0f30018
Compare
Review scope correction at
|
Resolve UDL conflict (keep asset_schema_id and script_hex_from_recipient_id) and adapt pin_witness_output_to_recipient_id to the new ResolveWitness::resolve_witness(&PubWitness) signature. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
We needed to close the RGB HTLC flow.
The lock is a foreign HTLC UTXO, not an rgb-lib receive. witness_receive is only the claim/refund destination — a normal wallet-owned output on the colored keychain. rgb-lib does not register the HTLC script as a receive; RGB on the lock is inspected with contract_assignments_for_outpoints. After the claim, the receiver imports onto the witness_receive output.
The claim/refund PSBT is built externally (native SegWit inputs, unsigned). Coloring must happen in rgb-lib, but consume_fascia is not reversible, so this is two-phase: psbt_op_prepare colors the PSBT, writes the fascia and the per-asset consignments under operation_dir and registers an Initiated batch with the witness txid and an expiration (stash unchanged); after broadcast, once the indexer sees the tx, psbt_op_apply consumes the fascia and moves the batch to WaitingConfirmations. If the tx never lands, psbt_op_abort, or the expiry sweep fails the batch. psbt_op_reconcile reports the operation status. fail_transfers refuses to release the inputs of a prepare batch whose tx the indexer already knows; send_begin batches are unaffected.
Upload the consignment from operation_dir/consignments/ to the RGB proxy; the receiver imports with fetch_and_accept_transfer_by_recipient_id (pins output[vout] to the witness script, checks confirmations, compares the received assignment with ExpectedTransfer).
These were rust-only and unreachable from Go. Uniffi/UDL now exports psbt_op_prepare / psbt_op_apply / psbt_op_abort / psbt_op_reconcile, contract_assignments_for_outpoints, fetch_and_accept_transfer_by_recipient_id and script_hex_from_recipient_id.
issue
PR and solution are based on @txalkan PRs #4 & #5, adapted to prepare → broadcast → apply instead of coloring and consuming fascia in one call.