fix(pallet): rework billing loop (#702)

docs/architecture/0009-rework-billing-loop.md

@@ -0,0 +1,18 @@
+# 9. Rework smart contract billing loop
+
+Date: 2023-05-23
+
+## Status
+
+Accepted
+
+## Context
+
+See [here](https://github.com/threefoldtech/tfchain/issues/701) for more details.
+
+## Decision
+
+(1) Each contract (node/rent/name contracts) is inserted in billing loop only once, when contract is created.
+There is no more exception for node contracts with no public ips and no used resources on chain.
+
+(2) When a contract is to be billed during the billing loop execution (via the offchain worker), bill it ONLY if there is effectively some IP/SU/CU/NU consumed. Technically this comes down to trigger `bill_contract()` extrinsic only when there is some used ip / resources or consumption reports linked to this contract.

substrate-node/pallets/pallet-smart-contract/src/lib.rs

@@ -534,8 +534,10 @@ pub mod pallet {
             let _account_id = ensure_signed(origin)?;
 
             // Clean up contract from billing loop if it doesn't exist anymore
+            // This is the only place it should be done to guarantee we
+            // remove contract id from billing loop at right index
             if Contracts::<T>::get(contract_id).is_none() {
-                log::debug!("cleaning up deleted contract from storage");
+                log::debug!("cleaning up deleted contract from billing loop");
                 Self::remove_contract_from_billing_loop(index, contract_id)?;
                 return Ok(().into());
             }
@@ -660,8 +662,8 @@ pub mod pallet {
             // Let offchain worker check if there are contracts on the map at current index
             let current_index = Self::get_current_billing_loop_index();
 
-            let contracts = ContractsToBillAt::<T>::get(current_index);
-            if contracts.is_empty() {
+            let contract_ids = ContractsToBillAt::<T>::get(current_index);
+            if contract_ids.is_empty() {
                 log::info!(
                     "No contracts to bill at block {:?}, index: {:?}",
                     block_number,
@@ -672,11 +674,33 @@ pub mod pallet {
 
             log::info!(
                 "{:?} contracts to bill at block {:?}",
-                contracts,
+                contract_ids,
                 block_number
             );
 
-            for contract_id in contracts {
+            for contract_id in contract_ids {
+                if let Some(c) = Contracts::<T>::get(contract_id) {
+                    if let types::ContractData::NodeContract(node_contract) = c.contract_type {
+                        // Is there IP consumption to bill?
+                        let bill_ip = node_contract.public_ips > 0;
+
+                        // Is there CU/SU consumption to bill?
+                        // No need for preliminary call to contains_key() because default resource value is empty
+                        let bill_cu_su =
+                            !NodeContractResources::<T>::get(contract_id).used.is_empty();
+
+                        // Is there NU consumption to bill?
+                        // No need for preliminary call to contains_key() because default amount_unbilled is 0
+                        let bill_nu = ContractBillingInformationByID::<T>::get(contract_id)
+                            .amount_unbilled
+                            > 0;
+
+                        // Don't bill if no IP/CU/SU/NU to be billed
+                        if !bill_ip && !bill_cu_su && !bill_nu {
+                            continue;
+                        }
+                    }
+                }
                 let _res = Self::bill_contract_using_signed_transaction(contract_id);
             }
         }
@@ -864,22 +888,16 @@ impl<T: Config> Pallet<T> {
         let mut id = ContractID::<T>::get();
         id = id + 1;
 
+        if let types::ContractData::NodeContract(ref mut nc) = contract_type {
+            Self::_reserve_ip(id, nc)?;
+        };
+
         Self::validate_solution_provider(solution_provider_id)?;
 
         // Start billing frequency loop
         // Will always be block now + frequency
-        match contract_type {
-            types::ContractData::NodeContract(ref mut node_contract) => {
-                Self::_reserve_ip(id, node_contract)?;
-
-                // Insert created node contract in billing loop now only
-                // if there is at least one public ip attached to node
-                if node_contract.public_ips > 0 {
-                    Self::insert_contract_in_billing_loop(id);
-                }
-            }
-            _ => Self::insert_contract_in_billing_loop(id),
-        };
+        // Contract is inserted in billing loop ONLY once at contract creation
+        Self::insert_contract_in_billing_loop(id);
 
         let contract = types::Contract {
             version: CONTRACT_VERSION,
@@ -1013,13 +1031,6 @@ impl<T: Config> Pallet<T> {
                 // Do insert
                 NodeContractResources::<T>::insert(contract.contract_id, &contract_resource);
 
-                // Start billing frequency loop
-                // Insert node contract in billing loop only if there
-                // are non empty resources pushed for the contract
-                if !contract_resource.used.is_empty() {
-                    Self::insert_contract_in_billing_loop(contract.contract_id);
-                }
-
                 // deposit event
                 Self::deposit_event(Event::UpdatedUsedResources(contract_resource));
             }

substrate-node/pallets/pallet-smart-contract/src/tests.rs

@@ -38,14 +38,21 @@ fn test_create_node_contract_works() {
         run_to_block(1, None);
         prepare_farm_and_node();
 
+        let node_id = 1;
+        let public_ips = 0;
         assert_ok!(SmartContractModule::create_node_contract(
             RuntimeOrigin::signed(alice()),
-            1,
+            node_id,
             generate_deployment_hash(),
             get_deployment_data(),
-            0,
+            public_ips,
             None
         ));
+
+        // Also check if contract id was inserting in billing loop
+        let contract_id = 1;
+        let contract_to_bill = SmartContractModule::contract_to_bill_at_block(1);
+        assert_eq!(contract_to_bill, [contract_id]);
     });
 }
 
@@ -117,39 +124,6 @@ fn test_create_node_contract_with_public_ips_works() {
     });
 }
 
-#[test]
-fn test_create_node_contract_with_no_public_ips_billing_insertion_works() {
-    new_test_ext().execute_with(|| {
-        run_to_block(1, None);
-        prepare_farm_and_node();
-
-        let node_id = 1;
-        let public_ips = 0;
-        assert_ok!(SmartContractModule::create_node_contract(
-            RuntimeOrigin::signed(alice()),
-            node_id,
-            generate_deployment_hash(),
-            get_deployment_data(),
-            public_ips,
-            None
-        ));
-
-        let contract_to_bill = SmartContractModule::contract_to_bill_at_block(1);
-        assert_eq!(contract_to_bill.len(), 0);
-
-        let contract_id = 1;
-        push_contract_no_resources_used(contract_id);
-
-        let contract_to_bill = SmartContractModule::contract_to_bill_at_block(1);
-        assert_eq!(contract_to_bill.len(), 0);
-
-        push_contract_resources_used(contract_id);
-
-        let contract_to_bill = SmartContractModule::contract_to_bill_at_block(1);
-        assert_eq!(contract_to_bill, [contract_id]);
-    });
-}
-
 #[test]
 fn test_create_node_contract_with_undefined_node_fails() {
     new_test_ext().execute_with(|| {
@@ -3706,19 +3680,6 @@ fn push_contract_resources_used(contract_id: u64) {
     ));
 }
 
-fn push_contract_no_resources_used(contract_id: u64) {
-    let mut resources = Vec::new();
-    resources.push(types::ContractResources {
-        contract_id,
-        used: Resources::empty(),
-    });
-
-    assert_ok!(SmartContractModule::report_contract_resources(
-        RuntimeOrigin::signed(alice()),
-        resources
-    ));
-}
-
 fn check_report_cost(
     contract_id: u64,
     amount_billed: u64,