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

Replace redpallas stub in zebra-chain with full implementation #2044

Closed
dconnolly opened this issue Apr 20, 2021 · 2 comments · Fixed by #2099
Closed

Replace redpallas stub in zebra-chain with full implementation #2044

dconnolly opened this issue Apr 20, 2021 · 2 comments · Fixed by #2099
Assignees
Labels
A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement NU-5 Network Upgrade: NU5 specific tasks S-needs-design Status: Needs a design decision S-needs-investigation Status: Needs further investigation

Comments

@dconnolly
Copy link
Contributor

Is your feature request related to a problem? Please describe.
To suffice types in the sapling->orchard port in zebra-chain, we created redpallas types but they are functionally incomplete. They need to be functional to verify SpendAuth and Binding signatures for NU5.

Describe the solution you'd like
@str4d has implemented reddsa as a fork of redjubjub to abstract the common types and traits into a dependency for the redpallas types in the orchard crate. The reddsa orchard branch there defines the Orchard basepoints, which is not my favorite.

Should the redjubjub crate turn into something a more shared reddsa, that includes the redjubjub and reddsa? With FROST, it may be annoying to have 3 crates to juggle: reddsa as a dependency, redjubjub, redpallas, with boilerplate across them.

Or should we just duplicate where necessary?

@dconnolly dconnolly added A-rust Area: Updates to Rust code S-needs-design Status: Needs a design decision C-enhancement Category: This is an improvement S-needs-triage Status: A bug report needs triage NU-5 Network Upgrade: NU5 specific tasks S-needs-investigation Status: Needs further investigation P-Medium labels Apr 20, 2021
@dconnolly dconnolly self-assigned this Apr 20, 2021
@dconnolly dconnolly added this to To Do in 🦓 via automation Apr 20, 2021
@dconnolly dconnolly added this to No Estimate in Effort Affinity Grouping via automation Apr 20, 2021
@daira
Copy link
Contributor

daira commented Apr 20, 2021

RedDSA was designed to operate over a generic represented group. To my mind, it would be very unfortunate if we ended up duplicating effort. A small amount of boilerplate might be ok, but I think it's preferable to merge implementations as far as possible.

@str4d
Copy link
Contributor

str4d commented Apr 21, 2021

The reddsa orchard branch there defines the Orchard basepoints, which is not my favorite.

That was the simplest path forward that made the smallest change to the existing redjubjub code (to make it easy for you to review). I'd be happy with a further refactor that enabled reddsa to be completely generic; I just don't know whether that is compatible with the strict typing that redjubjub has. Given that I had to use sealed traits in redpallas anyway though, I think it's possible to remove them from reddsa, but I won't commit to doing that work unless it will actually be used by Zebra.

@mpguerra mpguerra moved this from No Estimate to S - 3 in Effort Affinity Grouping Apr 27, 2021
@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label Apr 27, 2021
@mpguerra mpguerra mentioned this issue Apr 30, 2021
53 tasks
@dconnolly dconnolly linked a pull request May 5, 2021 that will close this issue
2 tasks
🦓 automation moved this from To Do to Done May 6, 2021
Effort Affinity Grouping automation moved this from S - 3 to Done May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement NU-5 Network Upgrade: NU5 specific tasks S-needs-design Status: Needs a design decision S-needs-investigation Status: Needs further investigation
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

4 participants