[Merged by Bors] - Apply hotfix for inconsistent head

[paulhauner]

Issue Addressed

• Resolves Beacon node can not sync after a hard reset. #1616

Proposed Changes

If we look at the function which persists fork choice and the canonical head to disk:

lighthouse/beacon_node/beacon_chain/src/beacon_chain.rs

Lines 234 to 280 in 1db8daa

	/// Persists the core `BeaconChain` components (including the head block) and the fork choice.
	///
	/// ## Notes:
	///
	/// In this function we first obtain the head, persist fork choice, then persist the head. We
	/// do it in this order to ensure that the persisted head is always from a time prior to fork
	/// choice.
	///
	/// We want to ensure that the head never out dates the fork choice to avoid having references
	/// to blocks that do not exist in fork choice.
	pub fn persist_head_and_fork_choice(&self) -> Result<(), Error> {
	let canonical_head_block_root = self
	.canonical_head
	.try_read_for(HEAD_LOCK_TIMEOUT)
	.ok_or_else(|| Error::CanonicalHeadLockTimeout)?
	.beacon_block_root;
	let persisted_head = PersistedBeaconChain {
	canonical_head_block_root,
	genesis_block_root: self.genesis_block_root,
	ssz_head_tracker: self.head_tracker.to_ssz_container(),
	};
	let fork_choice_timer = metrics::start_timer(&metrics::PERSIST_FORK_CHOICE);
	let fork_choice = self.fork_choice.read();
	self.store.put_item(
	&Hash256::from_slice(&FORK_CHOICE_DB_KEY),
	&PersistedForkChoice {
	fork_choice: fork_choice.to_persisted(),
	fork_choice_store: fork_choice.fc_store().to_persisted(),
	},
	)?;
	drop(fork_choice);
	metrics::stop_timer(fork_choice_timer);
	let head_timer = metrics::start_timer(&metrics::PERSIST_HEAD);
	self.store
	.put_item(&Hash256::from_slice(&BEACON_CHAIN_DB_KEY), &persisted_head)?;
	metrics::stop_timer(head_timer);
	Ok(())
	}

There is a race-condition which might cause the canonical head and fork choice values to be out-of-sync.

I believe this is the cause of #1616. I managed to recreate the issue and produce a database that was unable to sync under the master branch but able to sync with this branch.

These new changes solve the issue by ignoring the persisted canonical_head_block_root value and instead getting fork choice to generate it. This ensures that the canonical head is in-sync with fork choice.

Additional Info

This is hotfix method that leaves some crusty code hanging around. Once this PR is merged (to satisfy the v0.2.x users) we should later update and merge #1638 so we can have a clean fix for the v0.3.x versions.

[michaelsproul]

Looks good overall, just one change needed IMO.

It will be good to revisit this for v0.3.0, and again as part of our work to resolve concurrency issues like #1557

[michaelsproul]

I don't think we should conjure a SystemTimeSlotClock from thin air here, as I think it will wreak havoc on the tests that use the testing slot clock. Instead I think we should use self.slot_clock, and patch up the existing couple of places where resume_from_db is called on the builder before slot_clock or testing_slot_clock (e.g. in beacon_chain/src/test_utils.rs)

[paulhauner]

Hmm yeah agree it's going to be bad for testing slot clock. The problem is that there's a chicken-and-egg problem; we need to know the genesis time (i.e., the genesis state) start the slot clock and we need the slot clock in order to learn the canonical head.

Presently those two actions are lumped together in one function and not generic across the slot clocks... I'll see what I can do...

[paulhauner]

I think I managed a decent solution (without changing the schema) that solves this. Let me know what you think :)

[michaelsproul]

Yep! Addresses the chicken-and-egg problem nicely!

[michaelsproul]

Looks good.

Approved conditional on the simulator being fixed

[paulhauner]

@michaelsproul please let me know if you're good with 5bb065b :)

[michaelsproul]

All good 👍

[paulhauner]

bors r+

[bors]

Pull request successfully merged into master.

Build succeeded:

• arbitrary-check
• cargo-audit
• cargo-fmt
• cargo-udeps
• check-benchmarks
• clippy
• debug-tests-ubuntu
• dockerfile-ubuntu
• ef-tests-ubuntu
• eth1-simulator-ubuntu
• no-eth1-simulator-ubuntu
• release-tests-and-install-macos
• release-tests-ubuntu
• state-transition-vectors-ubuntu