beacon_node/store: potential deadlock in hot_cold_store.rs

[Chain-Fox]

Description

Following #1241, for parking_lot::RwLock, “attempts to recursively acquire a read lock within a single thread may result in a deadlock.” I found two pairs of locks that may have potential deadlocks through static analysis, both in beacon_node/store/src/hot_cold_store.rs.

lighthouse/beacon_node/store/src/hot_cold_store.rs

Lines 668 to 674 in 46920a8

	fn load_cold_intermediate_state(&self, slot: Slot) -> Result<BeaconState<E>, Error> {
	// 1. Load the restore points either side of the intermediate state.
	let low_restore_point_idx = slot.as_u64() / self.config.slots_per_restore_point;
	let high_restore_point_idx = low_restore_point_idx + 1;
	// Acquire the read lock, so that the split can't change while this is happening.
	let split = self.split.read();

The first pair:
In fn load_cold_intermediate_state():
The first lock split.read() is at L674.
Following the path L682->L306 (or L682->L318->L625->L638->L682->L306),
the second lock split.read() is in get_split_slot() at L818.

The second pair:
In fn load_cold_intermediate_state():
The lock split.read() at L674 may deadlock with itself.
The path is L682->L318->L625->L638.

Version

stable

Present Behaviour

None

Expected Behaviour

None

Steps to resolve

Maybe pass the lock result down to the callee to avoid the second lock? Or use read_recursive() instead of read() to make it reentrant.

[michaelsproul]

This is awesome, thank you! I'll look at implementing fixes this week, and will read some more about your tool to see if it's something we could incorporate into our CI process