sn_data_types::PublicKey Wrapper Usage

Core Libraries
#1

In sn_data_types we wrap the various public/private/keypair used throughout. Though, the use of these types is not currently uniform across repos, and I’m unsure of why that is. For example, in sn_routing I often see imports like use ed25519_dalek::PublicKey as Ed25519PublicKey, which strikes me as odd because we already provide the sn_data_types::PublicKey::Ed25519 variant. Is there a reason for this, or is this just legacy code that slipped through?

If the latter is the case, it might be useful to go in and clean it up to use the sn_data_types provided wrapper so that we could remove the additional dependencies on ed25519_dalek and threshold_crypto in sn_api, sn_node, and sn_routing, as well as generally remove some redundant code for handling all of these things.

4 Likes
#2

I’m not entirely sure. I feel like it’s just legacy. but there may be other reasons. (i’m not suuuuper into routing).

mayb @Qi_Ma or adam (not sure of his handle on here), can answer this one.

1 Like
#3

In sn_routing we use ed25519_dalek::PublicKey and threshold_crypto::PublicKey for quite different purposes. I don’t think there is a place in the codebase where it makes sense to use both. So I don’t think replacing them with the wrapper would clean up the code that much. In fact, I think it would make it worse, because we would now have to add code branches for the wrong key types in places where we currently don’t need them.

3 Likes
#4

Hmm, yea I was taking a look at it just now, and that makes sense.

Then here’s a followup question to take it in the other direction: In what parts of the codebase does it make the most sense to use the wrapped version? What is the original intention behind providing an enum PublicKey and enum PrivateKey wrapper in sn_data_types? I wonder if there’s a document somewhere that indicates the use case a bit clearer, or if this could be added to the docs if it’s not there?

Seems to me from a quick GitHub search and through my own anecdotal code browsing, that these wrapped types don’t currently stand well on their own. Often importing sn_data_types::PublicKey is accompanied by an import of bls::PublicKey or some other library function. It seems a bit of “code-smell” that we have a wrapper that relies on the underlying implementation to be imported with it, no? Maybe I’m missing something? Does that indicate the wrapper needs to be fleshed out more? Or are we trying to mix apples and oranges too much?

In terms of keeping them in an enum, is there any place in code where it’s important match on the key, and dispatch to a different handler depending on the key type? It seems to me, on first glance, to be the opposite, where we often rely on the underlying type of the key to be a specific variant.

This is all leading to the reverse possibility. Just thinking aloud, but would we be better served by re-exporting threshold_crypto and ed25519_dalek from sn_data_types and wrapping just the error types instead? It would still unify the dependency versions, but would allow each key type to be treated on its own terms :thinking:

#5

One of the main uses of these enums are for our Policy in data types, for example here makes the owner of a Sequence to be any of the supported public keys, e.g. BLS, Ed25519, etc. Right now from the CLI we are not yet making use of BLS keys, but eventually we will do it to support multisig for data ownership and permissions.

1 Like
#6

Ah ok thanks for the info, that’s just what I was looking for!
I think I see why the distinction exists now, although it still seems it’d be nice to be able to use the same type across the board eventually :+1:

1 Like
#7

Are you seeing this in any other place apart from sn_routing? it’s worth reviewing it to make sure they make sense or it’s as you say something not that clean…

#8

After reading your post on the distinction, perhaps these handful of refactors could be still be applied to enforce the separation. The rest of what I had been noticing was indeed related in some way to routing (even if it’s not in the routing crate, which tripped me up originally. E.g. using threshold_crypto for loading reward keys in sn_node::node::state_db). Perhaps it’s useful to document that detail on the wrapper types somewhere, if it’s not already?

Refactor ideas:

  • if there’s no plan for this function, maybe remove the unused function bls_sk_from_hex() in sn_api, perhaps allowing removal of threshold_crypto dependency
  • Add an ed25519_from_bytes for PublicKey and SecretKey (also untested, but similar to above, might allow removal of the ed25519_dalek dependency in sn_api (since it’s only used in a handful of places for that functionality)
  • Add hex parse/display utility for PublicKey in sn_data_types to avoid reimplementation in sn_node and sn_api
  • Only discard wrapper PublicKey and SecretKey types internally in Network.rs in sn_node (instead of passing them back to for use in sn_node-internal code in the form of AdultState and ElderState snapshots structs).

I don’t know if each of these individual concepts will work exactly as described, but these are some of the things I thought of. Rather than me speculating here, I can try to replace them in my own binary and create a git issue/PR if it works out and allows me to drop some of the dependencies/clean up usage of the wrapper types in non-routing-related code.

Update

Truthfully, the more I look at it, it seems like this is one of the areas under heavy, active development. I’ll keep this in the back of my mind, but I don’t think it’s worth it in terms of return-on-investment for time considering this whole keys API seems to be undergoing refactor anyway :man_shrugging:

3 Likes
#9

Thanks a lot @Scorch , I’ll take a deep look next week at what you provided, and share my thoughts/findings too, as you say we can see when it’s the best moment to make these enhancements, I think it’s really good to spot them out already anyway.

2 Likes
#10

I’ll add this change in my next PR

These sounds good to me, but it sounds we need to be careful and consider what’s been brought up here by @mav about compatibility so we do it properly and then all crates can benefit from it: Simple web-based tool for BLS keys - #38 by mav - Apps - Safe Network Forum
If we define a proper way forward for these utilities I think we can work on adding them, we can gradually then switch other crates to start using them without affecting current ongoing development for testnet, or even postpone that for after testnet while preparing the utilities in sn_data_types…?

2 Likes
#11

I had meant to re-visit this earlier, but I think that makes the most sense :+1: . Anyway, I got around to open a PR for the utils, and then after the testnet, code in other crates can be readily converted/consolidated

1 Like