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

Code Audit Diffs #35

Open
yeastplume opened this issue Nov 29, 2018 · 1 comment
Open

Code Audit Diffs #35

yeastplume opened this issue Nov 29, 2018 · 1 comment

Comments

@yeastplume
Copy link
Member

yeastplume commented Nov 29, 2018

Upstream is the Elements project code with Bulletproof PR 23 applied:
BlockstreamResearch/secp256k1-zkp#23

Downstream is this repository, as of commit 4d64b7b

The major differences are:

  • We've made additions to the bulletproof code, hiding a four byte BIP32 identifier into the nonce 'alpha'. This was just an extension to work Andrew had already done to allow Proofs to be rewound
  • Changes to bulletproofs to allow for multi party proofs. The relevant PR with those changes is:
    Multi-party bulletproof #24
  • A completely custom aggsig module that doesn't exist upstream, at https://github.com/mimblewimble/secp256k1-zkp/tree/master/src/modules/aggsig
    This was originally based on Andrew's code, however that code was never particularly reviewed and it's been re-written over time by us to suit Grin's requirements.

Diffs of the include and src directories are here:

diffs_include.txt
diffs_src.txt

@Giszmo
Copy link

Giszmo commented Jun 3, 2019

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 BlockstreamResearch/secp256k1-zkp@5d5374f (a commit that BlockstreamResearch/secp256k1-zkp#23 was probably building on back in November 2018) as upstream revision and BlockstreamResearch/secp256k1-zkp#23 only as a reference?

The latest commit I see in all 3 referenced repos

is 452d8e4. 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.

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

No branches or pull requests

2 participants