Add documentation to SubmitSignedTransaction and actually make it work by tomusdrw · Pull Request #4200 · paritytech/substrate

tomusdrw · 2019-11-25T16:32:46Z

With recent changes to crypto stuff (notably IdentifyAccount that makes the AccountId diverge from PublicKey) the SubmitSignedTransaction trait family was not really working correctly.

The main complications arise because of application-crypto type wrappers (app-crypto) and signed aggregators (MultiSigner), this requires a bit of From/TryInto/Into magic and some additional trait bounds, that on the first sight look unnecessary in this code :)

This PR is an attempt to fix that and also make it more convenient to use local keys for transaction signing (see SigningAccountFinder). This additional trait bound can be used to create and submit signed transactions from offchain calls using all local keys (intersected with some list of accounts).

Still WiP, as I want to work on tests a bit more, but wanted to make this visible for @kianenigma @gnunicorn and @andresilva

frame/system/src/offchain.rs

…ctually make it work Squashed commit of the following: commit dc8d71c Author: Tomasz Drwięga <tomasz@parity.io> Date: Tue Dec 3 16:29:33 2019 +0100 Split the method to avoid confusing type error message. commit 0c4c037 Author: Tomasz Drwięga <tomasz@parity.io> Date: Tue Dec 3 16:19:55 2019 +0100 Make accounts optional, fix logic. commit d715f64 Author: Tomasz Drwięga <tomasz@parity.io> Date: Tue Dec 3 10:06:20 2019 +0100 Remove warning. commit 3f38218 Merge: f85b890 368318c Author: Tomasz Drwięga <tomasz@parity.io> Date: Tue Dec 3 07:08:05 2019 +0100 Merge branch 'master' into td-signed-transactions commit f85b890 Merge: f8c9540 d8d5da2 Author: Tomasz Drwięga <tomasz@parity.io> Date: Mon Dec 2 13:57:25 2019 +0100 Merge branch 'master' into td-signed-transactions commit f8c9540 Author: Tomasz Drwięga <tomasz@parity.io> Date: Mon Nov 25 17:34:52 2019 +0100 Forgotten import. commit a645b90 Author: Tomasz Drwięga <tomasz@parity.io> Date: Mon Nov 25 17:32:10 2019 +0100 Fix naming and bounds. commit bc28c60 Author: Tomasz Drwięga <tomasz@parity.io> Date: Mon Nov 25 17:01:05 2019 +0100 Add documentation to signed transactions and actually make them work.

gavofyork · 2020-01-03T20:57:26Z

@tomusdrw is this merge correct?

tomusdrw · 2020-01-05T16:05:41Z

Yup, thanks! Could I get a second review on this? @bkchr maybe if you have some time? 😈

bkchr

Just "some" nitpicks.

bkchr · 2020-01-06T08:21:09Z

bin/node/executor/tests/basic.rs

@@ -0,0 +1,837 @@
+// Copyright 2018-2019 Parity Technologies (UK) Ltd.


Suggested change

// Copyright 2018-2019 Parity Technologies (UK) Ltd.

// Copyright 2018-2020 Parity Technologies (UK) Ltd.

bkchr · 2020-01-06T08:21:31Z

bin/node/executor/tests/basic.rs

+use node_testing::keyring::*;
+
+mod common;
+use self::common::{*, sign};


Suggested change

use self::common::{*, sign};

use self::common::*;

This doesn't work as there is a conflict with glob imports (common and keystore), so we need to define sign explicitly.

bkchr · 2020-01-06T08:21:39Z

bin/node/executor/tests/basic.rs

+mod common;
+use self::common::{*, sign};
+
+


Suggested change

bin/node/executor/tests/common.rs

bin/node/executor/tests/fees.rs

frame/system/src/offchain.rs

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

frame/system/src/lib.rs

tomusdrw · 2020-01-09T08:24:26Z

@bkchr convinced me to change offchain_worker RuntimeApi and pass the entire header to make the initialization properly, I'm working on this right now, hence the inprogress tag.

* session: runtime api for generating session membership proofs * grandpa: add runtime api for creating equivocation report txs * grandpa: submit signed equivocation report transactions * grandpa: use proper equivocation report type * grandpa: report equivocations * grandpa: validate equivocation proof * grandpa: update to finality-grandpa 0.9.1 * grandpa: fix encoding of session membership proof * grandpa: initialize set id session mapping for genesis session * grandpa: fix bug in set_id session validation * fix compilation * cleanup from merge conflicts * cleanup crate tomls * grandpa: refactor equivocation handling to separate trait * node-template: fix compilation * fix test compilation * bump finality-grandpa to v0.10.2 * rpc: fix runtime version test * CHERRY-PICK #4200: Add documentation to SubmitSignedTransaction and actually make it work Squashed commit of the following: commit dc8d71c Author: Tomasz Drwięga <tomasz@parity.io> Date: Tue Dec 3 16:29:33 2019 +0100 Split the method to avoid confusing type error message. commit 0c4c037 Author: Tomasz Drwięga <tomasz@parity.io> Date: Tue Dec 3 16:19:55 2019 +0100 Make accounts optional, fix logic. commit d715f64 Author: Tomasz Drwięga <tomasz@parity.io> Date: Tue Dec 3 10:06:20 2019 +0100 Remove warning. commit 3f38218 Merge: f85b890 368318c Author: Tomasz Drwięga <tomasz@parity.io> Date: Tue Dec 3 07:08:05 2019 +0100 Merge branch 'master' into td-signed-transactions commit f85b890 Merge: f8c9540 d8d5da2 Author: Tomasz Drwięga <tomasz@parity.io> Date: Mon Dec 2 13:57:25 2019 +0100 Merge branch 'master' into td-signed-transactions commit f8c9540 Author: Tomasz Drwięga <tomasz@parity.io> Date: Mon Nov 25 17:34:52 2019 +0100 Forgotten import. commit a645b90 Author: Tomasz Drwięga <tomasz@parity.io> Date: Mon Nov 25 17:32:10 2019 +0100 Fix naming and bounds. commit bc28c60 Author: Tomasz Drwięga <tomasz@parity.io> Date: Mon Nov 25 17:01:05 2019 +0100 Add documentation to signed transactions and actually make them work. * grandpa: skip block initialization on report submission method * primitives: allow transaction pool access by default for offchain calls * grandpa: unused parameters * grandpa: remove unused method * grandpa: enable equivocation reporting * grandpa: add workaround for parameter encoding * grandpa: fix localized_payload calls in tests * fix submit_report_equivocation_extrinsic in runtimes * node: fix submit transaction test compilation * node: bump spec_version * rpc: fix api version test * grandpa: allow custom equivocation offence type * grandpa: add test for authorities::next_change_height * grandpa: cleanup report_equivocation function * node: move reporting app crypto to node-primitives * grandpa: move equivocation traits to own module * grandpa: rename app-crypto crate import * grandpa: export equivocation types * node: bump spec_version * grandpa: rename EquivocationReport to EquivocationProof * grandpa: add missing docs to primitives * grandpa: add missing docs to equivocation * node: fix compilation * grandpa: add missing docs to pallet * node: bump spec_version * fix whitespace * grandpa: return error on offence reporting * grandpa: expose session and validator count in proofs through traits * grandpa: use strong key in module KeyOwnerProofSystem * grandpa: move key ownership proof to grandpa runtime api * grandpa: remove unnecessary cloning when checking equivocation proof * grandpa: make report_equivocation a method in Environment * support: implement KeyOwnerProofSystem for () * grandpa: move KeyOwnerProofSystem to module trait * test-utils: fix runtime compilation * grandpa: fix test compilation * grandpa: fix test compilation after merge * grandpa: simplify transaction submission types * grandpa: validate equivocation report in signed extension * client: fix test * node: use ValidateEquivocationReport signed extension * grandpa: expose key ownership proof under opaque type * grandpa: better docs on key ownership proofs * grandpa: add note about signed extension * grandpa: add ValidateEquivocationReport::new * grandpa: remove skip_initialize_block from runtime api * grandpa: use new offchain transaction submission API * grandpa: take set_id in generate_key_ownership_proof * grandpa: update to finality-grandpa v0.12.2 * grandpa: cleanup usages of AuthoritySet::current * grandpa: fix test * grandpa: add mocking utilities for equivocation reporting * grandpa: add test for equivocation reporting * grandpa: move SetIdSession initialization * grandpa: add more tests * node: enable historical session manager * node: bump spec_version * node: use strong key types in KeyOwnerProofSystem definitions * grandpa: export GrandpaEquivocationOffence type

tomusdrw added 3 commits November 25, 2019 17:01

Add documentation to signed transactions and actually make them work.

bc28c60

Fix naming and bounds.

a645b90

Forgotten import.

f8c9540

gavofyork added the A3-in_progress Pull request is in progress. No review needed at this stage. label Nov 26, 2019

andresilva reviewed Nov 27, 2019

View reviewed changes

frame/system/src/offchain.rs Show resolved Hide resolved

tomusdrw added 7 commits December 2, 2019 13:57

Merge branch 'master' into td-signed-transactions

f85b890

Merge branch 'master' into td-signed-transactions

3f38218

Remove warning.

d715f64

Make accounts optional, fix logic.

0c4c037

Split the method to avoid confusing type error message.

dc8d71c

Move executor tests to integration.

173cf6c

Add submit transactions tests.

90a00fd

andresilva mentioned this pull request Dec 11, 2019

grandpa: report equivocations #3868

Merged

7 tasks

Merge branch 'master' into td-signed-transactions

901dbdb

tomusdrw mentioned this pull request Dec 12, 2019

Merge changes in frame-system offchain to upstream? tomusdrw/sub0-offchain-workshop#2

Open

HCastano and others added 9 commits December 19, 2019 00:28

Make submit_transaction tests compile

d103fa1

Remove a file that was accidently committed

99c54b7

Merge branch 'master' into td-signed-transactions

dbc81dd

Add can_sign helper function.

9eb7a2e

Fix compilation.

0ccb951

Add a key to keystore.

73a936c

Fix the tests.

d776d50

Remove env_logger.

0ac61d7

Fix sending multiple transactions.

8755539

tomusdrw force-pushed the td-signed-transactions branch from 8a0e597 to 8755539 Compare December 20, 2019 16:44

Remove commented code.

fc6b5fe

tomusdrw marked this pull request as ready for review December 20, 2019 17:07

tomusdrw requested a review from kianenigma as a code owner December 20, 2019 17:07

tomusdrw removed the A3-in_progress Pull request is in progress. No review needed at this stage. label Dec 20, 2019

gavofyork approved these changes Jan 3, 2020

View reviewed changes

gavofyork added A6-seemsok and removed A0-please_review Pull request needs code review. labels Jan 3, 2020

bkchr approved these changes Jan 6, 2020

View reviewed changes

tomusdrw and others added 7 commits January 6, 2020 10:39

Apply suggestions from code review

da816e2

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

Make sure to initialize block correctly.

a2c6d09

Initialize block for offchain workers.

af5724c

Add test for transaction validity.

3eda535

Merge branch 'master' into td-signed-transactions

765f96f

Fix tests.

6bafaff

Review suggestions.

021f586

tomusdrw requested review from Demi-Marie, gui1117 and pepyakin as code owners January 8, 2020 15:04

bkchr reviewed Jan 8, 2020

View reviewed changes

frame/system/src/lib.rs Outdated Show resolved Hide resolved

tomusdrw added 2 commits January 8, 2020 16:37

Remove redundant comment.

3ac00ff

Make sure to use correct block number of authoring.

7f38d4c

tomusdrw added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A6-seemsok labels Jan 8, 2020

tomusdrw added 3 commits January 9, 2020 16:07

Change the runtime API.

afb99f9

Support both versions.

a945a8c

Bump spec version, fix RPC test.

4c66a45

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 Jan 9, 2020

gavofyork merged commit 8974349 into master Jan 10, 2020

gavofyork deleted the td-signed-transactions branch January 10, 2020 00:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add documentation to SubmitSignedTransaction and actually make it work#4200

Add documentation to SubmitSignedTransaction and actually make it work#4200
gavofyork merged 37 commits intomasterfrom
td-signed-transactions

tomusdrw commented Nov 25, 2019

Uh oh!

Uh oh!

gavofyork commented Jan 3, 2020

Uh oh!

tomusdrw commented Jan 5, 2020

Uh oh!

bkchr left a comment

Uh oh!

bkchr Jan 6, 2020

Uh oh!

bkchr Jan 6, 2020

Uh oh!

tomusdrw Jan 8, 2020

Uh oh!

bkchr Jan 6, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tomusdrw commented Jan 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

		@@ -0,0 +1,837 @@
		// Copyright 2018-2019 Parity Technologies (UK) Ltd.

	// Copyright 2018-2019 Parity Technologies (UK) Ltd.
	// Copyright 2018-2020 Parity Technologies (UK) Ltd.

Conversation

tomusdrw commented Nov 25, 2019

Uh oh!

Uh oh!

gavofyork commented Jan 3, 2020

Uh oh!

tomusdrw commented Jan 5, 2020

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

bkchr Jan 6, 2020

Choose a reason for hiding this comment

Uh oh!

bkchr Jan 6, 2020

Choose a reason for hiding this comment

Uh oh!

tomusdrw Jan 8, 2020

Choose a reason for hiding this comment

Uh oh!

bkchr Jan 6, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tomusdrw commented Jan 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants