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.

Sign up for GitHub

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

Jump to bottom

Flesh out redpallas, direct port of redjubjub #2099

Merged
merged 5 commits into from
May 6, 2021
Merged

Flesh out redpallas, direct port of redjubjub #2099

merged 5 commits into from
May 6, 2021

Conversation

dconnolly
Copy link
Contributor

@dconnolly dconnolly commented May 3, 2021 •
edited
Loading

Motivation

Finishing Transaction::V5 requires a redpallas::Signature type, which was not part of the initial stub-out of the redpallas types in the sapling->orchard port in #1885.

Solution

Finish the direct redjubjub -> redpallas port for single signature verification, modulo the batching and the multiscalar multiplication required for that, as the math needs updating or replacing completely.

The code in this pull request has:

  • Documentation Comments
  • Unit Tests and Property Tests

Review

This unblocks @oxarbitrage

Related Issues

This technically fulfills #2044, but I don't know if we want it to be the long-term solution. @mpguerra

Follow Up Work

The batch math: #2098

@dconnolly
Flesh out redpallas, direct port of redjubjub
602097a 
Minus batching and the multiscalar multiplication required for that, as the math needs
updating or replacing completely.
@dconnolly dconnolly added A-rust Area: Updates to Rust code NU-5 Network Upgrade: NU5 specific tasks labels May 3, 2021
@dconnolly dconnolly requested review from teor2345 and oxarbitrage May 3, 2021 15:07
@dconnolly dconnolly self-assigned this May 3, 2021
@dconnolly dconnolly added this to the 2021 Sprint 8 milestone May 3, 2021
@dconnolly dconnolly requested a review from conradoplg May 3, 2021 15:09
@teor2345
Copy link
Contributor

teor2345 commented May 3, 2021

This technically fulfills #2044, but I don't know if we want it to be the long-term solution. @mpguerra

I think it's fine to close that ticket for now, and open an optional cleanup ticket for later.

teor2345
teor2345 suggested changes May 3, 2021
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Looks great.

I had a few suggestions, but they're all minor.

zebra-chain/src/primitives/redpallas/signing_key.rs Show resolved Hide resolved
zebra-chain/src/primitives/redpallas/constants.rs Outdated Show resolved Hide resolved
zebra-chain/src/primitives/redpallas/tests/prop.rs Show resolved Hide resolved
zebra-chain/src/primitives/redpallas/tests/prop.rs Outdated Show resolved Hide resolved
zebra-chain/src/primitives/redpallas/verification_key.rs Show resolved Hide resolved
conradoplg
conradoplg requested changes May 4, 2021
View reviewed changes
Copy link
Collaborator

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

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

On a first pass I just found a small issue in a comment.

Should I make a more in-depth review of the crypto part? (It should take a couple days to get familiar with RedDSA)

zebra-chain/src/primitives/redpallas/signature.rs Outdated Show resolved Hide resolved
@dconnolly dconnolly linked an issue May 5, 2021 that may be closed by this pull request
Replace redpallas stub in zebra-chain with full implementation #2044
Closed
dconnolly and others added 2 commits May 5, 2021 08:11
@dconnolly @conradoplg
s/RedJubjub/RedPallas/
f718c81 
Co-authored-by: Conrado Gouvea <conradoplg@gmail.com>
@dconnolly
Seed redpallas proptest rng securely
7b3ad86
teor2345
teor2345 previously approved these changes May 5, 2021
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks good, happy to merge if you're happy with it

@dconnolly dconnolly requested review from conradoplg and teor2345 May 6, 2021 13:54
@dconnolly dconnolly enabled auto-merge (squash) May 6, 2021 14:11
@dconnolly dconnolly disabled auto-merge May 6, 2021 14:53
@dconnolly dconnolly merged commit 3901dc9 into main May 6, 2021
@dconnolly dconnolly deleted the redpallas-sig-stub branch May 6, 2021 14:56
@mpguerra mpguerra mentioned this pull request May 13, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oxarbitrage oxarbitrage Awaiting requested review from oxarbitrage

@conradoplg conradoplg Awaiting requested review from conradoplg

@teor2345 teor2345 Awaiting requested review from teor2345

Assignees

@dconnolly dconnolly

Labels
A-rust Area: Updates to Rust code NU-5 Network Upgrade: NU5 specific tasks
Projects
None yet
Milestone
2021 Sprint 8
Development

Successfully merging this pull request may close these issues.

Replace redpallas stub in zebra-chain with full implementation
3 participants
@dconnolly @teor2345 @conradoplg