Cleanup Comments & Fix get_pow_block_hash_at_ttd()

[ethDreamer]

Issue Addressed

The algorithm for finding the TTD block hash will not terminate until it reaches the genesis block of the chain. Given that we can't cache the entire eth1 chain, this will cause us to download the whole thing over and over every time this function is called.

Proposed Changes

Instead of going all the way back to the genesis block, just stop the algorithm as soon as we reach a block with terminal difficulty < TOTAL_TERMINAL_DIFFICULTY.

[ethDreamer]

We talked about submitting a PR to clarify the current description of this function in the spec.

def get_pow_block_at_terminal_total_difficulty(pow_chain: Dict[Hash32, PowBlock]) -> Optional[PowBlock]:
    # `pow_chain` abstractly represents all blocks in the PoW chain
    for block in pow_chain.values():
        block_reached_ttd = block.total_difficulty >= TERMINAL_TOTAL_DIFFICULTY
        if block_reached_ttd:
            # If genesis block, no parent exists so reaching TTD alone qualifies as valid terminal block
            if block.parent_hash == Hash32():
                return block
            parent = pow_chain[block.parent_hash]
            parent_reached_ttd = parent.total_difficulty >= TERMINAL_TOTAL_DIFFICULTY
            if not parent_reached_ttd:
                return block

    return None

This could be due to inexperience in reading the spec, but when I look at this bit of code, I looks like the loop is just an abstraction to illustrate what this function is supposed to do. I get the feeling this function was originally expressed formally as:

{ B | B ∈ pow_chain ; B.totalDifficulty ≥ TTD ; parent(B).totalDifficulty < TTD }

or in English

Return the block in the POW chain with totalDifficulty ≥ TTD and parent's totalDifficulty < TTD.

And that statement was just translated into this code. Note the loop in this code doesn't imply an ordering to the iteration. It's a function that will return the terminal block hash if it exists, otherwise None.

It doesn't seem (to me) like this code is supposed to be implemented literally. It seems like the implementers are supposed to substitute their own efficient implementation of this function.

If we do submit a clarification to this function, we would need to imply an ordering to the iteration so it was clear we were starting from the highest block. That implies we'll need to define an abstract function (in the spec) that returns the latest block and then we need to proceed through the iteration by getting the parent block.

I'm wondering if that's too much detail for the spec (which is perhaps why that wasn't done)? I'm not exactly sure how that line is drawn though. I'm interested to hear what @paulhauner would have to say about this.

[paulhauner]

I'm wondering if that's too much detail for the spec (which is perhaps why that wasn't done)? I'm not exactly sure how that line is drawn though. I'm interested to hear what @paulhauner would have to say about this.

Good point, the .values() of a Dict are not sorted. So, even with your change we match the spec (assuming that total_difficulty is (non-strictly) increasing).

I think the intention of the spec is to have total_difficulty increasing. It's interesting, though, that with an arbitrary total_difficulty, the spec says "pick any random ttd-threshold-crossing block".

Perhaps it's still worth raising an issue on the spec to communicate that there's an unstated assumption that, when violated, leads to non-deterministic behaviour? It's somewhat nit-picky, but interesting still 🤷

EDIT: Discord discussion: https://discord.com/channels/595666850260713488/892088344438255616/915042353671192587

[paulhauner]

Looks good, great stuff!

I don't think the discussion about the spec needs to block this PR, so lets merge it 🚀