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

Async backing client ready #1302

Merged
merged 13 commits into from
Aug 6, 2024
Merged

Async backing client ready #1302

merged 13 commits into from
Aug 6, 2024

Conversation

ermalkaleci
Copy link
Contributor

@ermalkaleci ermalkaleci commented Jul 17, 2024

Update client to support async backing and apply necessary changes to runtime without enabling async-backing

@ermalkaleci ermalkaleci added client This PR/Issue is related to the topic “client”. runtime This PR/Issue is related to the topic “runtime”. labels Jul 17, 2024
@ermalkaleci ermalkaleci changed the title WIP: Async backing Async backing client ready Aug 5, 2024
@ermalkaleci ermalkaleci marked this pull request as ready for review August 5, 2024 11:04
Copy link
Member

@Dinonard Dinonard left a comment

Choose a reason for hiding this comment

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

In general looks good to me!

Just a few minor comments/questions.

runtime/astar/src/lib.rs Show resolved Hide resolved
runtime/astar/src/lib.rs Outdated Show resolved Hide resolved
runtime/astar/src/lib.rs Show resolved Hide resolved
Copy link

github-actions bot commented Aug 6, 2024

Code Coverage

Package Line Rate Branch Rate Health
pallets/dapp-staking-v3/rpc/runtime-api/src 0% 0%
pallets/dapp-staking-migration/src 0% 0%
pallets/static-price-provider/src 52% 0%
precompiles/dapp-staking-v3/src 90% 0%
pallets/ethereum-checked/src 74% 0%
precompiles/dispatch-lockdrop/src 86% 0%
pallets/astar-xcm-benchmarks/src/fungible 100% 0%
pallets/dapp-staking-v3/src/benchmarking 98% 0%
precompiles/unified-accounts/src 100% 0%
pallets/inflation/src 86% 0%
pallets/collator-selection/src 92% 0%
precompiles/assets-erc20/src 78% 0%
pallets/astar-xcm-benchmarks/src/generic 100% 0%
pallets/collective-proxy/src 86% 0%
pallets/dynamic-evm-base-fee/src 89% 0%
pallets/unified-accounts/src 77% 0%
chain-extensions/types/assets/src 0% 0%
precompiles/substrate-ecdsa/src 74% 0%
chain-extensions/pallet-assets/src 56% 0%
primitives/src 60% 0%
pallets/xc-asset-config/src 50% 0%
chain-extensions/unified-accounts/src 0% 0%
precompiles/xcm/src 71% 0%
precompiles/dapp-staking-v3/src/test 0% 0%
primitives/src/xcm 65% 0%
precompiles/sr25519/src 64% 0%
chain-extensions/types/unified-accounts/src 0% 0%
pallets/astar-xcm-benchmarks/src 86% 0%
pallets/price-aggregator/src 75% 0%
pallets/dapp-staking-v3/src 90% 0%
pallets/dapp-staking-v3/src/test 0% 0%
Summary 78% (3581 / 4570) 0% (0 / 0)

Minimum allowed line rate is 50%

// We got around 500ms for proposing
authoring_duration: Duration::from_millis(500),
collation_request_receiver: Some(request_stream),
authoring_duration: Duration::from_millis(1500),
Copy link
Member

Choose a reason for hiding this comment

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

Thinking about it now - shouldn't this still remain at 500?
At least until we turn on the async backing feature for some runtime?

E.g. we deploy this new client on the existing network now.
All collators still have half a second to propose the block, don't they?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The block limit is defined on runtime by weight limit. This duration here is the client time available to build the block. If not built within 1.5 secs then it will timeout (hence slot hardware). Normally it needs to match with runtime block limit but this is a transitory period until async backing is enabled. We need this out so when the time comes, all clients support async backing and we just enable it.

Copy link
Member

Choose a reason for hiding this comment

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

Right, weight limit would be the limiter of the block size, assuming that the collator hardware is at least powerful as the benchmark machine.

Setting this value to 1500 now means that collators allow themselves to take up to 1500 [ms] seconds to produce a block worth of 500 [ms] (hence the addition of HW spec check as you said).

I was about to suggest to make this into a CLI param, but I assume that would defeat the purpose of avoiding the client restart 🙂.

I think it's all clear now. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Client is still required to build block within 0.5s, if not it will miss timeframe of 2s build&gossip. This doesn't mean they've 1.5s to build the block. It's more a time so client don't continue building if this timeout is reached. After this timeout, it's pointless to continue.

@ermalkaleci ermalkaleci merged commit 1344b7e into master Aug 6, 2024
8 checks passed
@ermalkaleci ermalkaleci deleted the feat/async-backing branch August 6, 2024 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client This PR/Issue is related to the topic “client”. runtime This PR/Issue is related to the topic “runtime”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants