Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

don't throw ErrProducerEquivocated if authored blocks have different parents #2709

Merged
merged 52 commits into from
Nov 2, 2022

Conversation

kishansagathiya
Copy link
Contributor

Changes

Waiting on some spec related clarification

Tests

go test -tags integration github.com/ChainSafe/gossamer

Issues

Fixes #2682

Primary Reviewer

@timwu20

@codecov
Copy link

codecov bot commented Jul 28, 2022

Codecov Report

Merging #2709 (40d0c1f) into development (531f0aa) will increase coverage by 0.11%.
The diff coverage is 72.85%.

Additional details and impacted files
@@               Coverage Diff               @@
##           development    #2709      +/-   ##
===============================================
+ Coverage        63.14%   63.25%   +0.11%     
===============================================
  Files              219      219              
  Lines            27563    27590      +27     
===============================================
+ Hits             17404    17452      +48     
+ Misses            8545     8525      -20     
+ Partials          1614     1613       -1     

@kishansagathiya kishansagathiya marked this pull request as ready for review August 4, 2022 15:36
lib/babe/verify.go Outdated Show resolved Hide resolved
@qdm12
Copy link
Contributor

qdm12 commented Aug 8, 2022

Also would you be able to add a test case to cover such condition?
Or are unit tests messy such that's it's not easily feasible?

Copy link
Contributor

@noot noot left a comment

Choose a reason for hiding this comment

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

i think the whole equivocation check is wrong, afaik equivocation for block producers is defined as authoring two blocks in the same slot. we're allowed to author multiple blocks w the same number as long as they're in different slots, but the check is currently checking for multiple blocks w the same number, instead of multiple blocks in the same slot

@kishansagathiya
Copy link
Contributor Author

afaik equivocation for block producers is defined as authoring two blocks in the same slot.

Seems right. Will edit equivocation check accordingly.

https://github.com/paritytech/substrate/blob/8e600ee0515122667cd091d632104708d343ca34/client/consensus/slots/src/aux_schema.rs#L53

@timwu20
Copy link
Contributor

timwu20 commented Aug 11, 2022

afaik equivocation for block producers is defined as authoring two blocks in the same slot.

Seems right. Will edit equivocation check accordingly.

https://github.com/paritytech/substrate/blob/8e600ee0515122667cd091d632104708d343ca34/client/consensus/slots/src/aux_schema.rs#L53

@kishansagathiya is this PR ready for review then?

@kishansagathiya kishansagathiya marked this pull request as draft August 12, 2022 10:04
@kishansagathiya
Copy link
Contributor Author

@timwu20 made it draft, have to re-do equivocation logic after noot's comments

@kishansagathiya kishansagathiya marked this pull request as ready for review August 16, 2022 15:03
@kishansagathiya kishansagathiya changed the title don't through ErrProducerEquivocated if authored blocks have different parents don't throw ErrProducerEquivocated if authored blocks have different parents Sep 26, 2022
@kishansagathiya kishansagathiya requested a review from noot October 14, 2022 09:38
dot/state/block.go Outdated Show resolved Hide resolved
dot/state/block.go Outdated Show resolved Hide resolved
dot/state/block.go Show resolved Hide resolved
dot/types/babe.go Show resolved Hide resolved
lib/babe/state.go Outdated Show resolved Hide resolved
lib/babe/verify_test.go Outdated Show resolved Hide resolved
lib/blocktree/blocktree.go Outdated Show resolved Hide resolved
lib/blocktree/blocktree.go Outdated Show resolved Hide resolved
lib/blocktree/blocktree.go Outdated Show resolved Hide resolved
lib/babe/errors.go Show resolved Hide resolved
lib/babe/verify.go Show resolved Hide resolved
dot/state/block.go Outdated Show resolved Hide resolved
@kishansagathiya kishansagathiya requested a review from qdm12 October 31, 2022 05:52
dot/state/block.go Outdated Show resolved Hide resolved
dot/state/block.go Outdated Show resolved Hide resolved
dot/state/block_test.go Show resolved Hide resolved
lib/babe/verify.go Outdated Show resolved Hide resolved
lib/babe/verify.go Outdated Show resolved Hide resolved
lib/babe/verify_integration_test.go Outdated Show resolved Hide resolved
lib/babe/verify_test.go Outdated Show resolved Hide resolved
lib/babe/verify_test.go Outdated Show resolved Hide resolved
@kishansagathiya kishansagathiya merged commit 30f903b into development Nov 2, 2022
@kishansagathiya kishansagathiya deleted the kishan/fix/err-equivocatory-blocks branch November 2, 2022 04:51
@github-actions
Copy link

🎉 This PR is included in version 0.7.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

could not verify slot claim VRF proof
5 participants