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

Batch hash #932

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Batch hash #932

wants to merge 5 commits into from

Conversation

philsippl
Copy link
Contributor

@philsippl philsippl commented Jan 14, 2025

Calculates a hash out of the signup ids of the current batch and syncs this hash across the node. This makes sure the queues are actually in sync before starting processing. This just throws an error and the actor can't recover it on its own requiring manual intervention.

@philsippl philsippl changed the title Ps/batch hash Batch hash Jan 14, 2025
Comment on lines 1502 to 1507
let mut valid_merged = vec![false; max_batch_size];
for i in 0..self.comms[0].world_size() {
for j in 0..max_batch_size {
valid_merged[j] &= results[i][j] == 1;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we still keep this or fail? seems if we remove some entries than we risk them not being the same signupIds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But this can happen all the time. A client can even force this by sending us a wrongly encrypted share.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code has been there actually also before, just slightly rewritten, I can also restore the old version

Comment on lines 1502 to 1505
let mut valid_merged = vec![false; max_batch_size];
for i in 0..self.comms[0].world_size() {
for j in 0..max_batch_size {
valid_merged[j] &= results[i][j] == 1;
Copy link
Collaborator

@eaypek-tfh eaypek-tfh Jan 14, 2025

Choose a reason for hiding this comment

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

Given that we first set everything to false and then we apply &= operation, wouldn't it always resolve to false - no matter what rhs (results[i][j] == 1) is? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right, and this also shows an issue with our e2e tests. Since this means we don't return any results, there are no mismatches.

@philsippl philsippl requested a review from eaypek-tfh January 14, 2025 17:06
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