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

Enable async backing for system parachains #266

Conversation

s0me0ne-unkn0wn
Copy link
Contributor

This enables async backing with 6 sec block times for all the system parachains on both Kusama and Polkadot.

@s0me0ne-unkn0wn
Copy link
Contributor Author

@bkontur bridges fail tests because of constants defined here. How should we handle that? Do we need first to release a new version of bh-bridge-hub-cumulus with new values and then bump dependencies here?

@bkontur
Copy link
Contributor

bkontur commented Apr 6, 2024

@bkontur bridges fail tests because of constants defined here. How should we handle that? Do we need first to release a new version of bh-bridge-hub-cumulus with new values and then bump dependencies here?

I think there is no need to change bh-bridge-hub-cumulus, I added there stuff for async backing here: paritytech/polkadot-sdk#2966.

Please, try to change this BlockWeights -> BlockWeightsForAsyncBacking here, here, here and here

@s0me0ne-unkn0wn
Copy link
Contributor Author

@bkontur hmmm, it's not in 0.8.0 bh-bridge-hub-cumulus crate version which is a workspace dependency... Can I just bump it to the latest 0.10.0? Or better wait until you bump all the crate versions for the next release?

@bkontur
Copy link
Contributor

bkontur commented Apr 7, 2024

@bkontur hmmm, it's not in 0.8.0 bh-bridge-hub-cumulus crate version which is a workspace dependency... Can I just bump it to the latest 0.10.0? Or better wait until you bump all the crate versions for the next release?

hmm, sorry, I didn't check the version. I would not bump here (you could run into the other issues with incompatibility...),
I would just copy BlockWeightsForAsyncBacking and MAXIMUM_BLOCK_WEIGHT_FOR_ASYNC_BACKING directly here and here with some TODO: consider removing when 1.10.0 bump added here: #186

@@ -196,7 +196,7 @@ impl pallet_timestamp::Config for Runtime {
/// A timestamp: milliseconds since the unix epoch.
type Moment = u64;
type OnTimestampSet = Aura;
type MinimumPeriod = ConstU64<{ SLOT_DURATION / 2 }>;
type MinimumPeriod = ConstU64<0>;

Choose a reason for hiding this comment

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

is this a tmp value or is it going to be zero?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIUC, we don't need to set the minimum period anymore. With async backing at higher velocities, we may produce more blocks per slot, and nothing stops us from producing more than two blocks per slot, so this limit doesn't make sense, and I set it to zero here. But maybe some of the senior fellows will correct me.

Choose a reason for hiding this comment

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

If that's the case then I need to figure out how to deal with it on chopsticks

@s0me0ne-unkn0wn s0me0ne-unkn0wn marked this pull request as ready for review April 19, 2024 13:04
@s0me0ne-unkn0wn s0me0ne-unkn0wn mentioned this pull request Apr 19, 2024
9 tasks
@bkchr
Copy link
Contributor

bkchr commented Apr 23, 2024

Before we are doing this, we need to review that all these chains are capable of running with 6seconds. I mean that stuff that maybe has timeout measured in blocks would be broken by this.

@bkchr
Copy link
Contributor

bkchr commented Apr 23, 2024

CC @ggwpez

@xlc
Copy link
Contributor

xlc commented Apr 23, 2024

We can't change the block time without ensuring every pallet used by each runtime are dynamic block time friendly
I think we need to have a checklist somewhere
Related paritytech/polkadot-sdk#3635 paritytech/polkadot-sdk#3268

@s0me0ne-unkn0wn
Copy link
Contributor Author

Okay, so it's not that easy. Maybe I could create separate PRs for different parachains, then? Do we have any parachains that definitely can be switched to 6s block times without side effects?

The point is, we have teams in the ecosystem waiting for at least something running with 6s block times on mainnets. The sooner we show it works, the sooner they'll catch up. And sorting out the timing issue may take quite some time, I'm afraid.

@xlc
Copy link
Contributor

xlc commented Apr 24, 2024

there is good chance no existing ecosystem parachain can enable 6s block time because none of the existing governance pallet supports changing block time

@bkchr
Copy link
Contributor

bkchr commented Apr 24, 2024

Do we have any parachains that definitely can be switched to 6s block times without side effects?

Look at the chains, their pallets and if they depend on block number for tracking stuff.

@s0me0ne-unkn0wn
Copy link
Contributor Author

there is good chance no existing ecosystem parachain can enable 6s block time because none of the existing governance pallet supports changing block time

But governance does support runtime upgrades, and we have SLOT_DURATION as a runtime constant. That's not a concern IIUC. As well as all the stuff using proper time constants (those HOURS, MINUTES etc.) derived from the block time. Only stuff explicitly using block numbers to measure time is affected.

@bkchr
Copy link
Contributor

bkchr commented Apr 25, 2024

Only stuff explicitly using block numbers to measure time is affected.

What Bryan meant is that governance logic uses the block number to measure time. Most of the stuff is using/used block number for time. This is the problem we are facing here.

@s0me0ne-unkn0wn
Copy link
Contributor Author

Most of the stuff is using/used block number for time. This is the problem we are facing here.

Oh I see. Still, it's a shame we noticed the problem, we could otherwise get two fellowship salaries a month 🤣

@s0me0ne-unkn0wn
Copy link
Contributor Author

Okay, I went through the existing system parachains, and indeed, most of them require resolving timing issues. But what about the people chain? Are there any obstacles to running it with 6 sec block times? Maybe I'm missing something, but I don't really see anything that would break.

@joepetrowski
Copy link
Contributor

Okay, I went through the existing system parachains, and indeed, most of them require resolving timing issues. But what about the people chain? Are there any obstacles to running it with 6 sec block times? Maybe I'm missing something, but I don't really see anything that would break.

Agree, People should be fine as long as constants like HOURS etc. are updated accordingly.

fellowship-merge-bot bot pushed a commit that referenced this pull request Jun 20, 2024
This is an excerpt from #266. It aims to enable 6-second block times for
`people` parachain only.

If I'm not missing anything, the `people` parachain is the only
parachain not affected by paritytech/polkadot-sdk#3268, and thus,
6-second block times may be enabled without breaking something.

This PR was tested locally using the `kusama-local` relay chain. The
time of the session within which the runtime upgrade was enacted
expectedly deviated, but other than that, no problems were observed.

---------

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
@bkchr
Copy link
Contributor

bkchr commented Jul 25, 2024

@s0me0ne-unkn0wn can you please convert this into an issue. No need to keep the pr open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants