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

Allow updating configuration of changes tries #3201

Merged
merged 80 commits into from
Jan 16, 2020

Conversation

svyatonik
Copy link
Contributor

@svyatonik svyatonik commented Jul 25, 2019

IMPORTANT NOTES FOR REVIEWERS

  1. this PR adds column to the database, but provides no upgrade functionality. I've checked that last time we changed this constant, there were also no upgrade. But it has been ~4 months ago, so probably we need to implement auto-upgrade now?
  2. this PR should change absolutely nothing when chain is configured to work without changes tries (which is by default). That's (IMO) the main thing to look at in this PR

=================================================================

Follow up of #2840 (comment)

This PR introduces new changes-tries related digest item (ChangesTrieSignal) + separate SRML method that is used to change CT configuration (and emits this signal). It is also possible to use the same signal for emergency creation of max-level digest CT, though it'll be implemented in next PRs.

The idea is to use consensus cache (which from now shouldn't be called consensus cache anymore) to hold intervals (and also config itself) where CT configuration hasn't be changed. When building CT, you only need actual CT configuration => work is done within single interval. When fetching key changes/proof, this may affect several intervals => every interval will have its own state_machine/src/changes_trie/* related call.

Detailed description:

  • (extracted to separate PR) cache is modified to support pruning strategy selection - we do not actually need authorities once block is finalized (so cached entries can be pruned). But we may need to hold changes tries configuration forever. So there are two strategies: NeverPrune and ByDepth(N);
  • (extracted to separate PR) since cache is now not consensus-specific, I've moved well_known_cache_keys from consensus_common to the client::backend;
  • moved db-backed changes trie storage (DbChangesTrieStorage) from the db/src/lib.rs into dedicated file db/src/changes_tries_storage.rs. Now it holds reference to the cache where configuration changes are stored;
  • (extracted to separate PR) state_machine methods now accept new struct ChangesTrieState instead of simply ChangesTrieStorage. In addition to storage reference, it holds: active changes trie configuration and zero block of this configuration (i.e. parent block of the block where first changes trie is built using this configuration). This is required to build changes trie at current block;
  • Client::key_changes and Client::key_changes_proof are updated to support ranges where CT configuration has been changed. Client::max_key_changes_range isn't updated yet, because it tightly related to pruning (see below);
  • (extracted to separate PR) SurfaceIterator is extracted to dedicated state_machine/src/changes_trie/surface_iterator.rs;
  • (extracted to separate PR) when changes trie configuration change is detected inside the build.rs, max level digest is created. It may be a skewed digest (the digest that is created ahead of schedule), or a normal digest.
  • key changes functions are updated to support skewed digests.

Changes tries are pruned similarly to what has been before - i.e. there's a block for which we guarantee that all changes tries (if created) after this block are valid (i.e. if we prune some trie, then the digest trie that points to it, is also pruned) and all changes tries before this block should not be accessed. Since we now have tries that are built using different configurations, we store this block in special metadata entry (because it isn't that easy to calculate it).

@svyatonik svyatonik added A3-in_progress Pull request is in progress. No review needed at this stage. M4-core labels Jul 25, 2019
@svyatonik
Copy link
Contributor Author

Duplicating IMPORTANT NOTE here: please only merge this PR if you sure that we do not need code for db upgrade here. I haven't did that - not sure if it is required. If merged as-is, this PR will break all existing databases => nodes will be unable to start with old databases. Please respond if you think we should start database versioning stuff.

@svyatonik svyatonik added A0-please_review Pull request needs code review. and removed A5-grumble labels Jan 9, 2020
@gavofyork
Copy link
Member

will this affect typical deployments (like kusama nodes)?

@svyatonik
Copy link
Contributor Author

If you're talking about breaking db - yes, this will affect all nodes. So you can't just copy binary over previous - we need to erase db && then (if there's another source of blocks on the network), it'll sync from the scratch.

@Demi-Marie
Copy link
Contributor

@svyatonik will nodes with this patch be able to sync from nodes without it?

@svyatonik
Copy link
Contributor Author

@demimarie-parity Yes, it changes nothing in how blocks on existing chains are processed.

@gavofyork
Copy link
Member

If you're talking about breaking db - yes, this will affect all nodes. So you can't just copy binary over previous - we need to erase db && then (if there's another source of blocks on the network), it'll sync from the scratch.

how much trouble would a DB migration be? we have a semi-production network of ~500 nodes out there and i don't really want to have to force them all to resync on the next minor revision..

@svyatonik
Copy link
Contributor Author

Re upgrade performance - I'm not sure yet. Re code - shouldn't be a big issue, I just thought that maybe we do not need it yet. We have changed db format several times, but there never been any upgrade code. But it was probably been before Kusama. So I'll add db upgrade here. Will try to keep it localized in single commit to make it easy reviewable.

@svyatonik
Copy link
Contributor Author

(or perhaps will open additional PR to the changes_tries_update_configuration branch - I believe set of reviewers should be different)

@gavofyork
Copy link
Member

this one shouldn't be merged without migration code, so probably best if it's a single commit on top of this.

@svyatonik
Copy link
Contributor Author

This commit implements database upgrade. The only upgrade now is from v0 to v1, which: (1) adds new column to the database and (2) initializes changes tries configuration cache (with None configuration).

I also forgot that I've promised to sync Kusama with these changes in some of comments above => will temporary switch to inprogress and revert will try to sync.

@svyatonik svyatonik added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Jan 14, 2020
@cheme
Copy link
Contributor

cheme commented Jan 15, 2020

Had a quick peak on the db commit. Things looks good (considering meta column is the same between light and full).
Makes me think about a possible change (not sure it is needed but anyway it can be added later):

  • here new client instance will apply all upgrade in sequence. We could also have kvdb-rocksdb with a new function to check if db already exists and add a latest version file on creation (by default only run upgrade on client with existing db).
  • previous point does make sense but it is also a possible cause for error (if upgrade code does not match initialization code), so it is only interesting if we can manage db version per client (eg at this point polkadot version 0 corresponding to kusama version 1). So the versioning and upgrade code would be define per project, probably putting thing being an upgradedb trait and including the impl in cli initialisation parameters.

@svyatonik
Copy link
Contributor Author

Thanks for taking a time to review! I agree that function like db.exists() would be much better than the current approach. Probably we could even use directory.exists() here. But that could be added on next upgrade. For now I would prefer to stick with current version. The reason is simple: (1) now we have only one upgrade which works in both cases (when db_version exists and contains "0" and when it does not exists) (2) I hope most of nodes will upgrade to v1 (and db_version will be created with "1") until next upgrade (v2) will be required. This way we could minimize number of nodes that are upgrading from "either v0, or v2" to "v2" - in most cases we'll either have "v1", or "v2" (i.e. there'll (almost) be no nodes without db_version file).

As for the second - I'm not sure we are exposing anything internal from database to clients. The only exception is aux storage, but it already has its own upgrade path - see e.g. upgrade_from_* here. So I don't think there's a need for providing different upgrade scenarios for different projects. I'd say projects should use dedicated storage for their own needs.

@svyatonik
Copy link
Contributor Author

So I've finally synced both Kusama clients (full/light) with this PR - everything seems fine for me.

@svyatonik svyatonik 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 16, 2020
@gavofyork gavofyork added A8-looksgood and removed A0-please_review Pull request needs code review. A1-onice labels Jan 16, 2020
@gavofyork gavofyork merged commit 8e98643 into master Jan 16, 2020
@gavofyork gavofyork deleted the changes_tries_update_configuration branch January 16, 2020 16:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants