Zero-fee commitments support#660
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
| /// `option_anchor_zero_fee_commitments`. All the caveats and warnings in | ||
| /// [`AnchorChannelsConfig`] still apply. | ||
| /// [`AnchorChannelsConfig`]: Config::anchor_channels_config | ||
| pub enable_zero_fee_commitments: bool, |
There was a problem hiding this comment.
I don't think we'll wan to add a new flag here that's probably hard to understand for the user? Rather, shouldn't we enable this for the user based on our current 'trust model settings' here?
Also, from these docs it's very unclear what this setting even does, when the user would want to enable it, what drawbacks it has, etc
There was a problem hiding this comment.
FWIW, thinking about it again it seems that we should never set negotiate_anchor_zero_fee_commitments until we're positive our chain sources support submitpackage/TRUC, no? And once we are positive, we would always set it?
There was a problem hiding this comment.
Rather, shouldn't we enable this for the user based on our current 'trust model settings' here?
Don't quite follow here could you expand ? I think 0FC channels merit an explicit setting somewhere rather than derived from trust model settings.
Also, from these docs it's very unclear what this setting even does, when the user would want to enable it, what drawbacks it has, etc
Yes will expand
There was a problem hiding this comment.
Don't quite follow here could you expand ? I think 0FC channels merit an explicit setting somewhere rather than derived from trust model settings.
Why, what do they fundamentally change for the user compared to our three current modes (fully trusted/keep 0-reserve, still try to claim/keep X reserve, try to claim)? Keep in mind that communicating these three modes to the user is already very hard, they always have a very hard time understanding what this means. Now, how would we communicate any changed assumptions for 0FC here? If we already trust our counterparty already, wouldn't we always want to enable 0FC for the UX improvements?
There was a problem hiding this comment.
Why, what do they fundamentally change for the user compared to our three current modes (fully trusted/keep 0-reserve, still try to claim/keep X reserve, try to claim)?
Let me see I don't think they change anything ? Whether to enable or disable 0FC channels is orthogonal to these modes ie trusted_peers_no_reserve and per_channel_reserve_sats should have no influence on whether we enable 0FC channels (only that per_channel_reserve_sats should be set to some value). I suspect you don't agree :)
If we already trust our counterparty already, wouldn't we always want to enable 0FC for the UX improvements?
It seems to me trusting our counterparty -> keeping 0 reserve is orthogonal to whether the user wants to enable 0FC channels ? for example a user trusts their counterparty, but wants to wait for greater adoption of Core v29+ before using 0FC channels.
tankyleo
force-pushed
the
25-10-0fc-channel-config
branch
from
cb1cdf9 to
c874049
Compare
|
Marked as draft: I think we should wait for electrum and esplora submit package support before merging this PR. |
tankyleo
force-pushed
the
25-10-0fc-channel-config
branch
from
c874049 to
ef3ba7a
Compare
|
Successfully opened some 0FC channels, made payments, and force closed them with the esplora diff in this branch. https://mutinynet.com/tx/508a954d85f5b7daf224a2fdc54ea6de9a26c0f62f7d58284bf61c3cdfd346e6 |
tankyleo
force-pushed
the
25-10-0fc-channel-config
branch
from
ef3ba7a to
3ebd017
Compare
tankyleo
changed the title
AnchorChannelsConfig::enable_zero_fee_commitments
tankyleo
force-pushed
the
25-10-0fc-channel-config
branch
from
3ebd017 to
eda13d4
Compare
tankyleo
force-pushed
the
25-10-0fc-channel-config
branch
4 times, most recently
from
e136f33 to
d4a2a04
Compare
tankyleo
force-pushed
the
25-10-0fc-channel-config
branch
9 times, most recently
from
771f45b to
a7c7911
Compare
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
One comment, otherwise LGTM! Unfortunately needs another tiny rebase by now due to minor conflicts in the test files.
| are still open or unresolved. You must close and resolve all anchor \ | ||
| channels before disabling anchor channels." | ||
| ); | ||
| return Err(Error::ChannelConfigUpdateFailed); |
There was a problem hiding this comment.
Hmm, I really dislike the pattern of letting users configure something that we end up erroring on. If we think we need to be more strict now, we should probably find an API that doesn't allow the user to setup an 'unsafe' configuration in the first place.
In LDK we previously discussed making anchors mandatory. Should we rather trailblaze here, and simply make AnchorsChannelConfig non-optional?
If we do this, maybe we should make negotiating 0fc optional though, if only because of the chain source requirements, and we want to maintain backwards compat for a while?
There was a problem hiding this comment.
All makes sense to me, will prepare the fixups.
we want to maintain backwards compat for a while
A clarification I thought ldk-node does not allow downgrades ?
There was a problem hiding this comment.
@benthecarman does the above sound good to you ? First make anchors required in this release, then next release we turn on 0FC channels by default ?
There was a problem hiding this comment.
by anchors we just mean that they should always be available for negotiation, we will still negotiate down to legacy channels if needed.
There was a problem hiding this comment.
Done below:
AnchorChannelConfigis now required.- Negotiating 0fc channels is now optional via
AnchorChannelsConfig.enable_zero_fee_commitments
There was a problem hiding this comment.
A clarification I thought ldk-node does not allow downgrades ?
Yes, backwards compatible is something that allows for upgrades. Forwards compatible is something if it allows for downgrades. LDK Node guarantees the former, and hopefully eventually gets to the point where we can also guarantee the latter.
by anchors we just mean that they should always be available for negotiation, we will still negotiate down to legacy channels if needed.
FWIW, that's fine for now, but I think we discussed dropping the auto-reneg in LDK.
tankyleo
force-pushed
the
25-10-0fc-channel-config
branch
from
9f3544e to
1775010
Compare
I took the liberty to edit the commits directly. So I avoid rebasing for now to make this last push easier to review with a |
tankyleo
force-pushed
the
25-10-0fc-channel-config
branch
from
1775010 to
9b38158
Compare
|
Rebase on the same base commit: fix tests |
We only do this for rust files, and leave bat files untouched. Use git show --ignore-cr-at-eol to check that this commit has no other edits.
`BroadcasterInterface::broadcast_transactions` requires that any passed vector containing multiple transactions must be a single child together with its parents. We will lean on this contract in upcoming commits, so here we fix a case where we broke this contract.
In an upcoming commit, we will fix `check_sufficient_funds_for_channel` to check that we have on-chain funds to cover the anchor reserve for an additional anchor channel in the validation of outbound channel opens. Before we do this, we stop using this function to check that any splice-ins leave enough on-chain anchor reserves. This function keeps an anchor reserve for an additional anchor channel on top of the existing set of anchor channels, but after splice-ins, our anchor reserve only needs to cover the existing set of anchor channels.
When we are preparing to open a channel to a peer, we should reserve onchain funds for an anchor channel when the peer's init features signals anchor channels as optional, as channel negotiation with such a peer can result in an anchor channel. Tests written with codex.
|
I pushed this tag: https://github.com/tankyleo/ldk-node/releases/tag/2026-07-01-0620-0fc-channels It points to commit 9b38158, the top commit of this branch right before the rebase onto the new main branch commit. |
tankyleo
force-pushed
the
25-10-0fc-channel-config
branch
from
9b38158 to
71c0473
Compare
We previously allowed users to disable anchor channels and drain their anchor reserve while still having anchor channels open or pending resolution. This was acceptable for keyed anchor channels, as the commitment transaction therein still contained some fees, and had some chance of getting mined into a block without any anchor bumps. In upcoming commits, we will add support for 0FC channels, and their commitment transactions have zero fees and depend entirely on the anchor reserve to reach miners and get confirmed in a block. It is thus dangerous to disable anchor channels and drain the reserve after 0FC channels have been opened. Therefore, we make `AnchorChannelsConfig` required, and prevent this case from ever happening.
The patch adds support for the `broadcast_package` method added in electrum protocol v1.6. Upcoming commits will require this patch to pass CI.
The mempool/electrs docker image used in those tests only supports submitpackage via the esplora interface, not the electrum interface.
We bump the Bitcoin Core version used in kotlin and python tests to support ephemeral dust. This is required for 0FC channels.
In upcoming commits we will read this knob to determine whether to negotiate 0FC channels. For now, we make a best-effort attempt to make sure the configured chain source supports 0FC channels if this knob is set. Do this roundtrip at the same time we make a roundtrip to retrieve the feerates to keep startup as fast as possible.
Implementations of `BroadcasterInterface` cannot assume any topological ordering on the transactions received, so here we order the received transactions before adding them to the broadcast queue. Any consumers of the queue can now assume all transactions received to be topologically sorted. Codex wrote the tests.
These will be useful when we add support for broadcasting packages in an upcoming commit.
We rely on the `BroadcasterInterface` contract whereby any multi-transaction vector must be a single child and its parents, and must be broadcasted together as a package using `submitpackage`. In a prior commit, we added the guarantee that any packages received from the broadcast queue are already topologically sorted, and hence can be passed directly to the `submit_package` Bitcoin Core RPC.
tankyleo
force-pushed
the
25-10-0fc-channel-config
branch
from
71c0473 to
c8c2b88
Compare
|
Rebased on new main branch commit, and pushed a few more small fixes to the tests |
| Err(e) => self.log_broadcast_error(e, &[txid], &txs), | ||
| } | ||
| }, | ||
| 2.. => { |
There was a problem hiding this comment.
Codex:
High: Multi-transaction broadcasts now require package relay even when 0FC is disabled. At /home/tnull/workspace/ldk-node-pr-660/src/chain/mod.rs:521 the broadcaster drops transaction-type context, and all len >= 2 packages go through submitpackage in /home/tnull/workspace/ldk-node-pr-660/
src/chain/bitcoind.rs:630, /home/tnull/workspace/ldk-node-pr-660/src/chain/esplora.rs:463, and /home/tnull/workspace/ldk-node-pr-660/src/chain/electrum.rs:358. But package-support validation only runs when enable_zero_fee_commitments is true. Legacy anchor force-close bumps still emit
commitment+anchor transaction packages, so users with 0FC off and an Electrum/Esplora/bitcoind backend without package support can start successfully and then fail to broadcast the close-bump package. Either keep the old per-tx fallback for non-TRUC packages or require/validate package
support for all anchor channels.
There was a problem hiding this comment.
Either keep the old per-tx fallback for non-TRUC packages or require/validate package
support for all anchor channels.
Ah thank you good catch, hmm I need to decide which path is best
There was a problem hiding this comment.
I think Im going to go with "require support for all anchor channels"
| } | ||
| } | ||
|
|
||
| fn supports_anchor_channel_type(init_features: &InitFeatures) -> bool { |
There was a problem hiding this comment.
Codex:
Medium: Reserve prechecks treat any peer advertising 0FC as if the next channel will be anchor-like, even when local 0FC negotiation is disabled. /home/tnull/workspace/ldk-node-pr-660/src/lib.rs:219 returns true for supports_anchor_zero_fee_commitments(), and that drives outbound reserve
checks at /home/tnull/workspace/ldk-node-pr-660/src/lib.rs:1360 and LSPS2 checks at /home/tnull/workspace/ldk-node-pr-660/src/liquidity/service/lsps2.rs:455. For a 0FC-only peer with enable_zero_fee_commitments = false, LDK won’t negotiate 0FC, so this can falsely reject opens or reserve
funds for a static channel. The predicate should include local config: legacy anchors supported by peer, or 0FC supported by peer and locally enabled.
6 participants
No description provided.