From c471f19aa106614fb8c363b27977cccdf6a6adde Mon Sep 17 00:00:00 2001 From: Erwan Renaut <73958772+renauter@users.noreply.github.com> Date: Wed, 24 May 2023 09:29:17 -0300 Subject: [PATCH] fix(pallet): rework billing loop (#702) --- docs/architecture/0009-rework-billing-loop.md | 18 ++++++ .../pallets/pallet-smart-contract/src/lib.rs | 59 +++++++++++-------- .../pallet-smart-contract/src/tests.rs | 57 +++--------------- 3 files changed, 62 insertions(+), 72 deletions(-) create mode 100644 docs/architecture/0009-rework-billing-loop.md diff --git a/docs/architecture/0009-rework-billing-loop.md b/docs/architecture/0009-rework-billing-loop.md new file mode 100644 index 000000000..9b1d99969 --- /dev/null +++ b/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. \ No newline at end of file diff --git a/substrate-node/pallets/pallet-smart-contract/src/lib.rs b/substrate-node/pallets/pallet-smart-contract/src/lib.rs index 070f2201b..5c36124ec 100644 --- a/substrate-node/pallets/pallet-smart-contract/src/lib.rs +++ b/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::::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::::get(current_index); - if contracts.is_empty() { + let contract_ids = ContractsToBillAt::::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::::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::::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::::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 Pallet { let mut id = ContractID::::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 Pallet { // Do insert NodeContractResources::::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)); } diff --git a/substrate-node/pallets/pallet-smart-contract/src/tests.rs b/substrate-node/pallets/pallet-smart-contract/src/tests.rs index 414b47cb9..f0be578d0 100644 --- a/substrate-node/pallets/pallet-smart-contract/src/tests.rs +++ b/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,