Implement struct wrappers for Payment, DelayedPayment, HTLC and Revocation channel keys

[yellowred]

They will explicitly specify those types of keys in functions and structs allowing the language to verify what key is being used and enable idiomatic derivation of one key from another.
Only the internal public key will be saved in the monitor files, so this change will be transparent for channel monitors.

[codecov-commenter]

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (70ea110) 88.55% compared to head (7382611) 88.52%.
Report is 17 commits behind head on main.

❗ Current head 7382611 differs from pull request most recent head 935a716. Consider uploading reports for the commit 935a716 to get more accurate results

Files	Patch %	Lines
lightning/src/ln/channel_keys.rs	90.21%	2 Missing and 7 partials ⚠️
lightning/src/ln/channel.rs	87.50%	0 Missing and 2 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2675      +/-   ##
==========================================
- Coverage   88.55%   88.52%   -0.04%     
==========================================
  Files         113      114       +1     
  Lines       89323    89385      +62     
  Branches    89323    89385      +62     
==========================================
+ Hits        79097    79125      +28     
- Misses       7860     7884      +24     
- Partials     2366     2376      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[TheBlueMatt]

Thanks. Can we finish this out for all the types and make derive_public_key private now?

[TheBlueMatt]

Not sure why this got a 119 tag, its great and we should land it, but we certainly don't need to hold up a release on it.

[TheBlueMatt]

Looks like cargo test --verbose --color always --no-default-features --features=std,_test_vectors is failing, otherwise this basically LGTM.

[yellowred]

Looks like cargo test --verbose --color always --no-default-features --features=std,_test_vectors is failing, otherwise this basically LGTM.

I believe it's because it should be executed in lightning dir, otherwise grind_signatures feature is not being disabled.
😖 rust-lang/cargo#7160

[yellowred]

@wpaulino macos stable tests failed, because the env can not be built -- something in the tool chain settings? I ran ./ci/ci-tests.sh on my OSX 14 and it passed, but I have newest Rust and updates OSX developer tools. So I am not sure how could I fix this build.

[TheBlueMatt]

Looks like github just screwed up the macOS build images, dont worry about it.

[TheBlueMatt]

I'd like to see the docs provide more information for a public API, and we could probably cut down on boilerplate a bit, but generally this is fine and I'm happy to do those things in a followup if you prefer.

[TheBlueMatt]

nit: if we just make the inner pub we can probably skip the From and to_public_key impls, as users can always just write XPaymentKey(key).

Suggested change

	pub struct DelayedPaymentKey(PublicKey);
	pub struct DelayedPaymentKey(pub PublicKey);

[yellowred]

I personally enjoy that there is a an explicit step to put something into DelayedPaymentKey, but happy to remove those frictions as it may annoy more advanced users. to_public_key() is a helper, so one don't have to do payment_key.0, which is a bit cryptic.

[TheBlueMatt]

I'd appreciate if these docs did something other than just link to the BOLTs. Ideally we should at least have a full sentence or two that explains what the key is used for and stands on its own. Like, here, `The "base" point which is used, in conjunction with a key from our counterparty, to derive per-commitment delayed payment keys. The delayed payment keys are used to pay out non-HTLC-encumbered funds after a delay to give our counterparty time to punish us if we broadcast an old state."

[yellowred]

@TheBlueMatt I pushed an update to remove an LLVM optimized func and also to make inner PublicKey public for the ease of instantiation. The rest I would prefer to extract to a next PR, so the current one is not getting to old.

[TheBlueMatt]

Sure, can you go ahead and squash down fixup commits so that no commit is fixing or rewriting things done in a previous commit? Given this patchset isn't all that big, feel free to squash down into one commit if that's easier. Would also be good to get a bit of the why/what in the commit messages - rather than only a few words, commit messages really need a title (no more than 70 chars long) followed by a blank line, followed by several lines describing at least the current state of things, why that's bad, and what is being changed (each line no more than 70 chars long).

[wpaulino]

The docs should still be updated as per #2675 (comment).

The following diff could also be applied to remove some unnecessary from calls:

diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index 9f7a7b3f..ae1376ad 100644
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -2924,12 +2924,9 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 			&self.onchain_tx_handler.secp_ctx, &per_commitment_key);
 
 		let revocation_pubkey = RevocationKey::from_basepoint(&self.onchain_tx_handler.secp_ctx,
-			&RevocationBasepoint::from(self.holder_revocation_basepoint),
-			&their_per_commitment_point);
-		let delayed_payment_basepoint = DelayedPaymentBasepoint::from(self.counterparty_commitment_params.counterparty_delayed_payment_base_key);
+			&self.holder_revocation_basepoint, &their_per_commitment_point);
 		let delayed_key = DelayedPaymentKey::from_basepoint(&self.onchain_tx_handler.secp_ctx,
-			&delayed_payment_basepoint,
-			&their_per_commitment_point);
+			&self.counterparty_commitment_params.counterparty_delayed_payment_base_key, &their_per_commitment_point);
 		let revokeable_redeemscript = chan_utils::get_revokeable_redeemscript(&revocation_pubkey,
 			self.counterparty_commitment_params.on_counterparty_tx_csv, &delayed_key);
 
@@ -3107,7 +3104,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		if let Some(transaction) = tx {
 			let revocation_pubkey = RevocationKey::from_basepoint(
 				&self.onchain_tx_handler.secp_ctx,  &self.holder_revocation_basepoint, &per_commitment_point);
-			
+
 			let delayed_key = DelayedPaymentKey::from_basepoint(&self.onchain_tx_handler.secp_ctx, &self.counterparty_commitment_params.counterparty_delayed_payment_base_key, &per_commitment_point);
 
 			let revokeable_p2wsh = chan_utils::get_revokeable_redeemscript(&revocation_pubkey,
diff --git a/lightning/src/sign/mod.rs b/lightning/src/sign/mod.rs
index e73129e8..2c6da78a 100644
--- a/lightning/src/sign/mod.rs
+++ b/lightning/src/sign/mod.rs
@@ -507,13 +507,13 @@ impl HTLCDescriptor {
 		let broadcaster_keys = channel_params.broadcaster_pubkeys();
 		let counterparty_keys = channel_params.countersignatory_pubkeys();
 		let broadcaster_htlc_key = HtlcKey::from_basepoint(
-			secp, &HtlcBasepoint::from(broadcaster_keys.htlc_basepoint), &self.per_commitment_point
+			secp, &broadcaster_keys.htlc_basepoint, &self.per_commitment_point
 		);
 		let counterparty_htlc_key = HtlcKey::from_basepoint(
-			secp,  &HtlcBasepoint::from(counterparty_keys.htlc_basepoint), &self.per_commitment_point,
+			secp, &counterparty_keys.htlc_basepoint, &self.per_commitment_point,
 		);
 		let counterparty_revocation_key = &RevocationKey::from_basepoint(&secp, &counterparty_keys.revocation_basepoint, &self.per_commitment_point);
-				chan_utils::get_htlc_redeemscript_with_explicit_keys(
+		chan_utils::get_htlc_redeemscript_with_explicit_keys(
 			&self.htlc, channel_params.channel_type_features(), &broadcaster_htlc_key, &counterparty_htlc_key,
 			&counterparty_revocation_key,
 		)
@@ -1303,10 +1303,10 @@ impl EcdsaChannelSigner for InMemorySigner {
 		let witness_script = {
 			let counterparty_keys = self.counterparty_pubkeys().expect(MISSING_PARAMS_ERR);
 			let counterparty_htlcpubkey = HtlcKey::from_basepoint(
-				&secp_ctx,  &HtlcBasepoint::from(counterparty_keys.htlc_basepoint), &per_commitment_point,
+				&secp_ctx, &counterparty_keys.htlc_basepoint, &per_commitment_point,
 			);
 			let holder_htlcpubkey = HtlcKey::from_basepoint(
-				&secp_ctx,  &HtlcBasepoint::from(self.pubkeys().htlc_basepoint), &per_commitment_point,
+				&secp_ctx, &self.pubkeys().htlc_basepoint, &per_commitment_point,
 			);
 			let chan_type = self.channel_type_features().expect(MISSING_PARAMS_ERR);
 			chan_utils::get_htlc_redeemscript_with_explicit_keys(&htlc, chan_type, &counterparty_htlcpubkey, &holder_htlcpubkey, &revocation_pubkey)
@@ -1337,11 +1337,9 @@ impl EcdsaChannelSigner for InMemorySigner {
 		);
 		let counterparty_keys = self.counterparty_pubkeys().expect(MISSING_PARAMS_ERR);
 		let counterparty_htlcpubkey = HtlcKey::from_basepoint(
-			&secp_ctx,  &HtlcBasepoint::from(counterparty_keys.htlc_basepoint), &per_commitment_point,
-		);
-		let htlcpubkey = HtlcKey::from_basepoint(
-			&secp_ctx,  &HtlcBasepoint::from(self.pubkeys().htlc_basepoint), &per_commitment_point,
+			&secp_ctx, &counterparty_keys.htlc_basepoint, &per_commitment_point,
 		);
+		let htlcpubkey = HtlcKey::from_basepoint(&secp_ctx, &self.pubkeys().htlc_basepoint, &per_commitment_point);
 		let chan_type = self.channel_type_features().expect(MISSING_PARAMS_ERR);
 		let witness_script = chan_utils::get_htlc_redeemscript_with_explicit_keys(&htlc, chan_type, &counterparty_htlcpubkey, &htlcpubkey, &revocation_pubkey);
 		let mut sighash_parts = sighash::SighashCache::new(htlc_tx);
diff --git a/lightning/src/util/test_channel_signer.rs b/lightning/src/util/test_channel_signer.rs
index 037c7004..bfa1d804 100644
--- a/lightning/src/util/test_channel_signer.rs
+++ b/lightning/src/util/test_channel_signer.rs
@@ -232,7 +232,7 @@ impl EcdsaChannelSigner for TestChannelSigner {
 				input, &witness_script, htlc_descriptor.htlc.amount_msat / 1000, sighash_type
 			).unwrap();
 			let countersignatory_htlc_key = HtlcKey::from_basepoint(
-				&secp_ctx,&HtlcBasepoint::from(self.inner.counterparty_pubkeys().unwrap().htlc_basepoint), &htlc_descriptor.per_commitment_point,
+				&secp_ctx, &self.inner.counterparty_pubkeys().unwrap().htlc_basepoint, &htlc_descriptor.per_commitment_point,
 			);
 
 			secp_ctx.verify_ecdsa(

[yellowred]

@wpaulino @TheBlueMatt Got some time today and added the docs and applied the diff. Also rebased the main into it, so now it's on the bitcoin v0.30 crate.

[TheBlueMatt]

Aside from some style and docs this LGTM. I did go ahead and amend the commit to break up the long lines in the commit message to ensure they read well, though did not change the commit contents.

As I touched it, I'll let @wpaulino merge.