Skip to content

Make RGB channel funding restart-safe - #32

Draft
Jainakin wants to merge 2 commits into
UTEXO-Protocol:devfrom
Jainakin:hardik/restart-safe-rgb-funding
Draft

Make RGB channel funding restart-safe#32
Jainakin wants to merge 2 commits into
UTEXO-Protocol:devfrom
Jainakin:hardik/restart-safe-rgb-funding

Conversation

@Jainakin

@Jainakin Jainakin commented Aug 10, 2026

Copy link
Copy Markdown

Summary

Make inbound RGB channel funding recoverable across process failure without adding recovery state to ChannelManager or ChannelMonitor serialization.

The current patch is intentionally limited to four files:

  • lightning/src/ln/channelmanager.rs
  • lightning/src/rgb_utils/mod.rs
  • lightning/Cargo.toml
  • lightning-invoice/Cargo.toml

It does not include the payment resend, onion routing, colored-fee accounting, monitor-format, or unrelated upstream migration changes from the earlier revision.

Failure model

The former receiver path validated and mutated the live RGB stock synchronously during FundingCreated, then advanced the Lightning channel and monitor independently. A crash between those durability boundaries could leave RGB ownership and durable LDK channel state disagreeing.

Current design

  1. Remove the inbound channel from the peer map while holding its ChannelManager peer-state mutex.
  2. Persist a versioned RGB funding intent.
  3. Fetch and validate the transfer into an isolated rgb-lib stock using the transactional acceptance API from rgb-lib #80.
  4. Re-enter the peer-state mutex and promote the prepared stock while retaining an exact rollback snapshot.
  5. Complete LDK's normal funding_created transition and initial monitor persistence before releasing funding_signed.
  6. On any rejection before durable channel state, roll back the promoted RGB operation deterministically.
  7. Leave the final commit, rollback, or quarantine decision to the embedding node, which reconciles the journal against list_funded_channels().

The durable RGB stages are Validating, Prepared, Promoted, RollingBack, Finalizing, Finalized, and RetryRequired. All writes use the existing RGB KV namespace; no ChannelManager or ChannelMonitor wire format is changed.

Locking boundary

Transfer fetch and validation can take minutes for a high-history contract. It intentionally remains outside the ChannelManager peer-state mutex so local channel reads are not blocked for the entire network operation. Under normal PeerManager dispatch, the connection-map read lock prevents peer removal while the callback is active. The code reacquires the ChannelManager peer-state mutex before promotion and before any LDK channel transition.

This boundary is deliberate and is the main concurrency point requested for review. Peer message handling remains synchronous while validation runs; this PR does not claim to make the native funding operation asynchronous or cancellable.

Ownership split

  • rust-lightning validates, prepares, promotes, and rolls back failures that occur before durable funded-channel state.
  • rgb-lightning-node #139 owns startup/event reconciliation and the final commit, rollback, or fail-closed quarantine decision.
  • No monitor-completion action variants, connection_epoch, ChannelManager read reconciliation, or persisted LDK format changes remain.

Dependencies

Validation

Current head: c17909a9fba298d49dbe85d59befd9cbedbe881d

Local:

  • cargo +1.63.0 fmt --check
  • ./ci/check-lint.sh
  • cargo check --workspace --offline
  • cargo check -p lightning --features _rln_test_hooks --offline
  • git diff --check

GitHub Actions is green for stable and beta Linux builds, stable Windows and macOS builds, linting, and rustfmt.

The existing changes-requested review was submitted against the superseded e78e6b6 revision. The serialization and scope concerns raised there are not present in the current four-file patch.

Release gates

  • Review and merge the final rgb-lib dependency stack.
  • Rebase and replace contributor-fork pins with official immutable revisions.
  • Obtain explicit review of the out-of-peer-mutex validation boundary.
  • Run the downstream rgb-lightning-node crash/restart and full platform matrices against the final revisions.

This PR remains draft until those gates are complete.

@dcorral dcorral left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The root-cause analysis is correct (synchronous RGB mutation during FundingCreated diverging from LDK channel/monitor state is exactly the bug), and the receiver-side crash-safety is a real property we need.

My review is at the architecture level; two fundamental concerns before we get into line detail.

1. Scope: roughly half of this isn't funding, and some of it hides real behavior changes

Only about half of the ~2,600 added lines is the funding state machine. The rest falls into three buckets beyond funding:

  • re-enabled test modules (router.rs/msgs.rs/gossip.rs) with mechanical ..._without_rgb renames
  • RGB payment-routing work (onion_utils, tx_builder, the signer split)
  • upstream migration (onion_message BlindedMessagePath/node_signer).

To be clear: most of this is genuinely needed by the RLN side (#139), this is really the LDK half of the whole RGB funding+payment safety stack, not just funding, so the "restart-safe funding" title understates it. My ask is separation/retitling for reviewability, not removal, so each concern is reviewed on its own and nothing functional slips through implicitly. You already flag the test-surface restoration in the description, which I appreciate.

Two things in particular I'd pull into their own PRs:

  • outbound_payment.rs: send_spontaneous_payment now treats PaymentSendFailure::PartialFailure/MonitorUpdateInProgress as in-flight rather than retryable. It's part of #139's safety model, so it's needed, but it's a payment-resend semantics change in the same double-payment territory as rust-lightning#33, it should get explicit payment-safety review, not be approved implicitly by anyone reviewing funding.
  • The onion_utils RGB-amount fork and tx_builder colored-fee accounting are RGB payment mechanics the node needs, but they're a distinct concern from funding crash-safety.

The one genuinely unrelated piece is the onion_message BlindedMessagePath/node_signer migration, that's an upstream rebase artifact with no RGB or funding content, safe to drop from this stack.

2. Design: does the receiver commit need to be inside LDK at all?

The one thing that genuinely must live in LDK is validating the consignment before funding_signed is emitted (there's no event at that boundary, ChannelPending fires after). The pre-PR handle_funding already did that.

The new part, binding the RGB stock commit to monitor durability via serialized MonitorUpdateCompletionAction variants reconciled through ChannelManager::read, is the part I'd push back on, because the fact it encodes ("is this channel funded and its monitor durable?") is observable from outside LDK. This PR adds list_funded_channels() for exactly that, and the sender path already achieves full crash-safety with no in-LDK state machine, it prepares/promotes in the node's event handlers and commits-or-rolls-back at a startup reconcile keyed on list_funded_channels, using FundingTxBroadcastSafe.

Could the receiver mirror the sender? i.e. prepare+promote before funding_signed, then commit-or-rollback at a startup reconcile keyed on funded-ness. That would preserve the exact safety property ("no funding_signed without durable, valid RGB state") while removing, as far as I can tell:

  • the two FinalizeRgbFunding / FinalizeRgbFundingAwaitingSigner completion-action variants and the ChannelManager format change they require,
  • the reconcile wiring in the ChannelManager read/new path,
  • the MonitorRestoreUpdates.funding_signed plumbing,
  • and the whole connection_epoch subsystem (which only exists because prepare_funding's network I/O was moved outside the peer mutex, validation under the lock, as before, needs none of it).

It would also collapse the current three overlapping journals (LDK FundingAcceptanceStage, the node's RgbSenderFundingStage, and the receiver recovery view that reuses the sender stages) toward a single owner, instead of the receiver lifecycle being co-owned across the LDK/RLN boundary through a shared KV namespace.

3. On-disk format changes, please gate these separately

This commit changes two persisted formats:

  • Two required ("must-understand") TLV variants on MonitorUpdateCompletionAction, the test asserts encoded[0] == 6, "the safety-critical action must use a required enum id". A ChannelManager persisted mid-RGB-funding can't be read by a build without these variants, so it's not cleanly downgradable/revertable while an action is in flight.
  • ChannelMonitor SERIALIZATION_VERSION 1 -> 2 with a dual-parse-and-compare read path (decode twice as "standard" vs "legacy_rgb_layout" and compare .encode() to disambiguate).

Both are the kind of change I'd want reviewed as their own deliberate, gated decisions rather than as riders on a funding PR, the monitor v2 dual-parse heuristic especially, given it sits on the most safety-critical serialization we have. If the design in (2) lands, the ChannelManager format change may not be needed at all.

And a related one: the monitor read path is doing backward-compatibility work we shouldn't need. It keeps MIN_SERIALIZATION_VERSION = 1 and the dual-parse reads the legacy UTEXO v1 monitor layout (there's a reads_standard_and_utexo_v1_monitor_layouts test feeding it a version-1 byte). We're pre-release with no deployed nodes carrying the old format, if we don't need to read old monitors, drop the legacy path and just read v2. That removes the brittle parse-both-and-compare heuristic entirely and shrinks the format-change risk. If there's a specific migration case that needs it, let's make that explicit rather than carrying a silent compat path.

@Jainakin
Jainakin force-pushed the hardik/restart-safe-rgb-funding branch from e78e6b6 to c17909a Compare August 19, 2026 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants