Harden against IO failures using DB atomicity

[adaszko]

Issue Addressed

Partially addresses #692

[paulhauner]

I'm seeing a test failure on CI that looks like it could be related to changes in this PR:

test finalizes_after_resuming_from_db ... FAILED

failures:

---- finalizes_after_resuming_from_db stdout ----
thread 'finalizes_after_resuming_from_db' panicked at 'should not error during attestation processing: UnknownHeadBlock { beacon_block_root: 0x230dcb3f606e112424eb6c2c04d1f59719dfcde3d2c6fd349008685d2dce2e6e }', /home/runner/work/lighthouse/lighthouse/beacon_node/beacon_chain/src/test_utils.rs:577:21
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I'll flag this as WIP and hold off on a full review until we've sorted that out :)

[michaelsproul]

State storage needs to take into account the HotStateSummary here, as in the non-atomic case:

lighthouse/beacon_node/store/src/hot_cold_store.rs

Lines 372 to 398 in d79e079

	/// Store a post-finalization state efficiently in the hot database.
	///
	/// On an epoch boundary, store a full state. On an intermediate slot, store
	/// just a backpointer to the nearest epoch boundary.
	pub fn store_hot_state(
	&self,
	state_root: &Hash256,
	state: &BeaconState<E>,
	) -> Result<(), Error> {
	// On the epoch boundary, store the full state.
	if state.slot % E::slots_per_epoch() == 0 {
	trace!(
	self.log,
	"Storing full state on epoch boundary";
	"slot" => state.slot.as_u64(),
	"state_root" => format!("{:?}", state_root)
	);
	store_full_state(&self.hot_db, state_root, &state)?;
	}
	// Store a summary of the state.
	// We store one even for the epoch boundary states, as we may need their slots
	// when doing a look up by state root.
	self.put_state_summary(state_root, HotStateSummary::new(state_root, state)?)?;
	Ok(())
	}

The compositionality of the HotColdDB consisting of two sub-stores (hot_db and cold_db) with the same API is a lie, and is another reason why the Store trait should be refactored IMO. I don't think Store should be implemented for LevelDB, because we never use just one LevelDB database as the store any more, and it creates awkwardness when a Store API has to be implemented on the underlying store with assumptions about that API only getting used for the hot or cold DB (in this case we're assuming hot). I think it makes sense to put all this logic in the parent impl of do_atomically (i.e. HotColdDB), and stub out the impl for LevelDB so that it returns an error.

[michaelsproul]

I'm on board with using a reference here, but with state summaries existing it might make sense to mix in the two variants from StateBatch:

// Allow owned states for the storage of intermediate states in block processing (uncommon)
PutState(BeaconStateHash, Cow<'a, BeaconState<E>>),
PutStateSummary(BeaconStateHash, HotStateSummary),

See https://github.com/sigp/lighthouse/blob/master/beacon_node/store/src/state_batch.rs

StateBatch could then be deleted of course.

[adaszko]

I'm seeing a test failure on CI that looks like it could be related to changes in this PR:

test finalizes_after_resuming_from_db ... FAILED

failures:

---- finalizes_after_resuming_from_db stdout ----
thread 'finalizes_after_resuming_from_db' panicked at 'should not error during attestation processing: UnknownHeadBlock { beacon_block_root: 0x230dcb3f606e112424eb6c2c04d1f59719dfcde3d2c6fd349008685d2dce2e6e }', /home/runner/work/lighthouse/lighthouse/beacon_node/beacon_chain/src/test_utils.rs:577:21
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I'll flag this as WIP and hold off on a full review until we've sorted that out :)

That's related to my GitHub Actions test jobs not being triggered automatically.

I thought this PR is going to be a quick improvement but I see now it's way deeper and gonna involve more time.

[paulhauner]

That's related to my GitHub Actions test jobs not being triggered automatically.

Oh so this is an old test result?

EDIT: Oh I remember this now. Hopefully it changes now you're a contributor.

[michaelsproul]

That's related to my GitHub Actions test jobs not being triggered automatically.

I think Github Actions were working fine here, right? CI ran against the latest commit from the atomicity branch (7bf1629) and failed because of the reasons raised in my review.

[adaszko]

That's related to my GitHub Actions test jobs not being triggered automatically.

I think Github Actions were working fine here, right? CI ran against the latest commit from the atomicity branch (7bf1629) and failed because of the reasons raised in my review.

On the pull request, yes. On my fork, no.