search

Warchant / sr25519-crust

C bindings over RUST sr25519 https://github.com/w3f/schnorrkel
Apache License 2.0
31 stars 22 forks source link

Updated to latest version of schnorrkel #22

Closed gautamdhameja closed 5 years ago

gautamdhameja commented 5 years ago

I am getting an issue with the generated binary after these changes. If I compile in debug mode then the sr25519_verify fn returns true or false as expected, but when I compile in the release mode then it returns true even with negative tests. Any ideas? @Warchant

Warchant commented 5 years ago

Will check.

UPD: did you build C tests? You can do this with cmake flag -DTESTING=ON (cmake .. -DTESTING=ON). It is off by default to not force users download and build gtest/gmock.

It seems schnorrkel has some breaking changes, and our tests fail now.

List of failed tests:

UPD2: cargo test passes for both debug and release modes.

UPD3: internals of sr25519 implementation also changed. We will update soon.

gautamdhameja commented 5 years ago

Yeah, it seems there are some more changes needed as the internals have changed a bit. Thanks for taking a look and for sharing pointers, @Warchant. I am using the dll generated by this library to create dotnet bindings for sr25519. For now, I'll use the current master (0.1.1).

Warchant commented 5 years ago

@gautamdhameja yeah, it is safe to use latest release version. We follow semver, so when we fix everything and update version, new major version will be released.

burdges commented 5 years ago

We changed the scalar serialization for secret keys from ed25519 to canonical styles, which might break any tests anywhere. If your code is not deployed, then fix any secret keys by shifting the scalar left 3 bits, ala scalars::divide_scalar_bytes_by_cofactor. We also have some from*_ed25519_bytes mothods for if code is deployed. Any test vector called KeypairFromSeed should be broken by this, even if you use ExpansionMode::Ed25519.

I'd expect DeriveHardKnown should work if using the ExpansionMode::Ed25519. I kinda hate this mode existing, but we've too many people with keys derived this way for kusama, maybe we'll restrict to ExpansionMode::Uniform for polkadot, or maybe we'll give up and keep it this way.

There are no changes that break the actual soft derivation behind DeriveSoftKnown in 0.8.4 but the previous yanked 0.8.* broke this by mistake, so ignore any failures before 0.8.4.

I'd expect VerifyExisting would work if you employ the preaudit_deprecated feature, but this should disapear once all testnets update, so again just replace the test vector if the code is not deplotyed.

I'm unsure what ResultNotLess does.

cc @jacogr