Skip to content
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

add eddy crate: flow encryption implementation scaffold #1058

Merged
merged 11 commits into from
Jul 24, 2022

Conversation

avahowell
Copy link
Member

Co-Authored-By: Henry de Valence hdevalence@penumbra.zone

@avahowell avahowell temporarily deployed to smoke-test June 23, 2022 01:21 Inactive
@avahowell avahowell temporarily deployed to smoke-test June 23, 2022 01:25 Inactive
@avahowell avahowell temporarily deployed to smoke-test June 23, 2022 02:08 Inactive
@avahowell avahowell temporarily deployed to smoke-test June 23, 2022 03:01 Inactive
@avahowell avahowell temporarily deployed to smoke-test June 23, 2022 05:45 Inactive
@avahowell avahowell temporarily deployed to smoke-test June 23, 2022 08:15 Inactive
@avahowell avahowell temporarily deployed to smoke-test June 23, 2022 08:50 Inactive
Comment on lines 28 to 44
let value0 = table
.lookup(decryption0.compress().0)
.await?
.ok_or_else(|| anyhow::anyhow!("could not find value in LUT"))?;
let value1 = table
.lookup(decryption1.compress().0)
.await?
.ok_or_else(|| anyhow::anyhow!("could not find value in LUT"))?;
let value2 = table
.lookup(decryption2.compress().0)
.await?
.ok_or_else(|| anyhow::anyhow!("could not find value in LUT"))?;
let value3 = table
.lookup(decryption3.compress().0)
.await?
.ok_or_else(|| anyhow::anyhow!("could not find value in LUT"))?;
Copy link
Member

Choose a reason for hiding this comment

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

Mathematically, decryption can only fail if the value is not present in the lookup table, and while the point of the value integrity proofs is to guarantee that this cannot occur, someone could add too many ciphertexts at once, or similar, so it's important to model.

But in practice, the lookup table may not be entirely resident in memory, and might involve access to I/O, which is (1) async and (2) fallible. So there are two kinds of errors: errors from the underlying storage, and a failed lookup.

The lookup trait returns a Result<Option<u32>, anyhow::Error>, with an outer Result allowing indication of an arbitrary I/O error, and the inner Option signaling whether or not the value was present. This is a good API for external users to plug into, since they can put whatever I/O errors they want in the Err variant, and it's easy to signal missing values with None.

Here in decryption, though, we probably want a way to allow users to distinguish whether the error was due to underlying I/O issues, or whether a value went out of range. If we use anyhow! to make a stringy error, we can't do this.

There are two options:

  1. We could change the return type of decryption to Result<Option<Value>, anyhow::Error>, with the inner Option signaling whether the value was too large or not;
  2. We could leave the return type as Result<Value, anyhow::Error>, but create a LimbRangeError struct, and use thiserror to derive an Error implementation for it, then do .ok_or(LimbRangeError)? here.

If we do the first option, all downstream users have to manually transform the inner Option. If we do the second option, downstream users that don't care can bubble up a generic error, and downstream users that do care can use downcast_ref::<LimbRangeError>() to handle it specially.

So option (2) seems better to me.

Comment on lines +34 to +37
fn append_blinding_commitment(&mut self, label: &'static [u8], point: &decaf377::Element) {
self.append_message(b"dom-sep", label);
self.append_message(b"blinding-commitment", &point.compress().0);
}
Copy link
Member

Choose a reason for hiding this comment

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

While decaf377 gives us a prime-order group, the group still does have one small subgroup -- the identity element is a subgroup of size 1, and so I think we need to check that we're not processing a bunch of identity points.

One way we could do this is suggested in the Merlin docs -- include validation in the transcript functions, making them return Results and fail on identity points.

@avahowell avahowell temporarily deployed to smoke-test June 23, 2022 19:48 Inactive
@avahowell avahowell temporarily deployed to smoke-test July 6, 2022 23:33 Inactive
@avahowell avahowell temporarily deployed to smoke-test July 7, 2022 03:00 Inactive
@aubrika aubrika added this to the Testnet 21: Eurydome milestone Jul 14, 2022
@hdevalence hdevalence temporarily deployed to smoke-test July 23, 2022 09:53 Inactive
@hdevalence hdevalence temporarily deployed to smoke-test July 24, 2022 11:44 Inactive
@hdevalence hdevalence temporarily deployed to smoke-test July 24, 2022 11:49 Inactive
@hdevalence
Copy link
Member

Based on tendermint/tendermint#9053, it seems like ABCI++ is not going to ship in its complete form (i.e., including VoteExtensions) in a deployable version of Tendermint before late this year at the earliest. Unfortunately, this means that we will not be able to include flow encryption in the initial version of Penumbra, and we will have to add it in a later network upgrade some time in 2023.

As a result, we should pause this work, and just try to get it in a good enough state to merge in for now, and come back to it for v2. I did a docs/API pass to that effect.

@hdevalence hdevalence merged commit c453c08 into main Jul 24, 2022
@hdevalence hdevalence deleted the flow-encryption-impl branch July 24, 2022 12:33
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.

None yet

4 participants