feat: Node Operator migration #8370
Conversation
| //! This module provides functionality for node providers to migrate node operator | ||
| //! records from one principal to another within the same data center. |
There was a problem hiding this comment.
Not sure why this is called a "migration". IIUC, this just changes an NP's NO in a DC from one principal to another. I mean, I guess any change can be called a "migration", but to me, "migration" implies that a "large" amount of work is required.
| //! ## Use Cases | ||
| //! | ||
| //! - **Lost Private Key**: When a node operator loses access to their private key, | ||
| //! they can migrate to a new node operator identity while preserving all associated data. | ||
| //! - **Consolidation**: Merge multiple node operators into a single node operator within | ||
| //! the same data center. |
There was a problem hiding this comment.
Not sure if this is needed. I mean, I guess it's not harmful to include this.
The main question //! comments should answer is not really "Why would I want this?" but rather, "What does this do? How do I use it?", not "What do I use this FOR?". That's up to the user to decide.
| /// Minimum age a node operator must have before it can be migrated. | ||
| /// This prevents spam by requiring operators to exist for at least this duration | ||
| /// before they can be deleted through migration. | ||
| const MIGRATION_CAPACITY_INTERVAL_HOURS: u64 = 12; | ||
|
|
||
| /// The duration form of [`MIGRATION_CAPACITY_INTERVAL_HOURS`]. | ||
| const MIGRATION_CAPACITY_INTERVAL: Duration = | ||
| Duration::from_secs(MIGRATION_CAPACITY_INTERVAL_HOURS * 60 * 60); |
There was a problem hiding this comment.
Seems redundant. I would rather keep the second one, because then, you cannot make mistakes like mass_kg + duration_s, or duration_s + duration_hr. Raw numbers for physical quantities (like amount of time) is bad.
| let timestamp_created = Duration::from_nanos(timestamp_created_nanos); | ||
| let now_duration = now | ||
| .duration_since(SystemTime::UNIX_EPOCH) | ||
| .expect("SystemTime before UNIX EPOCH"); | ||
|
|
||
| if now_duration < timestamp_created + MIGRATION_CAPACITY_INTERVAL { |
There was a problem hiding this comment.
| let timestamp_created = Duration::from_nanos(timestamp_created_nanos); | |
| let now_duration = now | |
| .duration_since(SystemTime::UNIX_EPOCH) | |
| .expect("SystemTime before UNIX EPOCH"); | |
| if now_duration < timestamp_created + MIGRATION_CAPACITY_INTERVAL { | |
| let created_at = SystemTime::UNIX_EPOCH | |
| .checked_add(Duration::from_nanos(timestamp_created_nanos)) | |
| .expect(...); | |
| let age = now.duration_since(created_at).expect(...) | |
| if age < MIGRATION_CAPACITY_INTERVAL { |
Admittedly, this is not much different from what you did, but to me, this is slightly more readable.
| // The caller must be the owner of both of the node operator | ||
| // records. | ||
| if caller.to_vec() != old_node_operator_record.node_provider_principal_id { | ||
| return Err(MigrateError::CallerMismatch { | ||
| caller, | ||
| expected: PrincipalId(Principal::from_slice( | ||
| &old_node_operator_record.node_provider_principal_id, | ||
| )), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Seems like this check should be done as soon as old_node_operator_record is fetched. I mean, I guess it does not make a big difference, but that seems more natural to me, because that is the first point when we might be able to reject based on authorization.
| ]) | ||
| } | ||
|
|
||
| /// Merges data from the old node operator record into the new node operator record. |
There was a problem hiding this comment.
List the fields that get "transferred"?
| .map(|(key, record)| { | ||
| upsert( | ||
| make_node_record_key(key).as_bytes(), | ||
| NodeRecord { | ||
| node_operator_id: new_node_operator_id.to_vec(), | ||
| ..record | ||
| } | ||
| .encode_to_vec(), | ||
| ) | ||
| }) |
There was a problem hiding this comment.
Do as you see fit.
Alternatively,
| .map(|(key, record)| { | |
| upsert( | |
| make_node_record_key(key).as_bytes(), | |
| NodeRecord { | |
| node_operator_id: new_node_operator_id.to_vec(), | |
| ..record | |
| } | |
| .encode_to_vec(), | |
| ) | |
| }) | |
| .map(|(key, mut record)| { | |
| record.node_operator_id = new_node_operator_id.to_vec(); | |
| upsert( | |
| make_node_record_key(key).as_bytes(), | |
| record.encode_to_vec(), | |
| ) | |
| }) |
| /// the data present in the old node operator record. | ||
| /// | ||
| /// If this node operator doesn't exist, it will be created. | ||
| #[prost(message, optional, tag = "1")] |
There was a problem hiding this comment.
Is this copied from somewhere?? Shouldn't prost stuff be generated by Prost??
| DataCenterMismatch { old: String, new: String }, | ||
|
|
||
| /// The caller is not the node provider that owns the operators. | ||
| CallerMismatch { |
There was a problem hiding this comment.
Do as you see fit.
| CallerMismatch { | |
| NotAuthorized { |
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
If I do not review this soon, please, remind me.
2 participants
This PR introduces the core implementation for the Node Operator Migration feature, which allows node providers to migrate node operator records from one principal to another within the same data center. This addresses the use case where a node operator loses access to their private key and needs to migrate to a new identity while preserving all associated data, removing the need to redeploy nodes (if they are actively participating in subnets).
There is some pending work left that will be done in follow-up PRs to keep the scope of this PR mostly to a single (new) file.
Work remaining to be done: