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 neuron certificate handling #2

Closed
wants to merge 382 commits into from
Closed

add neuron certificate handling #2

wants to merge 382 commits into from

Conversation

andreea-popescu-reef
Copy link
Collaborator

@andreea-popescu-reef andreea-popescu-reef commented Apr 2, 2024

Description

implemented logic for neuron certificate discovery and storage:

  • add neuronInfo_getNeuronCertificate: ( netuid, hotkey ) --> certificate pallet storage for neuron certificates
  • add fn get_neuron_certificate(netuid, neuron_uid) rpc to be called via bittensor library to fetch the certificate for a particular neuron
  • add fn serve_axon_tls(..., certificate) as an alternative for existing serve_axon taking an additional certificate parameter and storing it

compatible with older versions of bittensor as it doesn't modify the behaviour of existing rpc calls.

Checklist

  • Added tests for intended behaviors
  • Added tests for error and/or panic states
  • Updated relevant documentation
  • Tracked rustfmt changes
  • No new compiler warnings

@andreea-popescu-reef andreea-popescu-reef force-pushed the encrypt branch 2 times, most recently from b4e04f5 to e5ad0a0 Compare April 4, 2024 11:23
@andreea-popescu-reef andreea-popescu-reef changed the title wip add neuron certificate discovery Apr 4, 2024
@andreea-popescu-reef andreea-popescu-reef changed the title add neuron certificate discovery add neuron certificate handling Apr 4, 2024
Copy link

@emnoor-reef emnoor-reef left a comment

Choose a reason for hiding this comment

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

Could not compile due to certificate module non-existing. I could be wrong, but if a file was missing from git, I'll look at it again once you add the file.

I have little to no idea about subtensor internals, so left some style related nitpick comments 😁. But going through it, it looks good to me (assuming certificate::Certificate is just an alias for Vec<u8>), apart from the nomenclature thing pawel mentioned.

pallets/subtensor/src/lib.rs Outdated Show resolved Hide resolved
pallets/subtensor/src/lib.rs Outdated Show resolved Hide resolved
pallets/subtensor/src/serving.rs Outdated Show resolved Hide resolved
pallets/subtensor/src/lib.rs Outdated Show resolved Hide resolved
pallets/subtensor/src/neuron_info.rs Outdated Show resolved Hide resolved
pallets/subtensor/src/neuron_info.rs Outdated Show resolved Hide resolved
runtime/src/lib.rs Outdated Show resolved Hide resolved
@andreea-popescu-reef andreea-popescu-reef force-pushed the encrypt branch 2 times, most recently from 6a5aedc to 9b12bcc Compare April 5, 2024 02:15
Comment on lines 1 to 4
use sp_std::vec::Vec;

// TODO Certificate Encryption Pubkey
pub type Certificate = Vec<u8>;

Choose a reason for hiding this comment

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

As odd pointed out in slack, having [u8; 32] would be more appropriate than Vec<u8> as the certificates are supposed to be 256-bit in length. We don't want to allow users to store data of arbitrary size here.

@@ -14,6 +14,7 @@ sp_api::decl_runtime_apis! {
pub trait NeuronInfoRuntimeApi {
fn get_neurons(netuid: u16) -> Vec<u8>;
fn get_neuron(netuid: u16, uid: u16) -> Vec<u8>;
fn get_neuron_certificate(netuid: u16, uid: u16) -> Vec<u8>;
Copy link

@emnoor-reef emnoor-reef Apr 5, 2024

Choose a reason for hiding this comment

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

Here and every other method the returns a certificate should have a return type of Option<...> ( Option<Vec<u8>>, Option<[u8; 32]> if we go the array route).

I'd also prefer using Certificate instead of Vec<u8>/[u8; 32] as we are defining a type anyway.

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.