Adds an offchain call to submit double vote reports #966

montekki · 2020-04-03T11:22:48Z

A follow up to #840

This adds a call that allows submitting double vote reports and creates and submits a signed transaction with these reports.

These changes need implementations of CreateTransaction on Runtime. Here I have copied the code from substrate changing only the SignedExtra accordingly. I am not sure that this is ok, so while I am trying to understand if it is a careful review of that part is needed.
A new fisherman Key Type is added in the same fashion as in grandpa: report equivocations substrate#3868. Probably in the end only the on in substrate should remain.
grandpa: report equivocations substrate#3868 submits a transaction with SubmitSignedTransaction::submit_signed_from first asking for all local keys with K::all(), and that's why it needs a K type bound to RuntimeAppPublic + IdentifyAccount<AccountId = T::AccountId>, i assume. to dodge this SubmitSignedFrom::submit_signed is used; I may be wrong here but the two approaches look equal.

bkchr · 2020-04-03T12:31:51Z

For 3., you are right that this looks bad^^ We should not add IdentifyAccount impls for these application specific keys.

tomusdrw · 2020-04-03T13:47:19Z

For 3., you are right that this looks bad^^ We should not add IdentifyAccount impls for these application specific keys.

You need some conversion between RuntimeAppPublic and the Public type used in the runtime at least (or more specificaly between RuntimeAppPublic::Generic and frame_system::Public). Going directly to AccountId should not be required, but it's a shortcut you are taking here.

Would this PR make it cleaner:
paritytech/substrate#5182
?

rphmeier · 2020-04-15T18:54:07Z

primitives/src/parachain.rs

@@ -88,6 +88,17 @@ application_crypto::with_pair! {
 /// so we define it to be the same type as `SessionKey`. In the future it may have different crypto.
 pub type ValidatorSignature = validator_app::Signature;

+/// The key type ID for a fisherman key.
+pub const FISHERMAN_KEY_TYPE_ID: KeyTypeId = KeyTypeId(*b"fish");


I think it's OK to have this key type, as the one in the Grandpa equivocations PR is in substrate-node, not a common library.

@andresilva does this conflict with #1000 ?

We will use this key in polkadot for all equivocation reporting across BABE, GRANDPA and Parachains. It conflicts with #1000 since I added the same key, but I'll change it after this one is merged.

runtime/test-runtime/src/lib.rs

rphmeier

Looks fine but seeking a second look-over from @andresilva

andresilva

Changes LGTM, just a nit regarding test-runtime. This will conflict with #1000 but I'll deal with the conflicts after we merge this one first.

runtime/common/src/parachains.rs

runtime/common/src/registrar.rs

runtime/test-runtime/src/lib.rs

andresilva · 2020-04-16T13:19:20Z

Needs a merge with master to fix conflicts. And also enabling this on test-runtime's SignedExtra (unless you think it's unnecessary).

andresilva

Just one more nit regarding sp-application-crypto that I didn't noticed.

andresilva · 2020-04-16T14:22:45Z

runtime/common/Cargo.toml

@@ -19,6 +19,7 @@ sp-io = { git = "https://github.com/paritytech/substrate", branch = "master", de
 sp-runtime = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }
 sp-staking = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }
 sp-core = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }
+sp-application-crypto = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }


We need to add sp-application-crypto/std below in the std feature list.

Not sure if it's only needed as dev-dependency.

andresilva · 2020-04-16T14:23:36Z

runtime/polkadot/Cargo.toml

@@ -18,6 +18,7 @@ babe-primitives = { package = "sp-consensus-babe", git = "https://github.com/par
 sp-api = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }
 inherents = { package = "sp-inherents", git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }
 offchain-primitives = { package = "sp-offchain", git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }
+sp-application-crypto = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }


Same as above, needs sp-application-crypto/std below.

Not sure if needed actually.

yep, it's not needed here, and in common it is only needed as a dev dep, thanks

23dda62 Rococo <> Wococo messages relay (#1030) bcde21d Update the wasm builder to substrate master (#1029) a8318ce Make target signer optional when sending message. (#1018) f8602e1 Fix insufficient balance when send message. (#1020) d95c0a7 greedy relayer don't need message dispatch to be prepaid if dispatch is supposed to be paid at the target chain (#1016) ad5876f Update types. (#1027) 116cbbc CI: fix starting the pipeline (#1022) 7e0fadd Add temporary `canary` job (#1019) 6787091 Update types to contain dispatch_fee_payment (#1017) 03f79ad Allow Root to assume SourceAccount. (#1011) 372d019 Return dispatch_fee_payment from message details RPC (#1014) 604eb1c Relay basic single-bit message dispatch results back to the source chain (#935) bf52fff Use plain source_queue view when selecting nonces for delivery (#1010) fc5cf7d pay dispatch fee at target chain (#911) 1e35477 Bump Substrate to `286d7ce` (#1006) 7ad07b3 Add --only-mandatory-headers mode (#1004) 5351dc9 Messages relayer operating mode (#995) 9bc29a7 Rococo <> Wococo relayer balance guard (#998) bc17341 rename messages_dispatch_weight -> message_details (#996) 95be244 Bump Rococo and Wococo spec versions (#999) c35567b Move ChainWithBalances::NativeBalance -> Chain::Balance (#990) 1bfece1 Fix some nits (#988) 334ea0f Increase pause before starting relays again (#989) 7fb8248 Fix clippy in test code (#993) d60ae50 fix clippy issues (#991) 75ca813 Make sure GRANDPA shares state with RPC. (#987) da2a38a Bump Substrate (#986) 5a9862f Update submit finality proof weight formula (#981) 69df513 Flag for rejecting all outbound messages (#982) 14d0506 Add script to setup bench machine. (#984) e74e8ab Move CI from GitHub Actions to GitLab (#814) c5ca5dd Custom justification verification (#979) 643f10d Always run on-demand headers relay in complex relay (#975) a35b0ef Add JSON type definitions for Rococo<>Wococo bridge (#977) 0eb83f2 Update cargo.deny (#980) e1d1f4c Bump Rococo/Wococo spec_version (#976) deac90d increase pause before starting relays (#974) 68d6d79 Revert to use InspectCmd, bump substrate `6bef4f4` (#966) 66e1508 Avoid hashing headers twice in verify_justification (#973) a31844f Bump `environmental` dependency (#972) 2a4c29a in auto-relays keep trying to connect to nodes until connection is established (#971) 0e767b3 removed stray file (#969) b9545dc Serve multiple lanes with single complex relay instance (#964) 73419f4 Correct type error (#968) bac256f Start finality relay spec-version guards for Rococo <> Wococo finality relays (#965) bfd7037 pass source and target chain ids to account_ownership_proof (#963) 8436073 Upstream changes from Polkadot repo (#961) e58d851 Increase account endowment amount (#960) git-subtree-dir: bridges git-subtree-split: 23dda6248236b27f20d76cbedc30e189cc6f736c

fd24e98 send R<>W messages using relay 8386837 Rococo <> Wococo messages relays 738b756 moved dummy runtime from primitives to relay client 9a42472 copypaste storage_map_final_key to avoid different runtime references/dummy runtimes 52fdf6f impl Parameter for () 80411ba FromBridgedChainEncodedMessageCall<B> -> FromBridgedChainEncodedMessageCall<DecodedCall> 2714baf Chain::ID -> Bridge::THIS_CHAIN_ID+Bridge::BRIDGED_CHAIN_ID c6ef980 MessagesInstance -> BridgedMessagesInstance 03f79ad Allow Root to assume SourceAccount. (#1011) 372d019 Return dispatch_fee_payment from message details RPC (#1014) 604eb1c Relay basic single-bit message dispatch results back to the source chain (#935) bf52fff Use plain source_queue view when selecting nonces for delivery (#1010) fc5cf7d pay dispatch fee at target chain (#911) 1e35477 Bump Substrate to `286d7ce` (#1006) 7ad07b3 Add --only-mandatory-headers mode (#1004) 5351dc9 Messages relayer operating mode (#995) 9bc29a7 Rococo <> Wococo relayer balance guard (#998) bc17341 rename messages_dispatch_weight -> message_details (#996) 95be244 Bump Rococo and Wococo spec versions (#999) c35567b Move ChainWithBalances::NativeBalance -> Chain::Balance (#990) 1bfece1 Fix some nits (#988) 334ea0f Increase pause before starting relays again (#989) 7fb8248 Fix clippy in test code (#993) d60ae50 fix clippy issues (#991) 75ca813 Make sure GRANDPA shares state with RPC. (#987) da2a38a Bump Substrate (#986) 5a9862f Update submit finality proof weight formula (#981) 69df513 Flag for rejecting all outbound messages (#982) 14d0506 Add script to setup bench machine. (#984) e74e8ab Move CI from GitHub Actions to GitLab (#814) c5ca5dd Custom justification verification (#979) 643f10d Always run on-demand headers relay in complex relay (#975) a35b0ef Add JSON type definitions for Rococo<>Wococo bridge (#977) 0eb83f2 Update cargo.deny (#980) e1d1f4c Bump Rococo/Wococo spec_version (#976) deac90d increase pause before starting relays (#974) 68d6d79 Revert to use InspectCmd, bump substrate `6bef4f4` (#966) 66e1508 Avoid hashing headers twice in verify_justification (#973) a31844f Bump `environmental` dependency (#972) 2a4c29a in auto-relays keep trying to connect to nodes until connection is established (#971) 0e767b3 removed stray file (#969) b9545dc Serve multiple lanes with single complex relay instance (#964) 73419f4 Correct type error (#968) bac256f Start finality relay spec-version guards for Rococo <> Wococo finality relays (#965) bfd7037 pass source and target chain ids to account_ownership_proof (#963) 8436073 Upstream changes from Polkadot repo (#961) e58d851 Increase account endowment amount (#960) git-subtree-dir: bridges git-subtree-split: fd24e9849fffafa92402440db9584daab96c36f2

Adds an offchain call to submit double vote reports

2d40e4b

montekki added the A0-please_review Pull request needs code review. label Apr 3, 2020

montekki requested review from andresilva and rphmeier April 3, 2020 11:22

paritytech-ci added the B2-breaksapi label Apr 3, 2020

Some tweaks

62bf421

montekki added 2 commits April 3, 2020 21:53

Remove unnecessary IdentifyAccount impls

45062eb

Merge branch 'master' into fs-parachain-double-vote-signed-transaction

c7fc689

rphmeier reviewed Apr 15, 2020

View reviewed changes

runtime/test-runtime/src/lib.rs Show resolved Hide resolved

rphmeier approved these changes Apr 15, 2020

View reviewed changes

andresilva suggested changes Apr 15, 2020

View reviewed changes

runtime/common/src/parachains.rs Show resolved Hide resolved

runtime/common/src/registrar.rs Show resolved Hide resolved

runtime/test-runtime/src/lib.rs Show resolved Hide resolved

Merge branch 'master' into fs-parachain-double-vote-signed-transaction

4b20808

Adds ValidateDoubleVoteReports to test runtime

0925fb1

andresilva suggested changes Apr 16, 2020

View reviewed changes

sp-application-crypto is only a dev dependency

186c1c8

andresilva approved these changes Apr 16, 2020

View reviewed changes

andresilva merged commit dd59eb3 into paritytech:master Apr 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds an offchain call to submit double vote reports #966

Adds an offchain call to submit double vote reports #966

montekki commented Apr 3, 2020

bkchr commented Apr 3, 2020

tomusdrw commented Apr 3, 2020

rphmeier Apr 15, 2020

andresilva Apr 16, 2020

rphmeier left a comment

andresilva left a comment

andresilva commented Apr 16, 2020

andresilva left a comment

andresilva Apr 16, 2020

andresilva Apr 16, 2020

andresilva Apr 16, 2020

andresilva Apr 16, 2020

montekki Apr 16, 2020

Adds an offchain call to submit double vote reports #966

Adds an offchain call to submit double vote reports #966

Conversation

montekki commented Apr 3, 2020

bkchr commented Apr 3, 2020

tomusdrw commented Apr 3, 2020

rphmeier Apr 15, 2020

Choose a reason for hiding this comment

andresilva Apr 16, 2020

Choose a reason for hiding this comment

rphmeier left a comment

Choose a reason for hiding this comment

andresilva left a comment

Choose a reason for hiding this comment

andresilva commented Apr 16, 2020

andresilva left a comment

Choose a reason for hiding this comment

andresilva Apr 16, 2020

Choose a reason for hiding this comment

andresilva Apr 16, 2020

Choose a reason for hiding this comment

andresilva Apr 16, 2020

Choose a reason for hiding this comment

andresilva Apr 16, 2020

Choose a reason for hiding this comment

montekki Apr 16, 2020

Choose a reason for hiding this comment