Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Parent hash to session index mapping in parachains module too aggressive #924

Closed
rphmeier opened this issue Mar 22, 2020 · 1 comment · Fixed by #928
Closed

Parent hash to session index mapping in parachains module too aggressive #924

rphmeier opened this issue Mar 22, 2020 · 1 comment · Fixed by #928
Assignees
Labels
I4-annoyance Code behaves within expectations, however this “expected behaviour” itself is at issue.

Comments

@rphmeier
Copy link
Contributor

#840 (comment)

cc @montekki

The pruning introduced in this PR is too aggressive, as we will not be able to accept any reports from prior eras. The main issue with this is that at the very beginning of an era there is almost nothing that can be slashed for. Technically, we should be able to accept reports from all eras for which are currently bonded, although slashing for the current era as well as the last gets us the desired behavior in practice. Eras will be ~24 hours long, so if a misbehavior is not detected within that time, it may never be.

Also as a nit, it would be nice to remove the direct dependence on staking module. Not really a problem as we are in polkadot space, not generic component space, but it still feels cleaner to expose a generic interface

@rphmeier rphmeier added the I4-annoyance Code behaves within expectations, however this “expected behaviour” itself is at issue. label Mar 22, 2020
@montekki montekki self-assigned this Mar 22, 2020
@rphmeier
Copy link
Contributor Author

rphmeier commented Mar 22, 2020

A completely alternative strategy to achieve the same goal that the parent_hash -> session_index mapping did just occur to me, which may be cleaner overall.

The issue that we have without this mapping is that we have no way of ensuring that the proof-of-set-membership actually corresponds to the same session as the Statements were signed in. However, every signature on a Statement also encompasses the parent_hash. The mapping lets us associate the parent-hash we need to check the signature with the correct session.

This has two drawbacks:

  • We have to maintain the mapping and prune it, which is difficult and potentially heavy on the runtime side (thousands of old parent_hashes to clean up when pruning)
  • We can only slash double-vote offences on our chain, since any parent_hash from another fork will be unrecognized. This weakens accountable-safety somewhat because it's best to be slashable on all forks.

So as an alternative to the mapping that solves the same problem, I propose that we instead include the session_index in the payload for fn sign_table_statement. The implementation fallout from this would include at least these tasks:

  • Add a primitive type for SigningContext { session_index, parent_hash }.
  • Add a runtime API for fn signing_context() -> SigningContext which returns the context with current session index and parent hash.
  • Alter sign_table_statement and check_statement functions to accept SigningContext
  • Extend TableContext initialization with a SigningContext
  • Pass SigningContext fetched from to the network when we callnew_local_leaf`.
  • Amend DoubleVoteReport to take a SigningContext, and compare the session_index there with the Proof's.

tomusdrw added a commit that referenced this issue May 3, 2021
f43c92430 Fix account derivation in CLI (#952)
9ac07e733 Add backbone configuration of cargo-spellcheck (#924)
2761c3fef Message dispatch support multiple instances (#942)
801c99f3d Add Wococo<>Rococo Header Relayer (#925)
21f490514 Remove Westend<>Rococo header sync (#940)
06235f162 do not panic if pallet is not yet initialized (#937)
a13ee0bc3 Bump Substrate (#939)
f8680cbfc jsonrpsee alpha6 (#938)
6163bcbf4 reonnect to failed client in on-demand relay background task (#936)
14e82bea3 Do not spawn additional task for on-demand relays (#933)
b1557b882 Relay at least one header for every source chain session (#923)
9420649c1 Remove deprecated Runtime Header APIs (#932)
9627011e1 Update README.md (#931)
7b736b9cc Truncate output in logs. (#930)
faad06e39 Make sure that relayers have dates in logs. (#927)
077345351 Update dump-logs script. (#928)
c2d56b2e9 Add pruning to bechmarks & update weights. (#918)
a30c51dc9 Add properties to Chain Spec (#917)
d691c73e9 Fix issue with on-demand headers relay not starting (#921)
8ee55c1e1 Fix image publishing. (#922)
f51fb59d0 Prefix in relay loops logs (#920)

git-subtree-dir: bridges
git-subtree-split: f43c924301c227d29ec161f6815d9bac458a211d
HCastano added a commit that referenced this issue May 4, 2021
b2099c5c Bump Substrate to `b094edaf` (#958)
3f037094 Bump endowment amounts on Rialto and Millau (#957)
b21fd07c Bump Substrate WASM builder (#947)
30ccd07c Bump Substrate to `ec180313` (#955)
a7422ab1 Upgrade to GitHub-native Dependabot (#945)
ed20ef34 Move pallet-bridge-dispatch types to primitives (#948)
2070c4d6 Endow accounts and add `bridgeIds` to chainspec. (#951)
f43c9243 Fix account derivation in CLI (#952)
9ac07e73 Add backbone configuration of cargo-spellcheck (#924)
2761c3fe Message dispatch support multiple instances (#942)
801c99f3 Add Wococo<>Rococo Header Relayer (#925)
21f49051 Remove Westend<>Rococo header sync (#940)
06235f16 do not panic if pallet is not yet initialized (#937)
a13ee0bc Bump Substrate (#939)
f8680cbf jsonrpsee alpha6 (#938)
6163bcbf reonnect to failed client in on-demand relay background task (#936)
14e82bea Do not spawn additional task for on-demand relays (#933)
b1557b88 Relay at least one header for every source chain session (#923)
9420649c Remove deprecated Runtime Header APIs (#932)
9627011e Update README.md (#931)
7b736b9c Truncate output in logs. (#930)
faad06e3 Make sure that relayers have dates in logs. (#927)
07734535 Update dump-logs script. (#928)
c2d56b2e Add pruning to bechmarks & update weights. (#918)
a30c51dc Add properties to Chain Spec (#917)
d691c73e Fix issue with on-demand headers relay not starting (#921)
8ee55c1e Fix image publishing. (#922)
f51fb59d Prefix in relay loops logs (#920)

git-subtree-dir: bridges
git-subtree-split: b2099c5c0baf569e2ec7228507b6e4f3972143cc
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
I4-annoyance Code behaves within expectations, however this “expected behaviour” itself is at issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants