Creating fresh destination scripts for every channel in KeysManager #1312

vss96 · 2022-02-16T08:01:06Z

This fixes #1139

TheBlueMatt

This is great!....except misses backwards compat in a few places.

lightning/src/chain/keysinterface.rs

codecov-commenter · 2022-02-19T14:01:49Z

Codecov Report

Base: 90.70% // Head: 90.70% // No change to project coverage 👍

Coverage data is based on head (fb6e018) compared to base (fb6e018).
Patch has no changes to coverable lines.

❗ Current head fb6e018 differs from pull request most recent head 75e7377. Consider uploading reports for the commit 75e7377 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1312   +/-   ##
=======================================
  Coverage   90.70%   90.70%           
=======================================
  Files          91       91           
  Lines       48392    48392           
  Branches    48392    48392           
=======================================
  Hits        43893    43893           
  Misses       4499     4499

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

lightning/src/chain/keysinterface.rs

vss96 · 2022-03-08T20:05:28Z

@TheBlueMatt do you know why the payment script we create from the pubkey in spendable outputs is not matching the output script? 🤔
The tests are basically failing because of that.

TheBlueMatt · 2022-03-09T01:21:56Z

Just using a derivation_idx of 3 in the spender doesn't solve it, you have to actually derive using the same pattern in both places.

vss96 · 2022-03-09T04:34:11Z

@TheBlueMatt Does this mean that instead of driving private keys using the derivation_idx, I'm just creating a fresh witness script directly? Right now we derive the private key -> pubkey -> Witness, Payment script.

TheBlueMatt · 2022-03-09T19:51:46Z

The new derivation you've added here isn't simply a derivation index of 3, which is what you've left in the spending side, both the spending side (the SpendableOutputDescriptor::StaticOutput handling) and the creation side (get_destination_script) need to match exactly.

vss96 · 2022-03-15T03:57:57Z

@TheBlueMatt So I guess the derivation_idx should be the first 8 bytes of the channel_keys_id if the channel_keys_id is present and the output_script matches the new style destination_script. If none of the conditions match just assign the idx to 2. Does that make sense?

vss96 · 2022-03-15T04:20:45Z

Addon question: There is secp engine which is passed to the spenable_outputs method and there is one available as part of the KeysManager. The newly derived destination script are using the KeysManager secp engine. Does that make a difference? Should I be passing the secp engine onto the KeysInterface method as a parameter?

TheBlueMatt · 2022-03-15T20:14:21Z

@TheBlueMatt So I guess the derivation_idx should be the first 8 bytes of the channel_keys_id if the channel_keys_id is present and the output_script matches the new style destination_script. If none of the conditions match just assign the idx to 2. Does that make sense?

Yep, exactly, the two things just have to match exactly. Now that I look closer, actually, however, I think we should do a two-level derivation for the get_destination_script call - first derive a sub-key at a new, constant, index (I think we dont use 3 yet?) then from there derive something using the channel_keys_id (and then do the same on the spending side).

Should I be passing the secp engine onto the KeysInterface method as a parameter?

Nope, the secp engine doesn't matter at all.

vss96 · 2022-03-20T06:22:42Z

@TheBlueMatt If we are deriving two different destination scripts for get_destination_script under which condition are we gonna return which script? That isn't clear :/

TheBlueMatt · 2022-03-21T23:42:01Z

@TheBlueMatt If we are deriving two different destination scripts for get_destination_script under which condition are we gonna return which script? That isn't clear :/

Hmm, I'm not sure I understand your question. My understanding of the goal here is to (a) on the creation side, we have a configuration boolean that determines whether we do "old-style" derivation, or "new-style" and (b) on the spending side, we always try new-style (if we have the channel_keys_id available) and then compare to the script that is provided to us in the spend descriptor (we always know the script_pubkey, and can re-generate the expected ones and compare, letting that decide how to spend).

vss96 · 2022-03-23T04:38:13Z

@TheBlueMatt I changed the creation side to create the new style destination script if the flag is set or set the idx to 3. For the tests that are failing, i noticed that both side use the channel_keys_id (spending / creation). Any idea why it's not matching? (the payment script and output script in spendable_outputs)

TheBlueMatt · 2022-03-23T17:59:01Z

Any idea why it's not matching?

Double check exactly what's derived where -

in get_destination_script, you derive from channel_master_key at an idx chosen from the channel keys. channel_master_key is derived from master_key at an idx of 3.
In spend_spendable_outputs you derive from master_key (not channel_master_key) at a variable index.

TheBlueMatt

Awesome work getting it all passing! I should have been a bit more clear in my previous feedback, I apologize for that, one suggested change in derivation and I think we're there.

TheBlueMatt · 2022-03-25T04:43:28Z

lightning/src/chain/keysinterface.rs

-				SpendableOutputDescriptor::StaticOutput { ref output, .. } => {
+				SpendableOutputDescriptor::StaticOutput { ref output, channel_keys_id,.. } => {
+
+					let destination_script = channel_keys_id.map_or_else(|| self.destination_script.clone(),|s| self.get_destination_script(s));


It looks like this is unused for the !channel_keys_id.is_some(), maybe just map and leave it as None, then in the if below do if Some(&output.script_pubkey) == destination_script.as_ref()

TheBlueMatt · 2022-03-25T04:47:43Z

lightning/src/chain/keysinterface.rs

+		};
+		match ExtendedPrivKey::new_master(Network::Testnet, &self.seed) {
+			Ok(master_key) => {
+				match master_key.ckd_priv(&self.secp_ctx, ChildNumber::from_hardened_idx(derivation_idx as u32).expect("key space exhausted")){


Hmm, I actually liked the two-level derivation - it seems like kinda strange to have two different sets of derivations against the master key - one with a few fixed indexes and one with a random number. I'd strongly prefer to just create a new fixed index (I think 4 is free?) and do the new-style derivations off of that. Obvious the old style derivations will need to stay where they are (fixed at 3 directly off the master).

so basically derive from the channel_master_key at either 3 or 4 (4 for new style), and then derive the byte slice from the channel_keys_id if it's new style?

Yep! That sounds great to me.

@TheBlueMatt I'm assuming that we'll have to use and derive the channel master key on the spending side as well? I tried out the changes and have some tests failing 😞

I basically have the assertion for the shutdown pubkey failing (2 tests).

TheBlueMatt · 2022-04-01T17:53:43Z

lightning/src/chain/keysinterface.rs

 					} else {
 						2
 					};
+
+					let master_derivation_idx = if self.new_style_derivation{


We shouldn't be looking at new_style_derivation at all in this method, I think - we can totally have something that had the keys picked in the current version of LDK, but which needs to be spent in a new version with this code. In that case, we have to decide which derivation to use purely based on the output.script_pub_key field, not the setting here.

TheBlueMatt · 2022-04-01T17:53:57Z

Sorry about the response delay here.

TheBlueMatt · 2022-04-22T16:11:24Z

Let me know when you want another round of review or if you're stuck getting CI to pass again after trying a few times.

vss96 · 2022-04-22T16:13:37Z

@TheBlueMatt I was afk for a couple of weeks so couldn't do this then (sorry about that).
I'm setting the master_derivation_idx based on whether the spent script matches the old script or not. Ran the tests and it seems like 2 tests are still failing (assertion of the shutdown pubkey seems to be the issue).

TheBlueMatt · 2022-05-03T17:57:09Z

lightning/src/chain/keysinterface.rs

 					let secret = {
 						// Note that when we aren't serializing the key, network doesn't matter
-						match ExtendedPrivKey::new_master(Network::Testnet, &self.seed) {
+						match self.channel_master_key.ckd_priv(&self.secp_ctx, ChildNumber::from_hardened_idx(master_derivation_idx).expect("key space exhausted")) {


Looks like you're always deriving a child key here? We should derive a child key if we're doing a new-style derivation, but if we're not we should just use the channel_master_key derivation and use that key directly.

lightning/src/chain/keysinterface.rs

TheBlueMatt · 2022-05-03T17:58:23Z

lightning/src/chain/keysinterface.rs

+					let master_derivation_idx = if output.script_pubkey == self.destination_script{
+						3
+					} else {
+						4


Maybe simplify all of this by moving the key derivation into the if's and do it all in one go - ie just use the single above if statement, and do the secret derivation in that if and store the key, instead of figuring out the indexes and then setting a variable based on it.

TheBlueMatt · 2022-05-24T21:37:24Z

Looks like this needs a rebase to address conflicts with upstream.

TheBlueMatt · 2022-09-29T23:02:59Z

Let me know when you want another round of review!

TheBlueMatt · 2022-11-30T19:29:30Z

Needs rebase again, sadly. Do you intend to push this forward?

vss96 · 2022-12-01T04:02:15Z

@TheBlueMatt Hey, been caught up with work. I'll try to work on this and wrap this up soon. Sorry about the massive delay 😓

TheBlueMatt · 2022-12-02T01:04:57Z

lightning/src/chain/keysinterface.rs

@@ -439,7 +444,7 @@ pub trait KeysInterface {
 	///
 	/// This method should return a different value each time it is called, to avoid linking
 	/// on-chain funds across channels as controlled to the same user.
-	fn get_destination_script(&self) -> Script;
+	fn get_destination_script(&self, channel_keys_id : [u8; 32]) -> Script;


If we're passing a channel_keys_id that's pretty clear indication the method belongs in Sign instead :)

tnull · 2023-01-23T18:07:07Z

@vss96 Did you close this on purpose, or are you still working on this?

vss96 · 2023-01-23T18:08:18Z

@tnull Working on it, I discarded the commit and started fresh cause the code diverged.

tnull · 2023-01-23T18:09:02Z

@tnull Working on it, I discarded the commit and started fresh cause the code diverged.

Great to hear, thank you!

vss96 force-pushed the Fresh-DestScripts branch 2 times, most recently from 5421128 to d478137 Compare February 16, 2022 08:17

TheBlueMatt reviewed Feb 16, 2022

View reviewed changes

lightning/src/chain/keysinterface.rs Outdated Show resolved Hide resolved

lightning/src/chain/keysinterface.rs Outdated Show resolved Hide resolved

lightning/src/chain/keysinterface.rs Outdated Show resolved Hide resolved

vss96 force-pushed the Fresh-DestScripts branch from d478137 to 7747490 Compare February 19, 2022 14:01

vss96 force-pushed the Fresh-DestScripts branch 3 times, most recently from 00d4141 to 7b49a35 Compare February 23, 2022 16:57

TheBlueMatt reviewed Feb 23, 2022

View reviewed changes

lightning/src/chain/keysinterface.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed Mar 7, 2022

View reviewed changes

lightning/src/chain/keysinterface.rs Outdated Show resolved Hide resolved

vss96 force-pushed the Fresh-DestScripts branch from 4d319a2 to e86fb20 Compare March 8, 2022 19:22

vss96 force-pushed the Fresh-DestScripts branch from e86fb20 to 6463ef3 Compare March 21, 2022 06:39

vss96 force-pushed the Fresh-DestScripts branch from 6463ef3 to c557fd5 Compare March 22, 2022 12:17

vss96 force-pushed the Fresh-DestScripts branch 2 times, most recently from 8b7dbc5 to 215f4f2 Compare March 24, 2022 17:14

TheBlueMatt reviewed Mar 25, 2022

View reviewed changes

vss96 force-pushed the Fresh-DestScripts branch from 215f4f2 to 8f4bf67 Compare March 29, 2022 05:19

TheBlueMatt reviewed Apr 1, 2022

View reviewed changes

vss96 force-pushed the Fresh-DestScripts branch from 8f4bf67 to 524359e Compare April 22, 2022 15:19

TheBlueMatt reviewed May 3, 2022

View reviewed changes

lightning/src/chain/keysinterface.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed May 3, 2022

View reviewed changes

vss96 force-pushed the Fresh-DestScripts branch 3 times, most recently from cac19ff to 67ee51a Compare September 26, 2022 19:46

vss96 force-pushed the Fresh-DestScripts branch 3 times, most recently from 67ee51a to 75e7377 Compare December 1, 2022 04:21

TheBlueMatt reviewed Dec 2, 2022

View reviewed changes

vss96 closed this Jan 23, 2023

vss96 force-pushed the Fresh-DestScripts branch from 75e7377 to 153b048 Compare January 23, 2023 03:38

vss96 mentioned this pull request Apr 11, 2023

Creating fresh destination scripts for every channel in KeysManager #2174

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Creating fresh destination scripts for every channel in KeysManager #1312

Creating fresh destination scripts for every channel in KeysManager #1312

vss96 commented Feb 16, 2022

TheBlueMatt left a comment

codecov-commenter commented Feb 19, 2022 •

edited

Loading

vss96 commented Mar 8, 2022

TheBlueMatt commented Mar 9, 2022

vss96 commented Mar 9, 2022

TheBlueMatt commented Mar 9, 2022

vss96 commented Mar 15, 2022 •

edited

Loading

vss96 commented Mar 15, 2022

TheBlueMatt commented Mar 15, 2022

vss96 commented Mar 20, 2022

TheBlueMatt commented Mar 21, 2022

vss96 commented Mar 23, 2022

TheBlueMatt commented Mar 23, 2022

TheBlueMatt left a comment

TheBlueMatt Mar 25, 2022

TheBlueMatt Mar 25, 2022

vss96 Mar 25, 2022

TheBlueMatt Mar 26, 2022

vss96 Mar 29, 2022

vss96 Mar 29, 2022

TheBlueMatt Apr 1, 2022

TheBlueMatt commented Apr 1, 2022

TheBlueMatt commented Apr 22, 2022

vss96 commented Apr 22, 2022

TheBlueMatt May 3, 2022

TheBlueMatt May 3, 2022

TheBlueMatt commented May 24, 2022

TheBlueMatt commented Sep 29, 2022

TheBlueMatt commented Nov 30, 2022

vss96 commented Dec 1, 2022

TheBlueMatt Dec 2, 2022

tnull commented Jan 23, 2023

vss96 commented Jan 23, 2023

tnull commented Jan 23, 2023

Creating fresh destination scripts for every channel in KeysManager #1312

Creating fresh destination scripts for every channel in KeysManager #1312

Conversation

vss96 commented Feb 16, 2022

TheBlueMatt left a comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 19, 2022 • edited Loading

Codecov Report

vss96 commented Mar 8, 2022

TheBlueMatt commented Mar 9, 2022

vss96 commented Mar 9, 2022

TheBlueMatt commented Mar 9, 2022

vss96 commented Mar 15, 2022 • edited Loading

vss96 commented Mar 15, 2022

TheBlueMatt commented Mar 15, 2022

vss96 commented Mar 20, 2022

TheBlueMatt commented Mar 21, 2022

vss96 commented Mar 23, 2022

TheBlueMatt commented Mar 23, 2022

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Apr 1, 2022

TheBlueMatt commented Apr 22, 2022

vss96 commented Apr 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented May 24, 2022

TheBlueMatt commented Sep 29, 2022

TheBlueMatt commented Nov 30, 2022

vss96 commented Dec 1, 2022

Choose a reason for hiding this comment

tnull commented Jan 23, 2023

vss96 commented Jan 23, 2023

tnull commented Jan 23, 2023

codecov-commenter commented Feb 19, 2022 •

edited

Loading

vss96 commented Mar 15, 2022 •

edited

Loading