Fix BaseHeaderChainSyncer not following freshly created blocks

[cburgdorf]

What was wrong?

As explained in #1788 our BaseHeaderChainSyncer currently fails to follow new blocks. It catches up to the tip and then stalls forever.

How was it fixed?

It looks like this is a case of https://github.com/ethereum/snake-charmers-tactical-manual/pull/70/files (cc @gsalgado ) and I was able to get it to work with this fix.

However, this is breaking one of our tests and I'm beginning to think the proper fix would be to break out of this yield await when the SkeletonSyncer was canceled.

trinity/trinity/sync/common/headers.py

Lines 119 to 122 in 615d6bc

	async def next_skeleton_segment(self) -> AsyncIterator[Tuple[BlockHeader, ...]]:
	while self.manager.is_running:
	yield await self._fetched_headers.get()
	self._fetched_headers.task_done()

Trying to figure out how to do that...

To-Do

• Clean up commit history

• no release notes entry because the bug did not exist in the past release

Cute Animal Picture

[gsalgado]

This doesn't look like the anti-pattern I described in ethereum/snake-charmers-tactical-manual#70. I haven't looked into how _full_skeleton_sync() is implemented, but I imagine it will crash somehow if syncer terminates

[cburgdorf]

Ah ok. I haven't fully groked yet why the code broke (could be a fall out from the migration to async-service).

What happens is that this loop of starting new skeleton syncs doesn't work anymore.

trinity/trinity/sync/common/headers.py

Lines 919 to 926 in 615d6bc

	async for peer in self._tip_monitor.wait_tip_info():
	try:
	await self._validate_peer_is_ahead(peer)
	except _PeerBehind:
	self.logger.debug("At or behind peer %s, skipping skeleton sync", peer)
	else:
	async with self._get_skeleton_syncer(peer) as syncer:
	await self._full_skeleton_sync(syncer)

Whenever the SkeletonSyncer cancels itself, we continue to hang on self._full_skeleton_sync(syncer) forever. The changes in this PR fixes it but it's probably not the right fix (hence the warnings). But I have an another idea to try out :)

[cburgdorf]

Ok, warnings are gone when I add task.cancel() but now it fails one of our tests 🙈 Investigating...

[cburgdorf]

If we can not use @external_asyncio_api directly on async def next_skeleton_segment then this feels like abusing the decorator I guess

[carver]

Yeah, agreed. It doesn't take too much work in async-service to expose the new API though, AFAICT.

[carver]

See ethereum/async-service#70