Split KeysInterface into EntropySource, NodeSigner, and SignerProvider #1910

arik-so · 2022-12-10T10:35:51Z

Context for reviewers:

The idea behind this PR is splitting KeysInterface into the three independent tasks and traits it actually comprises. The core changes of this PR are basically lightning::src::chain::keysinterface.rs:429-561.

While updating the trait implementers (KeyProvider [fuzz::chanmon_consistency, fuzz::full_stack, fuzz::onion_messages], Keys [channel::tests], KeysManager, PhantomKeysManager, OnlyReadsKeysInterface [util::test_utils], TestKeysInterface [util::test_utils]), we noticed that they all pretty much had the same implementation for get_node_id(), ecdh(), and sign_invoice(), with the sole exceptions of the PhantomKeysManager and TestKeysInterface.

So rather than requiring each type duplicate the same code, we put the implementation on the trait itself (NodeSigner), which, for sign_invoice(), required the addition of a secp context parameter.

All the other changes in this PR are simply a reflection of the new method location (docs) and the new method signature (mostly the invoice crate, which was using sign_invoice extensively, as well as a couple tests).

TheBlueMatt · 2022-12-12T23:34:58Z

Needs rebase, sorry about that.

lightning/src/chain/keysinterface.rs

TheBlueMatt · 2022-12-12T23:44:26Z

lightning/src/chain/keysinterface.rs

+	///
+	/// This method should return a different value each time it is called, to avoid linking
+	/// on-chain funds across channels as controlled to the same user.
+	fn get_destination_script(&self) -> Script;


Also, ISTM that get_destination_script and get_shutdown_script should be merged (we have an old issue for this, maybe a PR too?) and maybe placed in the Signer instead - its value should really be channel-specific.

Also fair point

Are you thinking to push this off to a later PR?

Yeah, the merging of these two should be in a separate PR imo

Actually, maybe lets not move these into Sign when we merge them. That way someone using an external wallet for static payouts doesn't need to touch InMemorySigner.

hm, interesting point. Thankfully outside the scope of this PR, but do you think that's sufficient reason? These values really are rather channel-specific.

In a more object-oriented world, the signers could in principle retain a reference to their SignerProvider, and by default take the value from there if they don't override the implementation, but I don't think that's a path we wanna go down.

arik-so · 2022-12-12T23:49:12Z

Agree with your comments, Matt, just trying not to break too much at once.

codecov-commenter · 2022-12-13T00:12:34Z

Codecov Report

Base: 90.73% // Head: 91.44% // Increases project coverage by +0.71% 🎉

Coverage data is based on head (9d7bb73) compared to base (56afbf5).
Patch coverage: 65.85% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1910      +/-   ##
==========================================
+ Coverage   90.73%   91.44%   +0.71%     
==========================================
  Files          94       95       +1     
  Lines       49603    53483    +3880     
  Branches    49603    53483    +3880     
==========================================
+ Hits        45006    48908    +3902     
+ Misses       4597     4575      -22

Impacted Files	Coverage Δ
lightning-background-processor/src/lib.rs	`95.42% <ø> (ø)`
lightning-invoice/src/utils.rs	`97.76% <ø> (ø)`
lightning/src/ln/chan_utils.rs	`93.56% <ø> (ø)`
lightning/src/ln/channel.rs	`88.76% <0.00%> (-0.10%)`	⬇️
lightning/src/ln/functional_test_utils.rs	`93.73% <ø> (ø)`
lightning/src/ln/functional_tests.rs	`98.39% <ø> (+1.44%)`	⬆️
lightning/src/ln/inbound_payment.rs	`93.49% <ø> (ø)`
lightning/src/ln/onion_route_tests.rs	`97.64% <ø> (ø)`
lightning/src/ln/payment_tests.rs	`99.30% <ø> (+0.55%)`	⬆️
lightning/src/ln/priv_short_conf_tests.rs	`96.54% <ø> (ø)`
... and 28 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt · 2022-12-13T21:02:30Z

Think this should wait for 114, given it, by itself, leaves the code in a kinda intermediate state.

wpaulino

Last unresolved link to fix:

error: unresolved link to `KeysInterface::derive_channel_signer`
   --> lightning/src/ln/channel.rs:743:8
    |
743 |     /// [`KeysInterface::derive_channel_signer`].
    |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `KeysInterface` has no associated item named `derive_channel_signer`

lightning/src/chain/keysinterface.rs

wpaulino · 2022-12-15T20:28:05Z

lightning/src/chain/keysinterface.rs

+	// We need to override the invoice signing implementation for phantom keys managers
+	fn sign_invoice<C: Signing>(&self, hrp_bytes: &[u8], invoice_data: &[u5], recipient: Recipient, _secp_context: &Secp256k1<C>) -> Result<RecoverableSignature, ()> {
+		let preimage = construct_invoice_preimage(&hrp_bytes, &invoice_data);
+		let secret = self.get_node_secret(recipient)?;


Wouldn't we want to return an error if Recipient::Node is provided so that we don't accidentally sign with the node secret? I know we currently can't because the PhantomKeysManager owns KeysManager, but perhaps we could if we took a reference?

I think it will be part of your get_node_secret removal PR :)

wpaulino · 2022-12-15T20:36:29Z

lightning/src/ln/channelmanager.rs

        T::Target: BroadcasterInterface,
-        K::Target: KeysInterface,
+        K::Target: KeysInterface + NodeSigner,


Do we still need the explicit NodeSigner constraint after keeping the methods with default implementations on the traits and not on impl blocks?

Unfortunately, we do.

We do not (anymore).

Sorry. When I responded, we still had the default implementation in the trait.

That's why I said "anymore" :)

lightning/src/ln/inbound_payment.rs

lightning-invoice/src/utils.rs

wpaulino

LGTM, let's remove "WIP" from the commit message. Also needs yet another rebase.

TheBlueMatt · 2022-12-18T17:54:52Z

lightning/src/chain/keysinterface.rs

-	fn ecdh(&self, recipient: Recipient, other_key: &PublicKey, tweak: Option<&Scalar>) -> Result<SharedSecret, ()>;
-	/// Get a script pubkey which we send funds to when claiming on-chain contestable outputs.
+	fn ecdh(&self, recipient: Recipient, other_key: &PublicKey, tweak: Option<&Scalar>) -> Result<SharedSecret, ()> {
+		let mut node_secret = self.get_node_secret(recipient)?;


I don't think we should add a default implementation here - (a) we want to remove get_node_secret, not rely on it more, (b) it makes this single commit less move-only, which I'd really prefer to keep it as move-only as possible.

Yeah, I see your point. When writing this PR, the idea was to just keep the same functions, but move them around, and for that situation, a reimplementation of the same code seemed redundant. I can add back the non-default implementations, or just leave it for @wpaulino's follow-up PR that's removing node_secret. Same with ecdh and sign_invoice.

Not moving then is less diff :)

TheBlueMatt · 2022-12-18T17:55:02Z

lightning/src/chain/keysinterface.rs

-	/// on-chain funds across channels as controlled to the same user.
-	fn get_shutdown_scriptpubkey(&self) -> ShutdownScript;
+	/// Errors if the [`Recipient`] variant is not supported by the implementation.
+	fn sign_invoice<C: Signing>(&self, hrp_bytes: &[u8], invoice_data: &[u5], recipient: Recipient, secp_context: &Secp256k1<C>) -> Result<RecoverableSignature, ()> {


TheBlueMatt · 2022-12-18T17:55:47Z

lightning/src/chain/keysinterface.rs

@@ -954,7 +984,6 @@ impl ReadableArgs<SecretKey> for InMemorySigner {
 pub struct KeysManager {
 	secp_ctx: Secp256k1<secp256k1::All>,
 	node_secret: SecretKey,
-	node_id: PublicKey,


Why are you removing the caching here?

Because it was throwing a notice saying it wasn't used.

Right, when you drop the default methods you'll want to keep the caching here. Even more reason to not do the default methods :)

TheBlueMatt · 2022-12-18T17:58:05Z

Once we land this, we'll definitely want to land the followup(s) required to at least split the traits into different objects in the same release. It'd be nice to get the followup PRs open sooner rather than later, though we can land this without.

arik-so · 2022-12-19T18:38:08Z

Soo… once I undo the part of the PR where I remove the overrides, is this PR otherwise ready?

TheBlueMatt · 2022-12-19T19:16:58Z

I'm happy with it, yes. As long as we move quickly to splitting the interfaces up fully.

TheBlueMatt · 2022-12-19T21:03:38Z

Please dont change something in one commit and then revert it in the next.

arik-so · 2022-12-19T21:28:26Z

Huh, what do you mean? Isn't the idea to squash all of these commits into one at the end?

TheBlueMatt · 2022-12-19T21:32:41Z

No - we dont squash + merge, we've always used merge commits.

arik-so · 2022-12-19T21:34:29Z

Right, but before we do that, I was planning to squash these commits myself.

TheBlueMatt · 2022-12-19T22:06:46Z

Please go ahead and squash so we can ACK it :)

arik-so · 2022-12-19T22:14:42Z

I will once the tests complete so it's easier to compare and go back. Will do once they're all green.

arik-so · 2022-12-19T22:35:29Z

Ok, enough tests have passed to to squash it now, I think

TheBlueMatt · 2022-12-20T17:45:22Z

lightning-invoice/src/utils.rs

@@ -17,7 +17,7 @@ use lightning::ln::inbound_payment::{create, create_from_hash, ExpandedKey};
 use lightning::routing::gossip::RoutingFees;
 use lightning::routing::router::{InFlightHtlcs, Route, RouteHint, RouteHintHop};
 use lightning::util::logger::Logger;
-use secp256k1::PublicKey;
+use secp256k1::{PublicKey};


Please keep diffs minimal.

TheBlueMatt · 2022-12-20T17:45:27Z

lightning-invoice/src/utils.rs

@@ -54,7 +54,7 @@ use core::time::Duration;
 pub fn create_phantom_invoice<K: Deref, L: Deref>(
 	amt_msat: Option<u64>, payment_hash: Option<PaymentHash>, description: String,
 	invoice_expiry_delta_secs: u32, phantom_route_hints: Vec<PhantomRouteHints>, keys_manager: K,
-	logger: L, network: Currency,
+	logger: L, network: Currency


same here (and on many lines below)

TheBlueMatt · 2022-12-20T17:46:05Z

lightning-invoice/src/utils.rs

 		T::Target: BroadcasterInterface,
 		K::Target: KeysInterface,
 		F::Target: FeeEstimator,
-		L::Target: Logger,
+		L::Target: Logger


incl here and below in tests.

TheBlueMatt · 2022-12-20T17:49:39Z

lightning/src/ln/channelmanager.rs

        T::Target: BroadcasterInterface,
-        K::Target: KeysInterface,
+        K::Target: KeysInterface + NodeSigner,


We do not (anymore).

TheBlueMatt · 2022-12-20T17:52:17Z

In the future, this kind of PR would probably be a chunk easier to review if it were separate commits - adding separate commits for the new traits would give a reviewer fewer things to look at in more digestible chunks.

…vider.

TheBlueMatt

Next time please be more careful with the patch.

TheBlueMatt · 2022-12-20T21:55:58Z

lightning/src/chain/keysinterface.rs

+	/// By parameterizing by the raw invoice bytes instead of the hash, we allow implementors of
+	/// this trait to parse the invoice and make sure they're signing what they expect, rather than
+	/// blindly signing the hash.
+	/// The hrp is ascii bytes, while the invoice data is base32.


Why did you decapitalize ASCII (and remove ticks on hrp, which maybe should read hrp_bytes instead)?

TheBlueMatt · 2022-12-20T22:17:38Z

lightning/src/util/test_utils.rs


 #[cfg(feature = "std")]
 use std::time::{SystemTime, UNIX_EPOCH};
+use bitcoin::secp256k1::ecdh::SharedSecret;


Why did this get moved away from the other secp entries?

valentinewallace · 2022-12-20T22:47:41Z

FYI if there's gonna be a follow-up, it looks like some unused warnings were introduced were introduced in cargo test

arik-so · 2022-12-20T22:51:12Z

I'm pretty sure some of them are false positives that depend on the configuration flag, because removing the unuseds causes things to break. I will try to narrow down the config flags for those imports in the follow-up though.

And yes, there will be at least two follow-ups to this PR.

Trivial #1910 Followups

TheBlueMatt reviewed Dec 12, 2022

View reviewed changes

arik-so force-pushed the 2022-12-keys-interface-name-split branch from 01cecb7 to 7b70d4b Compare December 13, 2022 05:38

TheBlueMatt added the blocked on next release Should Wait Until Next Release To Land label Dec 13, 2022

arik-so force-pushed the 2022-12-keys-interface-name-split branch 2 times, most recently from f07dbf9 to 06af918 Compare December 14, 2022 21:29

arik-so changed the title ~~WIP: Split KeysInterface into EntropySource, NodeSigner, and SignerProvider~~ Split KeysInterface into EntropySource, NodeSigner, and SignerProvider Dec 14, 2022

arik-so marked this pull request as ready for review December 14, 2022 21:29

wpaulino self-requested a review December 15, 2022 02:34

wpaulino reviewed Dec 15, 2022

View reviewed changes

arik-so force-pushed the 2022-12-keys-interface-name-split branch 2 times, most recently from 37535c1 to 7536509 Compare December 16, 2022 05:38

arik-so removed the blocked on next release Should Wait Until Next Release To Land label Dec 16, 2022

wpaulino reviewed Dec 16, 2022

View reviewed changes

arik-so requested review from wpaulino and TheBlueMatt December 16, 2022 19:04

arik-so force-pushed the 2022-12-keys-interface-name-split branch from 7536509 to 8527d4b Compare December 16, 2022 19:25

wpaulino previously approved these changes Dec 16, 2022

View reviewed changes

TheBlueMatt reviewed Dec 18, 2022

View reviewed changes

arik-so dismissed wpaulino’s stale review via 953127a December 19, 2022 20:46

arik-so force-pushed the 2022-12-keys-interface-name-split branch from 953127a to 7d8b0aa Compare December 19, 2022 21:37

arik-so force-pushed the 2022-12-keys-interface-name-split branch from 7d8b0aa to aa24b99 Compare December 19, 2022 22:35

arik-so requested review from TheBlueMatt and wpaulino December 20, 2022 00:40

TheBlueMatt reviewed Dec 20, 2022

View reviewed changes

Split out KeysInterface into EntropySource, NodeSigner, and SignerPro…

9d7bb73

…vider.

arik-so force-pushed the 2022-12-keys-interface-name-split branch from aa24b99 to 9d7bb73 Compare December 20, 2022 18:09

arik-so requested a review from TheBlueMatt December 20, 2022 18:11

wpaulino approved these changes Dec 20, 2022

View reviewed changes

TheBlueMatt approved these changes Dec 20, 2022

View reviewed changes

TheBlueMatt merged commit f7211fb into lightningdevkit:main Dec 20, 2022

valentinewallace mentioned this pull request Dec 20, 2022

Fix main build #1928

Merged

tnull mentioned this pull request Dec 21, 2022

Add BDK-based KeysManager example lightningdevkit/lightningdevkit.org#183

Merged

TheBlueMatt mentioned this pull request Dec 21, 2022

Trivial #1910 Followups #1931

Merged

arik-so added a commit that referenced this pull request Dec 21, 2022

Merge pull request #1931 from TheBlueMatt/2022-12-1910-followups

c04d1c9

Trivial #1910 Followups

Split KeysInterface into EntropySource, NodeSigner, and SignerProvider #1910

Split KeysInterface into EntropySource, NodeSigner, and SignerProvider #1910

Conversation

arik-so commented Dec 10, 2022 • edited

TheBlueMatt commented Dec 12, 2022

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

Choose a reason for hiding this comment

arik-so commented Dec 12, 2022

codecov-commenter commented Dec 13, 2022 • edited

Codecov Report

TheBlueMatt commented Dec 13, 2022

wpaulino left a 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

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

wpaulino left a comment • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Dec 18, 2022

arik-so commented Dec 19, 2022

TheBlueMatt commented Dec 19, 2022

TheBlueMatt commented Dec 19, 2022

arik-so commented Dec 19, 2022

TheBlueMatt commented Dec 19, 2022

arik-so commented Dec 19, 2022

TheBlueMatt commented Dec 19, 2022

arik-so commented Dec 19, 2022

arik-so commented Dec 19, 2022

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 commented Dec 20, 2022

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace commented Dec 20, 2022

arik-so commented Dec 20, 2022

arik-so commented Dec 10, 2022 •

edited

codecov-commenter commented Dec 13, 2022 •

edited

wpaulino left a comment •

edited