boltlabs-inc / tss-ecdsa

An implementation of a threshold ECDSA signature scheme
Other
12 stars 5 forks source link

Distinguish Error PiMod #419

Closed hridambasu closed 1 year ago

hridambasu commented 1 year ago

We're talking about the PiMod ZKP; this is a code quality issue, not a correctness issue. In arithmetic functions, determine when a failure is an actual customer-facing error vs an expected internal error, and try to refactor to use Options in the latter case. See comment. The problem is that we raise our external-facing errors in cases where it's actually not an external facing error (it's an internal thing that's expected to not work a bunch of times before it does work) A better solution would be to only raise an InternalError when we actually have a problem, and use a different paradigm to indicate "it didn't work this time but we haven't given up yet."

marsella commented 1 year ago

Some more specific notes on this issue:

That "could not find square roots modulo n" problem is not actually an error, it's just an expected part of the algorithm that you have to test + reject a bunch of things before you'll get a correct answer. However, the library currently throws an error and handles it internally, resulting in a million logs. Instead of throwing an error from the function that does this, I want to return an Option without any logging (to more clearly indicate that this is not a problem).

There are still some actual errors that can happen in the course pimod, like passing bad inputs to the chinese remainder theorem. So in some cases we might need to return Result<Option<_>>, where the Result encodes real errors and the Option indicates whether or not we found a square root.

The pattern we should remove from the code is anything that looks like:

match some_function_call(...) {
   Ok(val) => do_something(val),
   Err(_) => continue,
}

where the type signature for hte function is fn some_function_call(...) -> Result<Foo> If we have real errors (like the chinese remainder theorem error), this will ignore them, which is bad. A better paradigm would look like:


fn some_function_call(...) -> Result<Option<Foo>>;
...
match some_function_call(...)? {
    Some(val) => do_something(val),
    None => continue,
}