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

Request based availability distribution #2423

Merged
merged 66 commits into from
Feb 26, 2021

Conversation

eskimor
Copy link
Member

@eskimor eskimor commented Feb 11, 2021

PR went large, because I took the occasion to make ValidatorIndex a newtype wrapper, which is quite invasive in test code.

What it does in a nutshell:

  • It monitors occupied cores and keeps track of them.
  • For each occupied core that does not belong to our own backing group, we spawn a task for fetching/validating/storing the chunk. For fetching, we try each validator of the backing group in a random order, that stays fixed over the session. One exception: If validators did not deliver, they will be ranked back, so we won't waste time at each block, waiting for unresponsive validators. Maintaining the order is important, so networking will keep TCP connections open, when querying the same validator each block. We also save resources, by not opening more connections than necessary.
  • For each requested chunk: Fetch it from the store and deliver it.

Status: Tested manually and seems to work fine (adder-collator).

Still missing in this PR:

  • Tests
  • Instrumentation/metrics/jaeger
  • Guide update

Work on the requesting side of things.
as I will likely replace it with something smarter.
and getting things to type check.
@github-actions github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Feb 11, 2021
primitives/src/v0.rs Outdated Show resolved Hide resolved
@rphmeier
Copy link
Contributor

I filed follow-up issues for the two points I raised: K ancestors and parallelization. This PR just needs tests and then we're good to go.

@eskimor
Copy link
Member Author

eskimor commented Feb 24, 2021

I filed follow-up issues for the two points I raised: K ancestors and parallelization. This PR just needs tests and then we're good to go.

uh perfect, then tests now :-)

@@ -1562,7 +1535,7 @@ mod tests {
AllMessages::ChainApi(_) => unreachable!("Not interested in network events"),
AllMessages::CollatorProtocol(_) => unreachable!("Not interested in network events"),
AllMessages::StatementDistribution(_) => { cnt += 1; }
AllMessages::AvailabilityDistribution(_) => { cnt += 1; }
AllMessages::AvailabilityDistribution(_) => unreachable!("Not interested in network events"),
Copy link
Contributor

Choose a reason for hiding this comment

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

This can't be triggered by a malicious node, can it?

Copy link
Contributor

Choose a reason for hiding this comment

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

(oh, this is test code 👍 )

@rphmeier rphmeier merged commit 950447e into master Feb 26, 2021
@rphmeier rphmeier deleted the rk-availability-distribution-2306 branch February 26, 2021 17:58
@dvdplm
Copy link
Contributor

dvdplm commented Mar 3, 2021

* Instrumentation/metrics/jaeger

@eskimor Is there an issue open for adding back the jaeger spans that we can track?

@eskimor
Copy link
Member Author

eskimor commented Mar 3, 2021

Good catch - no, it is just on my list. Will make an issue: #2551

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.

7 participants