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

Request based PoV distribution #2640

Merged
merged 36 commits into from
Mar 28, 2021
Merged

Request based PoV distribution #2640

merged 36 commits into from
Mar 28, 2021

Conversation

eskimor
Copy link
Member

@eskimor eskimor commented Mar 18, 2021 •
edited
Loading

Fixes #2590

@github-actions github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Mar 18, 2021
rphmeier
rphmeier reviewed Mar 24, 2021
View reviewed changes
Copy link
Contributor

@rphmeier rphmeier left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think it would be better to keep the same external API of "get the PoV from someone in the group" as it keeps backing simple. Retrying & querying different peers can be handled within the subsystem.

Also, I don't think availability distribution is really the best home for this logic as it has little to do with availability and everything to do with backing

@eskimor
Copy link
Member Author

eskimor commented Mar 24, 2021

Also, I don't think availability distribution is really the best home for this logic as it has little to do with availability and everything to do with backing

Yeah, we would probably need a different name if we keep it that way. Pierre's blog post inspired me to an architecture, where we stay in "standard" Rust more, with easy sharing of data, via references and such and still get all the benefits of asynchronous programming and the efficiency/concurrency it brings. Basically simplifying things a lot/making it easier to reason about code and also improving performance. I wanted to try this out in the small here and give a knowledge share presentation if it worked out, to me it looks very promising, but only code does not lie.

@eskimor eskimor added 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. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Mar 27, 2021
@eskimor eskimor force-pushed the rk-pov-distribution-2590 branch from dbd3009 to b1a201a Compare March 27, 2021 13:48
@eskimor eskimor marked this pull request as ready for review March 27, 2021 22:45
@eskimor eskimor force-pushed the rk-pov-distribution-2590 branch from 94de933 to 071bcca Compare March 27, 2021 23:51
@eskimor eskimor merged commit 39124b0 into master Mar 28, 2021
@eskimor eskimor deleted the rk-pov-distribution-2590 branch March 28, 2021 15:11
ordian added a commit that referenced this pull request Mar 28, 2021
@ordian
Merge branch 'master' into ao-debug-approval-distribution-III
be4a89d 
* master:
  Call NetworkService::add_known_address before sending a request (#2726)
  Overseer: subsystems communicate directly (#2227)
  Request based PoV distribution (#2640)
rphmeier pushed a commit that referenced this pull request Mar 28, 2021
@eskimor @rphmeier
Request based PoV distribution (#2640)
00e18bd 
* Indentation fix.

* Prepare request-response for PoV fetching.

* Drop old PoV distribution.

* WIP: Fetch PoV directly from backing.

* Backing compiles.

* Runtime access and connection management for PoV distribution.

* Get rid of seemingly dead code.

* Implement PoV fetching.

Backing does not yet use it.

* Don't send `ConnectToValidators` for empty list.

* Even better - no need to check over and over again.

* PoV fetching implemented.

+ Typechecks
+ Should work

Missing:

- Guide
- Tests
- Do fallback fetching in case fetching from seconding validator fails.

* Check PoV hash upon reception.

* Implement retry of PoV fetching in backing.

* Avoid pointless validation spawning.

* Add jaeger span to pov requesting.

* Add back tracing.

* Review remarks.

* Whitespace.

* Whitespace again.

* Cleanup + fix tests.

* Log to log target in overseer.

* Fix more tests.

* Don't fail if group cannot be found.

* Simple test for PoV fetcher.

* Handle missing group membership better.

* Add test for retry functionality.

* Fix flaky test.

* Spaces again.

* Guide updates.

* Spaces.
@cwerling cwerling added D1-audited 👍 PR contains changes to critical logic that has been properly reviewed and externally audited. and removed D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Apr 20, 2021
@zqhxuyuan
Copy link
Contributor

@eskimor any update on implements guide? the doc still has pov-distribution here: https://w3f.github.io/parachain-implementers-guide/node/subsystems-and-jobs.html

@eskimor
Copy link
Member Author

eskimor commented Jun 10, 2021

I'll revisit the guide, thanks!

@eskimor eskimor mentioned this pull request Nov 11, 2022
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ordian ordian ordian left review comments

@rphmeier rphmeier rphmeier approved these changes

Assignees
No one assigned
Labels
A3-in_progress Pull request is in progress. No review needed at this stage. 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. D1-audited 👍 PR contains changes to critical logic that has been properly reviewed and externally audited.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Request based PoV distribution
5 participants
@eskimor @zqhxuyuan @ordian @rphmeier @cwerling