Clarify docs on `provide_channel_parameters` by TheBlueMatt · Pull Request #1903 · lightningdevkit/rust-lightning

TheBlueMatt · 2022-12-06T18:17:47Z

Its very confusing to say that LDK will call
provide_channel_parameters more than once - its true for a channel, but not for a given instance. Instead, phrase the docs with reference to a specific instance, which is much clearer.

codecov-commenter · 2022-12-06T19:03:50Z

Codecov Report

Base: 90.77% // Head: 90.76% // Decreases project coverage by -0.01%⚠️

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

Additional details and impacted files

@@ Coverage Diff @@## main #1903 +/- ##
==========================================
- Coverage 90.77% 90.76% -0.02% 
==========================================
Files 94 94 Lines 49603 49603 Branches 49603 49603 ==========================================
- Hits 45025 45020 -5 - Misses 4578 4583 +5

Impacted Files	Coverage Δ
lightning/src/chain/keysinterface.rs	`83.14% <ø> (ø)`
lightning/src/chain/onchaintx.rs	`95.17% <0.00%> (-0.21%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.07% <0.00%> (-0.07%)`	⬇️

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-12-06T19:11:31Z

I had something similar in a previous iteration and @devrandom thought otherwise #1867 (comment).

TheBlueMatt · 2022-12-06T19:21:23Z

Added a note that the "instance" here includes calling again if the object was created by read_chan_signer. I don't buy that we should just say "LDK calls this more than once", because it precisely does not, and saying that it can call this more than once but users need to somehow handle that and ignore the new state is even more confusing - if there's multiple instances of a given signer now you have to have some shared state across them.

wpaulino · 2022-12-08T18:51:26Z

+	///
+	/// This data is static, and will never change for a channel once set. For a given [`BaseSign`]
+	/// instance, LDK will call this method exactly once - either immediately after construction
+	/// (including if done via [`KeysInterface::read_chan_signer`]) or when the funding information


We don't ever call it immediately after read_chan_signer, it either must have been called in the past and was serialized, or it will be called once funding occurs.

Oops, lol, right.

wpaulino

LGTM after squash.

devrandom · 2022-12-09T11:10:23Z

I'm OK with the doc in this PR.

however, I have some thoughts about the BaseSign docs in general, which may affect the thinking here.

when a dev is looking at the BaseSign docs, it seems likely they are actually trying to implement their own signer. if so, the docs should guide the dev towards best practices, which includes making the signer properly enforcing. as an example, one consequence of the signer being enforcing is the need for signer persistence.

so if that's the most likely use case that the docs should focus on, then the docs should probably be adjusted to address it, rather than implicitly focusing on how to use InMemorySigner.

TheBlueMatt · 2022-12-12T03:31:10Z

I don't think the wording here is wrong given that, either, though? I suppose, indeed, we should include a little more guidance on enforcement (or, honestly, just link to VLS, cause its a ton of work lol), but I also think someone may implement the sign trait with a goal of using different keys, learning when a transaction is being signed, or just doing naive policy checks (like closing transaction pays to the right place).

TheBlueMatt · 2022-12-12T21:33:57Z

Will address #1892 (comment) here

Its very confusing to say that LDK will call `provide_channel_parameters` more than once - its true for a channel, but not for a given instance. Instead, phrase the docs with reference to a specific instance, which is much clearer.

03de059 appeared to revert updated docs due to a rebase error. This reverts the docs on `generate_channel_keys` to the state they were in prior to that commit, with one additional doc.

TheBlueMatt · 2022-12-13T21:42:30Z

Rebased and did so.

TheBlueMatt · 2022-12-06T18:17:47Z

Its very confusing to say that LDK will call
provide_channel_parameters more than once - its true for a channel, but not for a given instance. Instead, phrase the docs with reference to a specific instance, which is much clearer.

codecov-commenter · 2022-12-06T19:03:50Z

Codecov Report

Base: 90.77% // Head: 90.76% // Decreases project coverage by -0.01%⚠️

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

Additional details and impacted files

@@ Coverage Diff @@## main #1903 +/- ##
==========================================
- Coverage 90.77% 90.76% -0.02% 
==========================================
Files 94 94 Lines 49603 49603 Branches 49603 49603 ==========================================
- Hits 45025 45020 -5 - Misses 4578 4583 +5

Impacted Files	Coverage Δ
lightning/src/chain/keysinterface.rs	`83.14% <ø> (ø)`
lightning/src/chain/onchaintx.rs	`95.17% <0.00%> (-0.21%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.07% <0.00%> (-0.07%)`	⬇️

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-12-06T19:11:31Z

I had something similar in a previous iteration and @devrandom thought otherwise #1867 (comment).

TheBlueMatt · 2022-12-06T19:21:23Z

Added a note that the "instance" here includes calling again if the object was created by read_chan_signer. I don't buy that we should just say "LDK calls this more than once", because it precisely does not, and saying that it can call this more than once but users need to somehow handle that and ignore the new state is even more confusing - if there's multiple instances of a given signer now you have to have some shared state across them.

wpaulino · 2022-12-08T18:51:26Z

+	///
+	/// This data is static, and will never change for a channel once set. For a given [`BaseSign`]
+	/// instance, LDK will call this method exactly once - either immediately after construction
+	/// (including if done via [`KeysInterface::read_chan_signer`]) or when the funding information


We don't ever call it immediately after read_chan_signer, it either must have been called in the past and was serialized, or it will be called once funding occurs.

Oops, lol, right.

wpaulino

LGTM after squash.

devrandom · 2022-12-09T11:10:23Z

I'm OK with the doc in this PR.

however, I have some thoughts about the BaseSign docs in general, which may affect the thinking here.

when a dev is looking at the BaseSign docs, it seems likely they are actually trying to implement their own signer. if so, the docs should guide the dev towards best practices, which includes making the signer properly enforcing. as an example, one consequence of the signer being enforcing is the need for signer persistence.

so if that's the most likely use case that the docs should focus on, then the docs should probably be adjusted to address it, rather than implicitly focusing on how to use InMemorySigner.

TheBlueMatt · 2022-12-12T03:31:10Z

I don't think the wording here is wrong given that, either, though? I suppose, indeed, we should include a little more guidance on enforcement (or, honestly, just link to VLS, cause its a ton of work lol), but I also think someone may implement the sign trait with a goal of using different keys, learning when a transaction is being signed, or just doing naive policy checks (like closing transaction pays to the right place).

TheBlueMatt · 2022-12-12T21:33:57Z

Will address #1892 (comment) here

Its very confusing to say that LDK will call `provide_channel_parameters` more than once - its true for a channel, but not for a given instance. Instead, phrase the docs with reference to a specific instance, which is much clearer.

03de059 appeared to revert updated docs due to a rebase error. This reverts the docs on `generate_channel_keys` to the state they were in prior to that commit, with one additional doc.

TheBlueMatt · 2022-12-13T21:42:30Z

Rebased and did so.

TheBlueMatt · 2022-12-06T18:17:47Z

Its very confusing to say that LDK will call
provide_channel_parameters more than once - its true for a channel, but not for a given instance. Instead, phrase the docs with reference to a specific instance, which is much clearer.

codecov-commenter · 2022-12-06T19:03:50Z

Codecov Report

Base: 90.77% // Head: 90.76% // Decreases project coverage by -0.01%⚠️

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

Additional details and impacted files

@@ Coverage Diff @@## main #1903 +/- ##
==========================================
- Coverage 90.77% 90.76% -0.02% 
==========================================
Files 94 94 Lines 49603 49603 Branches 49603 49603 ==========================================
- Hits 45025 45020 -5 - Misses 4578 4583 +5

Impacted Files	Coverage Δ
lightning/src/chain/keysinterface.rs	`83.14% <ø> (ø)`
lightning/src/chain/onchaintx.rs	`95.17% <0.00%> (-0.21%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.07% <0.00%> (-0.07%)`	⬇️

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-12-06T19:11:31Z

I had something similar in a previous iteration and @devrandom thought otherwise #1867 (comment).

TheBlueMatt · 2022-12-06T19:21:23Z

Added a note that the "instance" here includes calling again if the object was created by read_chan_signer. I don't buy that we should just say "LDK calls this more than once", because it precisely does not, and saying that it can call this more than once but users need to somehow handle that and ignore the new state is even more confusing - if there's multiple instances of a given signer now you have to have some shared state across them.

wpaulino · 2022-12-08T18:51:26Z

+	///
+	/// This data is static, and will never change for a channel once set. For a given [`BaseSign`]
+	/// instance, LDK will call this method exactly once - either immediately after construction
+	/// (including if done via [`KeysInterface::read_chan_signer`]) or when the funding information


We don't ever call it immediately after read_chan_signer, it either must have been called in the past and was serialized, or it will be called once funding occurs.

Oops, lol, right.

wpaulino

LGTM after squash.

devrandom · 2022-12-09T11:10:23Z

I'm OK with the doc in this PR.

however, I have some thoughts about the BaseSign docs in general, which may affect the thinking here.

when a dev is looking at the BaseSign docs, it seems likely they are actually trying to implement their own signer. if so, the docs should guide the dev towards best practices, which includes making the signer properly enforcing. as an example, one consequence of the signer being enforcing is the need for signer persistence.

so if that's the most likely use case that the docs should focus on, then the docs should probably be adjusted to address it, rather than implicitly focusing on how to use InMemorySigner.

TheBlueMatt · 2022-12-12T03:31:10Z

I don't think the wording here is wrong given that, either, though? I suppose, indeed, we should include a little more guidance on enforcement (or, honestly, just link to VLS, cause its a ton of work lol), but I also think someone may implement the sign trait with a goal of using different keys, learning when a transaction is being signed, or just doing naive policy checks (like closing transaction pays to the right place).

TheBlueMatt · 2022-12-12T21:33:57Z

Will address #1892 (comment) here

Its very confusing to say that LDK will call `provide_channel_parameters` more than once - its true for a channel, but not for a given instance. Instead, phrase the docs with reference to a specific instance, which is much clearer.

03de059 appeared to revert updated docs due to a rebase error. This reverts the docs on `generate_channel_keys` to the state they were in prior to that commit, with one additional doc.

TheBlueMatt · 2022-12-13T21:42:30Z

Rebased and did so.

TheBlueMatt · 2022-12-06T18:17:47Z

Its very confusing to say that LDK will call
provide_channel_parameters more than once - its true for a channel, but not for a given instance. Instead, phrase the docs with reference to a specific instance, which is much clearer.

codecov-commenter · 2022-12-06T19:03:50Z

Codecov Report

Base: 90.77% // Head: 90.76% // Decreases project coverage by -0.01%⚠️

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

Additional details and impacted files

@@ Coverage Diff @@## main #1903 +/- ##
==========================================
- Coverage 90.77% 90.76% -0.02% 
==========================================
Files 94 94 Lines 49603 49603 Branches 49603 49603 ==========================================
- Hits 45025 45020 -5 - Misses 4578 4583 +5

Impacted Files	Coverage Δ
lightning/src/chain/keysinterface.rs	`83.14% <ø> (ø)`
lightning/src/chain/onchaintx.rs	`95.17% <0.00%> (-0.21%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.07% <0.00%> (-0.07%)`	⬇️

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-12-06T19:11:31Z

I had something similar in a previous iteration and @devrandom thought otherwise #1867 (comment).

TheBlueMatt · 2022-12-06T19:21:23Z

Added a note that the "instance" here includes calling again if the object was created by read_chan_signer. I don't buy that we should just say "LDK calls this more than once", because it precisely does not, and saying that it can call this more than once but users need to somehow handle that and ignore the new state is even more confusing - if there's multiple instances of a given signer now you have to have some shared state across them.

wpaulino · 2022-12-08T18:51:26Z

+	///
+	/// This data is static, and will never change for a channel once set. For a given [`BaseSign`]
+	/// instance, LDK will call this method exactly once - either immediately after construction
+	/// (including if done via [`KeysInterface::read_chan_signer`]) or when the funding information


We don't ever call it immediately after read_chan_signer, it either must have been called in the past and was serialized, or it will be called once funding occurs.

Oops, lol, right.

wpaulino

LGTM after squash.

devrandom · 2022-12-09T11:10:23Z

I'm OK with the doc in this PR.

however, I have some thoughts about the BaseSign docs in general, which may affect the thinking here.

when a dev is looking at the BaseSign docs, it seems likely they are actually trying to implement their own signer. if so, the docs should guide the dev towards best practices, which includes making the signer properly enforcing. as an example, one consequence of the signer being enforcing is the need for signer persistence.

so if that's the most likely use case that the docs should focus on, then the docs should probably be adjusted to address it, rather than implicitly focusing on how to use InMemorySigner.

TheBlueMatt · 2022-12-12T03:31:10Z

I don't think the wording here is wrong given that, either, though? I suppose, indeed, we should include a little more guidance on enforcement (or, honestly, just link to VLS, cause its a ton of work lol), but I also think someone may implement the sign trait with a goal of using different keys, learning when a transaction is being signed, or just doing naive policy checks (like closing transaction pays to the right place).

TheBlueMatt · 2022-12-12T21:33:57Z

Will address #1892 (comment) here

Its very confusing to say that LDK will call `provide_channel_parameters` more than once - its true for a channel, but not for a given instance. Instead, phrase the docs with reference to a specific instance, which is much clearer.

03de059 appeared to revert updated docs due to a rebase error. This reverts the docs on `generate_channel_keys` to the state they were in prior to that commit, with one additional doc.

TheBlueMatt · 2022-12-13T21:42:30Z

Rebased and did so.

TheBlueMatt · 2022-12-06T18:17:47Z

Its very confusing to say that LDK will call
provide_channel_parameters more than once - its true for a channel, but not for a given instance. Instead, phrase the docs with reference to a specific instance, which is much clearer.

codecov-commenter · 2022-12-06T19:03:50Z

Codecov Report

Base: 90.77% // Head: 90.76% // Decreases project coverage by -0.01%⚠️

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

Additional details and impacted files

@@ Coverage Diff @@## main #1903 +/- ##
==========================================
- Coverage 90.77% 90.76% -0.02% 
==========================================
Files 94 94 Lines 49603 49603 Branches 49603 49603 ==========================================
- Hits 45025 45020 -5 - Misses 4578 4583 +5

Impacted Files	Coverage Δ
lightning/src/chain/keysinterface.rs	`83.14% <ø> (ø)`
lightning/src/chain/onchaintx.rs	`95.17% <0.00%> (-0.21%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.07% <0.00%> (-0.07%)`	⬇️

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-12-06T19:11:31Z

I had something similar in a previous iteration and @devrandom thought otherwise #1867 (comment).

TheBlueMatt · 2022-12-06T19:21:23Z

Added a note that the "instance" here includes calling again if the object was created by read_chan_signer. I don't buy that we should just say "LDK calls this more than once", because it precisely does not, and saying that it can call this more than once but users need to somehow handle that and ignore the new state is even more confusing - if there's multiple instances of a given signer now you have to have some shared state across them.

wpaulino · 2022-12-08T18:51:26Z

+	///
+	/// This data is static, and will never change for a channel once set. For a given [`BaseSign`]
+	/// instance, LDK will call this method exactly once - either immediately after construction
+	/// (including if done via [`KeysInterface::read_chan_signer`]) or when the funding information


We don't ever call it immediately after read_chan_signer, it either must have been called in the past and was serialized, or it will be called once funding occurs.

Oops, lol, right.

wpaulino

LGTM after squash.

devrandom · 2022-12-09T11:10:23Z

I'm OK with the doc in this PR.

however, I have some thoughts about the BaseSign docs in general, which may affect the thinking here.

when a dev is looking at the BaseSign docs, it seems likely they are actually trying to implement their own signer. if so, the docs should guide the dev towards best practices, which includes making the signer properly enforcing. as an example, one consequence of the signer being enforcing is the need for signer persistence.

so if that's the most likely use case that the docs should focus on, then the docs should probably be adjusted to address it, rather than implicitly focusing on how to use InMemorySigner.

TheBlueMatt · 2022-12-12T03:31:10Z

I don't think the wording here is wrong given that, either, though? I suppose, indeed, we should include a little more guidance on enforcement (or, honestly, just link to VLS, cause its a ton of work lol), but I also think someone may implement the sign trait with a goal of using different keys, learning when a transaction is being signed, or just doing naive policy checks (like closing transaction pays to the right place).

TheBlueMatt · 2022-12-12T21:33:57Z

Will address #1892 (comment) here

Its very confusing to say that LDK will call `provide_channel_parameters` more than once - its true for a channel, but not for a given instance. Instead, phrase the docs with reference to a specific instance, which is much clearer.

03de059 appeared to revert updated docs due to a rebase error. This reverts the docs on `generate_channel_keys` to the state they were in prior to that commit, with one additional doc.

TheBlueMatt · 2022-12-13T21:42:30Z

Rebased and did so.

TheBlueMatt · 2022-12-06T18:17:47Z

Its very confusing to say that LDK will call
provide_channel_parameters more than once - its true for a channel, but not for a given instance. Instead, phrase the docs with reference to a specific instance, which is much clearer.

codecov-commenter · 2022-12-06T19:03:50Z

Codecov Report

Base: 90.77% // Head: 90.76% // Decreases project coverage by -0.01%⚠️

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

Additional details and impacted files

@@ Coverage Diff @@## main #1903 +/- ##
==========================================
- Coverage 90.77% 90.76% -0.02% 
==========================================
Files 94 94 Lines 49603 49603 Branches 49603 49603 ==========================================
- Hits 45025 45020 -5 - Misses 4578 4583 +5

Impacted Files	Coverage Δ
lightning/src/chain/keysinterface.rs	`83.14% <ø> (ø)`
lightning/src/chain/onchaintx.rs	`95.17% <0.00%> (-0.21%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.07% <0.00%> (-0.07%)`	⬇️

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-12-06T19:11:31Z

I had something similar in a previous iteration and @devrandom thought otherwise #1867 (comment).

TheBlueMatt · 2022-12-06T19:21:23Z

Added a note that the "instance" here includes calling again if the object was created by read_chan_signer. I don't buy that we should just say "LDK calls this more than once", because it precisely does not, and saying that it can call this more than once but users need to somehow handle that and ignore the new state is even more confusing - if there's multiple instances of a given signer now you have to have some shared state across them.

wpaulino · 2022-12-08T18:51:26Z

+	///
+	/// This data is static, and will never change for a channel once set. For a given [`BaseSign`]
+	/// instance, LDK will call this method exactly once - either immediately after construction
+	/// (including if done via [`KeysInterface::read_chan_signer`]) or when the funding information


We don't ever call it immediately after read_chan_signer, it either must have been called in the past and was serialized, or it will be called once funding occurs.

Oops, lol, right.

wpaulino

LGTM after squash.

devrandom · 2022-12-09T11:10:23Z

I'm OK with the doc in this PR.

however, I have some thoughts about the BaseSign docs in general, which may affect the thinking here.

when a dev is looking at the BaseSign docs, it seems likely they are actually trying to implement their own signer. if so, the docs should guide the dev towards best practices, which includes making the signer properly enforcing. as an example, one consequence of the signer being enforcing is the need for signer persistence.

so if that's the most likely use case that the docs should focus on, then the docs should probably be adjusted to address it, rather than implicitly focusing on how to use InMemorySigner.

TheBlueMatt · 2022-12-12T03:31:10Z

I don't think the wording here is wrong given that, either, though? I suppose, indeed, we should include a little more guidance on enforcement (or, honestly, just link to VLS, cause its a ton of work lol), but I also think someone may implement the sign trait with a goal of using different keys, learning when a transaction is being signed, or just doing naive policy checks (like closing transaction pays to the right place).

TheBlueMatt · 2022-12-12T21:33:57Z

Will address #1892 (comment) here

Its very confusing to say that LDK will call `provide_channel_parameters` more than once - its true for a channel, but not for a given instance. Instead, phrase the docs with reference to a specific instance, which is much clearer.

03de059 appeared to revert updated docs due to a rebase error. This reverts the docs on `generate_channel_keys` to the state they were in prior to that commit, with one additional doc.

TheBlueMatt · 2022-12-13T21:42:30Z

Rebased and did so.

TheBlueMatt · 2022-12-06T18:17:47Z

Its very confusing to say that LDK will call
provide_channel_parameters more than once - its true for a channel, but not for a given instance. Instead, phrase the docs with reference to a specific instance, which is much clearer.

codecov-commenter · 2022-12-06T19:03:50Z

Codecov Report

Base: 90.77% // Head: 90.76% // Decreases project coverage by -0.01%⚠️

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

Additional details and impacted files

@@ Coverage Diff @@## main #1903 +/- ##
==========================================
- Coverage 90.77% 90.76% -0.02% 
==========================================
Files 94 94 Lines 49603 49603 Branches 49603 49603 ==========================================
- Hits 45025 45020 -5 - Misses 4578 4583 +5

Impacted Files	Coverage Δ
lightning/src/chain/keysinterface.rs	`83.14% <ø> (ø)`
lightning/src/chain/onchaintx.rs	`95.17% <0.00%> (-0.21%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.07% <0.00%> (-0.07%)`	⬇️

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-12-06T19:11:31Z

I had something similar in a previous iteration and @devrandom thought otherwise #1867 (comment).

TheBlueMatt · 2022-12-06T19:21:23Z

Added a note that the "instance" here includes calling again if the object was created by read_chan_signer. I don't buy that we should just say "LDK calls this more than once", because it precisely does not, and saying that it can call this more than once but users need to somehow handle that and ignore the new state is even more confusing - if there's multiple instances of a given signer now you have to have some shared state across them.

wpaulino · 2022-12-08T18:51:26Z

+	///
+	/// This data is static, and will never change for a channel once set. For a given [`BaseSign`]
+	/// instance, LDK will call this method exactly once - either immediately after construction
+	/// (including if done via [`KeysInterface::read_chan_signer`]) or when the funding information


We don't ever call it immediately after read_chan_signer, it either must have been called in the past and was serialized, or it will be called once funding occurs.

Oops, lol, right.

wpaulino

LGTM after squash.

devrandom · 2022-12-09T11:10:23Z

I'm OK with the doc in this PR.

however, I have some thoughts about the BaseSign docs in general, which may affect the thinking here.

when a dev is looking at the BaseSign docs, it seems likely they are actually trying to implement their own signer. if so, the docs should guide the dev towards best practices, which includes making the signer properly enforcing. as an example, one consequence of the signer being enforcing is the need for signer persistence.

so if that's the most likely use case that the docs should focus on, then the docs should probably be adjusted to address it, rather than implicitly focusing on how to use InMemorySigner.

TheBlueMatt · 2022-12-12T03:31:10Z

I don't think the wording here is wrong given that, either, though? I suppose, indeed, we should include a little more guidance on enforcement (or, honestly, just link to VLS, cause its a ton of work lol), but I also think someone may implement the sign trait with a goal of using different keys, learning when a transaction is being signed, or just doing naive policy checks (like closing transaction pays to the right place).

TheBlueMatt · 2022-12-12T21:33:57Z

Will address #1892 (comment) here

Its very confusing to say that LDK will call `provide_channel_parameters` more than once - its true for a channel, but not for a given instance. Instead, phrase the docs with reference to a specific instance, which is much clearer.

03de059 appeared to revert updated docs due to a rebase error. This reverts the docs on `generate_channel_keys` to the state they were in prior to that commit, with one additional doc.

TheBlueMatt · 2022-12-13T21:42:30Z

Rebased and did so.

TheBlueMatt · 2022-12-06T18:17:47Z

Its very confusing to say that LDK will call
provide_channel_parameters more than once - its true for a channel, but not for a given instance. Instead, phrase the docs with reference to a specific instance, which is much clearer.

codecov-commenter · 2022-12-06T19:03:50Z

Codecov Report

Base: 90.77% // Head: 90.76% // Decreases project coverage by -0.01%⚠️

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

Additional details and impacted files

@@ Coverage Diff @@## main #1903 +/- ##
==========================================
- Coverage 90.77% 90.76% -0.02% 
==========================================
Files 94 94 Lines 49603 49603 Branches 49603 49603 ==========================================
- Hits 45025 45020 -5 - Misses 4578 4583 +5

Impacted Files	Coverage Δ
lightning/src/chain/keysinterface.rs	`83.14% <ø> (ø)`
lightning/src/chain/onchaintx.rs	`95.17% <0.00%> (-0.21%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.07% <0.00%> (-0.07%)`	⬇️

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-12-06T19:11:31Z

I had something similar in a previous iteration and @devrandom thought otherwise #1867 (comment).

TheBlueMatt · 2022-12-06T19:21:23Z

Added a note that the "instance" here includes calling again if the object was created by read_chan_signer. I don't buy that we should just say "LDK calls this more than once", because it precisely does not, and saying that it can call this more than once but users need to somehow handle that and ignore the new state is even more confusing - if there's multiple instances of a given signer now you have to have some shared state across them.

wpaulino · 2022-12-08T18:51:26Z

+	///
+	/// This data is static, and will never change for a channel once set. For a given [`BaseSign`]
+	/// instance, LDK will call this method exactly once - either immediately after construction
+	/// (including if done via [`KeysInterface::read_chan_signer`]) or when the funding information


We don't ever call it immediately after read_chan_signer, it either must have been called in the past and was serialized, or it will be called once funding occurs.

Oops, lol, right.

wpaulino

LGTM after squash.

devrandom · 2022-12-09T11:10:23Z

I'm OK with the doc in this PR.

however, I have some thoughts about the BaseSign docs in general, which may affect the thinking here.

when a dev is looking at the BaseSign docs, it seems likely they are actually trying to implement their own signer. if so, the docs should guide the dev towards best practices, which includes making the signer properly enforcing. as an example, one consequence of the signer being enforcing is the need for signer persistence.

so if that's the most likely use case that the docs should focus on, then the docs should probably be adjusted to address it, rather than implicitly focusing on how to use InMemorySigner.

TheBlueMatt · 2022-12-12T03:31:10Z

I don't think the wording here is wrong given that, either, though? I suppose, indeed, we should include a little more guidance on enforcement (or, honestly, just link to VLS, cause its a ton of work lol), but I also think someone may implement the sign trait with a goal of using different keys, learning when a transaction is being signed, or just doing naive policy checks (like closing transaction pays to the right place).

TheBlueMatt · 2022-12-12T21:33:57Z

Will address #1892 (comment) here

Its very confusing to say that LDK will call `provide_channel_parameters` more than once - its true for a channel, but not for a given instance. Instead, phrase the docs with reference to a specific instance, which is much clearer.

03de059 appeared to revert updated docs due to a rebase error. This reverts the docs on `generate_channel_keys` to the state they were in prior to that commit, with one additional doc.

TheBlueMatt · 2022-12-13T21:42:30Z

Rebased and did so.

TheBlueMatt requested a review from wpaulino December 6, 2022 18:17

TheBlueMatt force-pushed the 2022-12-1867-followups branch from 94831d8 to ec6fabdCompare December 6, 2022 19:20

TheBlueMatt mentioned this pull request Dec 6, 2022
Re-derive signers instead of persisting them #1867
Merged

wpaulino reviewed Dec 8, 2022
View reviewed changes

TheBlueMatt added the Seeking Code Review label Dec 8, 2022

wpaulino reviewed Dec 8, 2022
View reviewed changes

TheBlueMatt force-pushed the 2022-12-1867-followups branch from 0afc89a to 78f6fb2Compare December 12, 2022 03:31

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

TheBlueMatt added 2 commits December 13, 2022 21:31

Clarify docs on provide_channel_parameters
6ec23aa
Its very confusing to say that LDK will call `provide_channel_parameters` more than once - its true for a channel, but not for a given instance. Instead, phrase the docs with reference to a specific instance, which is much clearer.

Correct docs on generate_channel_keys
2059190
03de059 appeared to revert updated docs due to a rebase error. This reverts the docs on `generate_channel_keys` to the state they were in prior to that commit, with one additional doc.

TheBlueMatt force-pushed the 2022-12-1867-followups branch from 78f6fb2 to 2059190Compare December 13, 2022 21:42

wpaulino approved these changes Dec 13, 2022
View reviewed changes

devrandom approved these changes Dec 14, 2022
View reviewed changes

TheBlueMatt merged commit 08d296a into lightningdevkit:mainDec 16, 2022