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

Skip slot before creating inherent data providers during major sync #5344

Merged
merged 4 commits into from
Aug 25, 2024
Merged

Skip slot before creating inherent data providers during major sync #5344

merged 4 commits into from
Aug 25, 2024

Conversation

LGLO
Copy link
Contributor

@LGLO LGLO commented Aug 13, 2024 •
edited
Loading

Description

Moves create_inherent_data_provider after checking if major sync is in progress.

Integration

Change is internal to sc-consensus-slots. It should be no-op unless someone is using fork of this SDK.

Review Notes

Motivation for this change is to avoid calling create_inherent_data_providers if it's result is going to be discarded anyway during major sync. This has potential to speed up node operations during major sync by not calling possibly expensive create_inherent_data_provider.

TODO: labels T0-node D0-simple
TODO: there is no tests for Slots, should I add one for this case?

Checklist

  • My PR includes a detailed description as outlined in the "Description" and its two subsections above.
  • My PR follows the labeling requirements of this project (at minimum one label for T
    required)
    • External contributors: ask maintainers to put the right label on your PR.
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

@LGLO LGLO force-pushed the major-sync-skip-slot-before-idp branch from b68fd63 to 35ca5f7 Compare August 13, 2024 14:05
@bkchr bkchr added the T0-node This PR/Issue is related to the topic “node”. label Aug 13, 2024
@bkchr bkchr requested a review from a team August 13, 2024 22:17
@michalkucharczyk michalkucharczyk requested a review from a team August 14, 2024 07:19
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 2/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7000829

@skunert
Copy link
Contributor

skunert commented Aug 14, 2024

bot fmt

@command-bot
Copy link

command-bot bot commented Aug 14, 2024 •
edited
Loading

@skunert https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7004354 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 3-6bde3549-dd2f-4ab6-8352-d53a6613d0a4 to cancel this command or bot cancel to cancel all commands in this pull request.

@skunert skunert enabled auto-merge August 14, 2024 09:54
@command-bot
Copy link

command-bot bot commented Aug 14, 2024

@skunert Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7004354 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7004354/artifacts/download.

@bkchr
Copy link
Member

bkchr commented Aug 14, 2024

@LGLO please merge master and run cargo fmt. Ty

@github-actions GitHub Actions
Copy link

Review required! Latest push from author must always be reviewed

auto-merge was automatically disabled August 19, 2024 08:37

Head branch was pushed to by a user without write access

@LGLO
Copy link
Contributor Author

LGLO commented Aug 19, 2024

@bkchr Done, sorry for delay, I was AFK for few days.

@bkchr bkchr enabled auto-merge August 25, 2024 22:28
@bkchr bkchr added this pull request to the merge queue Aug 25, 2024
Merged via the queue into paritytech:master with commit 178e699 Aug 25, 2024
188 of 190 checks passed
ordian added a commit that referenced this pull request Aug 27, 2024
@ordian
Merge branch 'master' into ao-fix-backing-filtering-on-disputes
26dcc0c 
* master: (36 commits)
  Bump the ci_dependencies group across 1 directory with 2 updates (#5401)
  Remove deprecated calls in cumulus-parachain-system (#5439)
  Make the PR template a default for new PRs (#5462)
  Only log the propagating transactions when they are not empty (#5424)
  [CI] Fix SemVer check base commit (#5361)
  Sync status refactoring (#5450)
  Add build options to the srtool build step (#4956)
  `MaybeConsideration` extension trait for `Consideration` (#5384)
  Skip slot before creating inherent data providers during major sync (#5344)
  Add symlinks for code of conduct and contribution guidelines (#5447)
  pallet-collator-selection: correctly register weight in `new_session` (#5430)
  Derive `Clone` on `EncodableOpaqueLeaf` (#5442)
  Moving `Find FAIL-CI` check to GHA (#5377)
  Remove panic, as proof is invalid. (#5427)
  Reactive syncing metrics (#5410)
  [bridges] Prune messages from confirmation tx body, not from the on_idle (#5006)
  Change the chain to Rococo in the parachain template Zombienet config (#5279)
  Improve the appearance of crates on `crates.io` (#5243)
  Add initial version of `pallet_revive` (#5293)
  Update OpenZeppelin template documentation (#5398)
  ...
@ggwpez ggwpez mentioned this pull request Sep 9, 2024
Closed
@github-actions github-actions bot mentioned this pull request Oct 8, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bkchr bkchr bkchr approved these changes

@davxy davxy Awaiting requested review from davxy

@lexnv lexnv Awaiting requested review from lexnv

@michalkucharczyk michalkucharczyk Awaiting requested review from michalkucharczyk

@skunert skunert Awaiting requested review from skunert

Assignees
No one assigned
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@LGLO @paritytech-cicd-pr @skunert @bkchr @michalkucharczyk @davxy @lexnv