Add anchor support #141

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-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 bb689e0 to 893172c Compare June 10, 2024 16:59

jkczyz reviewed Jun 11, 2024

View reviewed changes

tnull force-pushed the 2023-07-add-anchor-support branch 2 times, most recently from 56f3040 to caf0aca Compare June 11, 2024 08:16

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

tnull force-pushed the 2023-07-add-anchor-support branch 3 times, most recently from caf0aca to a93fd5d Compare June 11, 2024 15:47

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.

tnull force-pushed the 2023-07-add-anchor-support branch from a93fd5d to e7c9824 Compare June 11, 2024 16:04

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

tnull dismissed jkczyz’s stale review via 436f2e4 June 11, 2024 17:50

tnull force-pushed the 2023-07-add-anchor-support branch from e7c9824 to 436f2e4 Compare June 11, 2024 17:50

jkczyz approved these changes Jun 11, 2024

View reviewed changes

tnull merged commit 2562f52 into lightningdevkit:main Jun 11, 2024

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

Add anchor support #141

Add anchor support #141

Uh oh!

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 10, 2024

Uh oh!

jkczyz commented Jun 10, 2024

Uh oh!

tnull commented Jun 10, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jkczyz Jun 10, 2024

Choose a reason for hiding this comment

Uh oh!

tnull Jun 11, 2024

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!

jkczyz Jun 11, 2024

Choose a reason for hiding this comment

Uh oh!

jkczyz Jun 10, 2024

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!

jkczyz Jun 11, 2024

Choose a reason for hiding this comment

Uh oh!

tnull Jun 11, 2024

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

Uh oh!

tnull commented Jun 11, 2024

Uh oh!

Uh oh!

Uh oh!

tnull commented Jun 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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