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

Reduce dispute coordinator load #5785

Merged
merged 51 commits into from
Aug 16, 2022
Merged

Reduce dispute coordinator load #5785

merged 51 commits into from
Aug 16, 2022

Conversation

eskimor
Copy link
Member

@eskimor eskimor commented Jul 17, 2022

This reduces the load on the dispute coordinator in normal operation by a huge factor by simply not importing approval votes. Instead approval votes are only imported on actual disputes.

In addition, I also removed the redundant import of backing votes, as those are already scraped from chain. The chain scraping of votes is way more efficient (because batched) and shaves off another few percent of CPU usage. It is secure, because disputes are only raised by approval voters and approval voters must have seen a candidate included on chain for them to start any work. If they have seen it included, they must have seen it backed as well, hence backing votes will be available.

A follow up PR will also improve performance of imports on actual disputes via batching.

Todos:

  • approval-voting tests need to be fixed
  • tests for new functionality
  • guide changes
  • burnin on Versi: in particular watch out for warnings - if we get some: Don't silence the warnings, but make sure to fix the issue as it will prevent disputes from working properly!
  • fix security issue with regards to slashing

Note: This PR enables Polkadot to scale way better in the number of parachains and I expect it to be mandatory for good performance on Kusama with 40 parachains.

into the dispute coordinator. This also gets rid of a redundant
signature check. Both should have some impact on backing performance.
In general this PR should make us scale better in the number of parachains.

Reasoning (aka why this is fine):

For the signature check: As mentioned, it is a redundant check. The
signature has already been checked at this point. This is even made
obvious by the used types. The smart constructor is not perfect as
discussed [here](https://github.com/paritytech/polkadot/issues/3455),
but is still a reasonable security.

For not importing to the dispute-coordinator: This should be good as the
dispute coordinator does scrape backing votes from chain. This suffices
in practice as a super majority of validators must have seen a backing
fork in order for a candidate to get included and only included
candidates pose a threat to our system. The import from chain is
preferable over direct import of backing votes for two reasons:

1. The import is batched, greatly improving import performance. All
   backing votes for a candidate are imported with a single import.
   And indeed we were able to see in metrics that importing votes
   from chain is fast.
2. We do less work in general as not every candidate for which
   statements are gossiped might actually make it on a chain. The
   dispute coordinator as with the current implementation would still
   import and keep those votes around for six sessions.

While redundancy is good for reliability in the event of bugs, this also
comes at a non negligible cost. The dispute-coordinator right now is the
subsystem with the highest load, despite the fact that it should not be
doing much during mormal operation and it is only getting worse
with more parachains as the load is a direct function of the number of statements.

We'll see on Versi how much of a performance improvement this PR
Use BTreeMap for ordered dispute votes.
@github-actions github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jul 17, 2022
@eskimor eskimor added the D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. label Jul 17, 2022
@eskimor
Copy link
Member Author

eskimor commented Jul 17, 2022

@tdimitrov Note this PR changes CandidateVotes to contain BTreeMaps, so this will cause a minor conflict with your pending PR. Should be failry easy to resolve.

@eskimor eskimor requested a review from tdimitrov July 17, 2022 06:18
@eskimor eskimor changed the title Fast dispute coordinator for normal operation Reduced dispute coordinator load Jul 17, 2022
@eskimor eskimor changed the title Reduced dispute coordinator load Reduce dispute coordinator load Jul 17, 2022
@tdimitrov tdimitrov marked this pull request as ready for review July 17, 2022 08:35
@github-actions github-actions bot added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jul 17, 2022
@tdimitrov tdimitrov marked this pull request as draft July 17, 2022 12:16
@tdimitrov
Copy link
Contributor

I removed the draft status by mistake.

Note that the introduced complexity is actually redundant.
@eskimor eskimor marked this pull request as ready for review July 19, 2022 16:29
@eskimor eskimor added B1-releasenotes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. labels Jul 19, 2022
@eskimor eskimor marked this pull request as draft July 19, 2022 16:50
Copy link
Contributor

@slumber slumber left a comment

Choose a reason for hiding this comment

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

Nice!

node/core/dispute-coordinator/src/import.rs Outdated Show resolved Hide resolved
node/core/dispute-coordinator/src/initialized.rs Outdated Show resolved Hide resolved
node/core/dispute-coordinator/src/import.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@sandreim sandreim left a comment

Choose a reason for hiding this comment

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

Thanks for the awesome documentation updates! I only have a few nits and questions, otherwise looks pretty good!

node/core/dispute-coordinator/src/import.rs Show resolved Hide resolved
node/core/dispute-coordinator/src/import.rs Show resolved Hide resolved
node/core/dispute-coordinator/src/import.rs Outdated Show resolved Hide resolved
node/core/dispute-coordinator/src/import.rs Show resolved Hide resolved
node/core/approval-voting/src/lib.rs Outdated Show resolved Hide resolved
node/core/dispute-coordinator/src/import.rs Show resolved Hide resolved
node/core/dispute-coordinator/src/import.rs Outdated Show resolved Hide resolved
node/core/dispute-coordinator/src/initialized.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@vstakhov vstakhov left a comment

Choose a reason for hiding this comment

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

That's great job! I have some minor neats mostly about logging.

node/core/approval-voting/src/lib.rs Outdated Show resolved Hide resolved
node/core/approval-voting/src/lib.rs Outdated Show resolved Hide resolved
node/core/approval-voting/src/lib.rs Outdated Show resolved Hide resolved
node/core/dispute-coordinator/src/import.rs Outdated Show resolved Hide resolved
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. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants