Zero-fee commitments support by tankyleo · Pull Request #660 · lightningdevkit/ldk-node

tankyleo · 2025-10-13T13:14:22Z

No description provided.

ldk-reviews-bot · 2025-10-13T13:14:24Z

👋 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 · 2025-10-13T13:25:43Z

+	/// `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,


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

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?

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

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?

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 · 2025-10-24T06:06:59Z

Marked as draft: I think we should wait for electrum and esplora submit package support before merging this PR.

tankyleo · 2025-10-27T05:28:19Z

Successfully opened some 0FC channels, made payments, and force closed them with the esplora diff in this branch.

https://mutinynet.com/tx/508a954d85f5b7daf224a2fdc54ea6de9a26c0f62f7d58284bf61c3cdfd346e6

ldk-reviews-bot · 2026-06-22T00:01:39Z

🔔 8th Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

benthecarman · 2026-06-22T19:15:03Z

needs rebase

tnull

Excuse the considerable delay here, did a first pass.

tnull · 2026-06-23T09:06:13Z

-								txs_to_broadcast.len()
-							);
-						}
+					let count: usize = unconfirmed_outbound_txids


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.

@tankyleo 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?

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.

Yes, a separate method makes sense to me, though then maybe we'll want to change the original to broadcast_transaction: (Transaction, TransactionType)? FWIW, the &[(&Transaction, TransactionType)] seems like an over-eager optimization that has resulted in many cases that force us to unnecessarily clone just to massage it into the right type. Seems like a simple (Transaction, TransactionType) or (&Transaction, TransactionType) would be easier, and then the callsite could decide when it's really necessary to clone.

tnull · 2026-06-23T09:08:41Z

 	}
 }
+
+fn sort_parents_child_package_topologically(txs: &mut [Transaction]) {


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?

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.

Done below, let me know what you think of the direction

Can you clarify this part ? Thank you I don't quite follow

Sorry, was referring to #660 (comment)

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.

FWIW, that might not even be necessary, we could also just do a wrapper struct that wraps Vec<(Transaction, TransactionType)>, but offers constructors (single, package) that enforce the correct ordering. Could even offer From implementations to keep it easy to use at the callsites.

tnull · 2026-06-23T09:09:41Z

 }
+
+fn sort_parents_child_package_topologically(txs: &mut [Transaction]) {
+	// LDK multi-transaction broadcasts are one child plus its direct parents, and the


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?

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.

Added the debug assertions below

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.

Right, I wasn't saying it's violated right now, just seems like the kind of thing that we might change slightly for some reason, and then our logic here would silently be off if we don't add defensive measures to detect that something changed (as surely nobody will remember to re-review the logic here, esp. if there's no API break).

tnull · 2026-06-23T09:13:39Z

+	if txs.len() < 2 {
+		return;
+	}
+	let mut child_pos = txs.len() - 1;


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); }

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.

tnull · 2026-06-23T09:17:01Z

+
+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"


If we do this, we should use a specific commit revision, not point to a general branch.

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

Ah, sorry, I took 2026-05-26-electrum-submit-package to be a branch name, not a tag. Would still be good to pin the specific revision hash here rather than a tag.

Done below, the tag remains useful to do a git checkout --depth 1 --branch <tag_name>

tnull · 2026-06-23T09:26:02Z

 ## 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


Why do we require submitpackage for all anchor channels now? Technically we only need it for 0fc channels, no?

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.

tnull · 2026-06-23T09:27:30Z

+			match timeout_fut.await {
+				Ok(res) => match res {
+					Ok(result) => {
+						if result.contains(r#""package_msg":"success""#) {


Hmm, rather than matching on the string, please rather properly parse the results.

see the commit below, note I only parse the package_msg field, and leave the rest untouched as they are not needed besides logging.

tnull · 2026-06-23T09:28:28Z

+		rpc_client
+			.call_method::<serde_json::Value>("submitpackage", &[package_json])
+			.await
+			.map(|value| value.to_string())


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).

see the commit below, note I only parse the package_msg field, and leave the rest untouched as they are not needed besides logging.

tnull · 2026-06-23T09:30:31Z

 					);
 				},
 			}
+		} else if package.len() > 1 {


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)

agreed we should DRY it up, haven't finished this yet.

tnull · 2026-06-23T09:33:20Z

 				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


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.

thanks for the heads up, yes this will need an update for 0fc channels, I will do this on the next rebase here.

tankyleo · 2026-06-24T06:54:41Z

Let me know if I can rebase

tankyleo · 2026-06-24T06:58:16Z

+	}
+
+	async fn get_node_version_inner(rpc_client: Arc<RpcClient>) -> Result<u64, RpcClientError> {
+		rpc_client.call_method::<serde_json::Value>("getnetworkinfo", &[]).await.and_then(|value| {


Can also look into properly parsing the return value here like how we did for submitpackage let me know

tnull

Feel free to squash/rebase

tnull · 2026-06-24T09:54:21Z


 const BCAST_PACKAGE_QUEUE_SIZE: usize = 50;

+pub(crate) enum TransactionsToBroadcast {


See comment above, I think I'd prefer a simple struct TransactionBroadcast that wraps a Vec, enforcing the rules in constructors that could even be called via From implementations?

Done below, let me know if this matches what you had in mind

tnull · 2026-06-24T09:56:22Z

 	}
 }

+pub struct SubmitPackageResponse(String);


Should we even return the full response string here rather than ()?

See the esplora and electrum chain sources, I prefer to log the full response at trace even in the success case for debugging purposes. We get information on replaced transactions, transactions already in the mempool, and transactions freshly accepted into the mempool.

`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 bump the Bitcoin Core version used in kotlin and python tests to support ephemeral dust. This is required for 0FC channels.

Do this roundtrip at the same time we make a roundtrip to retrieve the feerates to keep startup as fast as possible.

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 · 2026-06-25T03:52:02Z

Rebased, squashed the previous fixups, added a few more

tnull · 2026-06-25T08:58:39Z

 	}

+	pub(super) async fn validate_zero_fee_commitments_support(&self) -> Result<(), Error> {
+		self.esplora_client.submit_package(&super::dummy_package(), None, None).await.map_err(


Especially if we already submitted this package before, couldn't the backend return an different error that we'd misinterpret as 'doesn't support submitpackage'? E.g., typical transaction broadcast errors are 'already known' or 'missing-inputs'. Why can't we ever hit them here?

tnull · 2026-06-25T09:04:53Z

 			.init_features;
-		let anchor_channel = init_features.requires_anchors_zero_fee_htlc_tx();
+		let anchor_channel = init_features.requires_anchors_zero_fee_htlc_tx()
+			|| init_features.requires_anchor_zero_fee_commitments();


Hmm, pre-existing, but it seems for init features we should be using supports_?

tankyleo requested a review from tnull October 13, 2025 13:14

tnull reviewed Oct 13, 2025

View reviewed changes

tankyleo force-pushed the 25-10-0fc-channel-config branch from cb1cdf9 to c874049 Compare October 24, 2025 06:05

tankyleo requested a review from tnull October 24, 2025 06:06

tankyleo marked this pull request as draft October 24, 2025 06:06

tankyleo force-pushed the 25-10-0fc-channel-config branch from c874049 to ef3ba7a Compare October 27, 2025 05:15

tankyleo force-pushed the 25-10-0fc-channel-config branch from ef3ba7a to 3ebd017 Compare October 29, 2025 08:56

tankyleo changed the title ~~Add AnchorChannelsConfig::enable_zero_fee_commitments~~ Zero-fee commitments support Oct 30, 2025

tankyleo self-assigned this Oct 30, 2025

tankyleo added this to Weekly Goals Oct 30, 2025

tankyleo force-pushed the 25-10-0fc-channel-config branch from 3ebd017 to eda13d4 Compare November 11, 2025 04:16

tankyleo removed this from Weekly Goals Nov 12, 2025

tankyleo force-pushed the 25-10-0fc-channel-config branch 4 times, most recently from e136f33 to d4a2a04 Compare November 19, 2025 19:11

tankyleo force-pushed the 25-10-0fc-channel-config branch 9 times, most recently from 771f45b to a7c7911 Compare May 29, 2026 00:36

tankyleo marked this pull request as ready for review May 29, 2026 01:08

tankyleo requested a review from benthecarman May 29, 2026 01:08

tnull reviewed Jun 23, 2026

View reviewed changes

tankyleo dismissed benthecarman’s stale review via 34eeb06 June 24, 2026 06:43

tankyleo force-pushed the 25-10-0fc-channel-config branch from 42cba15 to 34eeb06 Compare June 24, 2026 06:43

tankyleo requested a review from tnull June 24, 2026 06:54

tankyleo commented Jun 24, 2026

View reviewed changes

tnull reviewed Jun 24, 2026

View reviewed changes

tankyleo added 7 commits June 25, 2026 00:46

Use a patched blockstream-electrs in CI

7d43482

The patch adds support for the `broadcast_package` method added in electrum protocol v1.6. Upcoming commits will require this patch to pass CI.

f: Assert HEAD matches the expected rev

ce4abd0

Switch cln lnd and eclair interop tests to esplora

6f7e34a

The mempool/electrs docker image used in those tests only supports submitpackage via the esplora interface, not the electrum interface.

Bump Bitcoin Core version used in kotlin and python tests

f5a260a

We bump the Bitcoin Core version used in kotlin and python tests to support ephemeral dust. This is required for 0FC channels.

Check that the chain source supports 0FC channels

68acdd9

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 34eeb06 to a8779ef Compare June 25, 2026 02:00

tankyleo added 9 commits June 25, 2026 02:07

f: cover ChainSourceNotSupported variant

92ca474

f: Use submitpackage to point to the specific RPC call

1031204

f: Add todo to switch electrum to protocol_version API

a667888

f: rework TransactionBroadcast type

91872b2

f: DRY the broadcast logic and errors across all chain sources

11b8b42

Include 0FC channels in anchor channel checks

3bc0b46

f: Include 0fc channels in ReserveType checks

ebfb6cf

Negotiate 0FC channels if the anchor config is set

fcf54fd

tankyleo force-pushed the 25-10-0fc-channel-config branch from a8779ef to fcf54fd Compare June 25, 2026 02:07

tankyleo requested a review from tnull June 25, 2026 03:46

tnull reviewed Jun 25, 2026

View reviewed changes


		const BCAST_PACKAGE_QUEUE_SIZE: usize = 50;

		pub(crate) enum TransactionsToBroadcast {

Uh oh!

Conversation

tankyleo commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tankyleo commented Oct 24, 2025

Uh oh!

tankyleo commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Jun 22, 2026

Uh oh!

benthecarman commented Jun 22, 2026

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tankyleo Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tankyleo Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tankyleo commented Oct 13, 2025 •

edited

Loading

ldk-reviews-bot commented Oct 13, 2025 •

edited

Loading

tnull Oct 13, 2025 •

edited

Loading

tankyleo commented Oct 27, 2025 •

edited

Loading

tnull Jun 24, 2026 •

edited

Loading

tankyleo Jun 24, 2026 •

edited

Loading

tankyleo Jun 23, 2026 •

edited

Loading