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
|
Discussed offline, I brought back |
Hmm, it seems that we could get users to upgrade (although it also doesn't seem seamless to them). But more generally I still find it a bit unfortunate to require |
Agreed, we can yes, I'll make the diff tomorrow. |
`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.
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.
tankyleo
force-pushed
the
25-10-0fc-channel-config
branch
from
7dc92d3 to
0726159
Compare
| } | ||
| } | ||
|
|
||
| fn supports_anchor_channel_type(init_features: &InitFeatures) -> bool { |
There was a problem hiding this comment.
Codex:
- P2: /home/tnull/worktrees/ldk-node/pr-660/src/lib.rs:219 treats a peer advertising option_zero_fee_commitments as requiring an anchor reserve even when local enable_zero_fee_commitments is false. But /home/tnull/worktrees/ldk-node/pr-660/src/config.rs:415 only enables 0FC negotiation when
that flag is true. A zero-fee-only peer would fall back to static remotekey, while open_channel/open_channel_with_all and LSPS2 still reserve per_channel_reserve_sats, potentially rejecting valid opens as InsufficientFunds.
There was a problem hiding this comment.
thanks addressed below
|
|
||
| match timeout_fut.await { | ||
| Ok(Ok(Ok(_))) => Ok(()), | ||
| Ok(Ok(Err(electrum_client::Error::AllAttemptsErrored(e)))) => { |
There was a problem hiding this comment.
Codex:
- P3: /home/tnull/worktrees/ldk-node/pr-660/src/chain/electrum.rs:338 only maps AllAttemptsErrored to ChainSourceNotSupported. electrum-client returns server protocol errors directly for unsupported methods, so an Electrum server without blockchain.transaction.broadcast_package will likely
surface as ConnectionFailed instead of the new ChainSourceNotSupported.
There was a problem hiding this comment.
some context: the current AllAttemptsErrored variant works when using the existing live blockstream electrum endpoints (ssl://electrum.blockstream.info:50002, and ssl://blockstream.info:700), so I kept it (it's how I picked that variant in the first place). I now also map the Protocol error to ChainSourceNotSupported, which codex tells me will address lack of support for submitpackage in mempool-electrs, romanz-electrs, Fulcrum, and ElectrumX
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
0726159 to
ad147c7
Compare
|
Noted CI, will address on the next rebase |
6 participants
No description provided.