Conversation
Jainakin
left a comment
Jainakin
left a comment
There was a problem hiding this comment.
I reviewed the exact head 201f3e00 against htlc_preparation and rechecked it against the production blockers raised on #65.
This PR contains meaningful improvements: payloads are hash-bound and durably replaced, consignment paths are Windows-safe, missing PSBTs fail closed, the UDA panic is converted to a typed error, and the focused witness_receive accounting path now has end-to-end coverage. The 18 focused HTLC tests pass locally.
I am still requesting changes because the recovery protocol remains unsafe at several externally observable boundaries:
- Generic
fail_transferscan still fail an HTLC after broadcast was markedAttempted. I reproduced this deterministically by addingmark_broadcast -> fail_transfers_singleto the existing broadcast test;fail_transfers_singlereturnedtrueand failed the batch. - The new broadcast/adoption APIs are Rust-only. The Go/UniFFI caller can invoke
htlc_abort, but cannot durably mark a broadcast attempt or adopt the operation. - Txid lookup does not recover a lost
htlc_prepareresponse, because the caller does not know the colored txid when that response is lost. A caller-known idempotency/operation key is required. - The marker ordering still depends on a non-atomic, non-fsynced three-file RGB stock write; it is not evidence that the preceding stock generation is durably coherent.
- A crash between publishing
Preparedmetadata and committing the SQL batch leaves an orphan operation that cannot be completed and can make later txid adoption ambiguous. - Corrupt operation metadata is silently omitted during scans, turning corruption into false
not foundresults and allowing SQL failure to proceed without coherent healing. - Broadcast/reconcile metadata transitions are not acknowledged by VSS. Device loss can restore stale
NotAttemptedevidence after a real broadcast.
The operation store also has no retention/pruning policy, and bulk failure still performs serial indexer lookups. Concurrent mark/abort/apply/fail operations need explicit serialization or compare-and-set semantics.
Validation on the exact head:
- focused HTLC tests: 18/18 passed;
- injected broadcast/fail regression: failed at the intended safety assertion;
- all-features Clippy: passed;
- no-default-features Clippy: failed on the unconditional
Hash/sha256import; - repository rustfmt check: failed;
- GitHub checks: none are registered for this stacked target branch.
Please keep #65's crash-consistency review open until these boundaries are closed and covered by deterministic crash/concurrency tests.
| if !runtime.stash_contains_fascia_witness(&fascia) { | ||
| runtime.consume_fascia(fascia, None)?; | ||
| } | ||
| runtime.persist()?; |
There was a problem hiding this comment.
Blocking: this does not yet establish the durable ordering claimed by the helper. RgbRuntime::persist() delegates to rgb-ops Stock::store(), which writes stash, state, and index as three independent stores (the dependency itself has a "revert on failure" TODO). Those files are not committed as one atomic generation, and the stock path does not provide the fsync/rename barrier that the marker path provides. A process kill or I/O failure inside any one store can therefore leave partial stock; the test hook runs only after all three stores have returned. Please introduce a recoverable/atomic stock-generation protocol, or a journal that can deterministically repair every partial-store boundary, and cover the individual persistence boundaries with process-kill tests before treating the marker as proof of durable stock consumption.
There was a problem hiding this comment.
Agreed: persist() is Stock::store() — three independent files, no atomic generation, upstream TODO: Revert on failure. The marker only means persist() returned Ok. I’ll spell that out in the helper rustdoc.
An atomic stock protocol and process-kill tests at each stash/state/index boundary belong in rgb-ops, not in this HTLC helper. The same store() path is already used by send/accept/issue; rgb-lib cannot inject faults between those writes without forking the dependency.
This PR covers the rgb-lib window: persist under the runtime lock, fsync the marker, skip a second consume if the stash already has the witness. Partial stock inside store() is the existing rgb-ops TODO — we’d track that upstream rather than block claim/refund on it.
| if Self::validate_htlc_operation_id(&name).is_err() { | ||
| continue; | ||
| } | ||
| if let Ok(meta) = self.htlc_read_meta(&name) { |
There was a problem hiding this comment.
Fail closed on recognized operation-directory corruption. This silently drops every htlc_read_meta error. As a result, a corrupt or partially durable meta.json becomes indistinguishable from "operation not found"; htlc_get_by_txid can return a false NotFound, and fail_transfers_finish can complete SQL failure/backup without healing or quarantining the corresponding operation. Return the inconsistency, or persist a typed quarantine record that callers must resolve. Please add malformed, truncated, and missing-meta scan tests.
| None, | ||
| ), | ||
| }; | ||
| self.htlc_write_meta(&operation_id, &meta)?; |
There was a problem hiding this comment.
There is still a prepare crash window here. Prepared metadata is published before the SQL batch exists. The error cleanup below only handles a returned Err; a process/power loss after this write leaves an apparently adoptable operation with no linked batch, so htlc_apply cannot complete it. Retrying prepare can publish a second operation for the same colored txid and make txid lookup ambiguous. Please model Preparing -> Prepared explicitly and reconcile/rollback the pre-commit intent idempotently, or atomically publish the operation only after the batch is recoverably committed. Add kill tests on both sides of the SQL commit and batch-identity write.
| self.check_online(online)?; | ||
| let txn = self.database().begin_transaction()?; | ||
| let outcome = | ||
| self.fail_transfers_impl(&txn, batch_transfer_idx, no_asset_only, skip_sync)?; |
There was a problem hiding this comment.
The generic failure path bypasses the new broadcast-state invariant. fail_transfers_impl does not inspect HTLC metadata before marking the batch failed; healing runs only afterward and blindly mirrors SQL Failed. I reproduced this on the exact head by extending htlc_abort_refused_after_broadcast_attempt: after htlc_mark_broadcast, fail_transfers_single(batch_idx) returned true and failed the transfer. An Attempted, Observed, or Ambiguous HTLC must be rejected/quarantined before the SQL transaction commits, including bulk expiry. Please add this regression for single and bulk failure paths.
| match meta.broadcast { | ||
| HtlcBroadcastState::NotAttempted => { | ||
| meta.broadcast = HtlcBroadcastState::Attempted; | ||
| self.htlc_write_meta(operation_id, &meta) |
There was a problem hiding this comment.
The safety-critical broadcast transition is neither reachable by the required external caller nor remotely durable. This PR adds no UniFFI/UDL method or HtlcBroadcastState binding, while issue #90 states that the HTLC flow is called from Go. It also returns after only the local metadata write and does not trigger, await, or fence a VSS backup. After a real broadcast followed by device loss, restore can regress to NotAttempted and permit rollback. Expose the transition through UniFFI and require durable acknowledgement (or integrate mark+broadcast behind one owned operation) before the caller broadcasts.
| /// <div class="warning">This method is meant for special usage and is normally not needed, use | ||
| /// it only if you know what you're doing</div> | ||
| #[cfg(any(feature = "electrum", feature = "esplora"))] | ||
| pub fn htlc_get_by_txid(&self, txid: &str) -> Result<HtlcOperation, Error> { |
There was a problem hiding this comment.
Txid is not a reliable adoption key for the response-loss case documented here. The UniFFI caller passes the original PSBT by value; coloring may add/change the RGB commitment and uses random GraphSeal blinding when static_blinding is absent. If htlc_prepare commits and its response is lost, the caller has neither operation_id nor the resulting colored txid, so it cannot call this lookup. This method is also not exposed through UniFFI. Please accept a caller-generated idempotency/operation key known before prepare, make retries return the same committed result, and expose the lookup/result types to the external binding.
| #[cfg(any(feature = "electrum", feature = "esplora"))] | ||
| use crate::utils::{recipient_id_from_script_buf, script_buf_from_recipient_id}; | ||
| use bdk_wallet::bitcoin::Transaction; | ||
| use bdk_wallet::bitcoin::hashes::{Hash, sha256}; |
There was a problem hiding this comment.
This import needs the same electrum || esplora cfg as the code that uses it. On the exact PR head, cargo clippy --lib --workspace --no-default-features -- -D warnings fails because both Hash and sha256 are unused.
201f3e0 to
41b6ec9
Compare
- Added `stash_contains_fascia_witness` method to check for fascia witness in stash. - Refactored `fail_transfers` methods in `multisig.rs` and `online.rs` to streamline transaction handling and error management. - Introduced new methods for handling HTLC operations, including `heal_htlc_ops_after_failed_transfers` and `fail_transfers_commit`. - Updated `rust_only.rs` with new constants and functions for HTLC consignment paths and SHA256 hashing. - Enhanced test coverage for HTLC operations, ensuring consistency and error handling during transaction failures.
41b6ec9 to
dc9dded
Compare
a2d3333 to
ab9f414
Compare
|
718e29b to
74522b0
Compare
3 participants
This PR is separated from htlc_preparation branch, cause its creates to much diffs, main goal of this pr to fix these issues - #65 (review)