Assign instance IDs in the Supervisor [ECR-3746]

exonum/src/blockchain/tests.rs

@@ -34,7 +34,7 @@ use crate::{
         error::ErrorKind,
         rust::{CallContext, Service, ServiceFactory, Transaction},
         AnyTx, ArtifactId, BlockchainData, DispatcherError, DispatcherSchema, ExecutionError,
-        InstanceId, SUPERVISOR_INSTANCE_ID,
+        InstanceId, InstanceSpec, SUPERVISOR_INSTANCE_ID,
     },
 };
 
@@ -141,9 +141,18 @@ impl TestDispatcherInterface for TestDispatcherService {
     }
 
     fn test_add(&self, mut context: CallContext<'_>, arg: TestAdd) -> Result<(), ExecutionError> {
-        let mut index = context.service_data().get_entry("val");
-        index.set(arg.value);
-        drop(index);
+        {
+            let mut index = context.service_data().get_entry("val");
+            index.set(arg.value);
+        }
+
+        let instance_id = {
+            let mut instance_id_entry = context.service_data().get_entry("instance_ids");
+            let instance_id = instance_id_entry.get().unwrap_or(TEST_SERVICE_ID + 1);
+            instance_id_entry.set(instance_id + 1);
+
+            instance_id
+        };
 
         let config = match arg.value {
             42 => TestExecute { value: 42 }.into_bytes(),
@@ -156,7 +165,15 @@ impl TestDispatcherInterface for TestDispatcherService {
         } else {
             TestDispatcherService.artifact_id().into()
         };
-        context.start_adding_service(artifact, format!("good-service-{}", arg.value), config)
+
+        let instance_name = format!("good-service-{}", arg.value);
+        let spec = InstanceSpec {
+            id: instance_id,
+            name: instance_name,
+            artifact,
+        };
+
+        context.start_adding_service(spec, config)
     }
 }
 
@@ -496,14 +513,6 @@ fn error_discards_transaction_changes() {
     }
 }
 
-#[test]
-#[should_panic(expected = "Instance identifier for builtin service should be lesser than")]
-fn test_dispatcher_incorrect_builtin_service_id() {
-    create_blockchain(vec![
-        InstanceCollection::new(TestDispatcherService).with_instance(1024, IDX_NAME, ())
-    ]);
-}
-
 #[test]
 fn test_dispatcher_deploy_good() {
     let keypair = crypto::gen_keypair();

exonum/src/runtime/dispatcher/mod.rs

@@ -44,14 +44,6 @@ mod schema;
 #[cfg(test)]
 mod tests;
 
-/// Max instance identifier for builtin service.
-///
-/// By analogy with the privileged ports of the network, we use a range 0..1023 of instance
-/// identifiers for built-in services which can be created only during the blockchain genesis
-/// block creation.
-// FIXME: remove
-pub const MAX_BUILTIN_INSTANCE_ID: InstanceId = 1024;
-
 #[derive(Debug)]
 struct ServiceInfo {
     runtime_id: u32,
@@ -109,22 +101,13 @@ impl Dispatcher {
     /// # Panics
     ///
     /// * If instance spec contains invalid service name or artifact id.
-    /// * If instance id is greater than [`MAX_BUILTIN_INSTANCE_ID`]
-    ///
-    /// [`MAX_BUILTIN_INSTANCE_ID`]: constant.MAX_BUILTIN_INSTANCE_ID.html
     pub(crate) fn add_builtin_service(
         &mut self,
         fork: &mut Fork,
         spec: InstanceSpec,
         artifact_spec: impl BinaryValue,
         constructor: Vec<u8>,
     ) -> Result<(), ExecutionError> {
-        assert!(
-            spec.id < MAX_BUILTIN_INSTANCE_ID,
-            "Instance identifier for builtin service should be lesser than {}",
-            MAX_BUILTIN_INSTANCE_ID
-        );
-
         // Register service artifact in the runtime.
         // TODO Write test for such situations [ECR-3222]
         if !self.is_artifact_deployed(&spec.artifact) {
@@ -377,11 +360,6 @@ impl Dispatcher {
         );
         Ok(())
     }
-
-    /// Assigns an instance identifier to the new service instance.
-    pub(crate) fn assign_instance_id(fork: &Fork) -> InstanceId {
-        Schema::new(fork).assign_instance_id()
-    }
 }
 
 /// Mailbox accumulating `Action`s to be performed by the dispatcher.

exonum/src/runtime/dispatcher/schema.rs

@@ -16,10 +16,10 @@
 
 use exonum_merkledb::{
     access::{Access, AccessExt},
-    Entry, Fork, MapIndex,
+    Fork, MapIndex,
 };
 
-use super::{ArtifactId, ArtifactSpec, Error, InstanceSpec, MAX_BUILTIN_INSTANCE_ID};
+use super::{ArtifactId, ArtifactSpec, Error, InstanceSpec};
 use crate::runtime::{DeployStatus, InstanceId, InstanceQuery};
 
 const ARTIFACTS: &str = "core.dispatcher.artifacts";
@@ -28,7 +28,6 @@ const SERVICE_INSTANCES: &str = "core.dispatcher.service_instances";
 const PENDING_INSTANCES: &str = "core.dispatcher.pending_service_instances";
 const INSTANCE_IDS: &str = "core.dispatcher.service_instance_ids";
 const PENDING_INSTANCE_IDS: &str = "core.dispatcher.pending_instance_ids";
-const VACANT_INSTANCE_ID: &str = "core.dispatcher.vacant_instance_id";
 
 /// Schema of the dispatcher, used to store information about pending artifacts / service
 /// instances, and to reload artifacts / instances on node restart.
@@ -195,22 +194,6 @@ impl Schema<&Fork> {
         Ok(())
     }
 
-    /// Vacant identifier for user service instances.
-    fn vacant_instance_id(&self) -> Entry<&Fork, InstanceId> {
-        self.access.get_entry(VACANT_INSTANCE_ID)
-    }
-
-    /// Assign unique identifier for an instance.
-    // TODO: could be performed by supervisor [ECR-3746]
-    pub(crate) fn assign_instance_id(&mut self) -> InstanceId {
-        let id = self
-            .vacant_instance_id()
-            .get()
-            .unwrap_or(MAX_BUILTIN_INSTANCE_ID);
-        self.vacant_instance_id().set(id + 1);
-        id
-    }
-
     /// Adds information about started service instance to the schema.
     pub(super) fn add_service(&mut self, spec: InstanceSpec) {
         debug_assert!(!self.service_instances().contains(&spec.name));

exonum/src/runtime/rust/call_context.rs

@@ -176,19 +176,13 @@ impl<'a> CallContext<'a> {
     #[doc(hidden)]
     pub fn start_adding_service(
         &mut self,
-        artifact: ArtifactId,
-        instance_name: String,
+        instance_spec: InstanceSpec,
         constructor: impl BinaryValue,
     ) -> Result<(), ExecutionError> {
         if self.instance.id != SUPERVISOR_INSTANCE_ID {
             panic!("`start_adding_service` called within a non-supervisor service");
         }
 
-        let instance_spec = InstanceSpec {
-            artifact,
-            name: instance_name,
-            id: Dispatcher::assign_instance_id(self.inner.fork),
-        };
         self.inner
             .child_context(self.instance.id)
             .start_adding_service(instance_spec, constructor)

services/supervisor/src/lib.rs

@@ -29,7 +29,7 @@ use exonum::{
     runtime::{
         api::ServiceApiBuilder,
         rust::{AfterCommitContext, CallContext, Service, Transaction},
-        BlockchainData, SUPERVISOR_INSTANCE_ID,
+        BlockchainData, InstanceId, SUPERVISOR_INSTANCE_ID,
     },
 };
 use exonum_derive::*;
@@ -45,6 +45,14 @@ mod proto_structures;
 mod schema;
 mod transactions;
 
+/// Instance identifier for first deployed service.
+///
+/// By analogy with the privileged ports of the network, we use a range 0..1023 of instance
+/// identifiers for built-in services which can be created only during the blockchain genesis
+/// block creation.
+// TODO: remove [ECR-3851]
+const MAX_BUILTIN_INSTANCE_ID: InstanceId = 1024;
+
 /// Decentralized supervisor.
 pub type DecentralizedSupervisor = Supervisor<mode::Decentralized>;
 
@@ -116,11 +124,10 @@ fn update_configs(context: &mut CallContext<'_>, changes: Vec<ConfigChange>) {
                 start_service.name,
                 start_service.artifact
             );
-            if let Err(e) = context.start_adding_service(
-                start_service.artifact,
-                start_service.name,
-                start_service.config,
-            ) {
+            let id = Schema::new(context.service_data()).assign_instance_id();
+            let (instance_spec, config) = start_service.into_parts(id);
+
+            if let Err(e) = context.start_adding_service(instance_spec, config) {
                 // Panic will cause changes to be rolled back.
                 panic!(
                     "An error occurred while starting the service instance: {:?}",

services/supervisor/src/proto_structures.rs

@@ -21,7 +21,7 @@ use exonum::{
     helpers::Height,
     impl_serde_hex_for_binary_value,
     messages::{AnyTx, Verified},
-    runtime::{rust::Transaction, ArtifactId, InstanceId, SUPERVISOR_INSTANCE_ID},
+    runtime::{rust::Transaction, ArtifactId, InstanceId, InstanceSpec, SUPERVISOR_INSTANCE_ID},
 };
 use exonum_crypto::{PublicKey, SecretKey};
 use exonum_derive::*;
@@ -62,6 +62,20 @@ pub struct StartService {
     pub config: Vec<u8>,
 }
 
+impl StartService {
+    /// Given the instance ID, splits the `StartService` request into `InstanceSpec`
+    /// and config value.
+    pub fn into_parts(self, id: InstanceId) -> (InstanceSpec, Vec<u8>) {
+        let spec = InstanceSpec {
+            id,
+            name: self.name,
+            artifact: self.artifact,
+        };
+
+        (spec, self.config)
+    }
+}
+
 /// Configuration parameters of the certain service instance.
 #[derive(
     Debug,

services/supervisor/src/schema.rs

@@ -12,13 +12,29 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use exonum::{crypto::Hash, helpers::multisig::ValidatorMultisig, runtime::ArtifactId};
+use exonum::{
+    crypto::Hash,
+    helpers::multisig::ValidatorMultisig,
+    runtime::{ArtifactId, InstanceId},
+};
 use exonum_merkledb::{
     access::{Access, FromAccess, Prefixed},
     Entry, Fork, ObjectHash, ProofMapIndex,
 };
 
-use super::{ConfigProposalWithHash, DeployConfirmation, DeployRequest, StartService};
+use super::{
+    ConfigProposalWithHash, DeployConfirmation, DeployRequest, StartService,
+    MAX_BUILTIN_INSTANCE_ID,
+};
+
+const DEPLOY_REQUESTS: &str = "deploy_requests";
+const DEPLOY_CONFIRMATIONS: &str = "deploy_confirmations";
+const PENDING_DEPLOYMENTS: &str = "pending_deployments";
+const PENDING_INSTANCES: &str = "pending_instances";
+const CONFIG_CONFIRMS: &str = "config_confirms";
+const PENDING_PROPOSAL: &str = "pending_proposal";
+const CONFIGURATION_NUMBER: &str = "configuration_number";
+const VACANT_INSTANCE_ID: &str = "vacant_instance_id";
 
 /// Service information schema.
 #[derive(Debug)]
@@ -30,32 +46,21 @@ pub struct Schema<T: Access> {
     pub config_confirms: ValidatorMultisig<T, Hash>,
     pub pending_proposal: Entry<T::Base, ConfigProposalWithHash>,
     pub configuration_number: Entry<T::Base, u64>,
+    pub vacant_instance_id: Entry<T::Base, InstanceId>,
 }
 
 impl<'a, T: Access> Schema<Prefixed<'a, T>> {
     /// Constructs schema for the given `access`.
     pub fn new(access: Prefixed<'a, T>) -> Self {
         Self {
-            deploy_requests: FromAccess::from_access(access.clone(), "deploy_requests".into())
-                .unwrap(),
-            deploy_confirmations: FromAccess::from_access(
-                access.clone(),
-                "deploy_confirmations".into(),
-            )
-            .unwrap(),
-            pending_deployments: FromAccess::from_access(
-                access.clone(),
-                "pending_deployments".into(),
-            )
-            .unwrap(),
-            pending_instances: FromAccess::from_access(access.clone(), "pending_instances".into())
-                .unwrap(),
-            config_confirms: FromAccess::from_access(access.clone(), "config_confirms".into())
-                .unwrap(),
-            pending_proposal: FromAccess::from_access(access.clone(), "pending_proposal".into())
-                .unwrap(),
-            configuration_number: FromAccess::from_access(access, "configuration_number".into())
-                .unwrap(),
+            deploy_requests: construct(&access, DEPLOY_REQUESTS),
+            deploy_confirmations: construct(&access, DEPLOY_CONFIRMATIONS),
+            pending_deployments: construct(&access, PENDING_DEPLOYMENTS),
+            pending_instances: construct(&access, PENDING_INSTANCES),
+            config_confirms: construct(&access, CONFIG_CONFIRMS),
+            pending_proposal: construct(&access, PENDING_PROPOSAL),
+            configuration_number: construct(&access, CONFIGURATION_NUMBER),
+            vacant_instance_id: construct(&access, VACANT_INSTANCE_ID),
         }
     }
 
@@ -80,4 +85,22 @@ impl Schema<Prefixed<'_, &Fork>> {
         let new_configuration_number = self.configuration_number.get().unwrap_or(0) + 1;
         self.configuration_number.set(new_configuration_number);
     }
+
+    /// Assign unique identifier for an instance.
+    pub(crate) fn assign_instance_id(&mut self) -> InstanceId {
+        let id = self
+            .vacant_instance_id
+            .get()
+            .unwrap_or(MAX_BUILTIN_INSTANCE_ID);
+        self.vacant_instance_id.set(id + 1);
+        id
+    }
+}
+
+/// Creates an index given its name and access object.
+fn construct<'a, T: Access, U: FromAccess<Prefixed<'a, T>>>(
+    access: &Prefixed<'a, T>,
+    index_name: &str,
+) -> U {
+    FromAccess::from_access(access.clone(), index_name.into()).unwrap()
 }