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 most issues related to dependency update, except hashbrown version #349

Merged
merged 3 commits into from
Oct 21, 2024

Conversation

Nicceboy
Copy link
Contributor

Fixes most of the issues related to dependency updates.
Could it be possible to do large changes in branch so that main does not end up into bad state?

hashbrown dependency update seems to break encoding of BitString type in BER for some standard tests, I don't know why yet. Should we downgrade that for now?

E.g. cargo test -p rasn-cms --test test_cms test_cms_signed 1> error.log

shows failures on BitString types only.

error.log

@Nicceboy
Copy link
Contributor Author

Nicceboy commented Oct 21, 2024

I think the issue is related to the change of the default hashing algorithm of hashbrown and custom SET OF hash function does not work as expected.

@XAMPPRocky
Copy link
Collaborator

hashbrown dependency update seems to break encoding of BitString type in BER for some standard tests, I don't know why yet. Should we downgrade that for now?

We can downgrade for now, but let's make an issue for this as I don't want us to be restricted in what hashing algorithm we use, anything that depends on that should be changed or removed.

@Nicceboy
Copy link
Contributor Author

Nicceboy commented Oct 21, 2024

hashbrown dependency update seems to break encoding of BitString type in BER for some standard tests, I don't know why yet. Should we downgrade that for now?

We can downgrade for now, but let's make an issue for this as I don't want us to be restricted in what hashing algorithm we use, anything that depends on that should be changed or removed.

The issue seems to be that new hashing algorithm is not deterministic anymore. Is deterministic too strict requirement? Only SET OF type would require that for equality checks. I don't know how we could do the type otherwise. Another option might be to use some shared hasher instance for both source and target in equality checks but I don't know if it will introduce other issues.

@XAMPPRocky
Copy link
Collaborator

Only SET OF type would require that for equality checks. I don't know how we could do the type otherwise.

I'm not sure what you mean, why would it be required? A regular Vec or HashSet don't rely on the hashes being equal for PartialEq implementations.

@XAMPPRocky XAMPPRocky merged commit d1208da into librasn:main Oct 21, 2024
65 checks passed
@Nicceboy
Copy link
Contributor Author

Only SET OF type would require that for equality checks. I don't know how we could do the type otherwise.

I'm not sure what you mean, why would it be required? A regular Vec or HashSet don't rely on the hashes being equal for PartialEq implementations.

SetOf was a special type since the order of the elements should never matter. For that to be true, equality check nor hash should not depend on the order. Since the type cannot have Ord or PartialOrd requirement for elements, we need to rely on hashes on equality check. If hashing alrogithm includes randomness, the check will fail.

@Nicceboy Nicceboy deleted the dep-update-fixes branch October 21, 2024 14:37
@XAMPPRocky
Copy link
Collaborator

SetOf was a special type since the order of the elements should never matter. For that to be true, equality check nor hash should not depend on the order. Since the type cannot have Ord or PartialOrd requirement for elements, we need to rely on hashes on equality check. If hashing alrogithm includes randomness, the check will fail.

I'm still not sure how that isn't solved by the following:

impl<T, S> PartialEq for HashSet<T, S>
where
    T: Eq + Hash,
    S: BuildHasher,
{
    fn eq(&self, other: &HashSet<T, S>) -> bool {
        if self.len() != other.len() {
            return false;
        }

        self.iter().all(|key| other.contains(key))
    }
}

@Nicceboy
Copy link
Contributor Author

SetOf was a special type since the order of the elements should never matter. For that to be true, equality check nor hash should not depend on the order. Since the type cannot have Ord or PartialOrd requirement for elements, we need to rely on hashes on equality check. If hashing alrogithm includes randomness, the check will fail.

I'm still not sure how that isn't solved by the following:

impl<T, S> PartialEq for HashSet<T, S>
where
    T: Eq + Hash,
    S: BuildHasher,
{
    fn eq(&self, other: &HashSet<T, S>) -> bool {
        if self.len() != other.len() {
            return false;
        }

        self.iter().all(|key| other.contains(key))
    }
}

It doesn't note the variance between duplicate items.. but if we add that maybe it could work.

@Nicceboy
Copy link
Contributor Author

Actually, the issue remains as the key is a hash value and if the algorithm is not deterministic, key might not be in the other object.

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.

2 participants