WIP: Downgrade schema v5 to v4

[winksaville]

No description provided.

[winksaville]

@michaelsproul, initial stab at downgrade, still need to add the db downgrade cli command, but would appreciate it if you could spare a few minutes to catch obvious errors.

[michaelsproul]

Looking good!

Just one suggestion

Thanks for taking the time to implement this 😊

[winksaville]

@michaelsproul , I added commit "Initial stab at a database_manager Ui", when you have time please take look.

Questions:

• Is the sub-command database_manager OK?
• Is open_as_is the right direction for "opening" the database?
• What do you advise is the proper way to initialize the parameters to open_as_is?

Txs!

[michaelsproul]

Is the sub-command database_manager OK?

Yep! We can alias it easily enough at the CLI to lighthouse db if we want

Is open_as_is the right direction for "opening" the database?

Yeah, although you could keep open the same and just pass a no-op schema upgrade like |_, _, _| Ok(()).

What do you advise is the proper way to initialize the parameters to open_as_is?

This one's a bit trickier. Currently the DB is only opened from the BN subcommand, so its config file parsing is embedded into the BN config, e.g. here:

lighthouse/beacon_node/client/src/config.rs

Lines 51 to 54 in 7c88f58

	/// Name of the directory inside the data directory where the main "hot" DB is located.
	pub db_name: String,
	/// Path where the freezer database will be located.
	pub freezer_db_path: Option<PathBuf>,

To avoid getting entangled in that I think we could have 3 separate flags for the database_manager:

• --datadir for setting the datadir. The best utility for determining this is get_data_dir in the BN directory, so you may need to make database_manager depend on beacon_node. Set the hot DB path to $datadir/beacon/chain_db.
• --freezer-dir for setting the freezer DB. If not provided it should default to $datadir/beacon/freezer_db.
• --slots-per-restore-point, default 2048.

The StoreConfig can be constructed with default values for its other fields (they're not really relevant):

let store_config = StoreConfig {
    slots_per_restore_point, /* from CLI or 2048 */
    ..StoreConfig::default()
};

Finally, for the ChainSpec, that's best sourced from a network configuration. So we should have a --network flag, which can be parsed by get_eth2_network_config (also in beacon_node). Given a NetworkConfig we can get the ChainSpec using chain_spec:

lighthouse/common/eth2_network_config/src/lib.rs

Lines 88 to 96 in 7c88f58

	/// Construct a consolidated `ChainSpec` from the YAML config.
	pub fn chain_spec<E: EthSpec>(&self) -> Result<ChainSpec, String> {
	ChainSpec::from_config::<E>(&self.config).ok_or_else(|| {
	format!(
	"YAML configuration incompatible with spec constants for {}",
	E::spec_name()
	)
	})
	}

Sorry, I know that's a lot! Hope that helps!

[winksaville]

@michaelsproul have I done something wrong, why is schema_version None?

wink@3900x:~/prgs/ethereum/myrepos/lighthouse (Downgrade-schema-v5-to-v4)
$ cargo run --bin lighthouse db -v
    Finished dev [unoptimized + debuginfo] target(s) in 0.20s
     Running `target/debug/lighthouse db -v`
Running database manager for mainnet network
database version: None

[michaelsproul]

The code looks OK to me. Are you running this against a database that already exists?

[winksaville]

No, it's a new database in a the tempdir.

[michaelsproul]

That makes sense then, open_as_is never writes the schema version because that happens when the migration runs.

[winksaville]

@michaelsproul I've got a solution, open_as_is now returns db and schema version. Does that work for you?

[winksaville]

@michaelsproul, started working on downgrade and I can't figure out how to call migrate_schema. needs "Witness" magic which is way beyond my rust knowledge :)

[michaelsproul]

Ah yeah this is a little bit clunky for our purposes but we can work around it by providing (arbitrary) types for the SlotClock and Eth1Chain like this:

migrate_schema::<Witness<SystemTimeSlotClock, CachingEth1Backend<E>, E, LevelDB<E>, LevelDB<E>>(..)

Type inference should be able to work out the rest if we just hint the first two:

migrate_schema::<Witness<SystemTimeSlotClock, CachingEth1Backend<E>, _, _, _>(..)

Witness, SystemTimeSlotClock and CachingEth1Backend will also need to be imported from their respective crates. Let us know if you have issues

[winksaville]

@michaelsproul , it is now "working", TYVM!

Could you provide a link or background information on what is going on and why this works?

I saw that @paulhauner added this code in #542 and the indirect call in open was so there wouldn't be a circular code. But I didn't see anything explaining the need for a Witness other than in the code we see:

/// An empty struct used to "witness" all the `BeaconChainTypes` traits. It has no user-facing
/// functionality and only exists to satisfy the type system.
pub struct Witness<TSlotClock, TEth1Backend, TEthSpec, THotStore, TColdStore>(
    PhantomData<(TSlotClock, TEth1Backend, TEthSpec, THotStore, TColdStore)>,
);

Which doesn't help me with why we have to go to a lot of trouble to convince the type system this call is Ok.

[michaelsproul]

It's all in aid of the BeaconChainBuilder, which allows us to build BeaconChains instantiated with various concrete types for the different components (the slot clock, the eth1 backend, the database, and most importantly the EthSpec which chooses between minimal and mainnet presets).

In the builder's case we don't need to specify the witness type manually because it can be inferred by the compiler when the builder methods are called. E.g. the testing_slot_clock method is specialised to TSlotClock = TestingSlotClock so if the compiler sees a builder.testing_slot_clock(..) call it can infer that the slot clock type is TestingSlotClock.

The reason migrate_schema is a bit unwieldy is because it was moved from within the store crate (where it didn't care about witnesses) to the beacon_chain crate, in order to get access to the ValidatorPubkeyCache. For better or for worse, the ValidatorPubkeyCache is parametrised by <T: BeaconChainTypes>, so that ripples up to the top-level. Until now migrate_schema was only called from the builder/beacon chain where a witness was already known.

I think the manual type hint is an OK solution for now, as it avoids us having to refactor (unrelated) code in the ValidatorPubkeyCache and gives migrate_schema the flexibility to interact with other types parametrised by a witness in future.

[winksaville]

I agree it's.OK for now, but it's the first time that neither me, rust-analyzer or rustc could figure out the right type hints. Which really makes me feel something isn't right. Probably with more rust experience I could have figured it out, but if a refactoring would make it more obvious I'd say it would be worth it. But, again, I agree not in this PR.

[michaelsproul]

Thanks for the great work on this @winksaville. I've implemented an updated version of the database_manager with some more features in a new PR (#3129) and we'll have some DB downgrades built atop that coming soon!

I'll close this PR in favour of the new one