Allow KeysManager to opt-into the new remote_key derivation
#4117
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4117 +/- ##
==========================================
+ Coverage 88.54% 88.72% +0.18%
==========================================
Files 179 180 +1
Lines 134333 135699 +1366
Branches 134333 135699 +1366
==========================================
+ Hits 118939 120405 +1466
+ Misses 12634 12519 -115
- Partials 2760 2775 +15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
TheBlueMatt
force-pushed
the
2025-09-actually-static-remote_key
branch
from
b8179e0 to
6fc6072
Compare
TheBlueMatt
force-pushed
the
2025-09-actually-static-remote_key
branch
from
6fc6072 to
9871b97
Compare
| funding_key: SecretKey, revocation_base_key: SecretKey, payment_key_v1: SecretKey, | ||
| payment_key_v2: SecretKey, delayed_payment_base_key: SecretKey, htlc_base_key: SecretKey, | ||
| commitment_seed: [u8; 32], channel_keys_id: [u8; 32], rand_bytes_unique_start: [u8; 32], | ||
| payment_key_v2: SecretKey, v2_remote_key_derivation: bool, |
There was a problem hiding this comment.
In what circumstances wouldn't users want to set v2_remote_key_derivation, given that it only applies to new/spliced channels? Do we even need that flag, or should this always be true?
There was a problem hiding this comment.
It breaks downgrade.
There was a problem hiding this comment.
It breaks downgrade.
Hmm, I assume any v2-channel-open/spliced channel would probably not be downgradeable anyways? Should we maybe always use it for V2 channels, and have this flag only apply to legacy channels?
There was a problem hiding this comment.
Hmm, that's an interesting idea. I assume you're thinking basically in ldk-node you'd never set the flag and then let users who choose to splice/enable dual-funding (once we get there) to opt into backwards incompatibility via those methods?
I suppose its trivial to just always enable for splices, so I'll go ahead and do that.
There was a problem hiding this comment.
Hmm, that's an interesting idea. I assume you're thinking basically in ldk-node you'd never set the flag and then let users who choose to splice/enable dual-funding (once we get there) to opt into backwards incompatibility via those methods?
No, in LDK Node we'll def. set the flag ASAP, as (unfortunately) we don't support downgrades currently anyways (given that BDK never gave any such guarantees).
I suppose its trivial to just always enable for splices, so I'll go ahead and do that.
Thanks!
| /// If `v2_remote_key_derivation` is set, the `script_pubkey`s which receive funds on-chain when | ||
| /// our counterparty force-closes will be one of a static set of [`STATIC_PAYMENT_KEY_COUNT`]*2 | ||
| /// possible `script_pubkey`s. This only applies to new or spliced channels, however if this is | ||
| /// set you *MUST NOT* downgrade to a version of LDK prior to 0.2. |
There was a problem hiding this comment.
Should we somehow persist something that fails the downgrade to enforce users won't ever attempt to?
There was a problem hiding this comment.
Yea, I'd love to, but its not clear to me how - none of the keys stuff gets persisted and doing some crazy dance to (a) detect a default KeysManager and then (b) persist a upgrade-breaking flag in ChannelMonitor seems too hacky to be worth it.
There was a problem hiding this comment.
Mhh, alright. But, if we did #4117 (comment) we would at least get that for free for any spliced/V2 channels I assume?
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
Yea, we should definitely still consider redoing the keys interface so that this key is fetched separate from the normal channel keys to make it easier to override. Of course users should be able to override it no problem already, but making it easier would be good. |
|
It might be worth noting that with anchor channels you can't just trivially use a regular on-chain wallet key - the output is actually a P2WSH of the key and CSV 1, so I think we shouldn't just fully require the user give us a key. |
|
✅ Added second reviewer: @valentinewallace |
tnull
requested review from
tnull and
tankyleo
and removed request for
valentinewallace
|
✅ Added second reviewer: @valentinewallace |
TheBlueMatt
force-pushed
the
2025-09-actually-static-remote_key
branch
from
9871b97 to
e0cc7c9
Compare
| SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4, self.node_secret[31]]).unwrap(), | ||
| SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 5, self.node_secret[31]]).unwrap(), | ||
| SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 6, self.node_secret[31]]).unwrap(), | ||
| SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 6, self.node_secret[31]]).unwrap(), |
There was a problem hiding this comment.
Would there any benefit if we used a different key here? Is this kept the same for convenience here and below? If so, maybe add a comment to explain why these are duplicated?
There was a problem hiding this comment.
I don't think so, its really just a key to use, I added a comment.
lightning/src/sign/mod.rs
Outdated
| revocation_base_key, | ||
| payment_key, | ||
| payment_key_v1, | ||
| self.derive_payment_key_v2(&commitment_seed), |
There was a problem hiding this comment.
Just curious: Any particular reason we are using commitment_seed and not seed here?
There was a problem hiding this comment.
Shouldn't matter.
|
🔔 1st Reminder Hey @tankyleo! This PR has been waiting for your review. |
TheBlueMatt
force-pushed
the
2025-09-actually-static-remote_key
branch
from
e0cc7c9 to
822aa83
Compare
TheBlueMatt
force-pushed
the
2025-09-actually-static-remote_key
branch
from
822aa83 to
febc9e4
Compare
TheBlueMatt
requested review from
valentinewallace and
tnull
and removed request for
tankyleo
|
Can we squash? |
The `remote_key` derived by default in `KeysManager` depends on the chanel's `channel_keys_id`, which generally has sufficient entropy that without it the `remote_key` cannot be re-derived. In disaster case where there is no remaining state except the `KeysManager`'s `seed`, this results in lost funds, even if the counterparty force-closes the channel. Luckily, because of the `static_remote_key` feature, there's no need for this. If the `remote_key` we derive is one of a countable set, we can simply scan the chain for outputs to our `remote_key`s. Here we set up such new derivation, adding logic to derive one of 1000 possible `remote_key`s (which translates to 2000 potential `script_pubkey`s on chain). We also update the spending code to check which of the two derivation formats where used and sign with the correct key.
The `remote_key` derived by default in `KeysManager` depends on the chanel's `channel_keys_id`, which generally has sufficient entropy that without it the `remote_key` cannot be re-derived. In disaster case where there is no remaining state except the `KeysManager`'s `seed`, this results in lost funds, even if the counterparty force-closes the channel. Luckily, because of the `static_remote_key` feature, there's no need for this. If the `remote_key` we derive is one of a countable set, we can simply scan the chain for outputs to our `remote_key`s. In the next commit, we'll start using different `remote_key`s based on a config knob the user sets, but with the current `ChannelSigner::pubkeys` API this would be invalid - we can't return a different set of keys for a re-derived `ChannelSigner`. Luckily, this isn't actually how LDK uses `ChannelSigner::pubkeys`, it actually only calls it when it wants a new set of pubkeys, either for a new channel or a splice. Thus, here, we rename `ChannelSigner::pubkeys` to `ChannelSigner::new_pubkeys` and update documentation to match.
The `remote_key` derived by default in `KeysManager` depends on the chanel's `channel_keys_id`, which generally has sufficient entropy that without it the `remote_key` cannot be re-derived. In disaster case where there is no remaining state except the `KeysManager`'s `seed`, this results in lost funds, even if the counterparty force-closes the channel. Luckily, because of the `static_remote_key` feature, there's no need for this. If the `remote_key` we derive is one of a countable set, we can simply scan the chain for outputs to our `remote_key`s. Here we finally allow users to opt into the new derivation scheme, using the new derivation scheme for `remote_key`s for new and spliced channels if a new `KeysManager::new` argument is set to `true`.
TheBlueMatt
force-pushed
the
2025-09-actually-static-remote_key
branch
from
febc9e4 to
de46c84
Compare
|
Went ahead and squashed. The diff since @tnull took a look (according to github's history) is $ git diff-tree -U2 e0cc7c9 de46c8457
diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs
index ca03139a43..b697fafe46 100644
--- a/fuzz/src/chanmon_consistency.rs
+++ b/fuzz/src/chanmon_consistency.rs
@@ -388,4 +388,6 @@ impl SignerProvider for KeyProvider {
SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4, self.node_secret[31]]).unwrap(),
SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 5, self.node_secret[31]]).unwrap(),
+ // We leave both the v1 and v2 derivation to_remote keys the same as there's not any
+ // real reason to fuzz differences here.
SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 6, self.node_secret[31]]).unwrap(),
SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 6, self.node_secret[31]]).unwrap(),
diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs
index 9f961bc814..277dc62612 100644
--- a/fuzz/src/full_stack.rs
+++ b/fuzz/src/full_stack.rs
@@ -477,4 +477,6 @@ impl SignerProvider for KeyProvider {
key[30] = 6 + if inbound { 0 } else { 6 };
f = key;
+ // We leave both the v1 and v2 derivation to_remote keys the same as there's not any real
+ // reason to fuzz differences here, and it keeps us consistent with past behavior.
let signer = InMemorySigner::new(a, b, c, c, true, d, e, f, keys_id, keys_id);
diff --git a/lightning-tests/src/upgrade_downgrade_tests.rs b/lightning-tests/src/upgrade_downgrade_tests.rs
index 9e60ae847d..dc95054a7f 100644
--- a/lightning-tests/src/upgrade_downgrade_tests.rs
+++ b/lightning-tests/src/upgrade_downgrade_tests.rs
@@ -228,5 +228,5 @@ fn test_125_dangling_post_update_actions() {
fn test_0_1_legacy_remote_key_derivation() {
// Test that a channel opened with a v1/legacy `remote_key` derivation will be properly spent
- // even after upgrading to 0.2 and opting into the new v2 derivation for new channels.
+ // even after upgrading and opting into the new v2 derivation for new channels.
let (node_a_ser, node_b_ser, mon_a_ser, mon_b_ser, commitment_tx, channel_id);
let node_a_blocks;
diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index 8244680934..53e013460f 100644
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -1858,5 +1858,5 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
assert!(commitment_transaction_number_obscure_factor <= (1 << 48));
let holder_pubkeys = &channel_parameters.holder_pubkeys;
- let counterparty_payment_script = chan_utils::get_counterparty_payment_script(
+ let counterparty_payment_script = chan_utils::get_countersigner_payment_script(
&channel_parameters.channel_type_features, &holder_pubkeys.payment_point
);
diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs
index e6a45527e3..ef58f76e0b 100644
--- a/lightning/src/ln/chan_utils.rs
+++ b/lightning/src/ln/chan_utils.rs
@@ -638,7 +638,7 @@ pub fn get_revokeable_redeemscript(revocation_key: &RevocationKey, contest_delay
}
-/// Returns the script for the counterparty's output on a holder's commitment transaction based on
-/// the channel type.
-pub fn get_counterparty_payment_script(
+/// Returns the script for the countersigner's (i.e. non-broadcaster's) output on a commitment
+/// transaction based on the channel type.
+pub fn get_countersigner_payment_script(
channel_type_features: &ChannelTypeFeatures, payment_key: &PublicKey,
) -> ScriptBuf {
diff --git a/lightning/src/sign/mod.rs b/lightning/src/sign/mod.rs
index cd836c7952..55174529ad 100644
--- a/lightning/src/sign/mod.rs
+++ b/lightning/src/sign/mod.rs
@@ -42,5 +42,5 @@ use crate::crypto::utils::{hkdf_extract_expand_twice, sign, sign_with_aux_rand};
use crate::ln::chan_utils;
use crate::ln::chan_utils::{
- get_counterparty_payment_script, get_revokeable_redeemscript, make_funding_redeemscript,
+ get_countersigner_payment_script, get_revokeable_redeemscript, make_funding_redeemscript,
ChannelPublicKeys, ChannelTransactionParameters, ClosingTransaction, CommitmentTransaction,
HTLCOutputInCommitment, HolderCommitmentTransaction,
@@ -142,9 +142,9 @@ pub(crate) const P2TR_KEY_PATH_WITNESS_WEIGHT: u64 = 1 /* witness items */
+ 1 /* schnorr sig len */ + 64 /* schnorr sig */;
-/// If a [`KeysManager`] is built with [`KeysManager::new`] with `v2_remote_key_derivation` set,
-/// the script which we receive funds to on-chain when our counterparty force-closes a channel is
-/// one of this many possible derivation paths.
+/// If a [`KeysManager`] is built with [`KeysManager::new`] with `v2_remote_key_derivation` set
+/// (and for all channels after they've been spliced), the script which we receive funds to on-chain
+/// when our counterparty force-closes a channel is one of this many possible derivation paths.
///
-/// Keping this limited allows for scanning the chain to find lost funds if our state is destroyed,
+/// Keeping this limited allows for scanning the chain to find lost funds if our state is destroyed,
/// while this being more than a handful provides some privacy by not constantly reusing the same
/// scripts on-chain across channels.
@@ -795,6 +795,6 @@ pub trait ChannelSigner {
/// Returns a *new* set of holder channel public keys and basepoints. They may be the same as a
/// previous value, but are also allowed to change arbitrarily. Signing methods must still
- /// support both old and new versions, but this should only be called either for new channels
- /// or new splices.
+ /// support signing for any keys which have ever been returned. This should only be called
+ /// either for new channels or new splices.
///
/// `splice_parent_funding_txid` can be used to compute a tweak to rotate the funding key in the
@@ -1296,6 +1296,6 @@ impl InMemorySigner {
let payment_point_v1 = PublicKey::from_secret_key(secp_ctx, &self.payment_key_v1);
let payment_point_v2 = PublicKey::from_secret_key(secp_ctx, &self.payment_key_v2);
- let spk_v1 = get_counterparty_payment_script(channel_type_features, &payment_point_v1);
- let spk_v2 = get_counterparty_payment_script(channel_type_features, &payment_point_v2);
+ let spk_v1 = get_countersigner_payment_script(channel_type_features, &payment_point_v1);
+ let spk_v2 = get_countersigner_payment_script(channel_type_features, &payment_point_v2);
let (remotepubkey, payment_key) = if spk_v1 == descriptor.output.script_pubkey {
@@ -1452,4 +1452,6 @@ impl ChannelSigner for InMemorySigner {
&self, splice_parent_funding_txid: Option<Txid>, secp_ctx: &Secp256k1<secp256k1::All>,
) -> ChannelPublicKeys {
+ // Because splices always break downgrades, we go ahead and always use the new derivation
+ // here as its just much better.
let use_v2_derivation =
self.v2_remote_key_derivation || splice_parent_funding_txid.is_some();
@@ -2074,8 +2076,13 @@ impl KeysManager {
/// Gets the set of possible `script_pubkey`s which can appear on chain for our
- /// non-HTLC-encumbered balance if our counterparty force-closes the channel.
+ /// non-HTLC-encumbered balance if our counterparty force-closes a channel.
///
- /// Only channels opened or spliced when using a [`KeysManager`] with the
- /// `v2_remote_key_derivation` argument to [`KeysManager::new`] will close to such scripts,
+ /// If you've lost all data except your seed, asking your peers nicely to force-close the
+ /// chanels they had with you (and hoping they don't broadcast a stale state and that there are
+ /// no pending HTLCs in the latest state) and scanning the chain for these `script_pubkey`s can
+ /// allow you to recover (some of) your funds.
+ ///
+ /// Only channels opened when using a [`KeysManager`] with the `v2_remote_key_derivation`
+ /// argument to [`KeysManager::new`] set, or any spliced channels will close to such scripts,
/// other channels will close to a randomly-generated `script_pubkey`.
pub fn possible_v2_counterparty_closed_balance_spks<C: Signing>(
@@ -2096,14 +2103,12 @@ impl KeysManager {
.private_key;
let pubkey = PublicKey::from_secret_key(secp_ctx, &key);
- res.push(get_counterparty_payment_script(&static_remote_key_features, &pubkey));
- res.push(get_counterparty_payment_script(&zero_fee_htlc_features, &pubkey));
+ res.push(get_countersigner_payment_script(&static_remote_key_features, &pubkey));
+ res.push(get_countersigner_payment_script(&zero_fee_htlc_features, &pubkey));
}
res
}
- fn derive_payment_key_v2(&self, params: &[u8; 32]) -> SecretKey {
- let mut eight_bytes = [0; 8];
- eight_bytes.copy_from_slice(¶ms[0..8]);
- let idx = u64::from_le_bytes(eight_bytes) % u64::from(STATIC_PAYMENT_KEY_COUNT);
+ fn derive_payment_key_v2(&self, key_idx: u64) -> SecretKey {
+ let idx = key_idx % u64::from(STATIC_PAYMENT_KEY_COUNT);
self.static_payment_key
.derive_priv(
@@ -2160,9 +2165,12 @@ impl KeysManager {
let prng_seed = self.get_secure_random_bytes();
+ let payment_key_v2_idx =
+ u64::from_le_bytes(commitment_seed[..8].try_into().expect("8 bytes"));
+
InMemorySigner::new(
funding_key,
revocation_base_key,
payment_key_v1,
- self.derive_payment_key_v2(&commitment_seed),
+ self.derive_payment_key_v2(payment_key_v2_idx),
self.v2_remote_key_derivation,
delayed_payment_base_key,
@@ -2205,5 +2213,5 @@ impl KeysManager {
#[cfg(test)]
if self.v2_remote_key_derivation {
- // In tests, we don't have to deal with upgrades from V1 sighers with
+ // In tests, we don't have to deal with upgrades from V1 signers with
// `v2_remote_key_derivation` set, so use this opportunity to test
// `possible_v2_counterparty_closed_balance_spks`. |
lightning/src/sign/mod.rs
Outdated
| impl InMemorySigner { | ||
| /// Creates a new [`InMemorySigner`]. | ||
| #[cfg(any(feature = "_test_utils", test))] | ||
| pub fn new<C: Signing>( |
There was a problem hiding this comment.
IIUC we can DRY this with the non-pub method below
There was a problem hiding this comment.
I don't think it buys us anything. We can call the below method but we end up just passing each argument through to the other method again which saves us no lines and no reduced code.
| // Feerates may fluctuate marginally based on signature size | ||
| assert!(htlc_tx_feerate >= prev_feerate - 1); | ||
| assert!(htlc_tx_feerate <= prev_feerate + 1); |
There was a problem hiding this comment.
This passes for me with this diff reverted, is it flaky...?
There was a problem hiding this comment.
Yes, it became flaky, IIRC it only failed on no-std or something dumb like that.
TheBlueMatt
force-pushed
the
2025-09-actually-static-remote_key
branch
from
de46c84 to
5301b33
Compare
There was a problem hiding this comment.
Diff/fixups look good to me, would be ready for squashing from my side.
CI is failing though:
error: variable does not need to be mutable
--> src/upgrade_downgrade_tests.rs:263:6
|
263 | let mut chanmon_cfgs = create_chanmon_cfgs(2);
| ----^^^^^^^^^^^^
| |
| help: remove this `mut`
|
= note: `-D unused-mut` implied by `-D warnings`
In the previous commit we (finally) allowed users to opt into a static `remote_key` derivation scheme, enabling them to scan the chain for funds on counterparty commitment transactions without any state at all. This is only possible, however, of course, if they have the full list of scripts to scan the chain for, which we expose here.
`get_counterparty_payment_script` fetches the countersigner's (i.e. non-broadcaster) payment script, but that could be ours or or counterparty's. Thus, it should read `get_countersigner_payment_script`, which we fix here.
TheBlueMatt
force-pushed
the
2025-09-actually-static-remote_key
branch
from
5301b33 to
9dbec80
Compare
|
Went ahead and squashed (and fixed the spurious mut). The diff since @valentinewallace looked at it yesterday (according to github, in addition to moving the docs on the all-keys-fetching method to the right commit) is: $ git diff-tree -U2 de46c84 9dbec804b
diff --git a/lightning-tests/src/upgrade_downgrade_tests.rs b/lightning-tests/src/upgrade_downgrade_tests.rs
index dc95054a7f..0b3f5dff42 100644
--- a/lightning-tests/src/upgrade_downgrade_tests.rs
+++ b/lightning-tests/src/upgrade_downgrade_tests.rs
@@ -261,11 +261,5 @@ fn test_0_1_legacy_remote_key_derivation() {
// Create a dummy node to reload over with the 0.1 state
-
- let mut chanmon_cfgs = create_chanmon_cfgs(2);
-
- // Our TestChannelSigner will fail as we're jumping ahead, so disable its state-based checks
- chanmon_cfgs[0].keys_manager.disable_all_state_policy_checks = true;
- chanmon_cfgs[1].keys_manager.disable_all_state_policy_checks = true;
-
+ let chanmon_cfgs = create_chanmon_cfgs(2);
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
let (persister_a, persister_b, chain_mon_a, chain_mon_b); |
Needs an upgrade test, but otherwise should be good.