Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

babe: replace usage of SharedEpochChanges with internal RPC #13883

Merged
merged 10 commits into from
Apr 18, 2023

Conversation

andresilva
Copy link
Contributor

@andresilva andresilva commented Apr 11, 2023

Currently in order for different components to fetch BABE-related data they share ownership of the SharedEpochChanges data structure, exposing internal details of BABE to external consumers. This PR creates a request/response system for BABE which runs in a background worker when the BABE import queue is created, and changes the BABE and SyncState RPC to instead retrieve their required data asynchronously using this worker (rather than directly from SharedEpochChanges).

There is still one user of SharedEpochChanges which is the manual-seal crate. I have made changes to support this new system but since there are no tests for this I am not sure if I have broken anything. I will deal with that in a future PR.

polkadot companion: paritytech/polkadot#7059

@andresilva andresilva added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Apr 11, 2023
@andresilva andresilva requested a review from davxy April 11, 2023 18:22
let answer_requests =
answer_requests(worker_rx, babe_link.config, client, babe_link.epoch_changes);

spawner.spawn_essential("babe-worker", None, Box::pin(answer_requests));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't great but I wanted to minimize changes to the structure of service.

SinkExt::send provides backpressure in case the channel is full
Copy link
Member

@davxy davxy left a comment

Choose a reason for hiding this comment

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

🟢

@andresilva
Copy link
Contributor Author

@davxy addressed

@andresilva
Copy link
Contributor Author

bot merge force

@paritytech-processbot paritytech-processbot bot merged commit cb95482 into master Apr 18, 2023
@paritytech-processbot paritytech-processbot bot deleted the andre/babe-refactor-shared-data branch April 18, 2023 09:38
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
…ch#13883)

* babe: replace usage of SharedEpochChanges with internal RPC

* babe-rpc: fix tests

* babe: use SinkExt::send instead of Sender::try_send

SinkExt::send provides backpressure in case the channel is full

* Update client/consensus/babe/src/lib.rs

Co-authored-by: Bastian Köcher <git@kchr.de>

* babe: fix spawn

* babe: send handles backpressure

* babe: use testing::TaskExecutor

* babe-rpc: better error handling

---------

Co-authored-by: Bastian Köcher <git@kchr.de>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants