mimblewimble / secp256k1-zkp

Fork of secp256k1-zkp for the Grin/MimbleWimble project
MIT License
32 stars 42 forks source link

Code Audit Diffs #35

Open yeastplume opened 5 years ago

yeastplume commented 5 years ago

Upstream is the Elements project code with Bulletproof PR 23 applied: https://github.com/ElementsProject/secp256k1-zkp/pull/23

Downstream is this repository, as of commit 4d64b7b

The major differences are:

Diffs of the include and src directories are here:

diffs_include.txt diffs_src.txt

Giszmo commented 5 years ago

Somehow GitHub seems to not know these repositories are related as I can't compare across them? Anyway, a static diff file is probably not something anybody would want to review neither.

This project and Elements using rebase and squashing in feature branches doesn't make things easier neither. Therefore, the most reasonable way to do a full review would be to consider https://github.com/ElementsProject/secp256k1-zkp/commit/5d5374f92c28fc53f86d8e7591201dabc1bd6dea (a commit that https://github.com/ElementsProject/secp256k1-zkp/pull/23 was probably building on back in November 2018) as upstream revision and https://github.com/ElementsProject/secp256k1-zkp/pull/23 only as a reference?

The latest commit I see in all 3 referenced repos

is 452d8e4d2a2f9f1b5be6b02e18f1ba102e5ca0b4. I ran

$ git remote add apoelstra https://github.com/apoelstra/secp256k1-mw
$ git remote add elements https://github.com/ElementsProject/secp256k1-zkp
...
$ git merge-base master apoelstra/bulletproofs-rangeonly elements/secp256k1-zkp
452d8e4d2a2f9f1b5be6b02e18f1ba102e5ca0b4
$ git diff 452d8e4d2a2f9f1b5be6b02e18f1ba102e5ca0b4 master --shortstat 
 68 files changed, 12238 insertions(+), 8 deletions(-)

... doesn't look like fun to review. The above mentioned commit:

$ git diff 5d5374f92c28fc53f86d8e7591201dabc1bd6dea master --shortstat 
 74 files changed, 6527 insertions(+), 2288 deletions(-)

doesn't look much better and

$ git diff apoelstra/bulletproofs-rangeonly master --shortstat 
 41 files changed, 2259 insertions(+), 2631 deletions(-)

is only slightly better.

I assume I'm missing something. How would I go about with this review? Reviewing is my every day job but Java, so I'm not sure how much actual review comments I can contribute but for now I lack a good starting point assuming apoelstra/bulletproofs-rangeonly and elements/secp256k1-zkp are well reviewed starting points.