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

Upgrade dependencies #52

Merged
merged 7 commits into from
Aug 23, 2019
Merged

Upgrade dependencies #52

merged 7 commits into from
Aug 23, 2019

Conversation

kigawas
Copy link
Contributor

@kigawas kigawas commented Aug 16, 2019

No description provided.

Upgrade ring to 0.14
@kigawas kigawas changed the title Upgrade ring to 0.14 Upgrade dependencies Aug 16, 2019
@kigawas kigawas closed this Aug 16, 2019
@kigawas kigawas deleted the patch-1 branch August 16, 2019 02:05
@kigawas kigawas restored the patch-1 branch August 16, 2019 02:12
@kigawas kigawas reopened this Aug 16, 2019
@kigawas kigawas mentioned this pull request Aug 16, 2019
@kigawas
Copy link
Contributor Author

kigawas commented Aug 16, 2019

Don't know why one check succeeded and another failed

@kigawas
Copy link
Contributor Author

kigawas commented Aug 16, 2019

@kigawas
Copy link
Contributor Author

kigawas commented Aug 16, 2019

Looks like it randomly failed at check_bit_length....

@omershlo
Copy link
Contributor

yes, there's an issue I think about it. it s a probabilistic test and therefore it fails once in a while

@kigawas
Copy link
Contributor Author

kigawas commented Aug 16, 2019

@omershlo
Looks like it always worked on my dev machine and github's builtin travis

But repeatedly failed on travis-ci.org

@omershlo
Copy link
Contributor

coincidence I would say.

@kigawas
Copy link
Contributor Author

kigawas commented Aug 16, 2019

Cool, I guess it's better to remove travis-ci.org check

@kigawas kigawas closed this Aug 22, 2019
@kigawas kigawas reopened this Aug 22, 2019
@kigawas
Copy link
Contributor Author

kigawas commented Aug 23, 2019

@omershlo
Did some minor updates on this, do you have some time to review?

@omershlo
Copy link
Contributor

Yes, I was planning on doing it over the weekend. My main issue is with updates to secp256k1 library. I need to make sure what changes are included in that version and why not bump it to the latest

@omershlo omershlo merged commit e8f1ab6 into ZenGo-X:master Aug 23, 2019
@kigawas kigawas deleted the patch-1 branch August 23, 2019 15:12
@kigawas
Copy link
Contributor Author

kigawas commented Sep 4, 2019

yes, there's an issue I think about it. it s a probabilistic test and therefore it fails once in a while

I guess I know why.
It should be related with prefix zeros in hex format of BigInt numbers

let l63 = BigInt::from_str_radix(
    "96208c61220f2cce171f0c1d10db7c094245fb66b17b9de3e0c4fbf4e68e73d",
    16,
)
.unwrap();
assert_eq!(l63.to_str_radix(16).len(), 63);
let l63 = BigInt::from_str_radix(
    "096208c61220f2cce171f0c1d10db7c094245fb66b17b9de3e0c4fbf4e68e73d",
    16,
)
.unwrap();
assert_eq!(l63.to_str_radix(16).len(), 63);

@kigawas
Copy link
Contributor Author

kigawas commented Sep 4, 2019

And since HashCommitment is the BigInt format of sha256, it probably starts with zero, this check is not appropriate.

@omershlo
Copy link
Contributor

omershlo commented Sep 4, 2019

This check is indeed problematic because of the probabilistic nature. Do you suggest we just remove it ?

@kigawas
Copy link
Contributor Author

kigawas commented Sep 4, 2019

I'll remove it in #61

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