[jkczyz]

Fixes #1168.

Node::splice_in_with_all estimated the maximum amount with a wallet drain and then ran coin selection for that exact amount. The resulting contribution had at most the anchor reserve as a change output, so bump_channel_funding_fee had nothing else to pay a higher fee from. With anchor channels enabled, the bump shrank the change output and spent the reserve on the splice fee. Without a reserve there was no change output at all, so the bump failed with ChannelSplicingFailed. The drain estimate also ran at the funding feerate estimate while the contribution could be built at a pending splice's higher RBF minimum.

This PR instead hands LDK the wallet's confirmed UTXOs as manually selected inputs (FundingBuilder::add_inputs). They are consumed in full with no change output, the amount added to the channel is whatever remains after fees at the template's feerate, and a later fee bump is paid by adding a little less to the channel. When an anchor reserve is required, it is kept as an explicit output back to one of the wallet's own addresses, which stays fixed across bumps.

The reserve address is marked used in BDK as soon as it is handed out, since the splice transaction only reaches the wallet once negotiation completes and until then BDK would offer the same address as change elsewhere. The mark is dropped again if the contribution cannot be built, or via DiscardFunding if LDK rejects it.

Insufficient-funds checks stay in front of LDK, which would otherwise build a near-empty contribution. The drain-based splice estimate is removed along with the shared-input handling in the drain helpers that only it used.

LDK adjusts a contribution of manually selected inputs only up to its maximum feerate and never supplements it by coin selection, so a bump past that maximum failed even when the wallet had received coins since. The last commit has bump_channel_funding_fee rebuild such a contribution from all confirmed funds instead: the pending round's inputs plus any coins confirmed in the meantime. Today most bumps more than 1.5x above the first round's estimate take this path because LDK checks the first round's maximum rather than the bump's (https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/issues/5282); once https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/pulls/5297 is in the pin, only bumps the pending inputs cannot cover do.

The first commit extracts the low -incrementalrelayfee bitcoind setup used by the splice RBF tests into a shared helper, since the new tests need it too.

The reserve and no-reserve bump tests fail on main: with a reserve the bump leaves the spliced-in amount unchanged while the fee rises, i.e. it is paid from the reserve; without a reserve bump_channel_funding_fee returns ChannelSplicingFailed.

🤖 Generated with Claude Code

[ldk-reviews-bot]

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[tnull]

Seems Leo's suggestion here might make sense to add in this PR?: #1103 (comment)

[tnull]

Codex:

1. [P2] Preserve small configured anchor reserves — src/lib.rs:1891 (/home/tnull/workspace/ldk-node-review-pr-1176-0T4Hm0/src/lib.rs:1891). With
   per_channel_reserve_sats = 546, this condition omits the reserve output. Because the previous balance check was removed, splice_in_with_all now succeeds
   and drains the wallet to zero sats. Preserve positive reserves using the destination’s dust threshold, or reject the splice when preservation is
   impossible.

[jkczyz]

Thoughts on doing it for open_channel_with_all in this PR? See below.

Claude:

Fixed. A positive reserve is always kept as an output now, rounded up to the dust limit when it is below it, and the insufficient-funds check uses that amount. Added splice_in_with_all_bump_fee_retains_dust_limit_anchor_reserve. open_channel_with_all has the same > comparison; I left it alone here.

[tnull]

Codex:

2. [P2] Report fee shortfalls as InsufficientFunds — src/lib.rs:1970 (/home/tnull/workspace/ldk-node-review-pr-1176-0T4Hm0/src/lib.rs:1970). With 26,000 sats
   confirmed, a 25,000-sat reserve, no local channel balance, and a 2,500 sat/kwu feerate, LDK rejects the contribution before the subsequent insufficient-
   funds check. This mapping returns ChannelSplicingFailed, preventing callers from identifying the funding shortage. Map ManuallySelectedInputsInsufficient
   to InsufficientFunds.

[jkczyz]

Claude:

Fixed. ManuallySelectedInputsInsufficient maps to InsufficientFunds; the other build errors still map to ChannelSplicingFailed.

[tnull]

CI is failing, not sure if flake:

thread 'splice_rounds_discarded_while_the_channel_is_listed_fail_at_close' (307885) panicked at tests/integration_tests_rust.rs:4488:5:
assertion `left == right` failed: the bump reused an input of the first round
  left: [OutPoint { txid: ec9c78fceb249288b98f6b1772c1b2b648178638e4799baeae8a6b086af0c555, vout: 1 }, OutPoint { txid: ec9c78fceb249288b98f6b1772c1b2b648178638e4799baeae8a6b086af0c555, vout: 0 }]
 right: [OutPoint { txid: ec9c78fceb249288b98f6b1772c1b2b648178638e4799baeae8a6b086af0c555, vout: 1 }]
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    splice_rounds_discarded_while_the_channel_is_listed_fail_at_close

[shubhamkmr04]

Since a bump now keeps these manually selected inputs, LDK no longer falls back to coin selection (it returns ManuallySelectedInputsInsufficient instead of MissingCoinSelectionSource). So bumping above the prior contribution's max feerate (1.5x) now fails, even if the wallet has received new confirmed coins since the splice. Before, the bump could use those new coins. Is that OK?

[jkczyz]

Hmm... good point.

Claude:

The check is LDK's: the builder adjusts the prior contribution against its old max_feerate before storing the new one, so adding inputs to the prior-seeded builder fails the same way (filed as https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/issues/5282). Added a commit so bump_channel_funding_fee handles it: when rbf_prior_contribution returns ManuallySelectedInputsInsufficient, the bump is rebuilt via without_prior_contribution from all confirmed funds, the pending round's inputs plus any coins confirmed since, with the reserve paid to a fresh reserved address. splice_in_with_all_bump_fee_above_max_feerate_adds_new_coins covers it. Once LDK applies the caller's maximum, a bump past the ceiling without new coins is adjusted in place again; the rebuild stays for pulling in new coins.

[shubhamkmr04]

With this, the splice_in_with_all docs line "The amount spliced in is determined at min_fee_rate, or at the estimate if unset." (from #1159) is no longer accurate: the amount is what remains after fees at feerate, which can be a pending splice's RBF minimum, and a fee bump lowers it. Maybe update it to say that?

[jkczyz]

Done.

[shubhamkmr04]

@tnull On the CI failure: I don't think it's a flake. splice_rounds_discarded_while_the_channel_is_listed_fail_at_close was written for the old behavior, where the splice-in left no change and the bump had to pay from a fresh coin. With manually selected inputs, the bump now adjusts the first round in place and reuses its wallet input (ec9c…:0 in the log), so the assert fails every time. The test needs updating for the new behavior.

[jkczyz]

@tnull On the CI failure: I don't think it's a flake. splice_rounds_discarded_while_the_channel_is_listed_fail_at_close was written for the old behavior, where the splice-in left no change and the bump had to pay from a fresh coin. With manually selected inputs, the bump now adjusts the first round in place and reuses its wallet input (ec9c…:0 in the log), so the assert fails every time. The test needs updating for the new behavior.

Yup, updating the test.

[jkczyz]

Seems Leo's suggestion here might make sense to add in this PR?: #1103 (comment)

Claude ended up writing this, but maybe best to leave for a separate PR given we want to add splice_out_with_all.

[jkczyz]

Seems Leo's suggestion here might make sense to add in this PR?: #1103 (comment)

Claude ended up writing this, but maybe best to leave for a separate PR given we want to add splice_out_with_all.

Dropped in favor of including it in #1181.

[tankyleo]

Have we considered that while the anchor reserves sits on the output of the unconfirmed splice transaction, it cannot be used to help confirm force-closes on other channels ? This is particularly relevant for zero-fee commitment channels; for those channels a failure to select any confirmed output directly leads to a failure to propagate the force-close.

I am thinking the fundamental issue is that none of the reserve sits in independent, locked utxos. A separate set of utxos 100% dedicated to the anchor reserve, and that doesn't get touched when a user calls splice_in_with_all would be useful. Issue #1198 proposes an approach for how to build this set of utxos.

[tnull]

Mostly looks good, claude:

1. An existing test now covers less, and its doc comment is stale (tests/integration_tests_rust.rs:4953-4966, splice_rounds_discarded_while_the_channel_is_listed_fail_at_close).
   • The test used to bump from a fresh coin so that "the two rounds are contributions of their own". It asserted that the bump shared only the channel funding output with the first round,
     and that RECLAIMED_ADDRESSES == 2, i.e. each DiscardFunding returned its own round's contribution.
   • The PR changes it so the bump reuses the first round's inputs and asserts == 1. The case where each discard event returns a separate contribution is no longer tested, yet the doc
     comment still says the test "bumps the round's fee from another coin, so the two rounds are contributions of their own".
   • Suggested fix: keep the original scenario by bumping above the first round's maximum feerate after a new coin confirms. That now goes through the rebuild path and gives the bump its
     own inputs. Alternatively, make the first round a fixed-amount splice_in. At minimum, update the doc comment.
2. The commit message promises more than the code delivers (main commit). It says the reserve mark is dropped "if the contribution cannot be built or LDK rejects it". But if
   funding_contributed fails with ContributionPending via QuiescentError::DoNothing, LDK sends no DiscardFunding. The address then stays marked used until restart. The only cost is a
   skipped address (a small address-gap/privacy issue), so either soften the wording or release the address explicitly on that error.
3. A fee bump can now pull in newly confirmed coins. When a bump goes past the earlier maximum feerate, the rebuild sweeps in any coins confirmed since the splice. A user who asked for a
   higher fee ends up putting more money into the channel. It's documented in the rustdoc and CHANGELOG, so this is a design point for discussion rather than a bug. Failing with an error,
   or only adding new coins when the original inputs can't cover the fee, would be the more conservative choice.
4. A tiny value_added isn't caught on the in-place bump path. The below-dust check in build_splice_in_with_all_contribution only runs when the contribution is built or rebuilt. If
   rbf_prior_contribution succeeds by adjusting the existing contribution, value_added could drop below dust without a check. This is a very unlikely edge case and LDK may reject it
   anyway.

[jkczyz]

Fixed 1 and 4. The other two are either unreachable or intended / will need to be fixed once we bump the LDK dependency. Claude responses inline.

Mostly looks good, claude:

1. An existing test now covers less, and its doc comment is stale (tests/integration_tests_rust.rs:4953-4966, splice_rounds_discarded_while_the_channel_is_listed_fail_at_close).

   • The test used to bump from a fresh coin so that "the two rounds are contributions of their own". It asserted that the bump shared only the channel funding output with the first round,
     and that RECLAIMED_ADDRESSES == 2, i.e. each DiscardFunding returned its own round's contribution.
   • The PR changes it so the bump reuses the first round's inputs and asserts == 1. The case where each discard event returns a separate contribution is no longer tested, yet the doc
     comment still says the test "bumps the round's fee from another coin, so the two rounds are contributions of their own".
   • Suggested fix: keep the original scenario by bumping above the first round's maximum feerate after a new coin confirms. That now goes through the rebuild path and gives the bump its
     own inputs. Alternatively, make the first round a fixed-amount splice_in. At minimum, update the doc comment.

Fixed, the first way: a new coin confirms, then the bump asks for 1000 sat/kwu, past the first round's maximum of 375, so it is rebuilt from the round's inputs and the coin. The test checks the bump spends exactly those, and both discards return a contribution again (RECLAIMED_ADDRESSES == 2). The doc comment describes the new scenario.

2. The commit message promises more than the code delivers (main commit). It says the reserve mark is dropped "if the contribution cannot be built or LDK rejects it". But if
   funding_contributed fails with ContributionPending via QuiescentError::DoNothing, LDK sends no DiscardFunding. The address then stays marked used until restart. The only cost is a
   skipped address (a small address-gap/privacy issue), so either soften the wording or release the address explicitly on that error.

I don't think that case is reachable. funding_contributed returns DoNothing only when unique_contributions() is None, i.e. the contribution reserved nothing of its own; with anything reserved it returns DiscardFunding { inputs, outputs } and the manager pushes the event, as it does on the ChannelUnavailable paths. A contribution from splice_in_with_all_contribution pays the reserve to an address reserved for it, which is not among the pending round's outputs and so counts among reserved_outputs(); without a reserve no address is marked. The in-place bump through rbf_prior_contribution marks nothing either. So whenever an address was marked, a rejection comes with the event that clears it. Happy to spell that out in the message if you'd like.

3. A fee bump can now pull in newly confirmed coins. When a bump goes past the earlier maximum feerate, the rebuild sweeps in any coins confirmed since the splice. A user who asked for a
   higher fee ends up putting more money into the channel. It's documented in the rustdoc and CHANGELOG, so this is a design point for discussion rather than a bug. Failing with an error,
   or only adding new coins when the original inputs can't cover the fee, would be the more conservative choice.

This is mostly https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/issues/5282. LDK checks the in-place adjustment against the first round's max_feerate before storing the bump's, so today any bump more than 1.5x above the first round's estimate fails and takes the rebuild. Once https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/pulls/5297 is in the pin, the check uses the bump's own maximum, which bump_channel_funding_fee always sets above its target. The rebuild is then reached only when the pending inputs can't cover the fee, or would leave less than dust to add, which is your second option. Until then I'd keep the rebuild rather than fail: without it a with-all splice can't be bumped past that ceiling at all. Added a TODO where splice_in_with_all_bump_fee_above_max_feerate_adds_new_coins and the discard test bump: both rely on the stale check, fail once the pin moves, and say how to keep the rebuild under test.

(Follow-up for the pin bump: both tests reach the rebuild only through the stale-maximum check; after https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/pulls/5297 they adjust in place and the discard test's reclaim count drops back to one. Give them a bump the pending inputs can't cover, or return the discard test to a fixed-amount first round.)

4. A tiny value_added isn't caught on the in-place bump path. The below-dust check in build_splice_in_with_all_contribution only runs when the contribution is built or rebuilt. If
   rbf_prior_contribution succeeds by adjusting the existing contribution, value_added could drop below dust without a check. This is a very unlikely edge case and LDK may reject it
   anyway.

Fixed. LDK's adjustment of manually selected inputs only checks that the fee is covered, and funding_contributed only checks the splice-out maximum, so a sub-dust amount would have gone through. bump_channel_funding_fee now treats an adjusted contribution whose amount added fell below dust like ManuallySelectedInputsInsufficient and rebuilds it, so the dust check in build_splice_in_with_all_contribution applies: without new coins the bump fails with InsufficientFunds and the pending splice stands. splice_in_with_all_bump_fee_leaving_dust_fails covers it.

[tnull]

This needs a rebase, also feel free to squash.

[jkczyz]

This needs a rebase, also feel free to squash.

Done.

[tankyleo]

Discussed offline, utxo splits are out of scope for this PR

[tnull]

Looks good I think, just one comment on code hygiene

[tnull]

Hmm, if we now refactor to have release_outputs, can we inline these calls rather than having both the old and new APIs available. Same with reserve_internal_address above.

While LLMs don't get confused, I personally dislike the growing number of tiny helper methods that all do something slightly different. IMO, it almost always makes sense to inline helper methods less than 5-7 LoC as it's easier to spot at the callsite what actually happens rather than having to jump around/remembering a bazillion tiny methods with slightly different semantics.

[jkczyz]

Had Claude rename get_new_internal_address to next_unused_internal_address to avoid a single pub(crate) method taking a bool.

🤖 Fixed for release_outputs: cancel_tx and release_outputs_inner are gone, the loop is inlined, and the DiscardFunding handler passes its outputs straight to it. cancel_tx did the same thing with a different argument type, so dropping it lost nothing. get_new_internal_address and reserve_internal_address do different things, one marks the address used and the other doesn't, so collapsing them means one method with a bool switch, and get_new_internal_address(true) at the call site says less than reserve_internal_address() does. I kept the two names over the private method that takes the flag, and renamed get_new_internal_address to next_unused_internal_address, since repeated calls return the same address until it is used or reserved.

[tnull]

Feel free to squash!