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

Modular block request handler #1524

Merged
merged 8 commits into from
Sep 15, 2023

Conversation

rahulksnv
Copy link
Contributor

@rahulksnv rahulksnv commented Sep 12, 2023

Submit the outstanding PRs from the old repos(these were already reviewed and approved before the repo rorg, but not yet submitted):
Main PR: paritytech/substrate#14014
Companion PRs: paritytech/polkadot#7134, paritytech/cumulus#2489

The changes in the PR:

  1. ChainSync currently calls into the block request handler directly. Instead, move the block request handler behind a trait. This allows new protocols to be plugged into ChainSync.
  2. BuildNetworkParams is changed so that custom relay protocol implementations can be (optionally) passed in during network creation time. If custom protocol is not specified, it defaults to the existing block handler
  3. BlockServer and BlockDownloader traits are introduced for the protocol implementation. The existing block handler has been changed to implement these traits
  4. Other changes:
    [X] Make TxHash serializable. This is needed for exchanging the serialized hash in the relay protocol messages
    [X] Clean up types no longer used(OpaqueBlockRequest, OpaqueBlockResponse)

@rahulksnv rahulksnv marked this pull request as ready for review September 12, 2023 19:50
@bkchr bkchr requested a review from a team September 13, 2023 07:42
@altonen
Copy link
Contributor

altonen commented Sep 13, 2023

I haven't looked at the changes yet but this may have to wait a little while since @dmitry-markin is doing refactoring on ChainSync, especially w.r.t. to request handling.

@nazar-pc
Copy link
Contributor

PR was out at the old repo since April, I was really hoping we can upstream this since it touches pretty fundamental internals and every time something changes upstream it is a bit of PITA to rebase it.

@altonen
Copy link
Contributor

altonen commented Sep 13, 2023

Understandable and apologies for stalling with the review of the original PR. We've finally found time to start refactoring the syncing code which is long overdue and the idea is to convert ChainSync to do no I/O or polling, which, after a brief look, is at odds with the changes of this PR. We're most likely doing a facelift for the request/response code relatively soon as well.

@dmitry-markin can you give an estimate how much work is it to rebase your changes on top of these changes?

substrate/client/network/sync/src/lib.rs Outdated Show resolved Hide resolved
substrate/client/network/sync/src/lib.rs Outdated Show resolved Hide resolved
substrate/client/network/sync/src/block_relay_protocol.rs Outdated Show resolved Hide resolved
@dmitry-markin
Copy link
Contributor

@dmitry-markin can you give an estimate how much work is it to rebase your changes on top of these changes?

I think we can merge this, the changes of this PR are mostly self contained and don't interfere in a bad way (and even simplify some things) with work on extracting request-responses. I'll just move the BlockDownloader one level up to the SyncingEngine.

As a side note, the situation around this PR seems quite weird, because the original PR was approved a long time ago, just nobody pressed the "merge" button.

@dmitry-markin dmitry-markin added the T0-node This PR/Issue is related to the topic “node”. label Sep 14, 2023
substrate/client/network/sync/src/lib.rs Outdated Show resolved Hide resolved
@rahulksnv
Copy link
Contributor Author

As a side note, the situation around this PR seems quite weird, because the original PR was approved a long time ago, just nobody pressed the "merge" button.

Apologies on that. I had a hard time getting green on all three PRs (the main one + two in the companion repos) .. by the time I get green, something would have changed and had to resync/retry. At some point I got busy with other stuff and this went to the back burner. So glad that the companion repos have been consolidated into one repo now

rahulksnv and others added 5 commits September 14, 2023 10:29
Patch the outstanding PRs from the old repos:
paritytech/substrate#14014
paritytech/polkadot#7134
paritytech/cumulus#2489

These were already reviewed and approved, but not yet
submitted.
Co-authored-by: Dmitry Markin <dmitry@markin.tech>
Co-authored-by: Dmitry Markin <dmitry@markin.tech>
Co-authored-by: Dmitry Markin <dmitry@markin.tech>
@rahulksnv
Copy link
Contributor Author

@dmitry-markin @altonen Addressed the comments, please let me know what else is needed in order to merge the PR, thanks.

@dmitry-markin
Copy link
Contributor

bot fmt

@command-bot
Copy link

command-bot bot commented Sep 15, 2023

@dmitry-markin https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3704149 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 5-6f203b54-1fd3-419a-b655-322cd76803f4 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Sep 15, 2023

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

@dmitry-markin dmitry-markin enabled auto-merge (squash) September 15, 2023 07:00
@dmitry-markin dmitry-markin merged commit b35b28c into paritytech:master Sep 15, 2023
6 checks passed
@nazar-pc
Copy link
Contributor

Thanks you for quick progress here!

@dmitry-markin
Copy link
Contributor

@dmitry-markin @altonen Addressed the comments, please let me know what else is needed in order to merge the PR, thanks.

All is good, the PR is merged. Thanks for contributing!

@rahulksnv
Copy link
Contributor Author

@dmitry-markin @altonen Addressed the comments, please let me know what else is needed in order to merge the PR, thanks.

All is good, the PR is merged. Thanks for contributing!

Thanks @dmitry-markin @altonen for the help in getting this merged

JoshOrndorff pushed a commit to Off-Narrative-Labs/Tuxedo that referenced this pull request Oct 23, 2023
JoshOrndorff added a commit to Off-Narrative-Labs/Tuxedo that referenced this pull request Oct 23, 2023
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-release-analysis-v1-2-0/4451/1

HCastano added a commit to entropyxyz/entropy-core that referenced this pull request Jan 20, 2024
HCastano added a commit to entropyxyz/entropy-core that referenced this pull request Jan 23, 2024
* Get `entropy-shared` compiling with updated deps

* Fix `entropy-shared` build for `wasm-no-std`

* Make `pallet-staking-extension` compile

Starts introducing some of the changes brought in by
paritytech/polkadot-sdk#1484.

* Introduce staking changes from paritytech/substrate#12970

* Add RuntimeFreezeReason from paritytech/polkadot-sdk#1900

* Re-map errors in staking extension pallet

* Bump `pallet-programs`

* Missed a crate dep for staking extension

* Remove commented out code

* Get `pallet-relayer` compiling

* Fix relayer test compilation

* Fix `pallet-free-tx`'s tests

* Reorder config types

* Bump `pallet-propagation` dependencies

* Bump `pallet-slashing` dependencies

* Bump `pallet-transaction-pause`

This includes the deprecation of `Balances::transfer` from
paritytech/polkadot-sdk#1226.

* TaploFmt

* Use `StakingAccount` instead of `.into()`

* Import `vec` macro in `pallet-slashing`

* Import `vec` in other pallets

* Bump TOML dependencies for runtime

Haven't fixed the Rust code yet though

* Self define types instead of using `node-primitives`

* Update staking related configs

Values were mostly taken from the Polkadot runtime config

* Update `pallet-preimage` config

Changes introduced in paritytech/polkadot-sdk#1363

* Add `MaxNominators` to `pallet_babe`

Introduced in paritytech/substrate#14471

* Add `MaxNominators` to `pallet_grandpa`

Also introduced in paritytech/substrate#14471

* Add `RuntimeFreezeReason` to `pallet_nomination_pools`

* Add `MaxTipAmount` to `pallet_tips`

Introduced in paritytech/polkadot-sdk#1709

* Add `IdentityInformation` to `pallet_identity`

Introduced in paritytech/polkadot-sdk#1661

* finish runtime

* Use `crates.io` packages in `entropy` TOML

This doesn't compile yet

* Remove reference to `node_primitives`

* Move imports under correct dependency section

These shouldn't be built-deps, my bad

* Change `WarpSyncParams` import to come from `sc_network_sync`

Change introduced here: paritytech/polkadot-sdk#1912

* Add missing `sc-consensus-babe` package

* Remove second generic from `DefaultImportQueue`

This was removed in paritytech/substrate#14612

* Add temporary cursed RPC bounds

* Switch `sp-consensus-grandpa` to be from Crates

* Add Grandpa justification period to service

* Rename `justification_period to` `justification_generation_period`

* Add missing `block_relay` param

Introduced in paritytech/polkadot-sdk#1524

* Try to clean up runtime types

* Add `opaque` types to runtime

These match what was in `node-primitives`. In particular we're interested in the `Block` type which
we were using in a lot of places, and I incorrectly changed to be `Unchechecked` instead of
`Opaque`.

* Use opaque block type in service

* Add full session key impls

* Remove cursed trait bounds for RPC

* Convert some tabs to spaces

Sorry, I had to

* Import `Vec` from `sp_std` in benchmarks

Turns out the issue responsible for this change is this:
paritytech/polkadot-sdk#172

* Pull `TrackedStorageKey` from `sp_storage`

From here: paritytech/substrate#14787

* Add `BenchmarkHelper` to treasury pallet

* TaploFmt

* change max freezes

* Sort TOML imports in runtime manifest

* Move `version` field to be first in runtime manifest

* Organize imports in pallet manifests

* Sort and organize imports for client

* Address TODOs in `entropy-shared`

* Remove TODOs related to `node-primitives`

* Pull `substrate-wasm-builder` from crates.io

* TaploFmt

* Fix benchmarking import post-merge

* Fix another program import port-merge

* Import `Vec` to programs benches

* Wrong vec import 🙃

* Poke CI

* Bump node metadata

---------

Co-authored-by: Jesse Abramowitz <jesse@entropy.xyz>
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
Submit the outstanding PRs from the old repos(these were already
reviewed and approved before the repo rorg, but not yet submitted):
Main PR: paritytech/substrate#14014
Companion PRs: paritytech/polkadot#7134,
paritytech/cumulus#2489

The changes in the PR:
1. ChainSync currently calls into the block request handler directly.
Instead, move the block request handler behind a trait. This allows new
protocols to be plugged into ChainSync.
2. BuildNetworkParams is changed so that custom relay protocol
implementations can be (optionally) passed in during network creation
time. If custom protocol is not specified, it defaults to the existing
block handler
3. BlockServer and BlockDownloader traits are introduced for the
protocol implementation. The existing block handler has been changed to
implement these traits
4. Other changes:
[X] Make TxHash serializable. This is needed for exchanging the
serialized hash in the relay protocol messages
[X] Clean up types no longer used(OpaqueBlockRequest,
OpaqueBlockResponse)

---------

Co-authored-by: Dmitry Markin <dmitry@markin.tech>
Co-authored-by: command-bot <>
AurevoirXavier added a commit to darwinia-network/darwinia that referenced this pull request Jun 21, 2024
AurevoirXavier added a commit to darwinia-network/darwinia that referenced this pull request Jun 28, 2024
* Setup deps

* Remove Koi from account migration test

* paritytech/polkadot-sdk#1495

* Bump

* paritytech/polkadot-sdk#1524

* !! paritytech/polkadot-sdk#1363

* paritytech/polkadot-sdk#1492

* paritytech/polkadot-sdk#1911

* paritytech/polkadot-sdk#1900

Signed-off-by: Xavier Lau <xavier@inv.cafe>

* paritytech/polkadot-sdk#1661

* paritytech/polkadot-sdk#2144

* paritytech/polkadot-sdk#2048

* paritytech/polkadot-sdk#1672

* paritytech/polkadot-sdk#2303

* paritytech/polkadot-sdk#1256

* Remove identity and vesting

* Fixes

* paritytech/polkadot-sdk#2657

* paritytech/polkadot-sdk#1313

* paritytech/polkadot-sdk#2331

* paritytech/polkadot-sdk#2409 part.1

* paritytech/polkadot-sdk#2767

* paritytech/polkadot-sdk#2521

Signed-off-by: Xavier Lau <xavier@inv.cafe>

* paritytech/polkadot-sdk#1222

* paritytech/polkadot-sdk#1234 part.1

* Satisfy compiler

* XCM V4 part.1

* paritytech/polkadot-sdk#1246

* Remove pallet-democracy part.1

* paritytech/polkadot-sdk#2142

* paritytech/polkadot-sdk#2428

* paritytech/polkadot-sdk#3228

* XCM V4 part.2

* Bump

* Build all runtimes

* Build node

* Remove pallet-democracy

Signed-off-by: Xavier Lau <xavier@inv.cafe>

* Format

* Fix pallet tests

* Fix precompile tests

* Format

* Fixes

* Async, remove council, common pallet config

* Fix `ethtx-forward` test case (#1519)

* Fix ethtx-forward tests

* Format

* Fix following the review

* Fixes

* Fixes

* Use default impl

* Benchmark helper

* Bench part.1

* Bench part.2

* Bench part.3

* Fix all tests

* Typo

* Feat

* Fix EVM tracing build

* Reuse upstream `proof_size_base_cost()` (#1521)

* Format issue

* Fixes

* Fix CI

---------

Signed-off-by: Xavier Lau <xavier@inv.cafe>
Co-authored-by: Bear Wang <boundless.forest@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants