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

Malus: improvements in dispute ancestor and suggest garbage candidate implementation #5011

Merged
merged 46 commits into from
Apr 13, 2022
Merged

Malus: improvements in dispute ancestor and suggest garbage candidate implementation #5011

merged 46 commits into from
Apr 13, 2022

Conversation

sandreim
Copy link
Contributor

@sandreim sandreim commented Mar 1, 2022 •
edited
Loading

First step(s), see #4874

  • Implemented fake validation as a component that is reused in dispute_valid_candidate.rs and suggest garbage candidate.rs.
  • The fake validation is configurable to report valid candidates as invalid or report malicious candidates as valid to the backing/approval subsystems.
  • Implemented suggest garbage candidate using the draft from Malus: Implement storing invalid chunks #4711 as a starting point

These two malus nemesis implementations will be used in the parachain zombienet test suite:

  • malicious nodes initiate disputes of valid candidates (dispute_valid_candidate)
  • honest nodes initiate disputes of invalid candidates backed by malicious nodes

I attached some screenshots to show how suggest garbage candidate malus nodes perform in a zombienet test. The first image shows disputes for malicious candidates concluding as valid and the inclusion blocks being finalized. The next two show disputes concluding as invalid and chain reversions.

This shows up on honest validators when they run the PVF with a malicious PoV:

2022-03-30 14:02:30 panicked at 'invalid block data format.: Error', /home/sandreim/polkadot/parachain/test-parachains/undying/src/wasm_[validation.rs:30](http://validation.rs:30/):58

Screenshot 2022-03-30 at 17 04 47

Screenshot 2022-03-30 at 17 48 43

Screenshot 2022-03-30 at 17 59 32

@sandreim sandreim added A3-in_progress Pull request is in progress. No review needed at this stage. 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. labels Mar 1, 2022
@sandreim
Merge branch 'master' of github.com:paritytech/polkadot into sandreim…
9941f03 
…/malus_improvement
@sandreim
cargo lock
a3172b0 
Signed-off-by: Andrei Sandu <[email protected]>
@sandreim
spell check
9100866 
Signed-off-by: Andrei Sandu <[email protected]>
@sandreim
spellcheck
efd37ad 
Signed-off-by: Andrei Sandu <[email protected]>
@sandreim
typos
d4da58d 
Signed-off-by: Andrei Sandu <[email protected]>
@sandreim sandreim marked this pull request as ready for review March 2, 2022 14:49
@github-actions github-actions bot removed the A3-in_progress Pull request is in progress. No review needed at this stage. label Mar 2, 2022
@sandreim sandreim requested a review from drahnr March 2, 2022 14:50
@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Mar 2, 2022
drahnr
drahnr reviewed Mar 4, 2022
View reviewed changes
node/malus/src/malus.rs Outdated Show resolved Hide resolved
drahnr
drahnr approved these changes Mar 10, 2022
View reviewed changes
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, Valid should be implemented before merging. Besides that - 👍

node/malus/src/malus.rs Outdated Show resolved Hide resolved
node/malus/src/variants/dispute_valid_candidates.rs Outdated Show resolved Hide resolved
node/malus/src/variants/dispute_valid_candidates.rs Outdated Show resolved Hide resolved
node/malus/src/variants/dispute_valid_candidates.rs Outdated Show resolved Hide resolved
node/malus/src/variants/dispute_valid_candidates.rs Outdated Show resolved Hide resolved
node/malus/src/variants/dispute_valid_candidates.rs Outdated Show resolved Hide resolved
sandreim and others added 7 commits March 23, 2022 14:16
@sandreim
Merge branch 'master' of github.com:paritytech/polkadot into sandreim…
9e72a42 
…/malus_improvement
@sandreim
chores
c4a4560 
Signed-off-by: Andrei Sandu <[email protected]>
@sandreim
Impl valid - still wip
e90ab09 
Signed-off-by: Andrei Sandu <[email protected]>
@sandreim
fixes
8226f4e 
Signed-off-by: Andrei Sandu <[email protected]>
@sandreim
fmt
35c8c8e 
Signed-off-by: Andrei Sandu <[email protected]>
@sandreim @Lldenaurois
Pull Ladi's implementation:
1c279dc 
#4711

Co-authored-by: Lldenaurois <[email protected]>
Co-authored-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
@sandreim
Fix build
8932494 
Signed-off-by: Andrei Sandu <[email protected]>
@sandreim sandreim requested a review from eskimor April 1, 2022 11:53
@sandreim
spellcheck
7d6b472 
Signed-off-by: Andrei Sandu <[email protected]>
@sandreim
some more fixes
3103ce7 
Signed-off-by: Andrei Sandu <[email protected]>
eskimor
eskimor reviewed Apr 7, 2022
View reviewed changes
Copy link
Member

@eskimor eskimor left a comment

Choose a reason for hiding this comment

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

First pass, got half way through. Remaining review coming soon.

node/core/approval-voting/src/lib.rs Outdated Show resolved Hide resolved
node/core/backing/src/lib.rs Outdated Show resolved Hide resolved
node/core/candidate-validation/src/lib.rs Show resolved Hide resolved
node/core/candidate-validation/src/tests.rs Outdated Show resolved Hide resolved
eskimor
eskimor reviewed Apr 7, 2022
View reviewed changes
Copy link
Member

@eskimor eskimor left a comment

Choose a reason for hiding this comment

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

Looks good overall - code is clean and relatively easy to follow as well.

node/malus/src/variants/common.rs Outdated Show resolved Hide resolved
node/malus/src/variants/common.rs Show resolved Hide resolved
node/malus/src/variants/common.rs Show resolved Hide resolved
node/malus/src/variants/dispute_valid_candidates.rs Outdated Show resolved Hide resolved
@sandreim
Refactor and fix it
bde2403 
Signed-off-by: Andrei Sandu <[email protected]>
@sandreim
review feedback
3784972 
Signed-off-by: Andrei Sandu <[email protected]>
@sandreim
typo
991d77f 
Signed-off-by: Andrei Sandu <[email protected]>
@sandreim
tests review feedback
0fa5890 
Signed-off-by: Andrei Sandu <[email protected]>
@sandreim
refactor disputer
71c35a9 
Signed-off-by: Andrei Sandu <[email protected]>
@sandreim
fix tests
fee4722 
Signed-off-by: Andrei Sandu <[email protected]>
@sandreim
Fix zombienet disputes test
6fb63d2 
Signed-off-by: Andrei Sandu <[email protected]>
@sandreim sandreim requested a review from drahnr April 11, 2022 09:15
@sandreim
spellcheck
ab22c59 
Signed-off-by: Andrei Sandu <[email protected]>
@sandreim
fix
f67b522 
Signed-off-by: Andrei Sandu <[email protected]>
@sandreim
Fix ui tests
6181157 
Signed-off-by: Andrei Sandu <[email protected]>
@sandreim sandreim mentioned this pull request Aug 24, 2023
Open
@sandreim
fix typo
155524c 
Signed-off-by: Andrei Sandu <[email protected]>
@sandreim
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit c8c1933 into master Apr 13, 2022
@paritytech-processbot paritytech-processbot bot deleted the sandreim/malus_improvement branch April 13, 2022 13:45
@ordian ordian mentioned this pull request Nov 10, 2022
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@drahnr drahnr drahnr approved these changes

@eskimor eskimor eskimor approved these changes

@Lldenaurois Lldenaurois Awaiting requested review from Lldenaurois

Assignees
No one assigned
Labels
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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sandreim @drahnr @eskimor