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

Make Sortition Hash compliant with specification #917

Merged

Conversation

fed-franz
Copy link
Contributor

@fed-franz fed-franz commented Jun 1, 2023

Resolves #907

This PR changes the sortition hash from $H(round||counter||step||seed)$ to $H(seed||round||step||counter)$,
to be compliant with Deterministic Sortition specification.

The following tests are also updated: test_sortition_hash, test_generate_sortition_score, test_collect_votes, test_deterministic_sortition_1, test_deterministic_sortition_2.

Note that test_collect_votes code has been also overhauled to make it simpler and easier to update in the future.

❗ IMPORTANT: This is a BREAKING CHANGE

@fed-franz fed-franz linked an issue Jun 1, 2023 that may be closed by this pull request
@fed-franz fed-franz changed the title Make Sortition Hash compliant with spefication Make Sortition Hash compliant with specification Jun 1, 2023
@fed-franz
Copy link
Contributor Author

IMHO, test_collect_votes should use a committee size of 64 instead of 10

@fed-franz fed-franz force-pushed the 907-make-sortition-hash-compliant-with-specification branch 2 times, most recently from 6af2ffe to 83f51d3 Compare June 2, 2023 10:41
@fed-franz fed-franz marked this pull request as ready for review June 2, 2023 11:17
Copy link
Member

@herr-seppia herr-seppia left a comment

Choose a reason for hiding this comment

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

Since we are aiming to test compatibility between node and dusk-blockchain I'm gonna to block this merge until the same changes are reflected on dusk-blockchain too

@herr-seppia herr-seppia dismissed their stale review August 9, 2023 09:58

No more dusk-blockchain compatibility is required

@herr-seppia
Copy link
Member

@fed-franz I've dismissed my blocking review. Please go on if this is still required

@fed-franz fed-franz force-pushed the 907-make-sortition-hash-compliant-with-specification branch from 83f51d3 to 5bcb3ec Compare August 9, 2023 14:05
@fed-franz
Copy link
Contributor Author

Does this mean the mirror issue on dusk-blockchain is not relevant anymore?

@fed-franz fed-franz requested a review from herr-seppia August 9, 2023 14:32
@fed-franz fed-franz force-pushed the 907-make-sortition-hash-compliant-with-specification branch from 5bcb3ec to 6de3313 Compare August 11, 2023 07:54
Copy link
Member

@herr-seppia herr-seppia left a comment

Choose a reason for hiding this comment

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

LGTM

This changes the hash from H(round||counter||step||seed)
to H(seed||round||step||counter), to be compliant with
Deterministic Sortition specification.

Tests 'test_sortition_hash()' and 'test_generate_sortition_score()'
are also updated.
Fixed tests:
 - test_collect_votes
 - test_deterministic_sortition_1
 - test_deterministic_sortition_2

test_collect_votes code has been simplified.
@fed-franz fed-franz force-pushed the 907-make-sortition-hash-compliant-with-specification branch from 6de3313 to 54c3aa4 Compare August 14, 2023 14:58
@fed-franz fed-franz merged commit 6a0b472 into master Aug 16, 2023
@fed-franz fed-franz deleted the 907-make-sortition-hash-compliant-with-specification branch August 16, 2023 07:21
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.

Make Sortition Hash compliant with specification
2 participants