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

Ignore the enforceDeterminism value #7441

Merged
merged 1 commit into from
Dec 12, 2022

Conversation

andir
Copy link
Member

@andir andir commented Dec 10, 2022

We used to set enforceDeterminism to true in the settings (by default) and thus did send a non-zero value over the wire. The value should probably be ignored as it should only matter if nrRounds is non-zero as well.

Having the old code here where the value is expected to be zero only works with the same version of Nix where we are sending zero. We should always test this against older Nix versions being client or server as otherwise upgrade in larger networks might be a pain.

Fixes 8e0946e

See #7099 (comment) for further discussion.

We used to set enforceDeterminism to true in the settings (by default)
and thus did send a non-zero value over the wire. The value should
probably be ignored as it should only matter if nrRounds is non-zero
as well.

Having the old code here where the value is expected to be zero only
works with the same version of Nix where we are sending zero. We
should always test this against older Nix versions being client or
server as otherwise upgrade in larger networks might be a pain.

Fixes 8e0946e
Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

Thanks so much for fixing!

Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @andir !

We should always test this against older Nix versions being client or server as otherwise upgrade in larger networks might be a pain.

Yes! We do test against an older Nix daemon, but not an older Nix client. All the logic is there, we “just” need to add it and look at all the tests failures in that scenario, see whether they are justified or not.

Would you feel like doing that by any chance? 😇

@thufschmitt thufschmitt merged commit c00fb26 into NixOS:master Dec 12, 2022
@Ericson2314
Copy link
Member

I have PRs that did that but they were never merged. It would be great @andir if you want to resurect them.

@andir andir deleted the ignoreEnforceDeterminism branch December 12, 2022 09:50
@andir
Copy link
Member Author

andir commented Dec 12, 2022

Thanks for the fix @andir !

We should always test this against older Nix versions being client or server as otherwise upgrade in larger networks might be a pain.

Yes! We do test against an older Nix daemon, but not an older Nix client. All the logic is there, we “just” need to add it and look at all the tests failures in that scenario, see whether they are justified or not.

Would you feel like doing that by any chance? 😇

I tried to figure out how that stuff currently works but cut off because I was lost in the mixture of bash and Nix.

Could probably figure it out but I did time box this as it already distracted me from other things.

I have PRs that did that but they were never merged. It would be great @andir if you want to resurect them.

@Ericson2314 Can you link to those? I could try rebasing / updating them.

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.

None yet

3 participants