Update schnorrkel to 0.9, remove deprecated api usage #5138

NikVolf · 2020-03-05T10:47:04Z

No description provided.

NikVolf · 2020-03-05T10:47:20Z

primitives/core/src/sr25519.rs

@@ -776,6 +780,7 @@ mod test {
 	}

 	#[test]
+	#[ignore = "sr25519 signatures from 1.1 version are no longer supported"]


what can be used instead?

Remove? The test is useless now^^

andresilva

changes lgtm, not sure about backwards compatibility issues.

andresilva · 2020-03-05T11:01:53Z

primitives/core/src/sr25519.rs

-			).is_ok(),
-			Err(_) => false,
-		}
+		let signature = match schnorrkel::Signature::from_bytes(sig) {


Maybe we should try to sync kusama with this, not sure if there's txs signed with schnorrkel 0.1.1 that we won't be able to verify anymore. cc @burdges

Or BABE blocks for that matter.

Maybe we should try to sync kusama with this, not sure if there's txs signed with schnorrkel 0.1.1 that we won't be able to verify anymore. cc @burdges

Yep, trying to do it right now

@burdges said something that 1.1 were only used on Alexander

We never supported VRFs from 0.1.1 because BABE did not work until after the audit. ;)

Just send schnorrkel a PR reverting w3f/schnorrkel@8f37065 if you really need it, but if these exist then we maybe we could keep that pain isolated to kusama, and disable the preaudit_deprecated feature for polkadot.

There will be old txs (old blocks) signed with 0.1.1 - the JS side only moved to 0.8.5 in Jan once Alex was dropped.

bkchr

Did you synced Kusama with it?

bkchr · 2020-03-05T12:44:38Z

primitives/core/src/sr25519.rs

@@ -776,6 +780,7 @@ mod test {
 	}

 	#[test]
+	#[ignore = "sr25519 signatures from 1.1 version are no longer supported"]


Remove? The test is useless now^^

bkchr · 2020-03-05T12:44:43Z

primitives/core/src/sr25519.rs

@@ -609,6 +612,7 @@ mod compatibility_test {
 	}

 	#[test]
+	#[ignore = "sr25519 signatures from version 1.1 are no longer supported"]


NikVolf · 2020-03-05T14:22:38Z

Did you synced Kusama with it?

got error in block 3876

Also can confirm that concerns in #5112 are valid.
Node just keeps spamming about runtime panic when invalid block is encountered.

burdges · 2020-03-05T17:07:39Z

Any thoughts?

I've temporarily pushed f4447cd7eb0c793905aa85af4d32c71a78ea020a that reverts 8f3706591dcf0648086b5dfb4bd832970483ac03 and restores the preaudit_deprecated feature, but..

We should avoid support for preaudit_deprecated in polkadot because allowing it screws up batch verification for ed25519 without adding a key type field.

We actually have some key type field I suppose, so maybe we could leverage that for kusama somehow.

burdges · 2020-03-05T18:55:29Z

I've an idea: Could kusama verify these old signature entirely from the runtime? We'd drop support for them from in the host in this case. Is that too annoying?

bkchr · 2020-03-05T20:36:22Z

I've an idea: Could kusama verify these old signature entirely from the runtime? We'd drop support for them from in the host in this case. Is that too annoying?

No, the problem is that we can not modify the old runtimes anymore. However, we could probably workaround and only accept from now on the new signatures when building a block but still support verifying the old ones.

burdges · 2020-03-16T12:25:34Z

I'll need to publish schnorrkel 0.9.1 with w3f/schnorrkel@f4447cd included I suppose?

~~Would it help if we assigned some new identifier byte for signature types?~~

NikVolf · 2020-03-16T12:30:34Z

I'll need to publish schnorrkel 0.9.1 with w3f/schnorrkel@f4447cd included I suppose?

Yeah, if it is an improvement on 0.8.*, we can update to it

burdges · 2020-03-17T00:18:43Z

I've published 0.9.1 that brings back the preaudit_deprecated feature.

We cannot do a logical or in a batch verification, so #5023 requires the old and new signatures be distinguished. We need not distinguish them on-chain however. Instead, we expose both old and new verification mechanisms to the runtime, so the runtime could switch at some block height or whatever.

Demi-Marie · 2020-03-17T01:01:29Z

I've published 0.9.1 that brings back the preaudit_deprecated feature.

We cannot do a logical or in a batch verification, so #5023 requires the old and new signatures be distinguished. We need not distinguish them on-chain however. Instead, we expose both old and new verification mechanisms to the runtime, so the runtime could switch at some block height or whatever.

Will we be able to get rid of the cruft at some point, once a certain block hash becomes hard-coded in the client?

burdges · 2020-03-17T02:17:23Z

Will we be able to get rid of the cruft at some point, once a certain block hash becomes hard-coded in the client?

Yes and no. We'll ignore this entirely on Polkadot, so we can drop it from substrate builds that only support Polkadot. We cannot drop it from substrate builds that support Kusama, at least not until some consensus upgrade forces some "harder" fork. We should've gone through this pain for alexandre vs kusama, but we took the more relaxed approach there so we pay for it now.

NikVolf · 2020-03-17T05:55:42Z

I've published 0.9.1 that brings back the preaudit_deprecated feature.

We cannot do a logical or in a batch verification, so #5023 requires the old and new signatures be distinguished. We need not distinguish them on-chain however. Instead, we expose both old and new verification mechanisms to the runtime, so the runtime could switch at some block height or whatever.

Can this signatures be distinguished at runtime, at runtime?
We can just skip batch for them then, and forbid when block building.

burdges · 2020-03-17T12:39:03Z

Can this signatures be distinguished at runtime, at runtime?
We can just skip batch for them then, and forbid when block building.

Yes I think this works, so no fancy checks.

Signatures by 0.1.1 should fail Signature::from_bytes. I published 0.1.1 on 2 April 2019, but only added the distinguishing marker checked by Signature::from_bytes on 19 April 2019: https://github.com/w3f/schnorrkel/commits/master?after=2d004cffa5dc3bf6dafeebee8d4d40b9f696462f+34&path%5B%5D=src&path%5B%5D=sign.rs

I published 0.2-0.7 in June-July, but yanked them all before Kusama's launch. We kept support for 0.1.1 for the alexandre testnet and only screwed up by not supporting 0.1.1 in Kusama.

I'd still prefer if polkadot avoided any support 0.1.1. ;)

bkchr · 2020-03-19T16:25:47Z

Was done by: #5316

NikVolf · 2020-03-19T19:56:37Z

It was not done by #5316, doing it properly (with disabling generation of deprecated signatures) requires runtime api versioning I'm working on now

bkchr · 2020-03-19T20:13:11Z

I know, but I see this as a new pr with a different scope.

burdges · 2020-03-19T20:25:39Z

I'll amend my comment above: We'll separate proper schnorrkel signatures from these deprecated signatures for batch verification using schnorrkel::Signature::from_bytes, but we've no similar mechanism for separating the 0.1.1 signatures from ed25519 signatures for ed25519 batch verification. We could however just leave ed25519 batch verification off for kusma, while leaving schnorrkel 0.1.1 verification off for polkadot. If this switch gets controlled in the runtime, then kusama could flip ed25519 batch verification on at some point too.

bkchr · 2020-03-19T20:53:49Z

Batch verification will require runtime changes and when this comes we will have support for disabling the old signature verification.

bkchr · 2020-03-19T20:59:03Z

Tldr, we should get this working.

kirushik · 2020-04-14T14:42:32Z

@bkchr is there a tracking issue for this task somewhere? Or any other way of monitoring the progress (and, more importantly, of ensuring that we don't leave preaudit_deprecated feature of schnorrkel on for any Polkadot release builds)?

bkchr · 2020-04-14T14:52:23Z

The proper solution was implemented here: #5358

The problem is that we will never be able to disable preaudit_deprecated, because Kusama requires it when syncing from genesis. And the way we currently build Kusama, prevents us from disabling the feature for Polkadot, because it would automatically be enabled by Kusama (everything is build in the same env). However, newer runtimes don't expose the old function, this means we don't accept anymore signatures that are build with the 1.0 schnorrkel.

Demi-Marie · 2020-04-14T15:21:49Z

@bkchr we can (and probably should) switch to separate environments instead. We can also hardcode the hash of every block made on Kusama with the old signature. We then don’t need to validate the signatures any more.

bkchr · 2020-04-14T15:44:16Z

Why should we do this? We can not call this implementation anymore from newer runtimes. (it would work, but you would need to have control over the build process and get this runtime as the canonical one)

Demi-Marie · 2020-04-15T22:16:39Z

@bkchr to remove cruft, reduce code size, and to ensure that this does not wind up in Polkadot.

NikVolf requested review from andresilva and Demi-Marie as code owners March 5, 2020 10:47

NikVolf commented Mar 5, 2020

View reviewed changes

andresilva reviewed Mar 5, 2020

View reviewed changes

bkchr reviewed Mar 5, 2020

View reviewed changes

NikVolf added the A3-in_progress Pull request is in progress. No review needed at this stage. label Mar 5, 2020

update schnorrkel

3819350

NikVolf force-pushed the nv-update-schnorrkel branch from b062614 to 3819350 Compare March 5, 2020 13:56

bkchr closed this Mar 19, 2020

bkchr deleted the nv-update-schnorrkel branch March 19, 2020 16:25

NikVolf restored the nv-update-schnorrkel branch March 20, 2020 15:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update schnorrkel to 0.9, remove deprecated api usage #5138

Update schnorrkel to 0.9, remove deprecated api usage #5138

NikVolf commented Mar 5, 2020

NikVolf Mar 5, 2020

bkchr Mar 5, 2020

andresilva left a comment

andresilva Mar 5, 2020

andresilva Mar 5, 2020

NikVolf Mar 5, 2020

burdges Mar 5, 2020

jacogr Mar 5, 2020

bkchr left a comment

bkchr Mar 5, 2020

bkchr Mar 5, 2020

NikVolf commented Mar 5, 2020

burdges commented Mar 5, 2020

burdges commented Mar 5, 2020 •

edited

Loading

bkchr commented Mar 5, 2020

burdges commented Mar 16, 2020 •

edited

Loading

NikVolf commented Mar 16, 2020

burdges commented Mar 17, 2020

Demi-Marie commented Mar 17, 2020

burdges commented Mar 17, 2020

NikVolf commented Mar 17, 2020

burdges commented Mar 17, 2020 •

edited

Loading

bkchr commented Mar 19, 2020

NikVolf commented Mar 19, 2020

bkchr commented Mar 19, 2020

burdges commented Mar 19, 2020

bkchr commented Mar 19, 2020

bkchr commented Mar 19, 2020

kirushik commented Apr 14, 2020

bkchr commented Apr 14, 2020

Demi-Marie commented Apr 14, 2020

bkchr commented Apr 14, 2020

Demi-Marie commented Apr 15, 2020

Update schnorrkel to 0.9, remove deprecated api usage #5138

Update schnorrkel to 0.9, remove deprecated api usage #5138

Conversation

NikVolf commented Mar 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresilva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkchr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikVolf commented Mar 5, 2020

burdges commented Mar 5, 2020

burdges commented Mar 5, 2020 • edited Loading

bkchr commented Mar 5, 2020

burdges commented Mar 16, 2020 • edited Loading

NikVolf commented Mar 16, 2020

burdges commented Mar 17, 2020

Demi-Marie commented Mar 17, 2020

burdges commented Mar 17, 2020

NikVolf commented Mar 17, 2020

burdges commented Mar 17, 2020 • edited Loading

bkchr commented Mar 19, 2020

NikVolf commented Mar 19, 2020

bkchr commented Mar 19, 2020

burdges commented Mar 19, 2020

bkchr commented Mar 19, 2020

bkchr commented Mar 19, 2020

kirushik commented Apr 14, 2020

bkchr commented Apr 14, 2020

Demi-Marie commented Apr 14, 2020

bkchr commented Apr 14, 2020

Demi-Marie commented Apr 15, 2020

burdges commented Mar 5, 2020 •

edited

Loading

burdges commented Mar 16, 2020 •

edited

Loading

burdges commented Mar 17, 2020 •

edited

Loading