Disputes which are unknown for the Runtime are sent with priority by the Provisioner when preparing inherent data #5336

tdimitrov · 2022-04-18T09:22:51Z

This PR adds logic which improves the efficiency of the on-chain disputes importing in provisioner subsystem. First step for resolving #4573

Motivation

When the provisioner prepares inherent data (on message RequestInherentData) it requests all disputes the current node knows about from the disputes-coordinator (via DisputeCoordinatorMessage::RecentDisputes message). These disputes are sent to the Runtime no matter if they are already on-chain or not. Furthermore if the disputes count is too large - it is trimmed by randomly dropping disputes. This approach works but is not effective.

What's changed in the PR

Before preparing the disputes list the provisioner fetches all on-chain disputes by issuing a staging API call to the Runtime - RuntimeApiRequest::Disputes. Then the provisioner filters out the result from DisputeCoordinatorMessage::RecentDisputes by removing the disputes already imported on-chain. The rest of the logic remains the same.
The description above is not correct. A dispute occurring and concluding within the time a single block is created is a quite happy case. So it is not acceptable to import a dispute once and consider it done, because the node might receive new votes for ongoing disputes and they will never be sent on chain.
For this reason the PR implements the following logic:

Case 1: All RECENT disputes can be sent to the Runtime

Provisioner gets all RECENT disputes. They are sent to the runtime.

Case 2: RECENT disputes are too much (more than MAX_DISPUTES_FORWARDED_TO_RUNTIME). ACTIVE disputes fit in the limit.

Provisioner queries the Runtime about the disputes it knows about (with RuntimeApiRequest::Disputes).
The set of ACTIVE disputes is extended with RECENT ones until it reaches MAX_DISPUTES_FORWARDED_TO_RUNTIME limit. Disputes unknown to the Runtime are added with priority during the extension.

Case 3: Both RECENT and ACTIVE disputes are more than MAX_DISPUTES_FORWARDED_TO_RUNTIME limit.

Provisioner queries the Runtime about the disputes it knows about (with RuntimeApiRequest::Disputes).
Provisioner adds creates a set of ACTIVE disputes with size MAX_DISPUTES_FORWARDED_TO_RUNTIME. Disputes unknown to the Runtime are added with priority in this set.

Benefits

Potentially smaller dispute set is set to the Runtime
The chance to drop a dispute which is unknown on-chain is decreased thanks to the filtering mentioned above.

Feature flag

Due to the usage of a staging API call the whole functionality is put behind a feature flag - staging-client. All features using staging Runtime calls will be behind this feature flag.

node/core/runtime-api/src/cache.rs

node/subsystem-types/src/messages.rs

primitives/src/v2/mod.rs

node/core/provisioner/src/onchain_disputes.rs

node/core/provisioner/src/lib.rs

sandreim · 2022-04-18T11:21:18Z

node/core/provisioner/src/lib.rs

@@ -712,7 +714,20 @@ async fn select_disputes(
 	// In case of an overload condition, we limit ourselves to active disputes, and fill up to the
 	// upper bound of disputes to pass to wasm `fn create_inherent_data`.
 	// If the active ones are already exceeding the bounds, randomly select a subset.
-	let recent = request_disputes(sender, RequestType::Recent).await;
+	let mut recent = request_disputes(sender, RequestType::Recent).await;


Doesn't the dispute coordinator subsystem also scrape onchain votes? I imagine we can move this there and then we could have a RequestType::RecentOffChain (not sure if this is the best naming) request type that we can use here.

If I'm not mistaken what's scraped in the disputes-coordinator are the resolved disputes while here we are fetching the ongoing disputes so one way or another - we'll have to make the runtime API call.

In which module to do it - you have a point. It can also be done in the disputes-coordinator but for me it felt more natural to keep this logic in the provisioner because the inherent data is prepared there. I don't insist on this approach though. Keeping it in the disputes-coordinator is very reasonable too.

The on chain votes are the votes that are imported in that very block, not the overall disputes. Put another way, the votes the provisioner provides can later be found in the on chain votes of the created block. This is a backup mechanism, in case a validator misses disputes off-chain, it will still know about current disputes via those onchain vote scraping.

primitives/src/v2/mod.rs

node/subsystem-types/src/messages.rs

node/core/runtime-api/src/cache.rs

node/core/provisioner/src/lib.rs

node/core/provisioner/src/onchain_disputes.rs

node/core/provisioner/Cargo.toml

node/core/provisioner/src/onchain_disputes.rs

rphmeier · 2022-04-27T21:53:59Z

What's happening here? Seems stagnant for a week?

tdimitrov · 2022-04-28T07:26:02Z

What's happening here? Seems stagnant for a week?

I'm finishing the tests today and this will be ready for review.

node/core/provisioner/src/lib.rs

node/core/provisioner/src/onchain_disputes.rs

eskimor

In summary, I don't think that #4573 is resolved by this PR. In particular we don't take into account votes the chain knows about disputes at all. Also we still don't seem to care whether the dispute already concluded on chain. (Votes for such disputes can be imported with lower priority.)

With #4573, I don't think that we need random selection of votes/disputes at all anymore:

We limit to only information the chain does not know about
If that is still too much (by some metric), we just truncate the data. There is no need for random selection, as the remaining data will get imported on the next blocks, as the next block producers will again only include data the chain does not know about.

node/core/provisioner/src/lib.rs

tdimitrov · 2022-04-29T18:17:25Z

In summary, I don't think that #4573 is resolved by this PR.

I agree, this is actually half of the work and I forgot to remove 'Closes $4573' from the description. I edited it now.

About the rest, especially the votes - what I understood was that I should focus on the votes in separate PR. Is this correct or I should extend the PR to address the votes too?

P.S. After rereading the PR description I realise that I don't mention anywhere that the votes should be handled separately. My bad.

eskimor

nice work, @tdimitrov ! It certainly is an improvement a full #4573 can be a follow up.

node/core/provisioner/src/lib.rs

node/core/provisioner/src/onchain_disputes.rs

Co-authored-by: Andrei Sandu <54316454+sandreim@users.noreply.github.com>

Co-authored-by: Andronik <write@reusable.software>

…ing runtime api

Co-authored-by: Bernhard Schuster <bernhard@ahoi.io>

- Logging - Separate error module - Add additional fields for GetOnchainDisputesErr - logging and impl MallocSizeOf - fix impl MallocSizeOf for DisputeState - fix tests

Co-authored-by: Andronik <write@reusable.software>

dummy metrics instance Co-authored-by: Bernhard Schuster <bernhard@ahoi.io>

This reverts commit 6dc518c.

… functions

tdimitrov · 2022-05-06T12:56:15Z

bot merge

paritytech-processbot · 2022-05-06T12:56:21Z

Waiting for commit status.

tdimitrov force-pushed the disputes-import branch from ff2efae to 92515d6 Compare April 18, 2022 09:40

tdimitrov commented Apr 18, 2022

View reviewed changes

node/core/runtime-api/src/cache.rs Outdated Show resolved Hide resolved

tdimitrov commented Apr 18, 2022

View reviewed changes

node/subsystem-types/src/messages.rs Outdated Show resolved Hide resolved

tdimitrov commented Apr 18, 2022

View reviewed changes

primitives/src/v2/mod.rs Show resolved Hide resolved

tdimitrov force-pushed the disputes-import branch from 92515d6 to c1f587e Compare April 18, 2022 10:37

sandreim reviewed Apr 18, 2022

View reviewed changes

ordian reviewed Apr 19, 2022

View reviewed changes

eskimor reviewed Apr 19, 2022

View reviewed changes

node/core/provisioner/Cargo.toml Show resolved Hide resolved

node/core/provisioner/src/onchain_disputes.rs Outdated Show resolved Hide resolved

node/core/provisioner/src/onchain_disputes.rs Outdated Show resolved Hide resolved

tdimitrov 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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. labels Apr 20, 2022

tdimitrov force-pushed the disputes-import branch 3 times, most recently from e4f4e0f to dccb17e Compare April 20, 2022 13:35

tdimitrov force-pushed the disputes-import branch from e487906 to 4826cdb Compare April 28, 2022 11:23

tdimitrov marked this pull request as ready for review April 28, 2022 12:56

tdimitrov requested a review from drahnr April 28, 2022 13:00

tdimitrov force-pushed the disputes-import branch from 49b2049 to 6d41b0a Compare April 28, 2022 13:40

tdimitrov requested review from eskimor and ordian April 28, 2022 14:52

tdimitrov force-pushed the disputes-import branch 4 times, most recently from 3571e8e to a435768 Compare April 28, 2022 18:08

drahnr suggested changes Apr 28, 2022

View reviewed changes

node/core/provisioner/src/lib.rs Show resolved Hide resolved

node/core/provisioner/src/lib.rs Outdated Show resolved Hide resolved

node/core/provisioner/src/lib.rs Outdated Show resolved Hide resolved

node/core/provisioner/src/onchain_disputes.rs Outdated Show resolved Hide resolved

eskimor reviewed Apr 29, 2022

View reviewed changes

node/core/provisioner/src/lib.rs Show resolved Hide resolved

node/core/provisioner/src/lib.rs Show resolved Hide resolved

eskimor approved these changes May 2, 2022

View reviewed changes

tdimitrov and others added 19 commits May 6, 2022 08:46

Implementation of Disputes Runtime API message

bf78f4e

Modify on-chain dispute import

becdfb5

Add feature flag for the new functionality

ccf0d85

Update node/core/provisioner/src/onchain_disputes.rs

4087fd4

Co-authored-by: Andrei Sandu <54316454+sandreim@users.noreply.github.com>

Add target to log messages

2c0d4bd

Update node/core/provisioner/src/lib.rs

767d980

Co-authored-by: Andronik <write@reusable.software>

Use staging-client feature to enable the client code using the stag…

0c679c9

…ing runtime api

Remove TODO comment

a6a22de

Don't filter out DisputeState

564588f

Fix disputes selection logic

4e8ff70

spelling

40a7c5f

Tests

532b6bc

Rename Disputes message to StagingDisputes

6bb6d3b

Update node/core/provisioner/src/lib.rs

2ea33ac

Co-authored-by: Bernhard Schuster <bernhard@ahoi.io>

Code review feedback

edf5c04

- Logging - Separate error module - Add additional fields for GetOnchainDisputesErr - logging and impl MallocSizeOf - fix impl MallocSizeOf for DisputeState - fix tests

Update node/core/provisioner/src/error.rs

3c2c794

Co-authored-by: Andronik <write@reusable.software>

Update node/core/provisioner/src/lib.rs

03a60f1

Co-authored-by: Andronik <write@reusable.software>

Update node/core/provisioner/src/lib.rs

c02cb5c

Co-authored-by: Andronik <write@reusable.software>

Apply suggestions from code review

619d7f9

dummy metrics instance Co-authored-by: Bernhard Schuster <bernhard@ahoi.io>

tdimitrov force-pushed the disputes-import branch from 62c677a to b85c854 Compare May 6, 2022 05:47

tdimitrov added 4 commits May 6, 2022 15:18

Revert "Apply suggestions from code review"

5eb8f9b

This reverts commit 6dc518c.

Code review feedback: #[cfg(test)] for new_dummy() in metrics

828eb7e

Code review feedback: break the disputes generation logic in separate…

b44ca54

… functions

Code review feedback - align_eight

4b89c69

tdimitrov force-pushed the disputes-import branch from b85c854 to 4b89c69 Compare May 6, 2022 12:19

paritytech-processbot bot merged commit 73afd80 into paritytech:master May 6, 2022

tdimitrov deleted the disputes-import branch May 6, 2022 13:41

tdimitrov mentioned this pull request May 19, 2022

Improved dispute votes import in provisioner #5567

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disputes which are unknown for the Runtime are sent with priority by the Provisioner when preparing inherent data #5336

Disputes which are unknown for the Runtime are sent with priority by the Provisioner when preparing inherent data #5336

tdimitrov commented Apr 18, 2022 •

edited

Loading

sandreim Apr 18, 2022 •

edited

Loading

tdimitrov Apr 18, 2022

eskimor Apr 19, 2022

rphmeier commented Apr 27, 2022

tdimitrov commented Apr 28, 2022

eskimor left a comment

tdimitrov commented Apr 29, 2022 •

edited

Loading

eskimor left a comment

tdimitrov commented May 6, 2022

paritytech-processbot bot commented May 6, 2022

Disputes which are unknown for the Runtime are sent with priority by the Provisioner when preparing inherent data #5336

Disputes which are unknown for the Runtime are sent with priority by the Provisioner when preparing inherent data #5336

Conversation

tdimitrov commented Apr 18, 2022 • edited Loading

Motivation

What's changed in the PR

Benefits

Feature flag

sandreim Apr 18, 2022 • edited Loading

Choose a reason for hiding this comment

tdimitrov Apr 18, 2022

Choose a reason for hiding this comment

eskimor Apr 19, 2022

Choose a reason for hiding this comment

rphmeier commented Apr 27, 2022

tdimitrov commented Apr 28, 2022

eskimor left a comment

Choose a reason for hiding this comment

tdimitrov commented Apr 29, 2022 • edited Loading

eskimor left a comment

Choose a reason for hiding this comment

tdimitrov commented May 6, 2022

paritytech-processbot bot commented May 6, 2022

tdimitrov commented Apr 18, 2022 •

edited

Loading

sandreim Apr 18, 2022 •

edited

Loading

tdimitrov commented Apr 29, 2022 •

edited

Loading