Initial implementation of wallet functionality #33

tnull · 2022-11-24T12:53:00Z

This PR implements the initial wallet functionality, which was previously included in access.rs.

tnull · 2022-12-15T11:41:42Z

Now includes a custom KeysInterface implementation that overrides the shutdown/destination scripts so that they immediately show up in the BDK wallet after closing/claiming on-chain.

src/wallet.rs

jkczyz · 2023-01-12T02:05:46Z

src/wallet.rs

+	pub(crate) fn set_runtime(&self, tokio_runtime: Arc<tokio::runtime::Runtime>) {
+		*self.tokio_runtime.write().unwrap() = Some(tokio_runtime);
+	}
+
+	pub(crate) fn drop_runtime(&self) {
+		*self.tokio_runtime.write().unwrap() = None;
+	}


Not sure how well this fits into the larger picture -- but related to #10 (comment) -- another approach to avoid these may be to have any functions needing a runtime take it as a parameter. For sync traits, implement them on something like (Wallet<D>, Arc<tokio::runtime::Runtime>) if the implementation needs to call those functions.

Unfortunately the latter wouldn't work I think, as we need to give the BroadcasterInterface / FeeEstimator impls to ChannelManager and ChainMonitor. However, as we want to make parameters of them and/or Wallet configurable (e.g., wallet entropy source, Filter impl) based on chain data source, etc., we'd need to instantiate these in the Builder, where we do however not have access to a tokio runtime.

src/wallet.rs

jkczyz · 2023-01-12T02:12:32Z

src/wallet.rs

+		// We double-check that no inputs try to spend non-witness outputs. As we use a SegWit
+		// wallet descriptor this technically shouldn't ever happen, but better safe than sorry.


Where / how is this enforced?

This is guaranteed as we're using a Bip84 descriptor template for the BDK wallet, which happens currently in lib.rs.

src/wallet.rs

jkczyz · 2023-01-12T02:59:30Z

src/wallet.rs

+	fn get_destination_script(&self) -> Script {
+		let address =
+			self.wallet.get_new_address().expect("Failed to retrieve new address from wallet.");
+		address.script_pubkey()
+	}


Wish there were a better way of doing this without needing all the boilerplate to override / delegate. Wonder if we should think about parameterizing KeysManager in some way to make this simpler.

Yes, I'm not a fan of this solution either, also because we now have an redundant unused key in KeysManager, which could be used directly by any future code change. I'd be +1 to introducing some upstream changes to let us override that key.
However, last time I spoke to @TheBlueMatt about the matter he thought the current solution would be the cleanest way.

Yea, if we're trying to wrap an inner there's not much else we can do. If we wanted to drop the boilerplate we could have expose the KeysManager to the user and have them use that when they need a NodeSigner/EntropySource, but that's maybe even worse.

Seems like you could parameterize KeysManager with a strategy for generating the scripts. Either from keys derived from the seed or from some other wallet. Then you wouldn't wrap at all but rather have Wallet implement a trait that this new parameter is bounded by.

Are we fine going ahead with the current version, until we decide on some upstream changes and have them implemented?

Yes, that's fine by me.

TheBlueMatt

The new commits on top of #10 look good in isolation.

TheBlueMatt · 2023-01-16T18:42:23Z

src/wallet.rs

+		}
+	}
+
+	pub(crate) fn set_runtime(&self, tokio_runtime: Arc<tokio::runtime::Runtime>) {


Curious to see how this works once we get everything landed :)

tnull · 2023-01-16T22:25:54Z

The new commits on top of #10 look good in isolation.

Uh, rebased.

src/wallet.rs

jkczyz · 2023-01-17T20:48:53Z

src/wallet.rs

+	fn get_destination_script(&self) -> Script {
+		let address =
+			self.wallet.get_new_address().expect("Failed to retrieve new address from wallet.");
+		address.script_pubkey()
+	}


Seems like you could parameterize KeysManager with a strategy for generating the scripts. Either from keys derived from the seed or from some other wallet. Then you wouldn't wrap at all but rather have Wallet implement a trait that this new parameter is bounded by.

tnull · 2023-01-26T22:22:50Z

Squashed without further changes.

jkczyz

LGTM modulo some minor comments.

jkczyz · 2023-01-27T04:49:27Z

src/wallet.rs

+	fn get_destination_script(&self) -> Script {
+		let address =
+			self.wallet.get_new_address().expect("Failed to retrieve new address from wallet.");
+		address.script_pubkey()
+	}


Yes, that's fine by me.

src/wallet.rs

This implementation of `KeysInterface` allows to override the shutdown/destination scripts and forwards any other calls to an inner instance of `KeysManager` otherwise.

tnull · 2023-01-27T14:58:44Z

Squashed again with the following changes:

> git diff-tree -U2 4728f13 97716a4
diff --git a/src/wallet.rs b/src/wallet.rs
index 598d627..dab6cdc 100644
--- a/src/wallet.rs
+++ b/src/wallet.rs
@@ -111,5 +111,10 @@ where
                                }
                                Err(e) => {
-                                       log_error!(self.logger, "Failed to update fee rate estimation: {}", e);
+                                       log_error!(
+                                               self.logger,
+                                               "Failed to update fee rate estimation for {:?}: {}",
+                                               target,
+                                               e
+                                       );
                                }
                        }
@@ -223,5 +228,5 @@ where
        D: BatchDatabase,
 {
-       /// Constructs a `WalletKeysManager` that
+       /// Constructs a `WalletKeysManager` that overrides the destination and shutdown scripts.
        ///
        /// See [`KeysManager::new`] for more information on `seed`, `starting_time_secs`, and

src/wallet.rs

This was referenced Nov 24, 2022

Initial chain access implementation #9

Closed

Initial implementation of LDK Node #11

Merged

tnull force-pushed the 2022-11-initial-wallet branch 2 times, most recently from 5264c2a to cca2687 Compare November 30, 2022 12:02

tnull force-pushed the 2022-11-initial-wallet branch from 71c560d to a919af3 Compare December 15, 2022 11:28

tnull mentioned this pull request Dec 15, 2022

Make it possible to spend LDK UTXO's in BDK bitcoindevkit/bdk#710

Closed

tnull force-pushed the 2022-11-initial-wallet branch 3 times, most recently from 7d54c61 to 4fd40d8 Compare December 16, 2022 17:02

tnull commented Dec 17, 2022

View reviewed changes

src/wallet.rs Outdated Show resolved Hide resolved

tnull force-pushed the 2022-11-initial-wallet branch 2 times, most recently from a05b1f0 to 35a25ff Compare December 19, 2022 10:38

tnull requested review from jkczyz and TheBlueMatt January 4, 2023 12:52

tnull force-pushed the 2022-11-initial-wallet branch 2 times, most recently from 7d7d108 to c1c43f2 Compare January 11, 2023 09:46

jkczyz reviewed Jan 12, 2023

View reviewed changes

tnull force-pushed the 2022-11-initial-wallet branch 2 times, most recently from 829eab3 to 876403e Compare January 16, 2023 15:34

TheBlueMatt reviewed Jan 16, 2023

View reviewed changes

tnull force-pushed the 2022-11-initial-wallet branch from 876403e to 6fe5475 Compare January 16, 2023 22:24

tnull requested a review from jkczyz January 17, 2023 17:09

jkczyz reviewed Jan 17, 2023

View reviewed changes

tnull force-pushed the 2022-11-initial-wallet branch 4 times, most recently from 77ce8ce to bf9bcc2 Compare January 17, 2023 22:48

tnull force-pushed the 2022-11-initial-wallet branch from a11268a to 4728f13 Compare January 26, 2023 22:22

tnull requested a review from jkczyz January 26, 2023 22:23

jkczyz approved these changes Jan 27, 2023

View reviewed changes

tnull added 2 commits January 27, 2023 08:57

Initial implementation of wallet functionality

43f1d2d

Implement WalletKeysManager

97716a4

This implementation of `KeysInterface` allows to override the shutdown/destination scripts and forwards any other calls to an inner instance of `KeysManager` otherwise.

tnull force-pushed the 2022-11-initial-wallet branch from 4728f13 to 97716a4 Compare January 27, 2023 14:57

tnull merged commit adfdc24 into lightningdevkit:main Jan 27, 2023

jkczyz reviewed Jan 27, 2023

View reviewed changes

src/wallet.rs Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation of wallet functionality #33

Initial implementation of wallet functionality #33

tnull commented Nov 24, 2022

tnull commented Dec 15, 2022

jkczyz Jan 12, 2023

tnull Jan 16, 2023 •

edited

Loading

jkczyz Jan 12, 2023

tnull Jan 17, 2023 •

edited

Loading

jkczyz Jan 12, 2023

tnull Jan 16, 2023 •

edited

Loading

TheBlueMatt Jan 16, 2023

jkczyz Jan 17, 2023

tnull Jan 26, 2023

jkczyz Jan 27, 2023

TheBlueMatt left a comment

TheBlueMatt Jan 16, 2023

tnull commented Jan 16, 2023

jkczyz Jan 17, 2023

tnull commented Jan 26, 2023

jkczyz left a comment

jkczyz Jan 27, 2023

tnull commented Jan 27, 2023

		// We double-check that no inputs try to spend non-witness outputs. As we use a SegWit
		// wallet descriptor this technically shouldn't ever happen, but better safe than sorry.

Initial implementation of wallet functionality #33

Initial implementation of wallet functionality #33

Conversation

tnull commented Nov 24, 2022

tnull commented Dec 15, 2022

Choose a reason for hiding this comment

tnull Jan 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull Jan 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull commented Jan 16, 2023

Choose a reason for hiding this comment

tnull commented Jan 26, 2023

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull commented Jan 27, 2023

tnull Jan 16, 2023 •

edited

Loading

tnull Jan 17, 2023 •

edited

Loading

tnull Jan 16, 2023 •

edited

Loading