Include interactive funding candidates on broadcast#4570
Include interactive funding candidates on broadcast#4570jkczyz wants to merge 2 commits intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
|
I've now thoroughly reviewed every file and hunk in the diff, verified the contribution-offset math, checked the No new issues found beyond what was already flagged in prior review comments. Here is my final summary: Review Summary No new issues found. Prior review comment status:
Verification performed:
|
|
🔔 1st Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
1 similar comment
|
🔔 1st Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
1 similar comment
|
🔔 2nd Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
| if !self.context.is_manual_broadcast | ||
| && !waiting_for_batch | ||
| && self.funding.get_funding_tx_confirmation_height().is_none() | ||
| { | ||
| if let Some(tx) = &self.funding.funding_transaction { | ||
| return Some(( | ||
| tx.clone(), | ||
| TransactionType::Funding { | ||
| channels: vec![( | ||
| self.context.counterparty_node_id, | ||
| self.context.channel_id, | ||
| )], | ||
| }, | ||
| )); | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: After 0-conf splice promotion, self.pending_splice is None (cleared at maybe_promote_splice_funding line 11680) and self.funding holds the splice's FundingScope with funding_tx_confirmation_height == 0 and funding_transaction = Some(splice_tx). This initial-funding branch will match:
is_manual_broadcast= false (normal channel)waiting_for_batch= false (channel isChannelReady)get_funding_tx_confirmation_height().is_none()= true (height is 0)funding_transaction=Some(splice_tx)
...and return TransactionType::Funding wrapping the splice transaction. The splice branch below is unreachable because self.pending_splice is None.
The test test_splice_rebroadcast_uses_splice_type_after_0conf_promotion (added in this PR) expects TransactionType::Splice with the original contribution preserved, but this code would produce TransactionType::Funding. That test will fail.
A possible fix: check self.funding.channel_transaction_parameters.splice_parent_funding_txid — it is Some(...) for promoted splices and None for initial funding. You'd also need to persist the contribution and replaced_txid metadata somewhere that survives promotion (e.g., on FundingScope or ChannelContext), since pending_splice is cleared.
There was a problem hiding this comment.
Discussed offline. Dropped the commit to retry for now. We may do this later in ChannelMonitor where we'll store the FundingContribution.
jkczyz
force-pushed
the
2026-04-splice-transaction-type
branch
from
dbaf6d9 to
86dcede
Compare
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Needs rebase, also ci is very sad.
jkczyz
force-pushed
the
2026-04-splice-transaction-type
branch
from
86dcede to
dacf092
Compare
|
Rebased. Compilation failures were from new tests in main. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4570 +/- ##
==========================================
- Coverage 87.15% 86.21% -0.94%
==========================================
Files 161 159 -2
Lines 109251 109136 -115
Branches 109251 109136 -115
==========================================
- Hits 95215 94097 -1118
- Misses 11560 12429 +869
- Partials 2476 2610 +134
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🔔 3rd Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
|
🔔 1st Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
| contribution: Option<FundingContribution>, | ||
| /// For an RBF replacement, the txid of the prior negotiated splice candidate being | ||
| /// replaced. `None` for the first splice attempt. | ||
| replaced_txid: Option<Txid>, |
There was a problem hiding this comment.
Hmm, what happens when we make multiple subsequent RBFs? Could/should we make this a Vec and track all prior conflicting Txids to make sure we're not losing any intermediary step when tracking these events?
Also, when integrating BDK's RBF events in LDK Node we found that it would be nice if the API clearly defined an ordering for the replaced_txids, i.e., so we can always identify the 'original' txid (under which we started tracking a certain payment). That is, if we make this a Vec it would be great if the API could guarantee that replaced_txids[0] is always the 'original'.
There was a problem hiding this comment.
That seems reasonable, though things get hairy if we have multiple splice-RBFs that conflict across channels. ISTM LDK Node might need to track that somehow, but maybe we can just avoid ever building such things?
There was a problem hiding this comment.
Ok, discussed a bit offline. Ideally, we'll want to use a format in LDK Node that is compatible with batching. I've pushed a change that refactors TransactionType::Splice to TransactionType::InteractiveFunding, which supports batches of both v2 channel establishment and splices in the future. It also contains all previous attempts (including contributions) along with the corresponding txid instead of replaced_txid. Including simplifies the downstream handling. Then in 0.4 we can add re-broadcasts.
See how it's used in LDK Node here: lightningdevkit/ldk-node#888. LMK what you think.
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Changes LGTM, no strong opinion on the API, if it works for LDK Node alright, though I agree exposing all the previous RBFs might be nice.
| contribution: Option<FundingContribution>, | ||
| /// For an RBF replacement, the txid of the prior negotiated splice candidate being | ||
| /// replaced. `None` for the first splice attempt. | ||
| replaced_txid: Option<Txid>, |
There was a problem hiding this comment.
That seems reasonable, though things get hairy if we have multiple splice-RBFs that conflict across channels. ISTM LDK Node might need to track that somehow, but maybe we can just avoid ever building such things?
|
🔔 5th Reminder Hey @wpaulino! This PR has been waiting for your review. |
jkczyz
force-pushed
the
2026-04-splice-transaction-type
branch
2 times, most recently
from
925b58d to
4b82770
Compare
|
CI is very sad but feel free to squash. |
jkczyz
force-pushed
the
2026-04-splice-transaction-type
branch
from
4b82770 to
741430b
Compare
|
Squashed and rebased to fix CI. |
| impl_writeable_tlv_based!(FundingCandidate, { | ||
| (1, txid, required), | ||
| (3, channels, required_vec), | ||
| }); | ||
|
|
||
| impl_writeable_tlv_based!(ChannelFunding, { | ||
| (1, counterparty_node_id, required), | ||
| (3, channel_id, required), | ||
| (5, purpose, required), | ||
| (7, contribution, option), | ||
| }); | ||
|
|
||
| impl_writeable_tlv_based_enum!(FundingPurpose, | ||
| (0, Establishment) => {}, | ||
| (2, Splice) => {}, | ||
| ); |
There was a problem hiding this comment.
Nit: these three serialization impls (FundingCandidate, ChannelFunding, FundingPurpose) appear to be dead code — TransactionType is never serialized (no Writeable/Readable impl), and these types are only used inside TransactionType::InteractiveFunding, which is purely in-memory and passed through BroadcasterInterface::broadcast_transactions as a reference.
If these are forward-looking (e.g., for future persistence or cross-process messaging), a brief comment explaining the intent would help. Otherwise they could be removed to avoid confusion.
There was a problem hiding this comment.
We need these for downstream
There was a problem hiding this comment.
Might be worth adding a comment?
jkczyz
changed the title
TransactionType::Splice
jkczyz
force-pushed
the
2026-04-splice-transaction-type
branch
from
741430b to
8c3d58f
Compare
jkczyz
force-pushed
the
2026-04-splice-transaction-type
branch
2 times, most recently
from
f245f31 to
d94e8fb
Compare
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
LGTM, one non-blocking question.
| /// | ||
| /// This is used to provide context about the type of transaction being broadcast, which may be | ||
| /// useful for logging, filtering, or prioritization purposes. | ||
| #[derive(Clone, Debug, Hash, PartialEq, Eq)] |
There was a problem hiding this comment.
Any particular reason we drop the Hash here? Can't fully recall if we need it or not, but in general it seems like a reasonable thing to keep in the API?
There was a problem hiding this comment.
Ah, for some reason I had been under the impression Hash wasn't implemented for some of the rust-bitcoin types. Fixed now.
| /// [`ChannelManager::splice_channel`]: crate::ln::channelmanager::ChannelManager::splice_channel | ||
| Splice { | ||
| /// The `node_id` of the channel counterparty. | ||
| counterparty_node_id: PublicKey, |
There was a problem hiding this comment.
Hmm, unfortunate that counterparty_node_id / channel_id are moved now. I see that keeping them would be prohibitive in the batching case, but it seems this will make classification / attribution way less intuitive downstream. But of course we already have essentially the same concern with the Sweep variant.
There was a problem hiding this comment.
Hmm... they are still here just deeper in the structs. Similar how the the Funding variant has them in a Vec for v1 batches. For interactive funding, we may not know which set will be applicable until confirmed since an RBF may add / remove channels when batching.
Replace TransactionType::Splice with TransactionType::InteractiveFunding so downstream consumers can update their own state tracking from the broadcast callback. The local contribution data isn't recoverable from the on-chain transaction, so the broadcast must surface it directly. Each candidate carries the participating channels and their local contributions; the broadcast lists every negotiated candidate — original first, then each RBF replacement — letting downstream reconcile any historical txid, not just the immediate predecessor. The new variant is structured to be forward-compatible with batches and V2 (dual-funded) channel establishment, neither of which is implemented today. The new types are Writeable/Readable so downstream can persist them directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add public getters for `estimated_fee`, `inputs`, and `max_feerate`, and elevate `feerate` from `pub(super)` to `pub`. Together with the existing `value_added`, `outputs`, and `change_output`, this gives downstream consumers of `TransactionType::Splice` (notably LDK Node, which updates `PaymentDetails` from the broadcast callback) the data they need without reaching into the raw transaction. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jkczyz
force-pushed
the
2026-04-splice-transaction-type
branch
from
d94e8fb to
6e1894b
Compare
| .get_funding_txid() | ||
| .expect("negotiated candidates should have a funding txid"); | ||
| let contribution = i | ||
| .checked_sub(contrib_offset) |
There was a problem hiding this comment.
Isn't the offset always 0 here? We're currently exchanging tx_signatures, which means we can't have a different negotiation in progress and the current one we're exchanging signatures for has already been tracked as a candidate above.
| impl_writeable_tlv_based!(FundingCandidate, { | ||
| (1, txid, required), | ||
| (3, channels, required_vec), | ||
| }); | ||
|
|
||
| impl_writeable_tlv_based!(ChannelFunding, { | ||
| (1, counterparty_node_id, required), | ||
| (3, channel_id, required), | ||
| (5, purpose, required), | ||
| (7, contribution, option), | ||
| }); | ||
|
|
||
| impl_writeable_tlv_based_enum!(FundingPurpose, | ||
| (0, Establishment) => {}, | ||
| (2, Splice) => {}, | ||
| ); |
There was a problem hiding this comment.
Might be worth adding a comment?
6 participants
Include every negotiated funding candidate (original + RBF replacements) at broadcast time so downstream consumers can update their own state tracking from the broadcaster callback. The local contribution data isn't recoverable from the on-chain transaction, and the replaced txids aren't either, so the broadcaster callback is the only place where this information can be observed.
TransactionType::Spliceis replaced withTransactionType::InteractiveFunding { candidates: Vec<FundingCandidate> }. EachFundingCandidatecarries itstxidand the channels participating in it; eachChannelFundingcarries the channel id, counterparty node id, purpose (Establishment / Splice), and the local node's contribution for this candidate. The list covers every negotiated candidate in order — original first, then each RBF replacement — letting downstream reconcile any historical txid, not just the immediate predecessor. The alternative (reacting toSplicePending) was worse: that event races with BDK wallet sync and doesn't carry the replaced txid.channelslist has length 1.FundingCandidate,ChannelFunding, andFundingPurposeimplementWriteable/Readableso downstream can persist them directly without mirroring.FundingContributiongains public getters forestimated_fee,inputs, andmax_feerate;feerateis elevated frompub(super)topub. Combined with existingvalue_added,outputs,change_output, this exposes everything a downstream consumer needs without reaching into the raw transaction.