De-generify Sign methods #861

devrandom · 2021-04-01T08:49:07Z

This allows all methods of Sign (herein renamed to BaseSign) to be non-generic, which allows it to have a vtable and be a dyn object.

In general, the following can prevent a trait for being used with dyn:

any of the trait methods is generic
the trait requires Sized, directly or indirectly (e.g. via Clone)

codecov · 2021-04-01T09:14:09Z

Codecov Report

Merging #861 (0821fb3) into main (e23c270) will increase coverage by 0.29%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #861      +/-   ##
==========================================
+ Coverage   90.60%   90.89%   +0.29%     
==========================================
  Files          51       51              
  Lines       27295    28351    +1056     
==========================================
+ Hits        24730    25769    +1039     
- Misses       2565     2582      +17

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	`87.32% <ø> (ø)`
lightning/src/ln/functional_tests.rs	`97.00% <ø> (+0.25%)`	⬆️
lightning-persister/src/lib.rs	`97.31% <100.00%> (+1.69%)`	⬆️
lightning/src/chain/keysinterface.rs	`93.62% <100.00%> (+0.02%)`	⬆️
lightning/src/util/enforcing_trait_impls.rs	`90.09% <100.00%> (ø)`
lightning/src/routing/router.rs	`96.87% <0.00%> (-0.10%)`	⬇️

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 e23c270...0821fb3. Read the comment docs.

devrandom · 2021-04-01T09:29:14Z

Hmm... not sure why the router benchmarks are failing. They are also failing for me in master.

Oh, they are not required.

lightning/src/util/ser.rs

TheBlueMatt

Hmmmmm, is there a way to avoid the dropping of Sign: Clone? I'm not sure there is a way to avoid requiring ChannelMonitor: Clone in the Java bindings (as there is no other way to give java a reference to something in Rust that is only valid for the scope of a function call, which we require for ChannelMonitor), I just haven't spent the time to figure out if I want to upstream that or not :/.

If its really required, maybe there's some other clever way around it - obviously the "Rust-y" way of doing the dyn would be to enum a wrapper, though it requires a few layers of wrappers so may be more challenging here. Or maybe there's a way to have a dynamic clone-but-return-the-boxed-one thing that I'm not aware of (we could always define our own trait for that).

enum DynamicSigner {
    A(SignerImplA), B(SignerImplB)
}
impl Sign for DynamicSigner {
  fn f(&self) { match self { A(a) => a.f(), B(b) -> b.f() } }
}

lightning/src/util/ser.rs

lightning/src/ln/onion_utils.rs

lightning/src/util/ser_macros.rs

lightning/src/util/ser.rs

devrandom · 2021-04-01T21:40:02Z

Hmmmmm, is there a way to avoid the dropping of Sign: Clone? I'm not sure there is a way to avoid requiring ChannelMonitor: Clone in the Java bindings (as there is no other way to give java a reference to something in Rust that is only valid for the scope of a function call, which we require for ChannelMonitor), I just haven't spent the time to figure out if I want to upstream that or not :/.

~~Might be able to impl Clone for Box<Signer>. Would it be acceptable to Box it in ChannelMonitor? I assume there's no memory allocation concerns because these are somewhat long lived.~~

Decided this is not needed, see BaseSign instead.

TheBlueMatt · 2021-04-01T21:45:48Z

Might be able to impl Clone for Box. Would it be acceptable to Box it in ChannelMonitor? I assume there's no memory allocation concerns because these are somewhat long lived.

Correct, if you are using a Box<Signer> that work work, but I think that would be hard, too, but I haven't ever dug into that too deeply.

TheBlueMatt · 2021-04-05T21:24:04Z

Heh, sometimes rust is just really stupid, BaseSign looks good, though, can you add docs for it? I think this PR otherwise looks basically good, mod #861 (comment)

lightning/src/util/ser.rs

TheBlueMatt

Looks good, just tiny stuff, really. In general, best to word-wrap your commit messages to ~80 chars.

lightning/src/util/ser.rs

lightning/src/util/ser_macros.rs

lightning-persister/src/lib.rs

ariard

Sounds good to me, minor comments

ariard · 2021-04-08T00:40:12Z

lightning/src/ln/msgs.rs

+					1 +
+						1 + 1 + high_zero_bytes_dropped_length(self.amt_to_forward) +
+						1 + 1 + high_zero_bytes_dropped_length(self.outgoing_cltv_value as u64) +
+						1 + 1 + 8 // short_channel_id


What's the variance there, smallest 17 bytes, biggest 27 bytes ?

Theoretically, yes, but in practice outgoing_cltv_value needs four bytes on mainnet.

ariard · 2021-04-08T00:45:51Z

lightning/src/ln/msgs.rs

+							None => 0,
+							Some(payment_data) =>
+								1 + 1 + high_zero_bytes_dropped_length(payment_data.total_msat) +
+									32 // payment_secret


Code-style, what do you think about ?

diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs index 5e65c9ec..477474d0 100644 --- a/lightning/src/ln/msgs.rs +++ b/lightning/src/ln/msgs.rs @@ -909,16 +909,11 @@ mod fuzzy_internal_msgs { 1 + 1 + 1 + high_zero_bytes_dropped_length(self.amt_to_forward) + 1 + 1 + high_zero_bytes_dropped_length(self.outgoing_cltv_value as u64) + - match payment_data { - None => 0, - Some(payment_data) => - 1 + 1 + high_zero_bytes_dropped_length(payment_data.total_msat) + - 32 // payment_secret - } + if payment_data.is_none() { 0 } + else { 1 + 1 + high_zero_bytes_dropped_length(payment_data.total_msat) + 32 } // payment_secret } } } - } pub struct DecodedOnionErrorPacket {

Well, payment_data would need an unwrap in the else branch, which I usually try to avoid if I need to do something in both some/none cases. Just aesthetics, so happy to change it if LDK is standardizing on the if/else form.

lightning/src/util/ser.rs

jkczyz · 2021-04-08T06:11:43Z

lightning/src/chain/keysinterface.rs

-	secp_ctx: Secp256k1<secp256k1::SignOnly>,
+	secp_ctx: Secp256k1<secp256k1::All>,


Was this change deliberate? It doesn't seem to go with the commit message.

Yes. I'll add to the commit message.

It just doesn't seem worth it to deal with both kinds of contexts separately, given that the caller might as well have an All handy to be able to supply both for different calls.

I think it's also not possible to pass in an All when Signing is expected without generics, which we are trying to get away from here.

jkczyz · 2021-04-08T06:40:44Z

lightning/src/util/ser.rs

+	pub fn size_hint(&mut self, size: usize) {
+		self.0.reserve(size)
+	}


I was looking into how reserve is implemented. It seems it increases the capacity to the max of either the desired capacity or double the current capacity. This makes me wonder how useful size_hint really is.

I suppose we may save some allocations if the writer has 0 capacity, but I'm wondering if the same thing can be accomplished while defining our public interfaces in terms of std::io::Write instead of Writer. For instance, if a message knows it size, we could do something like:

Writer::with_capacity(message.size_hint());

Rather than having the message call size_hint on Writer. Thoughts?

We could do reserve_exact.

But I am guessing that the caller almost always calls these with a zero sized writer, otherwise it's kind of ineffective. If this is called with a non-zero sized writer (e.g. if this object is a field in a larger struct), then, yes, the size_hint mechanism won't be very effective and we should think about moving to something like with_capacity.

Looking at this further, it looks like size_hint is called when serializing sub-structs quite a bit. I wonder how effective it really is for that purpose, given what you say, and also given that the enclosing object would have to estimate the size without the sub-object that also has a hint (otherwise it would be double counting).

Maybe @TheBlueMatt has more insight about how size_hint is meant to be used.

Hmm, in general it probably doesn't make sense to use it for sub-structs. It's mostly intended for where we know the exact size, which we generally do in all our P2P message sending, avoiding copying the data a few times. Luckily, the max Jeff referenced means we can't really do worse than the "normal" behavior with it, though.

As for putting it in messages vs calling it on a Writer, I certainly don't care which we end to with, but it should only be one or the other, and this PR is already nontrivial in size.

Hmm, in general it probably doesn't make sense to use it for sub-structs.

It seems to be getting called for sub-structs, given that it's called on non-empty Writer instances during the tests. Should I open an issue to look into that?

@TheBlueMatt Not sure I understand how that would look with regards to a ChannelMonitor serializing its parameterized Sign.

Right, I guess there's a few options - take a reference to the KeysManager and call KeysManager::serialize_signer(sign: &Sign). Or we could go with a straightforward Sign::serialize() -> Vec<u8> only.

@devrandom Could DynSigner also have a field storing something like FnOnce that downcasts the boxed signer and calls write? Then DynSigner could implement Writeable by calling this. The instantiator of DynSigner would need to give this function, but presumably that is possible since they are also giving boxed signer and thus know what type to downcast to.

Yes, that works. There are actually more options, since a generic Write can be converted to a dyn Write inside DynSigner.

But in any case, moving Writeable from BaseSign to Sign is definitely a valid approach. I am not worried about the sunk cost of having rewritten the Writeable trait, happy to back that out. Should decide whether or not to do that based on whether we actually expect some advantage from being able to write to arbitrary destinations without going via a Vec intermediary representation.

Here's a branch with the above implemented: https://github.com/rust-bitcoin/rust-lightning/compare/main...lightning-signer:degenerify1?expand=1

Yes, that works.

Huh! I guess when the limits the language is applying are arbitrary, the workarounds are arbitrary too :).

That looks good to me, too, what do you think @jkczyz?

Huh! I guess when the limits the language is applying are arbitrary, the workarounds are arbitrary too :).

That looks good to me, too, what do you think @jkczyz?

Works for me! Would be nice to move Writeable off to where it is needed rather than making it a sub-trait, if that's possible. Wasn't quite clear if the workaround allowed for that. But either way seems clean enough.

I am not worried about the sunk cost of having rewritten the Writeable trait, happy to back that out.

@devrandom Regarding Writer trait, I do like making that a struct and using std::io::Write, but that is no longer applicable to this PR and can come at some later time, as you said. I think you meant Writer here, at least.

lightning/src/chain/keysinterface.rs

devrandom · 2021-04-08T11:16:58Z

This is ready for another look.

The generic methods prevent Sign from being a dyn object. Use Secp256k1<All> as part of removing generics from Secp256k1 contexts passed into Sign methods.

Clone requires Sized, which prevents Sign from being a dyn object.

devrandom · 2021-04-09T18:36:38Z

OK, the changes to Writer / Writeable have been taken out, and Writeable is now part of Sign instead of BaseSign. This was as a result of the discussion in #861 (comment).

This is ready for another review.

ariard · 2021-04-09T22:35:02Z

Code Review ACK 0821fb3

jkczyz · 2021-04-09T23:05:27Z

lightning/src/chain/keysinterface.rs

+#[test]
+pub fn dyn_sign() {
+	let _signer: Box<dyn BaseSign>;
+}


nit: add newline

devrandom requested review from TheBlueMatt and jkczyz April 1, 2021 08:49

devrandom force-pushed the degenerify branch 2 times, most recently from f0df509 to 13c733e Compare April 1, 2021 08:58

devrandom force-pushed the degenerify branch 2 times, most recently from d36f3a8 to 198f27a Compare April 1, 2021 11:11

devrandom changed the title ~~De-generify Writer~~ De-generify Sign methods Apr 1, 2021

jkczyz reviewed Apr 1, 2021

View reviewed changes

lightning/src/util/ser.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed Apr 1, 2021

View reviewed changes

devrandom force-pushed the degenerify branch 2 times, most recently from 96f36e4 to 97e0704 Compare April 3, 2021 20:12

devrandom mentioned this pull request Apr 4, 2021

Draft: Dynamic Signer and KeysManager valentinewallace/ldk-sample#1

Closed

devrandom force-pushed the degenerify branch from c86edda to 4d20627 Compare April 5, 2021 09:13

TheBlueMatt added this to the 0.0.14 milestone Apr 5, 2021

devrandom force-pushed the degenerify branch 3 times, most recently from 1f5b1ab to 4290706 Compare April 6, 2021 19:27

TheBlueMatt reviewed Apr 7, 2021

View reviewed changes

lightning/src/util/ser.rs Outdated Show resolved Hide resolved

lightning/src/util/ser.rs Outdated Show resolved Hide resolved

devrandom force-pushed the degenerify branch 3 times, most recently from 1b6e001 to 216f35c Compare April 7, 2021 08:19

TheBlueMatt reviewed Apr 7, 2021

View reviewed changes

ariard reviewed Apr 8, 2021

View reviewed changes

jkczyz reviewed Apr 8, 2021

View reviewed changes

devrandom force-pushed the degenerify branch from 216f35c to 4aa6102 Compare April 8, 2021 09:48

devrandom force-pushed the degenerify branch from 4aa6102 to 1b289ed Compare April 8, 2021 11:12

TheBlueMatt approved these changes Apr 8, 2021

View reviewed changes

devrandom added 4 commits April 9, 2021 11:19

Eliminate unnecessary generics from Sign

b69ae07

The generic methods prevent Sign from being a dyn object. Use Secp256k1<All> as part of removing generics from Secp256k1 contexts passed into Sign methods.

Separate Clone from Sign

db02871

Clone requires Sized, which prevents Sign from being a dyn object.

Move Writeable from BaseSign to Sign

14e094a

Test that BaseSign can have a vtable

0821fb3

devrandom force-pushed the degenerify branch from 1b289ed to 0821fb3 Compare April 9, 2021 18:34

jkczyz approved these changes Apr 9, 2021

View reviewed changes

lightning/src/chain/keysinterface.rs

#[test]

pub fn dyn_sign() {

let _signer: Box<dyn BaseSign>;

}

Copy link

Contributor

jkczyz Apr 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add newline

TheBlueMatt merged commit dba0709 into lightningdevkit:main Apr 9, 2021

TheBlueMatt mentioned this pull request Apr 9, 2021

Add missing EOF newline #873

Merged

		secp_ctx: Secp256k1<secp256k1::SignOnly>,
		secp_ctx: Secp256k1<secp256k1::All>,

De-generify Sign methods #861

De-generify Sign methods #861

Uh oh!

Conversation

devrandom commented Apr 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

devrandom commented Apr 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

devrandom commented Apr 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Apr 1, 2021

Uh oh!

TheBlueMatt commented Apr 5, 2021

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ariard left a comment

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!

Choose a reason for hiding this comment

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!

devrandom Apr 8, 2021 • 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!

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!

devrandom Apr 9, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

devrandom commented Apr 1, 2021 •

edited

Loading

codecov bot commented Apr 1, 2021 •

edited

Loading

devrandom commented Apr 1, 2021 •

edited

Loading

devrandom commented Apr 1, 2021 •

edited

Loading

devrandom Apr 8, 2021 •

edited

Loading

devrandom Apr 9, 2021 •

edited

Loading