Filter votes from disabled validators in `BackedCandidates` in process_inherent_data #1863

tdimitrov · 2023-10-12T12:46:30Z

Please refer to the issue for details.

tdimitrov · 2023-10-12T12:47:36Z

polkadot/runtime/parachains/src/paras_inherent/mod.rs

+
+// Filters statements from disabled validators in `BackedCandidate` and `MultiDisputeStatementSet`.
+// Returns `true` if at least one statement is removed and `false` otherwise.
+fn filter_backed_statements<T: Config>(


@ordian can you have a look at this? Does the index arithmetic here make sense?

I just merged #1257 that has a disabled_validators implementation, which could be extracted into a common module. There's a small caveat though, it doesn't work at session boundaries. But maybe that's not really an issue since availability cores are cleared out on session boundaries anyway.

I've moved your implementation in pallet_shared so that we can call it from the api impl and paras_inherent. Quite a lot of boilerplate code for such small change but it is what it is :)

If you have got an idea for a better place - please share.

ordian · 2023-10-12T13:15:33Z

polkadot/runtime/parachains/src/paras_inherent/mod.rs

@@ -480,6 +487,7 @@ impl<T: Config> Pallet<T> {
 			}

 			ensure!(all_weight_before.all_lte(max_block_weight), Error::<T>::InherentOverweight);
+			ensure!(!statements_were_dropped, Error::<T>::InherentOverweight);


do we want to reject blocks where at least one statement is dropped? what do we do with disputes?

Afair the else statement is executed during block execution only (right?) and this is a sanity check that we are not importing votes from disabled validators. We discussed this here.

polkadot/runtime/parachains/src/paras_inherent/mod.rs

…rait to fetch the disabled validators from paras_inherent.

…and use it it in paras_inherent too

* `set_claimqueue` in `scheduler` * `add_allowed_relay_parent` in `shared`

ordian

Good job! Looking cleaner now, left some tiny nits.

polkadot/runtime/parachains/src/paras_inherent/mod.rs

Co-authored-by: ordian <[email protected]>

Overkillus

Overall looking good and nothing blocking code-wise. Noted a few suggestions here and there.

A few bookkeeping issues I'd like to raise:

PR desc / linked issue
Docs (parainherent.md)

PR desc / linked issue
Firstly edit or post an update to the issue with the current desired behaviour that this PR should fix/add. For instance I don't see any dispute or availability bitfields filtering so either scratch them off the issue, split them into separate issues or add a checklist for the items. In our case I believe both were made redundant so feel free to link to my comment here and simply scratch them.
Docs (parainherent.md)
There's also a few things I'd like to get fixed around docs. Most logical changes apply to parainherent so I'd like to see that noted in the parainherent.md file. It's not only Rob's sketchpad but something we should maintain and this one should be a low hanging fruit. There's a lit of behaviours around disputes so now we can add one around disabling (filtering votes from disabled and rejecting them in re-execution).

polkadot/runtime/parachains/src/paras_inherent/mod.rs

polkadot/runtime/parachains/src/paras_inherent/tests.rs

Co-authored-by: Maciej <[email protected]>

…er_backed_statements_from_disabled_validators

tdimitrov · 2023-11-28T13:43:26Z

Thanks for the feedback @Overkillus. Regarding the issues you've raised:

PR desc / linked issue

The issue was written while disabling strategy was still work in progress. Editing the initial description will remove the context from the rest of the comments. We'd better not touch it. The PR title is rather self-explanatory I think. Let's leave it that way.

Docs (parainherent.md)

Good suggestion! I've added a section regarding data sanitization in the document.

Overkillus

Few nits and looking decent 👍

polkadot/roadmap/implementers-guide/src/runtime/parainherent.md

Co-authored-by: Maciej <[email protected]>

paritytech-cicd-pr · 2023-11-29T08:03:47Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4532515

…s_inherent_data (#1863) Fixes #1592 Please refer to the issue for details. --------- Co-authored-by: ordian <[email protected]> Co-authored-by: ordian <[email protected]> Co-authored-by: Maciej <[email protected]>

Closes #1591. The purpose of this PR is filter out backing statements from the network signed by disabled validators. This is just an optimization, since we will do filtering in the runtime in #1863 to avoid nodes to filter garbage out at block production time. - [x] Ensure it's ok to fiddle with the mask of manifests - [x] Write more unit tests - [x] Test locally - [x] simple zombienet test - [x] PRDoc --------- Co-authored-by: Tsvetomir Dimitrov <[email protected]>

…s_inherent_data (#2889) Backport of #1863 to master Extend candidate sanitation in paras_inherent by removing backing votes from disabled validators. Check #1592 for more details. This change is related to the disabling strategy implementation (#2226). --------- Co-authored-by: ordian <[email protected]> Co-authored-by: ordian <[email protected]> Co-authored-by: Maciej <[email protected]>

…s_inherent_data (#2889) Backport of paritytech/polkadot-sdk#1863 to master Extend candidate sanitation in paras_inherent by removing backing votes from disabled validators. Check paritytech/polkadot-sdk#1592 for more details. This change is related to the disabling strategy implementation (paritytech/polkadot-sdk#2226). --------- Co-authored-by: ordian <[email protected]> Co-authored-by: ordian <[email protected]> Co-authored-by: Maciej <[email protected]>

…s_inherent_data (paritytech#2889) Backport of paritytech#1863 to master Extend candidate sanitation in paras_inherent by removing backing votes from disabled validators. Check paritytech#1592 for more details. This change is related to the disabling strategy implementation (paritytech#2226). --------- Co-authored-by: ordian <[email protected]> Co-authored-by: ordian <[email protected]> Co-authored-by: Maciej <[email protected]>

* "refund" proof size in GRANDPa pallet * clippy * extra_proof_size_bytes_works * use saturated_into * fix review comments

tdimitrov added 2 commits October 12, 2023 15:42

Filter backing votes in paras_inherent - initial work

259cb4b

Error handling

d451886

tdimitrov commented Oct 12, 2023

View reviewed changes

ordian reviewed Oct 12, 2023

View reviewed changes

polkadot/runtime/parachains/src/paras_inherent/mod.rs Outdated Show resolved Hide resolved

Merge branch 'master' into tsv-disabling-runtime

a3bafba

ordian mentioned this pull request Oct 16, 2023

statement-distribution: validator disabling #1841

Merged

5 tasks

Merge branch 'tsv-disabling' into tsv-disabling-runtime

75d707a

tdimitrov changed the base branch from master to tsv-disabling October 16, 2023 11:37

tdimitrov added the A0-needs_burnin Pull request needs to be tested on a live validator node before merge. DevOps is notified via matrix label Oct 16, 2023

tdimitrov added 5 commits October 16, 2023 16:39

Add fn disabled_validators to trait DisabledValidators. Use the t…

cb0d8fc

…rait to fetch the disabled validators from paras_inherent.

Extract the implementation of disabled_validators from rutnime api …

8a8893e

…and use it it in paras_inherent too

Error handling

7b868b0

Remove DisabledValidators from paras_inherent

a41d21c

Fix a warning

34a3bf0

tdimitrov mentioned this pull request Oct 20, 2023

Disabling - feature PR #1955

Closed

4 tasks

tdimitrov added 10 commits October 23, 2023 10:53

Merge branch 'tsv-disabling' into tsv-disabling-runtime

88eb018

Merge branch 'tsv-disabling' into tsv-disabling-runtime

305d5b8

Add setters used for unit tests

e241312

* `set_claimqueue` in `scheduler` * `add_allowed_relay_parent` in `shared`

Mock for DisabledValidators in Test runtime

2aa1323

Fix test state and add an optimisation for filter_backed_statements

a8efd15

Make disabled validators mock mutable

3567a93

Extract common code in get_test_data

04dc32a

Additional cases

cce71ee

Fixes in filter_backed_statements

7a19f55

Refactor filter_backed_statements

3260187

tdimitrov marked this pull request as ready for review October 26, 2023 13:56

tdimitrov requested review from a team October 26, 2023 13:56

tdimitrov requested a review from athei as a code owner October 26, 2023 13:56

Merge branch 'tsv-disabling' into tsv-disabling-runtime

452b202

ordian linked an issue Nov 8, 2023 that may be closed by this pull request

Enforce validator disabling in the runtime #1592

Closed

ordian approved these changes Nov 9, 2023

View reviewed changes

tdimitrov and others added 2 commits November 15, 2023 13:53

Apply suggestions from code review

cb60cf0

Co-authored-by: ordian <[email protected]>

Code review feedback

750f2db

tdimitrov requested a review from Overkillus November 15, 2023 12:02

Merge branch 'tsv-disabling' into tsv-disabling-runtime

7022003

Overkillus approved these changes Nov 21, 2023

View reviewed changes

polkadot/runtime/parachains/src/paras_inherent/mod.rs Outdated Show resolved Hide resolved

polkadot/runtime/parachains/src/paras_inherent/mod.rs Outdated Show resolved Hide resolved

polkadot/runtime/parachains/src/paras_inherent/tests.rs Outdated Show resolved Hide resolved

tdimitrov and others added 3 commits November 28, 2023 13:41

Apply suggestions from code review: remove duplicated assert

8b5e5a8

Co-authored-by: Maciej <[email protected]>

Code review feedback - filter_backed_statements_from_disabled -> filt…

a50800f

…er_backed_statements_from_disabled_validators

Update parainherent.md with some details about data sanitization

7a308cd

Add newline to parainherent.md

7ccb942

Overkillus approved these changes Nov 28, 2023

View reviewed changes

tdimitrov and others added 4 commits November 28, 2023 19:08

Apply suggestions from code review

1834e20

Co-authored-by: Maciej <[email protected]>

Apply suggestions from code review

610b45d

Co-authored-by: Maciej <[email protected]>

Fixes in parainherent.md

83adc93

Merge branch 'tsv-disabling' into tsv-disabling-runtime

158557d

Merge branch 'tsv-disabling' into tsv-disabling-runtime

79f4c40

tdimitrov merged commit f2d4c48 into tsv-disabling Nov 30, 2023
9 of 10 checks passed

tdimitrov deleted the tsv-disabling-runtime branch November 30, 2023 11:16

Overkillus mentioned this pull request Dec 11, 2023

Enforce validator disabling in the runtime #1592

Closed

tdimitrov mentioned this pull request Jan 9, 2024

Filter votes from disabled validators in BackedCandidates in process_inherent_data #2889

Merged

bkchr pushed a commit that referenced this pull request Apr 10, 2024

"refund" proof size in GRANDPa pallet (#1863)

bae329c

* "refund" proof size in GRANDPa pallet * clippy * extra_proof_size_bytes_works * use saturated_into * fix review comments

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter votes from disabled validators in `BackedCandidates` in process_inherent_data #1863

Filter votes from disabled validators in `BackedCandidates` in process_inherent_data #1863

tdimitrov commented Oct 12, 2023 •

edited

Loading

tdimitrov Oct 12, 2023

ordian Oct 12, 2023

tdimitrov Oct 17, 2023 •

edited

Loading

ordian Oct 12, 2023

tdimitrov Oct 12, 2023

ordian left a comment

Overkillus left a comment •

edited

Loading

tdimitrov commented Nov 28, 2023

Overkillus left a comment

paritytech-cicd-pr commented Nov 29, 2023

Filter votes from disabled validators in BackedCandidates in process_inherent_data #1863

Filter votes from disabled validators in BackedCandidates in process_inherent_data #1863

Conversation

tdimitrov commented Oct 12, 2023 • edited Loading

tdimitrov Oct 12, 2023

Choose a reason for hiding this comment

ordian Oct 12, 2023

Choose a reason for hiding this comment

tdimitrov Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

ordian Oct 12, 2023

Choose a reason for hiding this comment

tdimitrov Oct 12, 2023

Choose a reason for hiding this comment

ordian left a comment

Choose a reason for hiding this comment

Overkillus left a comment • edited Loading

Choose a reason for hiding this comment

tdimitrov commented Nov 28, 2023

Overkillus left a comment

Choose a reason for hiding this comment

paritytech-cicd-pr commented Nov 29, 2023

Filter votes from disabled validators in `BackedCandidates` in process_inherent_data #1863

Filter votes from disabled validators in `BackedCandidates` in process_inherent_data #1863

tdimitrov commented Oct 12, 2023 •

edited

Loading

tdimitrov Oct 17, 2023 •

edited

Loading

Overkillus left a comment •

edited

Loading