secp256k1_ecdh_hash_function must return 0 or 1

[sipa]

Originally discussed at bitcoin-core/secp256k1@4f72054#r371029012 (github fails to link to the discussion; scroll through bitcoin-core/secp256k1#710 instead).

See https://github.com/rust-bitcoin/rust-secp256k1/blob/master/src/ecdh.rs#L162, where it seems behavior relies on seeing the return value of the hash function propagated up to the signing return value.

The documentation for hash functions specifies that it should return 1 or 0, and similarly, secp256k1_ecdh will return 0 or 1.

[sipa]

cc @elichai, @gmaxwell

[elichai]

Thank you for opening the issue @sipa
I think because we "control" the upstream code here then we're currently fine.

But I already have an idea around this without a breaking change, so I'll open a PR next week and we can discuss it until the next upstream bump.

Any thoughts? @apoelstra

[apoelstra]

Nice, good find. We should definitely fix rust-secp to match the documented upstream behavior. Curious what your non-breaking solution is Elichai.

[sipa]

It seems that the correct solution would be to pass any other return data using a field in data pointer's object.

[elichai]

My original thought was across the lines that @sipa suggested.

But then I realized we can do this simpler by instead of passing a raw u8 pointer in the *unsigned char output we can pass a pointer to the SharedSecret object and just write directly into it.

here are the code changes for both ways:

1. elichai@b81efd0

2. elichai@550e74e

[real-or-random]

But then I realized we can do this simpler by instead of passing a raw u8 pointer in the *unsigned char output we can pass a pointer to the SharedSecret object and just write directly into it.

This variant wrongly assumes that the u8 buffer is the first field in the struct (and that there's no padding in front of it). edit: We could add repr(C) if we want to but feels like a hack in this case.

(Tbh, what #180 introduced is pretty complex. I'm still not convinced if it's not better to get the raw x and y data from C and handle the entire user-provided callback purely in Rust.)

[elichai]

This variant wrongly assumes that the u8 buffer is the first field in the struct (and that there's no padding in front of it). edit: We could add repr(C) if we want to but feels like a hack in this case.

(Tbh, what #180 introduced is pretty complex. I'm still not convinced if it's not better to get the raw x and y data from C and handle the entire user-provided callback purely in Rust.)

Well that depends if upstream promises to not touch the output pointer.
if no "unknown" code touches it then there's no assumption and we can easily cast that pointer back to our own type. so if upstream allows this assumption and there's no weird C rule I haven't heard of which says that strict aliasing also applies without dereferencing the pointer, then IMHO it should be fine.

[real-or-random]

This variant wrongly assumes that the u8 buffer is the first field in the struct (and that there's no padding in front of it). edit: We could add repr(C) if we want to but feels like a hack in this case.

Well that depends if upstream promises to not touch the output pointer.
if no "unknown" code touches it then there's no assumption and we can easily cast that pointer back to our own type. so if upstream allows this assumption and there's no weird C rule I haven't heard of which says that strict aliasing also applies without dereferencing the pointer, then IMHO it should be fine.

Ah I see, I understand now. I think you're right. (And anyway, no-aliasing rules don't apply to unsigned char.)

(Tbh, what #180 introduced is pretty complex. I'm still not convinced if it's not better to get the raw x and y data from C and handle the entire user-provided callback purely in Rust.)

What do people think about this? At the moment, the user gives us a Rust callback, that we wrap by creating a C callback from it. Then we call from Rust into C, which calls the C callback, which calls back into the user-provided Rust. This asks for complexity, if not for trouble. And my failure to understand the code is at least partially a result of this.

What I suggest is to write a custom pass-through callback function that simply outputs the x and y values. Then after the FFI call we're done with C business, and then we apply the user-provided hash function (in Rust).

Besides the reduced complexity, one advantage is that we don't need to deal with the case that the user-provided function panics. Would there be any drawbacks?

[elichai]

I understand your concern of complexity.
I think your suggestion seem simpler but language wise it's basically the same except the panicking part, meaning the second you're in rust code you have rust semantics no matter how you got there, the same for C.

One "problem" with your suggestion is that if your cleaning PR(bitcoin-core/secp256k1#636) will ever get merged than we introduce here an explicit stack copy of those elements that we have nothing to do about it.
although thinking about it, that's also the situation currently with returning SharedSecret from the rust closure argh. (weirdly enough for this problem C++ move and copy semantics are easier to control)

FWIW Rust->C->Rust is a thing that also happens in the rust compiler.

[real-or-random]

I think your suggestion seem simpler but language wise it's basically the same except the panicking part, meaning the second you're in rust code you have rust semantics no matter how you got there, the same for C.

I see that it's not a huge difference in the end but if it's "basically the same except the panicking part", isn't it then better because of the panicking part?

[real-or-random]

This has been resolved by https://github.com/rust-bitcoin/rust-secp256k1/pull/201/files#diff-59f47f7f8b0a5de504143abb670e06f2R94.