If this is good, we should do the same in secp.

Pull Request Test Coverage Report for Build 7749854763

• -22 of 129 (82.95%) changed or added relevant lines in 22 files are covered.
• 3 unchanged lines in 3 files lost coverage.
• Overall coverage decreased (-0.003%) to 84.145%

Totals
Change from base Build 7742657266:	-0.003%
Covered Lines:	19328
Relevant Lines:	22970

💛 - Coveralls

Will fix CI tomorrow, I'm hosed right now.

Concept ACK, will wait until CI fix.

Concept ACK

We should enable the trivially_copy_pass_by_ref clippy lint which enforces this. (Or maybe this PR already does, I haven't looked because you said you'd update it soon for CI :)).

I had a play with trivially_copy_pass_by_ref yesterday and all it did was tell me change millions of &self to self then after I did that clippy gave another million warnings about is_foo not being meant to take self. (I also had to configure breaking API lints to get the lint to work on pubic functions.) It also did not warn about any of the changes in this PR (IIRC).

This PR has shown that we (or at least I) don't have a single command that can be run to sanity check all the code in the the whole repo that doesn't take a whole age to run. Epic PITA.

For types with a size less than 16 bytes or so I understand this, but for 32 byte arrays it might be a pretty big performance hit to continually copy the same 32 bytes over and over on the stack instead of an 8 byte pointer.

perhaps investigating perf hits caused by this might be warranted.

ie. Network enum is a strong ACK, but I’m still torn on all the private and public keys.

Thoughts?

@junderw IIRC someone benchmarked it and the perf hit was very small. The copies are generally optimized away after inlining etc. I was thinking that it might be somewhat weird to do things like not accept references in methods called is_ and also even though perf hit is tiny, it's still more than zero. However there's I think strong argument for this: use of map becomes much more convenient since you can write .map(Foo::bar) instead of .map(|item| item.bar). This only applies to one-argument functions but having exception for multi-argument ones seems weird.

OTOH if you iterate over slices you need to use .copied or a closure. We could accept T: Borrow<Foo> but that seems too much and would probably cause troubles with type inference.

FTR This PR does not change an of instances of self/&self - that was on purpose because, as hinted at above with the lint stuff, I think the story around self is a bit more convoluted that other function parameters.

but for 32 byte arrays it might be a pretty big performance hit to continually copy the same 32 bytes over and over on the stack instead of an 8 byte pointer.

32 bytes is just 4 words (on my machine), who cares? If there are some benchmarks that prove this is slower then sure but this feels like something that the language and the compiler should be handling and the Rust convention seems to be if its Copy pass it by value. (I don't have citations off the top of my head though.)

utACK 208c2ce8aae9c43ee1d685a87df9f2b8e23534e1 but I'm really struggling with whether we want to bring this in without machine tooling to keep it in sync. It'll be a mildly annoying breaking change for all our users and I'd rather only do it once.

@tcharding I'm unsure if your diagnosis of Clippy is correct.

• If I run cargo clippy -- -W clippy::trivially_copy_pass_by_ref (I also have to -A a couple other warnings but forget that for now..) then I only get a few things about private types.
• If I then add avoid-breaking-exported-api = false to my clippy.toml, then the interesting stuff happens -- I get a lot of warnings. Some of which are suggesting that I change to_ methods from &self to self.
• I also get some warnings about to_ methods taking self, but it appears these are not the methods I just changed. They're separate, valid warnings, that clippy had been ignoring because they'd break our public API, but which appear to be legit.

Let me know if I'm wrong; there is a long list of warnings and I didn't fix them all so it's possible I'm getting mixed up reading the churn.

Thanks for looking, I'll have another play with it.

Props to you @apoelstra, you were correct. I've added a bunch more patches to this PR.

I've had enough of this for one day, will fix CI tomorrow.

Rebase only, no other changes.

This PR is totally non-urgent, can be re-done at any time.

On ice till next release.

Hey @mpbagot do you feel like checking if we need to bring any of this back to life? By that I mean do we need to do any API changes, specifically in the stabalizing crates, to pass by value. Doing so requires digging into clippy usage and is probably non-trivial to do. Only pick this up if you want to and you have capacity. Might be worth thinking about in relation to the key changes you've been doing also.

8184ddf needs rebase

8184ddf needs rebase

lol, I'd smack that bot if my arm was long enough.

Hey mpbagot do you feel like checking if we need to bring any of this back to life? By that I mean do we need to do any API changes, specifically in the stabalizing crates, to pass by value.

The only changes that I could find were block_hash() on Header, combine() on the MerkleNode types and is_valid() + is_error() on NumOpResult. If hashes is included, then hash_again() on the sha256 hash too.
That said, should Header be Copy? It's convenient to be so, but it's an 80 byte type, which makes it very expensive if the compiler ends up copying it in memory.

Doing so requires digging into clippy usage and is probably non-trivial to do.

The Clippy lint you used here (trivially_copy_pass_by_ref) doesn't seem to trigger anymore. I tried changing various functions on Copy types and enabling the lint and Clippy refused to complain.

Might be worth thinking about in relation to the key changes you've been doing also.

I'll take a look at this too.

Hey mpbagot do you feel like checking if we need to bring any of this back to life? By that I mean do we need to do any API changes, specifically in the stabalizing crates, to pass by value.

The only changes that I could find were block_hash() on Header, combine() on the MerkleNode types and is_valid() + is_error() on NumOpResult. If hashes is included, then hash_again() on the sha256 hash too. That said, should Header be Copy? It's convenient to be so, but it's an 80 byte type, which makes it very expensive if the compiler ends up copying it in memory.

I"m a bit lost, I looked a bunch of these up on master and they don't take non-Self args?

I think Header should be Copy (as it is). 80 bytes isn't thaaat much and its never getting any bigger. But I'm not an optimisation expert by any means.

Doing so requires digging into clippy usage and is probably non-trivial to do.

The Clippy lint you used here (trivially_copy_pass_by_ref) doesn't seem to trigger anymore. I tried changing various functions on Copy types and enabling the lint and Clippy refused to complain.

WIN. Thanks for investigating man.

Might be worth thinking about in relation to the key changes you've been doing also.

I'll take a look at this too.

I"m a bit lost, I looked a bunch of these up on master and they don't take non-Self args?

Is that not part of the problem here? In this PR (93194aa), you made changes to functions that took only self by ref on a Copy type. I thus assumed this was part of the issue you were wanting to solve.

The Clippy lint you used here (trivially_copy_pass_by_ref) doesn't seem to trigger anymore.

Did you enable the "api break" config parameter for Clippy? Without it it won't lint on anything in the public API.

I did not, which was an oversight. I have tested everything again now with that config parameter, and I still can't trigger the trivially_copy_pass_by_ref lint. I can trigger wrong_self_convention for is_valid + is_error, which I could not before, so the config appears to be working.