feat: verifier integration #79
Conversation
first configuration, then contracts that we depend on
markspanbroek
force-pushed
the
feat/verifier-integration
branch
from
f21cd15
to
d5cc5fc
Compare
| contract Endian { | ||
| /// reverses byte order to allow conversion between little endian and big | ||
| /// endian integers | ||
| function _byteSwap(bytes32 input) internal pure returns (bytes32 output) { |
There was a problem hiding this comment.
Wouldn't it be much faster to do the conversion outside the contract?
There was a problem hiding this comment.
It would (and be cheaper as well), but the data that are converted with this are gathered from the contract's data and are not "input to function call", so IMHO not really possible without removing the goal that we don't want to submit the public inputs to the submitProof() call. Have a look here: https://github.com/codex-storage/codex-contracts-eth/pull/79/files#diff-5973808e32384782b5978a63e85b893b67d634be8aa42001f0ef480f189a0688R162-R193
There was a problem hiding this comment.
But thinking about it a bit more, maybe this conversion would in itself warrant actually dropping that goal in order to optimize it from a gas perspective. I don't know what is cheaper though. IMHO we don't need to analyze it now, but it would be good to have a look on it later on with some analysis.
There was a problem hiding this comment.
We cannot allow hosts to supply their own public inputs, that would defeat the security of the scheme. They have to come from values that are on-chain.
What we could do for the two values that are converted is the following:
- for the merkle root in the storage request, we can change its definition from a bytes32 to a uint256, which gets rid of the need to interpret bytes as an integer (be it little or big endian)
- for the randomness, we could interpret that as a big-endian integer, instead of a little-endian integer in the nim code
This however has two disadvantages:
- when we move to sha256 instead of poseidon2, we have to change the type of the merkle root back to bytes32 again
- we get a scheme for the zero-knowledge proof where most of the time we interpret bytes as little-endian, but in a special case we interpret bytes as big-endian
There was a problem hiding this comment.
Yeah, I did not really though it through. I thought that "we would do assertions on the submitted public inputs" to verify if it is not "defeating the security", but that would only circle back to the need to the conversions 😅 Never mind.
There was a problem hiding this comment.
There isn't any particular reason we're using little endian in the client side, why not change it there?
There was a problem hiding this comment.
In fact, it was probably an oversight that we didn't set off with bigendian from the get go, given that the EVM uses bigendian.
There was a problem hiding this comment.
We chose little endian because the circom and arkworks tooling use it.
There was a problem hiding this comment.
LGTM! Pretty good work. Thanks for taking it over and finishing it! 👍 I can't approve my own PR, so somebody else needs to do that. 🤪
Just a side note, there were a lot of formatting changes. It is kind of distracting for reviewing... Maybe in the future, we could adapt some aggressive styler like Prettier just for Solidity to prevent these in the future.
Thanks for the review, I'll just wait for @emizzle's review before merging.
Good idea, try |
|
I'm merging this, we can always update the code when we think of a better way to handle endianness. |
This is a draft PR of my integration of the verifier, which I managed to get to before going to vacation. I managed to get to the basic integration point, but further work is needed to finish it.
The current state is that the Verifier is deployed and works with the generated proof that is part of the Circuit assets build here. The tests
Proofs.test.jsare passing (which was my personal goal), but the whole suite not yet.There are numerous TODOs spread around the changes to better indicate what needs to be finished, but the main high-level things are:
submitProof()call