maidsafe / sn_dbc

Safe Network DBCs
BSD 3-Clause "New" or "Revised" License
15 stars 16 forks source link

Decouple deps #158

Closed dan-da closed 2 years ago

dan-da commented 2 years ago

note: this PR includes #154 and #157 which have not yet been merged. If/when those are merged, this can be rebased to make a smaller diff for review. edit: merged. this is ready for review.


feat: decouple deps and remove bls_dkg

-- Change 1: re-export blsttc, blst_ringct --

The idea is to give API consumers the option to use the exported crates
without needing explicit Cargo.toml entries.

This in turn decouples dependencies and reduces conflicts.

See rationale/discussion here:
https://github.com/rust-lang/api-guidelines/discussions/176

-- Change 2: use deps from within blsttc, blst_ringct --

blsttc and blst_ringct now reexport deps used in their public APIs.

So we remove those deps from our Cargo.toml and 'use' the sub-deps
in our code.

One gross thing remains which is that both crates use rand and
they could differ.  (although at present they are the same.)

This means that sn_dbc APIs that take Rng arg to be passed to
blsttc API can potentially be different than those that take
an Rng arg to be passed to blst_ringct API.  Although the compiler
presently let's us pass the same rng to both.  It's a footgun!

For now, I've created an 'rng' module to make it easy to pass the
appropriate rng, and also I updated the test cases, example, bench
to use this methodology, to demonstrate best practice.

Perhaps/probably a better long-term approach would be to have
blst_ringct integrate/depend/use blsttc, so that sn_dbc has only
a single dep and is not trying to resolve the matter.

-- Change 3: remove bls_dkg dependency/feature --

bls_dkg was only being used for a single function used by
test cases.  It was simple to replace this function with
blsttc calls which simplifies things a lot.

Cargo changes:

* use publshed quickcheck 1.0.3
* use dan-da/blsttc/reexport_pr
* remove rand, rand_core deps
* remove xor_name dep
* remove bls_dkg dep
* remove dkg feature
* remove blstrs dep
* use single-line format for all deps. (more concise)

lib.rs changes:
* re-export blst_ringct and blsttc
* export additional types used by builder module public API.
* add rng module, to simplfy rng usage for callers.
* modify bls_dkg_id() to use blsttc directly, without bls_dkg

Code changes:

* update mint-repl and reissue bench to use updated sn_dbc API.
* update tests to separate ringct rng from blsttc rng.
* update SimpleSigner From impl now that bls_dks is removed.

edit: Change 4: export and use a single rand

Our public API uses/requires rand quite prominently, with several methods requiring caller to pass in an Rng. (The same dep issues occur for blstrs as well, but it is only indirectly exposed in sn_dbc public API).

Both blst_ringct and blsttc export rand, but they do not depend on eachother, so it is possible that they could be (in the future perhaps) using incompatible versions. So at first glance, the cleanest seeming thing is for fn that utilize blsttc::rand to accept param of that type and fn that accept blst_ringct::rand to accept param of that type.

That is what I first impl'd but it makes using our API ugly, as sometimes callers must create both rng types in the same fn. Worse, the compiler does not even enforce the types or issue any warning because it sees that they are presently using the exact same rand crate. So it is a bit of a footgun waiting to happen for API users. I modified all the tests, examples, bench to differentiate between the two as "best practice" and found the result dis-pleasing. We had simply gone from instantiating rng7+rng8 to rng_ringct+rng_ttc.

I made an (aborted) attempt to unify the two rand's (and blstrs) by adding a blsttc dep inside blst-bulletproofs. So then bulletproofs uses the rand, blstrs (and I think group) from blsttc, even though it doesn't use blsttc itself. and blst_ringct uses them from bulletproofs. However (a) it was gross to depend on blsttc without actually using its API, and (b) it didn't truly solve the problem because from sn_dbc's perspective, there is still blst_ringct::rand and blst_ringct::bulletproofs::blsttc::rand, which could theoretically differ in the future. So I reverted that bulletproofs commit. (but, it remains an option.)

So finally I settled on the "solution" in https://github.com/maidsafe/sn_dbc/commit/1986cd3ba127d713d42164cef083b1423ca5efac that sn_dbc arbitrarily picks blst_ringct::rand as the sn_dbc rand and re-exports it. All library code references crate::rand, including methods that pass Rng to blsttc APIs. In doing this, sn_dbc is implicitly guaranteeing to our API users that rand (and blstrs) will remain compatible between blstrs and blsttc. I think we can make that promise because maidsafe is planning to maintain both. And foremost, callers were pretty much guaranteed to use it this way anyway, because the compiler allows it and it is non-obvious to use the stricter-but-correct approach.

If there is some way to make the compiler be more strict, then perhaps its better to revert https://github.com/maidsafe/sn_dbc/commit/1986cd3ba127d713d42164cef083b1423ca5efac and go with the strict approach. Otherwise, I think this is best....

davidrusu commented 2 years ago

Will review once ~#154~ #157 goes in

dan-da commented 2 years ago

be sure to read 'Change 4' comment I just added above.