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.

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

Test: Offchain workers disabled by default for container chains #723

Merged

Conversation

chexware
Copy link
Contributor

@chexware chexware commented Oct 16, 2024

Adding tests to verify that offchain workers are disabled by default for container chains. Created dummy offchain worker pallet that submits an unsigned extrinsic and emits an event. The pallet is added it to the simple container chain template and offchain actions must be disabled by default.

TO DO:

  • Add storage variable that enables disables calling offchain workers extrinsics. Set it to false by default
  • Add usigned extrinsics that emits event an can be called from the offchain worker if enabled
  • Update the tests to verify that the offchain worker pallet won't emit events after it is enabled

@chexware chexware changed the title Offchain workers disabled by default for container chains Test: Offchain workers disabled by default for container chains Oct 16, 2024
@chexware chexware added B0-silent Changes should not be mentioned in any release notes not-breaking Does not need to be mentioned in breaking changes D2-notlive PR doesn't change runtime/client code labels Oct 23, 2024
Copy link
Contributor

github-actions bot commented Oct 23, 2024

WASM runtime size check:

Compared to target branch

dancebox runtime: 1412 KB (no changes) ✅

flashbox runtime: 832 KB (no changes) ✅

dancelight runtime: 2008 KB (no changes) ✅

container chain template simple runtime: 1088 KB (no changes) ✅

container chain template frontier runtime: 1388 KB (no changes) ✅

Copy link
Contributor

github-actions bot commented Oct 23, 2024

Coverage Report

(master)

@@                                    Coverage Diff                                     @@
##           master   aleks-container-chains-disabled-offchain-workers-check      +/-   ##
==========================================================================================
- Coverage   65.56%                                                   65.44%   -0.12%     
+ Files         308                                                      309       +1     
+ Lines       53879                                                    53985     +106     
==========================================================================================
+ Hits        35325                                                    35328       +3     
+ Misses      18554                                                    18657     +103     
Files Changed Coverage
/container-chains/runtime-templates/simple/src/lib.rs 87.25% (+0.02%)

Coverage generated Wed Nov 6 13:49:22 UTC 2024

@girazoki
Copy link
Contributor

I think we need to test this better. I think we trully need to understand what the offchain worker does, which for starters should call a extrinsic of some sort to emit an event and you be able to detect it. In its current state I dont think we are testing much...

I think the "easiest" way to test this would be:

  • add a storage item that enables the offchainWorker call or not in the pallet.
  • add a extrinsic that the offchain worker calls and emits the event
  • call this from unsigned?

@chexware chexware marked this pull request as ready for review October 30, 2024 15:28
…hen disabling offchain worker using the switch extrinsic
@chexware chexware added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes and removed B0-silent Changes should not be mentioned in any release notes labels Oct 30, 2024
@tmpolaczyk
Copy link
Contributor

I was able to trigger a test failure by enabling offchain workers like this, good job!

diff --git a/client/node-common/src/service.rs b/client/node-common/src/service.rs
index 9500a431..cad274a0 100644
--- a/client/node-common/src/service.rs
+++ b/client/node-common/src/service.rs
@@ -552,7 +552,7 @@ where
 
         let collator = parachain_config.role.is_authority();
 
-        if parachain_config.offchain_worker.enabled {
+        if parachain_config.offchain_worker.enabled || true {
             task_manager.spawn_handle().spawn(
                 "offchain-workers-runner",
                 "offchain-work",

@chexware chexware merged commit 2fd4532 into master Nov 6, 2024
45 checks passed
@chexware chexware deleted the aleks-container-chains-disabled-offchain-workers-check branch November 6, 2024 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes breaking Needs to be mentioned in breaking changes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants