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

Move import queue from ChainSync to SyncingEngine #1736

Merged
merged 3 commits into from
Sep 29, 2023
Merged

Move import queue from ChainSync to SyncingEngine #1736

merged 3 commits into from
Sep 29, 2023

Conversation

dmitry-markin
Copy link
Contributor

@dmitry-markin dmitry-markin commented Sep 28, 2023 •
edited

This PR is part of Sync 2.0 refactoring aimed at making ChainSync a pure state machine.

Resolves #501.

@dmitry-markin dmitry-markin added R0-silent Changes should not be mentioned in any release notes T0-node This PR/Issue is related to the topic “node”. labels Sep 28, 2023
@dmitry-markin dmitry-markin marked this pull request as ready for review September 28, 2023 09:58
@dmitry-markin dmitry-markin linked an issue Sep 28, 2023 that may be closed by this pull request
Move import queue out of ChainSync to SyncingEngine #501
Closed
altonen
altonen approved these changes Sep 28, 2023
View reviewed changes
substrate/client/network/common/src/sync.rs Show resolved Hide resolved
substrate/client/network/sync/src/lib.rs
enum BlockRequestEvent<B: BlockT> {
/// Action that [`engine::SyncingEngine`] should perform after reporting imported blocks with
/// [`ChainSync::on_blocks_processed`].
enum BlockRequestAction<B: BlockT> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally all these block/import/response actions would be under one SyncAction enum. SyncingEngine calls ChainSync, ChainSync pushes the actions it wants SyncingEngine to perform to its internal action queue and after each call to ChainSync, SyncingEngine would do while let Some(action) = self.chain_sync.next_action() { match action ... } until all actions are consumed. We'd get rid of the OnBlockResponse::Nothing for free.

Not in this PR though but a future task

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, but may be returning actions from every call to ChainSync instead of fetching them from next_action()? Otherwise we'll need to be extremely careful to call next_action() after every call to ChainSync.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't go with returning an action from every function. Some calls to ChainSync could result in more than one action so we'd have to return an iterator of actions from every call or make exceptions that some calls return an iterator where others return an Option<Action> and then handle those differently on the call site. Doesn't seem too elegant.

If ChainSync is only called when there's an I/O-related event, the handling of actions can be a simple loop after select!

substrate/client/network/sync/src/lib.rs Show resolved Hide resolved
substrate/client/network/sync/src/lib.rs
Comment on lines 1521 to 1523
self.network_service
.disconnect_peer(id, self.block_announce_protocol_name.clone());
self.network_service.report_peer(id, repu);
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs a deeper analysis but I'm not sure if ChainSync requires a handle to NetworkService anymore. Disconnecting & reporting a peer could be another action.

We could probably get rid of the NetworkServiceProvider hack if the handle to NetworkService was only passed to SyncingEngine::run().

Copy link
Member

Choose a reason for hiding this comment

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

Disconnecting & reporting a peer could be another action.

It's probably fine as long as these actions will be handled synchornously. Just want to make sure we won't end up with reputation changes being pushed to an async queue again. We had problems with that in the past.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact, right now they are pushed to an async queue:

polkadot-sdk/substrate/client/network/sync/src/service/network.rs

Line 83 in 0691c91

let _ = self.tx.unbounded_send(ToServiceCommand::ReportPeer(who, cost_benefit));

But luckily after removing NetworkServiceProvider we will be able to update reputations synchronously, because PeerStore (formerly PeerSet) has a sync implementation now:

polkadot-sdk/substrate/client/network/src/peer_store.rs

Line 96 in 0691c91

self.inner.lock().report_peer(peer_id, change)

@dmitry-markin
Copy link
Contributor Author

@arkpar I've assigned you as a reviewer for this PR because you made some changes to syncing in the past. Let me know if you are not the right person to ask for review of ChainSync now.

@dmitry-markin dmitry-markin merged commit 0691c91 into master Sep 29, 2023
114 of 120 checks passed
@dmitry-markin dmitry-markin deleted the dm-move-import-queue-to-syncing-engine branch September 29, 2023 14:58
ordian added a commit that referenced this pull request Oct 3, 2023
@ordian
Merge branch 'master' into tsv-disabling-node-side
a3bcb2f 
* master: (24 commits)
  Init System Parachain storage versions and add migration check jobs to CI (#1344)
  no-bound derives: Use absolute path for `core` (#1763)
  migrate alliance, fast-unstake and bags list to use derive-impl (#1636)
  Tvl pool staking (#1322)
  improve service error (#1734)
  frame-support: `RuntimeDebug\Eq\PartialEq` impls for `Imbalance` (#1717)
  Point documentation links to monorepo (#1741)
  [NPoS] Fix for Reward Deficit in the pool (#1255)
  Move import queue from `ChainSync` to `SyncingEngine` (#1736)
  Enable mocking contracts (#1331)
  Revert "fix(review-bot): pull secrets from `master` environment" (#1748)
  Remove kusama and polkadot runtime crates (#1731)
  Use `Extensions` to register offchain worker custom extensions (#1719)
  [RPC-Spec-V2] chainHead: use integer for block index and adjust RuntimeVersion JSON format (#1666)
  fix(review-bot): pull secrets from `master` environment (#1745)
  Fix `subkey inspect` output text padding (#1744)
  archive: Implement height, hashByHeight and call (#1582)
  rpc/client: Propagate `rpc_methods` method to reported methods (#1713)
  rococo-runtime: `RococoGenesisExt` removed (#1490)
  PVF: more filesystem sandboxing (#1373)
  ...
ordian added a commit that referenced this pull request Oct 10, 2023
@ordian
Merge branch 'tsv-disabling-node-side' into tsv-disabling-backing
863fc7d 
* tsv-disabling-node-side: (24 commits)
  Init System Parachain storage versions and add migration check jobs to CI (#1344)
  no-bound derives: Use absolute path for `core` (#1763)
  migrate alliance, fast-unstake and bags list to use derive-impl (#1636)
  Tvl pool staking (#1322)
  improve service error (#1734)
  frame-support: `RuntimeDebug\Eq\PartialEq` impls for `Imbalance` (#1717)
  Point documentation links to monorepo (#1741)
  [NPoS] Fix for Reward Deficit in the pool (#1255)
  Move import queue from `ChainSync` to `SyncingEngine` (#1736)
  Enable mocking contracts (#1331)
  Revert "fix(review-bot): pull secrets from `master` environment" (#1748)
  Remove kusama and polkadot runtime crates (#1731)
  Use `Extensions` to register offchain worker custom extensions (#1719)
  [RPC-Spec-V2] chainHead: use integer for block index and adjust RuntimeVersion JSON format (#1666)
  fix(review-bot): pull secrets from `master` environment (#1745)
  Fix `subkey inspect` output text padding (#1744)
  archive: Implement height, hashByHeight and call (#1582)
  rpc/client: Propagate `rpc_methods` method to reported methods (#1713)
  rococo-runtime: `RococoGenesisExt` removed (#1490)
  PVF: more filesystem sandboxing (#1373)
  ...
@kiltbot kiltbot mentioned this pull request Nov 7, 2023
Closed
@ahmadkaouk ahmadkaouk mentioned this pull request Nov 16, 2023
Closed
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
@dmitry-markin
Move import queue from ChainSync to SyncingEngine (paritytech#1736)
c098425 
This PR is part of [Sync
2.0](paritytech#534) refactoring
aimed at making `ChainSync` a pure state machine.

Resolves paritytech#501.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@altonen altonen altonen approved these changes

@arkpar arkpar arkpar approved these changes

Assignees
No one assigned
Labels
R0-silent Changes should not be mentioned in any release notes T0-node This PR/Issue is related to the topic “node”.
Projects
Networking
Status: Done ✅
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move import queue out of ChainSync to SyncingEngine
3 participants
@dmitry-markin @arkpar @altonen