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

Remove Parent Hash to Session mapping #928

Merged
merged 9 commits into from
Mar 25, 2020
Merged

Remove Parent Hash to Session mapping #928

merged 9 commits into from
Mar 25, 2020

Conversation

montekki
Copy link
Contributor

Closes #924

Implements the idea described in the issue above:

  1. Removes the mapping from parent hashes to sessions
  2. Adds a SigningContext type.

@montekki montekki added the A3-in_progress Pull request is in progress. No review needed at this stage. label Mar 23, 2020
@montekki montekki 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 Mar 23, 2020
@montekki montekki requested a review from rphmeier March 23, 2020 16:59
@@ -673,6 +682,8 @@ sp_api::decl_runtime_apis! {
/// Extract the abridged head that was set in the extrinsics.
fn get_heads(extrinsics: Vec<<Block as BlockT>::Extrinsic>)
-> Option<Vec<AbridgedCandidateReceipt>>;
/// Get a `SigningContext` with current `SessionIndex` and parent hash.
fn signing_context() -> SigningContext<Hash>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bkchr it's safe to add this runtime API?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a function is relative safe. Is this already used somewhere? If yes, we should check that the version of the api matches, because we probably have a new node release before a runtime upgrade.

validation/src/lib.rs Outdated Show resolved Hide resolved
bkchr
bkchr previously requested changes Mar 24, 2020
@@ -673,6 +682,8 @@ sp_api::decl_runtime_apis! {
/// Extract the abridged head that was set in the extrinsics.
fn get_heads(extrinsics: Vec<<Block as BlockT>::Extrinsic>)
-> Option<Vec<AbridgedCandidateReceipt>>;
/// Get a `SigningContext` with current `SessionIndex` and parent hash.
fn signing_context() -> SigningContext<Hash>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a function is relative safe. Is this already used somewhere? If yes, we should check that the version of the api matches, because we probably have a new node release before a runtime upgrade.

@@ -334,11 +334,13 @@ impl<C, N, P, SP> ParachainValidationInstances<C, N, P, SP> where
}
}

let signing_context = self.client.runtime_api().signing_context(&id)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be guarded. As otherwise we try to call an api that is not yet available.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we should just error out of this function in this case? I think that then the node will not be able to do validation work until the runtime is upgraded.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the api function can not be found, an error is returned, so yes^^

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I bumped ParachainHost and added a guard.

Copy link
Contributor

@rphmeier rphmeier Mar 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bkchr This should not be triggered during major sync, regardless, as the validation service only dispatches work for chain head.

)? {
api.signing_context(&id)?
} else {
return Err(Error::Consensus(ConsensusError::ChainLookup("runtime version mismatch".to_string())));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this spam the logs?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • This error doesn't really tell you what kind of mismatch it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

improved the error message.

all errors in this function including calls to runtime will be logged with warn!, as far as the code goes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not good. We have Kusama and all runtimes from the genesis until this new api is added, will now print this warning on import.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I fully understand how things should work in case of updated node but not upgraded runtime. The node code expects a call to this new runtime api point. What should it do in case the runtime is not updated with it? Should the code mantain two paths of execution: one with old logic and one with new one? If not how things can possibly work in this situation b.c. it looks like here if we error out on has_api_with we will no longer be able to progress the chain and we will never get to the runtime upgrade then. How can this possibly work if the update to the node and runtime are not done atomically at the same time?

As for the error: if it occures we can no longer continue the execution of a function so we exit with an error. The caller logs all errors originating from this function. Do we remove this logging at all? The same type of error can be returned in other cases, so we can not easily filter that out.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got confused by something different:

2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0x9e818a61288f0ad76e16fe252447c83cd9e0865405f548dd36cf90a932ffbe10
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0x74e4681d930c3892872bfd04730d0e6019ea4ed2ec7d6e97de6996110e0eb729
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0x86cd4c5962471beba6bfa7dd7ae00fe38142e1f10c7e482b7cfa2106eaaece81
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0x0271a53da7c0fb96b4545226966bfb09df936f8732b0b244f81eac40131df088
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0xe032a3737d4a218c27456da551be3d971efbc7ac7d3b0649b35357c2e9c1364d
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0x69738a9f8995fcb6bc86c95dc457233d57e69208ea2bd9ef814abdc81e8733e9
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0x2a285510b49c6e5737542a479080c6efc8a5b36fe47cafa2be376ef527d5a38b
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0x92bc36382c7b0424fd4c1e279b06c563ab6ac442cde2b4b1f6e6be6f333a1531
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0xfce756fdbab4f534752a6a2666f707794bf7ab10e7f925a5ca4f377f21b25e68
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0x4ab00bf4cc8efdc834c6e228dbd9470a1aaa091c416c754321b876e0e3827b71
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0x88ed3030837c3d465409db10fef417134ac7372f1aa929e1a88b021aff36f216
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0x4b2f09b2b2227d3a9f747c157b6b31c911a1372c6bf3ca9e3177d35ec3dbb935
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0x4f86d7c83f54ef359b5c34e8b7bcc336cefe623fa80def689df996805cc9095e
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0xd1fdb6baf18a2297b930d453137344bf421dd3557e0280f97c119965068985eb
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0x30d8492a5f36ad7f57c5c226c284330f673555f32245322c5c51a94f64796337
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0xb1c23716f87fe34504254b399b3819c37c3b836b5a697b7a2cae73e36203dbb5
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0x45a998350b0016951bcb37d0faf595f72f6172fa2dd36d14feac64713cb74eef
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0xd0a60f3033634b020e2bebf618c7c972b45a7f46221c900ab17b2975eb04864a
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0x6f64a7445184b28a13223709964cbed14ea693bea750d6063632b5ac5dd629ae
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0xd7937ac2e17bfd75f029cc4c332f68ac32be42f85618e951a0c830b400c1785f
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0xefdebdfc396c183b34974641c7e844fcfc89942ad31c91afbb46e5900fdb1e47
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0xad3b7034ff9699aa680242fc5743fb4713611d77ba22a9392fbb80b4ab755118
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0xa4c9c7d4e69d127982fd697a3bfdb6eb11114c65ccefbcd0625d316a65ebb2c2
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0xa537f974a01e8d2b78daed4d98bc558ff78c43927b77ddfcc29d425cd57ed5f7
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0x4be013375c1cff6f7b29e844bb90f35bdd3562f6a4a60f5905342cdf5536d53e
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0xc8e3884806ae778f48190a97bbb477faf0cfd5d4f7f6c661106ec646e294573f
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0xc844450c7bf56f0f0409a607576f56edb97ab2f8d7aa6f49956a0e9878bc24ad
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0x41bd10b3c1e3dff0d1d4bdf21f5f3fe701bc536dd7912871d122016323ffba51
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0x3731f4cd742413e5dd1e0b58b09b13fcb58aa2972355c0940df616e684b6a627
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0x1db425f4aecc4ba8bb66b420056a5f1e76bc06a11c9551bc6d73674b1ad0b3f5
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0x1b78e3af1937c1092c876acf754ac2c0972f09307276b38c858bbbe80779c8bb
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0x7798990a8261142d2b51555c52fb5717b0e414b1221f31a7a25f2d791bf4737e
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0x57391d08909de1df04078a44f03f57c11ab3f16972fea771800f1365866753b2
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0x49d2c15920a0fae55e7b0685f45e8686391ea1ebecb1355a4862a2a448ed635f
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0x3fe55b8f9442f5644913ae32e02ac13e5dfa31e594ba7f817c9053b36b69cabb
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0x608b59fdff7c031fb60b9f5515b3d2c7aee1ec5592f687f4c9f957ce5a055068
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0x1de5188b5324dd533be6910581a1a36c0cbd869b17bd573bca6a81eee1743f20
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0xf30718a95bbdf68cc79ad96663f73f5e555cc629577bd8fb3ec47ab9c537e420
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0xfac619bee77d7e975fd511dab1d4ca3fbfe47441dba257f5b599e7902a3bda60
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0x906bcb5de1014cbdbb0749853947baf6e2e552d75d323dfca625e6ab591b7630
2020-03-24 22:03:57 Failed to extract candidates from block body of imported block 0xede605174ad12c7c972f4cc6cdb7df493edc0a738cdac0f07b04664ab579f879

Thought that this is related, but that is another piece of the code.

Sorry!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is from availability store printed on every finality notification. Since no block now has parachain heads set. If that is annoying it makes sense to remove this log until we actually have parachains.

@rphmeier rphmeier merged commit 710f4ca into paritytech:master Mar 25, 2020
HCastano added a commit to HCastano/polkadot that referenced this pull request Apr 27, 2021
f95cc7a5 Merge branch 'master' into hc-add-wococo-support
a13ee0bc Bump Substrate (paritytech#939)
f8680cbf jsonrpsee alpha6 (paritytech#938)
6163bcbf reonnect to failed client in on-demand relay background task (paritytech#936)
14e82bea Do not spawn additional task for on-demand relays (paritytech#933)
b1557b88 Relay at least one header for every source chain session (paritytech#923)
9420649c Remove deprecated Runtime Header APIs (paritytech#932)
9627011e Update README.md (paritytech#931)
7b736b9c Truncate output in logs. (paritytech#930)
faad06e3 Make sure that relayers have dates in logs. (paritytech#927)
07734535 Update dump-logs script. (paritytech#928)
efe215e4 RustFmt
02522249 Fix test
48b41d82 Add support for relaying headers between Rococo and Wococo
f16b6b41 Add CLI support for initializing the Wococo<>Rococo bridge
8c6e6443 Add more Wococo boilerplate code
c2d56b2e Add pruning to bechmarks & update weights. (paritytech#918)
a30c51dc Add properties to Chain Spec (paritytech#917)
28d3ed2f Add Wococo primitives crate
d691c73e Fix issue with on-demand headers relay not starting (paritytech#921)
8ee55c1e Fix image publishing. (paritytech#922)
f51fb59d Prefix in relay loops logs (paritytech#920)

git-subtree-dir: bridges
git-subtree-split: f95cc7a57b48948f17d33f5be3ea01c752deba94
HCastano added a commit that referenced this pull request Apr 29, 2021
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: 801c99f3de0fa4d0b61e4e065fa30817179368ea
tomusdrw added a commit that referenced this pull request 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 pull request 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
A0-please_review Pull request needs code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parent hash to session index mapping in parachains module too aggressive
4 participants