Re-derive signers instead of persisting them by wpaulino · Pull Request #1867 · lightningdevkit/rust-lightning

wpaulino · 2022-11-21T21:37:34Z

codecov-commenter · 2022-11-22T01:22:15Z

Codecov Report

Base: 90.69% // Head: 90.54% // Decreases project coverage by -0.14%⚠️

Coverage data is based on head (444fce7) compared to base (52edb35).
Patch coverage: 89.55% of modified lines in pull request are covered.

Additional details and impacted files

@@ Coverage Diff @@## main #1867 +/- ##
==========================================
- Coverage 90.69% 90.54% -0.15% 
==========================================
Files 91 91 Lines 48408 48401 -7 Branches 48408 48401 -7 ==========================================
- Hits 43902 43825 -77 - Misses 4506 4576 +70

Impacted Files	Coverage Δ
lightning/src/chain/package.rs	`92.84% <ø> (ø)`
lightning/src/util/byte_utils.rs	`100.00% <ø> (ø)`
lightning/src/util/test_utils.rs	`73.50% <71.42%> (-3.56%)`	⬇️
lightning/src/ln/channelmanager.rs	`86.30% <80.00%> (+0.01%)`	⬆️
lightning/src/ln/channel.rs	`88.70% <80.43%> (-0.14%)`	⬇️
lightning/src/chain/channelmonitor.rs	`90.88% <92.85%> (+0.14%)`	⬆️
lightning/src/chain/keysinterface.rs	`83.14% <100.00%> (-7.82%)`	⬇️
lightning/src/chain/onchaintx.rs	`95.13% <100.00%> (-0.17%)`	⬇️
lightning/src/ln/chan_utils.rs	`93.62% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.59% <100.00%> (ø)`
... and 14 more

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.

wpaulino · 2022-11-22T19:49:15Z

Pushed a new update that allows downgrading and replaces get_channel_signer with derive_channel_signer.

TheBlueMatt · 2022-11-22T20:06:03Z

Hmmmmm, now that I think about it we really need to pass the user_channel_id through to the new-signer function. We could do that by passing an enum that either has a keys_id or a user_id to the method, but given the two methods are likely to do very different things (read state vs persist new state) I'm very unconvinced that they need to be merged.

arik-so · 2022-11-22T20:26:39Z

Hm, yeah, I can see how that can be helpful, though imo that's only necessary prior to a channel_keys_id existing to identify the channel. Perhaps the derive method should take an enum of either a user_channel_id or a channel_keys_id?

TheBlueMatt · 2022-11-22T20:51:32Z

Right, if we want a single derive method we'd probably want to do that, but that also makes me thinking we dont want to merge the methods.

arik-so · 2022-11-22T21:02:26Z

I disagree. I feel very strongly wr shouldn't have both get and derive. However, I think I have a better idea. Stay tuned lol

TheBlueMatt · 2022-11-22T21:15:33Z

Why not? They do two fundamentally different things - one loads an instance from disk, one constructs a new instance (and presumably writes it to disk).

arik-so · 2022-11-22T22:41:57Z

Because the entire point of this exercise is a) to obviate this distinction and b) to start phasing out disk I/O altogether.

arik-so · 2022-11-22T23:29:19Z


 let holder_selected_contest_delay = config.channel_handshake_config.our_to_self_delay;
- let holder_signer = keys_provider.get_channel_signer(false, channel_value_satoshis);
+ let channel_keys_id = keys_provider.generate_channel_keys_id(user_id);


completely unrelated to this PR, but that variable really shouldn't be called user_id

obviously, user_provided_channel_id is a bit verbose, but at least it's accurate

TheBlueMatt · 2022-11-23T04:11:35Z

Because the entire point of this exercise is a) to obviate this distinction and b) to start phasing out disk I/O altogether.

No? We cannot remove the I/O for a signer - an enforcing signer absolutely must do IO for every action. What doesn't make sense is the I/O being done "for" a user - they have to do it inline during the signing operations themselves.

arik-so · 2022-11-23T04:22:00Z

Sorry, I meant I/O done by us.

TheBlueMatt · 2022-11-23T04:34:10Z

Right, I don't see how that's relevant to whether we combine the new-signer and the derive-preexisting-signer methods?

TheBlueMatt

Okay, I think we're basically on the same page, one note about further code we can remove but otherwise looks good.

wpaulino · 2022-12-01T23:18:03Z

Had to rebase on latest to address a conflict.

TheBlueMatt · 2022-12-02T19:03:49Z

Needs #1867 (comment) addressed.

`get_channel_signer` previously had two different responsibilites: generating unique `channel_keys_id` and using said ID to derive channel keys. We decide to split it into two methods `generate_channel_keys_id` and `derive_channel_signer`, such that we can use the latter to fulfill our goal of re-deriving signers instead of persisting them. There's no point in storing data that can be easily re-derived.

Now that ready_channel is also called on startup upon deserializing channels, we opt to rename it to a more indicative name. We also derive `PartialEq` on ChannelTransactionParameters to allow implementations to determine whether `provide_channel_parameters` calls are idempotent after the channel parameters have already been provided.

To do so, we introduce a new serialization version that doesn't store a channel's signer, and instead stores its signer's `channel_keys_id`. This is a unique identifier that can be provided to our `KeysInterface` to re-derive all private key material for said channel. We choose to not upgrade the minimum compatible serialization version until a later time, which will also remove any signer serialization logic on implementations of `KeysInterface` and `Sign`.

Similar to the previous commit, we introduce a new serialization version that doesn't store a monitor's signer. Since the monitor already knows of a channel's `channel_keys_id`, there's no need to store any new data to re-derive all private key material for said channel.

Since `ChannelMonitor`s will now re-derive signers rather than persisting them, we can no longer use the OnlyReadsKeysInterface concrete implementation.

Now that we opt to always re-derive channel secrets whenever required, we can drop the Clone requirement from Sign.

Now that to_be_bytes is available under our current MSRV of 1.41, we can use it instead of our own version.

TheBlueMatt · 2022-12-05T21:09:18Z

Would like an ACK from @devrandom

arik-so · 2022-12-06T00:14:41Z

 	node_secret: SecretKey,
 	inbound_payment_key: KeyMaterial,
 	counter: AtomicU64,
+	signer_state: RefCell<HashMap<u8, (bool, Arc<Mutex<EnforcementState>>)>>


I know it's internal only, but a comment here would be extremely helpful. I think a comment above counter would be helpful, too. What are we counting, right?

arik-so

It's ready.

TheBlueMatt

Will open a followup.

TheBlueMatt · 2022-12-06T18:06:00Z

-	/// they MUST NOT be allowed to change to different values once set.
+	/// counterparty_selected/holder_selected_contest_delay and funding outpoint. Since these are
+	/// static channel data, they MUST NOT be allowed to change to different values once set, as LDK
+	/// may call this method more than once.


Huh? LDK will absolutely not call this method more than once (for a given instance).

well, for a disk-backed signer that returns a shared reference to derive_* (which is how VLS works internally), it effectively does that. the instances are indexed by the channel keys ID.

or another way to look at it - an enforcing signer must return a shared reference so that it can keep track of a unique signer state

Right, my point is that for a given instance of the trait we won't call it more than once. From the perspective of something that exists as multiple instances of the trait its different. Anyway, we can continue this discussion on #1903

wpaulino · 2022-11-21T21:37:34Z

Fixes#1209.

codecov-commenter · 2022-11-22T01:22:15Z

Codecov Report

Base: 90.69% // Head: 90.54% // Decreases project coverage by -0.14%⚠️

Coverage data is based on head (444fce7) compared to base (52edb35).
Patch coverage: 89.55% of modified lines in pull request are covered.

Additional details and impacted files

@@ Coverage Diff @@## main #1867 +/- ##
==========================================
- Coverage 90.69% 90.54% -0.15% 
==========================================
Files 91 91 Lines 48408 48401 -7 Branches 48408 48401 -7 ==========================================
- Hits 43902 43825 -77 - Misses 4506 4576 +70

Impacted Files	Coverage Δ
lightning/src/chain/package.rs	`92.84% <ø> (ø)`
lightning/src/util/byte_utils.rs	`100.00% <ø> (ø)`
lightning/src/util/test_utils.rs	`73.50% <71.42%> (-3.56%)`	⬇️
lightning/src/ln/channelmanager.rs	`86.30% <80.00%> (+0.01%)`	⬆️
lightning/src/ln/channel.rs	`88.70% <80.43%> (-0.14%)`	⬇️
lightning/src/chain/channelmonitor.rs	`90.88% <92.85%> (+0.14%)`	⬆️
lightning/src/chain/keysinterface.rs	`83.14% <100.00%> (-7.82%)`	⬇️
lightning/src/chain/onchaintx.rs	`95.13% <100.00%> (-0.17%)`	⬇️
lightning/src/ln/chan_utils.rs	`93.62% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.59% <100.00%> (ø)`
... and 14 more

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.

wpaulino · 2022-11-22T19:49:15Z

Pushed a new update that allows downgrading and replaces get_channel_signer with derive_channel_signer.

TheBlueMatt · 2022-11-22T20:06:03Z

Hmmmmm, now that I think about it we really need to pass the user_channel_id through to the new-signer function. We could do that by passing an enum that either has a keys_id or a user_id to the method, but given the two methods are likely to do very different things (read state vs persist new state) I'm very unconvinced that they need to be merged.

arik-so · 2022-11-22T20:26:39Z

Hm, yeah, I can see how that can be helpful, though imo that's only necessary prior to a channel_keys_id existing to identify the channel. Perhaps the derive method should take an enum of either a user_channel_id or a channel_keys_id?

TheBlueMatt · 2022-11-22T20:51:32Z

Right, if we want a single derive method we'd probably want to do that, but that also makes me thinking we dont want to merge the methods.

arik-so · 2022-11-22T21:02:26Z

I disagree. I feel very strongly wr shouldn't have both get and derive. However, I think I have a better idea. Stay tuned lol

TheBlueMatt · 2022-11-22T21:15:33Z

Why not? They do two fundamentally different things - one loads an instance from disk, one constructs a new instance (and presumably writes it to disk).

arik-so · 2022-11-22T22:41:57Z

Because the entire point of this exercise is a) to obviate this distinction and b) to start phasing out disk I/O altogether.

arik-so · 2022-11-22T23:29:19Z


 let holder_selected_contest_delay = config.channel_handshake_config.our_to_self_delay;
- let holder_signer = keys_provider.get_channel_signer(false, channel_value_satoshis);
+ let channel_keys_id = keys_provider.generate_channel_keys_id(user_id);


completely unrelated to this PR, but that variable really shouldn't be called user_id

obviously, user_provided_channel_id is a bit verbose, but at least it's accurate

TheBlueMatt · 2022-11-23T04:11:35Z

Because the entire point of this exercise is a) to obviate this distinction and b) to start phasing out disk I/O altogether.

No? We cannot remove the I/O for a signer - an enforcing signer absolutely must do IO for every action. What doesn't make sense is the I/O being done "for" a user - they have to do it inline during the signing operations themselves.

arik-so · 2022-11-23T04:22:00Z

Sorry, I meant I/O done by us.

TheBlueMatt · 2022-11-23T04:34:10Z

Right, I don't see how that's relevant to whether we combine the new-signer and the derive-preexisting-signer methods?

TheBlueMatt

Okay, I think we're basically on the same page, one note about further code we can remove but otherwise looks good.

wpaulino · 2022-12-01T23:18:03Z

Had to rebase on latest to address a conflict.

TheBlueMatt · 2022-12-02T19:03:49Z

Needs #1867 (comment) addressed.

`get_channel_signer` previously had two different responsibilites: generating unique `channel_keys_id` and using said ID to derive channel keys. We decide to split it into two methods `generate_channel_keys_id` and `derive_channel_signer`, such that we can use the latter to fulfill our goal of re-deriving signers instead of persisting them. There's no point in storing data that can be easily re-derived.

Now that ready_channel is also called on startup upon deserializing channels, we opt to rename it to a more indicative name. We also derive `PartialEq` on ChannelTransactionParameters to allow implementations to determine whether `provide_channel_parameters` calls are idempotent after the channel parameters have already been provided.

To do so, we introduce a new serialization version that doesn't store a channel's signer, and instead stores its signer's `channel_keys_id`. This is a unique identifier that can be provided to our `KeysInterface` to re-derive all private key material for said channel. We choose to not upgrade the minimum compatible serialization version until a later time, which will also remove any signer serialization logic on implementations of `KeysInterface` and `Sign`.

Similar to the previous commit, we introduce a new serialization version that doesn't store a monitor's signer. Since the monitor already knows of a channel's `channel_keys_id`, there's no need to store any new data to re-derive all private key material for said channel.

Since `ChannelMonitor`s will now re-derive signers rather than persisting them, we can no longer use the OnlyReadsKeysInterface concrete implementation.

Now that we opt to always re-derive channel secrets whenever required, we can drop the Clone requirement from Sign.

Now that to_be_bytes is available under our current MSRV of 1.41, we can use it instead of our own version.

TheBlueMatt · 2022-12-05T21:09:18Z

Would like an ACK from @devrandom

arik-so · 2022-12-06T00:14:41Z

 	node_secret: SecretKey,
 	inbound_payment_key: KeyMaterial,
 	counter: AtomicU64,
+	signer_state: RefCell<HashMap<u8, (bool, Arc<Mutex<EnforcementState>>)>>


I know it's internal only, but a comment here would be extremely helpful. I think a comment above counter would be helpful, too. What are we counting, right?

arik-so

It's ready.

TheBlueMatt

Will open a followup.

TheBlueMatt · 2022-12-06T18:06:00Z

-	/// they MUST NOT be allowed to change to different values once set.
+	/// counterparty_selected/holder_selected_contest_delay and funding outpoint. Since these are
+	/// static channel data, they MUST NOT be allowed to change to different values once set, as LDK
+	/// may call this method more than once.


Huh? LDK will absolutely not call this method more than once (for a given instance).

well, for a disk-backed signer that returns a shared reference to derive_* (which is how VLS works internally), it effectively does that. the instances are indexed by the channel keys ID.

or another way to look at it - an enforcing signer must return a shared reference so that it can keep track of a unique signer state

Right, my point is that for a given instance of the trait we won't call it more than once. From the perspective of something that exists as multiple instances of the trait its different. Anyway, we can continue this discussion on #1903

wpaulino · 2022-11-21T21:37:34Z

Fixes#1209.

codecov-commenter · 2022-11-22T01:22:15Z

Codecov Report

Base: 90.69% // Head: 90.54% // Decreases project coverage by -0.14%⚠️

Coverage data is based on head (444fce7) compared to base (52edb35).
Patch coverage: 89.55% of modified lines in pull request are covered.

Additional details and impacted files

@@ Coverage Diff @@## main #1867 +/- ##
==========================================
- Coverage 90.69% 90.54% -0.15% 
==========================================
Files 91 91 Lines 48408 48401 -7 Branches 48408 48401 -7 ==========================================
- Hits 43902 43825 -77 - Misses 4506 4576 +70

Impacted Files	Coverage Δ
lightning/src/chain/package.rs	`92.84% <ø> (ø)`
lightning/src/util/byte_utils.rs	`100.00% <ø> (ø)`
lightning/src/util/test_utils.rs	`73.50% <71.42%> (-3.56%)`	⬇️
lightning/src/ln/channelmanager.rs	`86.30% <80.00%> (+0.01%)`	⬆️
lightning/src/ln/channel.rs	`88.70% <80.43%> (-0.14%)`	⬇️
lightning/src/chain/channelmonitor.rs	`90.88% <92.85%> (+0.14%)`	⬆️
lightning/src/chain/keysinterface.rs	`83.14% <100.00%> (-7.82%)`	⬇️
lightning/src/chain/onchaintx.rs	`95.13% <100.00%> (-0.17%)`	⬇️
lightning/src/ln/chan_utils.rs	`93.62% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.59% <100.00%> (ø)`
... and 14 more

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.

wpaulino · 2022-11-22T19:49:15Z

Pushed a new update that allows downgrading and replaces get_channel_signer with derive_channel_signer.

TheBlueMatt · 2022-11-22T20:06:03Z

Hmmmmm, now that I think about it we really need to pass the user_channel_id through to the new-signer function. We could do that by passing an enum that either has a keys_id or a user_id to the method, but given the two methods are likely to do very different things (read state vs persist new state) I'm very unconvinced that they need to be merged.

arik-so · 2022-11-22T20:26:39Z

Hm, yeah, I can see how that can be helpful, though imo that's only necessary prior to a channel_keys_id existing to identify the channel. Perhaps the derive method should take an enum of either a user_channel_id or a channel_keys_id?

TheBlueMatt · 2022-11-22T20:51:32Z

Right, if we want a single derive method we'd probably want to do that, but that also makes me thinking we dont want to merge the methods.

arik-so · 2022-11-22T21:02:26Z

I disagree. I feel very strongly wr shouldn't have both get and derive. However, I think I have a better idea. Stay tuned lol

TheBlueMatt · 2022-11-22T21:15:33Z

Why not? They do two fundamentally different things - one loads an instance from disk, one constructs a new instance (and presumably writes it to disk).

arik-so · 2022-11-22T22:41:57Z

Because the entire point of this exercise is a) to obviate this distinction and b) to start phasing out disk I/O altogether.

arik-so · 2022-11-22T23:29:19Z


 let holder_selected_contest_delay = config.channel_handshake_config.our_to_self_delay;
- let holder_signer = keys_provider.get_channel_signer(false, channel_value_satoshis);
+ let channel_keys_id = keys_provider.generate_channel_keys_id(user_id);


completely unrelated to this PR, but that variable really shouldn't be called user_id

obviously, user_provided_channel_id is a bit verbose, but at least it's accurate

TheBlueMatt · 2022-11-23T04:11:35Z

Because the entire point of this exercise is a) to obviate this distinction and b) to start phasing out disk I/O altogether.

No? We cannot remove the I/O for a signer - an enforcing signer absolutely must do IO for every action. What doesn't make sense is the I/O being done "for" a user - they have to do it inline during the signing operations themselves.

arik-so · 2022-11-23T04:22:00Z

Sorry, I meant I/O done by us.

TheBlueMatt · 2022-11-23T04:34:10Z

Right, I don't see how that's relevant to whether we combine the new-signer and the derive-preexisting-signer methods?

TheBlueMatt

Okay, I think we're basically on the same page, one note about further code we can remove but otherwise looks good.

wpaulino · 2022-12-01T23:18:03Z

Had to rebase on latest to address a conflict.

TheBlueMatt · 2022-12-02T19:03:49Z

Needs #1867 (comment) addressed.

`get_channel_signer` previously had two different responsibilites: generating unique `channel_keys_id` and using said ID to derive channel keys. We decide to split it into two methods `generate_channel_keys_id` and `derive_channel_signer`, such that we can use the latter to fulfill our goal of re-deriving signers instead of persisting them. There's no point in storing data that can be easily re-derived.

Now that ready_channel is also called on startup upon deserializing channels, we opt to rename it to a more indicative name. We also derive `PartialEq` on ChannelTransactionParameters to allow implementations to determine whether `provide_channel_parameters` calls are idempotent after the channel parameters have already been provided.

To do so, we introduce a new serialization version that doesn't store a channel's signer, and instead stores its signer's `channel_keys_id`. This is a unique identifier that can be provided to our `KeysInterface` to re-derive all private key material for said channel. We choose to not upgrade the minimum compatible serialization version until a later time, which will also remove any signer serialization logic on implementations of `KeysInterface` and `Sign`.

Similar to the previous commit, we introduce a new serialization version that doesn't store a monitor's signer. Since the monitor already knows of a channel's `channel_keys_id`, there's no need to store any new data to re-derive all private key material for said channel.

Since `ChannelMonitor`s will now re-derive signers rather than persisting them, we can no longer use the OnlyReadsKeysInterface concrete implementation.

Now that we opt to always re-derive channel secrets whenever required, we can drop the Clone requirement from Sign.

Now that to_be_bytes is available under our current MSRV of 1.41, we can use it instead of our own version.

TheBlueMatt · 2022-12-05T21:09:18Z

Would like an ACK from @devrandom

arik-so · 2022-12-06T00:14:41Z

 	node_secret: SecretKey,
 	inbound_payment_key: KeyMaterial,
 	counter: AtomicU64,
+	signer_state: RefCell<HashMap<u8, (bool, Arc<Mutex<EnforcementState>>)>>


I know it's internal only, but a comment here would be extremely helpful. I think a comment above counter would be helpful, too. What are we counting, right?

arik-so

It's ready.

TheBlueMatt

Will open a followup.

TheBlueMatt · 2022-12-06T18:06:00Z

-	/// they MUST NOT be allowed to change to different values once set.
+	/// counterparty_selected/holder_selected_contest_delay and funding outpoint. Since these are
+	/// static channel data, they MUST NOT be allowed to change to different values once set, as LDK
+	/// may call this method more than once.


Huh? LDK will absolutely not call this method more than once (for a given instance).

well, for a disk-backed signer that returns a shared reference to derive_* (which is how VLS works internally), it effectively does that. the instances are indexed by the channel keys ID.

or another way to look at it - an enforcing signer must return a shared reference so that it can keep track of a unique signer state

Right, my point is that for a given instance of the trait we won't call it more than once. From the perspective of something that exists as multiple instances of the trait its different. Anyway, we can continue this discussion on #1903

wpaulino · 2022-11-21T21:37:34Z

Fixes#1209.

codecov-commenter · 2022-11-22T01:22:15Z

Codecov Report

Base: 90.69% // Head: 90.54% // Decreases project coverage by -0.14%⚠️

Coverage data is based on head (444fce7) compared to base (52edb35).
Patch coverage: 89.55% of modified lines in pull request are covered.

Additional details and impacted files

@@ Coverage Diff @@## main #1867 +/- ##
==========================================
- Coverage 90.69% 90.54% -0.15% 
==========================================
Files 91 91 Lines 48408 48401 -7 Branches 48408 48401 -7 ==========================================
- Hits 43902 43825 -77 - Misses 4506 4576 +70

Impacted Files	Coverage Δ
lightning/src/chain/package.rs	`92.84% <ø> (ø)`
lightning/src/util/byte_utils.rs	`100.00% <ø> (ø)`
lightning/src/util/test_utils.rs	`73.50% <71.42%> (-3.56%)`	⬇️
lightning/src/ln/channelmanager.rs	`86.30% <80.00%> (+0.01%)`	⬆️
lightning/src/ln/channel.rs	`88.70% <80.43%> (-0.14%)`	⬇️
lightning/src/chain/channelmonitor.rs	`90.88% <92.85%> (+0.14%)`	⬆️
lightning/src/chain/keysinterface.rs	`83.14% <100.00%> (-7.82%)`	⬇️
lightning/src/chain/onchaintx.rs	`95.13% <100.00%> (-0.17%)`	⬇️
lightning/src/ln/chan_utils.rs	`93.62% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.59% <100.00%> (ø)`
... and 14 more

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.

wpaulino · 2022-11-22T19:49:15Z

Pushed a new update that allows downgrading and replaces get_channel_signer with derive_channel_signer.

TheBlueMatt · 2022-11-22T20:06:03Z

Hmmmmm, now that I think about it we really need to pass the user_channel_id through to the new-signer function. We could do that by passing an enum that either has a keys_id or a user_id to the method, but given the two methods are likely to do very different things (read state vs persist new state) I'm very unconvinced that they need to be merged.

arik-so · 2022-11-22T20:26:39Z

Hm, yeah, I can see how that can be helpful, though imo that's only necessary prior to a channel_keys_id existing to identify the channel. Perhaps the derive method should take an enum of either a user_channel_id or a channel_keys_id?

TheBlueMatt · 2022-11-22T20:51:32Z

Right, if we want a single derive method we'd probably want to do that, but that also makes me thinking we dont want to merge the methods.

arik-so · 2022-11-22T21:02:26Z

I disagree. I feel very strongly wr shouldn't have both get and derive. However, I think I have a better idea. Stay tuned lol

TheBlueMatt · 2022-11-22T21:15:33Z

Why not? They do two fundamentally different things - one loads an instance from disk, one constructs a new instance (and presumably writes it to disk).

arik-so · 2022-11-22T22:41:57Z

Because the entire point of this exercise is a) to obviate this distinction and b) to start phasing out disk I/O altogether.

arik-so · 2022-11-22T23:29:19Z


 let holder_selected_contest_delay = config.channel_handshake_config.our_to_self_delay;
- let holder_signer = keys_provider.get_channel_signer(false, channel_value_satoshis);
+ let channel_keys_id = keys_provider.generate_channel_keys_id(user_id);


completely unrelated to this PR, but that variable really shouldn't be called user_id

obviously, user_provided_channel_id is a bit verbose, but at least it's accurate

TheBlueMatt · 2022-11-23T04:11:35Z

Because the entire point of this exercise is a) to obviate this distinction and b) to start phasing out disk I/O altogether.

No? We cannot remove the I/O for a signer - an enforcing signer absolutely must do IO for every action. What doesn't make sense is the I/O being done "for" a user - they have to do it inline during the signing operations themselves.

arik-so · 2022-11-23T04:22:00Z

Sorry, I meant I/O done by us.

TheBlueMatt · 2022-11-23T04:34:10Z

Right, I don't see how that's relevant to whether we combine the new-signer and the derive-preexisting-signer methods?

TheBlueMatt

Okay, I think we're basically on the same page, one note about further code we can remove but otherwise looks good.

wpaulino · 2022-12-01T23:18:03Z

Had to rebase on latest to address a conflict.

TheBlueMatt · 2022-12-02T19:03:49Z

Needs #1867 (comment) addressed.

`get_channel_signer` previously had two different responsibilites: generating unique `channel_keys_id` and using said ID to derive channel keys. We decide to split it into two methods `generate_channel_keys_id` and `derive_channel_signer`, such that we can use the latter to fulfill our goal of re-deriving signers instead of persisting them. There's no point in storing data that can be easily re-derived.

Now that ready_channel is also called on startup upon deserializing channels, we opt to rename it to a more indicative name. We also derive `PartialEq` on ChannelTransactionParameters to allow implementations to determine whether `provide_channel_parameters` calls are idempotent after the channel parameters have already been provided.

To do so, we introduce a new serialization version that doesn't store a channel's signer, and instead stores its signer's `channel_keys_id`. This is a unique identifier that can be provided to our `KeysInterface` to re-derive all private key material for said channel. We choose to not upgrade the minimum compatible serialization version until a later time, which will also remove any signer serialization logic on implementations of `KeysInterface` and `Sign`.

Similar to the previous commit, we introduce a new serialization version that doesn't store a monitor's signer. Since the monitor already knows of a channel's `channel_keys_id`, there's no need to store any new data to re-derive all private key material for said channel.

Since `ChannelMonitor`s will now re-derive signers rather than persisting them, we can no longer use the OnlyReadsKeysInterface concrete implementation.

Now that we opt to always re-derive channel secrets whenever required, we can drop the Clone requirement from Sign.

Now that to_be_bytes is available under our current MSRV of 1.41, we can use it instead of our own version.

TheBlueMatt · 2022-12-05T21:09:18Z

Would like an ACK from @devrandom

arik-so · 2022-12-06T00:14:41Z

 	node_secret: SecretKey,
 	inbound_payment_key: KeyMaterial,
 	counter: AtomicU64,
+	signer_state: RefCell<HashMap<u8, (bool, Arc<Mutex<EnforcementState>>)>>


I know it's internal only, but a comment here would be extremely helpful. I think a comment above counter would be helpful, too. What are we counting, right?

arik-so

It's ready.

TheBlueMatt

Will open a followup.

TheBlueMatt · 2022-12-06T18:06:00Z

-	/// they MUST NOT be allowed to change to different values once set.
+	/// counterparty_selected/holder_selected_contest_delay and funding outpoint. Since these are
+	/// static channel data, they MUST NOT be allowed to change to different values once set, as LDK
+	/// may call this method more than once.


Huh? LDK will absolutely not call this method more than once (for a given instance).

well, for a disk-backed signer that returns a shared reference to derive_* (which is how VLS works internally), it effectively does that. the instances are indexed by the channel keys ID.

or another way to look at it - an enforcing signer must return a shared reference so that it can keep track of a unique signer state

Right, my point is that for a given instance of the trait we won't call it more than once. From the perspective of something that exists as multiple instances of the trait its different. Anyway, we can continue this discussion on #1903

wpaulino · 2022-11-21T21:37:34Z

Fixes#1209.

codecov-commenter · 2022-11-22T01:22:15Z

Codecov Report

Base: 90.69% // Head: 90.54% // Decreases project coverage by -0.14%⚠️

Coverage data is based on head (444fce7) compared to base (52edb35).
Patch coverage: 89.55% of modified lines in pull request are covered.

Additional details and impacted files

@@ Coverage Diff @@## main #1867 +/- ##
==========================================
- Coverage 90.69% 90.54% -0.15% 
==========================================
Files 91 91 Lines 48408 48401 -7 Branches 48408 48401 -7 ==========================================
- Hits 43902 43825 -77 - Misses 4506 4576 +70

Impacted Files	Coverage Δ
lightning/src/chain/package.rs	`92.84% <ø> (ø)`
lightning/src/util/byte_utils.rs	`100.00% <ø> (ø)`
lightning/src/util/test_utils.rs	`73.50% <71.42%> (-3.56%)`	⬇️
lightning/src/ln/channelmanager.rs	`86.30% <80.00%> (+0.01%)`	⬆️
lightning/src/ln/channel.rs	`88.70% <80.43%> (-0.14%)`	⬇️
lightning/src/chain/channelmonitor.rs	`90.88% <92.85%> (+0.14%)`	⬆️
lightning/src/chain/keysinterface.rs	`83.14% <100.00%> (-7.82%)`	⬇️
lightning/src/chain/onchaintx.rs	`95.13% <100.00%> (-0.17%)`	⬇️
lightning/src/ln/chan_utils.rs	`93.62% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.59% <100.00%> (ø)`
... and 14 more

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.

wpaulino · 2022-11-22T19:49:15Z

Pushed a new update that allows downgrading and replaces get_channel_signer with derive_channel_signer.

TheBlueMatt · 2022-11-22T20:06:03Z

Hmmmmm, now that I think about it we really need to pass the user_channel_id through to the new-signer function. We could do that by passing an enum that either has a keys_id or a user_id to the method, but given the two methods are likely to do very different things (read state vs persist new state) I'm very unconvinced that they need to be merged.

arik-so · 2022-11-22T20:26:39Z

Hm, yeah, I can see how that can be helpful, though imo that's only necessary prior to a channel_keys_id existing to identify the channel. Perhaps the derive method should take an enum of either a user_channel_id or a channel_keys_id?

TheBlueMatt · 2022-11-22T20:51:32Z

Right, if we want a single derive method we'd probably want to do that, but that also makes me thinking we dont want to merge the methods.

arik-so · 2022-11-22T21:02:26Z

I disagree. I feel very strongly wr shouldn't have both get and derive. However, I think I have a better idea. Stay tuned lol

TheBlueMatt · 2022-11-22T21:15:33Z

Why not? They do two fundamentally different things - one loads an instance from disk, one constructs a new instance (and presumably writes it to disk).

arik-so · 2022-11-22T22:41:57Z

Because the entire point of this exercise is a) to obviate this distinction and b) to start phasing out disk I/O altogether.

arik-so · 2022-11-22T23:29:19Z


 let holder_selected_contest_delay = config.channel_handshake_config.our_to_self_delay;
- let holder_signer = keys_provider.get_channel_signer(false, channel_value_satoshis);
+ let channel_keys_id = keys_provider.generate_channel_keys_id(user_id);


completely unrelated to this PR, but that variable really shouldn't be called user_id

obviously, user_provided_channel_id is a bit verbose, but at least it's accurate

TheBlueMatt · 2022-11-23T04:11:35Z

Because the entire point of this exercise is a) to obviate this distinction and b) to start phasing out disk I/O altogether.

No? We cannot remove the I/O for a signer - an enforcing signer absolutely must do IO for every action. What doesn't make sense is the I/O being done "for" a user - they have to do it inline during the signing operations themselves.

arik-so · 2022-11-23T04:22:00Z

Sorry, I meant I/O done by us.

TheBlueMatt · 2022-11-23T04:34:10Z

Right, I don't see how that's relevant to whether we combine the new-signer and the derive-preexisting-signer methods?

TheBlueMatt

Okay, I think we're basically on the same page, one note about further code we can remove but otherwise looks good.

wpaulino · 2022-12-01T23:18:03Z

Had to rebase on latest to address a conflict.

TheBlueMatt · 2022-12-02T19:03:49Z

Needs #1867 (comment) addressed.

`get_channel_signer` previously had two different responsibilites: generating unique `channel_keys_id` and using said ID to derive channel keys. We decide to split it into two methods `generate_channel_keys_id` and `derive_channel_signer`, such that we can use the latter to fulfill our goal of re-deriving signers instead of persisting them. There's no point in storing data that can be easily re-derived.

Now that ready_channel is also called on startup upon deserializing channels, we opt to rename it to a more indicative name. We also derive `PartialEq` on ChannelTransactionParameters to allow implementations to determine whether `provide_channel_parameters` calls are idempotent after the channel parameters have already been provided.

To do so, we introduce a new serialization version that doesn't store a channel's signer, and instead stores its signer's `channel_keys_id`. This is a unique identifier that can be provided to our `KeysInterface` to re-derive all private key material for said channel. We choose to not upgrade the minimum compatible serialization version until a later time, which will also remove any signer serialization logic on implementations of `KeysInterface` and `Sign`.

Similar to the previous commit, we introduce a new serialization version that doesn't store a monitor's signer. Since the monitor already knows of a channel's `channel_keys_id`, there's no need to store any new data to re-derive all private key material for said channel.

Since `ChannelMonitor`s will now re-derive signers rather than persisting them, we can no longer use the OnlyReadsKeysInterface concrete implementation.

Now that we opt to always re-derive channel secrets whenever required, we can drop the Clone requirement from Sign.

Now that to_be_bytes is available under our current MSRV of 1.41, we can use it instead of our own version.

TheBlueMatt · 2022-12-05T21:09:18Z

Would like an ACK from @devrandom

arik-so · 2022-12-06T00:14:41Z

 	node_secret: SecretKey,
 	inbound_payment_key: KeyMaterial,
 	counter: AtomicU64,
+	signer_state: RefCell<HashMap<u8, (bool, Arc<Mutex<EnforcementState>>)>>


I know it's internal only, but a comment here would be extremely helpful. I think a comment above counter would be helpful, too. What are we counting, right?

arik-so

It's ready.

TheBlueMatt

Will open a followup.

TheBlueMatt · 2022-12-06T18:06:00Z

-	/// they MUST NOT be allowed to change to different values once set.
+	/// counterparty_selected/holder_selected_contest_delay and funding outpoint. Since these are
+	/// static channel data, they MUST NOT be allowed to change to different values once set, as LDK
+	/// may call this method more than once.


Huh? LDK will absolutely not call this method more than once (for a given instance).

well, for a disk-backed signer that returns a shared reference to derive_* (which is how VLS works internally), it effectively does that. the instances are indexed by the channel keys ID.

or another way to look at it - an enforcing signer must return a shared reference so that it can keep track of a unique signer state

Right, my point is that for a given instance of the trait we won't call it more than once. From the perspective of something that exists as multiple instances of the trait its different. Anyway, we can continue this discussion on #1903

wpaulino · 2022-11-21T21:37:34Z

Fixes#1209.

codecov-commenter · 2022-11-22T01:22:15Z

Codecov Report

Base: 90.69% // Head: 90.54% // Decreases project coverage by -0.14%⚠️

Coverage data is based on head (444fce7) compared to base (52edb35).
Patch coverage: 89.55% of modified lines in pull request are covered.

Additional details and impacted files

@@ Coverage Diff @@## main #1867 +/- ##
==========================================
- Coverage 90.69% 90.54% -0.15% 
==========================================
Files 91 91 Lines 48408 48401 -7 Branches 48408 48401 -7 ==========================================
- Hits 43902 43825 -77 - Misses 4506 4576 +70

Impacted Files	Coverage Δ
lightning/src/chain/package.rs	`92.84% <ø> (ø)`
lightning/src/util/byte_utils.rs	`100.00% <ø> (ø)`
lightning/src/util/test_utils.rs	`73.50% <71.42%> (-3.56%)`	⬇️
lightning/src/ln/channelmanager.rs	`86.30% <80.00%> (+0.01%)`	⬆️
lightning/src/ln/channel.rs	`88.70% <80.43%> (-0.14%)`	⬇️
lightning/src/chain/channelmonitor.rs	`90.88% <92.85%> (+0.14%)`	⬆️
lightning/src/chain/keysinterface.rs	`83.14% <100.00%> (-7.82%)`	⬇️
lightning/src/chain/onchaintx.rs	`95.13% <100.00%> (-0.17%)`	⬇️
lightning/src/ln/chan_utils.rs	`93.62% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.59% <100.00%> (ø)`
... and 14 more

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.

wpaulino · 2022-11-22T19:49:15Z

Pushed a new update that allows downgrading and replaces get_channel_signer with derive_channel_signer.

TheBlueMatt · 2022-11-22T20:06:03Z

Hmmmmm, now that I think about it we really need to pass the user_channel_id through to the new-signer function. We could do that by passing an enum that either has a keys_id or a user_id to the method, but given the two methods are likely to do very different things (read state vs persist new state) I'm very unconvinced that they need to be merged.

arik-so · 2022-11-22T20:26:39Z

Hm, yeah, I can see how that can be helpful, though imo that's only necessary prior to a channel_keys_id existing to identify the channel. Perhaps the derive method should take an enum of either a user_channel_id or a channel_keys_id?

TheBlueMatt · 2022-11-22T20:51:32Z

Right, if we want a single derive method we'd probably want to do that, but that also makes me thinking we dont want to merge the methods.

arik-so · 2022-11-22T21:02:26Z

I disagree. I feel very strongly wr shouldn't have both get and derive. However, I think I have a better idea. Stay tuned lol

TheBlueMatt · 2022-11-22T21:15:33Z

Why not? They do two fundamentally different things - one loads an instance from disk, one constructs a new instance (and presumably writes it to disk).

arik-so · 2022-11-22T22:41:57Z

Because the entire point of this exercise is a) to obviate this distinction and b) to start phasing out disk I/O altogether.

arik-so · 2022-11-22T23:29:19Z


 let holder_selected_contest_delay = config.channel_handshake_config.our_to_self_delay;
- let holder_signer = keys_provider.get_channel_signer(false, channel_value_satoshis);
+ let channel_keys_id = keys_provider.generate_channel_keys_id(user_id);


completely unrelated to this PR, but that variable really shouldn't be called user_id

obviously, user_provided_channel_id is a bit verbose, but at least it's accurate

TheBlueMatt · 2022-11-23T04:11:35Z

Because the entire point of this exercise is a) to obviate this distinction and b) to start phasing out disk I/O altogether.

No? We cannot remove the I/O for a signer - an enforcing signer absolutely must do IO for every action. What doesn't make sense is the I/O being done "for" a user - they have to do it inline during the signing operations themselves.

arik-so · 2022-11-23T04:22:00Z

Sorry, I meant I/O done by us.

TheBlueMatt · 2022-11-23T04:34:10Z

Right, I don't see how that's relevant to whether we combine the new-signer and the derive-preexisting-signer methods?

TheBlueMatt

Okay, I think we're basically on the same page, one note about further code we can remove but otherwise looks good.

wpaulino · 2022-12-01T23:18:03Z

Had to rebase on latest to address a conflict.

TheBlueMatt · 2022-12-02T19:03:49Z

Needs #1867 (comment) addressed.

`get_channel_signer` previously had two different responsibilites: generating unique `channel_keys_id` and using said ID to derive channel keys. We decide to split it into two methods `generate_channel_keys_id` and `derive_channel_signer`, such that we can use the latter to fulfill our goal of re-deriving signers instead of persisting them. There's no point in storing data that can be easily re-derived.

Now that ready_channel is also called on startup upon deserializing channels, we opt to rename it to a more indicative name. We also derive `PartialEq` on ChannelTransactionParameters to allow implementations to determine whether `provide_channel_parameters` calls are idempotent after the channel parameters have already been provided.

To do so, we introduce a new serialization version that doesn't store a channel's signer, and instead stores its signer's `channel_keys_id`. This is a unique identifier that can be provided to our `KeysInterface` to re-derive all private key material for said channel. We choose to not upgrade the minimum compatible serialization version until a later time, which will also remove any signer serialization logic on implementations of `KeysInterface` and `Sign`.

Similar to the previous commit, we introduce a new serialization version that doesn't store a monitor's signer. Since the monitor already knows of a channel's `channel_keys_id`, there's no need to store any new data to re-derive all private key material for said channel.

Since `ChannelMonitor`s will now re-derive signers rather than persisting them, we can no longer use the OnlyReadsKeysInterface concrete implementation.

Now that we opt to always re-derive channel secrets whenever required, we can drop the Clone requirement from Sign.

Now that to_be_bytes is available under our current MSRV of 1.41, we can use it instead of our own version.

TheBlueMatt · 2022-12-05T21:09:18Z

Would like an ACK from @devrandom

arik-so · 2022-12-06T00:14:41Z

 	node_secret: SecretKey,
 	inbound_payment_key: KeyMaterial,
 	counter: AtomicU64,
+	signer_state: RefCell<HashMap<u8, (bool, Arc<Mutex<EnforcementState>>)>>


I know it's internal only, but a comment here would be extremely helpful. I think a comment above counter would be helpful, too. What are we counting, right?

arik-so

It's ready.

TheBlueMatt

Will open a followup.

TheBlueMatt · 2022-12-06T18:06:00Z

-	/// they MUST NOT be allowed to change to different values once set.
+	/// counterparty_selected/holder_selected_contest_delay and funding outpoint. Since these are
+	/// static channel data, they MUST NOT be allowed to change to different values once set, as LDK
+	/// may call this method more than once.


Huh? LDK will absolutely not call this method more than once (for a given instance).

well, for a disk-backed signer that returns a shared reference to derive_* (which is how VLS works internally), it effectively does that. the instances are indexed by the channel keys ID.

or another way to look at it - an enforcing signer must return a shared reference so that it can keep track of a unique signer state

Right, my point is that for a given instance of the trait we won't call it more than once. From the perspective of something that exists as multiple instances of the trait its different. Anyway, we can continue this discussion on #1903

wpaulino · 2022-11-21T21:37:34Z

Fixes#1209.

codecov-commenter · 2022-11-22T01:22:15Z

Codecov Report

Base: 90.69% // Head: 90.54% // Decreases project coverage by -0.14%⚠️

Coverage data is based on head (444fce7) compared to base (52edb35).
Patch coverage: 89.55% of modified lines in pull request are covered.

Additional details and impacted files

@@ Coverage Diff @@## main #1867 +/- ##
==========================================
- Coverage 90.69% 90.54% -0.15% 
==========================================
Files 91 91 Lines 48408 48401 -7 Branches 48408 48401 -7 ==========================================
- Hits 43902 43825 -77 - Misses 4506 4576 +70

Impacted Files	Coverage Δ
lightning/src/chain/package.rs	`92.84% <ø> (ø)`
lightning/src/util/byte_utils.rs	`100.00% <ø> (ø)`
lightning/src/util/test_utils.rs	`73.50% <71.42%> (-3.56%)`	⬇️
lightning/src/ln/channelmanager.rs	`86.30% <80.00%> (+0.01%)`	⬆️
lightning/src/ln/channel.rs	`88.70% <80.43%> (-0.14%)`	⬇️
lightning/src/chain/channelmonitor.rs	`90.88% <92.85%> (+0.14%)`	⬆️
lightning/src/chain/keysinterface.rs	`83.14% <100.00%> (-7.82%)`	⬇️
lightning/src/chain/onchaintx.rs	`95.13% <100.00%> (-0.17%)`	⬇️
lightning/src/ln/chan_utils.rs	`93.62% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.59% <100.00%> (ø)`
... and 14 more

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.

wpaulino · 2022-11-22T19:49:15Z

Pushed a new update that allows downgrading and replaces get_channel_signer with derive_channel_signer.

TheBlueMatt · 2022-11-22T20:06:03Z

Hmmmmm, now that I think about it we really need to pass the user_channel_id through to the new-signer function. We could do that by passing an enum that either has a keys_id or a user_id to the method, but given the two methods are likely to do very different things (read state vs persist new state) I'm very unconvinced that they need to be merged.

arik-so · 2022-11-22T20:26:39Z

Hm, yeah, I can see how that can be helpful, though imo that's only necessary prior to a channel_keys_id existing to identify the channel. Perhaps the derive method should take an enum of either a user_channel_id or a channel_keys_id?

TheBlueMatt · 2022-11-22T20:51:32Z

Right, if we want a single derive method we'd probably want to do that, but that also makes me thinking we dont want to merge the methods.

arik-so · 2022-11-22T21:02:26Z

I disagree. I feel very strongly wr shouldn't have both get and derive. However, I think I have a better idea. Stay tuned lol

TheBlueMatt · 2022-11-22T21:15:33Z

Why not? They do two fundamentally different things - one loads an instance from disk, one constructs a new instance (and presumably writes it to disk).

arik-so · 2022-11-22T22:41:57Z

Because the entire point of this exercise is a) to obviate this distinction and b) to start phasing out disk I/O altogether.

arik-so · 2022-11-22T23:29:19Z


 let holder_selected_contest_delay = config.channel_handshake_config.our_to_self_delay;
- let holder_signer = keys_provider.get_channel_signer(false, channel_value_satoshis);
+ let channel_keys_id = keys_provider.generate_channel_keys_id(user_id);


completely unrelated to this PR, but that variable really shouldn't be called user_id

obviously, user_provided_channel_id is a bit verbose, but at least it's accurate

TheBlueMatt · 2022-11-23T04:11:35Z

Because the entire point of this exercise is a) to obviate this distinction and b) to start phasing out disk I/O altogether.

No? We cannot remove the I/O for a signer - an enforcing signer absolutely must do IO for every action. What doesn't make sense is the I/O being done "for" a user - they have to do it inline during the signing operations themselves.

arik-so · 2022-11-23T04:22:00Z

Sorry, I meant I/O done by us.

TheBlueMatt · 2022-11-23T04:34:10Z

Right, I don't see how that's relevant to whether we combine the new-signer and the derive-preexisting-signer methods?

TheBlueMatt

Okay, I think we're basically on the same page, one note about further code we can remove but otherwise looks good.

wpaulino · 2022-12-01T23:18:03Z

Had to rebase on latest to address a conflict.

TheBlueMatt · 2022-12-02T19:03:49Z

Needs #1867 (comment) addressed.

`get_channel_signer` previously had two different responsibilites: generating unique `channel_keys_id` and using said ID to derive channel keys. We decide to split it into two methods `generate_channel_keys_id` and `derive_channel_signer`, such that we can use the latter to fulfill our goal of re-deriving signers instead of persisting them. There's no point in storing data that can be easily re-derived.

Now that ready_channel is also called on startup upon deserializing channels, we opt to rename it to a more indicative name. We also derive `PartialEq` on ChannelTransactionParameters to allow implementations to determine whether `provide_channel_parameters` calls are idempotent after the channel parameters have already been provided.

To do so, we introduce a new serialization version that doesn't store a channel's signer, and instead stores its signer's `channel_keys_id`. This is a unique identifier that can be provided to our `KeysInterface` to re-derive all private key material for said channel. We choose to not upgrade the minimum compatible serialization version until a later time, which will also remove any signer serialization logic on implementations of `KeysInterface` and `Sign`.

Similar to the previous commit, we introduce a new serialization version that doesn't store a monitor's signer. Since the monitor already knows of a channel's `channel_keys_id`, there's no need to store any new data to re-derive all private key material for said channel.

Since `ChannelMonitor`s will now re-derive signers rather than persisting them, we can no longer use the OnlyReadsKeysInterface concrete implementation.

Now that we opt to always re-derive channel secrets whenever required, we can drop the Clone requirement from Sign.

Now that to_be_bytes is available under our current MSRV of 1.41, we can use it instead of our own version.

TheBlueMatt · 2022-12-05T21:09:18Z

Would like an ACK from @devrandom

arik-so · 2022-12-06T00:14:41Z

 	node_secret: SecretKey,
 	inbound_payment_key: KeyMaterial,
 	counter: AtomicU64,
+	signer_state: RefCell<HashMap<u8, (bool, Arc<Mutex<EnforcementState>>)>>


I know it's internal only, but a comment here would be extremely helpful. I think a comment above counter would be helpful, too. What are we counting, right?

arik-so

It's ready.

TheBlueMatt

Will open a followup.

TheBlueMatt · 2022-12-06T18:06:00Z

-	/// they MUST NOT be allowed to change to different values once set.
+	/// counterparty_selected/holder_selected_contest_delay and funding outpoint. Since these are
+	/// static channel data, they MUST NOT be allowed to change to different values once set, as LDK
+	/// may call this method more than once.


Huh? LDK will absolutely not call this method more than once (for a given instance).

well, for a disk-backed signer that returns a shared reference to derive_* (which is how VLS works internally), it effectively does that. the instances are indexed by the channel keys ID.

or another way to look at it - an enforcing signer must return a shared reference so that it can keep track of a unique signer state

Right, my point is that for a given instance of the trait we won't call it more than once. From the perspective of something that exists as multiple instances of the trait its different. Anyway, we can continue this discussion on #1903

wpaulino · 2022-11-21T21:37:34Z

Fixes#1209.

codecov-commenter · 2022-11-22T01:22:15Z

Codecov Report

Base: 90.69% // Head: 90.54% // Decreases project coverage by -0.14%⚠️

Coverage data is based on head (444fce7) compared to base (52edb35).
Patch coverage: 89.55% of modified lines in pull request are covered.

Additional details and impacted files

@@ Coverage Diff @@## main #1867 +/- ##
==========================================
- Coverage 90.69% 90.54% -0.15% 
==========================================
Files 91 91 Lines 48408 48401 -7 Branches 48408 48401 -7 ==========================================
- Hits 43902 43825 -77 - Misses 4506 4576 +70

Impacted Files	Coverage Δ
lightning/src/chain/package.rs	`92.84% <ø> (ø)`
lightning/src/util/byte_utils.rs	`100.00% <ø> (ø)`
lightning/src/util/test_utils.rs	`73.50% <71.42%> (-3.56%)`	⬇️
lightning/src/ln/channelmanager.rs	`86.30% <80.00%> (+0.01%)`	⬆️
lightning/src/ln/channel.rs	`88.70% <80.43%> (-0.14%)`	⬇️
lightning/src/chain/channelmonitor.rs	`90.88% <92.85%> (+0.14%)`	⬆️
lightning/src/chain/keysinterface.rs	`83.14% <100.00%> (-7.82%)`	⬇️
lightning/src/chain/onchaintx.rs	`95.13% <100.00%> (-0.17%)`	⬇️
lightning/src/ln/chan_utils.rs	`93.62% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.59% <100.00%> (ø)`
... and 14 more

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.

wpaulino · 2022-11-22T19:49:15Z

Pushed a new update that allows downgrading and replaces get_channel_signer with derive_channel_signer.

TheBlueMatt · 2022-11-22T20:06:03Z

Hmmmmm, now that I think about it we really need to pass the user_channel_id through to the new-signer function. We could do that by passing an enum that either has a keys_id or a user_id to the method, but given the two methods are likely to do very different things (read state vs persist new state) I'm very unconvinced that they need to be merged.

arik-so · 2022-11-22T20:26:39Z

Hm, yeah, I can see how that can be helpful, though imo that's only necessary prior to a channel_keys_id existing to identify the channel. Perhaps the derive method should take an enum of either a user_channel_id or a channel_keys_id?

TheBlueMatt · 2022-11-22T20:51:32Z

Right, if we want a single derive method we'd probably want to do that, but that also makes me thinking we dont want to merge the methods.

arik-so · 2022-11-22T21:02:26Z

I disagree. I feel very strongly wr shouldn't have both get and derive. However, I think I have a better idea. Stay tuned lol

TheBlueMatt · 2022-11-22T21:15:33Z

Why not? They do two fundamentally different things - one loads an instance from disk, one constructs a new instance (and presumably writes it to disk).

arik-so · 2022-11-22T22:41:57Z

Because the entire point of this exercise is a) to obviate this distinction and b) to start phasing out disk I/O altogether.

arik-so · 2022-11-22T23:29:19Z


 let holder_selected_contest_delay = config.channel_handshake_config.our_to_self_delay;
- let holder_signer = keys_provider.get_channel_signer(false, channel_value_satoshis);
+ let channel_keys_id = keys_provider.generate_channel_keys_id(user_id);


completely unrelated to this PR, but that variable really shouldn't be called user_id

obviously, user_provided_channel_id is a bit verbose, but at least it's accurate

TheBlueMatt · 2022-11-23T04:11:35Z

Because the entire point of this exercise is a) to obviate this distinction and b) to start phasing out disk I/O altogether.

No? We cannot remove the I/O for a signer - an enforcing signer absolutely must do IO for every action. What doesn't make sense is the I/O being done "for" a user - they have to do it inline during the signing operations themselves.

arik-so · 2022-11-23T04:22:00Z

Sorry, I meant I/O done by us.

TheBlueMatt · 2022-11-23T04:34:10Z

Right, I don't see how that's relevant to whether we combine the new-signer and the derive-preexisting-signer methods?

TheBlueMatt

Okay, I think we're basically on the same page, one note about further code we can remove but otherwise looks good.

wpaulino · 2022-12-01T23:18:03Z

Had to rebase on latest to address a conflict.

TheBlueMatt · 2022-12-02T19:03:49Z

Needs #1867 (comment) addressed.

`get_channel_signer` previously had two different responsibilites: generating unique `channel_keys_id` and using said ID to derive channel keys. We decide to split it into two methods `generate_channel_keys_id` and `derive_channel_signer`, such that we can use the latter to fulfill our goal of re-deriving signers instead of persisting them. There's no point in storing data that can be easily re-derived.

Now that ready_channel is also called on startup upon deserializing channels, we opt to rename it to a more indicative name. We also derive `PartialEq` on ChannelTransactionParameters to allow implementations to determine whether `provide_channel_parameters` calls are idempotent after the channel parameters have already been provided.

To do so, we introduce a new serialization version that doesn't store a channel's signer, and instead stores its signer's `channel_keys_id`. This is a unique identifier that can be provided to our `KeysInterface` to re-derive all private key material for said channel. We choose to not upgrade the minimum compatible serialization version until a later time, which will also remove any signer serialization logic on implementations of `KeysInterface` and `Sign`.

Similar to the previous commit, we introduce a new serialization version that doesn't store a monitor's signer. Since the monitor already knows of a channel's `channel_keys_id`, there's no need to store any new data to re-derive all private key material for said channel.

Since `ChannelMonitor`s will now re-derive signers rather than persisting them, we can no longer use the OnlyReadsKeysInterface concrete implementation.

Now that we opt to always re-derive channel secrets whenever required, we can drop the Clone requirement from Sign.

Now that to_be_bytes is available under our current MSRV of 1.41, we can use it instead of our own version.

TheBlueMatt · 2022-12-05T21:09:18Z

Would like an ACK from @devrandom

arik-so · 2022-12-06T00:14:41Z

 	node_secret: SecretKey,
 	inbound_payment_key: KeyMaterial,
 	counter: AtomicU64,
+	signer_state: RefCell<HashMap<u8, (bool, Arc<Mutex<EnforcementState>>)>>


I know it's internal only, but a comment here would be extremely helpful. I think a comment above counter would be helpful, too. What are we counting, right?

arik-so

It's ready.

TheBlueMatt

Will open a followup.

TheBlueMatt · 2022-12-06T18:06:00Z

-	/// they MUST NOT be allowed to change to different values once set.
+	/// counterparty_selected/holder_selected_contest_delay and funding outpoint. Since these are
+	/// static channel data, they MUST NOT be allowed to change to different values once set, as LDK
+	/// may call this method more than once.


Huh? LDK will absolutely not call this method more than once (for a given instance).

well, for a disk-backed signer that returns a shared reference to derive_* (which is how VLS works internally), it effectively does that. the instances are indexed by the channel keys ID.

or another way to look at it - an enforcing signer must return a shared reference so that it can keep track of a unique signer state

Right, my point is that for a given instance of the trait we won't call it more than once. From the perspective of something that exists as multiple instances of the trait its different. Anyway, we can continue this discussion on #1903

wpaulino marked this pull request as ready for review November 21, 2022 21:37

arik-so reviewed Nov 21, 2022
View reviewed changes

Comment threadlightning/src/ln/channel.rs Outdated

TheBlueMatt reviewed Nov 22, 2022
View reviewed changes

Comment threadlightning/src/chain/keysinterface.rs Outdated
Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/chain/onchaintx.rs

wpaulino force-pushed the remove-signer-persistence branch from f139370 to 5defb92Compare November 22, 2022 19:47

wpaulino force-pushed the remove-signer-persistence branch from 5defb92 to 224dbd3Compare November 22, 2022 23:20

arik-so reviewed Nov 22, 2022
View reviewed changes

TheBlueMatt reviewed Nov 23, 2022
View reviewed changes

Comment threadlightning/src/chain/keysinterface.rs Outdated
Comment threadlightning/src/chain/onchaintx.rs Outdated
Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated

ariard reviewed Nov 24, 2022
View reviewed changes

Comment threadlightning/src/chain/keysinterface.rs
Comment threadlightning/src/chain/keysinterface.rs Outdated

wpaulino force-pushed the remove-signer-persistence branch from 224dbd3 to cda514cCompare November 29, 2022 17:20

TheBlueMatt reviewed Nov 29, 2022
View reviewed changes

Comment threadlightning/src/chain/keysinterface.rs Outdated
Comment threadlightning/src/chain/keysinterface.rs
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/util/byte_utils.rs Outdated

devrandom reviewed Nov 30, 2022
View reviewed changes

Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/chain/onchaintx.rs
Comment threadlightning/src/ln/channel.rs Outdated

wpaulino force-pushed the remove-signer-persistence branch from cda514c to 8e2a89fCompare November 30, 2022 23:05

tnull mentioned this pull request Dec 1, 2022
Clean up docs in keysinterface.rs#1892
Merged

TheBlueMatt added this to the 0.0.113 milestone Dec 1, 2022

wpaulino force-pushed the remove-signer-persistence branch from 8e2a89f to a079946Compare December 1, 2022 18:52

TheBlueMatt assigned TheBlueMatt and arik-so Dec 1, 2022

TheBlueMatt reviewed Dec 1, 2022
View reviewed changes

Comment threadlightning/src/util/byte_utils.rs Outdated
Comment threadlightning/src/chain/keysinterface.rs Outdated
Comment threadlightning/src/chain/keysinterface.rs
Comment threadlightning/src/chain/onchaintx.rs
Comment threadlightning/src/ln/channel.rs Outdated

wpaulino force-pushed the remove-signer-persistence branch from a079946 to 1141cbbCompare December 1, 2022 22:58

TheBlueMatt reviewed Dec 2, 2022
View reviewed changes

Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/chain/keysinterface.rs

wpaulino force-pushed the remove-signer-persistence branch from 840cf90 to 0572c77Compare December 2, 2022 01:42

TheBlueMatt previously approved these changes Dec 2, 2022
View reviewed changes

arik-so previously approved these changes Dec 2, 2022
View reviewed changes

TheBlueMatt mentioned this pull request Dec 2, 2022
Fix some onion errors and assert their length is correct #1895
Merged

wpaulino dismissed stale reviews from arik-so and TheBlueMatt via 4501bffDecember 5, 2022 18:54

wpaulino force-pushed the remove-signer-persistence branch from 0572c77 to 4501bffCompare December 5, 2022 18:54

devrandom reviewed Dec 5, 2022
View reviewed changes

Comment threadlightning/src/chain/keysinterface.rs Outdated

wpaulino force-pushed the remove-signer-persistence branch from 4501bff to 9acf5f0Compare December 5, 2022 20:08

wpaulino added 7 commits December 5, 2022 12:11

Avoid use of OnlyReadsKeysInterface
215619b
Since `ChannelMonitor`s will now re-derive signers rather than persisting them, we can no longer use the OnlyReadsKeysInterface concrete implementation.

Drop Clone requirement from Sign
607cd6f
Now that we opt to always re-derive channel secrets whenever required, we can drop the Clone requirement from Sign.

Remove unnecessary byte_utils helpers
444fce7
Now that to_be_bytes is available under our current MSRV of 1.41, we can use it instead of our own version.

wpaulino force-pushed the remove-signer-persistence branch from 9acf5f0 to 444fce7Compare December 5, 2022 20:11

arik-so reviewed Dec 6, 2022
View reviewed changes

Comment threadlightning/src/chain/keysinterface.rs

arik-so approved these changes Dec 6, 2022
View reviewed changes

devrandom approved these changes Dec 6, 2022
View reviewed changes

TheBlueMatt approved these changes Dec 6, 2022
View reviewed changes

TheBlueMatt merged commit 5588eeb into lightningdevkit:mainDec 6, 2022

TheBlueMatt mentioned this pull request Dec 6, 2022
Do not default to storing InMemoryChannelKeys keys on disk #1209
Closed

wpaulino deleted the remove-signer-persistence branch December 6, 2022 19:09

wpaulino mentioned this pull request Dec 6, 2022
Clarify docs on provide_channel_parameters#1903
Merged