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

AuthorNoting should only deal with parachains with assigned collators (instead of all) #884

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

nanocryk
Copy link
Contributor

@nanocryk nanocryk commented Feb 24, 2025

Currently the runtime checks that the inherents contain the relay proof of all registered parachains to notice new blocks.

If a chain doesn't have assigned collators, we know there can't be new blocks, so it's safe to skip them in the runtime, and thus not insert them in the inherents. This PR thus updates both runtime and client sides to only check/insert inherents related to parachains with assigned collators.

Copy link
Contributor

github-actions bot commented Feb 24, 2025

WASM runtime size check:

Compared to target branch

dancebox runtime: 1428 KB (no changes) ✅

flashbox runtime: 848 KB (no changes) ✅

dancelight runtime: 2180 KB (no changes) ✅

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

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

Copy link
Contributor

github-actions bot commented Feb 24, 2025

Coverage Report

(master)

@@                                 Coverage Diff                                 @@
##           master   jeremy-author-noting-only-chains-with-collators      +/-   ##
===================================================================================
- Coverage   65.57%                                            65.54%   -0.03%     
  Files         351                                               351              
+ Lines       61167                                             61236      +69     
===================================================================================
+ Hits        40107                                             40136      +29     
+ Misses      21060                                             21100      +40     
Files Changed Coverage
/chains/orchestrator-paras/node/src/service.rs 15.42% (-0.12%)
/chains/orchestrator-paras/runtime/dancebox/src/lib.rs 87.80% (-0.26%)
/chains/orchestrator-paras/runtime/flashbox/src/lib.rs 46.03% (-0.92%)
/pallets/author-noting/src/lib.rs 85.71% (-1.51%)
/pallets/collator-assignment/src/lib.rs 98.21% (-0.48%)

Coverage generated Mon Mar 3 16:00:16 UTC 2025

@nanocryk nanocryk marked this pull request as ready for review February 26, 2025 11:35
Copy link
Contributor

@girazoki girazoki left a comment

Choose a reason for hiding this comment

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

Changes overall look good! small comments

@girazoki
Copy link
Contributor

Actually I just realized there is one change we need to do:

registered_paras() currently tries to detect whether the next block is going to change the session. If so, we use the scheduled registered paras for the next session. See

fn registered_paras() -> Vec<ParaId> {

The reason behind this is that runtime-apis are called with the state on-finalize of the previous block. And if the previous block is a session-boundary, we will have the wrong things injected here.

MY suggestion is that you keep the new parachains_with_some_collators. Meaning if it's a session boundary block, we take the nextPendingCollators or whatever the name is

@girazoki girazoki self-requested a review February 26, 2025 16:17
@tmpolaczyk
Copy link
Contributor

@nanocryk dev_tanssi tests are failing because the author noting weight changed. Could you update the test to use this syntax? So that we see the new value the next time the test fails:

const min = (CLEAR_ORIGIN_WEIGHT * 90n) / 100n;
const max = (CLEAR_ORIGIN_WEIGHT * 110n) / 100n;

expect(
    weight,
    `weightMessage returned ${weight} but expected a value between ${min} and ${max}`
).to.satisfy((val) => val >= min && val <= max);

@girazoki
Copy link
Contributor

@nanocryk dev_tanssi tests are failing because the author noting weight changed. Could you update the test to use this syntax? So that we see the new value the next time the test fails:

const min = (CLEAR_ORIGIN_WEIGHT * 90n) / 100n;
const max = (CLEAR_ORIGIN_WEIGHT * 110n) / 100n;

expect(
    weight,
    `weightMessage returned ${weight} but expected a value between ${min} and ${max}`
).to.satisfy((val) => val >= min && val <= max);

this is actually a good sign, I do believe that we have 2 registered chains but only 1 gets collators. If that is the case the weight reduction would make sense. Can you check that?

@girazoki girazoki self-requested a review February 26, 2025 17:02
@nanocryk nanocryk added A8-mergeoncegreen Pull request is reviewed well. B3-apinoteworthy Changes should be mentioned in the release notes of the next minor version release. B5-clientnoteworthy Changes should be mentioned in any downstream projects' release notes B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited not-breaking Does not need to be mentioned in breaking changes labels Mar 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A8-mergeoncegreen Pull request is reviewed well. B3-apinoteworthy Changes should be mentioned in the release notes of the next minor version release. B5-clientnoteworthy Changes should be mentioned in any downstream projects' release notes B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited not-breaking Does not need to be mentioned in breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants