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

frame: remove finality-tracker #7228

Merged
4 commits merged into from
Oct 15, 2020
Merged

frame: remove finality-tracker #7228

4 commits merged into from
Oct 15, 2020

Conversation

andresilva
Copy link
Contributor

@andresilva andresilva commented Sep 28, 2020

The initial idea was to use the finality-tracker module to automatically trigger GRANDPA's offline fallback. Due to efficiency concerns the module currently only allows tracking finality over short periods (e.g. last 100 blocks), which is too little to act automatically on. Furthermore, we recently added #6725 which instead allows us to trigger GRANDPA's offline fallback manually through governance which is a lot safer (and preferred). Removing this as suggested in #6725 (comment).

polkadot companion: paritytech/polkadot#1762

@andresilva andresilva added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. A1-needs_burnin Pull request needs to be tested on a live validator node before merge. DevOps is notified via matrix labels Sep 28, 2020
const INHERENT_IDENTIFIER: InherentIdentifier = INHERENT_IDENTIFIER;

fn create_inherent(data: &InherentData) -> Option<Self::Call> {
if let Ok(final_num) = data.finalized_number() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be fine to remove the inherent provider right away since the runtime module already expects that inherent data about finalized number might not be available.

@andresilva andresilva added the A3-in_progress Pull request is in progress. No review needed at this stage. label Oct 13, 2020
@andresilva
Copy link
Contributor Author

Found an issue during burnin, needs some changes.

@kianenigma
Copy link
Contributor

What issues? I was actually wondering what issue could possibly arise from removal of a pallet that you did a burnin.

@andresilva
Copy link
Contributor Author

@kianenigma Since other nodes are still adding the inherent data the node will fail to decode those inherents that come in the block.

Oct 09 15:52:12 kusama-sentry-ew1-1 kusama[27879]: Oct 09 15:52:12.155  WARN 💔 Verification failed for block 0x5228ee456ee78813978d5cd0000af129bf32e719f27daa3b1530835a12f6319b received from peer: 12D3KooWAHbuhp5pq8uFeLwDBbxNWd1qtjGS95z3XVuseZ9nJDHS, "Execution: Could not convert parameter `block` between node and runtime: Error decoding field Block.extrinsics"

Still not sure about what to do since for historical blocks these inherents will always exist.

@bkchr
Copy link
Member

bkchr commented Oct 13, 2020

@kianenigma Since other nodes are still adding the inherent data the node will fail to decode those inherents that come in the block.

Oct 09 15:52:12 kusama-sentry-ew1-1 kusama[27879]: Oct 09 15:52:12.155  WARN 💔 Verification failed for block 0x5228ee456ee78813978d5cd0000af129bf32e719f27daa3b1530835a12f6319b received from peer: 12D3KooWAHbuhp5pq8uFeLwDBbxNWd1qtjGS95z3XVuseZ9nJDHS, "Execution: Could not convert parameter `block` between node and runtime: Error decoding field Block.extrinsics"

Still not sure about what to do since for historical blocks these inherents will always exist.

Wait. That means you don't have bumped the spec_version.

@kianenigma
Copy link
Contributor

I guess historical blocks will be fine because a historical runtime will also have this pallet? The problem seem more like that this PR is kinda hard to burn-in in the public, because of what you said: other nodes will produce blocks with the inherent while your node will reject.

@andresilva
Copy link
Contributor Author

andresilva commented Oct 13, 2020

@kianenigma Since other nodes are still adding the inherent data the node will fail to decode those inherents that come in the block.

Oct 09 15:52:12 kusama-sentry-ew1-1 kusama[27879]: Oct 09 15:52:12.155  WARN 💔 Verification failed for block 0x5228ee456ee78813978d5cd0000af129bf32e719f27daa3b1530835a12f6319b received from peer: 12D3KooWAHbuhp5pq8uFeLwDBbxNWd1qtjGS95z3XVuseZ9nJDHS, "Execution: Could not convert parameter `block` between node and runtime: Error decoding field Block.extrinsics"

Still not sure about what to do since for historical blocks these inherents will always exist.

Wait. That means you don't have bumped the spec_version.

Yeah, I'm an idiot 🤦. @kianenigma disregard what I said.

@andresilva
Copy link
Contributor Author

Burn-in done, no issues found.

@andresilva
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented Oct 15, 2020

Trying merge.

@ghost ghost merged commit cefcd99 into master Oct 15, 2020
@ghost ghost deleted the andre/remove-finality-tracker branch October 15, 2020 20:58
@andresilva andresilva removed A1-needs_burnin Pull request needs to be tested on a live validator node before merge. DevOps is notified via matrix A3-in_progress Pull request is in progress. No review needed at this stage. labels Oct 15, 2020
This pull request was closed.
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. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants