Full Multi-query support onchain #321

daveroga · 2024-12-03T10:10:47Z

Implement multi-query support similar to existing requests now in Universal Verifier.

Manage multi-query (Query) independently and in the same way we are managing Requests in ZKPVerifierBase now (setQuery, getQuery, …). Avoid ZKP and generalize methods in Requests. setRequest, getRequest, submitResponse, …
Change _proofs mapping from address user to uint256 userID to keep identifier of the user.
Create 2 mappings for managing link between userAddress and userID . In this first version 1 userID will have only 1 address? Review best approach or if we have to keep an array of addresses.

mapping(address userAddress => uint256 userID) _user_address_to_id;
mapping(unit256 userID => address userAddress) _id_to_user_address; // check address[] ?

Manage Auth in specific type of requests. Auth requests.

requestId. 32 bytes (in Big Endian): 31-0x00(not used), 30-0x01(requestType), 29..0 hash calculated Id,

For requestType:
   - 0x00 - regular request
   - 0x01 - auth request

Define challenge based on the ethereum address in this first version
Figure out linkID between the Requests and Auth of the same Query.
Consider that more than one type of auth may be available there for specific multiRequest. And each of the multiRequests should define it’s own set of valid auth along with requests.
RequestId change from uint64 to uint256.

Link to Tech Spec for changes: https://www.notion.so/privado-id/Multi-query-Tech-Spec-13d4f86a875180e68fc8e3fa5362805e

coveralls · 2024-12-03T11:12:16Z

Pull Request Test Coverage Report for Build 13087224067

Details

524 of 586 (89.42%) changed or added relevant lines in 31 files are covered.
8 unchanged lines in 3 files lost coverage.
Overall coverage increased (+1.7%) to 85.66%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
contracts/lib/ClaimBuilder.sol	13	14	92.86%
contracts/lib/PrimitiveTypeUtils.sol	3	4	75.0%
contracts/validators/request/CredentialAtomicQueryV3Validator.sol	26	27	96.3%
contracts/validators/request/LinkedMultiQueryValidator.sol	58	59	98.31%
contracts/lib/IdentityBase.sol	0	2	0.0%
contracts/verifiers/ValidatorWhitelist.sol	6	8	75.0%
contracts/identitytreestore/IdentityTreeStore.sol	3	6	50.0%
contracts/lib/IdentityLib.sol	7	11	63.64%
contracts/verifiers/Verifier.sol	226	230	98.26%
contracts/lib/SmtLib.sol	13	20	65.0%

Files with Coverage Reduction	New Missed Lines	%
contracts/lib/IdentityBase.sol	1	61.11%
contracts/payment/MCPayment.sol	3	92.42%
contracts/state/State.sol	4	75.13%

Totals
Change from base Build 13073554522:	1.7%
Covered Lines:	1313
Relevant Lines:	1435

💛 - Coveralls

contracts/verifiers/UniversalVerifierMultiQuery.sol

.solhintignore

contracts/state/State.sol

AndriianChestnykh · 2025-01-27T18:10:16Z

contracts/validators/request/CredentialAtomicQueryValidatorBase.sol

        string[] _supportedCircuitIds;
-        IState state;
+        uint256[1] __gap;


Need a small comment why we put this __gap

Validator will always work only with one State contract. Why do you need to pass it as a param to methods and removed it from here?

Yes, that is what was true before we started transferring cross-chain timestamps of state and GIST.
But, as long as now Verifier should insert cross-chain messages into State contracts (Validator is not efficient to do the same instead), it would be a duplication of the link to the State contract in both Validator and Verifier. At least in terms of contract init, It's better only one component to keep the link.

To me it looks strange and a place for a calling contract / EOA to make a mistake. The less possibilities to make a mistake - the better.

So please revert this part and remove passing of state contract address from the verify function.

contracts/interfaces/IGroth16Verifier.sol

contracts/interfaces/IRequestValidator.sol

contracts/interfaces/IVerifier.sol

OBrezhniev · 2025-01-29T19:52:57Z

contracts/lib/ArrayUtils.sol

-        require(length > 0, "Length should be greater than 0");
-        require(length <= limit, "Length limit exceeded");
-        require(start < arrLength, "Start index out of bounds");
+        if (length == 0) {


require allows passing error as a second param instead of a string. And I think require statements are much easier to read than ifs, especially in else-if cases. So unless there are side effects of doing so (e.g. much higher gas cost or unexpected behavior in specific cases like explained in the docs) I would use require.

Custom errors save more gas than using strings in require error messages. This is because the error message in custom errors is encoded into the bytecode of the contract, so it does not need to be stored on the blockchain.

The gas cost is almost the same if message length < 32 bytes as far as I remember, however for custom errors logging is better as we can add params. So I would add params like "length", "limit" etc.

Yes, I'll add some more params for the user to know this limits and have more deails about these errors.

Well, I was proposing to use:

require(length > 0, LenghtShouldBeGreaterThanZero());

instead of:

if (length == 0) { revert LenghtShouldBeGreaterThanZero(); }

In this case we could even do this if it's question to compact it in 1 line

if (length == 0) revert LenghtShouldBeGreaterThanZero();

contracts/lib/IdentityLib.sol

contracts/verifiers/Verifier.sol

OBrezhniev · 2025-01-29T23:01:46Z

contracts/verifiers/Verifier.sol

+    function _getRequestType(uint256 requestId) internal pure returns (uint8) {
+        // 0x0000000000000000 - prefix for old uint64 requests
+        // 0x0000000000000001 - prefix for keccak256 cut to fit in the remaining 192 bits
+        return uint8(requestId >> 192);


We dedicated 64 bits to this, why return just 8? If all the other bits are for flags, then maybe we should move 01 to the beginning of the prefix, so it becomes 0x0100000000000000 and the rest of the bytes (7) would be for flags. @vmidyllic @demonsh what do you think?

In my opinion it seems more natural like you propose. In fact it was the first approach and we changed it.

I have implemented it like this. First byte 0x00 for previous uint64 requestIds. 0x01 for new uint256 requestIds.
We will reserve the next 7 bytes for future potential updates and then the remaining 24 bytes (192 bits) are for requestId hash from request params.

Thanks for quick change. But I just recalled why I though highest byte was not a good idea, because this byte is not fully usable, e.f. 0xFF would make number not fit into the field. Actually max byte there is ~0x29. So we might want to shift this type to second byte. So, let's do 0x0001000000000000 prefix for keccak. And do not use most significant byte there at all (keep first byte 0x00 always).

OBrezhniev

Please make state contract preconfigured in validators. See rationale in the comments.

contracts/interfaces/IVerifier.sol

vmidyllic · 2025-01-31T12:23:36Z

contracts/interfaces/IAuthValidator.sol

+        bytes calldata proof,
+        bytes calldata params,
+        address sender,
+        IState state,


are we sure that we need state contract as an explicit parameter and not as a part of 'params' or other 'generic' platform?
What if state contract is not neede for some specific key validation / etc .

as well as expectedNonce

Changed as we agreed

vmidyllic · 2025-01-31T12:29:08Z

contracts/interfaces/IVerifier.sol

+     * @param proof proof to verify.
+     */
+    struct AuthResponse {
+        string authType; //zkp-auth-v2, zkp-auth-v3, etc.


let's remove this comment until it is not yet decided how types will be named

vmidyllic · 2025-01-31T12:30:11Z

contracts/interfaces/IVerifier.sol

+     * @param metadata Metadata for the multiRequest. Empty in first version.
+     */
+    struct MultiRequest {
+        uint256 multiRequestId;


I'm not sure what will be a protocol value for multiRequestId

vmidyllic · 2025-01-31T12:53:39Z

contracts/test-helpers/EmbeddedVerifierWrapper.sol

+        super.__EmbeddedVerifier_init(initialOwner, state);
+    }
+
+    function _beforeProofSubmit(


beforeTransfer / afterTransfer are standard hooks for erc20, that is why we named ours accordingly!

vmidyllic · 2025-01-31T13:02:15Z

contracts/validators/request/LinkedMultiQueryValidator.sol

+        uint256[10] circuitQueryHash;
+    }
+
+    string public constant VERSION = "1.0.0-beta";


I guess right syntax is "1.0.0-beta.1"

vmidyllic · 2025-01-31T13:06:51Z

hardhat.config.ts

+        settings: {
+          optimizer: {
+            enabled: true,
+            runs: 200,


are we sure that current production version has '200' runs and not '1000' , won't it influence the upgrade

Currently we were not working with optimizer in the deployments we have. We achieved max limit size of the contract bytecode and with this specific settings for every contract that is at maximum limit size it reduces in most of the cases to half of the size and we are able to deploy them.

OBrezhniev · 2025-01-31T23:46:25Z

contracts/verifiers/Verifier.sol

        );

+        if (
+            keccak256(abi.encodePacked(authResponse.authMethod)) ==
+            keccak256(abi.encodePacked("authV2"))


Maybe we could have it precalculated as a constant.

OBrezhniev · 2025-01-31T23:47:27Z

contracts/verifiers/Verifier.sol

+                keccak256(abi.encodePacked("challenge"))
+            ) {
+                bytes32 expectedNonce = keccak256(abi.encode(sender, responses)) &
+                    0x0FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF;


I think it should be 0x00FFFF... if we want to zero out the first byte.

Only with the first position as 0 that is 4 bits we have 252 bits remaining for the calculation that fits in a field number. But I could do it with 1 byte if you think it's better an we we'll use 248 bits then.

daveroga and others added 22 commits November 26, 2024 11:18

new universal verifier multi-query

bf71699

Discussion results

74b24a1

Discussion results 2

16a8b27

update universal verifier multi query

f05104d

remove ZKP

779513f

add auth validators

8da12c7

Broader discussion results

ba75a5d

Add agreed universal verifier solution

ae9cec1

update universal verifier multi query with agreed design

790a0e3

query status and submit response

1858dce

get request type from 30 byte of the requestId

4527834

add EthIdentityValidator

bb82ff3

updates from review

e66a160

Discussion result

d416b9a

updates from discussion

a26861c

check response field values from linked response fields

0c2e792

fix some errors in compilation

16f8b5a

test universal verifier multi-query

4afb814

different validators for sig and authv2

8f55467

check specific storage values

657fa6c

overrides solidity compiler optimizations for specific contract

46d17ab

remove some methods

1e7b877

daveroga added 4 commits December 3, 2024 12:16

fix solhint

2942cdf

update from discussion

a91c1e7

set group of requests and check linkID

849e790

fix solhint

a02592c

daveroga force-pushed the PID-2709-full-multi-query-support-on-chain branch from b70f050 to a02592c Compare December 5, 2024 11:33

AndriianChestnykh requested changes Dec 5, 2024

View reviewed changes

updates from review

6b82ff0

daveroga added 2 commits January 27, 2025 14:14

remove ICircuitValidator references

4458158

move getters for storage data back to the beginning

256d571

AndriianChestnykh reviewed Jan 27, 2025

View reviewed changes

daveroga and others added 5 commits January 28, 2025 10:48

resolve solhint warnings

19da90c

update from review comments

f238b05

update AuthV2Validator

127c40c

add function getResponseFields to verifier

374ea04

Remove unused tests and comments

f553fed

AndriianChestnykh marked this pull request as ready for review January 28, 2025 18:28

AndriianChestnykh previously approved these changes Jan 28, 2025

View reviewed changes

AndriianChestnykh self-requested a review January 28, 2025 18:31

OBrezhniev reviewed Jan 30, 2025

View reviewed changes

AndriianChestnykh dismissed their stale review via f553fed January 30, 2025 16:04

daveroga added 3 commits January 31, 2025 00:29

updates from review

67bc7fe

update requestId calculation

a817f35

fix solhint

569c6b6

daveroga requested a review from OBrezhniev January 30, 2025 23:47

OBrezhniev requested changes Jan 31, 2025

View reviewed changes

OBrezhniev reviewed Jan 31, 2025

View reviewed changes

contracts/interfaces/IVerifier.sol Outdated Show resolved Hide resolved

update RequestStatus for RequestProofStatus

5025be6

daveroga requested a review from OBrezhniev January 31, 2025 00:37

vmidyllic reviewed Jan 31, 2025

View reviewed changes

daveroga added 3 commits January 31, 2025 15:29

more updates from review

de9d7a5

more updates from review

50960a8

change AuthType to AuthMethod

6334aa3

daveroga requested a review from vmidyllic January 31, 2025 18:25

OBrezhniev reviewed Jan 31, 2025

View reviewed changes

precalculate keccack256 of field names

707a770

daveroga force-pushed the PID-2709-full-multi-query-support-on-chain branch from df615af to 707a770 Compare February 1, 2025 09:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Full Multi-query support onchain #321

Full Multi-query support onchain #321

daveroga commented Dec 3, 2024 •

edited

Loading

coveralls commented Dec 3, 2024 •

edited

Loading

AndriianChestnykh Jan 27, 2025

OBrezhniev Jan 29, 2025

AndriianChestnykh Jan 30, 2025

OBrezhniev Jan 31, 2025 •

edited

Loading

OBrezhniev Jan 31, 2025

daveroga Jan 31, 2025

OBrezhniev Jan 29, 2025

daveroga Jan 30, 2025

AndriianChestnykh Jan 30, 2025

daveroga Jan 30, 2025 •

edited

Loading

OBrezhniev Jan 31, 2025

daveroga Jan 31, 2025

OBrezhniev Jan 29, 2025

daveroga Jan 30, 2025

OBrezhniev Jan 31, 2025 •

edited

Loading

daveroga Jan 31, 2025

OBrezhniev left a comment

vmidyllic Jan 31, 2025

vmidyllic Jan 31, 2025

daveroga Jan 31, 2025

vmidyllic Jan 31, 2025

vmidyllic Jan 31, 2025

vmidyllic Jan 31, 2025

vmidyllic Jan 31, 2025

daveroga Jan 31, 2025

vmidyllic Jan 31, 2025

daveroga Jan 31, 2025 •

edited

Loading

OBrezhniev Jan 31, 2025

daveroga Feb 1, 2025

OBrezhniev Jan 31, 2025

daveroga Feb 1, 2025

Full Multi-query support onchain #321

Are you sure you want to change the base?

Full Multi-query support onchain #321

Conversation

daveroga commented Dec 3, 2024 • edited Loading

coveralls commented Dec 3, 2024 • edited Loading

Pull Request Test Coverage Report for Build 13087224067

Details

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OBrezhniev Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daveroga Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OBrezhniev Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OBrezhniev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daveroga Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daveroga commented Dec 3, 2024 •

edited

Loading

coveralls commented Dec 3, 2024 •

edited

Loading

OBrezhniev Jan 31, 2025 •

edited

Loading

daveroga Jan 30, 2025 •

edited

Loading

OBrezhniev Jan 31, 2025 •

edited

Loading

daveroga Jan 31, 2025 •

edited

Loading