Add Bridge Header Sync to Rococo Runtime

[HCastano]

This PR adds support for syncing headers between the Rococo and Wococo networks.

Before this can be deployed on the live Rococo and Wococo networks we need to change the
pallet owners from Dave and Eve to non-dev accounts.

[HCastano]

I think "Not Live" is the right audit status for this, but let me know if that's wrong

[ordian]

I have some concerns with the new dependencies introduced.

Also, what was the point of having it as a subtree again? The history doesn't seem to be preserved
https://github.com/paritytech/polkadot/commits/master/bridges, or at least I don't see how.

[HCastano]

I have some concerns with the new dependencies introduced.

I was a bit hasty in adding the bridges code to the Polkadot workspace, there was no need for this. Good catch.

Also, what was the point of having it as a subtree again? The history doesn't seem to be preserved
https://github.com/paritytech/polkadot/commits/master/bridges, or at least I don't see how.

You can get a little bit of context here, but there were a few reasons. One was that since some of our components (notably the code in relays/bin-substrate) would need to depend on the Polkadot/Kusama runtimes it would be easier for us to become part of the Polkadot repo so we could use them directly. This has changed since we initially introduced the Subtree though, and the relayer code is totally standalone now.

Another reason was that it was easier to keep Substrate dependencies in sync. If a breaking change was introduced in Substrate, you'd need a companion PR for the bridges repo and the Polkadot repo - which would add complexity to making changes to Substrate. By having it be part of the Polkadot repo one companion would be needed, and the bridges repo could be updated independently at a later date.

Maybe @tomusdrw can also add some other reasons that I forgot to list here.

[ordian]

Also, what was the point of having it as a subtree again? The history doesn't seem to be preserved
https://github.com/paritytech/polkadot/commits/master/bridges, or at least I don't see how.

I guess my question was rather why do we need to merge it instead of squash-merge?
#2978 (comment) provides some reasoning that I don't really follow

Turns out that if we squash-merge these subtree update PRs then the subtree history in
future updates won't be super accurate (but the diff will look correct). The history,
which is what you see in this commit for example 70985ce, will always start at the same
commit instead of showing the changes between the last update and the current update.

The history of commits will not be accurate in both cases, your example showed a commit message, not history, also you can't even see it with git blame which defeats (have to follow another parent commit) the purpose of such history.

BTW, github allows you to edit commit message if you click on squash and merge button manually.

[HCastano]

I guess my question was rather why do we need to merge it instead of squash-merge?
#2978 (comment) provides some reasoning that I don't really follow

In this case since we're not updating the subtree (e.g git subtree pull ...) it's fine to squash merge.

The history of commits will not be accurate in both cases, your example showed a commit message, not history

So I'm not too worried about this history within the bridges folder as much as I am about the history of the Polkadot master branch. See this commit for example. Notice that it includes all the commits since the last Subtree update. If we had squash merged that commit into master next time we updated we would have that commit show the history from [f51fb59d, parity-bridges-common@HEAD] instead of [b2099c5c, parity-bridges-common@HEAD]. We shouldn't be re-adding the entire bridges history every update, we only want the set of changes since the last update.

also you can't even see it with git blame which defeats (have to follow another parent commit) the purpose of such history.

Yeah, I guess that's a good point, but we don't expect people to be checking the history of changes from the bridges repo from the Polkadot repo.

We need to go back to the motivation of adding this subtree. It is not meant as a collaboration tool in the sense the development is shared between the two repos. Instead it is the best way we found to keep Substrate dependencies aligned between different repos given the fast pace of development.

BTW, github allows you to edit commit message if you click on squash and merge button manually.

Yeah I guess, I think Git would still retain knowledge of the "actual" updates and we might end up with a case where we update the entire Subtree every update, which is not what we want.

[tomusdrw]

The runtime changes look good, but we should be using real private keys and avoid modifying any of the bridges code.

[tomusdrw]

I suppose these changes don't come via subtree update, IMHO we shouldn't be mixing adding bridges code and polkadot code in one PR. The point of bridges code being part of the repository is to make it easy for polkadot devs to fix any substrate inconsistencies, not to add features.

[tomusdrw]

Woah, please use real accounts, that's rococo staging.

[HCastano]

Well the problem is what accounts do we use? I figured it would be best to use dev accounts, and then talk to the DevOps team to see what "real" accounts to use.

We'd need to talk to them anyways before this would be deployed to Rococo anyways

[tomusdrw]

Suggested change

	pallet_bridge_grandpa: rococo_runtime::BridgeRococoGrandpaConfig {
	owner: Some(get_account_id_from_seed::<sr25519::Public>("Dave")),
	..Default::default()
	},
	pallet_bridge_grandpa_Instance1: rococo_runtime::BridgeWococoGrandpaConfig {
	owner: Some(get_account_id_from_seed::<sr25519::Public>("Eve")),
	..Default::default()
	pallet_bridge_grandpa: rococo_runtime::BridgeRococoGrandpaConfig {
	owner: Some(root_key),
	..Default::default()
	},
	pallet_bridge_grandpa_Instance1: rococo_runtime::BridgeWococoGrandpaConfig {
	owner: Some(root_key),
	..Default::default()

[HCastano]

I guess root is reasonable to start

[tomusdrw]

Suggested change

	pub const MaxRequests: u32 = 50;
	pub const MaxRequests: u32 = 4 * HOURS as u32;

Can we do something like that instead? This will allow us to catch-up immediately in case of 4 hours downtime.

[tomusdrw]

Re @ordian

I guess my question was rather why do we need to merge it instead of squash-merge?
#2978 (comment) provides some reasoning that I don't really follow
BTW, github allows you to edit commit message if you click on squash and merge button manually.

Modifying squash-merge commit message is a cool suggestion, we can try that. I'm a bit skeptical though, because it seem that every subtree update is actually two commits (squashed changes & merge commit).

The history of commits will not be accurate in both cases, your example showed a commit message, not history, also you can't > even see it with git blame which defeats (have to follow another parent commit) the purpose of such history.

Well you can't git blame on any of the dependency changes either. The purpose of the history & merging is to make it a bit easier for the Bridges team to update the subtree. Bridges code should be treated as an external dependency from polkadot PoV (similar to how Substrate is treated). The only reason to have it added as git subtree is to make it easy for developers contributing to polkadot repository to fix any substrate-originating inconsistencies without going through some tedious release cycle or PR juggling (see how cumbersome it is for BEEFY, which is added as regular dependency pointing to git repo).
Obviously one could argue that the bridges code should simply be part of Polkadot repository, but this comes with couple of additional inconveniences, namely: notification churn, responsibility dilution, lack of independence (both design decisions, code style, tooling, etc). Ideally we would like to just use crates-io deps, but it's difficult if we keep following Substrate master, and that's kind of necessary due to fast development pace. Taking all that into account I still think that even though imperfect, subtree solution is a worthwhile compromise, although I'm super happy to re-consider other possible options at any point in time.

[ordian]

Thanks for the explanations!

It makes sense, but would be cool if we could use the same merge strategy for all PRs.

Modifying squash-merge commit message is a cool suggestion, we can try that. I'm a bit skeptical though, because it seem that every subtree update is actually two commits (squashed changes & merge commit).

I've quickly tested it by doing

• git subtree pull --squash --prefix bridges https://github.com/paritytech/parity-bridges-common e58d85148863bde467e84c373243c89ab64b9a79
• then git rebase master
• and then git subtree pull ... again, which resulted in no-op
  (Subtree is already at commit e58d85148863bde467e84c373243c89ab64b9a79.).

[tomusdrw]

I've quickly tested it by doing

Hmm, but that's not necessarily equivalent of squash-merging a PR, is it? Like git rebase master will simply re-apply the same two commits that were created by git subtree pull on top of master commits. You would need to try with git rebase -i HEAD^^ and then squashing the two commits produced by git subtree pull.
But yeah, I'm actually not sure why "Rebase & Merge" from github does not work - every time I tried it it failed and mentioned conflicts, even though the branch was up to date with lastest master.

[ordian]

I've quickly tested it by doing

Hmm, but that's not necessarily equivalent of squash-merging a PR, is it? Like git rebase master will simply re-apply the same two commits that were created by git subtree pull on top of master commits. You would need to try with git rebase -i HEAD^^ and then squashing the two commits produced by git subtree pull.
But yeah, I'm actually not sure why "Rebase & Merge" from github does not work - every time I tried it it failed and mentioned conflicts, even though the branch was up to date with lastest master.

If applied from a branch it would flatten the tree, having the same effect as git rebase -i HEAD^^
from

*   735d0adb9 (HEAD -> temp-branch) Merge commit 'd7a168e207326eb9f37d4b6e7d929d0ea7ee76e3' into temp-branch
|\  
| * d7a168e20 Squashed 'bridges/' changes from b2099c5c0..e58d85148
* | 78b8cb259 (origin/master, master) CI: bot allow simnet failure (#2993)

to

* 52c0bba13 (HEAD -> temp-branch) Squashed 'bridges/' changes from b2099c5c0..e58d85148
* 78b8cb259 (origin/master, master) CI: bot allow simnet failure (#2993)

I don't really know why it failed before, maybe someone has made some commits to the bridges subtree that were not present in upstream?

[ordian]

I've quickly tested it by doing

Hmm, but that's not necessarily equivalent of squash-merging a PR, is it? Like git rebase master will simply re-apply the same two commits that were created by git subtree pull on top of master commits. You would need to try with git rebase -i HEAD^^ and then squashing the two commits produced by git subtree pull.
But yeah, I'm actually not sure why "Rebase & Merge" from github does not work - every time I tried it it failed and mentioned conflicts, even though the branch was up to date with lastest master.

If applied from a branch it would flatten the tree, having the same effect as git rebase -i HEAD^^
from

*   735d0adb9 (HEAD -> temp-branch) Merge commit 'd7a168e207326eb9f37d4b6e7d929d0ea7ee76e3' into temp-branch
|\  
| * d7a168e20 Squashed 'bridges/' changes from b2099c5c0..e58d85148
* | 78b8cb259 (origin/master, master) CI: bot allow simnet failure (#2993)

to

* 52c0bba13 (HEAD -> temp-branch) Squashed 'bridges/' changes from b2099c5c0..e58d85148
* 78b8cb259 (origin/master, master) CI: bot allow simnet failure (#2993)

I don't really know why it failed before, maybe someone has made some commits to the bridges subtree that were not present in upstream?

Disregard what I said, git seems to treat the branch with Squashed 'bridges/' changes from ... as the subtree, so it seems to imply it has to be on a separate branch with merge commits into master, otherwise it would think that master == subtree.

[tomusdrw]

lgtm!

[tomusdrw]

I think an explanatory comment would be nice here, cause we are adding Rococo bridge pallet to Rococo runtime. It's only because Rococo and Wococo have exactly the same runtime, but the intention is to use BridgeWococoGrandpa pallet on Rococo and BridgeRococoGrandpa pallet on Wococo. The other pallet will always be uninitialized/halted.
This is a bit excessive and confusing, but I think it's acceptable for the matter of simplicity (no runtime duplication).

[tomusdrw]

bot merge

[ghost]

Waiting for commit status.

[ghost]

Merge aborted: Checks failed for d998df1

[ordian]

bot merge

[ghost]

Waiting for commit status.