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

Remove dependency on rust-crypto #137

Merged
merged 1 commit into from
Sep 14, 2021
Merged

Remove dependency on rust-crypto #137

merged 1 commit into from
Sep 14, 2021

Conversation

survived
Copy link
Contributor

@survived survived commented Sep 7, 2021

Removes deprecated dependency on rust-crypto. The only thing is changed in the library is merkle tree support.

  • Changed the lib providing merkle tree support: merkle-sha3 v0.1merkle v1
    (merkle-sha3 is abandoned)
  • Merkle trees support is optional
    merkle crate depends on ring. Dependency on ring might be not desirable, so I made it optional. See feature merkle-tree.
  • Changed the hash function used by merkle proofs: keccak256sha256
    ring has no support of keccak256. This change means that any proofs generated by earlier versions of curv are not compatible with the latest curv.

@elichai
Copy link
Contributor

elichai commented Sep 7, 2021

idk, ring is quite big to take in just for sha256.
Maybe we should fork/implement one of the merkle libraries?
Or maybe use this? https://github.com/nervosnetwork/merkle-tree (it allows us to use whatever hash function we want)

@survived
Copy link
Contributor Author

survived commented Sep 8, 2021

merkle-cbt looks fine to me! I changed the code to use this crate, now we have no dependency on ring! 🎉

But in favour of this dependency switching, I had to change merkle trees API, and proofs generated by previous curv still are not compatible with latest curv. But I think it's fine.

pub fn create_tree(leaves: Vec<Point<E>>) -> Self {
let hashes = leaves
.iter()
.map(|leaf| H::new().chain_point(leaf).finalize())
Copy link
Contributor

Choose a reason for hiding this comment

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

just want to note, that before we hashed only the first 32 bytes of the point and now we hash the full point (which is better)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah you're right. That was strange.

Comment on lines +39 to +38
let index = (0u32..)
.zip(&self.leaves)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think enumerate() is more idiomatic

Copy link
Contributor

Choose a reason for hiding this comment

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

although from codegen standpoint(https://godbolt.org/z/3YKPqE6e6) this is better, and there's probably no way that there are more than 2^32 leaves here (although maybe assert that in create_tree)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

enumerate yields usize that then needs to be converted to u32 via cast expression n as u32, or conversion trait u32::try_from(n).unwrap(). I find zipping more pretty among all these options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think there's no way number of leaves can overflow u32

Copy link
Contributor

@elichai elichai left a comment

Choose a reason for hiding this comment

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

LGTM

@survived survived merged commit 6126e62 into master Sep 14, 2021
@survived survived deleted the remove-rust-crypto branch September 14, 2021 06:33
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