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

fix bit size in puzzle #2509

Closed
wants to merge 1 commit into from

Conversation

qy3u
Copy link
Contributor

@qy3u qy3u commented Jul 1, 2024

The bit size of bls12-377 Fr is 253, not 254.

@vicsn vicsn requested a review from raychu86 July 1, 2024 13:59
@raychu86 raychu86 requested a review from d0cd July 2, 2024 20:43
@d0cd
Copy link
Contributor

d0cd commented Jul 3, 2024

The additional padding bit does not affect the correctness of the puzzle.
This change, while valid, would break compatibility with the existing puzzle.
As a result, I would not recommend this change.

@d0cd
Copy link
Contributor

d0cd commented Jul 3, 2024

Closing this issue for now, but please feel free to reopen if you have any strong concerns.

@d0cd d0cd closed this Jul 3, 2024
@qy3u
Copy link
Contributor Author

qy3u commented Jul 3, 2024

Hi @d0cd. I believe this change wouldn't be a breaking change.

Leave bits were converted to bytes before hashing. Therefore, hash_leave(&vec![false; 253]) and hash_leave(&vec![false; 254]) will produce the same result because vec![false; 254] and vec![false; 255] (with a prepended bit) are converted into the same bytes.

(1, 256) => bits_from_bytes_le(&sha3_256_native(&bytes_from_bits_le(input))).collect(),

If this change is valid and doesn't introduce any breaking changes, I think we should merge it during the Testnet phase. Let's avoid having these slightly odd [bool; 253], [bool; 253], ... , [bool; 254], ...] leaves in the mainnet.

@d0cd
Copy link
Contributor

d0cd commented Jul 3, 2024

Ah good catch @qy3u!
If its just a cosmetic change, then is there a strong reason for changing the code?
It would also be great if we could add tests verifying that solutions aren't changed

@vicsn
Copy link
Contributor

vicsn commented Jul 4, 2024

In case no change to the padding is made, I would still consider adding a comment in the code for future readers.

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.

3 participants