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

Add Rococo People <> Rococo Bulletin bridge support to Rococo Bridge Hub #2540

Merged
merged 15 commits into from
Dec 14, 2023

Conversation

svyatonik
Copy link
Contributor

@svyatonik svyatonik commented Nov 29, 2023

This PR adds Rococo People <> Rococo Bulletin to the Rococo Bridge Hub code. There's a couple of things left to do here:

  • add remaining tests - it'd need some refactoring in the bridge-hub-test-utils - will do in a separate PR;
  • actually run benchmarks for new messaging pallet (do we have bot nowadays?).

The reason why I'm opening it before this ^^^ is ready, is that I'd like to hear others opinion on how to deal with hacks with that bridge. Initially I was assuming that Rococo Bulletin will be the 1:1 copy of the Polkadot Bulletin (to avoid maintaining multiple runtimes/releases/...), so you can see many PolkadotBulletin mentions in this PR, even though we are going to bridge with the parallel chain (RococoBulletin). That's because e.g. pallet names from construct_runtime are affecting runtime storage keys and bridges are using runtime storage proofs => it is important to use names that the Bulletin chain expects.

But in the end, this hack won't work - we can't use Polkadot Bulletin runtime to bridge with Rococo Bridge Hub, because Polkadot Bulletin expects Polkadot Bridge hub to use 1002 parachain id and Rococo Bridge Hub seats on the 1013. This also affects storage keys using in bridging, so I had to add the rococo feature to the Bulletin chain. So now we can actually alter its runtime and adapt it for Rococo.

So the question here is - what's better for us here

  • to leave everything as is (seems hacky and non-trivial);
  • change Bulletin chain runtime when rococo feature is used - e.g. use proper names there (WithPolkadotGrandpa -> WithRococoGrandpa, ...)
  • add another set of pallets to the Bulletin chain runtime to bridge with Rococo and never use them in production. Similar to hack that we had in Rococo/Wococo

cc @acatangiu @bkontur @serban300

also cc @joepetrowski as the main "client" of this bridge


A couple words on how this bridge is different from the Rococo <> Westend bridge:

  • it is a bridge with a chain that uses GRANDPA finality, not the parachain finality (hence the tests needs to be changed);
  • it is a fee-free bridge. So AllowExplicitUnpaidExecutionFrom<Equals<SiblingPeople>> + we are not paying any rewards to relayers (apart from compensating transaction costs).

@svyatonik svyatonik added R0-silent Changes should not be mentioned in any release notes T15-bridges This PR/Issue is related to bridges. labels Nov 29, 2023
@svyatonik svyatonik requested a review from a team November 29, 2023 11:25
Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

Haven't looked at the tests, but everything else looks good as is.

@acatangiu
Copy link
Contributor

So the question here is - what's better for us here
- to leave everything as is (seems hacky and non-trivial)
- change Bulletin chain runtime when rococo feature is used - e.g. use proper names there (WithPolkadotGrandpa -> WithRococoGrandpa, ...)
- add another set of pallets to the Bulletin chain runtime to bridge with Rococo and never use them in production. Similar to hack that we had in Rococo/Wococo

If I understand correctly, the question only affects the Bulletin chain runtime (which is deployed on both Polkadot and Rococo), and how it defines/sees the bridges (to PolkadotBH or RococoBH depending on where itself is deployed), right?

IMO any of the 3 options is good. I expect that when Bulletin chain will be launched, it will also be moved to fellowship and we'll keep a Rococo copy here, and the problem "goes away". So in the meantime, I'm fine with whatever option is easiest for us :)

…bridge_to_bulletin_config.rs

Co-authored-by: Adrian Catangiu <adrian@parity.io>
@svyatonik
Copy link
Contributor Author

So the question here is - what's better for us here

  • to leave everything as is (seems hacky and non-trivial)
  • change Bulletin chain runtime when rococo feature is used - e.g. use proper names there (WithPolkadotGrandpa -> WithRococoGrandpa, ...)
  • add another set of pallets to the Bulletin chain runtime to bridge with Rococo and never use them in production. Similar to hack that we had in Rococo/Wococo

If I understand correctly, the question only affects the Bulletin chain runtime (which is deployed on both Polkadot and Rococo), and how it defines/sees the bridges (to PolkadotBH or RococoBH depending on where itself is deployed), right?

IMO any of the 3 options is good. I expect that when Bulletin chain will be launched, it will also be moved to fellowship and we'll keep a Rococo copy here, and the problem "goes away". So in the meantime, I'm fine with whatever option is easiest for us :)

Ok, thanks. I'm leaving as is, because it is already deployed somewhere

svyatonik added a commit that referenced this pull request Dec 13, 2023
So far the `bridge-hub-test-utils` contained a tests set for testing
BridgeHub runtime that is bridging with the remote **parachain**. But we
have #2540 coming, which
would add Rococo <> Bulletin chain bridge (where Bulletin = standalone
chain that is using GRANDPA finality). Then it'll be expanded to
Polkadot BH as well.

So this PR adds the same set of tests to the `bridge-hub-test-utils`,
but for the case when remote chain is the chain with GRANDPA finality.
There's a lot of changes in this PR - I'll describe some:
- I've added `BasicParachainRuntime` trait to decrease number of lines
we need to add to `where` clause. Could revert, but imo it is useful;
- `cumulus/parachains/runtimes/bridge-hubs/test-utils/src/test_data` is
a submodule for generating test data for the test sets.
`from_parachain.rs` is used in tests for the case when remote chain is a
parachain, `from_grandpa_chain.rs` - for the bridges with remote GRANDPA
chains. `mod.rs` has some code, shared by both types of tests;
- `cumulus/parachains/runtimes/bridge-hubs/test-utils/src/test_data` is
a submodule with all test cases. The `mod.rs` has tests, suitable for
all cases. There's also `wth_parachain.rs` and `with_grandpa_chain.rs`
with the same meaning as above;
- I've merged the "core" code of two previous tests -
`relayed_incoming_message_works` and `complex_relay_extrinsic_works`
into one single `relayed_incoming_message_works` test. So now we are
always constructing extrinsics and are dispatching them using executive
module (meaning all signed extensions are also tested).

New test set is used here:
#2540. Once this PR is
merged, I'll merge that other PR with master to remove duplicate
changes.

I'm also planning to cleanup generic constraints + remove some
unnecessary assumptions about used chains in a follow-up PRs. But for
now I think this PR has enough changes, so don't want to complicate it
even more.

---

Breaking changes for the code that have used those tests before:
- the `construct_and_apply_extrinsic` callback now accepts the
`RuntimeCall` instead of the `pallet_utility::Call`;
- the `construct_and_apply_extrinsic` now may be called multiple times
for the single test, so make sure the `frame_system::CheckNonce` is
correctly constructed;
- all previous tests have been moved from
`bridge_hub_test_utils::test_cases` to
`bridge_hub_test_utils::test_cases::from_parachain` module;
- there are several changes in test arguments - please refer to
https://github.com/paritytech/polkadot-sdk/compare/sv-tests-for-bridge-with-remote-grandpa-chain?expand=1#diff-79a28d4d3e1749050341c2424f00c4c139825b1a20937767f83e58b95166735c
for details.
Copy link
Contributor

@bkontur bkontur left a comment

Choose a reason for hiding this comment

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

nice, lgtm, just few very nits which will be solved after rebase to actual master

@@ -1004,6 +1063,41 @@ impl_runtime_apis! {
}
}

impl BridgeMessagesConfig<bridge_to_bulletin_config::WithRococoBulletinMessagesInstance> for Runtime {
fn is_relayer_rewarded(_relayer: &Self::AccountId) -> bool {
// we do not pay any rewards in this bridge
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: is_relayer_rewarded == true => "we do not pay any rewards" is little bit kind of confusing, but probably we can live with that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, we can't test that the relayer is rewarded if we don't pay any rewards :) I could change a comment, if you have some better version :)

// - BridgeWestendParachains
// - BridgeWestendMessages
// - BridgeRelayers
// Bridge relayers pallet, used by several bridges here.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed the grouping here - this runtime is no longer shared by Rococo and Wococo, so imo it makes sense to group by bridged chain.

@svyatonik
Copy link
Contributor Author

bot bench cumulus-bridge-hubs --runtime=bridge-hub-rococo --pallet=pallet_bridge_messages

@command-bot
Copy link

command-bot bot commented Dec 14, 2023

@svyatonik "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=bridge-hub-rococo --runtime_dir=bridge-hubs --target_dir=cumulus --pallet=pallet_bridge_messages (https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4724228) was cancelled in #2540 (comment)

@svyatonik
Copy link
Contributor Author

bot cancel 5-a34ea1ac-b42a-424e-ac12-0d58da204a4d

@command-bot
Copy link

command-bot bot commented Dec 14, 2023

@svyatonik Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=bridge-hub-rococo --runtime_dir=bridge-hubs --target_dir=cumulus --pallet=pallet_bridge_messages has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4724228 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4724228/artifacts/download.

@svyatonik
Copy link
Contributor Author

bot bench cumulus-bridge-hubs --runtime=bridge-hub-rococo --pallet=pallet_bridge_messages

@command-bot
Copy link

command-bot bot commented Dec 14, 2023

@svyatonik https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4725738 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=bridge-hub-rococo --runtime_dir=bridge-hubs --target_dir=cumulus --pallet=pallet_bridge_messages. 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 6-9648540f-f9f0-49cc-9dd8-e6daca935b12 to cancel this command or bot cancel to cancel all commands in this pull request.

svyatonik and others added 5 commits December 14, 2023 14:36
Bumps [actions/checkout](https://github.com/actions/checkout) from 4.1.0
to 4.1.1.
<details>
<summary>Release notes</summary>
<p><em>Sourced from <a
href="https://github.com/actions/checkout/releases">actions/checkout's
releases</a>.</em></p>
<blockquote>
<h2>v4.1.1</h2>
<h2>What's Changed</h2>
<ul>
<li>Update CODEOWNERS to Launch team by <a
href="https://github.com/joshmgross"><code>@​joshmgross</code></a> in <a
href="https://redirect.github.com/actions/checkout/pull/1510">actions/checkout#1510</a></li>
<li>Correct link to GitHub Docs by <a
href="https://github.com/peterbe"><code>@​peterbe</code></a> in <a
href="https://redirect.github.com/actions/checkout/pull/1511">actions/checkout#1511</a></li>
<li>Link to release page from what's new section by <a
href="https://github.com/cory-miller"><code>@​cory-miller</code></a> in
<a
href="https://redirect.github.com/actions/checkout/pull/1514">actions/checkout#1514</a></li>
</ul>
<h2>New Contributors</h2>
<ul>
<li><a
href="https://github.com/joshmgross"><code>@​joshmgross</code></a> made
their first contribution in <a
href="https://redirect.github.com/actions/checkout/pull/1510">actions/checkout#1510</a></li>
<li><a href="https://github.com/peterbe"><code>@​peterbe</code></a> made
their first contribution in <a
href="https://redirect.github.com/actions/checkout/pull/1511">actions/checkout#1511</a></li>
</ul>
<p><strong>Full Changelog</strong>: <a
href="https://github.com/actions/checkout/compare/v4.1.0...v4.1.1">https://github.com/actions/checkout/compare/v4.1.0...v4.1.1</a></p>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a
href="https://github.com/actions/checkout/commit/b4ffde65f46336ab88eb53be808477a3936bae11"><code>b4ffde6</code></a>
Link to release page from what's new section (<a
href="https://redirect.github.com/actions/checkout/issues/1514">#1514</a>)</li>
<li><a
href="https://github.com/actions/checkout/commit/8530928916aaef40f59e6f221989ccb31f5759e7"><code>8530928</code></a>
Correct link to GitHub Docs (<a
href="https://redirect.github.com/actions/checkout/issues/1511">#1511</a>)</li>
<li><a
href="https://github.com/actions/checkout/commit/7cdaf2fbc075e6f3b9ca94cfd6cec5adc8a75622"><code>7cdaf2f</code></a>
Update CODEOWNERS to Launch team (<a
href="https://redirect.github.com/actions/checkout/issues/1510">#1510</a>)</li>
<li>See full diff in <a
href="https://github.com/actions/checkout/compare/v4.1.0...b4ffde65f46336ab88eb53be808477a3936bae11">compare
view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility
score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=actions/checkout&package-manager=github_actions&previous-version=4.1.0&new-version=4.1.1)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after
your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating
it. You can achieve the same result by closing it manually
- `@dependabot show <dependency name> ignore conditions` will show all
of the ignore conditions of the specified dependency
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)


</details>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
This tool makes it easy to run parachain consensus stress/performance
testing on your development machine or in CI.

## Motivation
The parachain consensus node implementation spans across many modules
which we call subsystems. Each subsystem is responsible for a small part
of logic of the parachain consensus pipeline, but in general the most
load and performance issues are localized in just a few core subsystems
like `availability-recovery`, `approval-voting` or
`dispute-coordinator`. In the absence of such a tool, we would run large
test nets to load/stress test these parts of the system. Setting up and
making sense of the amount of data produced by such a large test is very
expensive, hard to orchestrate and is a huge development time sink.

## PR contents
- CLI tool 
- Data Availability Read test
- reusable mockups and components needed so far
- Documentation on how to get started

### Data Availability Read test

An overseer is built with using a real `availability-recovery` susbsytem
instance while dependent subsystems like `av-store`, `network-bridge`
and `runtime-api` are mocked. The network bridge will emulate all the
network peers and their answering to requests.

The test is going to be run for a number of blocks. For each block it
will generate send a “RecoverAvailableData” request for an arbitrary
number of candidates. We wait for the subsystem to respond to all
requests before moving to the next block.
At the same time we collect the usual subsystem metrics and task CPU
metrics and show some nice progress reports while running.

### Here is how the CLI looks like:

```
[2023-11-28T13:06:27Z INFO  subsystem_bench::core::display] n_validators = 1000, n_cores = 20, pov_size = 5120 - 5120, error = 3, latency = Some(PeerLatency { min_latency: 1ms, max_latency: 100ms })
[2023-11-28T13:06:27Z INFO  subsystem-bench::availability] Generating template candidate index=0 pov_size=5242880
[2023-11-28T13:06:27Z INFO  subsystem-bench::availability] Created test environment.
[2023-11-28T13:06:27Z INFO  subsystem-bench::availability] Pre-generating 60 candidates.
[2023-11-28T13:06:30Z INFO  subsystem-bench::core] Initializing network emulation for 1000 peers.
[2023-11-28T13:06:30Z INFO  subsystem-bench::availability] Current block 1/3
[2023-11-28T13:06:30Z INFO  substrate_prometheus_endpoint] 〽️ Prometheus exporter started at 127.0.0.1:9999
[2023-11-28T13:06:30Z INFO  subsystem_bench::availability] 20 recoveries pending
[2023-11-28T13:06:37Z INFO  subsystem_bench::availability] Block time 6262ms
[2023-11-28T13:06:37Z INFO  subsystem-bench::availability] Sleeping till end of block (0ms)
[2023-11-28T13:06:37Z INFO  subsystem-bench::availability] Current block 2/3
[2023-11-28T13:06:37Z INFO  subsystem_bench::availability] 20 recoveries pending
[2023-11-28T13:06:43Z INFO  subsystem_bench::availability] Block time 6369ms
[2023-11-28T13:06:43Z INFO  subsystem-bench::availability] Sleeping till end of block (0ms)
[2023-11-28T13:06:43Z INFO  subsystem-bench::availability] Current block 3/3
[2023-11-28T13:06:43Z INFO  subsystem_bench::availability] 20 recoveries pending
[2023-11-28T13:06:49Z INFO  subsystem_bench::availability] Block time 6194ms
[2023-11-28T13:06:49Z INFO  subsystem-bench::availability] Sleeping till end of block (0ms)
[2023-11-28T13:06:49Z INFO  subsystem_bench::availability] All blocks processed in 18829ms
[2023-11-28T13:06:49Z INFO  subsystem_bench::availability] Throughput: 102400 KiB/block
[2023-11-28T13:06:49Z INFO  subsystem_bench::availability] Block time: 6276 ms
[2023-11-28T13:06:49Z INFO  subsystem_bench::availability] 
    
    Total received from network: 415 MiB
    Total sent to network: 724 KiB
    Total subsystem CPU usage 24.00s
    CPU usage per block 8.00s
    Total test environment CPU usage 0.15s
    CPU usage per block 0.05s
```

### Prometheus/Grafana stack in action
<img width="1246" alt="Screenshot 2023-11-28 at 15 11 10"
src="https://github.com/paritytech/polkadot-sdk/assets/54316454/eaa47422-4a5e-4a3a-aaef-14ca644c1574">
<img width="1246" alt="Screenshot 2023-11-28 at 15 12 01"
src="https://github.com/paritytech/polkadot-sdk/assets/54316454/237329d6-1710-4c27-8f67-5fb11d7f66ea">
<img width="1246" alt="Screenshot 2023-11-28 at 15 12 38"
src="https://github.com/paritytech/polkadot-sdk/assets/54316454/a07119e8-c9f1-4810-a1b3-f1b7b01cf357">

---------

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
This PR backports `transaction_version` bump from `1.5.0` release back
to `master`
…=bridge-hub-rococo --runtime_dir=bridge-hubs --target_dir=cumulus --pallet=pallet_bridge_messages
@command-bot
Copy link

command-bot bot commented Dec 14, 2023

@svyatonik Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=bridge-hub-rococo --runtime_dir=bridge-hubs --target_dir=cumulus --pallet=pallet_bridge_messages has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4725738 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4725738/artifacts/download.

@svyatonik svyatonik marked this pull request as ready for review December 14, 2023 12:55
@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/4726740

@svyatonik svyatonik merged commit 097308e into master Dec 14, 2023
115 of 116 checks passed
@svyatonik svyatonik deleted the add-with-bulletin-bridge-to-rococo-bridge-hub branch December 14, 2023 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes T15-bridges This PR/Issue is related to bridges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants