akosba / xjsnark

A high-level framework for developing efficient zk-SNARK circuits
MIT License
182 stars 33 forks source link

Inconsistent state counters #25

Closed sanchopansa closed 5 years ago

sanchopansa commented 5 years ago

For the Pedersen implementation, which is included in #24, I see a warning about internal inconsistencies:

[1st Phase] Running Initial Circuit Analysis for < PedersenTest >
Phase 1: Analysis Completed!
[2nd Phase] Running Circuit Generator for < PedersenTest >
Circuit Generation Done for < PedersenTest >  
Internal Inconsistency Detected! -- Inconsistent State Counters [575184,575172]
     Total Number of Constraints :  42502
Even if result is correct, please report this case.

Running Sample Run: Sample_Run1
Evaluating Input on the circuit 
    [output] Value of Wire # 1070422 :: 1806c365c536dceb23220bf4e19c6af9fd05dca0a287e9e76cbe7cdf0cff4265
    [output] Value of Wire # 1070423 :: 1704d80da8629d8dca929a034202c23c72bdc90ae2775a414f108a0f37b07336
Evaluation Done 
Sample Run: Sample_Run1 finished!

One thing to point out is that I only see this happening if the SNARK program (PedersenTest in this case), is calling functions from other files (PedersenHash in this case). If I put all the code in the SNARK program itself, the inconsistent state counters warning is gone and also the results/output for both approaches is the same.

Am I doing something wrong in the above?

akosba commented 5 years ago

Thanks for reporting this case. This is related to the initialization of the static member variables that you have in helper class. Although there should be no problem with using static members, the problem is with the location of the initialization expression. I will describe the reason and workarounds below. I will also mention other issues with the code you have.

1) Initialization of static member variables:

Initialization of static member variables with native java types should have no issues. The problem applies to variables with xJsnark types. xJsnark processes the program twice, but does not currently handle the case of static initialization of variables with xJsnark types which happens once. This is why an inconsistency is detected between the first and second phases. This might not lead to issues in all cases, but I'd encourage avoiding this warning. Inconsistency of state counters can lead to more critical issues in other cases. (It might not have been visible here, because you're using native field types.)

In the most recent commit, the editor will generate hints at static initialization statements and blocks. In your code example, probably one way to avoid this is to add another static method in the BabyJubJub class that you have, say initStaticMembers(), and initialize the static members inside it. Then, you could call this method in outsource(). There are other solutions like using native java types only for the static constants and then defining xJsnark local variables accordingly. This was used in one of the examples (the pour circuit Util class if I remember correctly), but it was one method, so it was not problematic.

2) Other code issues:

This did not contribute to the issue you reported, but will likely lead to incorrect results. I have noticed conditional blocks like the following in your code:

if (b0) { 
  acc = BabyJubJub.doubleMontgomeryPoint(acc[0], acc[1]); 
}

This should display a warning while editing in your case. As the warning in the editor shows, this is not expected to work properly, because this code changes a java reference (a java array) in a conditional block. Ways to avoid this:

While I recommend the simple alternative for your case, if you give RuntimeStructs a try and observe any issues in the future, please let me know.