Initial implementation of wallet functionality by tnull · Pull Request #33 · lightningdevkit/ldk-node

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.

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

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

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

+ // 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.

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

+	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

+ }
+	}
+
+	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.

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

+	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

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

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 97716a4diff --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

…-help-command server: Handle --help command

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 cca2687Compare November 30, 2022 12:02

tnull force-pushed the 2022-11-initial-wallet branch from 71c560d to a919af3Compare 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 4fd40d8Compare December 16, 2022 17:02

tnull commented Dec 17, 2022
View reviewed changes

Comment threadsrc/wallet.rs Outdated

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

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

tnull force-pushed the 2022-11-initial-wallet branch 2 times, most recently from 7d7d108 to c1c43f2Compare 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 876403eCompare 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 6fe5475Compare 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 bf9bcc2Compare January 17, 2023 22:48

tnull force-pushed the 2022-11-initial-wallet branch from a11268a to 4728f13Compare 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 97716a4Compare January 27, 2023 14:57

tnull merged commit adfdc24 into lightningdevkit:mainJan 27, 2023

jkczyz reviewed Jan 27, 2023
View reviewed changes

Comment threadsrc/wallet.rs

joostjager pushed a commit to joostjager/ldk-node that referenced this pull request Jan 28, 2026
Merge pull requestlightningdevkit#33from vincenzopalazzo/macros/fix…
8a7dd1e
…-help-command server: Handle --help command

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

Uh oh!

Conversation

tnull commented Nov 24, 2022

Uh oh!

tnull commented Dec 15, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnullJan 16, 2023• 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!

tnullJan 17, 2023• 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnullJan 16, 2023• 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!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull commented Jan 16, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull commented Jan 26, 2023

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tnull commented Jan 27, 2023

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tnullJan 16, 2023•
edited
Loading

tnullJan 17, 2023•
edited
Loading

tnullJan 16, 2023•
edited
Loading