Skip to content
69 changes: 57 additions & 12 deletions lightning/src/sign/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1983,29 +1983,61 @@ impl KeysManager {
.expect("Your RNG is busted");
unique_start.input(&child_privkey.private_key[..]);

let seed = Sha256::from_engine(unique_start).to_byte_array();
let unique_seed = Sha256::from_engine(unique_start).to_byte_array();

let commitment_seed = {
let mut sha = Sha256::engine();
sha.input(&seed);
sha.input(&unique_seed);
sha.input(&b"commitment seed"[..]);
Sha256::from_engine(sha).to_byte_array()
};
macro_rules! key_step {
($info: expr, $prev_key: expr) => {{
let mut sha = Sha256::engine();
sha.input(&seed);
sha.input(&unique_seed);
sha.input(&$prev_key[..]);
sha.input(&$info[..]);
SecretKey::from_slice(&Sha256::from_engine(sha).to_byte_array())
.expect("SHA-256 is busted")
}};
}
let funding_key = key_step!(b"funding key", commitment_seed);
let revocation_base_key = key_step!(b"revocation base key", funding_key);
let payment_key = key_step!(b"payment key", revocation_base_key);
let delayed_payment_base_key = key_step!(b"delayed payment base key", payment_key);
let htlc_base_key = key_step!(b"HTLC base key", delayed_payment_base_key);

let funding_key;
let revocation_base_key;
let payment_key;
let delayed_payment_base_key;
let htlc_base_key;

let channel_keys_derivation_version =
channel_keys_derivation_version_from_id(channel_keys_id);
if channel_keys_derivation_version < 1 {
// In LDK versions prior to 0.1 we used to derive the `payment_key` uniquely on a
// per-channel basis, which disallowed users to re-derive them if they lost their
// `channel_keys_id`.
funding_key = key_step!(b"funding key", commitment_seed);
revocation_base_key = key_step!(b"revocation base key", funding_key);
payment_key = key_step!(b"payment key", revocation_base_key);
delayed_payment_base_key = key_step!(b"delayed payment base key", payment_key);
htlc_base_key = key_step!(b"HTLC base key", delayed_payment_base_key);
} else {
funding_key = key_step!(b"funding key", commitment_seed);
revocation_base_key = key_step!(b"revocation base key", funding_key);
delayed_payment_base_key = key_step!(b"delayed payment base key", revocation_base_key);
htlc_base_key = key_step!(b"HTLC base key", delayed_payment_base_key);

// Starting with LDK v0.1, we derive `payment_key` directly from our seed, allowing
// users to re-derive it even when losing all channel state. This allows them to
// recover any non-HTLC-encumbered funds in case of data loss if the counterparty is so
// nice to force-closure for them.
payment_key = {
let mut sha = Sha256::engine();
sha.input(&self.seed);
sha.input(&b"static payment key"[..]);
SecretKey::from_slice(&Sha256::from_engine(sha).to_byte_array())
.expect("SHA-256 is busted")
};
Copy link
Contributor

Choose a reason for hiding this comment

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

question: doesn't this derivation create the same payment_key across all the InMemorySigner's derived by this KeysManager ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is essentially the intended outcome. Or rather, we want to be able to derive a payment key without including additional data such as channel keys id.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I'm concerned about a privacy leak here - in case of force close of two channels onchain, these commit tx would both have the same scriptpubkey in the to_remote output right ?

I know CLN hsm_tool allows people to recover their to_remote outputs via a trial-and-error method, we could take a look at how they derive their payment_key.

See hsmtool guesstoremote <P2WPKH address> <node id> <tries> "<path/to/hsm_secret>"

Copy link
Contributor

Choose a reason for hiding this comment

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

That guesstoremote method is intended to be used in case the local party loses all channel state, and the remote party force closes the channel - the same scenario addressed in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

One more clarification: <node id> in that method is the node id of the remote party, not the local party.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I'm concerned about a privacy leak here - in case of force close of two channels onchain, these commit tx would both have the same scriptpubkey in the to_remote output right ?

Yes, this is true and a good point, although note we currently have that issue with destination/shutdown scripts, too. Though we plan to change this with #1139 which I considered to pick up as part / as a follow-up of this PR.

I guess if we manage to completely isolate the payment_key derivation to a interface method we could consider including the counterparty_node_id, or, even better have the user override that method so that it returns a fresh address from their wallet, which would also get rid of the additional spending step for StaticPaymentOutputs. We considered that refactor before, but so far intended to punt on it as it's likely going to be a larger one and maintaining backwards compatibility for all of this might get tricky. But yeah, maybe you're right and we should try to do it 'the right way' right away.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is true and a good point, although note we currently have that issue with destination/shutdown scripts, too. Though we plan to change this with #1139 which I considered to pick up as part / as a follow-up of this PR.

I see ! I had overlooked this code :) I let you determine the best path forward here.

I guess if we manage to completely isolate the payment_key derivation to a interface method we could consider including the counterparty_node_id, or, even better have the user override that method so that it returns a fresh address from their wallet, which would also get rid of the additional spending step for StaticPaymentOutputs.

Can't the user already decide how payment_key is derived in SignerProvider::derive_channel_signer ?

We are also constrained by the LN spec if the channel has anchors. But I agree that it would be very cool in the non-anchor case to have the funds land directly in the on-chain wallet from the commitment transaction.

I took a brief look at the API of BDK's Wallet, and they don't offer an easy way to ask for raw public keys - just addresses - which makes sense considering that the core of the wallet is an output descriptor.

But yeah, maybe you're right and we should try to do it 'the right way' right away.

Your call :)

};

let prng_seed = self.get_secure_random_bytes();

InMemorySigner::new(
Expand Down Expand Up @@ -2249,6 +2281,14 @@ impl OutputSpender for KeysManager {
}
}

/// The version of the channel key derivation scheme.
pub(crate) const CHANNEL_KEYS_DERIVATION_VERSION: u8 = 1;

/// Returns the keys derviation version set in `channel_keys_id`.
pub(crate) fn channel_keys_derivation_version_from_id(channel_keys_id: [u8; 32]) -> u8 {
channel_keys_id[0]
}

impl SignerProvider for KeysManager {
type EcdsaSigner = InMemorySigner;
#[cfg(taproot)]
Expand All @@ -2261,11 +2301,16 @@ impl SignerProvider for KeysManager {
// `child_idx` is the only thing guaranteed to make each channel unique without a restart
// (though `user_channel_id` should help, depending on user behavior). If it manages to
// roll over, we may generate duplicate keys for two different channels, which could result
// in loss of funds. Because we only support 32-bit+ systems, assert that our `AtomicUsize`
// doesn't reach `u32::MAX`.
assert!(child_idx < core::u32::MAX as usize, "2^32 channels opened without restart");
// in loss of funds. Because we only support 32-bit+ systems, and we use the first byte as
// a versioning field, assert that our `AtomicUsize`
// doesn't reach the maximal 24-bit value, i.e., `U24_MAX`.
const U24_MAX: usize = 0xFFFFFF;
assert!(child_idx < U24_MAX, "2^24 channels opened without restart");
let child_idx_be_bytes = (child_idx as u32).to_be_bytes();

let mut id = [0; 32];
id[0..4].copy_from_slice(&(child_idx as u32).to_be_bytes());
id[0] = CHANNEL_KEYS_DERIVATION_VERSION;
id[1..4].copy_from_slice(&child_idx_be_bytes[1..4]);
id[4..8].copy_from_slice(&self.starting_time_nanos.to_be_bytes());
id[8..16].copy_from_slice(&self.starting_time_secs.to_be_bytes());
id[16..32].copy_from_slice(&user_channel_id.to_be_bytes());
Expand Down