Skip to content

Make RGB channel funding restart-safe - #32

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

Make RGB channel funding restart-safe#32
Jainakin wants to merge 1 commit 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 acceptance recoverable across process failure without adding recovery state to ChannelManager or ChannelMonitor serialization.

The current patch is one commit and four files:

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

Current head: 32b8c6b6046c3bfac44ee94549cadf9916e234ae.

Failure model

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

Design

  1. Remove the inbound channel from the peer map under the existing peer-state mutex.
  2. Persist a versioned RGB funding intent.
  3. Fetch and validate the transfer into an isolated rgb-lib stock through rgb-lib #80, outside the peer-state mutex.
  4. Re-enter the peer-state mutex and promote the prepared stock while retaining an exact rollback snapshot.
  5. Build and consume the funding commitment through the same operation-owned journal.
  6. Complete LDK's normal funding_created transition and initial monitor persistence before releasing funding_signed.
  7. Roll back deterministically on rejection before durable channel state.
  8. Leave final commit, rollback, or quarantine to the embedding node, which reconciles the journal against list_funded_channels().

The durable receiver stages are Validating, Prepared, Promoted, RollingBack, Finalizing, Finalized, and RetryRequired. No ChannelManager or ChannelMonitor persisted format changes are introduced.

Locking and ownership

Network-heavy transfer fetch and validation remain outside the ChannelManager peer-state mutex, so channel reads do not inherit the complete high-history validation delay. The mutex is reacquired before stock promotion and before any LDK channel transition.

  • rust-lightning owns validation, preparation, promotion, and rollback 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, payment resend changes, onion-routing changes, colored-fee changes, or unrelated migration work remain

Peer message handling remains synchronous while validation runs. This PR does not claim to make the native funding operation asynchronous or cancellable.

Validation

  • cargo +1.63.0 fmt --all -- --check
  • ./ci/check-lint.sh
  • cargo check --workspace --features lightning/electrum --verbose
  • GitHub Actions: 6/6 checks green across Linux, Windows, macOS, stable, beta, lint, and rustfmt

The existing changes-requested review was submitted against a superseded architecture. Its serialization, payment, routing, and scope concerns are absent from this four-file head.

Dependency and merge order

  1. rgb-lib #80
  2. This PR, repinned to the official #80 merge commit
  3. rgb-lightning-node #139
  4. rgb-lightning-node #140

Release gates

  • review the out-of-peer-mutex preparation boundary explicitly
  • replace contributor-fork pins with official immutable revisions after merge
  • rerun the downstream crash/restart and platform matrices against those 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
@Jainakin
Jainakin force-pushed the hardik/restart-safe-rgb-funding branch 2 times, most recently from 34a59af to c031393 Compare August 27, 2026 15:51
@Jainakin
Jainakin force-pushed the hardik/restart-safe-rgb-funding branch from c031393 to 32b8c6b Compare August 27, 2026 16:26
@Jainakin

Copy link
Copy Markdown
Author

Final current-head update:

  • Head: 32b8c6b6046c3bfac44ee94549cadf9916e234ae
  • One focused commit across four files.
  • No ChannelManager or ChannelMonitor serialization changes.
  • No payment-routing, onion, fee, or unrelated migration changes.
  • Network-heavy receiver preparation remains outside the peer mutex; final channel transition remains under the existing synchronization boundary.
  • Current CI: 6/6 checks successful.

The existing requested-changes review targets superseded commit e78e6b6. Could you re-review the current head?

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