Include layer index before node when creating label preimage. #1139
Conversation
| fn test_private_por_input_circuit_poseidon_oct() { | ||
| test_private_por_input_circuit::<TestTree<PoseidonHasher, typenum::U8>>(1_068); | ||
| } | ||
|
|
There was a problem hiding this comment.
This was a missing test I added while I was editing circuit tests. It is not otherwise related to this PR.
porcuquine
force-pushed
the
feat/layer-index-label
branch
3 times, most recently
from
038454f
to
ff97285
Compare
|
I'm not sure why the seal lifecycle tests are failing on CI. With fresh groth parameters, they (at least 2Kib) pass locally for me. Maybe I accidentally reused a CI parameter cache key. Will poke at it more tomorrow. |
Yes, I also made that mistake, so v27c as a key has been burned first here: #1132 Then here: #1138 Go ahead and claim v27d and I'll update mine for things other than that. |
| @@ -16,7 +16,7 @@ use std::io::{self, SeekFrom}; | |||
| use std::path::{Path, PathBuf}; | |||
|
|
|||
| /// Bump this when circuits change to invalidate the cache. | |||
| pub const VERSION: usize = 27; | |||
| pub const VERSION: usize = 28; | |||
There was a problem hiding this comment.
Oh, this requires a version bump here to 28?
There was a problem hiding this comment.
this version shouldn’t change, only when we have published 27 and need to change sth
There was a problem hiding this comment.
Hmmmm. I guess we have concretized a more recent policy. I'll go with that but note it means developers will more often get caught with stale parameters.
| @@ -177,8 +182,15 @@ mod tests { | |||
| assert_eq!(cs.num_constraints(), 532_024); | |||
|
|
|||
| let (l1, l2) = data.split_at_mut(size * NODE_SIZE); | |||
| super::super::super::vanilla::create_label_exp(&graph, &id_fr.into(), &*l2, l1, node) | |||
| .unwrap(); | |||
| super::super::super::vanilla::create_label_exp( | |||
There was a problem hiding this comment.
Super not cool. Maybe add use crate::stacked::vanilla::create_label_exp to the imports
There was a problem hiding this comment.
I believe you mean Super not chocolate ...
There was a problem hiding this comment.
Dammit, why did this change have to touch that line? @DrPeterVanNostrand I'll look into it.
porcuquine
force-pushed
the
feat/layer-index-label
branch
4 times, most recently
from
b1290c8
to
c3ded18
Compare
porcuquine
force-pushed
the
feat/layer-index-label
branch
from
c3ded18
to
28813c1
Compare
This adds constraints, which slightly surprised me — since it does not increase the length of the preimage being hashed.I believe the reason for this is that the padding bits being replaced were in fact boolean constants. Some initial operations in the sha256 computation involving them may therefore have been optimized away at synthesis time.UPDATE: I don't think my explanation above makes sense. I think the actual source of the increase is the booleanity constraints introduced when allocating the bits.
UPDATE 2: Although my update is accurate, there should not have been an allocation. The latest should now correctly use constants everywhere.