Make RGB channel funding restart-safe - #139
Conversation
260617b to
680036c
Compare
bf8f57a to
6fe3f25
Compare
6fe3f25 to
6993058
Compare
| } | ||
|
|
||
| #[allow(dead_code)] | ||
| pub(crate) fn list_rgb_funding_recoveries( |
There was a problem hiding this comment.
These are #[allow(dead_code)] with no caller in #139, only #140 calls them. Dead code shouldn't live in the PR that doesn't use it. Move the API functions, the RgbFundingRecoveryCommand / RgbFundingRecoveryRequiredAction presentation enums, and the DTO fields that only they consume into #140 alongside the routes that wire them. #139 keeps the recovery engine; #140 owns the whole API surface.
There was a problem hiding this comment.
Addressed in a8ebbf4. #139 no longer contains the public recovery commands, list/resolve entry points, presentation enums, routes, or binding DTOs. It retains only the internal typed recovery state and action used by startup reconciliation and quarantine. The complete public surface is owned by #140, which is restacked directly on the current #139 head.
| .insert(funding_txid.to_owned()); | ||
| } | ||
|
|
||
| pub(crate) fn ensure_financial_operations_allowed( |
There was a problem hiding this comment.
Holding on an ambiguous broadcast instead of guessing is the right instinct. Two problems with how it's applied. Scope: it's node-wide, any one quarantined txid blocks all sends/opens/swaps, but the ambiguity is per-funding, so one stuck open freezes the whole node; it should be scoped to the affected funding/allocation. Ordering: the only exit (recheck/resume) is in #140, so #139 introduces the quarantine but not its way out. They need to land together, #139 shouldn't reach a deployable state ahead of #140.
There was a problem hiding this comment.
Addressed to the narrowest boundary currently safe. 7aee28f allows BTC-only existing-channel payments and invoice operations to bypass the RGB lease, while pure reads remain available. Channel opens, on-chain wallet operations, swaps, and RGB payments still touch shared wallet/stock state and therefore remain gated. rgb-lib currently has one pending acceptance/rollback snapshot, so per-funding admission could let a second mutation overwrite the sole rollback owner; true per-allocation concurrency requires resource-keyed stock journals. #139 and #140 remain release-coupled: they may merge sequentially, but #139 must not be tagged or deployed without #140.
There was a problem hiding this comment.
Addressed at the currently safe boundary in f31ea35. BTC-only existing-channel operations bypass the RGB lease, while channel opens, swaps, on-chain wallet operations, and other shared-stock mutations remain wallet-wide because rgb-lib currently has one rollback owner. #139 and #140 remain release-coupled.
| /// The funding state machine handles persistence errors for `psbt` and `pending_funding`, so | ||
| /// these records can fail closed without panicking the event handler. | ||
| #[cfg(feature = "vss")] | ||
| fn requires_remote_durability(primary_namespace: &str, secondary_namespace: &str) -> bool { |
There was a problem hiding this comment.
channel_info isn't in the fail-closed set, so runtime balance updates (update_rgb_channel_amount then write_rgb_channel_info) are best-effort and ack even while VSS is down. Outage, payments settle, device loss before the queue drains, restore reverts to the funding-time split and we mis-attribute balances. Can we fail-close channel_info/channel_info_pending, or document why runtime updates may be lost?
There was a problem hiding this comment.
Addressed in 2f99ce1 and 57af376. Writes and removals for both rgb/channel_info and rgb/channel_info_pending now require remote acknowledgement whenever VSS is configured. Device-loss tests restore from an empty local store and verify both durable writes and durable deletions. Transient remote failure blocks or retries the transition; it is not acknowledged locally as complete.
There was a problem hiding this comment.
Addressed in 0ed09b1. Writes and removals for canonical and pending RGB channel metadata require remote VSS acknowledgement whenever VSS is configured. Device-loss coverage restores from an empty local store and verifies both durable writes and deletions.
| Ok((completed, unresolved)) | ||
| } | ||
|
|
||
| pub(crate) fn reconcile_rgb_sender_funding( |
There was a problem hiding this comment.
These arms propagate with ?, so a transient failure at boot (a VSS timeout, an indexer hiccup) aborts the whole node start, including the read-only recovery surface. The FailClosed path already degrades to quarantine; could the recoverable arms do the same on a transient error instead of failing boot?
There was a problem hiding this comment.
Addressed. Per-record VSS, indexer, and reconciliation failures are caught and retained as typed unresolved recovery evidence, so startup continues and the recovery surface remains available while RGB mutation is quarantined. Startup still aborts for failures that make the journal inventory unreadable or leave stock with no safely identifiable recovery owner; degrading those cases would not be fail-closed.
There was a problem hiding this comment.
Addressed in 0ed09b1. Per-record VSS, indexer, and reconciliation failures preserve typed unresolved recovery evidence, allow startup and diagnostics to remain available, and quarantine financial mutation until the evidence is reconciled.
| Some( | ||
| unlocked_state | ||
| .rgb_funding_recovery_guard | ||
| .lock_operation() |
There was a problem hiding this comment.
The single op lock is held across several spawn_blocking calls plus a VSS backup plus a consignment upload, stalling the event loop for other funding events, and concurrent sends make the second return ChangingState even with no recovery pending. Worth measuring (keysend throughput is the canary) and ideally scoping per-operation rather than global.
There was a problem hiding this comment.
Addressed at the safe boundary rather than weakening the recovery invariant. 7aee28f proves BTC-only channel payments do not acquire the RGB lease during either an active mutation or quarantine, and lease-duration instrumentation reports holds above 250 ms. 2afa5e3 additionally gives LDK's output sweeper a bounded, fair admission window, fixing the starvation exposed by multi_open_close without introducing an unbounded wait. Operations touching rgb-lib stock remain wallet-wide because the rollback snapshot is wallet-wide. I am not claiming a network-throughput benchmark from the deterministic admission tests.
There was a problem hiding this comment.
Addressed in f31ea35 without weakening the recovery invariant. BTC-only channel operations bypass the RGB lease, and the output sweeper receives a bounded, fair one-second admission window before deferring. Operations touching rgb-lib stock remain wallet-wide because the rollback snapshot is wallet-wide; no unsupported throughput claim is being made.
| @@ -157,10 +163,398 @@ const CONFIG_WALLET_ACCOUNT_XPUB_COLORED: &str = "wallet_account_xpub_colored"; | |||
| const CONFIG_WALLET_MASTER_FINGERPRINT: &str = "wallet_master_fingerprint"; | |||
| const VIRTUAL_CHANNEL_DRAFTS_KEY: &str = "virtual_channel_drafts"; | |||
| const VIRTUAL_CHANNEL_SESSIONS_KEY: &str = "virtual_channel_sessions"; | |||
|
|
|||
| #[cfg(test)] | |||
| pub(crate) static ACK_NEXT_RGB_FUNDING_BROADCAST_SAFE_WITHOUT_PROCESSING: AtomicBool = | |||
There was a problem hiding this comment.
Crash-test coverage stops at the first checkpoint. This broadcast-safe replay hook is defined but no test ever sets it, and nothing covers HandoffReady through Finalized, ResumeBroadcast, or FailClosed/quarantine entry. That's the coverage that matters most here; can we add deterministic crash tests for those?
There was a problem hiding this comment.
Addressed. The sender crash matrix covers preparation, stock promotion, HandoffReady, HandedToLdk, broadcast-safe observation, broadcasting, broadcast commitment, finalization, and durable completion, including resume-broadcast and fail-closed outcomes. Receiver coverage includes prepared rollback and promoted-state quarantine. The #147 channel-open crash regression is included with channel-count and asset-conservation assertions.
There was a problem hiding this comment.
Addressed in 0ed09b1. The deterministic matrix covers preparation, promotion, handoff, broadcast-safe observation, broadcasting, broadcast commitment, finalization, durable completion, resume-broadcast, and ambiguous fail-closed outcomes. Receiver coverage includes prepared rollback and promoted-state quarantine, and the #147 channel-open crash regression is included.
| @@ -605,39 +1018,59 @@ impl UnlockedAppState { | |||
| pub(crate) fn list_updated_inbound_payments(&self) -> LdkHashMap<PaymentHash, PaymentInfo> { | |||
There was a problem hiding this comment.
Two things read cleaner as their own change. The payment-read purity work here (effective_inbound_payment_status + this rewrite) is a payment-listing behavior change, closer to #33 and self-contained. And the ~60-site ensure_financial_operations_allowed sprinkle across routes/sdk: keep the guard type, but the blanket application to every financial endpoint is a big cross-cutting diff that's easier to review as its own commit.
There was a problem hiding this comment.
Separated at commit boundaries for independent review: 6993058 contains payment-read purity, 50b9a4f contains recovery admission, and 7aee28f contains the BTC/RGB classification refinement. Admission remains in #139 because landing a recovery journal without enforcing its mutation boundary would leave the recovery race open; the behavior changes are no longer interleaved through the recovery implementation.
There was a problem hiding this comment.
Addressed in the current history. 0ed09b1 contains the recovery engine, f31ea35 contains cross-cutting recovery admission and BTC/RGB classification, and 7f6708c contains payment-read purity. The resulting final tree is byte-identical to the fully validated prior head, and the fresh current-head matrix completed with 46 successful checks.
|
|
||
| #[derive(Clone, Copy, Debug, Deserialize, Eq, PartialEq, Serialize)] | ||
| #[serde(rename_all = "snake_case")] | ||
| pub(crate) enum RgbSenderFundingStage { |
There was a problem hiding this comment.
Some state modeling is heavier than the recovery logic needs. HandoffReady/HandedToLdk are never branched apart, and Broadcasting/BroadcastCommitted are never distinctly recovered, so this 10-stage enum is really ~7 decisions. RgbFundingRecoveryRequiredAction::AwaitingLdkEventReplay is never constructed anywhere. And the receiver is forced into this sender enum: rgb_receiver_funding_recovery_view lossy-maps its own FundingAcceptanceStage onto RgbSenderFundingStage (Promoted and Finalizing both collapse to StockPromoted) because the shared RgbFundingRecovery DTO types stage to the sender enum. A type smell worth cleaning up.
There was a problem hiding this comment.
Addressed in a8ebbf4 and completed across the bindings in #140. Recovery now uses role-specific Sender(...) and Receiver(...) stages, so receiver Promoted and Finalizing are no longer collapsed into sender stages. The unused AwaitingLdkEventReplay action was removed. Sender handoff and broadcast stages remain explicit because they are durable side-effect boundaries, preserve crash provenance, and are independently exercised by the recovery matrix even where two stages intentionally lead to the same conservative action.
There was a problem hiding this comment.
Addressed in 0ed09b1 and completed across the public surface in #140. Internal recovery uses exact sender and receiver stage types, so receiver stages are no longer lossy-mapped onto sender stages. The unused action was removed. Durable handoff and broadcast stages remain distinct because they are independently tested side-effect boundaries; #140 exposes stage only as an opaque diagnostic string.
* pin rgb-lib to exact version
* CI: switch from nightly to beta build
* add coverage support
* complete /listunspents fields
* fix README
* fix alphabetical order
* consignment/media for LN ops via p2p
* support out-of-band consignment/ack
* improve /refresh result
* add test reproducing sweeper breakage on force close with pending RGB HTLC
* fix for close_force_pending_htlc test
* use a generic drop guard for test-only node override statics
* restart the payer with the HTLC pending in close_force_pending_htlc
* update rust-lightning submodule
* add test for push_asset_amount above channel asset amount
* update rust-lightning (fixes push_asset_amount_above_chan_amt)
* wait for electrs to be usable after restarting it
* defer claiming payments in tests to fix pending status races
* replace magic-crypt with scrypt and XChaCha20Poly1305
The mnemonic file now records the scrypt work factors next to the salt, nonce
and ciphertext, so they can be raised later without leaving existing files
undecryptable. Key derivation and cipher setup are shared with the backup code,
whose derived key is unchanged.
Mnemonic files written by previous versions can no longer be read, including
those contained in older backups, so affected wallets have to be re-initialized.
* fix network conversion in WalletSource::list_confirmed_utxos
* shut down on panic or unexpected background processor exit
* improve shutdown on panic
* drop default indexer
* add electrum/esplora features
* Make bitcoind optional: add transaction-sync as an alternative chain backend
Adds `lightning-transaction-sync` (electrum/esplora) as an alternative to the
existing `lightning-block-sync` (bitcoind) chain backend, rather than replacing
it.
The chain backend is selected explicitly at unlock time via the `ldk_chain_sync`
field of the `/unlock` request, an adjacently-tagged `{ "mode", "config" }`
object:
- `BlockSync`: consume full blocks from a trusted/local bitcoind over JSON-RPC.
The `bitcoind_rpc_*` parameters live in this mode's `config`, so they are
required exactly when this mode is selected.
- `TransactionSync`: sync through an electrum/esplora indexer, so no bitcoind
is required. By default it reuses the wallet's `indexer_url`; a dedicated LN
indexer can be set in this mode's `config`.
Each backend is gated behind a Cargo feature (`block-sync`, `transaction-sync`),
both enabled by default and composing with the existing `electrum`/`esplora`
features, so a user can build with only the sync dependency they need. The mode
variants and their wiring are feature-gated accordingly, a build with neither
feature fails with a `compile_error!`, and the single-feature builds are covered
in the build and lint workflows.
The backends live in a new `ldk_chain_backend` module (`block_sync`,
`transaction_sync` and a shared `mod.rs` holding the chain-backend types and the
common fee-estimate logic). LDK is wired against trait objects for the fee
estimator, broadcaster and gossip UTXO lookup so one set of type aliases serves
both backends. Because `lightning-block-sync`'s `GossipVerifier` requires the
`P2PGossipSync` to be typed with `Arc<Self>`, a `BlockSyncGossipVerifier`
provides the block-sync gossip UTXO lookup against the shared trait-object
gossip sync, mirroring upstream's block cache. The transaction-sync gossip
lookup enforces the same six-confirmation depth as the block-sync path.
For the electrum backend, transactions registered via `Filter::register_tx` are
confirmed with a supplementary pass against the electrum server (without holding
its lock across network calls), as `ElectrumSyncClient::sync` alone does not
notify the confirmables about them in this setup.
Adds an integration test for the transaction-sync backend against both electrum
and esplora indexers (an esplora service is added to the regtest compose file):
three nodes are unlocked in transaction-sync mode, two announced RGB asset
channels are opened and node1 makes a multihop RGB payment to node3 (exercising
the indexer gossip lookup), the nodes are restarted (re-establishing the
channels after syncing via the indexer) and a channel is cooperatively closed.
Closes UTEXO-Protocol#125
* add electrum_opret_confirm test
* update rust-lightning (fix for electrum_opret_confirm test)
* improve the transaction-sync backend
* drop workaround for note indexed transactions in electrum transaction
sync
* verify the funding output is unspent in the indexer gossip lookup as
the block-sync path already did
* require indexer_url in the transaction-sync config, following the
default indexer removal
* lint all 9 feature combinations in CI
* test suite no longer needs block-sync; wait for esplora when mining
* minor improvements/changes
* return errors instead of panicking on recoverable wallet failures
* point rgb-lib to master and bump MSRV to 1.94.0
* add asset filter and optional txid to listtransfers
* add BOLT11 description and description_hash to invoice creation, decode and payment APIs
* Send asset media over p2p on virtual channel opens and make funding transfer failures terminal
* Restore consignment generation for external-signer sends and wire p2p transfer limits through config
* decode sweep transfer info without panicking under the txes lock
* keep consignment cleanup non-fatal and tolerate a poisoned txes lock
* drop the cached sweep tx when persisting it fails
* reuse sweep receives across retries instead of issuing one per attempt
* shrink the cached sweep receive reuse margin when reusing addresses
* point rgb-lib at the upstream sync branch
* assert the sweep receive reuse margin is non-zero
* lower the scrypt work factors for the library test targets too, editing only the two log_n expressions
* pin the backup v1 derived key with a test vector
* validate a backup before installing it, so a failed restore leaves the node initializable
* say that the poisoned lock is ignored, not propagated
* update rust-lightning (electrum unindexed-output fix) and enable lightning indexer features
* migrate uniffi binding consumers to LdkChainSync and test the indexer_url config fallback
* bump the stale MSRV pins, format c-ffi, and keep backup v1 work factors out of feature gating
* point the rust-lightning submodule at the pushed sync branch
* stop the peer reconnect loop when the node stops instead of polling its database forever
* delete the staged funding consignment file once it is promoted to the KVStore
* pin the p2p funding transport and mandatory rgb-lib expirations in the sdk payment test
* make the asset-less transfer filter and receive reuse margin assertions non-vacuous
* use as_chunks for the fixed-size hex pair split
* route panics through a bounded shutdown that releases the vss fence only on a complete teardown
* gate scrypt CURRENT work factors on cfg(test) only
* run vss teardown headline tests in CI
* error instead of vanilla-sweeping a colored output missing from a non-empty transfer map
* stop the monitor kv store on abandoned vss teardown paths
* extract main exit decision so watchdog tests exercise the real one
* validate asset id and txid inputs to getconsignment against path traversal
* treat empty invoice description as none so description_hash invoices mint
* expose bolt11 description and description_hash on sdk and binding decode paths
* cover the pre-sync colored-channel deserialization tripwire
* bump rgb-lib pin and rust-lightning gitlink for the consignment and channel fixes
* flip rgb-lib pin to released UTEXO tag v0.3.0-beta.34 and bump submodule
* point rust-lightning submodule at the merged UTEXO dev branch
* do not fail CI when the codecov upload has no token
---------
Co-authored-by: Zoe Faltibà <zoefaltiba@gmail.com>
Co-authored-by: Nicola Busanello <nicola.busanello@gmail.com>
Co-authored-by: 0xaudron <r.arpeet82@gmail.com>
Co-authored-by: Zoe Faltibà <7492268+zoedberg@users.noreply.github.com>
Co-authored-by: bitwalt <walterm21@proton.me>
2afa5e3 to
c7e2563
Compare
c7e2563 to
7f6708c
Compare
|
Final current-head update:
All review threads now contain replies against the final history. Please re-review after the ordered #80 and #32 dependency review. |
2 participants
Summary
Make RGB channel funding restart-safe and fail closed across process crashes, restarts, VSS outages, and ambiguous broadcast outcomes.
This PR owns the RLN funding journal, startup/event reconciliation, final RGB commit/rollback decision, financial-operation admission, VSS-durable RGB channel metadata, and deterministic crash coverage. rust-lightning #32 supplies the narrow receiver preparation boundary. Public recovery commands remain isolated in #140.
Current head:
7f6708c51e6182e272c3550fada17304e988a793. The PR is three logical commits on currentdev(db194f7):0ed09b1implements the internal journal, reconciliation engine, durability rules, and crash coverage.f31ea35applies recovery admission across wallet mutations, including BTC-only bypass and bounded output-sweeper admission.7f6708ckeeps payment reads side-effect free when recovery blocks persistence.Recovery model
Sender
Receiver
Admission and liveness
ChangingStateand recovery-required policyThe wallet-wide boundary is intentional because rgb-lib currently has one pending acceptance/rollback snapshot. Resource-scoped concurrency requires resource-keyed stock journals.
VSS durability
rgb/channel_infoandrgb/channel_info_pendingwrites and removals require remote acknowledgement when VSS is configuredCrash and regression coverage
The deterministic sender matrix covers preparation, stock promotion, handoff, broadcast-safe observation, broadcasting, broadcast commitment, finalization, durable completion, resume-broadcast, and ambiguous-evidence paths. Receiver tests cover prepared rollback, promoted-state quarantine, restart from the same storage directory, exact balance conservation, absence of phantom channels, and subsequent channel creation.
The channel-open crash regression from #147 is included. Coverage also includes VSS outage behavior, financial admission, SDK transition readiness, and the output-sweeper starvation observed by
multi_open_close.Validation
multi_open_closepassed in 98.70 seconds with final conservation assertionsThis three-commit head has the exact same Git tree as fully validated head
c7e2563; only commit boundaries changed. The fresh current-head GitHub matrix completed with 46 successful checks and one optional review job skipped. The only red check is the expectedcheck-gitmodulesancestry gate because rust-lightning #32 is not yet reachable from officialdev; that gate must not be waived.Review boundaries and merge order
#138 remains the merged generic durability work. Payment-resend semantics and contract-only import remain separate. This PR and #140 are release-coupled: #139 must not be tagged or deployed without the recovery surface in #140.
This PR remains draft until the official pins, final matrix, review, and production-LSP interoperability gates complete.