feat(storage): check and clean for pallet smart contract #659

renauter · 2023-04-07T16:00:51Z

Related to:

renauter · 2023-04-14T17:59:47Z

To run the "migration" live against Devnet do:

cargo run --release --features=try-runtime try-runtime --runtime target/release/wbuild/tfchain-runtime/tfchain_runtime.compact.wasm --chain chainspecs/main/chainSpecRaw.json on-runtime-upgrade live --uri ws://10.10.0.151:9944

Note that the process consists in:
(1) check storage and display the warnings
(2) clean storage
(3) re-check storage with exact same code as (1) and no more warnings should remain

DylanVerstraete

Where is the new storage version actually set?

DylanVerstraete · 2023-04-17T14:36:15Z

substrate-node/pallets/pallet-smart-contract/src/storage_state/v9.rs

+
+impl<T: Config> OnRuntimeUpgrade for CheckStorageStateV9<T> {
+    fn on_runtime_upgrade() -> Weight {
+        if PalletVersion::<T>::get() == types::StorageVersion::V9 {


This won't work on all networks, since the version on qa/test/main is v8

renauter · 2023-04-17T14:57:41Z

Where is the new storage version actually set?

It is not set because there is no modification of the structure, and this check/clean is valid for v9

This won't work on all networks, since the version on qa/test/main is v8

Indeed but the purpose of this check/clean is related to a specific version.
When we will upgrade qa/test/main it will go to v9 and the "migration" can be applied

Where is the pre and post check?

pre and post check are used by testing tools and not triggered when running command

cargo run --release --features=try-runtime try-runtime --runtime target/release/wbuild/tfchain-runtime/tfchain_runtime.compact.wasm --chain chainspecs/main/chainSpecRaw.json on-runtime-upgrade live --uri ws://10.10.0.151:9944

I wanted to integrate them by using try_on_runtime_upgrade() with no success
So for now on_runtime_upgrade() has check + clean + check
Open to better solution

… compatible with v8

DylanVerstraete · 2023-04-18T07:35:38Z

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

@@ -25,7 +25,7 @@ pub enum StorageVersion {

 impl Default for StorageVersion {
    fn default() -> StorageVersion {
-        StorageVersion::V9
+        StorageVersion::V8


Since we are going to V9, default is now V9

renauter · 2023-04-18T15:28:11Z

I added the weights and made it compatible with v8 and v9
Actually v8 is on qa/test/mainnet and v9 on devnet
But v9 (which is a billing loop cleaning) can be discarded since the checking/cleaning in this PR already includes what v9 do

Run on:

Devnet
QAnet
Testnet
Mainnet

And no remaining warnings after cleaning

renauter · 2023-04-18T15:35:08Z

substrate-node/pallets/pallet-smart-contract/src/storage_state/v8.rs

+        if !ContractLock::<T>::contains_key(contract_id) {
+            let now = <timestamp::Pallet<T>>::get().saturated_into::<u64>() / 1000;
+            r += 1;
+            let mut contract_lock = types::ContractLock::default();
+            contract_lock.lock_updated = now;
+            ContractLock::<T>::insert(contract_id, contract_lock);
+            w += 1;
+        }


Not sure it is the right thing to do here, in case there is no contract lock associated to a contract

DylanVerstraete · 2023-04-25T10:35:02Z

Current weight is above the block threshold:

TryRuntime_on_runtime_upgrade executed without errors. Consumed weight = (2462275000000 ps, 0 byte), total weight = (2000000000000 ps, 18446744073709551615 byte) (123.11 %, 0.00 %).```

-> 123%

DylanVerstraete · 2023-04-25T11:01:40Z

@renauter I refactored the logging, and set all logs to debug. You can now see all the logs from your migration like:

RUST_LOG=debug cargo run --release --features=try-runtime try .......

Because logging in production also takes up compute time.

I also put back the Post/Pre checks. I know why they were not firing, see:

tfchain/substrate-node/runtime/src/lib.rs

Line 989 in fba8109

let weight = Executive::try_runtime_upgrade(true).unwrap();

DylanVerstraete · 2023-04-25T11:03:02Z

Now that we know that the storage migration takes 123% of a total blocks weight we need to think about execution strategy. Either we optimize the migration code OR we do a rolling migration that is triggered by an extrinsic?

renauter · 2023-04-26T02:16:50Z

I also put back the Post/Pre checks. I know why they were not firing, see:

Nice! So I guess I can remove the weights from the checking process

renauter · 2023-04-26T02:35:53Z

Now that we know that the storage migration takes 123% of a total blocks weight we need to think about execution strategy. Either we optimize the migration code OR we do a rolling migration that is triggered by an extrinsic?

I just re-checked and as the checking process is now done on pre/post checks, as it should be (because never meant to be executed on-chain but is meant to be used by testing tools), the 123% decreases to 30%:

TryRuntime_on_runtime_upgrade executed without errors. Consumed weight = (604750000000 ps, 0 byte), total weight = (2000000000000 ps, 18446744073709551615 byte) (30.24 %, 0.00 %).

Which corresponds to the cleaning process alone that would be performed on-chain.
So no need to formulate a strategy, correct?

DylanVerstraete · 2023-04-26T07:43:17Z

@renauter on which network did you execute to get a 30% blockweight execution time? It's crucial this is measured against mainnet (which is the biggest chain in terms of data)

renauter · 2023-04-26T12:37:13Z

@renauter on which network did you execute to get a 30% blockweight execution time? It's crucial this is measured against mainnet (which is the biggest chain in terms of data)

Indeed, was on devnet... my bad
With the current code we have:

Devnet => 30%
QAnet => 13%
Testnet => 43%
Mainnet => 123%

So for the strategies:

Optimize migration code
I gonna check but at first sight seems complicated to reduce
Rolling migration that is triggered by an extrinsic
not sure if I get this one because the extrinsic will remain right?
maybe not the best in terms of impact

There is no other way to run the migration in 2 steps?
ex: we split migration code in 2 (v9_1 and v9_2) and we run them at different moment (block)

DylanVerstraete · 2023-04-26T12:50:03Z

check https://substrate.stackexchange.com/questions/1329/how-to-perform-a-large-migration

renauter · 2023-04-27T18:54:15Z

@DylanVerstraete out of reach in Telegram
which is blocked in Brazil since yesterday
https://apnews.com/article/brazil-telegram-suspension-social-media-school-violence-d72acaacd3c1b4d07c2c4fcb094f4ce6
I send alternative via gtalk

DylanVerstraete · 2023-04-27T18:59:36Z

Oh damn, that some hard censorship right there

renauter · 2023-04-27T19:02:05Z

Oh damn, that some hard censorship right there

Indeed, It s the second time in 1 year ...

renauter · 2023-05-11T19:31:03Z

I adapted the strategy of the migration code to be able to simulate the rolling migration and check if storage was effectively cleaned by executing the following steps:

(1) run a local node with the actual (forked) chain_specs of dev/test/mainnet

(2) run a try-runtime operation on local node with the checking code in pre_upgrade() to check storage BEFORE cleaning

RUST_LOG=debug cargo run --release --features=try-runtime try-runtime --runtime target/release/wbuild/tfchain-runtime/tfchain_runtime.compact.wasm --chain chainspecs/main/chainSpecRaw.json on-runtime-upgrade live --uri ws://localhost:9944

(3) do a runtime upgrade using set_code() extrinsic (via polkadot.js UI) to start the rolling migration of the storage cleaning

(4) when migration ended, repeat step (2) to check storage AFTER cleaning

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

brandonpille

Great work. Some comments:

substrate-node/pallets/pallet-smart-contract/src/storage_state/v9.rs

brandonpille · 2023-05-22T13:38:49Z

substrate-node/pallets/pallet-smart-contract/src/storage_state/v9.rs

+                contract_id, contract.contract_id
+            );
+        }
+        if !contract_id_range.contains(&contract_id) {


I'm not sure I get why you are checking if the id is in the range. It's always the case no? And if it would not be the case why not do:

let current_contract_id = ContractID::<T>::get();

to then replace this if by:

if contract_id >= current_contract_id

I'm not sure I get why you are checking if the id is in the range. It's always the case no?

Indeed, it should always be the case that s why I am checking it to identify bad cases that could result from old code.
We never know, I have seen some unexpected cases investigating the storage.

And if it would not be the case why not do: ...

Because I also want to check contract_id > 0 so using the range I check both at the same time with same complexity I guess

But ContractID is u64 so it can never be < 0... It will always be >= 0

Indeed we still want to identify contract_id == 0 case

Yes but that simplifies the check to

if contract_id == 0 { // log }

Yes, I could have done

if contract_id == 0 || contract_id > ContractID::<T>::get() { // log }

but I found it more elegant to do

let contract_id_range = 1..=ContractID::<T>::get(); if !contract_id_range.contains(&contract_id) { // log }

I think complexity is same, correct?

substrate-node/pallets/pallet-smart-contract/src/storage_state/v9.rs

LeeSmet · 2023-05-23T15:34:24Z

substrate-node/pallets/pallet-smart-contract/src/migrations/v9.rs

+// ✅ ContractsToBillAt
+// ✅ Contracts
+// ✅ ActiveNodeContracts
+// ✅ ActiveRentContractForNode
+// ✅ ContractIDByNodeIDAndHash
+// ✅ ContractIDByNameRegistration
+// ✅ ContractLock
+// ✅ SolutionProviders
+// ✅ ContractBillingInformationByID
+// ✅ NodeContractResources


Remove commented code

LeeSmet · 2023-05-23T15:40:21Z

substrate-node/pallets/pallet-smart-contract/src/migrations/v9.rs

-            "🔎 CleanBillingLoop pre migration: Number of existing billing loop indexes {:?}",
-            contracts_to_bill_count
+fn check_node_contract<T: Config>(node_id: u32, contract_id: u64, deployment_hash: HexHash) {
+    if let Some(_) = pallet_tfgrid::Nodes::<T>::get(node_id) {


The contained value of the Some variant is not used, so this check only cares if the key exists at all, which according to the docs can be checked as:

Suggested change

if let Some(_) = pallet_tfgrid::Nodes::<T>::get(node_id) {

if pallet_tfgrid::Nodes::<T>::contains_key(node_id) {

indeed
which is also equivalent to:

if pallet_tfgrid::Nodes::<T>::get(node_id).is_some() {

correct?

LeeSmet · 2023-05-23T15:44:23Z

substrate-node/pallets/pallet-smart-contract/src/migrations/v9.rs

-            pre_contracts_to_bill_count,
-            "Number of billing loop indexes migrated does not match"
+fn check_rent_contract<T: Config>(node_id: u32, contract: &types::Contract<T>) {
+    if let Some(_) = pallet_tfgrid::Nodes::<T>::get(node_id) {


Suggested change

if let Some(_) = pallet_tfgrid::Nodes::<T>::get(node_id) {

if pallet_tfgrid::Nodes::<T>::contains_key(node_id) {

…e_checking

feat: smart contract pallet storage check wip

6d45330

renauter changed the title ~~feat: smart contract pallet storage check~~ feat: storage check fot pallet smart contract Apr 7, 2023

renauter added 4 commits April 11, 2023 08:33

feat: more contract storage check

926cfe1

feat: cleaning billing loop storage

58437b9

feat: cleaning pallet smart contract storages wip

6b3211c

feat: cleaning ContractIDByNodeIDAndHash storage

2808279

renauter marked this pull request as ready for review April 14, 2023 17:52

renauter requested review from DylanVerstraete, brandonpille, robvanmieghem and LeeSmet as code owners April 14, 2023 17:52

feat(storage): remove contracts with default deployment hash

869f715

renauter changed the title ~~feat: storage check fot pallet smart contract~~ feat(storage): check and clean for pallet smart contract Apr 17, 2023

DylanVerstraete reviewed Apr 17, 2023

View reviewed changes

renauter added 4 commits April 17, 2023 17:15

feat(storage): remove v9 migration and make storage checking/cleaning…

b3bca96

… compatible with v8

feat(storage): add default contract lock if there is not

19d7151

feat(storage): handle Contracts storage before billing loop

cea5be9

feat(storage): add weights wip

a4c81b0

DylanVerstraete reviewed Apr 18, 2023

View reviewed changes

renauter added 2 commits April 18, 2023 11:15

feat(storage): set default storage version to v9

f6f431c

feat(storage): add weights to checking process

86bf4c1

renauter commented Apr 18, 2023

View reviewed changes

chore: rework logging and add pre/post checks

fba8109

chore: remove weights from checking process

e21bd96

chore: make use of saturating concept for weight calculation

00bffd4

renauter added 3 commits May 3, 2023 23:40

chore: try a rolling migration over multiple blocks

999259a

chore: allow triggering the rolling migration

8979044

chore: allow storage cleaning and checking to be triggered separately

cc15fc6

DylanVerstraete reviewed May 22, 2023

View reviewed changes

substrate-node/pallets/pallet-smart-contract/src/lib.rs Outdated Show resolved Hide resolved

brandonpille reviewed May 22, 2023

View reviewed changes

renauter added 3 commits May 22, 2023 22:58

fix: remove unnecessary cfg

a88f071

migration review

eefcc44

chore: move v9 file to migartions folder

615744e

LeeSmet requested changes May 23, 2023

View reviewed changes

renauter added 6 commits May 23, 2023 21:08

chore: review

93ddf2b

chore: add ADR file

33959fc

chore: remove ADR file

def5a76

feat: remove no ips / no resource restriction

556b519

fix: billing loop cleaning

4afc95d

Merge remote-tracking branch 'origin' into development_runtime_storag…

51834b4

…e_checking

DylanVerstraete merged commit 704fc01 into development May 25, 2023
2 checks passed

DylanVerstraete deleted the development_runtime_storage_checking branch May 25, 2023 06:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(storage): check and clean for pallet smart contract #659

feat(storage): check and clean for pallet smart contract #659

renauter commented Apr 7, 2023 •

edited

renauter commented Apr 14, 2023 •

edited

DylanVerstraete left a comment

DylanVerstraete Apr 17, 2023

renauter commented Apr 17, 2023 •

edited

DylanVerstraete Apr 18, 2023

renauter commented Apr 18, 2023 •

edited

renauter Apr 18, 2023

DylanVerstraete commented Apr 25, 2023 •

edited

DylanVerstraete commented Apr 25, 2023

DylanVerstraete commented Apr 25, 2023

renauter commented Apr 26, 2023

renauter commented Apr 26, 2023

DylanVerstraete commented Apr 26, 2023

renauter commented Apr 26, 2023

DylanVerstraete commented Apr 26, 2023

renauter commented Apr 27, 2023

DylanVerstraete commented Apr 27, 2023

renauter commented Apr 27, 2023

renauter commented May 11, 2023

brandonpille left a comment

brandonpille May 22, 2023

renauter May 23, 2023 •

edited

brandonpille May 23, 2023

renauter May 23, 2023

brandonpille May 23, 2023

renauter May 23, 2023

LeeSmet May 23, 2023

LeeSmet May 23, 2023

renauter May 23, 2023

LeeSmet May 23, 2023

	if let Some(_) = pallet_tfgrid::Nodes::<T>::get(node_id) {
	if pallet_tfgrid::Nodes::<T>::contains_key(node_id) {

feat(storage): check and clean for pallet smart contract #659

feat(storage): check and clean for pallet smart contract #659

Conversation

renauter commented Apr 7, 2023 • edited

renauter commented Apr 14, 2023 • edited

DylanVerstraete left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renauter commented Apr 17, 2023 • edited

Choose a reason for hiding this comment

renauter commented Apr 18, 2023 • edited

Choose a reason for hiding this comment

DylanVerstraete commented Apr 25, 2023 • edited

DylanVerstraete commented Apr 25, 2023

DylanVerstraete commented Apr 25, 2023

renauter commented Apr 26, 2023

renauter commented Apr 26, 2023

DylanVerstraete commented Apr 26, 2023

renauter commented Apr 26, 2023

DylanVerstraete commented Apr 26, 2023

renauter commented Apr 27, 2023

DylanVerstraete commented Apr 27, 2023

renauter commented Apr 27, 2023

renauter commented May 11, 2023

brandonpille left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renauter May 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renauter commented Apr 7, 2023 •

edited

renauter commented Apr 14, 2023 •

edited

renauter commented Apr 17, 2023 •

edited

renauter commented Apr 18, 2023 •

edited

DylanVerstraete commented Apr 25, 2023 •

edited

renauter May 23, 2023 •

edited