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

Upgrade ed25519-zebra to version 2 #1811

Merged
merged 1 commit into from
Mar 2, 2021

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Feb 23, 2021

Motivation

We need to validate signatures after Canopy NU where ZIP-215 was implemented.

Solution

In this 2 tickets: #1565 and #1667 the conclusion is to just upgrade and validate everything using ed25519-zebra version 2.

This PR upgrades ed25519-zebra, all tests are passing and zebra is syncing.

I think some testing will be needed, some ideas could be the followings however not sure if totally required:

  • Sync zebra up to mainnet and testnet tips, where new signatures will be present.

Review

Related Issues

Follow Up Work

@zfnd-bot zfnd-bot bot added this to In progress in 🦓 Feb 23, 2021
@dconnolly dconnolly added A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code NU-4 Canopy Network Upgrade: Canopy specific tasks labels Feb 23, 2021
@dconnolly dconnolly added this to the 2021 Sprint 4 milestone Feb 23, 2021
@dconnolly dconnolly self-requested a review February 23, 2021 20:57
@dconnolly
Copy link
Contributor

Since this is activated at Canopy height, it would be interesting to check pre and post Canopy transactions, and these are for Sprout only, but specifically the signatures in the transactions, not whole blocks.

We should consider a stateful 'sync to Canopy, sync past Canopy' acceptance test, very similar to our Sapling stateful tests, which would be a nice, more finite, full integration test.

@oxarbitrage
Copy link
Contributor Author

Sounds good, ill see what i can do ;)

@oxarbitrage
Copy link
Contributor Author

Maybe we should go with the tests vector created by @hdevalence instead of the big sync for this PR: https://github.com/zcash/zcash/blob/master/src/gtest/test_consensus.cpp#L119

What do you think @dconnolly ?

@oxarbitrage
Copy link
Contributor Author

By discussions in the chat with @dconnolly and @teor2345 :

  • The test vectors from Henry, the ones we care about are already added in https://github.com/ZcashFoundation/ed25519-zebra/blob/0e7a96a267a756e642e102a28a44dd79b9c7df69/tests/small_order.rs#L12
  • We want to upgrade to version 2 and validate everything with that but we want to test this, ideas:
    • Deidre: We just need to have tests in place to catch any regressions at the tx joinsplit sig levels
    • Teor: We should test at least one signature that would fail under the new rules, but passes under the old rules. And one that should fail under both sets of rules.
    • Deidre: So what we need in zebra, are tests that if we validate a joinsplit signature pre-canopy, and post-canopy, like teor said. The test vectors we need may not exist. Or rather, may not be easy to pluck. The compatibility between low level, generic Ed25519 sigs that are ZIP-215 compatible look to be clear, but yeah as teor said, we want to have a transaction with a signature over it, pre and post canopy, bascially to test our integration of ed25519
    • Deidre: we want to have a transaction with a signature over it, pre and post canopy, bascially to test our integration of ed25519

@teor2345
Copy link
Contributor

We should consider a stateful 'sync to Canopy, sync past Canopy' acceptance test, very similar to our Sapling stateful tests, which would be a nice, more finite, full integration test.

Sounds like #1388.

@teor2345
Copy link
Contributor

The Windows CI failure is #1769

tower-batch/Cargo.toml Outdated Show resolved Hide resolved
@oxarbitrage oxarbitrage marked this pull request as draft February 24, 2021 14:10
@oxarbitrage oxarbitrage changed the title upgrade ed25519-zebra to version 2 [WIP] - upgrade ed25519-zebra to version 2 Feb 24, 2021
@oxarbitrage
Copy link
Contributor Author

We decided to do a manual cargo tree to test this PR and a full sync of the mainnet and testnet.

$ cargo tree | grep ed25519
├── ed25519-zebra v2.2.0
├── ed25519-zebra v2.2.0 (*)
$

Full output

@oxarbitrage oxarbitrage changed the title [WIP] - upgrade ed25519-zebra to version 2 Upgrade ed25519-zebra to version 2 Feb 25, 2021
@oxarbitrage oxarbitrage marked this pull request as ready for review February 25, 2021 12:27
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.

The changes here are good, we just need to run the full sync tests, then we can merge. I'll update the checklist in the ticket.

@mpguerra mpguerra removed this from the 2021 Sprint 4 milestone Feb 26, 2021
@oxarbitrage
Copy link
Contributor Author

I synchronized a mainnet today on top of this PR.

It started at 2.08 pm local time and ended 5:04 as can be seen in the network traffic chart:

mainnet

This is 2 hours 56 minutes. I am posting this because there were report of about 2 hours and a half. Unsure if this PR can be the difference or just network/hardware conditions.

I will try the testnet and post details.

@teor2345
Copy link
Contributor

teor2345 commented Mar 1, 2021

This is 2 hours 56 minutes. I am posting this because there were report of about 2 hours and a half. Unsure if this PR can be the difference or just network/hardware conditions.

I'm not concerned about this variance, it takes me about 4-6 hours locally, probably due to network speed/latency:
#1803 (comment)

If we got this change wrong, then we would be rejecting blocks and failing to reach the tip.

@oxarbitrage
Copy link
Contributor Author

The testnet is getting close to block 700_000 after 3 days. Very slow but it keeps moving. I can report again when it gets into the tip, if everything goes fine i think it will take at least 1 day more.

@teor2345
Copy link
Contributor

teor2345 commented Mar 1, 2021

The testnet is getting close to block 700_000 after 3 days. Very slow but it keeps moving. I can report again when it gets into the tip, if everything goes fine i think it will take at least 1 day more.

Hmm 3 days is a really bad sign, I usually see testnet sync within a few hours. But I also have local testnet zcashd and Zebra nodes.

So the slowness it could mean a bunch of different things:

  • testnet is all unstable
  • Zebra has unrelated bugs
  • this change makes Zebra really slow on testnet (which seems really unlikely, because it's mainly a cryptography change)

I'm going to put this on my list of things to double-check on testnet.

@teor2345
Copy link
Contributor

teor2345 commented Mar 2, 2021

I've just started an ephemeral mainnet and testnet sync for this PR, with some local peers for speed.

@teor2345
Copy link
Contributor

teor2345 commented Mar 2, 2021

Commit ccc5ae3 (merge 40d4b97) synced within 3 hours on testnet, and 5 hours on mainnet. That's pretty typical for me, I'm network-limited, and my local peers are a much larger proportion of testnet. (Also testnet data is much smaller.)

There were no verification issues, so I think we're good to merge this.

So it looks like your slow testnet sync was a local network issue, or a testnet stability issue. (Which we'll fix with #1222 pretty soon.)

You might also find it helps to keep a synced local Zebra or zcashd instance running, and configure your test instance talk to it. (If you have enough RAM and disk for that.)

@teor2345 teor2345 merged commit ca44fbd into ZcashFoundation:main Mar 2, 2021
🦓 automation moved this from In progress to Done Mar 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code NU-4 Canopy Network Upgrade: Canopy specific tasks
Projects
No open projects
🦓
  
Done
Development

Successfully merging this pull request may close these issues.

Move to the post-Canopy ed25519 signature rules Enable ZIP-215: Ed25519 Validation Rules
4 participants