Repository navigation
Count the funding fee in anchor reserve checks for channel opens - #1204
shubhamkmr04 wants to merge 3 commits into
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
8b47d00 to
d0a665b
Compare
| con_cm.connect_peer_if_necessary(con_node_id, con_addr).await | ||
| })?; | ||
|
|
||
| let cur_anchor_reserve_sats = |
There was a problem hiding this comment.
Hmm, it's a bit weird to duplicate this again after we somewhat recently moved the related logic into check_sufficient_onchain_funds. Can you maybe in a prefactor commit adjust that, eg something like
fn check_sufficient_onchain_funds(
&self, amount_sats: u64, peer_node_id: &PublicKey, for_new_channel: bool,
fee_rate: FeeRate,
) -> Result<(), Error> {
let cur_anchor_reserve_sats = total_anchor_channels_reserve_sats(..);
// existing spendable check, unchanged
if for_new_channel {
let new_channel_reserve = self.new_channel_anchor_reserve_sats(peer_node_id)?;
let total_reserve = cur_anchor_reserve_sats.saturating_add(new_channel_reserve);
// existing amount + reserve check, unchanged
let max_amount = self.wallet.get_max_funding_amount(total_reserve, fee_rate)?;
if amount_sats > max_amount { log; return Err(Error::InsufficientFunds); }
}
Ok(())
}and then use that method rather than re-introducing/duplicating the anchor-reserve-check logic?
There was a problem hiding this comment.
Could you please make the change to check_sufficient_onchain_funds in a prefactor commit and then simply use the new shape in the following commits?
There was a problem hiding this comment.
I'd previously kept this in a single commit, now split: 759dae1 only changes check_sufficient_onchain_funds, and the following commits use it.
d0a665b to
5b01338
Compare
|
Hi @tnull , could you please take a look again? Thanks! |
Yes, was just looking at it. Excuse the delay. |
5b01338 to
cfb157c
Compare
| amount_sats, | ||
| &counterparty_node_id, | ||
| false, | ||
| min_feerate, |
There was a problem hiding this comment.
It's odd to pass this in if it's not going to be used. Rather than having a bool and a FeeRate, can we just merge the two into a new_channel_with_funding_fee_rate: Option<FeeRate> parameter or similar?
There was a problem hiding this comment.
Merged them into new_channel_with_funding_fee_rate: Option<FeeRate>. The splice-in calls now pass None.
| let max_amount = self | ||
| .wallet | ||
| .get_max_funding_amount(total_anchor_reserve_sats, funding_fee_rate) | ||
| .unwrap_or(0); |
There was a problem hiding this comment.
Not sure if we should rather bubble up the error here?
There was a problem hiding this comment.
handle_event returns (), so there's nothing to bubble up to. Now we log the error and return, like the other failures in that handler, instead of treating it as a maximum of 0.
Opening a channel with an exact amount only checked that our on-chain funds cover the amount plus the anchor reserve, leaving out the fee of the funding transaction, which is paid from the same funds. Since the fee rate can now be chosen freely (lightningdevkit#1099), a high one could spend part of the reserve, or leave too little to build the funding transaction once the counterparty had accepted the channel. `check_sufficient_onchain_funds` now takes the fee rate of a new channel's funding transaction as an `Option` in place of its `for_new_channel` flag. For a new channel, it also checks that the amount doesn't exceed what opening the channel with all funds would add at that fee rate, which already accounts for the fee and the reserve, and fails with `InsufficientFunds` otherwise. `open_channel_inner` resolves the fee rate once for both funding amounts. Part of lightningdevkit#1166. Developed with assistance from Claude Code.
Opening a channel with an exact amount now fails with `InsufficientFunds` if the amount exceeds what the corresponding `_with_all` variant would fund at the same fee rate. Say so in the docs of the exact-amount variants and in the CHANGELOG, and test that an open whose fee would come out of the anchor reserve fails, while the same amount at a lower fee rate opens and keeps the reserve on-chain. Part of lightningdevkit#1166. Developed with assistance from Claude Code.
When acting as an LSPS2 service, we open a JIT channel once its payment arrives, after checking that our on-chain funds cover the channel amount plus the anchor reserve. As for exact-amount opens, this left out the fee of the funding transaction, which is built later at our fee estimate and paid from the same funds, so it could spend part of the reserve. Also check that the channel amount doesn't exceed what funding with all our funds would add at that estimate, and don't open the channel otherwise, or if that maximum can't be determined. To do so, the LSPS2 service now has access to the fee estimator. Part of lightningdevkit#1166. Developed with assistance from Claude Code.
cfb157c to
8c4cafb
Compare
Part of #1166.
When opening a channel with an exact amount, we checked that our on-chain funds cover the amount plus the anchor reserve, but not the fee of the funding transaction, which is paid from the same funds. Since #1099 lets the caller choose the fee rate, a high one could spend part of the reserve. The LSPS2 service's check before opening a JIT channel has the same gap.
Changes
open_channelvariants now also fail withInsufficientFundsif the amount exceeds what the corresponding_with_allvariant would fund at the same fee rate. That'sWallet::get_max_funding_amount, which already deducts the fee and the anchor reserves, including the new channel's. Previously, an amount that fit next to the reserve but not next to the fee was accepted. The funding transaction then either spent part of the reserve or, without a reserve, failed to build after the counterparty had accepted the channel.ChannelFundingestimate, which is the rate the funding transaction pays. The service gets the fee estimator for this.The maximum assumes all UTXOs are spent, so near the limit it's slightly conservative for wallets with many small UTXOs.
Testing
open_channel_fee_does_not_spend_anchor_reserve: the wallet holds 1,000,000 sats. Opening 970,000 at 100 sat/vB fails withInsufficientFunds, because the ~15,250 sat fee would leave only 14,750 for the 25,000 reserve. The same amount at the regtest estimate opens and keeps the reserve on-chain. Onmain, the first open succeeds.lsps2_service_funding_fee_does_not_spend_anchor_reserve: the service holds the JIT channel amount plus both anchor reserves plus 100 sats. The existing check passes, but the funding fee doesn't fit, so the service doesn't open the channel and keeps its funds.Splices are left for a follow-up once #1176 lands, since it reworks the same code. The follow-up covers exact
splice_in, and the reserve check when a fee bump falls back to the wallet, as discussed in #1181.This change was developed with the assistance of Claude Code.