Add anchor support by tnull · Pull Request #141 · lightningdevkit/ldk-node

tnull · 2023-07-24T07:49:40Z

Based on ~~#243~~, ~~#250, #253.~~

tnull · 2023-10-05T19:29:46Z

Rebased on #151.

tnull · 2023-12-21T17:23:52Z

Rebased on current main. Still need to expose a good API.

tnull · 2024-06-01T08:06:31Z

Kicked CI..

tnull · 2024-06-03T16:07:05Z

Squashed fixups without further changes.

src/config.rs

src/wallet.rs

src/balance.rs

src/lib.rs

src/wallet.rs

jkczyz · 2024-06-04T15:22:08Z

src/config.rs

+	/// funds stuck *or* even allow the counterparty to steal any in-flight funds after the
+	/// corresponding HTLCs time out.
+	pub trusted_peers_no_reserve: Vec<PublicKey>,
+	/// The amount of satoshis we keep as an emergency reserve in our on-chain wallet in order to


While the field name makes it clear that this is per channel, we should phrase the docs that way as well.

That said, should it even be per-channel? Do we prevent from bumping fees on any particular channel more than per_channel_reserve_sats? And if not, couldn't doing so bring us under the reserve? Using a total reserve instead of per-channel would remove a lot of complexity, it seems.

While the field name makes it clear that this is per channel, we should phrase the docs that way as well.

Fixed.

That said, should it even be per-channel?

Yes, I believe this to be currently the best trade-off between simplicity and adaptability.

Do we prevent from bumping fees on any particular channel more than per_channel_reserve_sats? And if not, couldn't doing so bring us under the reserve? Using a total reserve instead of per-channel would remove a lot of complexity, it seems.

No, it doesn't prevent spending more than the per-channel reserve. This is however generally true, as bumping could also always use more than the total reserve if more funds are spendable, which is exactly the reason for the trusted_peers_no_reserve field, which makes sure we'd never spend any funds on bumping to improve UX.

Also note that we made this a per-channel reserve as:
a) it at least somewhat scales with the number of channels
b) it allows us to reject inbound anchor channels if we don't have sufficient on-chain funds to cover the reserve. Otherwise we would need indiscriminately accept inbound channels which might result in a unreasonable total-reserve-to-channel-count ratio.

Hmm... "per channel" is a bit misleading then. It's more of a multiplier? Not sure if an alternative name is any better, but I think at least the docs need to be clear on: (a) how the reserve can be used, (b) how it is checked, and (c) that it can be exceeded (which, IIUC could prevent further channels from being opened).

Now added another paragraph to the docs, hope this sufficiently clarifies things

src/config.rs

src/event.rs

src/config.rs

jkczyz · 2024-06-05T15:15:10Z

src/config.rs

+	/// funds stuck *or* even allow the counterparty to steal any in-flight funds after the
+	/// corresponding HTLCs time out.
+	pub trusted_peers_no_reserve: Vec<PublicKey>,
+	/// The amount of satoshis we keep as an emergency reserve in our on-chain wallet in order to


Hmm... "per channel" is a bit misleading then. It's more of a multiplier? Not sure if an alternative name is any better, but I think at least the docs need to be clear on: (a) how the reserve can be used, (b) how it is checked, and (c) that it can be exceeded (which, IIUC could prevent further channels from being opened).

tnull · 2024-06-10T07:24:19Z

@jkczyz Let me know if I can squash.

jkczyz · 2024-06-10T14:16:24Z

@jkczyz Let me know if I can squash.

SGTM

tnull · 2024-06-10T17:00:30Z

@jkczyz Let me know if I can squash.

SGTM

Squashed fixups without further changes.

src/config.rs

jkczyz · 2024-06-10T23:50:21Z

src/lib.rs

+		let spendable_amount_sats =
+			self.wallet.get_balances(cur_anchor_reserve_sats).map(|(_, s)| s).unwrap_or(0);


For readability and DRY, could we add a get_spendable_amount to Wallet?

Hmm, now added a fixup to that end, but personally no big fan of one-liner helpers that IMO often tend to obfuscate what actually happens.

Yeah, ideally Wallet would just be aware of the anchors reserve (instead of passing it) and return an adjusted Balance, though that involves introducing dependencies to the Wallet. Even if it were aware, the reserve concept can't be applied to one of the Balance components without affecting the total, unfortunately.

What about defining our own internal OnchainBalance as:

struct OnchainBalance { pub anchor_reserve_sats: u64, pub spendable_amount_sats: u64, pub total_amount_sats: u64, }

Where Wallet::get_balances takes ChannelManager and Config to compute the anchor reserve internally instead of computing it externally at each call site and passing it in? That would keep the calculations in one place, including the one that adjusts the reserve based on the total balance.

Hm, I'd really like to keep the objects as separate as possible for the time being, which is why I went this way:

a) Currently every wallet operation tends to block on the sync which lives in its own thread. So rather than integrating it further, I'd rather move the opposite direction and isolate Wallet more where possible. I had considered before to fully rewrite it based on an Actor pattern and only communicating via channels from/to the wallet. I just didn't go that way yet as the upcoming update might mitigate a lot of the issues.
b) we're about to essentially rewrite the wallet integration with the BDK 1.0 more or less entirely
c) We might still consider introducing a Wallet trait to let users bring/integrate their own wallet. This is undecided, but at least has been requested many times in the past.

So if we end up really deciding against c), I'd at least would hold off on it until we upgrade or after the BDK upgrade, especially as it will complicate the balance-caching logic even more than it needs to be.

Sure, happy to wait if you prefer. Just seems if we always need to call total_anchor_channels_reserve_sats before calling Wallet::get_balance then we might as well merge them.

jkczyz · 2024-06-10T23:59:53Z

src/payment/onchain.rs


-		let cur_balance = self.wallet.get_balance()?;
-		if cur_balance.get_spendable() < amount_sats {
-			log_error!(self.logger, "Unable to send payment due to insufficient funds.");
+		let cur_anchor_reserve_sats =
+			crate::total_anchor_channels_reserve_sats(&self.channel_manager, &self.config);
+		let spendable_amount_sats =
+			self.wallet.get_balances(cur_anchor_reserve_sats).map(|(_, s)| s).unwrap_or(0);
+
+		if spendable_amount_sats < amount_sats {
+			log_error!(self.logger,
+				"Unable to send payment due to insufficient funds. Available: {}sats, Required: {}sats",
+				spendable_amount_sats, amount_sats
+			);
 			return Err(Error::InsufficientFunds);


Technically, this is a behavioral change when get_balances returns Err (i.e., a different variant than before will be returned). Maybe it doesn't matter?

Yeah, that's true, but I think it shouldn't matter all too much. We might need to revisit it though as part of the BDK 1.0 upgrade, which will entail switching BDK's persistence backend to our KVStore, meaning failure to access the wallet might happen more often, e.g., in a VSS setting. But essentially we'll have to thoroughly revisit and review all BDK connected code then, and before that I'll have to read some BDK internal code to learn what's actually going on under the hood and how we can deal with persistence failures best.

FYI, for context: We will also add a cache for retrieving the on-chain balances in the #281 follow-up as the Wallet mutex is blocked for the entire duration of any on-chain syncing task. This means that operations like these might (depending on the setting and how unreliable/rate-limited the configured Esplora server is) block upwards of a minute to retrieve the balance. This is painful here but unacceptable in event handling as it would bring the node essentially to a standstill. This will be properly fixed as part of the BDK 1.0 upgrade which will allow us to decouple retrieving the changes-to-sync and actually applying them, i.e., afterwards we won't need to hold the Wallet's mutex guard for the entire time, but before caching the balances is a requirement.

TLDR: Blocking on the wallet mutex bad, we'll work-around a few of the consequential issues in #281.

src/lib.rs

jkczyz · 2024-06-11T00:12:19Z

src/wallet.rs

 		let locked_wallet = self.inner.lock().unwrap();
-		let address_info = locked_wallet.get_address(AddressIndex::LastUnused).map_err(|e| {
-			log_error!(self.logger, "Failed to retrieve new address from wallet: {}", e);
-		})?;
+		let address_info =
+			locked_wallet.get_internal_address(AddressIndex::LastUnused).map_err(|e| {
+				log_error!(self.logger, "Failed to retrieve new address from wallet: {}", e);
+			})?;


Should we use get_new_internal_address instead?

Should we use get_new_internal_address instead?

Hmm, it could, butget_new_internal_address is essentially just a helper API for WalletKeysManager. While they are currently doing the same thing, it's not unthinkable the behavior might diverge in the future. Generally I'd prefer methods on Wallet to use self.inner and everything else to use the Wallet API.

jkczyz

Other than the earlier comment on balance computation, just some minor comments and I think we're good.

src/config.rs

tests/common/mod.rs

tnull · 2024-06-11T15:48:20Z

Other than the earlier comment on balance computation, just some minor comments and I think we're good.

Addressed the remaining doc comments. See my response to the balance/reserve above. Let me know if I can squash the fixups.

jkczyz · 2024-06-11T16:03:04Z

Other than the earlier comment on balance computation, just some minor comments and I think we're good.

Addressed the remaining doc comments. See my response to the balance/reserve above. Let me know if I can squash the fixups.

Yup, go ahead and squash.

.. allowing to configure the per-channel emergency reserve as well as some trusted peers for which we won't maintain any reserve.

tnull · 2024-06-11T16:04:14Z

Other than the earlier comment on balance computation, just some minor comments and I think we're good.

Addressed the remaining doc comments. See my response to the balance/reserve above. Let me know if I can squash the fixups.

Yup, go ahead and squash.

Squashed fixups without further changes.

src/event.rs

When Anchor outputs need to be spent LDK will generate `BumpTransactionEvent`s. Here, we add the corresponding event-handling and PSBT-signing support.

.. because they will be the new default. Note the upcoming CLN 24.02 release will make Anchors default, too, but for now we have to set the `experimental-anchors` config option.

.. which we somehow so far ommitted exposing in the API. We now introduce a `force_close` method and broadcast if the counterparty is not trusted.

.. i.e., without bumping.

tnull · 2024-06-11T17:51:14Z

Force-pushed with the indentation fixes:

> git diff-tree -U2 e7c9824 436f2e4
diff --git a/src/event.rs b/src/event.rs
index 19ecfbd..36769c0 100644
--- a/src/event.rs
+++ b/src/event.rs
@@ -850,8 +850,8 @@ where
                                                if spendable_amount_sats < required_amount_sats {
                                                        log_error!(
-                                                       self.logger,
-                                                       "Rejecting inbound Anchor channel from peer {} due to insufficient available on-chain reserves.",
-                                                       counterparty_node_id,
-                                               );
+                                                               self.logger,
+                                                               "Rejecting inbound Anchor channel from peer {} due to insufficient available on-chain reserves.",
+                                                               counterparty_node_id,
+                                                       );
                                                        self.channel_manager
                                                                .force_close_without_broadcasting_txn(
@@ -1147,7 +1147,7 @@ where
                                        {
                                                log_debug!(self.logger,
-                                                               "Ignoring BumpTransactionEvent for channel {} due to trusted counterparty {}",
-                                                               channel_id, counterparty_node_id
-                                                       );
+                                                       "Ignoring BumpTransactionEvent for channel {} due to trusted counterparty {}",
+                                                       channel_id, counterparty_node_id
+                                               );
                                                return;
                                        }

tnull marked this pull request as draft July 24, 2023 07:49

tnull mentioned this pull request Jul 24, 2023

Upgrade to LDK 0.0.116 #105

Merged

tnull mentioned this pull request Aug 15, 2023

LDK Node v0.2 Tracking Issue #107

Closed

9 tasks

tnull force-pushed the 2023-07-add-anchor-support branch from 340cdd5 to 405dcec Compare October 5, 2023 19:29

tnull mentioned this pull request Oct 24, 2023

Upgrade to LDK 0.0.118 #175

Merged

tnull mentioned this pull request Nov 15, 2023

LDK Node v0.3 Tracking Issue #192

Closed

19 tasks

tnull force-pushed the 2023-07-add-anchor-support branch from 405dcec to 08ba6e0 Compare December 21, 2023 17:23

tnull force-pushed the 2023-07-add-anchor-support branch from 08ba6e0 to 3bc7c2d Compare December 21, 2023 18:56

tnull force-pushed the 2023-07-add-anchor-support branch from 3bc7c2d to 0387783 Compare January 17, 2024 13:58

tnull force-pushed the 2023-07-add-anchor-support branch from 0387783 to 1fecf85 Compare February 5, 2024 11:29

tnull mentioned this pull request Feb 16, 2024

Expose unified list_balances interface method #250

Merged

tnull force-pushed the 2023-07-add-anchor-support branch 11 times, most recently from a6bdd44 to 8a01a2e Compare February 19, 2024 15:27

tnull added this to the 0.3 milestone Feb 19, 2024

tnull mentioned this pull request Feb 20, 2024

Move Config and associated consts to dedicatedconfig.rs #253

Merged

tnull force-pushed the 2023-07-add-anchor-support branch 2 times, most recently from 2741cf9 to 1510751 Compare February 20, 2024 10:35

tnull changed the title ~~WIP: Add anchor support~~ Add anchor support Feb 20, 2024

tnull marked this pull request as ready for review February 20, 2024 10:41

tnull force-pushed the 2023-07-add-anchor-support branch from 4c8ea12 to c642b42 Compare June 1, 2024 08:06

jkczyz self-requested a review June 3, 2024 15:57

tnull force-pushed the 2023-07-add-anchor-support branch from c642b42 to a0e16d5 Compare June 3, 2024 16:06

jkczyz reviewed Jun 4, 2024

View reviewed changes

tnull force-pushed the 2023-07-add-anchor-support branch from a0e16d5 to 5fe41f2 Compare June 4, 2024 07:11

jkczyz reviewed Jun 4, 2024

View reviewed changes

jkczyz reviewed Jun 5, 2024

View reviewed changes

tnull mentioned this pull request Jun 7, 2024

Expose a way to access channel or reserve type #305

Open

jkczyz reviewed Jun 11, 2024

View reviewed changes

tnull mentioned this pull request Jun 11, 2024

Expose NetworkGraph accessors #293

Merged

jkczyz reviewed Jun 11, 2024

View reviewed changes

src/config.rs Outdated Show resolved Hide resolved

src/config.rs Outdated Show resolved Hide resolved

src/config.rs Outdated Show resolved Hide resolved

tests/common/mod.rs Show resolved Hide resolved

Introduce AnchorChannelsConfig

a2691e7

.. allowing to configure the per-channel emergency reserve as well as some trusted peers for which we won't maintain any reserve.

jkczyz previously approved these changes Jun 11, 2024

View reviewed changes

src/event.rs Outdated Show resolved Hide resolved

src/event.rs Outdated Show resolved Hide resolved

tnull added 6 commits June 11, 2024 19:50

Implement Anchor channel event handling

db1b373

When Anchor outputs need to be spent LDK will generate `BumpTransactionEvent`s. Here, we add the corresponding event-handling and PSBT-signing support.

Maintain and expose anchor reserve

1952802

Use Anchor channels in CLN integration test

5ba1b35

.. because they will be the new default. Note the upcoming CLN 24.02 release will make Anchors default, too, but for now we have to set the `experimental-anchors` config option.

Add force_close method to allow channel force-closure

1887af8

.. which we somehow so far ommitted exposing in the API. We now introduce a `force_close` method and broadcast if the counterparty is not trusted.

Test force-closure 'happy' case

0aaa8f1

.. i.e., without bumping.

Use internal addresses/LastUnused for change scripts

436f2e4

jkczyz approved these changes Jun 11, 2024

View reviewed changes

tnull mentioned this pull request Jun 11, 2024

Periodically archive fully resolved channel monitors #307

Merged

		let spendable_amount_sats =
		self.wallet.get_balances(cur_anchor_reserve_sats).map(\|(_, s)\| s).unwrap_or(0);

Conversation

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

Uh oh!

Uh oh!

tnull commented Oct 5, 2023

Uh oh!

tnull commented Dec 21, 2023

Uh oh!

tnull commented Jun 1, 2024

Uh oh!

tnull commented Jun 3, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull commented Jun 10, 2024

Uh oh!

jkczyz commented Jun 10, 2024

Uh oh!

tnull commented Jun 10, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkczyz Jun 11, 2024 • 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!

tnull Jun 11, 2024 • 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!

tnull Jun 11, 2024 • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tnull commented Jun 11, 2024

Uh oh!

jkczyz commented Jun 11, 2024

tnull commented Jul 24, 2023 •

edited

Loading

jkczyz Jun 11, 2024 •

edited

Loading

tnull Jun 11, 2024 •

edited

Loading

tnull Jun 11, 2024 •

edited

Loading