Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: refactor Solidity Transcript and improve error handling in sol_honk flow #11158

Merged
merged 5 commits into from
Jan 13, 2025

Conversation

maramihali
Copy link
Contributor

@maramihali maramihali commented Jan 10, 2025

A cleanup PR in preparation for the ZK contract

  • create a RelationParameters struct so functions in RelationsLib don't need to take the Transcript as function argument ( this will be useful to reuse this library for both the zk and non-zk contract)
  • make loadProof less manual and more robust by creating some utility functions
  • ensure the sol_honk flow actually displays contract compilation errors and clear errors (if possible) when deploying to aid debugging

@@ -254,6 +254,18 @@ library Honk {
G1Point lagrangeLast;
}
Copy link
Contributor Author

@maramihali maramihali Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern of having to copy updates from Solidity to here is really not great (although same thing was done for Plonk). So we should come up with an alternative script

@maramihali maramihali changed the title chore: refactor Solidity Transcript and ensure the sol_honk displays contract compilation errors chore: refactor Solidity Transcript and improve error handling in sol_honk flow Jan 10, 2025
@ledwards2225 ledwards2225 self-requested a review January 13, 2025 16:55
Copy link
Contributor

@ledwards2225 ledwards2225 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG - one comment that maybe doesn't apply anyway

y_0: uint256(bytes32(proof[0x1a0:0x1c0])),
y_1: uint256(bytes32(proof[0x1c0:0x1e0]))
});
p.w1 = bytesToG1ProofPoint(proof[0x60:0xe0]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a nice improvement - a thought about how to make it even less error prone: A simple class that gets instantiated with a proof and has methods like read_G1, read_Fr etc. that automatically read the right amount of data and increment a cursor to prepare for the next read. Would avoid having to explicitly write out these ranges in hex which seems painful. Just something to consider

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, maybe this logic has to match the solidity so you can't do things like what I suggested?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah definitely can be on the list of TODOs

@@ -782,26 +735,26 @@ library RelationsLib {

function accumulatePermutationRelation(
Fr[NUMBER_OF_ENTITIES] memory p,
Transcript memory tp,
Honk.RelationParameters memory rp,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This two letter name is better than the single letter above it but cant it at least be params or something haha

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will address in followup :)

@maramihali maramihali enabled auto-merge (squash) January 13, 2025 17:35
@maramihali maramihali merged commit 58fdf87 into master Jan 13, 2025
48 checks passed
@maramihali maramihali deleted the mm/contract-cleanupp branch January 13, 2025 18:10
TomAFrench added a commit that referenced this pull request Jan 13, 2025
* master: (329 commits)
  fix(avm): mac build (#11195)
  fix: docs rebuild patterns (#11191)
  chore: refactor Solidity Transcript and improve error handling in  sol_honk flow (#11158)
  chore: move witness computation into class plus some other cleanup (#11140)
  fix: get_next_power_exponent off by 1 (#11169)
  chore(avm): vm2 followup cleanup (#11186)
  fix: underconstrained bug (#11174)
  refactor: VariableMerkleTree readability improvements (#11165)
  chore: removing noir bug workaround (#10535)
  chore(docs): Remove node pages  (#11161)
  git subrepo push --branch=master noir-projects/aztec-nr
  git_subrepo.sh: Fix parent in .gitrepo file. [skip ci]
  chore: replace relative paths to noir-protocol-circuits
  git subrepo push --branch=master barretenberg
  feat(avm2): avm redesign init (#10906)
  feat: Sync from noir (#11138)
  feat: simulator split (#11144)
  chore: rpc server cleanup & misc fixes (#11145)
  git subrepo push --branch=master noir-projects/aztec-nr
  git_subrepo.sh: Fix parent in .gitrepo file. [skip ci]
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants