Rename ChannelKeys -> Sign and generic it consistently #799

TheBlueMatt · 2021-02-16T22:10:04Z

Based on #789, this has really upset my OCD for some time, so needs fixing.

The ChannelKeys object really isn't about keys at all anymore,
its all about signing. At the same time, we rename the type aliases
used in traits from both ChanKeySigner and Keys to just
ChanSigner

codecov · 2021-02-16T22:22:10Z

Codecov Report

Merging #799 (4b6f0a3) into main (ee68ffa) will increase coverage by 0.02%.
The diff coverage is 95.34%.

@@            Coverage Diff             @@
##             main     #799      +/-   ##
==========================================
+ Coverage   90.80%   90.82%   +0.02%     
==========================================
  Files          45       45              
  Lines       24548    24547       -1     
==========================================
+ Hits        22290    22294       +4     
+ Misses       2258     2253       -5

Impacted Files	Coverage Δ
lightning-net-tokio/src/lib.rs	`76.70% <ø> (ø)`
lightning/src/chain/mod.rs	`100.00% <ø> (ø)`
lightning/src/ln/chan_utils.rs	`97.34% <ø> (ø)`
lightning/src/util/enforcing_trait_impls.rs	`90.09% <75.00%> (ø)`
lightning/src/chain/keysinterface.rs	`93.36% <80.00%> (ø)`
lightning/src/util/test_utils.rs	`83.16% <82.35%> (ø)`
lightning/src/ln/onchaintx.rs	`94.10% <94.11%> (-0.02%)`	⬇️
lightning/src/ln/channel.rs	`87.70% <98.61%> (ø)`
lightning-persister/src/lib.rs	`92.23% <100.00%> (ø)`
lightning/src/chain/chainmonitor.rs	`94.11% <100.00%> (ø)`
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee68ffa...4b6f0a3. Read the comment docs.

jkczyz · 2021-02-17T07:38:01Z

lightning/src/chain/mod.rs

-	type Keys: ChannelKeys;
+	type ChanSigner: ChannelSigner;


Not sure if you're warming to the using verbs for traits and nouns for types, but consider trait Sign and type ChannelSigner: Sign for consistency. Then you could dispense with abbreviated ChanSigner.

Considering the options in more detail:

N Trait Type Type Param

1 ChannelSigner InMemoryChannelSigner ChanSigner

2 Sign InMemoryChannelSigner ChannelSigner

3 ChannelSign InMemoryChannelSigner ChannelSigner

4 Sign InMemorySigner Signer

So 1 is this PR in its current state, 2 is what @jkczyz proposed. 3 seems to be a more consistent version of 2 which doesn't drop the word Channel on the trait. We could go for 4 given that signing at the channel level is such a fundamental aspect of a Lightning node, so might as well be a bit less verbose.

I prefer 4 somewhat, but I don't feel strongly.

Hmm, the problem I have with Sign is it doesn't really fully capture what's going on - it's a channel-specific thing that signs. If we moved it to channel::Sign it'd be more like chain::Watch but I'm not sure it makes sense there.

I tend to agree with @devrandom in that "channel" is fundamental so could be dropped. I'd favor (4) but with slight variation.

IMO, it is redundant when used as the type parameter to Channel.

pub(super) struct Channel<ChanSigner: ChannelSigner> { } impl<ChanSigner: ChannelSigner> Channel<ChanSigner> { }

That it is for a specific channel could be made clear in the documentation and is enforced by it being a type parameter for Channel and ChannelMonitor.

However, for parameterizing types where what signing pertains to isn't clear, the type parameter could be the more specific ChannelSigner instead Signer (e.g., in Watch).

Ok, I think I did this everywhere, let me know if I missed any cases.

devrandom

ACK, but consider some variations on naming

devrandom · 2021-02-17T09:19:57Z

lightning/src/chain/mod.rs

-	type Keys: ChannelKeys;
+	type ChanSigner: ChannelSigner;


Considering the options in more detail:

N Trait Type Type Param

1 ChannelSigner InMemoryChannelSigner ChanSigner

2 Sign InMemoryChannelSigner ChannelSigner

3 ChannelSign InMemoryChannelSigner ChannelSigner

4 Sign InMemorySigner Signer

So 1 is this PR in its current state, 2 is what @jkczyz proposed. 3 seems to be a more consistent version of 2 which doesn't drop the word Channel on the trait. We could go for 4 given that signing at the channel level is such a fundamental aspect of a Lightning node, so might as well be a bit less verbose.

I prefer 4 somewhat, but I don't feel strongly.

jkczyz

I'd prefer ChannelSigner over ChanSigner as the shortened version was introduced primarily to avoid conflicting with the trait name. But that is no longer a problem. I also find the "chan" is a little too close to "chain" when reading.

lightning-persister/src/lib.rs

jkczyz · 2021-02-18T18:24:24Z

lightning-persister/src/lib.rs

@@ -130,7 +130,7 @@ impl FilesystemPersister {
 	}
 }

-impl<ChanSigner: ChannelKeys + Send + Sync> channelmonitor::Persist<ChanSigner> for FilesystemPersister {
+impl<ChanSigner: Sign + Send + Sync> channelmonitor::Persist<ChanSigner> for FilesystemPersister {


Maybe here too?

Tangentially, I wonder if this type parameter should really be moved to the functions where it is clearer. Is it required at the struct level in order to work with another struct that is parameterized by this? Or is the compiler smart enough to make the determination?

Hmm, this seems to not have any other Channel context.

As to the question about functions, I believe its required in this context because ChannelMonitor stores a signer in the struct, not just accepting it as a parameter to a function.

For the context, I was seeing the module qualifier channelmonitor and the use below parameterizing ChannelMonitor. I don't feel too strongly about this, though.

Ah. Yea, I see your point. I don't care either way now, and its more work to change it :).

jkczyz · 2021-02-18T18:34:35Z

lightning/src/chain/channelmonitor.rs

@@ -2253,7 +2253,7 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 /// transaction and losing money. This is a risk because previous channel states
 /// are toxic, so it's important that whatever channel state is persisted is
 /// kept up-to-date.
-pub trait Persist<Keys: ChannelKeys>: Send + Sync {
+pub trait Persist<Keys: Sign>: Send + Sync {


What should we do for cases where Keys was used as a type parameter? Maybe best to standardize as Signer.

Oops, my regexes missed those ones.

jkczyz · 2021-02-18T18:38:28Z

lightning/src/chain/mod.rs

@@ -69,7 +69,7 @@ pub trait Access: Send + Sync {
 /// [`PermanentFailure`]: channelmonitor/enum.ChannelMonitorUpdateErr.html#variant.PermanentFailure
 pub trait Watch: Send + Sync {
 	/// Keys needed by monitors for creating and signing transactions.
-	type Keys: ChannelKeys;
+	type ChanSigner: Sign;


Similarly here only for using an associated type vs function type parameter.

Hmm, so this doesn't make sense as a function parameter - users don't ever want to receive arbitrary ChannelMonitors with unknown signer while serializing. That said, this probably shouldn't be an associated type - a Watch<A> is a different type from a Watch<B> and a single struct could (though probably won't) implement both.

lightning/src/ln/chan_utils.rs

jkczyz · 2021-02-19T01:36:59Z

I'd prefer ChannelSigner over ChanSigner as the shortened version was introduced primarily to avoid conflicting with the trait name. But that is no longer a problem. I also find the "chan" is a little too close to "chain" when reading.

If it's not too much trouble, changing this only touches a few files mostly where "chain" is used.

The `ChannelKeys` object really isn't about keys at all anymore, its all about signing. At the same time, we rename the type aliases used in traits from both `ChanKeySigner` and `Keys` to just `Signer` (or, in contexts where Channel isnt clear, `ChanSigner`).

TheBlueMatt · 2021-02-19T21:04:14Z

Rebased.

TheBlueMatt · 2021-02-19T21:09:46Z

Also threw in bindings updates cause they are trivial.

jkczyz

Getting some errors when running genbindings.sh:

++ rustc --version --verbose
++ grep host:
+ HOST_PLATFORM='host: x86_64-apple-darwin'
+ '[' 'host: x86_64-apple-darwin' = 'host: x86_64-apple-darwin' ']'
+ sed -i '' 's/typedef LDKnative.*Import.*LDKnative.*;//g' include/lightning.h
+ CFLAGS='-Wall -Wno-nullability-completeness -pthread'
+ gcc -Wall -Wno-nullability-completeness -pthread -Wall -g -pthread demo.c target/debug/libldk.a -ldl
In file included from demo.c:2:
./include/lightning.h:1605:18: error: expected member name or ';' after declaration specifiers
   LDKHTLCUpdate 0;
   ~~~~~~~~~~~~~ ^
./include/lightning.h:1605:17: error: expected ';' at end of declaration list
   LDKHTLCUpdate 0;
                ^
                ;
./include/lightning.h:1609:16: error: expected member name or ';' after declaration specifiers
   LDKOutPoint 0;
   ~~~~~~~~~~~ ^
./include/lightning.h:1609:15: error: expected ';' at end of declaration list
   LDKOutPoint 0;
              ^
              ;
./include/lightning.h:1724:38: error: expected member name or ';' after declaration specifiers
   LDKDelayedPaymentOutputDescriptor 0;
   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
./include/lightning.h:1724:37: error: expected ';' at end of declaration list
   LDKDelayedPaymentOutputDescriptor 0;
                                    ^
                                    ;
./include/lightning.h:1728:37: error: expected member name or ';' after declaration specifiers
   LDKStaticPaymentOutputDescriptor 0;
   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
./include/lightning.h:1728:36: error: expected ';' at end of declaration list
   LDKStaticPaymentOutputDescriptor 0;
                                   ^
                                   ;
./include/lightning.h:2385:16: error: expected member name or ';' after declaration specifiers
   LDKAPIError 0;
   ~~~~~~~~~~~ ^
./include/lightning.h:2385:15: error: expected ';' at end of declaration list
   LDKAPIError 0;
              ^
              ;
./include/lightning.h:2389:35: error: expected member name or ';' after declaration specifiers
   LDKCVec_CResult_NoneAPIErrorZZ 0;
   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
./include/lightning.h:2389:34: error: expected ';' at end of declaration list
   LDKCVec_CResult_NoneAPIErrorZZ 0;
                                 ^
                                 ;
./include/lightning.h:2393:22: error: expected member name or ';' after declaration specifiers
   LDKCVec_APIErrorZ 0;
   ~~~~~~~~~~~~~~~~~ ^
./include/lightning.h:2393:21: error: expected ';' at end of declaration list
   LDKCVec_APIErrorZ 0;
                    ^
                    ;
./include/lightning.h:2397:35: error: expected member name or ';' after declaration specifiers
   LDKCVec_CResult_NoneAPIErrorZZ 0;
   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
./include/lightning.h:2397:34: error: expected ';' at end of declaration list
   LDKCVec_CResult_NoneAPIErrorZZ 0;
                                 ^
                                 ;
16 errors generated.

lightning-c-bindings/demo.cpp

TheBlueMatt · 2021-02-19T21:24:48Z

./include/lightning.h:1605:18: error: expected member name or ';' after declaration specifiers
LDKHTLCUpdate 0;

Hmm, that isnt how the header looks for me. Can you try updating your cbindgen (or maybe I need to update mine?)

jkczyz · 2021-02-19T21:35:17Z

./include/lightning.h:1605:18: error: expected member name or ';' after declaration specifiers
LDKHTLCUpdate 0;

Hmm, that isnt how the header looks for me. Can you try updating your cbindgen (or maybe I need to update mine?)

Working on 0.17.0. I was on 0.14.4.

devrandom · 2021-02-20T08:52:17Z

Perhaps also rename chan_keys in several places (but not all places, it's used for tx keys in some spots).

Similarly for Channel.holder_keys and Channel.get_keys.

Also, we might consider renaming KeysInterface / KeysManager, but should probably do it in a followup in the interest of keeping PRs small.

Or if you want, I can do all this in a followup.

ACK on current

TheBlueMatt · 2021-02-20T15:06:50Z

Squashed earlier fixup commits and added a commit to rename the missing chan_keys objects.

devrandom · 2021-02-20T15:26:50Z

ACK 4b6f0a3

jkczyz · 2021-02-22T16:36:24Z

ACK 4b6f0a3

jkczyz reviewed Feb 17, 2021

View reviewed changes

devrandom approved these changes Feb 17, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-02-chansigner-rename branch from ba6121b to 2d4d20e Compare February 18, 2021 18:08

TheBlueMatt changed the title ~~Rename ChannelKeys -> ChannelSigner and generic it consistently~~ Rename ChannelKeys -> Sign and generic it consistently Feb 18, 2021

jkczyz reviewed Feb 18, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-02-chansigner-rename branch 2 times, most recently from 40f1115 to 3298f43 Compare February 18, 2021 21:54

jkczyz approved these changes Feb 19, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-02-chansigner-rename branch from ad705c0 to 6072707 Compare February 19, 2021 02:34

TheBlueMatt added 2 commits February 19, 2021 15:54

Use signer instead of keys_storage in OnchainTx to hold Sign

20a2596

TheBlueMatt force-pushed the 2021-02-chansigner-rename branch from 6072707 to 813f117 Compare February 19, 2021 21:04

TheBlueMatt added 2 commits February 19, 2021 16:04

Change Persist's Sign from an associated type to a generic param

523fcb6

Use ChannelSigner instead of ChanSigner for type parameters

aa127f5

TheBlueMatt force-pushed the 2021-02-chansigner-rename branch from 813f117 to aa127f5 Compare February 19, 2021 21:04

jkczyz reviewed Feb 19, 2021

View reviewed changes

lightning-c-bindings/demo.cpp Outdated Show resolved Hide resolved

lightning-c-bindings/demo.cpp Outdated Show resolved Hide resolved

TheBlueMatt force-pushed the 2021-02-chansigner-rename branch from c31f30d to b0d02bb Compare February 19, 2021 21:29

TheBlueMatt added 3 commits February 20, 2021 10:06

Update auto-generated bindings

aa3f841

Update C++ bindings demo with new struct naming

fc416d6

Further rename chan_keys variables to signers

4b6f0a3

TheBlueMatt force-pushed the 2021-02-chansigner-rename branch from b0d02bb to 4b6f0a3 Compare February 20, 2021 15:06

TheBlueMatt merged commit 94bb0c9 into lightningdevkit:main Feb 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename ChannelKeys -> Sign and generic it consistently #799

Rename ChannelKeys -> Sign and generic it consistently #799

TheBlueMatt commented Feb 16, 2021

codecov bot commented Feb 16, 2021 •

edited

Loading

jkczyz Feb 17, 2021

devrandom Feb 17, 2021

TheBlueMatt Feb 17, 2021

jkczyz Feb 17, 2021

TheBlueMatt Feb 18, 2021

devrandom left a comment

devrandom Feb 17, 2021

jkczyz left a comment

jkczyz Feb 18, 2021

TheBlueMatt Feb 18, 2021

jkczyz Feb 19, 2021

TheBlueMatt Feb 19, 2021

jkczyz Feb 18, 2021

TheBlueMatt Feb 18, 2021

jkczyz Feb 18, 2021

TheBlueMatt Feb 18, 2021

jkczyz commented Feb 19, 2021

TheBlueMatt commented Feb 19, 2021

TheBlueMatt commented Feb 19, 2021

jkczyz left a comment

TheBlueMatt commented Feb 19, 2021

jkczyz commented Feb 19, 2021

devrandom commented Feb 20, 2021

TheBlueMatt commented Feb 20, 2021

devrandom commented Feb 20, 2021

jkczyz commented Feb 22, 2021

N	Trait	Type	Type Param
1	ChannelSigner	InMemoryChannelSigner	ChanSigner
2	Sign	InMemoryChannelSigner	ChannelSigner
3	ChannelSign	InMemoryChannelSigner	ChannelSigner
4	Sign	InMemorySigner	Signer

Rename ChannelKeys -> Sign and generic it consistently #799

Rename ChannelKeys -> Sign and generic it consistently #799

Conversation

TheBlueMatt commented Feb 16, 2021

codecov bot commented Feb 16, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devrandom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz commented Feb 19, 2021

TheBlueMatt commented Feb 19, 2021

TheBlueMatt commented Feb 19, 2021

jkczyz left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Feb 19, 2021

jkczyz commented Feb 19, 2021

devrandom commented Feb 20, 2021

TheBlueMatt commented Feb 20, 2021

devrandom commented Feb 20, 2021

jkczyz commented Feb 22, 2021

codecov bot commented Feb 16, 2021 •

edited

Loading