Zero-fee commitments support#660
Conversation
|
👋 Thanks for assigning @benthecarman 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
|
🔔 3rd Reminder Hey @tnull @benthecarman! This PR has been waiting for your review. |
| rpc_password.clone(), | ||
| )); | ||
|
|
||
| let node_version = api_client.get_node_version().await.map_err(|e| { |
There was a problem hiding this comment.
Should we add a time out to this to be safe?
| rpc_password, | ||
| )); | ||
|
|
||
| let node_version = api_client.get_node_version().await.map_err(|e| { |
| user_config.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx = | ||
| config.anchor_channels_config.is_some(); | ||
| user_config.channel_handshake_config.negotiate_anchor_zero_fee_commitments = | ||
| config.anchor_channels_config.is_some(); |
There was a problem hiding this comment.
Should this be a setting in anchor_channels_config? Atm we basically break users who are using anchor channels but don't have chain source that supports 0 fee.
There was a problem hiding this comment.
We had a discussion about this last fall, and concluded we would rather not add an explicit config flag for 0FC channels.
I think we are safe to make it the default for users of anchor channels. The following chain sources are supported:
- Esplora: blockstream-electrs and mempool-electrs
- Electrum: fulcrum, electrumX, and romanz-electrs
- Bitcoin RPC/Rest: bitcoind v29+
blocktream-electrs and mempool-electrs electrum are not supported yet. For public electrum, I was able to launch a node on this branch against these endpoints from the list in sparrow wallet:
ssl://electrum.diynodes.com:50022
tcp://fulcrum.sethforprivacy.com:50001
ssl://frigate.2140.dev:50002
lu.ke, emzy.de, and bitaroo.net are all complaints from rustls.
|
needs rebase too |
|
🔔 4th Reminder Hey @tnull! This PR has been waiting for your review. |
tankyleo
force-pushed
the
25-10-0fc-channel-config
branch
from
4e72af0 to
954a150
Compare
|
Rebased, and added two fixups, one to address Ben's comments, the other to create a unique error type for "we could connect to the chain source, but it is not supported". |
|
🔔 5th Reminder Hey @tnull @benthecarman! This PR has been waiting for your review. |
| match timeout_fut.await { | ||
| Ok(res) => match res { | ||
| Ok(result) => { | ||
| if result.contains(r#""package_msg":"success""#) { |
There was a problem hiding this comment.
Could we keep this response structured instead of converting it to a string and matching with contains?
There was a problem hiding this comment.
thanks for the review, I made some attempts at adding some structure, for now I prefer to keep things simple. The additional structure didn't seem worth it, especially given that the interface for this RPC call could change in the future (it is marked as unstable).
| } | ||
| } | ||
|
|
||
| fn sort_parents_child_package_topologically(txs: &mut [Transaction]) { |
There was a problem hiding this comment.
Would it be worth adding a debug assertion here that, after sorting, the last transaction spends every other transaction in the package?
There was a problem hiding this comment.
thanks for the input hmm I don't think this should be added here: we'd be checking that the rust-lightning BroadcasterInterface actually gave us a single child with its parents (as it says it does), rather than checking that the current function here does its job properly.
| let spendable_amount_sats = | ||
| self.wallet.get_spendable_amount_sats(cur_anchor_reserve_sats).unwrap_or(0); | ||
| let anchor_channel = init_features.requires_anchors_zero_fee_htlc_tx() | ||
| || init_features.requires_anchor_zero_fee_commitments(); |
There was a problem hiding this comment.
I may be missing some context, but wouldsupports_* be worth considering for this check?
There was a problem hiding this comment.
it's a good question thank you, I went with the pre-existing requires variant. Can you open a PR for this and we can continue there ?
|
🔔 1st Reminder Hey @tnull @benthecarman! This PR has been waiting for your review. |
`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.
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.
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 also bump the Bitcoin Core version used in kotlin and python tests to support ephemeral dust.
Do this as early as possible during startup, only if `anchor_channels_config` is set.
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
954a150 to
42cba15
Compare
|
Squashed, rebased, and added a compatibility note in the changelog to warn people anchor channels now require a chain source that supports |
|
🔔 6th Reminder Hey @tnull @benthecarman! This PR has been waiting for your review. |
|
🔔 7th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 8th Reminder Hey @tnull! This PR has been waiting for your review. |
|
needs rebase |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Excuse the considerable delay here, did a first pass.
| txs_to_broadcast.len() | ||
| ); | ||
| } | ||
| let count: usize = unconfirmed_outbound_txids |
There was a problem hiding this comment.
There was a problem hiding this comment.
Makes sense, though in #888 we'll probably introduce a strong type for a package anyways, which should get rid of that API misuse case.
Sounds good !
Do you think it's worth doing the same change in LDK's interface, too? I.e., introduce a dedicated TransactionPackage type that disallows misuse?
Do you mean introducing a separate method to BroadcasterInterface ? ie
pub trait BroadcasterInterface {
fn broadcast_transactions(&self, txs: &[(&Transaction, TransactionType)]);
fn broadcast_package(&self, package: (&TransactionPackage, TransactionType));
}I do agree the current BroadcasterInterface API is prone to misuse. See lightningdevkit/rust-lightning#4173 for some context.
| } | ||
| } | ||
|
|
||
| fn sort_parents_child_package_topologically(txs: &mut [Transaction]) { |
There was a problem hiding this comment.
Hmm, rather than doing this here, maybe we should already do the change just mentioned in this PR and create a dedicated TransactionPackage type that enforces topological sorting upon creation?
There was a problem hiding this comment.
we should already do the change just mentioned in this PR
Can you clarify this part ? Thank you I don't quite follow
create a dedicated TransactionPackage type that enforces topological sorting upon creation
Yes overall makes sense to me. I'm thinking we create an enum, constructed at the current callsite of sort_parents_child_package_topologically , with one Singleton variant that contains a single transaction, and one Package variant that contains a child-with-parents package, itself topologically sorted on construction.
At the process_broadcast_package of each chain source, we match on the variant instead of matching on package.len() > 1 to determine whether we should use submit_package to broadcast the transaction.
| } | ||
|
|
||
| fn sort_parents_child_package_topologically(txs: &mut [Transaction]) { | ||
| // LDK multi-transaction broadcasts are one child plus its direct parents, and the |
There was a problem hiding this comment.
Do we see a scenario where this assumption could ever fail, i.e., where we change LDK's behavior (quietly)? Maybe it would at least be worth somehow detecting a counterexample and debug_asserting?
There was a problem hiding this comment.
I just made another pass on the LDK broadcast_transactions callsites on current 0.3, and at the moment I don't see this assumption failing: we always explicitly construct the array of transactions ie [tx1, tx2], no pushes or appends.
Certainly we can add a debug assert here thank you.
| if txs.len() < 2 { | ||
| return; | ||
| } | ||
| let mut child_pos = txs.len() - 1; |
There was a problem hiding this comment.
This seems equivalent and cleaner (?):
diff --git a/src/tx_broadcaster.rs b/src/tx_broadcaster.rs
index 01d23782..00000000 100644
--- a/src/tx_broadcaster.rs
+++ b/src/tx_broadcaster.rs
@@ -57,37 +57,19 @@ fn sort_parents_child_package_topologically(txs: &mut [Transaction]) {
if txs.len() < 2 {
return;
}
- let mut child_pos = txs.len() - 1;
- let mut pos = txs.len() - 1;
- 'outer: while pos > 0 {
- let txid_a = txs[pos - 1].compute_txid();
- for txid in txs[pos].input.iter().map(|input| input.previous_output.txid) {
- if txid == txid_a {
- child_pos = pos;
- break 'outer;
- }
- }
- let txid_b = txs[pos].compute_txid();
- for txid in txs[pos - 1].input.iter().map(|input| input.previous_output.txid) {
- if txid == txid_b {
- child_pos = pos - 1;
- break 'outer;
- }
- }
- if pos == 2 {
- pos = 1;
- } else {
- pos = pos.saturating_sub(2);
- }
- }
- debug_assert!(pos != 0);
- txs.swap(child_pos, txs.len() - 1);
+
+ let package_txids = txs.iter().map(Transaction::compute_txid).collect::<Vec<_>>();
+ let spends_package_tx = |tx: &Transaction| {
+ tx.input
+ .iter()
+ .any(|input| package_txids.contains(&input.previous_output.txid))
+ };
+
+ debug_assert!(txs.iter().any(spends_package_tx));
+ txs.sort_by_key(spends_package_tx);
}There was a problem hiding this comment.
Yes thanks initially I wanted to avoid hashing every transaction, but we can expect the number of transactions here to be small, so I am fine to hash all of them.
This also allows us to easily debug assert the multi-transaction == single-child-with-parents assumption.
|
|
||
| HOST_PLATFORM="$(rustc --version --verbose | grep "host:" | awk '{ print $2 }')" | ||
| ELECTRS_GIT_REPO="https://github.com/tankyleo/blockstream-electrs.git" | ||
| ELECTRS_TAG="2026-05-26-electrum-submit-package" |
There was a problem hiding this comment.
If we do this, we should use a specific commit revision, not point to a general branch.
There was a problem hiding this comment.
This is indeed my intention, and I believe the script currently does this.
See the tag here: https://github.com/tankyleo/blockstream-electrs/releases/tag/2026-05-26-electrum-submit-package
| ## Compatibility Notes | ||
| - Pending JIT-channel payments created before upgrading may fail after upgrade because the | ||
| prior LSPS2 fee-limit state stored in `PaymentKind::Bolt11Jit` is not migrated. | ||
| - Usage of anchor channels now requires an Esplora or Electrum chain source that supports |
There was a problem hiding this comment.
Why do we require submitpackage for all anchor channels now? Technically we only need it for 0fc channels, no?
There was a problem hiding this comment.
Further above we decided against adding a separate knob dedicated to 0FC channels. This means that when we turn on anchor channels, 0FC channels can be negotiated against any peer that supports them, so the corresponding chain source should support submitpackage.
| match timeout_fut.await { | ||
| Ok(res) => match res { | ||
| Ok(result) => { | ||
| if result.contains(r#""package_msg":"success""#) { |
There was a problem hiding this comment.
Hmm, rather than matching on the string, please rather properly parse the results.
| rpc_client | ||
| .call_method::<serde_json::Value>("submitpackage", &[package_json]) | ||
| .await | ||
| .map(|value| value.to_string()) |
There was a problem hiding this comment.
Let's please parse the result into a proper type to avoid having to deal with raw strings (if we need the data in there).
| ); | ||
| }, | ||
| } | ||
| } else if package.len() > 1 { |
There was a problem hiding this comment.
It seems we're duplicating a lot of the same handling/logging logic here (and possibly elsewhere). Do we see a chance to DRY it up? (possibly in a prefactor commit)
| let required_funds_sats = channel_amount_sats | ||
| + self.config.anchor_channels_config.as_ref().map_or(0, |c| { | ||
| if init_features.requires_anchors_zero_fee_htlc_tx() | ||
| if anchor_channel |
There was a problem hiding this comment.
We recently merged #841
Please validate that the logic for determining ReserveType there also still works for 0fc channels / whether it needs to be updated to account for them.
5 participants
No description provided.