-
Notifications
You must be signed in to change notification settings - Fork 157
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 #750
add neuron certificate handling #750
Conversation
88eff8c
to
ae34fd5
Compare
On seconds thoughts, adding the new extrinsic ensures backwards compatibility , so a bit undecided here |
pallets/subtensor/src/lib.rs
Outdated
#[derive(Decode, Encode, Default, TypeInfo, PartialEq, Eq, Clone, Debug)] | ||
pub struct NeuronCertificate { | ||
/// The neuron certificate. | ||
pub certificate: Vec<u8>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make it a bounded vector instead? There should be a limit of how much data can an axon store on-chain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better to add freeze_struct, even for this simple struct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will add freeze_struct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andreea-popescu-reef what about the using a bounded vec ? Have you tested with actual certificate sizes to see how much data it is ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for an ECDSA public key we would need 256bits. To support multiple algorithms 512bits should be enough for the key plus some additional bits to determine the algoritthm so let's reserve 520bits.
I'd also rename the certificate field
- public_key
would be more appropiate
does that sound good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its not about the naming , but more about people being able to dump arbitrary data (potentialy 5 MB worth) into the field. This is why we are suggesting a bounded vec. It would also be nice to see some tests showing those bounds are enforced
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made the changes
6bb6d24
to
b5d7c0e
Compare
as instructed by formalized_tensor
need to revert changes to runtime api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest revert refactor in an area of the code that doesn't have tests and we really, really want to get this PR merged, so we'll come back to that refactor in a separate PR
Description
implemented logic for neuron certificate discovery and storage:
neuronInfo_getNeuronCertificate: ( netuid, hotkey ) --> certificate
pallet storage for neuron certificatesfn get_neuron_certificate(netuid, neuron_uid)
rpc to be called via bittensor library to fetch the certificate for a particular neuronfn serve_axon_tls(..., certificate)
as an alternative for existingserve_axon
taking an additional certificate parameter and storing itcompatible with older versions of bittensor as it doesn't modify the behaviour of existing rpc calls.
Checklist
rustfmt
changes