refactor: Billing refactor

sameh-farouk commented 2 months ago

Description

This PR addresses several issues and introduces significant changes to the billing in the pallet-smart-contract module. Here are the key changes and the issues they resolve:

High-Level Changes:

Fixing multiple critical bugs in billing logic and redesigning the way overdue amount is tracked during the contract grace period.
- Billing no longer requires all validators to have off-chain workers enabled for proper functionality. However, having more than one is highly recommended for redundancy since the current implementation of OCWs in the substrate framework still lacks the guaranteed execution feature.
- Partial billing is now possible if a user lacks sufficient funds to cover the full amount due but can pay a portion. In such cases, the contract will enter a grace period as expected, and the remaining overdue amount will be added to the contract overdraft.
- Certified capacity is appropriately charged at a higher rate (25% more) compared to DIY capacity.
- Fund reservations no longer use locks/freezes. It was replaced with a more reliable reservation system, solving issues related to fund availability for reward distribution. payments on hold are no longer part of the free balance. The total balance should account for the reserved part.
Introducing new events to provide better visibility into the payment processes.
New revenue splits will be implemented based on new requirements:
- Eliminating the burning of TFT spent on utilization
- Eliminating the sales channel and solution providers' distribution.
- Increasing the percentage sent to the foundation account to 40%
- Increasing the percentage sent to the staking pool to 10%
- Distributing 50% to farmers (Note: This is not related to farmers getting 100% of extra fees set on dedicated nodes)

Find below more details

Key Changes and Improvements:

Refactoring Billing Logic:

The billing logic has been refactored to improve the billing feature for smart contracts. This includes more precise tracking of contract payments, handling of overdrafts, and fund reservations.

Improved off-chain worker logic

Updated off-chain worker logic to handle billing more effectively. is_next_block_author function was removed and the verification is now happening at runtime, ensuring that transaction fees are reliably waived for validators and still preventing duplicate transactions. It also implements a guaranteed execution layer for OCWs by enabling redundancy.

Benefits:

Redundancy, As long as at least one validator is running an off-chain worker, billing should work fine.
No longer depend on inferring the next author from the client side, which can be unreliable sometimes.
Ensuring Transaction Uniqueness: Preventing Duplicate Inclusions in the Same Block, still benefiting from the redundancy.
Validators are still exempt from transaction fees, but with a stronger guarantee, fixing a known issue that sometimes validators can incur a fee for submitting this call, or in other cases, the call might not sent at all.

Additionally, the improved logging ensures that the off-chain worker function works correctly without silent failures.

Introduction of ContractPaymentState:

Introducing ContractPaymentState aligns with the need to track and manage contract payment states accurately, especially during grace periods and when handling overdue payments. It fixes the issue where the amount tracked in the contract lock is incorrectly divergent from the lock on the user balance, leading to illiquidity issues when it's time to distribute rewards.

Improved Error Handling:

Enhanced error-handling mechanisms have been introduced to ensure that errors during billing are logged and appropriately managed, reducing the likelihood of contracts entering a dirty state.

Also, New error types were added RewardDistributionError and ContractPaymentStateNotExists.

Enhanced Events:

New events have been added, ContractGracePeriodElapsed, ContractPaymentOverdrafted, and RewardDistributed, to provide better visibility into the billing and payment processes.

ContractPaymentOverdrawn: This event is closely related to the ContractBilled event. Both events provide information about the amount due for the previous billing period. The ContractBilled event indicates that the due amount was successfully charged to the user's account. In contrast, the ContractPaymentOverdrawn event signifies that the due amount was not fully covered and has been added to the contract's overdraft. This distinction helps in accurately tracking the financial state of contracts.

RewardDistributed: This event signifies the successful distribution of rewards after the billing cycle, providing transparency and traceability for reward payouts.

ContractGracePeriodElapsed: This event is triggered when a contract has exceeded its grace period without sufficient funds, leading to its transition to a deleted state.

Other Events changes:

Locked: It have been replaced with Reserved event.
TokenBurned: The TokenBurned event is no longer emitted, reflecting the removal of TFT burning from TFChain.
Unlocked and Transfer Events: Both the Unlocked and Transfer events have been replaced with the ReserveRepatriated event.

Issues Addressed:

Issue #996

This issue has been addressed by aligning the distribution logic with the new requirements outlined in the recently accepted GEP.

Issue #991:

The PR resolves this issue by ensuring that the certified capacity is appropriately charged at a higher rate (25% more) compared to the DIY capacity.

Issue #990:

This issue is addressed by replacing the locking mechanism with a more reliable reserve/hold system, improving the overall efficiency and accuracy of the billing process. Uses reserves instead of locks to handle funds ensuring that the funds are earmarked for feature payments, preventing issues with illiquidity. Reserves align better with handling common billing scenarios such as partial payments, fund releases, and transfers more straightforwardly and reliably.

Issue #979:

The refactoring includes better handling of contracts in grace periods and ensures that proper cleanup is done for contracts that are canceled while associated with nodes in standby.

Issue #969 :

The PR fixes the issue where contracts in the grace period incorrectly required a higher amount to be available in the user balance to get restored while only deducting the due amount of the last billing cycle, rather than the whole overdraft amount from the user balance.

Issue #932 :

The PR fix bug where validators may incur fees while submitting bill_contract transactions, or fail to send the billContract transactions in other cases.

Issue #834:

The issue is resolved by accurately tracking the overdraft. The new approach ensures that the correct amount is always reserved, ensuring there are no errors in reward distribution.

Issue #994:

The PR resolves the issue where, during the grace period, contracts with public IP addresses may over-calculate the costs related to network usage (NU).

Overall, this PR enhances the billing capabilities of the pallet-smart-contract module, making it more robust and reliable.

TODO

[X] implement de-duplication at the TX pool level using a signedExtension.
[x] Harden fund distribution functions.
[x] Adjust the integration tests.
[x] Organize and reorder logs, ensuring correct severity.
[x] Clean up code.
[x] Plan changes that need to be propagated to other tools.
[x] Write new tests.
[x] Conduct thorough testing.
[x] Update Docs and ADRs

Some reconsideration might be needed, such as:

[ ] ~~Using the newer hold in the fungible trait instead of reserves in reservableCurrency, as the latter is marked for deprecation in future versions.~~
[X] Re-evaluating how to track the overdue amount in a more compatible way. This could involve monitoring the amount in a different storage map alongside the old contract lock or adding a new field to the existing structure and migrating the storage instead of lazily migrating to a new structure.

Checklist:

[x] My change requires a change to the documentation and I have updated it accordingly
[x] I have added tests to cover my changes.

iwanbk commented 1 month ago

Looks like this PR is quite big and contains many things.

Is it possible to split the PR into smaller things? Smaller PR will make it easier to test and review.

sameh-farouk commented 1 month ago

Looks like this PR is quite big and contains many things.

Is it possible to split the PR into smaller things? Smaller PR will make it easier to test and review.

Hi @iwanbk Thank you for your feedback. I understand the concern about the size of the PR. However, this PR is a comprehensive refactor of a large function, which includes modified structure, function names, and numerous fixes. Splitting it into smaller PRs would complicate the process for several reasons:

Interdependent Changes: The changes are highly interdependent. Splitting them would require tracking different changes across multiple branches, which could lead to conflicts and make the review process more cumbersome.
Testing Complexity: Testing the billing flow multiple times across different branches and then again after merging would be inefficient.
Conflict Resolution: Since all changes touch nearly the same function, merging smaller PRs would likely result in conflicts, making the process more complex, prone to errors, and time-consuming.

I believe keeping the PR as a single unit will ensure a smoother review and testing process. I am, of course, open to any specific suggestions you might have to improve the PR.

sameh-farouk commented 1 month ago

Thank you @renauter for your review! I have either addressed or replied to your notes. Please review and let me know if you have any further comments.

sameh-farouk commented 1 month ago

[2024-08-25T23:16:53Z INFO  pallet_smart_contract::migrations::v12]  >>> Unused Smart Contract pallet V12 migration
[2024-08-25T23:16:53Z INFO  try-runtime::cli] PoV size (zstd-compressed compact proof): 579.5 KB. For parachains, it's your responsibility to verify that a PoV of this size fits within any relaychain constraints.
[2024-08-25T23:16:53Z INFO  try-runtime::cli] Consumed ref_time: 1.464225s (73.21% of max 2s)
[2024-08-25T23:16:53Z INFO  try-runtime::cli] ✅ No weight safety issues detected. Please note this does not guarantee a successful runtime upgrade. Always test your runtime upgrade with recent state, and ensure that the weight usage of your migrations will not drastically differ between testing and actual on-chain execution.

Storage migration tested on Mainnet with try-runtime

renauter commented 1 month ago

Before starting to distribute rewards (which funds should come from previously reserved amounts) I feel a lack of extra check similar to ensure( account_reserved >= standard_rewards + additional_rewards).

Maybe located here: https://github.com/threefoldtech/tfchain/blob/b4431147e74987f9a1119f61910855973eae2f71/substrate-node/pallets/pallet-smart-contract/src/billing.rs#L507 With some comment like // At this point we don't expect any distribution failure since every fund to transfer should have been accumulated in account reserve along previous billing cycles

It would prevent some failure in billing logic, since funds reservation is at different phase in code (after some cycles of reseration everything reserved is transfered), and add better readability to code.

What do you think ?

sameh-farouk commented 1 month ago

Before starting to distribute rewards (which funds should come from previously reserved amounts) I feel a lack of extra check similar to ensure( account_reserved >= standard_rewards + additional_rewards).

Maybe located here:

https://github.com/threefoldtech/tfchain/blob/b4431147e74987f9a1119f61910855973eae2f71/substrate-node/pallets/pallet-smart-contract/src/billing.rs#L507

With some comment like // At this point we don't expect any distribution failure since every fund to transfer should have been accumulated in account reserve along previous billing cycles It would prevent some failure in billing logic, since funds reservation is at different phase in code (after some cycles of reseration everything reserved is transfered), and add better readability to code.

What do you think ?

I added the comment for clarity.

sameh-farouk commented 1 month ago

One last change I switched from using ValueQuery to OptionQuery for the ContractPaymentState. It is more prone to error since it always returns a default value from storage. Although we ensure that every contract has a payment state, the default value does not make sense here. Imagine calculating the cost for a contract where the last updated time in the payment state is 0!

I prefer to have a None returned and fail gracefully, rather than having a default value.

renauter commented 1 month ago

I switched from using ValueQuery to OptionQuery for the ContractPaymentState. It is more prone to error since it always returns a default value from storage.

Indeed, if we don't make a specific usage of a default value it is more convenient to use OptionQuery.

sameh-farouk commented 1 month ago

@renauter Thank you so much for reviewing! It was really helpful.

renauter commented 1 month ago

@sameh-farouk Thank you for your tremendous work!! This refactoring was a very hard task 👏🏻👏🏻👏🏻

threefoldtech / tfchain