Implement ok_or() for CtOption<T> about subtle HOT 11 CLOSED

I think discussion of extra combinators would probably be better to split off from the Option conversion. One point to consider is that there are a lot of option combinators, but if we have an easy conversion from a CtOption to an Option we can maybe avoid implementing them.

For the example of ok_or, since Result is not constant time, I'm not sure there's any benefit to having the subtle crate reimplement all the option/result conversion combinators rather than having users do Option::from(...).ok_or_whatever(). In the specific case of the jubjub crate, I think that there's a lot of API surface using CtOption unnecessarily that would be better just using Option.

from subtle.

@daira the second one looks cleaner TBH. I can include ok_or_vartime if you think it's a good idea (which it is IMVHO). That takes out the need of using Option::from() which adds verbosity and doesn't help a lot if you're just looking for a Result since you still need to call ok_or().

from subtle.

I would be fine with just Option::from. That solves the problem even if it isn't quite as concise as possible.

from subtle.

Yeah, this part of the API kinda sucks right now. I think that the cleanest way to fix this is to do #73 and provide an Option conversion -- then your example could be written as

Option::from(ct_opt).ok_or(...)

Implementing From gives an automatic Into impl, but it might not always work to call into() because it may not be possible to infer the return type. So there might be an ergonomic benefit to also providing CtOption<T>::into_option(self) -> Option<T> { self.into() }. Then I think it would be possible to write

let s = Scalar::from_bytes(&bytes).into_option().ok_or_else(|| MyError)?;

I'm not totally sure on what the naming of into_option should be -- it would be good to be consistent with the conversion from Choice to bool.

I'm really busy right now so I can't do this myself but I'd be happy to accept a PR!

from subtle.

Hey Henry, a pleasure to work on it.

I have some doubts since Option does not implement any subtle trait obviously, and I don't think we can enforce T to be ConditionallySelectable + ConstantTimeEq.

Therefore, I can't think of any other aproach which is not something like:

impl<T> From<CtOption<T>> for Option<T> {
    ///
    #[inline]
    fn from(source: CtOption<T>) -> Option<T> {
        if source.is_some().unwrap_u8() == 1u8 {
            Option::Some(source.value)
        } else {
            None
        }
    }
}

Since we're branching here, we're not enforcing ctant_time, that's for sure, but I don't know if we really care about constant time properties here. Since this will leak whether the value is Some<T> or None (if we analyze timings) as well as an unwrap() call will do.

Another think I thought and that it's cleaner is the following:

impl<T> From<CtOption<T>> for Option<T>
where
    T: ConditionallySelectable,
{
    /// Requires to implement `ConditionallySelect` for `Option`
    #[inline]
    fn from(source: CtOption<T>) -> Option<T> {
        let mut tmp = CtOption::new(Option::default(), Choice::from(1u8));
        let tmp = tmp.conditional_assign(source, !source.is_some);
        
        // Always safe
        tmp.unwrap()
    }
}

But that would require to implement ConditionallySelectable for Option. Which Idk if it's fine or not neither if it's possible.

Do you have any other idea in mind? I tried a couple of things, but since all of the CtOption methods return a CtOption<T> it's quite difficult to do it cleaner.

from subtle.

@CPerezz Yes, because Option<T> does not have constant-time operations, it's fine to have a non-constant-time conversion from CtOption<T> to Option<T>. So something like the first snippet would work fine. It's OK to leak whether the value is present or not in the conversion, because the result of the conversion is a structure (Option<T>) that always leaks.

from subtle.

I also suggested something similar to this and #73 (as ok_or_[else_]vartime and to_option_vartime) here.

Currently we have quite a few instances of ugly code like this in librustzcash:

{
    let cv = jubjub::ExtendedPoint::from_bytes(&bytes);
    if cv.is_none().into() {
        return Err(io::Error::new(io::ErrorKind::InvalidInput, "invalid cv"));
    }
    cv.unwrap()
}

which could be just

    Option::from(jubjub::ExtendedPoint::from_bytes(&bytes))
        .ok_or(Err(io::Error::new(io::ErrorKind::InvalidInput, "invalid cv")))?

or alternatively

    jubjub::ExtendedPoint::from_bytes(&bytes)
        .ok_or_vartime(Err(io::Error::new(io::ErrorKind::InvalidInput, "invalid cv")))?

from subtle.

Might as well also include ok_or_else_vartime, in that case. I think those two cover the most common cases of converting to Result (other cases can go via Option::from).

from subtle.

Should I add it in a different PR? Or use #75 and change the description? Whatever you prefer. Also, should we wait for @hdevalence to get some of his feedback?

from subtle.

Hi, sorry for the delay, I had to evacuate my house because of a forest fire so I'm behind on Github notifications.

from subtle.

I believe this should be closed via #75

from subtle.