Better logging when backfilling ancient blocks fail

[dvdplm]

Handle databases with holes

After snapshots have been downloaded and imported into the new DB we try to salvage existing blocks from the current DB before switching to the new DB ( this happens in migrate_blocks()).

The current code assumes that the blocks in the current DB form a chain all the way down to the genesis block (i.e. they are "ancient blocks"), which is not necessarily true. There are situations where the current DB have some amount of blocks in it that do not stretch all the way back to genesis.

I think the way this situation can occur is:

• start a node with an empty DB
• a snapshot is downloaded and the chain populated with [n..m] blocks, where m is close to the tip and n = m - 10k blocks
• maybe the node stays up and imports block and is generally fine
• the node is stopped and naturally falls out of sync
• the node is started again and as it's too far off the tip, a new snapshot is downloaded
• the previous db will have blocks [i..j] where i != 0 (it'll be 10k blocks before wherever the tip was in the FIRST snapshot) and j is farther behind the current tip - 10k

Before this PR the final step of the second snapshot will fail with anUnlinkedAncientBlockChain error because the previous DB's blocks do not stretch all the way back to 0.

With this PR, the attempt to salvage the old blocks is simply abbandoned and the new snapshot is allowed to complete, effectively tossing away the content of the previous DB.

I suspect that it might be possible to do better than that, but best do this step by step.

The diff is rather noisy but the key change of the PR starts here.

ref. #10793

Print total blocks imported, closes #10792

[tomusdrw]

Looks sane, modulo TODOs and the deadlock.

[tomusdrw]

I'd keep the comment, but drop the TODO: prefix :)

[tomusdrw]

We should actually always deadlock - because we already have self.restoration.lock() acquired in line 720. parking_lot locks are not re-entrant.

Instead of calling abort_restore() we should rather use *restoration = None here. and maybe add a trace line if you care about it.
Alternatively we could have abort_restore take the locked restoration. IMHO if we do that for some methods already we should do that for all of them.

[dvdplm]

We should actually always deadlock

Well, for some reason we do not. It's pretty rare, I've seen it twice.

Instead of calling abort_restore() we should rather use *restoration = None

That's what abort_restore() does though: here. Why is it better to do it here?

[tomusdrw]

Why is it better to do it here?

To avoid locking twice?

As I said the second ption should be abort_restore_with_restoration(locked_res: &mut Option<Restoration>) so we can keep the logic there, but just pass the existing lock.

[dvdplm]

So the reason I removed *restoration = None from here and replaced it with a call to abort_restore() was to collect all shutdown actions in one place. Not that abort_restore() is doing very much atm, but I still think it's good to have a single point of abortion.
is locking twice really a concern though: this is an error handler and I don't think we care much about performance?

abort_restore_with_restoration(locked_res…)

The restoration is a member of the struct here, so I'm not sure what the point of passing it as a param tbh. Now you got me confused!

[tomusdrw]

Since parking_lot::Mutex is not re-entrant locking it twice in the same thread can (or does) lead to a deadlock. So the call like that:

fn some(&self) {
  let mut restorating = self.restoration.lock();
  ....
  self.abort_restore();
}

fn abort_restore(&self) {
  *self.restoration.lock() = None
}

is guaranteed to deadlock, I thought always, but I suspect it might be just random.

In the same file I suppose we already had deadlock issues, so the with_restoration pattern emerged, where instead of locking resources locally in a particular function you get passed a (mutable) reference to the locked resource:

let mut restoration = self.restoration.lock();
self.feed_chunk_with_restoration(&mut restoration)

I propose to use exactly the same patter for abort_restore - I'm totally fine grouping all the shutdown actions there, but to prevent potential double-locking and potential deadlocks we can pass the locked resource, so the first example becomes:

fn some(&self) {
  let mut restoration = self.restoration.lock();
  ....
  self.abort_restore(&mut restoration);
}

fn abort_restore(&self, res: &mut Option<Restoration>) {
  *res = None
}

[dvdplm]

I see, thank you for explaining.

Is this an option?

		let r = {
			let mut restoration = self.restoration.lock();
			self.feed_chunk_with_restoration(&mut restoration, hash, chunk, is_state)
		};
		match r {
			…
			Err(e) => {
				…
				self.abort_restore();
				…
			}
		}

[ordian]

According to docs:

Attempts to lock a mutex in the thread which already holds the lock will result in a deadlock.

So I'm not sure either why it didn't always deadlock. The proposed fix looks good.

[ordian]

According to docs:

Attempts to lock a mutex in the thread which already holds the lock will result in a deadlock.

So I'm not sure either why it didn't always deadlock. The proposed fix looks good.

[ascjones]

Minor nits, otherwise LGTM.

[ascjones]

I think best_ancient_block being None can either mean: "no ancient blocks imported" or "all ancient blocks imported (no gaps)". So comment is misleading, although in the case when first_block is Some then the former is the true.

[dvdplm]

Right, I should move the parens I think.