0xPolygonMiden / miden-vm

STARK-based virtual machine
MIT License
611 stars 148 forks source link

Plafer cleanup block hash table #1333

Closed plafer closed 1 month ago

plafer commented 1 month ago

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

plafer commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

plafer commented 1 month ago

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