Fix MultiSigner, simplify tests by tomusdrw · Pull Request #2033 · paritytech/substrate

tomusdrw · 2019-03-18T16:17:28Z

Related #2022

Uses AnySignature for node/runtime and MultiSignature for test-runtime.

Superceded by #2028

jacogr

It solves the ed25519 signature issue, was able to submit both sr25519 and ed25519 signed transactions. (The CantPay which can be fixed with a restart issue is still there, but not meant to be fixed here)

jacogr · 2019-03-18T17:51:58Z

Also included in #2028

tomusdrw · 2019-03-25T13:56:03Z

Removing onice since I think it was only blocked by #2048 / #2028

gavofyork · 2019-03-26T13:50:29Z

node/cli/src/chain_spec.rs

 	sr25519::Pair::from_string(&format!("//{}", seed), None)
 		.expect("static values are valid; qed")
 		.public()
+		.unchecked_into()


unchecked_into is semantically unsafe, so shouldn't be in any production code. though it's kind of safe here, i'd prefer to use a real .into() since we are after all converting between two strong types.

gavofyork · 2019-03-26T13:51:08Z

node/executor/src/lib.rs

-				let key = AccountKeyring::from_public(&signed).unwrap();
-				let signature = payload.using_encoded(|b| {
+				let key = AccountKeyring::from_h256_public(signed.clone().into()).unwrap();
+				let signature = UncheckedFrom::unchecked_from(payload.using_encoded(|b| {


would prefer to keep strong types here, too.

gavofyork · 2019-03-26T13:51:46Z

node/executor/src/lib.rs

 				let payload = (index.into(), xt.function, era, GENESIS_HASH);
-				let key = AccountKeyring::from_public(&signed).unwrap();
-				let signature = payload.using_encoded(|b| {
+				let key = AccountKeyring::from_h256_public(signed.clone().into()).unwrap();


converting via H256 kills semantics. this stuff should retain strong typing as before.

gavofyork

A few instances where type-unsafety is being created.

tomusdrw · 2019-04-04T08:48:31Z

I've removed AnySigner and used MultiSigner instead, that should give us strong typing.

gavofyork · 2019-04-04T15:03:30Z

There's not much point using MultiSigner for AnySignature; the point of AnySigner was to keep compatibility with both straight Ed25519 and Sr25519 signers. The current code keeps that compatibility and doesn't have the additional type-punning.

tomusdrw · 2019-04-04T15:24:12Z

@gavofyork so current code is cheating a bit cause it uses AnySignature, but the Signer type is sr25519::Public (even though the signature might be ed25519) - I guess it works since they are the same width and have the same basic encoding.

If we use AnySigner(H256) I don't think we can escape unsafe/unchecked conversions.
We could introduce an encoding-compatible type enum AnySigner { Sd2519(..), Ed25519(..) } but Decode for such type would be defaulting to only one as well same as how UncheckedFrom<H256> is now.

I can implement the second option, cause I till believe this PR is worth merging, since it simplifies a bunch of code where we create transactions and uses the MulitSignature in the test-runtime, which required adding a bunch of trait implementations, cause otherwise the type is not usable in any runtime at all.

gavofyork · 2019-04-16T08:01:52Z

The extra 84 lines and semantic clutter (additional .into()s) it introduces doesn't seem to buy much. MultiSigner shouldn't be used with AnySignature, the point of it was only to type-pun between sr25519 and ed25519 and therefore it's fine to assume they're the same size.

I would be ok with a PR that purely made MultiSignature work correctly. I'm not especially bothered about having it as the default in the staging runtime though.

Demi-Marie · 2019-04-19T13:29:31Z

@gavofyork is there any chance that this could be merged soon? It might be a substantial help for BABE, which uses sr25519 everywhere.

tomusdrw · 2019-04-19T14:33:10Z

I'll take another shot on this (from scratch)

new take

tomusdrw · 2019-04-19T15:18:18Z

Ok, current approach fixes MultiSigner, adds a convenience From<ed255519::Signature> for AnySignature and doesn't touch Signer of AnySignature at all.
Additionally creation of signed extrinsics in tests has been simplfiied to use into_signed_tx()

tomusdrw · 2019-04-19T15:19:46Z

I'd really like to have a way to write some test and make sure that MultiSignature/MultiSigner is not broken in the future other than using MultiSignature in test-runtime which was rejected. So let me know if you have any ideas other than creating entire runtime as a test.

tomusdrw · 2019-04-19T15:20:48Z

@demimarie-parity How exactly did you expect this PR to help you with Sr signatures? node/runtime and node-template should already support both types with no issue.

burdges · 2019-06-04T23:21:01Z

Just fyi, schnorrkel 0.2's Signature::from_bytes now gives an error if you pass it an Ed25519 signature: https://github.com/w3f/schnorrkel/blob/master/src/sign.rs#L113

This should make batch verification of AnySignature's possible.

* Fix MultiSigner, use `into_signed_tx` * Rebuild.

tomusdrw requested review from gavofyork and jacogr March 18, 2019 16:17

devops-parity added the A4-gotissues label Mar 18, 2019

jacogr previously approved these changes Mar 18, 2019

View reviewed changes

devops-parity added the B2-breaksapi label Mar 18, 2019

gavofyork mentioned this pull request Mar 18, 2019

Revamp minting/burning handling #2028

Closed

gavofyork added A1-onice and removed A1-onice labels Mar 18, 2019

tomusdrw force-pushed the td-fix-accounts branch from adc92d5 to 7b690c8 Compare March 19, 2019 09:00

tomusdrw added A0-please_review Pull request needs code review. and removed A1-onice labels Mar 25, 2019

gavofyork reviewed Mar 26, 2019

View reviewed changes

gavofyork previously requested changes Mar 26, 2019

View reviewed changes

gavofyork added A4-gotissues and removed A0-please_review Pull request needs code review. A4-gotissues labels Mar 26, 2019

tomusdrw added A0-please_review Pull request needs code review. and removed A5-grumble labels Apr 4, 2019

bkchr mentioned this pull request Apr 19, 2019

Fix AnySignature for support sr25519 or ed25519 #2293

Closed

tomusdrw added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Apr 19, 2019

tomusdrw added 2 commits April 19, 2019 17:04

Fix MultiSigner, use into_signed_tx

37cc349

Rebuild.

bdd4636

tomusdrw force-pushed the td-fix-accounts branch from a728a7c to bdd4636 Compare April 19, 2019 15:15

tomusdrw changed the title ~~Allow both ed25519 and sr25519 signatures in node/runtime~~ Fix MultiSigner, simplify tests Apr 19, 2019

tomusdrw added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Apr 19, 2019

Merge branch 'master' into td-fix-accounts

8c0b2ac

gavofyork approved these changes Apr 23, 2019

View reviewed changes

gavofyork merged commit 7a8c3d7 into master Apr 23, 2019

gavofyork deleted the td-fix-accounts branch April 23, 2019 16:18

MTDK1 pushed a commit to bdevux/substrate that referenced this pull request Jul 10, 2019

Fix MultiSigner, simplify tests (paritytech#2033)

116398f

* Fix MultiSigner, use `into_signed_tx` * Rebuild.

Conversation

tomusdrw commented Mar 18, 2019 • edited by gavofyork Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacogr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacogr commented Mar 18, 2019

Uh oh!

tomusdrw commented Mar 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gavofyork Mar 26, 2019

Choose a reason for hiding this comment

Uh oh!

gavofyork Mar 26, 2019

Choose a reason for hiding this comment

Uh oh!

gavofyork Mar 26, 2019

Choose a reason for hiding this comment

Uh oh!

gavofyork left a comment

Choose a reason for hiding this comment

Uh oh!

tomusdrw commented Apr 4, 2019

Uh oh!

gavofyork commented Apr 4, 2019

Uh oh!

tomusdrw commented Apr 4, 2019

Uh oh!

gavofyork commented Apr 16, 2019

Uh oh!

Demi-Marie commented Apr 19, 2019

Uh oh!

tomusdrw commented Apr 19, 2019

Uh oh!

tomusdrw commented Apr 19, 2019

Uh oh!

tomusdrw commented Apr 19, 2019

Uh oh!

tomusdrw commented Apr 19, 2019

Uh oh!

burdges commented Jun 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tomusdrw commented Mar 18, 2019 •

edited by gavofyork

Loading

jacogr left a comment •

edited

Loading

tomusdrw commented Mar 25, 2019 •

edited

Loading