Submit (& sign) extrinsics from the runtime. #3514

tomusdrw · 2019-08-29T14:06:19Z

This PR introduces a helper types to construct, sign and submit transactions from within the runtime.

I've refactored the im-online code to use the new types and also created a way to submit signed transactions. That's going to be needed for instance for equivocation reporting.
Also all places where signatures where constructed (and hashed) are now replaced with SignedPayload. Proper typing on Extra also detected a bug in subkey (where the transfers did not contain the correct SignedExtensions payload).

Note that to actually use this API, the call needs to be invoked in the specific OffchainCall context.

core/sr-primitives/src/generic/unchecked_extrinsic.rs

bkchr · 2019-08-29T15:53:52Z

core/sr-primitives/src/generic/unchecked_extrinsic.rs

+	}
+}
+
+impl<Call, Extra> Encode for SignedPayload<Call, Extra> where


Why is this implementation required?

The generic Signer works on anything that is Encode. I'll rather remove the using_encoded function, I've only added it to be able to document the behavior.

You can also put documentation on top of a trait implementation. It will be shown in rustdocs as welll.

bkchr · 2019-08-29T15:54:37Z

core/sr-primitives/src/generic/unchecked_extrinsic.rs

+	/// Get an encoded version of this payload.
+	///
+	/// Payloads longer than 256 bytes are going to be `blake2_256`-hashed.
+	pub fn using_encoded<O>(&self, f: impl FnOnce(&[u8]) -> O) -> O {


Maybe we should give this a different name?

kianenigma · 2019-08-30T08:25:09Z

core/sr-primitives/src/generic/unchecked_extrinsic.rs

+/// is going to be different than the `SignaturePayload` - so the thing the extrinsic
+/// actually contains.
+pub struct SignedPayload<Call, Extra: SignedExtension> {
+	raw_payload: (


I could not find a reason but I just wonder if you also argued/tought of making this a flat struct rather than one with a single element of a tuple? or is there a strong reason?

Yeah, makes sense. I guess, arguably self.raw_payload.using_encoded is more readable than self.0, but indeed it feels simpler to avoid naming it.

kianenigma · 2019-08-30T08:28:57Z

node/runtime/src/lib.rs

 	type ReportLatency = ReportLatency;
 }

+impl system::offchain::CreateTransaction<Runtime, UncheckedExtrinsic> for Runtime {


where is this being used?

We pass this as SubmitTransaction to im-online:

type SubmitTransaction = TransactionSubmitter<ImOnlineId, Runtime, UncheckedExtrinsic>;

(see Runtime as the second parameter)

Co-Authored-By: Bastian Köcher <[email protected]>

kianenigma · 2019-08-30T08:32:22Z

srml/system/src/offchain.rs

+
+impl<Account, Signature, AppPublic> Signer<Account, Signature> for AppPublic where
+	AppPublic: RuntimeAppPublic + From<Account>,
+	AppPublic::Signature: Into<Signature>,


I am a bit unclear on these new crypto types: what do the bounds here translate to? Who will end up having this impl?

My understanding:

So we have a low-level crypto types (like sr25519 and ed25519) that are commonly used within the "core" runtime part (as AccountId, etc).

We also have application-specific crypto types, which are basically wrappers for the low-level types with additional information about the module (i.e. application) they are coming from.

So for instance, im-online is (or may be) using sr25519 as the crypto type, but instead of sr25119::Public you will rather use im_online::app::Public, as this type has additional APP_ID identification to know it's coming from im_online module.
The APP_ID is very useful especially when querying the Keystore cause we might have many sr25519 keys, but only one (or some) of them will be the one used by im_online module.

So the bounds here basically say:

Please let me know the application-specific crypto type that you want to use for a particular Account.

Now that I think of it, in the future it might actually be better to use AppPublic instead of Account right from the start (i.e. when we initiate the signing), rather then convert in the middle of the process.
I'll refactor this if I find it more convenient to use that way, when equivocations reporting will be implemented.

kianenigma · 2019-08-30T08:37:33Z

subkey/src/main.rs

 			println!("Using a genesis hash of {}", HexDisplay::from(&genesis_hash.as_ref()));

-			let raw_payload = (
+			let raw_payload = SignedPayload::from_raw(


great that this was identified! I exactly had in on a personal todo list to: check subkey with signed extension.

If not here, at some point I would like to add some tests to it to make sure subkey is always generating stuff that can be verified by node (if that's a correct assumption).

kianenigma

Just a bunch of questions; looks good!

…signing

andresilva

LGTM. So the idea is that when we use these APIs for equivocation reporting we'll have to call them under offchain context?

tomusdrw · 2019-08-30T19:32:50Z

@andresilva yes. There is a special OffchainCall context (see #3454) and you can specify the desired capabilities (for this you'd need Keystore and TransactionPool).

* Abstract constructing extrinsic and signing. * Initial impl of signer. * Implement get payload. * Clean up the code. * Improve docs. * Bump version. * Update core/sr-primitives/src/generic/unchecked_extrinsic.rs Co-Authored-By: Bastian Köcher <[email protected]> * Fix tests & address grumbles. * Fix build. * Fix runtime tests. * Fix bound test. * Fix bound test.

tomusdrw added 10 commits August 22, 2019 13:04

Abstract constructing extrinsic and signing.

51c3526

Initial impl of signer.

f4e7284

Merge branch 'master' into td-signing

6451ed1

Implement get payload.

9a179d9

Merge branch 'master' into td-signing

dd6b106

Merge branch 'master' into td-signing

ebe3078

Clean up the code.

7222869

Merge branch 'master' into td-signing

6883345

Improve docs.

f74cfc4

Bump version.

2c23ee0

tomusdrw added A0-please_review Pull request needs code review. M4-core labels Aug 29, 2019

tomusdrw requested review from gui1117 and kianenigma as code owners August 29, 2019 14:06

bkchr approved these changes Aug 29, 2019

View reviewed changes

kianenigma reviewed Aug 30, 2019

View reviewed changes

Update core/sr-primitives/src/generic/unchecked_extrinsic.rs

1516067

Co-Authored-By: Bastian Köcher <[email protected]>

kianenigma reviewed Aug 30, 2019

View reviewed changes

kianenigma approved these changes Aug 30, 2019

View reviewed changes

tomusdrw added 6 commits August 30, 2019 10:41

Fix tests & address grumbles.

5ee8936

Fix build.

4f13af4

Fix runtime tests.

ddf540f

Fix bound test.

b14dedb

Fix bound test.

09611d3

Merge branch 'td-signing' of github.com:paritytech/substrate into td-…

17db4b3

…signing

tomusdrw force-pushed the td-signing branch from 7604119 to 17db4b3 Compare August 30, 2019 16:53

andresilva approved these changes Aug 30, 2019

View reviewed changes

gavofyork merged commit 25eb50f into master Sep 1, 2019

gavofyork deleted the td-signing branch September 1, 2019 00:19

Submit (& sign) extrinsics from the runtime. #3514

Submit (& sign) extrinsics from the runtime. #3514

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomusdrw Aug 30, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kianenigma Aug 30, 2019 • 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!

kianenigma 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!

andresilva 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!

tomusdrw commented Aug 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tomusdrw commented Aug 29, 2019 •

edited

Loading

tomusdrw Aug 30, 2019 •

edited

Loading

kianenigma Aug 30, 2019 •

edited

Loading

kianenigma left a comment •

edited

Loading

andresilva left a comment •

edited

Loading