-
Notifications
You must be signed in to change notification settings - Fork 123
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good! Just some minor feedback, not sure if phase2
Performance wise this will do all constraints on every row and will do more rotations than the previous version, but seems like that shouldn't really impact performance in any significant way so I think a good tradeoff for simplicity.
keccaked_pi | ||
.iter() | ||
.take(16) | ||
.fold(F::ZERO, |acc: F, byte| { | ||
acc * F::from(BYTE_POW_BASE) + F::from(*byte as u64) | ||
}), | ||
keccaked_pi | ||
.iter() | ||
.skip(16) | ||
.fold(F::ZERO, |acc: F, byte| { | ||
acc * F::from(BYTE_POW_BASE) + F::from(*byte as u64) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use decode::value
(or rlc::value
, though decode makes it more clear what is being done).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is from John's old version haha
"PI acc constraints", | ||
|meta| { | ||
circuit!([meta, cb], { | ||
for (n, b, acc) in [parent_hash.clone() , block_hash.clone()] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
n
, b
and even acc
not the most self explaining names, especially because the values come from a tuple where they are also not named.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will change :)
0, | ||
DEFAULT_LEN, | ||
); | ||
let mut cb: ConstraintBuilder<F, PiCellType> = ConstraintBuilder::new(4, Some(cm.clone()), Some(evm_word.expr())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this set to 4
instead of 5
because the q_enable
selector is added afterwards and so doesn't get counted while adding the constraints? Seems like another good reason not to do it I think, more edge cases to take into account! (no problem of doing it like this for this temporary version though).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh yea! you're right this is such an important point I didn't think of. I'll revert the change to cb.build_constraints
then.
We have tested a few setups, the original one is single column, which leads to 1 column, >=224 rotations(1 x (32 x 7)) & 1 lookup(for byte), and then it explodes the aggregation circuit degree to 2^24, then we change layout to this 7x32, which means 7 columns, 32 rotations & 7 lookups, by a guessing it should get 4x lower than before, so 2^22 is enough, fortunately the fact is :). We continue guessing that maybe 15*15 is the best under this circuit config situation, can do some experiments later if necessary, after we solve that weird issue. |
Closed as a5-testnet is already updated. |
Description
Prove protocol instance get from Taiko contract.
keccak_output
<==> 'hi' andlo
values in circuit's instance columnparentHash
can be found in historical hashes of circuit. (BlockTable lookup)blockHash
can be found in circuit. (BlockTable lookup)keccak_output
)] can be found in KeccakTable lookup.Issue Link
#138 converted for A5 testnet
Type of change