Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(pallet): rework billing loop #702

Merged
merged 6 commits into from
May 24, 2023
Merged

fix(pallet): rework billing loop #702

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0e3cde1
feat: insert contract in billing loop only once at contract creation
renauter May 23, 2023
e66a40b
chore: update tests
renauter May 23, 2023
489529d
feat: bill node contract only if some IP/CU/SU/NU to bill
renauter May 23, 2023
9b96041
chore: comment removing contract id from billing loop
renauter May 23, 2023
a30cbc5
chore: add ADR file
renauter May 24, 2023
a0db20c
chore: review
renauter May 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions docs/architecture/0009-rework-billing-loop.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
54 changes: 30 additions & 24 deletions substrate-node/pallets/pallet-smart-contract/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 guaranty we
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

guarantee*

// 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());
}
Expand Down Expand Up @@ -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,
Expand All @@ -672,11 +674,28 @@ 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 {
let bill_ip = node_contract.public_ips > 0;
let bill_cu_su = NodeContractResources::<T>::contains_key(contract_id)
&& !NodeContractResources::<T>::get(contract_id).used.is_empty();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why contains_key and get afterwards ? I think get suffices because of the ValueQuery storage definition (default when not present)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed
But I have a conceptual case here
Ok, I am aware the get will return default value
And what if a default value was intentionally pushed? how to differentiate both scenarios? (nothing pushed / default value pushed)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And what if a default value was intentionally pushed?

This is something that is bad practice and should be avoided, it also results in the same. To me, this looks like 2 storage reads while it can be only 1 storage read.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, in our case it results the same
But if we change the default value for the resource we are broken
So, I ll get rid of extra storage read and add comment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we broken if we change the default value for resources? .used.is_empty() should always check if all values are 0 and this should not change

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if for some reason we set this for example

    pub fn default() -> Resources {
        Resources {
            hru: 1,
            sru: 1,
            cru: 1,
            mru: 1,
        }
    }

    pub fn empty() -> Resources {
        Resources {
            hru: 0,
            sru: 0,
            cru: 0,
            mru: 0,
        }
    }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, if the default is any other than 0 values, the user has to pay anyway

let bill_nu =
ContractBillingInformationByID::<T>::contains_key(contract_id)
&& ContractBillingInformationByID::<T>::get(contract_id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why contains_key and get afterwards ? I think get suffices because of the ValueQuery storage definition (default when not present)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same idea here

.amount_unbilled
> 0;

// Don't bill if no IP/CU/SU/NU to bill
if !bill_ip && !bill_cu_su && !bill_nu {
continue;
}
}
}
let _res = Self::bill_contract_using_signed_transaction(contract_id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this one line up? If there is no contract (deleted somehow but still in billing loop) we don't want to push an extrinsic?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, lets check #703 (comment) for this

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this one line up? If there is no contract (deleted somehow but still in billing loop) we don't want to push an extrinsic?

cannot move this line up because when contract is deleted its id in billing loop is cleaned up at next cycle by pushing the extrinsic

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I just noticed that as well this morning, but it's very weird and hidden behaviour. It's not clear that it's subject to the offchain worker to be cleaned up or not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now let's keep this behaviour but let's plan a future improvement where we redesign the connection between a contract and it's billing status / cycle

}
}
Expand Down Expand Up @@ -864,22 +883,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,
Expand Down Expand Up @@ -1013,13 +1026,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));
}
Expand Down
57 changes: 9 additions & 48 deletions substrate-node/pallets/pallet-smart-contract/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
});
}

Expand Down Expand Up @@ -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(|| {
Expand Down Expand Up @@ -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,
Expand Down