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

[fastx types] 32 byte authenticator #94

Merged
merged 1 commit into from
Dec 31, 2021

Conversation

sblackshear
Copy link
Collaborator

This is based on #88 and thus may be hard to review until that PR lands...

Previously, we assumed that Move addresses (16 bytes)/Authenticators and FastX addresses/authenticators (currently 32 bytes) had to coincide + used unsafe hacks to convert between them. This PR separates the two, allowing Move Authenticators to be 32 bytes + eliminating all the hacks.

@sblackshear sblackshear requested review from gdanezis, huitseeker, lxfind, oxade and patrickkuo and removed request for huitseeker December 27, 2021 04:38
@sblackshear sblackshear force-pushed the 32_byte_authenticator branch from 5bdfc5f to ac4ef4a Compare December 28, 2021 00:36
@huitseeker
Copy link
Contributor

huitseeker commented Dec 28, 2021

Consider making (the branch of) #88 the target branch?

@sblackshear
Copy link
Collaborator Author

I'm embarrassed to say I don't know how to do this when the base branch is on my fork--I only see the following options when I try to change it:
image

@huitseeker
Copy link
Contributor

@sblackshear 🤦 it's not you: that's impossible. This would be one case where only opening a (feature) branch on the main repo would work.

@sblackshear sblackshear force-pushed the 32_byte_authenticator branch from ac4ef4a to a46ec62 Compare December 29, 2021 06:18
fn try_from(bytes: &[u8]) -> Result<Self, FastPayError> {
let arr: [u8; dalek::PUBLIC_KEY_LENGTH] = bytes
.try_into()
.map_err(|_| FastPayError::InvalidAuthenticator)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

The single error case of this TryFrom is just an invalid length error. There is a host of other problems completely ignored by the current implementation:

  • there are sequences of 32 bytes that will structurally not ever be usable as an Ed25519 Public Key in any way,
  • there are sequences of 32 bytes that will 100% be usable as an Ed25519 Public Key, but that demonstrably and unambiguously aim at tricking anybody who does so.

The above takes exactly none of that into account. Further, several of those checks will not be performed by check_internal's dalek::PublicKey::from_bytes (and the library has a nice warning to say so).

I admit it's probably a completely orthogonal point to this PR, and worth tackling in a different issue (probably extracted from this comment), but I'd appreciate a spectacular comment on PublicKeyBytes making this clear. Here is an example of my personal minimum bar for the word "spectacular".

@huitseeker
Copy link
Contributor

huitseeker commented Dec 29, 2021

@sblackshear I'm happy to write a BCS-compatible serialization for Array<T, const N: usize> (possibly even upstream) if you open a fastNFT ticket for it & assign to me, I've already done most of the work in Narwhal in MystenLabs/narwhal#29

@sblackshear
Copy link
Collaborator Author

@sblackshear I'm happy to write a BCS-compatible serialization for Array<T, const N: usize> (possibly even upstream) if you open a fastNFT ticket for it & assign to me, I've already done most of the work in Narwhal in MystenLabs/narwhal#29

Thanks for flagging the issue above + this offer!

I'm not sure we need this for now because of the forthcoming decoupling of FastPay addresses and authenticators:

  • After this change an FastPayAddress, will instead be the hash of an authenticator (e.g. pubkey)
  • In Move, we'll probably need to encode a FastPayAddress with a vector<u8> (Move doesn't have fixed-size arrays, though would be nice to add them in the future), which has a different BCS serialization than a fixed-length array (because it includes a length prefix).

@sblackshear sblackshear force-pushed the 32_byte_authenticator branch 2 times, most recently from 3e217bf to da03bde Compare December 31, 2021 02:55
Previously, we assumed that Move `address`es (16 bytes)/`Authenticator`s and FastX addresses/authenticators (currently 32 bytes) had to coincide + used unsafe hacks to convert between them. This PR separates the two, allowing Move `Authenticator`s to be 32 bytes + eliminating all the hacks.
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