Make RGB channel funding restart-safe - #32
Conversation
dcorral
left a comment
There was a problem hiding this comment.
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_paymentnow treatsPaymentSendFailure::PartialFailure/MonitorUpdateInProgressas 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_utilsRGB-amount fork andtx_buildercolored-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/FinalizeRgbFundingAwaitingSignercompletion-action variants and the ChannelManager format change they require, - the reconcile wiring in the
ChannelManagerread/new path, - the
MonitorRestoreUpdates.funding_signedplumbing, - and the whole
connection_epochsubsystem (which only exists becauseprepare_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 assertsencoded[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. ChannelMonitorSERIALIZATION_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.
e78e6b6 to
c17909a
Compare
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.rslightning/src/rgb_utils/mod.rslightning/Cargo.tomllightning-invoice/Cargo.tomlIt 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
funding_createdtransition and initial monitor persistence before releasingfunding_signed.list_funded_channels().The durable RGB stages are
Validating,Prepared,Promoted,RollingBack,Finalizing,Finalized, andRetryRequired. 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
connection_epoch, ChannelManager read reconciliation, or persisted LDK format changes remain.Dependencies
Validation
Current head:
c17909a9fba298d49dbe85d59befd9cbedbe881dLocal:
cargo +1.63.0 fmt --check./ci/check-lint.shcargo check --workspace --offlinecargo check -p lightning --features _rln_test_hooks --offlinegit diff --checkGitHub 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
e78e6b6revision. The serialization and scope concerns raised there are not present in the current four-file patch.Release gates
This PR remains draft until those gates are complete.