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

Dispute coordinator - Recover disputes on startup #3481

Merged

Conversation

Lldenaurois
Copy link
Contributor

This PR modifies the dispute coordinator to send a DisputeCoordinatorMessage::Participate for each non-concluded dispute for which the subsystem has no local statement on restarts.

Before entering the main event loop, the subsystem will attempt to resume operation by:

  1. Fetching the set of active leaves by issuing ChainSelection::Leaves request.
  2. Reconstructing the rolling session window and session infos by calling handle_new_activations for the returned leaves.
  3. Loading the non-concluded disputes and their corresponding candidate votes from the database respectively.
  4. Sending a DisputeCoordinatorMessage::Participate for any disputes that are missing a local statement.

Along the way the test harness is also refactored to ergonomically support restarting the subsystem, while carrying over any database state between executions.

Fixes #3445

This commit introduces a resume capability for the
dispute coordinator subsystem. Specifically, this will allow
to recover data for disputes for which we have no local statements.
… Harness

This commit modifies the TestHarness to return a TestState. We subsequently
define a resume function on TestState that allows to interrupt the test and
test specifically for behavior on startup of the subsystem.
This commit implements the resume functionality for the subsystem.
In addition, we will forward any DisputeParticipation::Participate
message in order to ensure that disputes for which we do not have
local statements may be recovered in due time.
Copy link
Contributor Author

@Lldenaurois Lldenaurois left a comment

Choose a reason for hiding this comment

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

Additional Question:

Do we need to have any special handling for the dispute availability checks during restart? Or is it safe to ignore dispute availability checks?

node/core/dispute-coordinator/src/lib.rs Outdated Show resolved Hide resolved
node/core/dispute-coordinator/src/lib.rs Outdated Show resolved Hide resolved
node/core/dispute-coordinator/src/lib.rs Outdated Show resolved Hide resolved
@Lldenaurois Lldenaurois 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. I8-refactor Code needs refactoring. labels Jul 16, 2021
@Lldenaurois Lldenaurois changed the title Dispute coordinator no local statements Dispute coordinator - Recover disputes on restart Jul 16, 2021
@Lldenaurois Lldenaurois changed the title Dispute coordinator - Recover disputes on restart Dispute coordinator - Recover disputes on startup Jul 16, 2021
Copy link
Contributor

@drahnr drahnr left a comment

Choose a reason for hiding this comment

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

A few nits, generally looks good. The open questions need some more eyes / more dispute knowledgeable eyes.

node/core/dispute-coordinator/src/lib.rs Outdated Show resolved Hide resolved
node/core/dispute-coordinator/src/lib.rs Outdated Show resolved Hide resolved
node/core/dispute-coordinator/src/lib.rs Outdated Show resolved Hide resolved
node/core/dispute-coordinator/src/lib.rs Show resolved Hide resolved
node/core/dispute-coordinator/src/lib.rs Outdated Show resolved Hide resolved
node/core/dispute-coordinator/src/lib.rs Outdated Show resolved Hide resolved
node/core/dispute-coordinator/src/lib.rs Outdated Show resolved Hide resolved
@rphmeier rphmeier added A5-grumble and removed A0-please_review Pull request needs code review. labels Jul 17, 2021
@Lldenaurois Lldenaurois force-pushed the dispute-coordinator-no-local-statements branch 2 times, most recently from 3f2ce2b to c5c6bce Compare July 18, 2021 21:48
@Lldenaurois Lldenaurois force-pushed the dispute-coordinator-no-local-statements branch from c5c6bce to f3c30fa Compare July 18, 2021 22:20
@Lldenaurois
Copy link
Contributor Author

@rphmeier How do we keep track of when we've recovered all Statements for all leaves? In this current setting we simply always handle each leaf as it comes in.

@Lldenaurois Lldenaurois force-pushed the dispute-coordinator-no-local-statements branch from f3c30fa to e37d6d7 Compare July 19, 2021 00:57
Comment on lines 445 to 451
let missing_local_statement = validators.iter()
.enumerate()
.map(|(index, validator)| (ValidatorIndex(index as _), validator))
.filter(|(index, validator)|
voted_indices.contains(index) ||
state.keystore.key_pair::<ValidatorPair>(validator).map(|v| v.is_none()).unwrap_or_default())
.count() > 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let missing_local_statement = validators.iter()
.enumerate()
.map(|(index, validator)| (ValidatorIndex(index as _), validator))
.filter(|(index, validator)|
voted_indices.contains(index) ||
state.keystore.key_pair::<ValidatorPair>(validator).map(|v| v.is_none()).unwrap_or_default())
.count() > 0;
let missing_local_statement = validators.iter()
.enumerate()
.map(|(index, validator)| (ValidatorIndex(index as _), validator))
.any(|(index, validator)|
!voted_indices.contains(index) &&
state.keystore.key_pair::<ValidatorPair>(validator).ok().flatten().map_or(false, |v| v.is_some())
);

re: https://github.com/paritytech/polkadot/pull/3481/files#r672552299 and https://github.com/paritytech/polkadot/pull/3481/files#r672551425

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see how this is correct. The iterator contains an entry for each validator which (has voted | has no key). We really want (has not voted & has a key).

map_or is more expressive than using unwrap_or_default implicitly evaluating to false as a default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will write a test to ensure we can ensure this property.

Copy link
Contributor

@rphmeier rphmeier left a comment

Choose a reason for hiding this comment

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

missing_local_statement logic seems wrong to me. Other than that it looks fine.

@rphmeier
Copy link
Contributor

bot rebase

@ghost
Copy link

ghost commented Jul 19, 2021

Rebasing.

@rphmeier
Copy link
Contributor

@Lldenaurois I triggered the rebase because I wanted to merge, but it seems like you wanted to have another test. Let's block until that's in and then merge

@rphmeier rphmeier merged commit cc6ef8c into paritytech:master Jul 21, 2021
ordian added a commit that referenced this pull request Jul 23, 2021
* master:
  Reduce staking miner reward (companion `substrate/pull/9395`) (#3465)
  Parachains shared.rs to Frame V2 (#3425)
  Parachains hrmp.rs to Frame V2 (#3475)
  Migrate slots pallet to pallet attribute macro. (#3218)
  Improve test in bridge (#3507)
  parachain dmp.rs to Frame V2 (#3426)
  Parachains inclusion.rs to Frame V2 (#3440)
  Dispute coordinator - Recover disputes on startup (#3481)
  Use correct syntax for owning all files in a folder (#3510)
  Add wococo-local chain spec (#3509)
  Dispute vote filtering for block authors (#3498)
  Bump indexmap from 1.6.1 to 1.7.0 (#3497)
  Companion for substrate #9315 (#3477)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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. I8-refactor Code needs refactoring.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dispute Coordinator should recover started disputes on startup
4 participants