swap ed25519-dalek for ed25519-zebra

[jakehemmerle]

polkadot companion: paritytech/polkadot#3504
This should close #8055. All tests are pass in sp-core.

This should eventually close paritytech/polkadot-sdk#351, but the batching_works test in sp-io does not pass. Since this passes on the current Substrate master, this needs to get fixed.

batching_works stdout and panic log: https://pastebin.com/bA78t00a

@burdges maybe you can take a look at this?

[cla-bot-2021]

User @jakehemmerle, please sign the CLA here.

[burdges]

You still have the failing test case? If I recall, it looked like a case of using the identity as a key, likely zeros do not represent the identity but zebbra pushes them into being the identity or something. Where was it?

[jakehemmerle]

You still have the failing test case? If I recall, it looked like a case of using the identity as a key, likely zeros do not represent the identity but zebbra pushes them into being the identity or something. Where was it?

yes it's still failing. Failing case is batching_works in sp-io.

I didn't realize your identity key matrix message was from reading the code; I thought it was just an example!

[burdges]

There is a multiplication by the cofactor of 8 in https://github.com/ZcashFoundation/ed25519-zebra/blob/main/src/batch.rs#L212 which turns points of small order into the identity, and makes the batch and non-batch verification agree. I believe a bunch of zeros turns into a curve point of small order, which the multiplication by A_coeffs and R_coeffs preserves, and then zeros in the field turn into zero too.

[jakehemmerle]

Tests fixed! Should close paritytech/polkadot-sdk#351 and #8055.

One question: sr25519 (as did ed25519-dalek) throws an error when verifying identity arguments, while ed25519-zebra is fine with verifying identities. I've changed the test to expect this behavior; is this fine or will these differences be an issue in practice?

[burdges]

It depends what you mean by identity. Ristretto has the same identity as Ed25519, but serializes it as [0u8; 32]. I think schnorrkel would verify an identity signature, except [0u8; 64] triggers SignatureError::NotMarkedSchnorrkel.

Ed25519 serializes its identity not as [0u8; 32]. Yet, [0u8; 32] lies on the curve in the subgroup of order 8, so likely 1 in 8 messages should validate under the zero signature and public key.

[burdges]

As a rule, I would not care too much how we handle public keys consisting of zero bytes, except we avoid some code complexity if single and batch verification agree, which is the point here.

[jakehemmerle]

I swapped verifications using zero keys with signatures from another keypair. This should fix the above issues mentioned by Jeff.

[burdges]

I think

substrate/primitives/io/src/lib.rs

Line 1542 in d717bdb

crypto::sr25519_batch_verify(

likely fails in deserialization, which works fine, but maybe duplicate it with a test that fails like

substrate/primitives/io/src/lib.rs

Line 1588 in d717bdb

crypto::ed25519_batch_verify(&signature, msg, &pair2.public());

does.

[burdges]

LGTM modulo adding one test. :)

We'll get someone else from parity to review though.

[jakehemmerle]

Sweet; final test fixed!

[bkchr]

@jakehemmerle can you please merge master

[jakehemmerle]

@bkchr merged

[bkchr]

Sorry for the late reply, but the CI wasn't happy about your merge.

[jakehemmerle]

Looks like the lockfile. Will update shortly

[jakehemmerle]

@bkchr I fixed the lockfile and it's now passing the test-linux-stable-int build, but still failing the test-linux-stable due to a pallet_ui.rs test. I just spend almost two hours trying to understand why it's failing, but I see other test-linux-stable builds passing and I don't have enough context to know where to look to fix the error. I see a new trait_constant_valid_bounds.rs test has been added to pallet_ui by Andrew two weeks ago, but on the surface it looks unrelated. That new test is at https://github.com/paritytech/substrate/blob/master/frame/support/test/tests/pallet_ui.rs#L26 and the only file in the pass folder is https://github.com/paritytech/substrate/blob/master/frame/support/test/tests/pallet_ui/pass/trait_constant_valid_bounds.rs which, again, I'm not sure why my PR would cause this to not pass.

test-linux-stable failing at https://gitlab.parity.io/parity/substrate/-/jobs/987864#L988

Do you think you can take a look and point me in the right direction?

[bkchr]

@jakehemmerle you just need to merge master.

[bkchr]

This should fix your problem

[jakehemmerle]

@bkchr just merged, still having the same issue. I had been rebasing before

[gilescope]

You can't consume the seed in this fn but have raised a PR to make from_seed (above) take ownership of the Seed. ( #9683 )

That then allows SigningKey::from(seed) in the from_seed function.

[jakehemmerle]

This was my first rust ticket and I hadn't wrapped my head around ownership yet :)

I also agree with your point in that PR that the seed should be consumed; will update this if that gets merged

[burdges]

We've seemingly never merged this. Anything problematic crop up? Can we get this running on one of the testnets?

[gilescope]

@bkchr is there someone on your team that might put this to bed? Might be up @wigy-opensource-developer's street? Zebra is harder to make mistakes with than Dalek - am keen if we can have fewer cryptographic footguns.

[bkchr]

Maybe at some point. Going to close this one as it is stale anyway.

[jakehemmerle]

Taking another look at this today with a bit more rust under my belt

TLDR dalek rejects verification when using the zero public key, while zebra allows this.

To procede:

1. Is it worth the overhead to check if its a zero key right before this:

   substrate/primitives/core/src/ed25519.rs

   Line 515 in 0ba251c

   public_key.verify(message.as_ref(), &sig).is_ok()

   
   and rejecting the signature.

2. or should I modify the assertion to accept a the zero key?

Regarding burn in:

• do i just fork polkadot and replace everything that points to the paritytech/substrate repo to my repo/branch?
• I think I had had some lockfile issue during last burnin, whats the best way to update Cargo.lock to point to my repo without updating everything via cargo update ?

[jakehemmerle]

CC'ing @burdges @gilescope @bkchr on for the above, in case you don't get notifications on closed issues unless you're mentioned ^

[bkchr]

You can use diener to replace the branches etc to point to your own forks.

Regarding the zero public key, what does ed25519 standard says to this?

[jakehemmerle]

Cool tool!

I spent about 15 minutes looking for any sort of mention of how to handle zero keys for ECC (I found, literally, nothing), so I'm just going to do a burn in

[jakehemmerle]

Burn in passed. That made my day

I'm going to do #2: modify the assertion to accept the all-zero key

[jakehemmerle]

Done

[burdges]

We do not care about the zero key verifying everything behavior. Zero session or controller key are byzantine, while other zero account keys have no impact.

[jakehemmerle]

Cool, I'll put up a new PR shortly.

[crystalin]

Ideally this PR should contain a label to indicate a change in the host function (for people still pending to do the dependency upgrade)
(cc @bkchr )

[bkchr]

Ideally this PR should contain a label to indicate a change in the host function (for people still pending to do the dependency upgrade) (cc @bkchr )

We have done some sync testing on Polkadot/Kusama to ensure that nothing breaks. However, we should extend this process to Parachains as well.

[crystalin]

I think, no matter if it impacts an existing chain or not, when an assumption of a host function changes, it should be tagged, don't you think ?