Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
a5ff4ca
Don't import backing statements directly
eskimor Jun 16, 2022
0155ab9
Get rid of dead code.
eskimor Jun 16, 2022
550aa6b
Dont send approval vote
eskimor Jun 18, 2022
c2d2b1d
Make it pass CI
eskimor Jun 18, 2022
7f43db1
Merge branch 'rk-only-chain-vote-import' into rk-perf-test
eskimor Jun 18, 2022
f97e43b
Bring back tests for fixing them later.
eskimor Jul 14, 2022
c49214d
Explicit signature check.
eskimor Jul 14, 2022
7ee2e14
Resurrect approval-voting tests (not fixed yet)
eskimor Jul 17, 2022
6a99120
Send out approval votes in dispute-distribution.
eskimor Jul 17, 2022
5ae234e
Bring back an important warning.
eskimor Jul 17, 2022
7f950a9
Fix approval voting tests.
eskimor Jul 18, 2022
986e01d
Don't send out dispute message on import + test
eskimor Jul 19, 2022
32cabb4
Guide changes.
eskimor Jul 19, 2022
19ecc78
WIP: guide changes.
eskimor Jul 28, 2022
19833e6
Finish guide changes about dispute-coordinator
eskimor Aug 2, 2022
32e67a8
Finish guide changes for now.
eskimor Aug 3, 2022
98d4389
Remove own approval vote import logic.
eskimor Aug 4, 2022
e5fb770
Implement logic for retrieving approval-votes
eskimor Aug 4, 2022
24275ba
Update roadmap/implementers-guide/src/node/disputes/dispute-coordinat…
eskimor Aug 5, 2022
dd0ac1d
Review feedback.
eskimor Aug 8, 2022
912daf3
Incorporate Review Remarks
eskimor Aug 8, 2022
39e913a
Merge remote-tracking branch 'origin/rk-fast-dispute-coordinator' int…
eskimor Aug 8, 2022
11ab0e3
Get rid of superfluous space.
eskimor Aug 8, 2022
a890970
Tidy up import logic a bit.
eskimor Aug 9, 2022
8395724
We don't need/have empty imports.
eskimor Aug 10, 2022
31e0769
Fix tests and bugs.
eskimor Aug 10, 2022
2011f35
Remove error prone redundancy.
eskimor Aug 10, 2022
6624bc6
Import approval votes on dispute initiated/concluded.
eskimor Aug 10, 2022
e3be4c2
Add test for approval vote import.
eskimor Aug 10, 2022
24537b6
Make guide checker happy (hopefully)
eskimor Aug 11, 2022
793666b
Another sanity check + better logs.
eskimor Aug 11, 2022
943b064
Reasoning about boundedness.
eskimor Aug 11, 2022
4287137
Use `CandidateIndex` as opposed to `CoreIndex`.
eskimor Aug 11, 2022
8ae6797
Remove redundant import.
eskimor Aug 11, 2022
c126b76
Review remarks.
eskimor Aug 12, 2022
b11eadf
Add metric for calls to request signatures
eskimor Aug 12, 2022
b356acc
More review remarks.
eskimor Aug 12, 2022
f8cea8a
Add metric on imported approval votes.
eskimor Aug 12, 2022
d46b03a
Include candidate hash in logs.
eskimor Aug 12, 2022
1cdf49c
Merge branch 'master' into rk-fast-dispute-coordinator
eskimor Aug 13, 2022
73669dc
More trace log
eskimor Aug 13, 2022
4ac86f4
Break cycle.
eskimor Aug 15, 2022
e6754bb
Add some tracing.
eskimor Aug 15, 2022
3ec17cc
Cleanup allowed messages.
eskimor Aug 15, 2022
2c7fa5c
fmt
eskimor Aug 15, 2022
2b44d94
Tracing + timeout for get inherent data.
eskimor Aug 15, 2022
de0e85f
Better error.
eskimor Aug 15, 2022
23f1ee0
Break cycle in all places.
eskimor Aug 15, 2022
0ed7a1a
Clarified comment some more.
eskimor Aug 15, 2022
4466d30
Typo.
eskimor Aug 15, 2022
a002cb6
Break cycle approval-distribution - approval-voting.
eskimor Aug 15, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Don't import backing statements directly
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
  • Loading branch information
eskimor committed Jun 16, 2022
commit a5ff4caa06a684e50bd721ba29e38f4895d9d561
89 changes: 4 additions & 85 deletions node/core/backing/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,15 @@ use futures::{

use error::{Error, FatalResult};
use polkadot_node_primitives::{
AvailableData, InvalidCandidate, PoV, SignedDisputeStatement, SignedFullStatement, Statement,
ValidationResult, BACKING_EXECUTION_TIMEOUT,
AvailableData, InvalidCandidate, PoV, SignedFullStatement, Statement, ValidationResult,
BACKING_EXECUTION_TIMEOUT,
};
use polkadot_node_subsystem::{
jaeger,
messages::{
AvailabilityDistributionMessage, AvailabilityStoreMessage, CandidateBackingMessage,
CandidateValidationMessage, CollatorProtocolMessage, DisputeCoordinatorMessage,
ProvisionableData, ProvisionerMessage, RuntimeApiRequest, StatementDistributionMessage,
CandidateValidationMessage, CollatorProtocolMessage, ProvisionableData, ProvisionerMessage,
RuntimeApiRequest, StatementDistributionMessage,
},
overseer, ActiveLeavesUpdate, FromOrchestra, OverseerSignal, PerLeafSpan, SpawnedSubsystem,
Stage, SubsystemError,
Expand Down Expand Up @@ -380,7 +380,6 @@ async fn handle_active_leaves_update<Context>(

let job = CandidateBackingJob {
parent,
session_index,
assignment,
required_collator,
issued_statements: HashSet::new(),
Expand Down Expand Up @@ -411,8 +410,6 @@ struct JobAndSpan<Context> {
struct CandidateBackingJob<Context> {
/// The hash of the relay parent on top of which this job is doing it's work.
parent: Hash,
/// The session index this corresponds to.
session_index: SessionIndex,
/// The `ParaId` assigned to this validator
assignment: Option<ParaId>,
/// The collator required to author the candidate, if any.
Expand Down Expand Up @@ -783,8 +780,6 @@ async fn validate_and_make_available(
tx_command.send((relay_parent, make_command(res))).await.map_err(Into::into)
}

struct ValidatorIndexOutOfBounds;

#[overseer::contextbounds(CandidateBacking, prefix = self::overseer)]
impl<Context> CandidateBackingJob<Context> {
async fn handle_validated_candidate_command(
Expand Down Expand Up @@ -1014,21 +1009,6 @@ impl<Context> CandidateBackingJob<Context> {
)
};

if let Err(ValidatorIndexOutOfBounds) = self
.dispatch_new_statement_to_dispute_coordinator(ctx.sender(), candidate_hash, &statement)
.await
{
gum::warn!(
target: LOG_TARGET,
session_index = ?self.session_index,
relay_parent = ?self.parent,
validator_index = statement.validator_index().0,
"Supposedly 'Signed' statement has validator index out of bounds."
);

return Ok(None)
}

let stmt = primitive_statement_to_table(statement);

let summary = self.table.import_statement(&self.table_context, stmt);
Expand Down Expand Up @@ -1083,67 +1063,6 @@ impl<Context> CandidateBackingJob<Context> {
Ok(summary)
}

/// The dispute coordinator keeps track of all statements by validators about every recent
/// candidate.
///
/// When importing a statement, this should be called access the candidate receipt either
/// from the statement itself or from the underlying statement table in order to craft
/// and dispatch the notification to the dispute coordinator.
///
/// This also does bounds-checking on the validator index and will return an error if the
/// validator index is out of bounds for the current validator set. It's expected that
/// this should never happen due to the interface of the candidate backing subsystem -
/// the networking component responsible for feeding statements to the backing subsystem
/// is meant to check the signature and provenance of all statements before submission.
async fn dispatch_new_statement_to_dispute_coordinator(
&self,
sender: &mut impl overseer::CandidateBackingSenderTrait,
candidate_hash: CandidateHash,
statement: &SignedFullStatement,
) -> Result<(), ValidatorIndexOutOfBounds> {
// Dispatch the statement to the dispute coordinator.
let validator_index = statement.validator_index();
let signing_context =
SigningContext { parent_hash: self.parent, session_index: self.session_index };

let validator_public = match self.table_context.validators.get(validator_index.0 as usize) {
None => return Err(ValidatorIndexOutOfBounds),
Some(v) => v,
};

let maybe_candidate_receipt = match statement.payload() {
Statement::Seconded(receipt) => Some(receipt.to_plain()),
Statement::Valid(candidate_hash) => {
// Valid statements are only supposed to be imported
// once we've seen at least one `Seconded` statement.
self.table.get_candidate(&candidate_hash).map(|c| c.to_plain())
},
};

let maybe_signed_dispute_statement = SignedDisputeStatement::from_backing_statement(
statement.as_unchecked(),
signing_context,
validator_public.clone(),
)
.ok();

if let (Some(candidate_receipt), Some(dispute_statement)) =
(maybe_candidate_receipt, maybe_signed_dispute_statement)
{
sender
.send_message(DisputeCoordinatorMessage::ImportStatements {
candidate_hash,
candidate_receipt,
session: self.session_index,
statements: vec![(dispute_statement, validator_index)],
pending_confirmation: None,
})
.await;
}

Ok(())
}

async fn handle_second_msg(
&mut self,
root_span: &jaeger::Span,
Expand Down
Loading