Plafer cleanup block hash table

[plafer]

Makes the block hash table aux column builder more readable. Also adds more documentation.

[Al-Kindi-0]

Looks great, thank you!
Just a couple of minor nits from my side

[Al-Kindi-0]

nit: should we just use parent_id as in the docs? It was a bit easier for me to understand the logic of this virtual table using this name.

[plafer]

Agreed, switched back to parent_block_id

[bobbinth]

Looks great! Thank you!

One thing I am wondering is whether we should create a struct for the block hash table row. It could look something like this:

struct BlockHashTableRow {
    parent_block_id: Felt,
    child_block_hash: Word,
    is_first_child: bool,
    is_loop_body: bool,
}

Then, we could add various constructor methods to it:

impl BlockHashTableRow {
    pub fn from_end(trace: &MainTrace, row: usize) -> Self {
        todo!()
    }

    pub fn from_join(trace: &MainTrace, row: usize, is_first_child: bool) -> Self {
        todo!()
    }

    ...
}

And then we could add a to_value() method to reduce a row to a single value. It could look like this:

impl BlockHashTableRow {
    pub fn to_value<E: FieldElement<BaseField = Felt>>(&self, alphas: &[E]) -> E {
        todo!()
    }
}

Then, when using these, we could do something like:

SPLIT => BlockHashTableRow::from_split(main_trace, i).to_value(alphas),

I think this structure would make it a bit easier to follow - though, I think the improvement would not be all that big over what you already have. So, if you'd rather keep it as is, that's fine too.

[plafer]

I agree that a BlockHashTableRow would be a nice abstraction (we could modify the one that's already in the tests).

The only place where it breaks a little bit is with JOIN, where 2 rows are created from the opcode, so a BlockHashTableRow::from_join() wouldn't really work. So maybe the current version is better, since it doesn't assume that one opcode maps to one row?

[bobbinth]

The only place where it breaks a little bit is with JOIN, where 2 rows are created from the opcode, so a BlockHashTableRow::from_join() wouldn't really work. So maybe the current version is better, since it doesn't assume that one opcode maps to one row?

I think it could still works if the constructor has the following signature:

pub fn from_join(trace: &MainTrace, row: usize, is_first_child: bool) -> Self {
    todo!()
}

Then, this function can be called twice to generate two rows (once with is_first_child = true, and the other time with is_first_child = false). Then, the two rows would be reduced to values and multiplied.

[plafer]

@bobbinth implementation now uses BlockHashTableRow. I also renamed BlockHashTableRow::to_value() to collapse().

[plafer]

Note: clippy failures are due to a new version of nightly rust, and there are many errors, so I think we should do it in another PR instead

[bobbinth]

Looks good! Thank you! I left a couple of small comments inline. Once these are addressed, we can merge.