Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement PSBT fields that were missing for a Signer #2761

Merged
merged 1 commit into from
Apr 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lightning/src/chain/channelmonitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4330,6 +4330,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
revocation_pubkey: broadcasted_holder_revokable_script.2,
channel_keys_id: self.channel_keys_id,
channel_value_satoshis: self.channel_value_satoshis,
channel_transaction_parameters: Some(self.onchain_tx_handler.channel_transaction_parameters.clone()),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cloning of channel_transaction_parameters from self.onchain_tx_handler is noted. While cloning ensures data integrity and ownership rules compliance, it's essential to consider the performance implications for large data structures. If channel_transaction_parameters is sizable or complex, evaluating alternative strategies like borrowing (if lifecycle permits) or implementing a more efficient cloning method (e.g., using Arc for shared ownership) could enhance performance.

}));
}
}
Expand Down
110 changes: 79 additions & 31 deletions lightning/src/ln/channel_keys.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,19 +10,19 @@
//! Keys used to generate commitment transactions.
//! See: <https://github.com/lightning/bolts/blob/master/03-transactions.md#keys>

use bitcoin::hashes::Hash;
use bitcoin::hashes::HashEngine;
use bitcoin::secp256k1::Scalar;
use bitcoin::secp256k1::SecretKey;
use bitcoin::secp256k1::Secp256k1;
use bitcoin::secp256k1;
use crate::io;
use crate::ln::msgs::DecodeError;
use crate::util::ser::Readable;
use crate::io;
use crate::util::ser::Writer;
use crate::util::ser::Writeable;
use bitcoin::secp256k1::PublicKey;
use crate::util::ser::Writer;
use bitcoin::hashes::sha256::Hash as Sha256;
use bitcoin::hashes::Hash;
use bitcoin::hashes::HashEngine;
use bitcoin::secp256k1;
use bitcoin::secp256k1::PublicKey;
use bitcoin::secp256k1::Scalar;
use bitcoin::secp256k1::Secp256k1;
use bitcoin::secp256k1::SecretKey;

macro_rules! doc_comment {
($x:expr, $($tt:tt)*) => {
Expand All @@ -37,15 +37,28 @@ macro_rules! basepoint_impl {
pub fn to_public_key(&self) -> PublicKey {
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:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We really need some paragraph breaks in here.

/// `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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should work?

Suggested change
/// to the `from_basepoint` method, but without the addition operation, providing just the
/// to the [`Self::from_basepoint`] method, but without the addition operation, providing just the

/// tweak from the hash of the per_commitment_point and the basepoint.
pub fn derive_add_tweak(&self, per_commitment_point: &PublicKey) -> [u8; 32] {
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()
}
}

impl From<PublicKey> for $BasepointT {
fn from(value: PublicKey) -> Self {
Self(value)
}
}

}
};
}
macro_rules! key_impl {
($BasepointT:ty, $KeyName:expr) => {
Expand Down Expand Up @@ -87,11 +100,9 @@ macro_rules! key_read_write {
Ok(Self(key))
}
}
}
};
}



/// Base key used in conjunction with a `per_commitment_point` to generate a [`DelayedPaymentKey`].
///
/// The delayed payment key is used to pay the commitment state broadcaster their
Expand All @@ -102,7 +113,6 @@ pub struct DelayedPaymentBasepoint(pub PublicKey);
basepoint_impl!(DelayedPaymentBasepoint);
key_read_write!(DelayedPaymentBasepoint);


/// A derived key built from a [`DelayedPaymentBasepoint`] and `per_commitment_point`.
///
/// The delayed payment key is used to pay the commitment state broadcaster their
Expand Down Expand Up @@ -150,14 +160,26 @@ key_read_write!(HtlcKey);
/// Derives a per-commitment-transaction public key (eg an htlc key or a delayed_payment key)
/// from the base point and the per_commitment_key. This is the public equivalent of
/// derive_private_key - using only public keys to derive a public key instead of private keys.
fn derive_public_key<T: secp256k1::Signing>(secp_ctx: &Secp256k1<T>, per_commitment_point: &PublicKey, base_point: &PublicKey) -> PublicKey {
fn derive_public_key<T: secp256k1::Signing>(
secp_ctx: &Secp256k1<T>, per_commitment_point: &PublicKey, base_point: &PublicKey,
) -> PublicKey {
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 hashkey = PublicKey::from_secret_key(&secp_ctx,
&SecretKey::from_slice(&res).expect("Hashes should always be valid keys unless SHA-256 is broken"));
add_public_key_tweak(secp_ctx, base_point, &res)
}

/// Adds a tweak to a public key to derive a new public key.
pub fn add_public_key_tweak<T: secp256k1::Signing>(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this can panic if the tweak isn't a hash, I'd prefer if we either make this crate-private or at least document it.

secp_ctx: &Secp256k1<T>, base_point: &PublicKey, tweak: &[u8; 32],
) -> PublicKey {
let hashkey = PublicKey::from_secret_key(
&secp_ctx,
&SecretKey::from_slice(tweak)
.expect("Hashes should always be valid keys unless SHA-256 is broken"),
);
base_point.combine(&hashkey)
.expect("Addition only fails if the tweak is the inverse of the key. This is not possible when the tweak contains the hash of the key.")
}
Expand All @@ -169,7 +191,6 @@ pub struct RevocationBasepoint(pub PublicKey);
basepoint_impl!(RevocationBasepoint);
key_read_write!(RevocationBasepoint);


/// The revocation key is used to allow a channel party to revoke their state - giving their
/// counterparty the required material to claim all of their funds if they broadcast that state.
///
Expand All @@ -192,8 +213,7 @@ impl RevocationKey {
///
/// [`chan_utils::derive_private_revocation_key`]: crate::ln::chan_utils::derive_private_revocation_key
pub fn from_basepoint<T: secp256k1::Verification>(
secp_ctx: &Secp256k1<T>,
countersignatory_basepoint: &RevocationBasepoint,
secp_ctx: &Secp256k1<T>, countersignatory_basepoint: &RevocationBasepoint,
per_commitment_point: &PublicKey,
) -> Self {
let rev_append_commit_hash_key = {
Expand Down Expand Up @@ -227,28 +247,56 @@ impl RevocationKey {
}
key_read_write!(RevocationKey);


#[cfg(test)]
mod test {
use bitcoin::secp256k1::{Secp256k1, SecretKey, PublicKey};
use bitcoin::hashes::hex::FromHex;
use super::derive_public_key;
use bitcoin::hashes::hex::FromHex;
use bitcoin::secp256k1::{PublicKey, Secp256k1, SecretKey};

#[test]
fn test_key_derivation() {
// Test vectors from BOLT 3 Appendix E:
let secp_ctx = Secp256k1::new();

let base_secret = SecretKey::from_slice(&<Vec<u8>>::from_hex("000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f").unwrap()[..]).unwrap();
let per_commitment_secret = SecretKey::from_slice(&<Vec<u8>>::from_hex("1f1e1d1c1b1a191817161514131211100f0e0d0c0b0a09080706050403020100").unwrap()[..]).unwrap();
let base_secret = SecretKey::from_slice(
&<Vec<u8>>::from_hex(
"000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f",
)
.unwrap()[..],
)
.unwrap();
let per_commitment_secret = SecretKey::from_slice(
&<Vec<u8>>::from_hex(
"1f1e1d1c1b1a191817161514131211100f0e0d0c0b0a09080706050403020100",
)
.unwrap()[..],
)
.unwrap();

let base_point = PublicKey::from_secret_key(&secp_ctx, &base_secret);
assert_eq!(base_point.serialize()[..], <Vec<u8>>::from_hex("036d6caac248af96f6afa7f904f550253a0f3ef3f5aa2fe6838a95b216691468e2").unwrap()[..]);
assert_eq!(
base_point.serialize()[..],
<Vec<u8>>::from_hex(
"036d6caac248af96f6afa7f904f550253a0f3ef3f5aa2fe6838a95b216691468e2"
)
.unwrap()[..]
);

let per_commitment_point = PublicKey::from_secret_key(&secp_ctx, &per_commitment_secret);
assert_eq!(per_commitment_point.serialize()[..], <Vec<u8>>::from_hex("025f7117a78150fe2ef97db7cfc83bd57b2e2c0d0dd25eaf467a4a1c2a45ce1486").unwrap()[..]);

assert_eq!(derive_public_key(&secp_ctx, &per_commitment_point, &base_point).serialize()[..],
<Vec<u8>>::from_hex("0235f2dbfaa89b57ec7b055afe29849ef7ddfeb1cefdb9ebdc43f5494984db29e5").unwrap()[..]);
assert_eq!(
per_commitment_point.serialize()[..],
<Vec<u8>>::from_hex(
"025f7117a78150fe2ef97db7cfc83bd57b2e2c0d0dd25eaf467a4a1c2a45ce1486"
)
.unwrap()[..]
);

assert_eq!(
derive_public_key(&secp_ctx, &per_commitment_point, &base_point).serialize()[..],
<Vec<u8>>::from_hex(
"0235f2dbfaa89b57ec7b055afe29849ef7ddfeb1cefdb9ebdc43f5494984db29e5"
)
.unwrap()[..]
);
}
}
Loading
Loading