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

feat: Add basic consensus check to support data proxy tally #344

Merged
merged 9 commits into from
Aug 28, 2024

Conversation

hacheigriega
Copy link
Member

@hacheigriega hacheigriega commented Aug 26, 2024

Explanation of Changes

Until now, consensus was determined solely based on the reveal data at a given data path (Except when None filter is applied, in which case there is no check for consensus at all). Following @Thomasvdam 's proposal, this PR adds an additional "basic consensus check," which checks for consensus on the tuple (exit_code, pub_keys) before filter is applied. If less than 2/3 of reveals have the same tuple, a newly added error type ErrNoBasicConsensus is returned so that tally VM is not executed.

Note that when checking for basic consensus, the specific exit code values do not matter. Whether an exit code is a success or not is checked in the function parseReveals(), which is run by all filter types except for None filter. If more than 1/3 of the reveals have non-zero exit codes, the error ErrCorruptReveals is returned.

For example,

exit_code, data
1,A
1,A
0,A
0,A

results in a "no basic consensus" error, while

exit_code, data
1,A
1,A
1,A
0,A

results in a corruption error.

Also note that in case of "no basic consensus" error, an all-zero outlier list is returned and tally VM is not executed, unlike in case of the general consensus error.

Updated filter specs here

Testing

Additional test cases were added to TestFilter.

Related PRs and Issues

Closes: #317

@hacheigriega hacheigriega changed the title feat: Tally support for data proxy feat: Add basic consensus check to support data proxy tally Aug 26, 2024
Copy link
Member

@Thomasvdam Thomasvdam 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 it would be nice if we can emit the 'agreed' upon public keys in the tally event. That will allow us to display it in the explorer and should help with debugging :)

x/tally/types/errors.go Show resolved Hide resolved
x/tally/keeper/filter.go Outdated Show resolved Hide resolved
Copy link
Member

@Thomasvdam Thomasvdam left a comment

Choose a reason for hiding this comment

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

The only thing I'm unsure about it whether we should emit the data-proxy pubkeys when there isn't 2/3rds with a successful execution. But this won't really matter until we do incentives so I think we should leave it like this and once we decide how the rewards are done for all the different scenarios we can adjust the event accordingly. :)

@hacheigriega
Copy link
Member Author

The only thing I'm unsure about it whether we should emit the data-proxy pubkeys when there isn't 2/3rds with a successful execution. But this won't really matter until we do incentives so I think we should leave it like this and once we decide how the rewards are done for all the different scenarios we can adjust the event accordingly. :)

Noted. Thanks for the review!

@hacheigriega hacheigriega merged commit 3811dff into main Aug 28, 2024
16 of 17 checks passed
@hacheigriega hacheigriega deleted the hy/dp-tally branch August 28, 2024 19:01
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.

✨ Data Proxy in Tally Consensus
2 participants