[chore] Implement Stateful for all Chips #1199
Conversation
Commit: baf339a |
| impl<const NUM_BITS: usize> Stateful<Vec<u8>> for SharedBitwiseOperationLookupChip<NUM_BITS> { | ||
| fn load_state(&mut self, state: Vec<u8>) { | ||
| // AtomicU32 can be deserialized as u32 | ||
| let (count_range, count_xor): (Vec<u32>, Vec<u32>) = bitcode::deserialize(&state).unwrap(); |
There was a problem hiding this comment.
maybe we should resolve the choice of bitcode vs bincode before/during this PR?
| @@ -100,6 +104,9 @@ pub struct RangeTupleCheckerChip<const N: usize> { | |||
| count: Vec<Arc<AtomicU32>>, | |||
| } | |||
|
|
|||
| #[derive(Debug, Clone)] | |||
| pub struct SharedRangeTupleCheckerChip<const N: usize>(Arc<RangeTupleCheckerChip<N>>); | |||
There was a problem hiding this comment.
did you make a newtype for protected type purposes or because of some practical reason around traits?
| @@ -50,7 +50,7 @@ impl Sha256Air { | |||
| trace_width: usize, | |||
| trace_start_col: usize, | |||
| input: &[u32; SHA256_BLOCK_WORDS], | |||
| bitwise_lookup_chip: &BitwiseOperationLookupChip<8>, | |||
| bitwise_lookup_chip: SharedBitwiseOperationLookupChip<8>, | |||
There was a problem hiding this comment.
why are we doing this...? it doesn't really matter, but cloning Arc is worse than &.
You could also make SharedBitwiseOperationLookupChip implement Borrow I think?
There was a problem hiding this comment.
Wasn't clear why we needed Stateful...Chip: does rust prevent you from implementing Stateful on Arc<T> even when you own T?
The fact that you need to specify bitcode explicitly in every load_state seems bad. Let's discuss if we should update Stateful to either pass in a deserializer / serializer (openvm-org/stark-backend#20 (comment)) or you could do impl Stateful<(Deserializer, Vec<u8>)> for ... maybe?
nit: You can implement T on StatefulXChip to prevent some clone.
nyunyunyunyu
force-pushed
the
chore/impl-stateful
branch
from
baf339a to
dc39d3a
Compare
nyunyunyunyu
force-pushed
the
chore/impl-stateful
branch
from
dc39d3a to
bd068a9
Compare
No description provided.