From 23d9514259d0e9f8d9caecd96c0818674aaf7d92 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 19 Apr 2024 00:28:09 +0000 Subject: [PATCH 1/6] Simplify and clarify `derive_add_tweak` documentation --- lightning/src/ln/channel_keys.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lightning/src/ln/channel_keys.rs b/lightning/src/ln/channel_keys.rs index 423d4107407..76dc1e42b37 100644 --- a/lightning/src/ln/channel_keys.rs +++ b/lightning/src/ln/channel_keys.rs @@ -38,13 +38,13 @@ macro_rules! basepoint_impl { self.0 } - /// Derives a per-commitment-transaction (eg an htlc key or delayed_payment key) private key addition tweak - /// from a basepoint and a per_commitment_point: + /// Derives the "tweak" used to calculate the per-commitment private key. + /// + /// The per-commitment private key is calculates a private key as: /// `privkey = basepoint_secret + SHA256(per_commitment_point || basepoint)` - /// This calculates the hash part in the tweak derivation process, which is used to ensure - /// that each key is unique and cannot be guessed by an external party. It is equivalent - /// to the `from_basepoint` method, but without the addition operation, providing just the - /// tweak from the hash of the per_commitment_point and the basepoint. + /// + /// This calculates the hash part in the tweak derivation process, which is used to + /// ensure that each key is unique and cannot be guessed by an external party. pub fn derive_add_tweak(&self, per_commitment_point: &PublicKey) -> [u8; 32] { let mut sha = Sha256::engine(); sha.input(&per_commitment_point.serialize()); From 9f1c9062db3278597429751c00b3cc7b0ef277e0 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 19 Apr 2024 00:36:52 +0000 Subject: [PATCH 2/6] Use `Sha256`s for tweaks in `sign` to enforce randomness We assume that tweaks are the output of a SHA-256 hash function (and thus that failing to create a private key from the has negligible probability) in `add_public_key_tweak` and elsewhere. Thus, we really shouldn't be taking byte arrays in the public API but rather `Sha256` objects, and communicating in the docs for `add_public_key_tweak` that we can panic if its not the output of a hash function, both of which we do here. --- lightning/src/ln/channel_keys.rs | 12 +++++++----- lightning/src/sign/mod.rs | 2 +- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/lightning/src/ln/channel_keys.rs b/lightning/src/ln/channel_keys.rs index 76dc1e42b37..eaa14f27f33 100644 --- a/lightning/src/ln/channel_keys.rs +++ b/lightning/src/ln/channel_keys.rs @@ -45,11 +45,11 @@ macro_rules! basepoint_impl { /// /// This calculates the hash part in the tweak derivation process, which is used to /// ensure that each key is unique and cannot be guessed by an external party. - pub fn derive_add_tweak(&self, per_commitment_point: &PublicKey) -> [u8; 32] { + pub fn derive_add_tweak(&self, per_commitment_point: &PublicKey) -> Sha256 { let mut sha = Sha256::engine(); sha.input(&per_commitment_point.serialize()); sha.input(&self.to_public_key().serialize()); - Sha256::from_engine(sha).to_byte_array() + Sha256::from_engine(sha) } } @@ -166,18 +166,20 @@ fn derive_public_key( let mut sha = Sha256::engine(); sha.input(&per_commitment_point.serialize()); sha.input(&base_point.serialize()); - let res = Sha256::from_engine(sha).to_byte_array(); + let res = Sha256::from_engine(sha); add_public_key_tweak(secp_ctx, base_point, &res) } /// Adds a tweak to a public key to derive a new public key. +/// +/// May panic if `tweak` is not the output of a SHA-256 hash. pub fn add_public_key_tweak( - secp_ctx: &Secp256k1, base_point: &PublicKey, tweak: &[u8; 32], + secp_ctx: &Secp256k1, base_point: &PublicKey, tweak: &Sha256, ) -> PublicKey { let hashkey = PublicKey::from_secret_key( &secp_ctx, - &SecretKey::from_slice(tweak) + &SecretKey::from_slice(tweak.as_byte_array()) .expect("Hashes should always be valid keys unless SHA-256 is broken"), ); base_point.combine(&hashkey) diff --git a/lightning/src/sign/mod.rs b/lightning/src/sign/mod.rs index 43acc67af04..8148c88e104 100644 --- a/lightning/src/sign/mod.rs +++ b/lightning/src/sign/mod.rs @@ -401,7 +401,7 @@ impl SpendableOutputDescriptor { subtype: 0, key: "add_tweak".as_bytes().to_vec(), }, - add_tweak.to_vec(), + add_tweak.as_byte_array().to_vec(), )] .into_iter() .collect() From db31f43b87c89319cd6513b5bbf043bb2b55616e Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 19 Apr 2024 00:42:55 +0000 Subject: [PATCH 3/6] Word-wrap new doc comment in `DelayedPaymentOutputDescriptor` --- lightning/src/sign/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lightning/src/sign/mod.rs b/lightning/src/sign/mod.rs index 8148c88e104..3634f84aa0e 100644 --- a/lightning/src/sign/mod.rs +++ b/lightning/src/sign/mod.rs @@ -110,8 +110,8 @@ pub struct DelayedPaymentOutputDescriptor { pub channel_keys_id: [u8; 32], /// The value of the channel which this output originated from, possibly indirectly. pub channel_value_satoshis: u64, - /// The channel public keys and other parameters needed to generate a spending transaction or to provide to a re-derived signer through - /// [`ChannelSigner::provide_channel_parameters`]. + /// The channel public keys and other parameters needed to generate a spending transaction or + /// to provide to a re-derived signer through [`ChannelSigner::provide_channel_parameters`]. /// /// Added as optional, but always `Some` if the descriptor was produced in v0.0.123 or later. pub channel_transaction_parameters: Option, From 57c8275e0e5bac59a873581d6bbda048866e1fd0 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 22 Apr 2024 12:50:58 +0000 Subject: [PATCH 4/6] Only include `derive_add_tweak` for base key types that use it Specifically `RevocationBasepoint` has a different derivation, so shouldn't have a `derive_add_tweak` at all. We also use this opportunity to link to the `from_basepoint` function in the `derive_add_tweak` docs. --- lightning/src/ln/channel_keys.rs | 36 ++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/lightning/src/ln/channel_keys.rs b/lightning/src/ln/channel_keys.rs index eaa14f27f33..9e839b15e3c 100644 --- a/lightning/src/ln/channel_keys.rs +++ b/lightning/src/ln/channel_keys.rs @@ -31,26 +31,30 @@ macro_rules! doc_comment { }; } macro_rules! basepoint_impl { - ($BasepointT:ty) => { + ($BasepointT:ty $(, $KeyName: expr)?) => { impl $BasepointT { /// Get inner Public Key pub fn to_public_key(&self) -> PublicKey { self.0 } - /// Derives the "tweak" used to calculate the per-commitment private key. - /// - /// The per-commitment private key is calculates a private key as: - /// `privkey = basepoint_secret + SHA256(per_commitment_point || basepoint)` - /// - /// This calculates the hash part in the tweak derivation process, which is used to - /// ensure that each key is unique and cannot be guessed by an external party. - pub fn derive_add_tweak(&self, per_commitment_point: &PublicKey) -> Sha256 { - let mut sha = Sha256::engine(); - sha.input(&per_commitment_point.serialize()); - sha.input(&self.to_public_key().serialize()); - Sha256::from_engine(sha) - } + $(doc_comment!( + concat!( + "Derives the \"tweak\" used in calculate [`", $KeyName, "::from_basepoint`].\n", + "\n", + "[`", $KeyName, "::from_basepoint`] calculates a private key as:\n", + "`privkey = basepoint_secret + SHA256(per_commitment_point || basepoint)`\n", + "\n", + "This calculates the hash part in the tweak derivation process, which is used to\n", + "ensure that each key is unique and cannot be guessed by an external party." + ), + pub fn derive_add_tweak(&self, per_commitment_point: &PublicKey) -> Sha256 { + let mut sha = Sha256::engine(); + sha.input(&per_commitment_point.serialize()); + sha.input(&self.to_public_key().serialize()); + Sha256::from_engine(sha) + }); + )? } impl From for $BasepointT { @@ -110,7 +114,7 @@ macro_rules! key_read_write { /// state broadcasted was previously revoked. #[derive(PartialEq, Eq, Clone, Copy, Debug, Hash)] pub struct DelayedPaymentBasepoint(pub PublicKey); -basepoint_impl!(DelayedPaymentBasepoint); +basepoint_impl!(DelayedPaymentBasepoint, "DelayedPaymentKey"); key_read_write!(DelayedPaymentBasepoint); /// A derived key built from a [`DelayedPaymentBasepoint`] and `per_commitment_point`. @@ -137,7 +141,7 @@ key_read_write!(DelayedPaymentKey); /// Thus, both channel counterparties' HTLC keys will appears in each HTLC output's script. #[derive(PartialEq, Eq, Clone, Copy, Debug, Hash)] pub struct HtlcBasepoint(pub PublicKey); -basepoint_impl!(HtlcBasepoint); +basepoint_impl!(HtlcBasepoint, "HtlcKey"); key_read_write!(HtlcBasepoint); /// A derived key built from a [`HtlcBasepoint`] and `per_commitment_point`. From eeea0f473e3d9e6f7886f618cb5c3e32e1d48361 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 22 Apr 2024 13:00:38 +0000 Subject: [PATCH 5/6] Include `channel_keys.rs` in the rustfmt list Since its already formatted and there's not much active work going on in it. --- rustfmt_excluded_files | 1 - 1 file changed, 1 deletion(-) diff --git a/rustfmt_excluded_files b/rustfmt_excluded_files index a6a81a0f586..51731c40d09 100644 --- a/rustfmt_excluded_files +++ b/rustfmt_excluded_files @@ -190,7 +190,6 @@ ./lightning/src/ln/chanmon_update_fail_tests.rs ./lightning/src/ln/channel.rs ./lightning/src/ln/channel_id.rs -./lightning/src/ln/channel_keys.rs ./lightning/src/ln/channelmanager.rs ./lightning/src/ln/features.rs ./lightning/src/ln/functional_test_utils.rs From 6ab91cb43fbaeeb9dbf26b1c1c953373bc9fd4cf Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 22 Apr 2024 13:03:14 +0000 Subject: [PATCH 6/6] Drop the rustup-specific calling in `ci/rustfmt.sh` The +rustversion call semantics are specific to rustup and do not work with standard rust toolchains. However, because rustfmt formatting differs slightly between stable and our 1.63 target, we need to keep the +1.63.0 for rustup users. Thus, here, we check for the presence of a `rustup` command and pass the "+1.63.0" argument if we find one. --- ci/rustfmt.sh | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/ci/rustfmt.sh b/ci/rustfmt.sh index dfc181dee79..a1fdf6bd7f3 100755 --- a/ci/rustfmt.sh +++ b/ci/rustfmt.sh @@ -4,10 +4,16 @@ set -eox pipefail # Generate initial exclusion list #find . -name '*.rs' -type f |sort >rustfmt_excluded_files +# The +rustversion syntax only works with rustup-installed rust toolchains, +# not with any distro-provided ones. Thus, we check for a rustup install and +# only pass +1.63.0 if we find one. +VERS="" +[ "$(which rustup)" != "" ] && VERS="+1.63.0" + # Run fmt TMP_FILE=$(mktemp) find . -name '*.rs' -type f |sort >$TMP_FILE for file in $(comm -23 $TMP_FILE rustfmt_excluded_files); do echo "Checking formatting of $file" - rustfmt +1.63.0 --check $file + rustfmt $VERS --check $file done