Skip to content

fix: apply modulus in sharding#3530

Merged
Ivansete-status merged 1 commit intomasterfrom
correct-shard-mod
Aug 3, 2025
Merged

fix: apply modulus in sharding#3530
Ivansete-status merged 1 commit intomasterfrom
correct-shard-mod

Conversation

@Ivansete-status
Copy link
Copy Markdown
Collaborator

Description

It seems that the suggested operation is not equivalent for all cases.

For example, the following don't give the same results:

  echo "25 mod 7 == " & $(25.uint64 mod 7.uint64)
  echo "25 and (7 - 1) == " & $(25.uint64 and uint64(7 - 1))

Issue

@github-actions
Copy link
Copy Markdown

github-actions bot commented Aug 1, 2025

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:3530

Built from d36d7e1

@Ivansete-status Ivansete-status marked this pull request as ready for review August 1, 2025 10:55
Copy link
Copy Markdown
Contributor

@darshankabariya darshankabariya left a comment

Choose a reason for hiding this comment

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

Amazing catch, Thanks @Ivansete-status. LGTM

Copy link
Copy Markdown
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

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

Amazing!
Actually the original only works for count that is power of two.
Btw, it is never a good idea to optimize a code this way. Trust the compiler it knows better.
If you check with compiler explorer the C code equivalent compiles exactly according to what compiler can predict. https://godbolt.org/z/f7MaKeKzG
So such a solution can only work if you are sure the count is always power of two.

@Ivansete-status Ivansete-status merged commit 7611137 into master Aug 3, 2025
12 of 13 checks passed
@Ivansete-status Ivansete-status deleted the correct-shard-mod branch August 3, 2025 15:28
Ivansete-status added a commit that referenced this pull request Jan 28, 2026
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