Clean up docs in `keysinterface.rs` by tnull · Pull Request #1892 · lightningdevkit/rust-lightning

tnull · 2022-12-01T14:21:04Z

Started off when I found some room for improvement in the docs of SpendableOutputDescriptor, took me down the rabbit hole of "since I'm already here"...

Hope this doesn't have too many conflicts with #1867 (but if I see correctly, it shouldn't)...

ariard · 2022-12-01T22:31:46Z

+	/// This may be useful in re-deriving keys used in the channel to spend the output.
 	pub channel_keys_id: [u8; 32],
-	/// The value of the channel which this transactions spends.
+	/// The value of the channel which this transaction spends.


"The value of the funding channel output spent by this commitment transaction. This may be useful in re-deriving keys used in the channel to spend the output". This value isn't consumed directly by the signature digest as the output spent is a to_remote on a counterparty commitment transaction.

Now went with "The value of the funding channel output which this transaction spends." to keep it concise. Let me know if that's fine by you.

codecov-commenter · 2022-12-02T08:05:07Z

Codecov Report

Base: 90.60% // Head: 91.06% // Increases project coverage by +0.45% 🎉

Coverage data is based on head (6d49c17) compared to base (d9d4611).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 6d49c17 differs from pull request most recent head 350e8a1. Consider uploading reports for the commit 350e8a1 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1892 +/- ##
==========================================
+ Coverage 90.60% 91.06% +0.45% 
==========================================
Files 91 91 Lines 48656 51205 +2549 Branches 48656 51205 +2549 ==========================================
+ Hits 44087 46632 +2545 - Misses 4569 4573 +4

Impacted Files	Coverage Δ
lightning/src/ln/functional_tests.rs	`96.96% <ø> (-0.18%)`	⬇️
lightning/src/chain/keysinterface.rs	`83.14% <100.00%> (ø)`
lightning/src/chain/onchaintx.rs	`93.77% <0.00%> (+0.82%)`	⬆️
lightning/src/ln/channelmanager.rs	`89.41% <0.00%> (+2.72%)`	⬆️
lightning/src/ln/channel.rs	`92.23% <0.00%> (+3.37%)`	⬆️

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.

tnull · 2022-12-06T19:18:42Z

Rebased after #1867 got merged.

TheBlueMatt · 2022-12-06T20:03:39Z

 #[derive(Clone, Debug, PartialEq, Eq)]
 pub struct DelayedPaymentOutputDescriptor {
-	/// The outpoint which is spendable
+	/// The outpoint which is spendable.


This isn't a fully-formed valid sentence, not sure why its getting a period? Same goes for a number of later things.

Mh, while it stands out in this case because the line is so short, I still think it should have a period, in particular to maintain consistency with other examples with longer lines, in which we def. want a period. E.g.:
/// An output to a P2WPKH, spendable exclusively by our payment key (i.e., the private key /// which corresponds to the `payment_point` in [`BaseSign::pubkeys`]). The witness /// in the spending input is, thus, simply:

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

+	/// This may be useful in re-deriving keys used in the channel to spend the output.
 	pub channel_keys_id: [u8; 32],
-	/// The value of the channel which this transactions spends.
+	/// The value of the funding channel output which this transaction spends.


I find "the funding channel output" substantially more confusing than "the channel". Its now much easier to get confused and think this is the value in the output, but its really the value of the channel.

I tend to agree, a change in this direction was however suggested in #1892 (comment).
@ariard, are you fine with me reverting this?

tnull · 2022-12-07T09:12:04Z

Rebased again after #1825 got merged.

TheBlueMatt · 2022-12-07T23:22:39Z

Feel free to squash, IMO. With some resolution on #1892 (comment) I'm happy with this I think.

tnull · 2022-12-08T09:38:20Z

Feel free to squash, IMO. With some resolution on #1892 (comment) I'm happy with this I think.

Squashed with the reverted version. @ariard let me know if you ACK.

tnull · 2022-12-10T08:53:51Z

Squashed again with requested changes.

wpaulino

Note this will conflict a bit with #1910 so it'd be nice to land this soon.

tnull · 2022-12-12T20:32:59Z

Squashed again with changes:

> git diff-tree -U2 350e8a19 03de0598
diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs
index 74294d5c..1426ff5c 100644
--- a/lightning/src/chain/keysinterface.rs
+++ b/lightning/src/chain/keysinterface.rs
@@ -295,5 +295,4 @@ pub trait BaseSign {
/// [`ChannelMonitor`]: crate::chain::channelmonitor::ChannelMonitor
// TODO: Document the things someone using this interface should enforce before signing.
- // TODO: Key derivation failure should panic rather than Err
fn sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction,
secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(Signature, Vec<Signature>), ()>;
@@ -535,5 +534,6 @@ pub trait KeysInterface {
/// a secure external signer.
pub struct InMemorySigner {
- /// Private key of an anchor or 2-of-2 multisig transaction.
+ /// Holder secret key in the 2-of-2 multisig script of a channel. This key also backs the
+ /// holder's anchor output in a commitment transaction, if one is present.
pub funding_key: SecretKey,
/// Holder secret key for blinded revocation pubkey.

TheBlueMatt · 2022-12-12T21:29:37Z

-	/// [`KeysInterface::derive_channel_signer`]. The `user_channel_id` is provided to allow
-	/// implementations of `KeysInterface` to maintain a mapping between it and the generated
-	/// `channel_keys_id`.
+	/// Get a new set of [`Sign`] for per-channel secrets. These MUST be unique even if you


This doc makes much less sense to me than the previous one. We're not generating a "set" here, I think, and we should make explicit reference to derive_channel_signer.

Ah it seems like this came from a rebase. We want to keep the docs as they currently are in main.

Let's just revert it in a followup, since this is blocking other stuff.

I'll do it in #1903 since that will need rebase on this anyway.

TheBlueMatt

One comment but otherwise definitely looks good. Also happy to address it in a followup.

tnull · 2022-12-01T14:21:04Z

Started off when I found some room for improvement in the docs of SpendableOutputDescriptor, took me down the rabbit hole of "since I'm already here"...

Hope this doesn't have too many conflicts with #1867 (but if I see correctly, it shouldn't)...

ariard · 2022-12-01T22:31:46Z

+	/// This may be useful in re-deriving keys used in the channel to spend the output.
 	pub channel_keys_id: [u8; 32],
-	/// The value of the channel which this transactions spends.
+	/// The value of the channel which this transaction spends.


"The value of the funding channel output spent by this commitment transaction. This may be useful in re-deriving keys used in the channel to spend the output". This value isn't consumed directly by the signature digest as the output spent is a to_remote on a counterparty commitment transaction.

Now went with "The value of the funding channel output which this transaction spends." to keep it concise. Let me know if that's fine by you.

codecov-commenter · 2022-12-02T08:05:07Z

Codecov Report

Base: 90.60% // Head: 91.06% // Increases project coverage by +0.45% 🎉

Coverage data is based on head (6d49c17) compared to base (d9d4611).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 6d49c17 differs from pull request most recent head 350e8a1. Consider uploading reports for the commit 350e8a1 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1892 +/- ##
==========================================
+ Coverage 90.60% 91.06% +0.45% 
==========================================
Files 91 91 Lines 48656 51205 +2549 Branches 48656 51205 +2549 ==========================================
+ Hits 44087 46632 +2545 - Misses 4569 4573 +4

Impacted Files	Coverage Δ
lightning/src/ln/functional_tests.rs	`96.96% <ø> (-0.18%)`	⬇️
lightning/src/chain/keysinterface.rs	`83.14% <100.00%> (ø)`
lightning/src/chain/onchaintx.rs	`93.77% <0.00%> (+0.82%)`	⬆️
lightning/src/ln/channelmanager.rs	`89.41% <0.00%> (+2.72%)`	⬆️
lightning/src/ln/channel.rs	`92.23% <0.00%> (+3.37%)`	⬆️

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.

tnull · 2022-12-06T19:18:42Z

Rebased after #1867 got merged.

TheBlueMatt · 2022-12-06T20:03:39Z

 #[derive(Clone, Debug, PartialEq, Eq)]
 pub struct DelayedPaymentOutputDescriptor {
-	/// The outpoint which is spendable
+	/// The outpoint which is spendable.


This isn't a fully-formed valid sentence, not sure why its getting a period? Same goes for a number of later things.

Mh, while it stands out in this case because the line is so short, I still think it should have a period, in particular to maintain consistency with other examples with longer lines, in which we def. want a period. E.g.:
/// An output to a P2WPKH, spendable exclusively by our payment key (i.e., the private key /// which corresponds to the `payment_point` in [`BaseSign::pubkeys`]). The witness /// in the spending input is, thus, simply:

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

+	/// This may be useful in re-deriving keys used in the channel to spend the output.
 	pub channel_keys_id: [u8; 32],
-	/// The value of the channel which this transactions spends.
+	/// The value of the funding channel output which this transaction spends.


I find "the funding channel output" substantially more confusing than "the channel". Its now much easier to get confused and think this is the value in the output, but its really the value of the channel.

I tend to agree, a change in this direction was however suggested in #1892 (comment).
@ariard, are you fine with me reverting this?

tnull · 2022-12-07T09:12:04Z

Rebased again after #1825 got merged.

TheBlueMatt · 2022-12-07T23:22:39Z

Feel free to squash, IMO. With some resolution on #1892 (comment) I'm happy with this I think.

tnull · 2022-12-08T09:38:20Z

Feel free to squash, IMO. With some resolution on #1892 (comment) I'm happy with this I think.

Squashed with the reverted version. @ariard let me know if you ACK.

tnull · 2022-12-10T08:53:51Z

Squashed again with requested changes.

wpaulino

Note this will conflict a bit with #1910 so it'd be nice to land this soon.

tnull · 2022-12-12T20:32:59Z

Squashed again with changes:

> git diff-tree -U2 350e8a19 03de0598
diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs
index 74294d5c..1426ff5c 100644
--- a/lightning/src/chain/keysinterface.rs
+++ b/lightning/src/chain/keysinterface.rs
@@ -295,5 +295,4 @@ pub trait BaseSign {
/// [`ChannelMonitor`]: crate::chain::channelmonitor::ChannelMonitor
// TODO: Document the things someone using this interface should enforce before signing.
- // TODO: Key derivation failure should panic rather than Err
fn sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction,
secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(Signature, Vec<Signature>), ()>;
@@ -535,5 +534,6 @@ pub trait KeysInterface {
/// a secure external signer.
pub struct InMemorySigner {
- /// Private key of an anchor or 2-of-2 multisig transaction.
+ /// Holder secret key in the 2-of-2 multisig script of a channel. This key also backs the
+ /// holder's anchor output in a commitment transaction, if one is present.
pub funding_key: SecretKey,
/// Holder secret key for blinded revocation pubkey.

TheBlueMatt · 2022-12-12T21:29:37Z

-	/// [`KeysInterface::derive_channel_signer`]. The `user_channel_id` is provided to allow
-	/// implementations of `KeysInterface` to maintain a mapping between it and the generated
-	/// `channel_keys_id`.
+	/// Get a new set of [`Sign`] for per-channel secrets. These MUST be unique even if you


This doc makes much less sense to me than the previous one. We're not generating a "set" here, I think, and we should make explicit reference to derive_channel_signer.

Ah it seems like this came from a rebase. We want to keep the docs as they currently are in main.

Let's just revert it in a followup, since this is blocking other stuff.

I'll do it in #1903 since that will need rebase on this anyway.

TheBlueMatt

One comment but otherwise definitely looks good. Also happy to address it in a followup.

tnull · 2022-12-01T14:21:04Z

Started off when I found some room for improvement in the docs of SpendableOutputDescriptor, took me down the rabbit hole of "since I'm already here"...

Hope this doesn't have too many conflicts with #1867 (but if I see correctly, it shouldn't)...

ariard · 2022-12-01T22:31:46Z

+	/// This may be useful in re-deriving keys used in the channel to spend the output.
 	pub channel_keys_id: [u8; 32],
-	/// The value of the channel which this transactions spends.
+	/// The value of the channel which this transaction spends.


"The value of the funding channel output spent by this commitment transaction. This may be useful in re-deriving keys used in the channel to spend the output". This value isn't consumed directly by the signature digest as the output spent is a to_remote on a counterparty commitment transaction.

Now went with "The value of the funding channel output which this transaction spends." to keep it concise. Let me know if that's fine by you.

codecov-commenter · 2022-12-02T08:05:07Z

Codecov Report

Base: 90.60% // Head: 91.06% // Increases project coverage by +0.45% 🎉

Coverage data is based on head (6d49c17) compared to base (d9d4611).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 6d49c17 differs from pull request most recent head 350e8a1. Consider uploading reports for the commit 350e8a1 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1892 +/- ##
==========================================
+ Coverage 90.60% 91.06% +0.45% 
==========================================
Files 91 91 Lines 48656 51205 +2549 Branches 48656 51205 +2549 ==========================================
+ Hits 44087 46632 +2545 - Misses 4569 4573 +4

Impacted Files	Coverage Δ
lightning/src/ln/functional_tests.rs	`96.96% <ø> (-0.18%)`	⬇️
lightning/src/chain/keysinterface.rs	`83.14% <100.00%> (ø)`
lightning/src/chain/onchaintx.rs	`93.77% <0.00%> (+0.82%)`	⬆️
lightning/src/ln/channelmanager.rs	`89.41% <0.00%> (+2.72%)`	⬆️
lightning/src/ln/channel.rs	`92.23% <0.00%> (+3.37%)`	⬆️

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.

tnull · 2022-12-06T19:18:42Z

Rebased after #1867 got merged.

TheBlueMatt · 2022-12-06T20:03:39Z

 #[derive(Clone, Debug, PartialEq, Eq)]
 pub struct DelayedPaymentOutputDescriptor {
-	/// The outpoint which is spendable
+	/// The outpoint which is spendable.


This isn't a fully-formed valid sentence, not sure why its getting a period? Same goes for a number of later things.

Mh, while it stands out in this case because the line is so short, I still think it should have a period, in particular to maintain consistency with other examples with longer lines, in which we def. want a period. E.g.:
/// An output to a P2WPKH, spendable exclusively by our payment key (i.e., the private key /// which corresponds to the `payment_point` in [`BaseSign::pubkeys`]). The witness /// in the spending input is, thus, simply:

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

+	/// This may be useful in re-deriving keys used in the channel to spend the output.
 	pub channel_keys_id: [u8; 32],
-	/// The value of the channel which this transactions spends.
+	/// The value of the funding channel output which this transaction spends.


I find "the funding channel output" substantially more confusing than "the channel". Its now much easier to get confused and think this is the value in the output, but its really the value of the channel.

I tend to agree, a change in this direction was however suggested in #1892 (comment).
@ariard, are you fine with me reverting this?

tnull · 2022-12-07T09:12:04Z

Rebased again after #1825 got merged.

TheBlueMatt · 2022-12-07T23:22:39Z

Feel free to squash, IMO. With some resolution on #1892 (comment) I'm happy with this I think.

tnull · 2022-12-08T09:38:20Z

Feel free to squash, IMO. With some resolution on #1892 (comment) I'm happy with this I think.

Squashed with the reverted version. @ariard let me know if you ACK.

tnull · 2022-12-10T08:53:51Z

Squashed again with requested changes.

wpaulino

Note this will conflict a bit with #1910 so it'd be nice to land this soon.

tnull · 2022-12-12T20:32:59Z

Squashed again with changes:

> git diff-tree -U2 350e8a19 03de0598
diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs
index 74294d5c..1426ff5c 100644
--- a/lightning/src/chain/keysinterface.rs
+++ b/lightning/src/chain/keysinterface.rs
@@ -295,5 +295,4 @@ pub trait BaseSign {
/// [`ChannelMonitor`]: crate::chain::channelmonitor::ChannelMonitor
// TODO: Document the things someone using this interface should enforce before signing.
- // TODO: Key derivation failure should panic rather than Err
fn sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction,
secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(Signature, Vec<Signature>), ()>;
@@ -535,5 +534,6 @@ pub trait KeysInterface {
/// a secure external signer.
pub struct InMemorySigner {
- /// Private key of an anchor or 2-of-2 multisig transaction.
+ /// Holder secret key in the 2-of-2 multisig script of a channel. This key also backs the
+ /// holder's anchor output in a commitment transaction, if one is present.
pub funding_key: SecretKey,
/// Holder secret key for blinded revocation pubkey.

TheBlueMatt · 2022-12-12T21:29:37Z

-	/// [`KeysInterface::derive_channel_signer`]. The `user_channel_id` is provided to allow
-	/// implementations of `KeysInterface` to maintain a mapping between it and the generated
-	/// `channel_keys_id`.
+	/// Get a new set of [`Sign`] for per-channel secrets. These MUST be unique even if you


This doc makes much less sense to me than the previous one. We're not generating a "set" here, I think, and we should make explicit reference to derive_channel_signer.

Ah it seems like this came from a rebase. We want to keep the docs as they currently are in main.

Let's just revert it in a followup, since this is blocking other stuff.

I'll do it in #1903 since that will need rebase on this anyway.

TheBlueMatt

One comment but otherwise definitely looks good. Also happy to address it in a followup.

tnull · 2022-12-01T14:21:04Z

Started off when I found some room for improvement in the docs of SpendableOutputDescriptor, took me down the rabbit hole of "since I'm already here"...

Hope this doesn't have too many conflicts with #1867 (but if I see correctly, it shouldn't)...

ariard · 2022-12-01T22:31:46Z

+	/// This may be useful in re-deriving keys used in the channel to spend the output.
 	pub channel_keys_id: [u8; 32],
-	/// The value of the channel which this transactions spends.
+	/// The value of the channel which this transaction spends.


"The value of the funding channel output spent by this commitment transaction. This may be useful in re-deriving keys used in the channel to spend the output". This value isn't consumed directly by the signature digest as the output spent is a to_remote on a counterparty commitment transaction.

Now went with "The value of the funding channel output which this transaction spends." to keep it concise. Let me know if that's fine by you.

codecov-commenter · 2022-12-02T08:05:07Z

Codecov Report

Base: 90.60% // Head: 91.06% // Increases project coverage by +0.45% 🎉

Coverage data is based on head (6d49c17) compared to base (d9d4611).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 6d49c17 differs from pull request most recent head 350e8a1. Consider uploading reports for the commit 350e8a1 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1892 +/- ##
==========================================
+ Coverage 90.60% 91.06% +0.45% 
==========================================
Files 91 91 Lines 48656 51205 +2549 Branches 48656 51205 +2549 ==========================================
+ Hits 44087 46632 +2545 - Misses 4569 4573 +4

Impacted Files	Coverage Δ
lightning/src/ln/functional_tests.rs	`96.96% <ø> (-0.18%)`	⬇️
lightning/src/chain/keysinterface.rs	`83.14% <100.00%> (ø)`
lightning/src/chain/onchaintx.rs	`93.77% <0.00%> (+0.82%)`	⬆️
lightning/src/ln/channelmanager.rs	`89.41% <0.00%> (+2.72%)`	⬆️
lightning/src/ln/channel.rs	`92.23% <0.00%> (+3.37%)`	⬆️

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.

tnull · 2022-12-06T19:18:42Z

Rebased after #1867 got merged.

TheBlueMatt · 2022-12-06T20:03:39Z

 #[derive(Clone, Debug, PartialEq, Eq)]
 pub struct DelayedPaymentOutputDescriptor {
-	/// The outpoint which is spendable
+	/// The outpoint which is spendable.


This isn't a fully-formed valid sentence, not sure why its getting a period? Same goes for a number of later things.

Mh, while it stands out in this case because the line is so short, I still think it should have a period, in particular to maintain consistency with other examples with longer lines, in which we def. want a period. E.g.:
/// An output to a P2WPKH, spendable exclusively by our payment key (i.e., the private key /// which corresponds to the `payment_point` in [`BaseSign::pubkeys`]). The witness /// in the spending input is, thus, simply:

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

+	/// This may be useful in re-deriving keys used in the channel to spend the output.
 	pub channel_keys_id: [u8; 32],
-	/// The value of the channel which this transactions spends.
+	/// The value of the funding channel output which this transaction spends.


I find "the funding channel output" substantially more confusing than "the channel". Its now much easier to get confused and think this is the value in the output, but its really the value of the channel.

I tend to agree, a change in this direction was however suggested in #1892 (comment).
@ariard, are you fine with me reverting this?

tnull · 2022-12-07T09:12:04Z

Rebased again after #1825 got merged.

TheBlueMatt · 2022-12-07T23:22:39Z

Feel free to squash, IMO. With some resolution on #1892 (comment) I'm happy with this I think.

tnull · 2022-12-08T09:38:20Z

Feel free to squash, IMO. With some resolution on #1892 (comment) I'm happy with this I think.

Squashed with the reverted version. @ariard let me know if you ACK.

tnull · 2022-12-10T08:53:51Z

Squashed again with requested changes.

wpaulino

Note this will conflict a bit with #1910 so it'd be nice to land this soon.

tnull · 2022-12-12T20:32:59Z

Squashed again with changes:

> git diff-tree -U2 350e8a19 03de0598
diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs
index 74294d5c..1426ff5c 100644
--- a/lightning/src/chain/keysinterface.rs
+++ b/lightning/src/chain/keysinterface.rs
@@ -295,5 +295,4 @@ pub trait BaseSign {
/// [`ChannelMonitor`]: crate::chain::channelmonitor::ChannelMonitor
// TODO: Document the things someone using this interface should enforce before signing.
- // TODO: Key derivation failure should panic rather than Err
fn sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction,
secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(Signature, Vec<Signature>), ()>;
@@ -535,5 +534,6 @@ pub trait KeysInterface {
/// a secure external signer.
pub struct InMemorySigner {
- /// Private key of an anchor or 2-of-2 multisig transaction.
+ /// Holder secret key in the 2-of-2 multisig script of a channel. This key also backs the
+ /// holder's anchor output in a commitment transaction, if one is present.
pub funding_key: SecretKey,
/// Holder secret key for blinded revocation pubkey.

TheBlueMatt · 2022-12-12T21:29:37Z

-	/// [`KeysInterface::derive_channel_signer`]. The `user_channel_id` is provided to allow
-	/// implementations of `KeysInterface` to maintain a mapping between it and the generated
-	/// `channel_keys_id`.
+	/// Get a new set of [`Sign`] for per-channel secrets. These MUST be unique even if you


This doc makes much less sense to me than the previous one. We're not generating a "set" here, I think, and we should make explicit reference to derive_channel_signer.

Ah it seems like this came from a rebase. We want to keep the docs as they currently are in main.

Let's just revert it in a followup, since this is blocking other stuff.

I'll do it in #1903 since that will need rebase on this anyway.

TheBlueMatt

One comment but otherwise definitely looks good. Also happy to address it in a followup.

tnull · 2022-12-01T14:21:04Z

Started off when I found some room for improvement in the docs of SpendableOutputDescriptor, took me down the rabbit hole of "since I'm already here"...

Hope this doesn't have too many conflicts with #1867 (but if I see correctly, it shouldn't)...

ariard · 2022-12-01T22:31:46Z

+	/// This may be useful in re-deriving keys used in the channel to spend the output.
 	pub channel_keys_id: [u8; 32],
-	/// The value of the channel which this transactions spends.
+	/// The value of the channel which this transaction spends.


"The value of the funding channel output spent by this commitment transaction. This may be useful in re-deriving keys used in the channel to spend the output". This value isn't consumed directly by the signature digest as the output spent is a to_remote on a counterparty commitment transaction.

Now went with "The value of the funding channel output which this transaction spends." to keep it concise. Let me know if that's fine by you.

codecov-commenter · 2022-12-02T08:05:07Z

Codecov Report

Base: 90.60% // Head: 91.06% // Increases project coverage by +0.45% 🎉

Coverage data is based on head (6d49c17) compared to base (d9d4611).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 6d49c17 differs from pull request most recent head 350e8a1. Consider uploading reports for the commit 350e8a1 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1892 +/- ##
==========================================
+ Coverage 90.60% 91.06% +0.45% 
==========================================
Files 91 91 Lines 48656 51205 +2549 Branches 48656 51205 +2549 ==========================================
+ Hits 44087 46632 +2545 - Misses 4569 4573 +4

Impacted Files	Coverage Δ
lightning/src/ln/functional_tests.rs	`96.96% <ø> (-0.18%)`	⬇️
lightning/src/chain/keysinterface.rs	`83.14% <100.00%> (ø)`
lightning/src/chain/onchaintx.rs	`93.77% <0.00%> (+0.82%)`	⬆️
lightning/src/ln/channelmanager.rs	`89.41% <0.00%> (+2.72%)`	⬆️
lightning/src/ln/channel.rs	`92.23% <0.00%> (+3.37%)`	⬆️

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.

tnull · 2022-12-06T19:18:42Z

Rebased after #1867 got merged.

TheBlueMatt · 2022-12-06T20:03:39Z

 #[derive(Clone, Debug, PartialEq, Eq)]
 pub struct DelayedPaymentOutputDescriptor {
-	/// The outpoint which is spendable
+	/// The outpoint which is spendable.


This isn't a fully-formed valid sentence, not sure why its getting a period? Same goes for a number of later things.

Mh, while it stands out in this case because the line is so short, I still think it should have a period, in particular to maintain consistency with other examples with longer lines, in which we def. want a period. E.g.:
/// An output to a P2WPKH, spendable exclusively by our payment key (i.e., the private key /// which corresponds to the `payment_point` in [`BaseSign::pubkeys`]). The witness /// in the spending input is, thus, simply:

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

+	/// This may be useful in re-deriving keys used in the channel to spend the output.
 	pub channel_keys_id: [u8; 32],
-	/// The value of the channel which this transactions spends.
+	/// The value of the funding channel output which this transaction spends.


I find "the funding channel output" substantially more confusing than "the channel". Its now much easier to get confused and think this is the value in the output, but its really the value of the channel.

I tend to agree, a change in this direction was however suggested in #1892 (comment).
@ariard, are you fine with me reverting this?

tnull · 2022-12-07T09:12:04Z

Rebased again after #1825 got merged.

TheBlueMatt · 2022-12-07T23:22:39Z

Feel free to squash, IMO. With some resolution on #1892 (comment) I'm happy with this I think.

tnull · 2022-12-08T09:38:20Z

Feel free to squash, IMO. With some resolution on #1892 (comment) I'm happy with this I think.

Squashed with the reverted version. @ariard let me know if you ACK.

tnull · 2022-12-10T08:53:51Z

Squashed again with requested changes.

wpaulino

Note this will conflict a bit with #1910 so it'd be nice to land this soon.

tnull · 2022-12-12T20:32:59Z

Squashed again with changes:

> git diff-tree -U2 350e8a19 03de0598
diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs
index 74294d5c..1426ff5c 100644
--- a/lightning/src/chain/keysinterface.rs
+++ b/lightning/src/chain/keysinterface.rs
@@ -295,5 +295,4 @@ pub trait BaseSign {
/// [`ChannelMonitor`]: crate::chain::channelmonitor::ChannelMonitor
// TODO: Document the things someone using this interface should enforce before signing.
- // TODO: Key derivation failure should panic rather than Err
fn sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction,
secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(Signature, Vec<Signature>), ()>;
@@ -535,5 +534,6 @@ pub trait KeysInterface {
/// a secure external signer.
pub struct InMemorySigner {
- /// Private key of an anchor or 2-of-2 multisig transaction.
+ /// Holder secret key in the 2-of-2 multisig script of a channel. This key also backs the
+ /// holder's anchor output in a commitment transaction, if one is present.
pub funding_key: SecretKey,
/// Holder secret key for blinded revocation pubkey.

TheBlueMatt · 2022-12-12T21:29:37Z

-	/// [`KeysInterface::derive_channel_signer`]. The `user_channel_id` is provided to allow
-	/// implementations of `KeysInterface` to maintain a mapping between it and the generated
-	/// `channel_keys_id`.
+	/// Get a new set of [`Sign`] for per-channel secrets. These MUST be unique even if you


This doc makes much less sense to me than the previous one. We're not generating a "set" here, I think, and we should make explicit reference to derive_channel_signer.

Ah it seems like this came from a rebase. We want to keep the docs as they currently are in main.

Let's just revert it in a followup, since this is blocking other stuff.

I'll do it in #1903 since that will need rebase on this anyway.

TheBlueMatt

One comment but otherwise definitely looks good. Also happy to address it in a followup.

tnull · 2022-12-01T14:21:04Z

Started off when I found some room for improvement in the docs of SpendableOutputDescriptor, took me down the rabbit hole of "since I'm already here"...

Hope this doesn't have too many conflicts with #1867 (but if I see correctly, it shouldn't)...

ariard · 2022-12-01T22:31:46Z

+	/// This may be useful in re-deriving keys used in the channel to spend the output.
 	pub channel_keys_id: [u8; 32],
-	/// The value of the channel which this transactions spends.
+	/// The value of the channel which this transaction spends.


"The value of the funding channel output spent by this commitment transaction. This may be useful in re-deriving keys used in the channel to spend the output". This value isn't consumed directly by the signature digest as the output spent is a to_remote on a counterparty commitment transaction.

Now went with "The value of the funding channel output which this transaction spends." to keep it concise. Let me know if that's fine by you.

codecov-commenter · 2022-12-02T08:05:07Z

Codecov Report

Base: 90.60% // Head: 91.06% // Increases project coverage by +0.45% 🎉

Coverage data is based on head (6d49c17) compared to base (d9d4611).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 6d49c17 differs from pull request most recent head 350e8a1. Consider uploading reports for the commit 350e8a1 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1892 +/- ##
==========================================
+ Coverage 90.60% 91.06% +0.45% 
==========================================
Files 91 91 Lines 48656 51205 +2549 Branches 48656 51205 +2549 ==========================================
+ Hits 44087 46632 +2545 - Misses 4569 4573 +4

Impacted Files	Coverage Δ
lightning/src/ln/functional_tests.rs	`96.96% <ø> (-0.18%)`	⬇️
lightning/src/chain/keysinterface.rs	`83.14% <100.00%> (ø)`
lightning/src/chain/onchaintx.rs	`93.77% <0.00%> (+0.82%)`	⬆️
lightning/src/ln/channelmanager.rs	`89.41% <0.00%> (+2.72%)`	⬆️
lightning/src/ln/channel.rs	`92.23% <0.00%> (+3.37%)`	⬆️

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.

tnull · 2022-12-06T19:18:42Z

Rebased after #1867 got merged.

TheBlueMatt · 2022-12-06T20:03:39Z

 #[derive(Clone, Debug, PartialEq, Eq)]
 pub struct DelayedPaymentOutputDescriptor {
-	/// The outpoint which is spendable
+	/// The outpoint which is spendable.


This isn't a fully-formed valid sentence, not sure why its getting a period? Same goes for a number of later things.

Mh, while it stands out in this case because the line is so short, I still think it should have a period, in particular to maintain consistency with other examples with longer lines, in which we def. want a period. E.g.:
/// An output to a P2WPKH, spendable exclusively by our payment key (i.e., the private key /// which corresponds to the `payment_point` in [`BaseSign::pubkeys`]). The witness /// in the spending input is, thus, simply:

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

+	/// This may be useful in re-deriving keys used in the channel to spend the output.
 	pub channel_keys_id: [u8; 32],
-	/// The value of the channel which this transactions spends.
+	/// The value of the funding channel output which this transaction spends.


I find "the funding channel output" substantially more confusing than "the channel". Its now much easier to get confused and think this is the value in the output, but its really the value of the channel.

I tend to agree, a change in this direction was however suggested in #1892 (comment).
@ariard, are you fine with me reverting this?

tnull · 2022-12-07T09:12:04Z

Rebased again after #1825 got merged.

TheBlueMatt · 2022-12-07T23:22:39Z

Feel free to squash, IMO. With some resolution on #1892 (comment) I'm happy with this I think.

tnull · 2022-12-08T09:38:20Z

Feel free to squash, IMO. With some resolution on #1892 (comment) I'm happy with this I think.

Squashed with the reverted version. @ariard let me know if you ACK.

tnull · 2022-12-10T08:53:51Z

Squashed again with requested changes.

wpaulino

Note this will conflict a bit with #1910 so it'd be nice to land this soon.

tnull · 2022-12-12T20:32:59Z

Squashed again with changes:

> git diff-tree -U2 350e8a19 03de0598
diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs
index 74294d5c..1426ff5c 100644
--- a/lightning/src/chain/keysinterface.rs
+++ b/lightning/src/chain/keysinterface.rs
@@ -295,5 +295,4 @@ pub trait BaseSign {
/// [`ChannelMonitor`]: crate::chain::channelmonitor::ChannelMonitor
// TODO: Document the things someone using this interface should enforce before signing.
- // TODO: Key derivation failure should panic rather than Err
fn sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction,
secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(Signature, Vec<Signature>), ()>;
@@ -535,5 +534,6 @@ pub trait KeysInterface {
/// a secure external signer.
pub struct InMemorySigner {
- /// Private key of an anchor or 2-of-2 multisig transaction.
+ /// Holder secret key in the 2-of-2 multisig script of a channel. This key also backs the
+ /// holder's anchor output in a commitment transaction, if one is present.
pub funding_key: SecretKey,
/// Holder secret key for blinded revocation pubkey.

TheBlueMatt · 2022-12-12T21:29:37Z

-	/// [`KeysInterface::derive_channel_signer`]. The `user_channel_id` is provided to allow
-	/// implementations of `KeysInterface` to maintain a mapping between it and the generated
-	/// `channel_keys_id`.
+	/// Get a new set of [`Sign`] for per-channel secrets. These MUST be unique even if you


This doc makes much less sense to me than the previous one. We're not generating a "set" here, I think, and we should make explicit reference to derive_channel_signer.

Ah it seems like this came from a rebase. We want to keep the docs as they currently are in main.

Let's just revert it in a followup, since this is blocking other stuff.

I'll do it in #1903 since that will need rebase on this anyway.

TheBlueMatt

One comment but otherwise definitely looks good. Also happy to address it in a followup.

tnull · 2022-12-01T14:21:04Z

Started off when I found some room for improvement in the docs of SpendableOutputDescriptor, took me down the rabbit hole of "since I'm already here"...

Hope this doesn't have too many conflicts with #1867 (but if I see correctly, it shouldn't)...

ariard · 2022-12-01T22:31:46Z

+	/// This may be useful in re-deriving keys used in the channel to spend the output.
 	pub channel_keys_id: [u8; 32],
-	/// The value of the channel which this transactions spends.
+	/// The value of the channel which this transaction spends.


"The value of the funding channel output spent by this commitment transaction. This may be useful in re-deriving keys used in the channel to spend the output". This value isn't consumed directly by the signature digest as the output spent is a to_remote on a counterparty commitment transaction.

Now went with "The value of the funding channel output which this transaction spends." to keep it concise. Let me know if that's fine by you.

codecov-commenter · 2022-12-02T08:05:07Z

Codecov Report

Base: 90.60% // Head: 91.06% // Increases project coverage by +0.45% 🎉

Coverage data is based on head (6d49c17) compared to base (d9d4611).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 6d49c17 differs from pull request most recent head 350e8a1. Consider uploading reports for the commit 350e8a1 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1892 +/- ##
==========================================
+ Coverage 90.60% 91.06% +0.45% 
==========================================
Files 91 91 Lines 48656 51205 +2549 Branches 48656 51205 +2549 ==========================================
+ Hits 44087 46632 +2545 - Misses 4569 4573 +4

Impacted Files	Coverage Δ
lightning/src/ln/functional_tests.rs	`96.96% <ø> (-0.18%)`	⬇️
lightning/src/chain/keysinterface.rs	`83.14% <100.00%> (ø)`
lightning/src/chain/onchaintx.rs	`93.77% <0.00%> (+0.82%)`	⬆️
lightning/src/ln/channelmanager.rs	`89.41% <0.00%> (+2.72%)`	⬆️
lightning/src/ln/channel.rs	`92.23% <0.00%> (+3.37%)`	⬆️

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.

tnull · 2022-12-06T19:18:42Z

Rebased after #1867 got merged.

TheBlueMatt · 2022-12-06T20:03:39Z

 #[derive(Clone, Debug, PartialEq, Eq)]
 pub struct DelayedPaymentOutputDescriptor {
-	/// The outpoint which is spendable
+	/// The outpoint which is spendable.


This isn't a fully-formed valid sentence, not sure why its getting a period? Same goes for a number of later things.

Mh, while it stands out in this case because the line is so short, I still think it should have a period, in particular to maintain consistency with other examples with longer lines, in which we def. want a period. E.g.:
/// An output to a P2WPKH, spendable exclusively by our payment key (i.e., the private key /// which corresponds to the `payment_point` in [`BaseSign::pubkeys`]). The witness /// in the spending input is, thus, simply:

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

+	/// This may be useful in re-deriving keys used in the channel to spend the output.
 	pub channel_keys_id: [u8; 32],
-	/// The value of the channel which this transactions spends.
+	/// The value of the funding channel output which this transaction spends.


I find "the funding channel output" substantially more confusing than "the channel". Its now much easier to get confused and think this is the value in the output, but its really the value of the channel.

I tend to agree, a change in this direction was however suggested in #1892 (comment).
@ariard, are you fine with me reverting this?

tnull · 2022-12-07T09:12:04Z

Rebased again after #1825 got merged.

TheBlueMatt · 2022-12-07T23:22:39Z

Feel free to squash, IMO. With some resolution on #1892 (comment) I'm happy with this I think.

tnull · 2022-12-08T09:38:20Z

Feel free to squash, IMO. With some resolution on #1892 (comment) I'm happy with this I think.

Squashed with the reverted version. @ariard let me know if you ACK.

tnull · 2022-12-10T08:53:51Z

Squashed again with requested changes.

wpaulino

Note this will conflict a bit with #1910 so it'd be nice to land this soon.

tnull · 2022-12-12T20:32:59Z

Squashed again with changes:

> git diff-tree -U2 350e8a19 03de0598
diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs
index 74294d5c..1426ff5c 100644
--- a/lightning/src/chain/keysinterface.rs
+++ b/lightning/src/chain/keysinterface.rs
@@ -295,5 +295,4 @@ pub trait BaseSign {
/// [`ChannelMonitor`]: crate::chain::channelmonitor::ChannelMonitor
// TODO: Document the things someone using this interface should enforce before signing.
- // TODO: Key derivation failure should panic rather than Err
fn sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction,
secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(Signature, Vec<Signature>), ()>;
@@ -535,5 +534,6 @@ pub trait KeysInterface {
/// a secure external signer.
pub struct InMemorySigner {
- /// Private key of an anchor or 2-of-2 multisig transaction.
+ /// Holder secret key in the 2-of-2 multisig script of a channel. This key also backs the
+ /// holder's anchor output in a commitment transaction, if one is present.
pub funding_key: SecretKey,
/// Holder secret key for blinded revocation pubkey.

TheBlueMatt · 2022-12-12T21:29:37Z

-	/// [`KeysInterface::derive_channel_signer`]. The `user_channel_id` is provided to allow
-	/// implementations of `KeysInterface` to maintain a mapping between it and the generated
-	/// `channel_keys_id`.
+	/// Get a new set of [`Sign`] for per-channel secrets. These MUST be unique even if you


This doc makes much less sense to me than the previous one. We're not generating a "set" here, I think, and we should make explicit reference to derive_channel_signer.

Ah it seems like this came from a rebase. We want to keep the docs as they currently are in main.

Let's just revert it in a followup, since this is blocking other stuff.

I'll do it in #1903 since that will need rebase on this anyway.

TheBlueMatt

One comment but otherwise definitely looks good. Also happy to address it in a followup.

tnull · 2022-12-01T14:21:04Z

Started off when I found some room for improvement in the docs of SpendableOutputDescriptor, took me down the rabbit hole of "since I'm already here"...

Hope this doesn't have too many conflicts with #1867 (but if I see correctly, it shouldn't)...

ariard · 2022-12-01T22:31:46Z

+	/// This may be useful in re-deriving keys used in the channel to spend the output.
 	pub channel_keys_id: [u8; 32],
-	/// The value of the channel which this transactions spends.
+	/// The value of the channel which this transaction spends.


"The value of the funding channel output spent by this commitment transaction. This may be useful in re-deriving keys used in the channel to spend the output". This value isn't consumed directly by the signature digest as the output spent is a to_remote on a counterparty commitment transaction.

Now went with "The value of the funding channel output which this transaction spends." to keep it concise. Let me know if that's fine by you.

codecov-commenter · 2022-12-02T08:05:07Z

Codecov Report

Base: 90.60% // Head: 91.06% // Increases project coverage by +0.45% 🎉

Coverage data is based on head (6d49c17) compared to base (d9d4611).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 6d49c17 differs from pull request most recent head 350e8a1. Consider uploading reports for the commit 350e8a1 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1892 +/- ##
==========================================
+ Coverage 90.60% 91.06% +0.45% 
==========================================
Files 91 91 Lines 48656 51205 +2549 Branches 48656 51205 +2549 ==========================================
+ Hits 44087 46632 +2545 - Misses 4569 4573 +4

Impacted Files	Coverage Δ
lightning/src/ln/functional_tests.rs	`96.96% <ø> (-0.18%)`	⬇️
lightning/src/chain/keysinterface.rs	`83.14% <100.00%> (ø)`
lightning/src/chain/onchaintx.rs	`93.77% <0.00%> (+0.82%)`	⬆️
lightning/src/ln/channelmanager.rs	`89.41% <0.00%> (+2.72%)`	⬆️
lightning/src/ln/channel.rs	`92.23% <0.00%> (+3.37%)`	⬆️

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.

tnull · 2022-12-06T19:18:42Z

Rebased after #1867 got merged.

TheBlueMatt · 2022-12-06T20:03:39Z

 #[derive(Clone, Debug, PartialEq, Eq)]
 pub struct DelayedPaymentOutputDescriptor {
-	/// The outpoint which is spendable
+	/// The outpoint which is spendable.


This isn't a fully-formed valid sentence, not sure why its getting a period? Same goes for a number of later things.

Mh, while it stands out in this case because the line is so short, I still think it should have a period, in particular to maintain consistency with other examples with longer lines, in which we def. want a period. E.g.:
/// An output to a P2WPKH, spendable exclusively by our payment key (i.e., the private key /// which corresponds to the `payment_point` in [`BaseSign::pubkeys`]). The witness /// in the spending input is, thus, simply:

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

+	/// This may be useful in re-deriving keys used in the channel to spend the output.
 	pub channel_keys_id: [u8; 32],
-	/// The value of the channel which this transactions spends.
+	/// The value of the funding channel output which this transaction spends.


I find "the funding channel output" substantially more confusing than "the channel". Its now much easier to get confused and think this is the value in the output, but its really the value of the channel.

I tend to agree, a change in this direction was however suggested in #1892 (comment).
@ariard, are you fine with me reverting this?

tnull · 2022-12-07T09:12:04Z

Rebased again after #1825 got merged.

TheBlueMatt · 2022-12-07T23:22:39Z

Feel free to squash, IMO. With some resolution on #1892 (comment) I'm happy with this I think.

tnull · 2022-12-08T09:38:20Z

Feel free to squash, IMO. With some resolution on #1892 (comment) I'm happy with this I think.

Squashed with the reverted version. @ariard let me know if you ACK.

tnull · 2022-12-10T08:53:51Z

Squashed again with requested changes.

wpaulino

Note this will conflict a bit with #1910 so it'd be nice to land this soon.

tnull · 2022-12-12T20:32:59Z

Squashed again with changes:

> git diff-tree -U2 350e8a19 03de0598
diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs
index 74294d5c..1426ff5c 100644
--- a/lightning/src/chain/keysinterface.rs
+++ b/lightning/src/chain/keysinterface.rs
@@ -295,5 +295,4 @@ pub trait BaseSign {
/// [`ChannelMonitor`]: crate::chain::channelmonitor::ChannelMonitor
// TODO: Document the things someone using this interface should enforce before signing.
- // TODO: Key derivation failure should panic rather than Err
fn sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction,
secp_ctx: &Secp256k1<secp256k1::All>) -> Result<(Signature, Vec<Signature>), ()>;
@@ -535,5 +534,6 @@ pub trait KeysInterface {
/// a secure external signer.
pub struct InMemorySigner {
- /// Private key of an anchor or 2-of-2 multisig transaction.
+ /// Holder secret key in the 2-of-2 multisig script of a channel. This key also backs the
+ /// holder's anchor output in a commitment transaction, if one is present.
pub funding_key: SecretKey,
/// Holder secret key for blinded revocation pubkey.

TheBlueMatt · 2022-12-12T21:29:37Z

-	/// [`KeysInterface::derive_channel_signer`]. The `user_channel_id` is provided to allow
-	/// implementations of `KeysInterface` to maintain a mapping between it and the generated
-	/// `channel_keys_id`.
+	/// Get a new set of [`Sign`] for per-channel secrets. These MUST be unique even if you


This doc makes much less sense to me than the previous one. We're not generating a "set" here, I think, and we should make explicit reference to derive_channel_signer.

Ah it seems like this came from a rebase. We want to keep the docs as they currently are in main.

Let's just revert it in a followup, since this is blocking other stuff.

I'll do it in #1903 since that will need rebase on this anyway.

TheBlueMatt

One comment but otherwise definitely looks good. Also happy to address it in a followup.

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from 53eab2b to b579852Compare December 1, 2022 14:39

ariard reviewed Dec 1, 2022
View reviewed changes

tnull added the Seeking Code Review label Dec 3, 2022

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from b749d60 to afeb894Compare December 6, 2022 19:16

tnull requested a review from wpaulino December 6, 2022 19:19

TheBlueMatt reviewed Dec 6, 2022
View reviewed changes

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from afeb894 to 47c98c1Compare December 7, 2022 09:11

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch 2 times, most recently from c6580ac to d6cd997Compare December 7, 2022 10:11

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from d6cd997 to bcb1f04Compare December 8, 2022 09:37

wpaulino reviewed Dec 9, 2022
View reviewed changes

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

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from 6d49c17 to 350e8a1Compare December 10, 2022 08:53

wpaulino reviewed Dec 12, 2022
View reviewed changes

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

TheBlueMatt removed the Seeking Code Review label Dec 12, 2022

TheBlueMatt assigned TheBlueMatt and wpaulino Dec 12, 2022

Clean up docs in keysinterface.rs
03de059

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from 350e8a1 to 03de059Compare December 12, 2022 20:31

wpaulino approved these changes Dec 12, 2022
View reviewed changes

TheBlueMatt reviewed Dec 12, 2022
View reviewed changes

TheBlueMatt approved these changes Dec 12, 2022
View reviewed changes

TheBlueMatt mentioned this pull request Dec 12, 2022
Clarify docs on provide_channel_parameters#1903
Merged

TheBlueMatt merged commit 0fa67fb into lightningdevkit:mainDec 12, 2022

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from 53eab2b to b579852Compare December 1, 2022 14:39

ariard reviewed Dec 1, 2022
View reviewed changes

tnull added the Seeking Code Review label Dec 3, 2022

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from b749d60 to afeb894Compare December 6, 2022 19:16

tnull requested a review from wpaulino December 6, 2022 19:19

TheBlueMatt reviewed Dec 6, 2022
View reviewed changes

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from afeb894 to 47c98c1Compare December 7, 2022 09:11

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch 2 times, most recently from c6580ac to d6cd997Compare December 7, 2022 10:11

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from d6cd997 to bcb1f04Compare December 8, 2022 09:37

wpaulino reviewed Dec 9, 2022
View reviewed changes

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

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from 6d49c17 to 350e8a1Compare December 10, 2022 08:53

wpaulino reviewed Dec 12, 2022
View reviewed changes

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

TheBlueMatt removed the Seeking Code Review label Dec 12, 2022

TheBlueMatt assigned TheBlueMatt and wpaulino Dec 12, 2022

Clean up docs in keysinterface.rs
03de059

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from 350e8a1 to 03de059Compare December 12, 2022 20:31

wpaulino approved these changes Dec 12, 2022
View reviewed changes

TheBlueMatt reviewed Dec 12, 2022
View reviewed changes

TheBlueMatt approved these changes Dec 12, 2022
View reviewed changes

TheBlueMatt mentioned this pull request Dec 12, 2022
Clarify docs on provide_channel_parameters#1903
Merged

TheBlueMatt merged commit 0fa67fb into lightningdevkit:mainDec 12, 2022

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from 53eab2b to b579852Compare December 1, 2022 14:39

ariard reviewed Dec 1, 2022
View reviewed changes

tnull added the Seeking Code Review label Dec 3, 2022

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from b749d60 to afeb894Compare December 6, 2022 19:16

tnull requested a review from wpaulino December 6, 2022 19:19

TheBlueMatt reviewed Dec 6, 2022
View reviewed changes

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from afeb894 to 47c98c1Compare December 7, 2022 09:11

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch 2 times, most recently from c6580ac to d6cd997Compare December 7, 2022 10:11

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from d6cd997 to bcb1f04Compare December 8, 2022 09:37

wpaulino reviewed Dec 9, 2022
View reviewed changes

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

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from 6d49c17 to 350e8a1Compare December 10, 2022 08:53

wpaulino reviewed Dec 12, 2022
View reviewed changes

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

TheBlueMatt removed the Seeking Code Review label Dec 12, 2022

TheBlueMatt assigned TheBlueMatt and wpaulino Dec 12, 2022

Clean up docs in keysinterface.rs
03de059

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from 350e8a1 to 03de059Compare December 12, 2022 20:31

wpaulino approved these changes Dec 12, 2022
View reviewed changes

TheBlueMatt reviewed Dec 12, 2022
View reviewed changes

TheBlueMatt approved these changes Dec 12, 2022
View reviewed changes

TheBlueMatt mentioned this pull request Dec 12, 2022
Clarify docs on provide_channel_parameters#1903
Merged

TheBlueMatt merged commit 0fa67fb into lightningdevkit:mainDec 12, 2022

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from 53eab2b to b579852Compare December 1, 2022 14:39

ariard reviewed Dec 1, 2022
View reviewed changes

tnull added the Seeking Code Review label Dec 3, 2022

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from b749d60 to afeb894Compare December 6, 2022 19:16

tnull requested a review from wpaulino December 6, 2022 19:19

TheBlueMatt reviewed Dec 6, 2022
View reviewed changes

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from afeb894 to 47c98c1Compare December 7, 2022 09:11

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch 2 times, most recently from c6580ac to d6cd997Compare December 7, 2022 10:11

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from d6cd997 to bcb1f04Compare December 8, 2022 09:37

wpaulino reviewed Dec 9, 2022
View reviewed changes

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

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from 6d49c17 to 350e8a1Compare December 10, 2022 08:53

wpaulino reviewed Dec 12, 2022
View reviewed changes

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

TheBlueMatt removed the Seeking Code Review label Dec 12, 2022

TheBlueMatt assigned TheBlueMatt and wpaulino Dec 12, 2022

Clean up docs in keysinterface.rs
03de059

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from 350e8a1 to 03de059Compare December 12, 2022 20:31

wpaulino approved these changes Dec 12, 2022
View reviewed changes

TheBlueMatt reviewed Dec 12, 2022
View reviewed changes

TheBlueMatt approved these changes Dec 12, 2022
View reviewed changes

TheBlueMatt mentioned this pull request Dec 12, 2022
Clarify docs on provide_channel_parameters#1903
Merged

TheBlueMatt merged commit 0fa67fb into lightningdevkit:mainDec 12, 2022

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from 53eab2b to b579852Compare December 1, 2022 14:39

ariard reviewed Dec 1, 2022
View reviewed changes

tnull added the Seeking Code Review label Dec 3, 2022

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from b749d60 to afeb894Compare December 6, 2022 19:16

tnull requested a review from wpaulino December 6, 2022 19:19

TheBlueMatt reviewed Dec 6, 2022
View reviewed changes

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from afeb894 to 47c98c1Compare December 7, 2022 09:11

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch 2 times, most recently from c6580ac to d6cd997Compare December 7, 2022 10:11

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from d6cd997 to bcb1f04Compare December 8, 2022 09:37

wpaulino reviewed Dec 9, 2022
View reviewed changes

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

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from 6d49c17 to 350e8a1Compare December 10, 2022 08:53

wpaulino reviewed Dec 12, 2022
View reviewed changes

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

TheBlueMatt removed the Seeking Code Review label Dec 12, 2022

TheBlueMatt assigned TheBlueMatt and wpaulino Dec 12, 2022

Clean up docs in keysinterface.rs
03de059

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from 350e8a1 to 03de059Compare December 12, 2022 20:31

wpaulino approved these changes Dec 12, 2022
View reviewed changes

TheBlueMatt reviewed Dec 12, 2022
View reviewed changes

TheBlueMatt approved these changes Dec 12, 2022
View reviewed changes

TheBlueMatt mentioned this pull request Dec 12, 2022
Clarify docs on provide_channel_parameters#1903
Merged

TheBlueMatt merged commit 0fa67fb into lightningdevkit:mainDec 12, 2022

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from 53eab2b to b579852Compare December 1, 2022 14:39

ariard reviewed Dec 1, 2022
View reviewed changes

tnull added the Seeking Code Review label Dec 3, 2022

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from b749d60 to afeb894Compare December 6, 2022 19:16

tnull requested a review from wpaulino December 6, 2022 19:19

TheBlueMatt reviewed Dec 6, 2022
View reviewed changes

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from afeb894 to 47c98c1Compare December 7, 2022 09:11

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch 2 times, most recently from c6580ac to d6cd997Compare December 7, 2022 10:11

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from d6cd997 to bcb1f04Compare December 8, 2022 09:37

wpaulino reviewed Dec 9, 2022
View reviewed changes

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

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from 6d49c17 to 350e8a1Compare December 10, 2022 08:53

wpaulino reviewed Dec 12, 2022
View reviewed changes

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

TheBlueMatt removed the Seeking Code Review label Dec 12, 2022

TheBlueMatt assigned TheBlueMatt and wpaulino Dec 12, 2022

Clean up docs in keysinterface.rs
03de059

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from 350e8a1 to 03de059Compare December 12, 2022 20:31

wpaulino approved these changes Dec 12, 2022
View reviewed changes

TheBlueMatt reviewed Dec 12, 2022
View reviewed changes

TheBlueMatt approved these changes Dec 12, 2022
View reviewed changes

TheBlueMatt mentioned this pull request Dec 12, 2022
Clarify docs on provide_channel_parameters#1903
Merged

TheBlueMatt merged commit 0fa67fb into lightningdevkit:mainDec 12, 2022

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from 53eab2b to b579852Compare December 1, 2022 14:39

ariard reviewed Dec 1, 2022
View reviewed changes

tnull added the Seeking Code Review label Dec 3, 2022

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from b749d60 to afeb894Compare December 6, 2022 19:16

tnull requested a review from wpaulino December 6, 2022 19:19

TheBlueMatt reviewed Dec 6, 2022
View reviewed changes

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from afeb894 to 47c98c1Compare December 7, 2022 09:11

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch 2 times, most recently from c6580ac to d6cd997Compare December 7, 2022 10:11

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from d6cd997 to bcb1f04Compare December 8, 2022 09:37

wpaulino reviewed Dec 9, 2022
View reviewed changes

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

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from 6d49c17 to 350e8a1Compare December 10, 2022 08:53

wpaulino reviewed Dec 12, 2022
View reviewed changes

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

TheBlueMatt removed the Seeking Code Review label Dec 12, 2022

TheBlueMatt assigned TheBlueMatt and wpaulino Dec 12, 2022

Clean up docs in keysinterface.rs
03de059

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from 350e8a1 to 03de059Compare December 12, 2022 20:31

wpaulino approved these changes Dec 12, 2022
View reviewed changes

TheBlueMatt reviewed Dec 12, 2022
View reviewed changes

TheBlueMatt approved these changes Dec 12, 2022
View reviewed changes

TheBlueMatt mentioned this pull request Dec 12, 2022
Clarify docs on provide_channel_parameters#1903
Merged

TheBlueMatt merged commit 0fa67fb into lightningdevkit:mainDec 12, 2022

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from 53eab2b to b579852Compare December 1, 2022 14:39

ariard reviewed Dec 1, 2022
View reviewed changes

tnull added the Seeking Code Review label Dec 3, 2022

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from b749d60 to afeb894Compare December 6, 2022 19:16

tnull requested a review from wpaulino December 6, 2022 19:19

TheBlueMatt reviewed Dec 6, 2022
View reviewed changes

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from afeb894 to 47c98c1Compare December 7, 2022 09:11

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch 2 times, most recently from c6580ac to d6cd997Compare December 7, 2022 10:11

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from d6cd997 to bcb1f04Compare December 8, 2022 09:37

wpaulino reviewed Dec 9, 2022
View reviewed changes

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

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from 6d49c17 to 350e8a1Compare December 10, 2022 08:53

wpaulino reviewed Dec 12, 2022
View reviewed changes

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

TheBlueMatt removed the Seeking Code Review label Dec 12, 2022

TheBlueMatt assigned TheBlueMatt and wpaulino Dec 12, 2022

Clean up docs in keysinterface.rs
03de059

tnull force-pushed the 2022-12-spendableoutputdescriptor-doccs branch from 350e8a1 to 03de059Compare December 12, 2022 20:31

wpaulino approved these changes Dec 12, 2022
View reviewed changes

TheBlueMatt reviewed Dec 12, 2022
View reviewed changes

TheBlueMatt approved these changes Dec 12, 2022
View reviewed changes

TheBlueMatt mentioned this pull request Dec 12, 2022
Clarify docs on provide_channel_parameters#1903
Merged

TheBlueMatt merged commit 0fa67fb into lightningdevkit:mainDec 12, 2022

Uh oh!

Conversation

tnull commented Dec 1, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnullDec 2, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Dec 2, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tnull commented Dec 6, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tnull commented Dec 7, 2022

Uh oh!

TheBlueMatt commented Dec 7, 2022

Uh oh!

tnull commented Dec 8, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tnull commented Dec 10, 2022

Uh oh!

wpaulino left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tnull commented Dec 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

tnull commented Dec 1, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

tnull commented Dec 1, 2022•
edited
Loading

tnullDec 2, 2022•
edited
Loading

codecov-commenter commented Dec 2, 2022•
edited
Loading

tnull commented Dec 8, 2022•
edited
Loading

tnull commented Dec 1, 2022•
edited
Loading

tnullDec 2, 2022•
edited
Loading

codecov-commenter commented Dec 2, 2022•
edited
Loading

tnull commented Dec 8, 2022•
edited
Loading