Skip to content

Simplify callback logic to returning raw coordinates #201

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 6, 2020

Conversation

elichai
Copy link
Member

@elichai elichai commented Mar 1, 2020

As requested by @real-or-random in #196 (comment)
this replaces the current logic where the callback is run by the C code to using a callback that just returns the raw coordinates and then the user's callback is executed in the rust side where panics are "fine" and no need to catch them or return an error.

Upsides:

  1. Less code.
  2. Less relying on rust internals which ease the bar of reviewing(less need to dive into rust's semantics)
  3. No need to return a Result anymore.
  4. 1 safe function no matter std or no-std.

Downsides:

  1. Secrets are even at more places at the stack then they already are (right now we don't really try to mitigate against this, especially as long as Clear sensitive memory without getting optimized out bitcoin-core/secp256k1#636 isn't merged).
  2. A bit less straight forward logic wise IMO.

FWIW, I tested both before and after this change with Miri on my c2rust branch(https://github.com/elichai/rust-secp256k1/tree/c2rust)
and they both pass.

@elichai
Copy link
Member Author

elichai commented Mar 1, 2020

FWIW, I tested both before and after this change with Miri on my c2rust branch(https://github.com/elichai/rust-secp256k1/tree/c2rust)
and they both pass.

You can run it yourself by doing:

rustup toolchain install nightly --allow-downgrade -c miri rust-src
cargo +nightly miri setup
git clone https://github.com/elichai/rust-secp256k1.git --branch c2rust
cd rust-secp256k1
cargo +nightly miri test --features=c2rust -- -- ecdh 

and then you can cherry-pick this commit and fix merge problems or apply this diff: https://gist.github.com/elichai/8414db8f410801d14b6479255e0f098f
and run cargo +nightly miri test --features=c2rust -- -- ecdh again

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK. This is a million times easier to understand.

cc @real-or-random what do you think?

Copy link
Collaborator

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

ACK

@@ -137,35 +109,17 @@ impl SharedSecret {
ptr::null_mut(),
)
};
debug_assert_eq!(res, 1); // The default `secp256k1_ecdh_hash_function_default` should always return 1.
// The default `secp256k1_ecdh_hash_function_default` should always return 1.
// and the scalar was verified to be valid(0 > scalar > group_order) via the type system
Copy link
Collaborator

Choose a reason for hiding this comment

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

"via the type system" confused me for a minute. Yes, it's a valid scalar, otherwise it wouldn't have type SecretKey. But our code ensures this, not the type system. (Totally okay to merge like this, just wanted to note this.)

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fair to say the "type system" is in charge of ensuring validity of data, if there is a type that the API prevents you from constructing in an invalid way.

@apoelstra
Copy link
Member

Travis passed, except emscripten.

@apoelstra apoelstra merged commit 86751b2 into rust-bitcoin:master Apr 6, 2020
@elichai
Copy link
Member Author

elichai commented Apr 7, 2020

I did not realize this was merged :) we should remember this is a breaking change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants