Clean up docs in keysinterface.rs

[tnull]

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]

"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.

[tnull]

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]

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]

Rebased after #1867 got merged.

[TheBlueMatt]

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

[tnull]

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]

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.

[tnull]

I tend to agree, a change in this direction was however suggested in #1892 (comment).

@ariard, are you fine with me reverting this?

[tnull]

Rebased again after #1825 got merged.

[TheBlueMatt]

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

[tnull]

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]

Squashed again with requested changes.

[wpaulino]

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

[tnull]

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]

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.

[wpaulino]

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

[TheBlueMatt]

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

[TheBlueMatt]

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.