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

4844 #157

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

4844 #157

wants to merge 8 commits into from

Conversation

wzmuda
Copy link

@wzmuda wzmuda commented Aug 6, 2024

No description provided.

src/data/VerifierLookupTable.sol Outdated Show resolved Hide resolved
src/test/mock/SimpleVerifier.sol Show resolved Hide resolved
src/test/DeletionTreeVerifier16.sol Outdated Show resolved Hide resolved
src/test/InsertionTreeVerifier16.sol Outdated Show resolved Hide resolved
src/WorldIDIdentityManagerImplV1.sol Outdated Show resolved Hide resolved
@wzmuda wzmuda marked this pull request as ready for review August 26, 2024 22:39
src/WorldIDIdentityManagerImplV3.sol Outdated Show resolved Hide resolved
src/WorldIDIdentityManagerImplV3.sol Outdated Show resolved Hide resolved
src/WorldIDIdentityManagerImplV3.sol Outdated Show resolved Hide resolved
Copy link
Contributor

@kustosz kustosz left a comment

Choose a reason for hiding this comment

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

Some style + this is cryptographically broken

src/WorldIDIdentityManagerImplV3.sol Outdated Show resolved Hide resolved
src/WorldIDIdentityManagerImplV3.sol Outdated Show resolved Hide resolved
src/interfaces/ITreeVerifier.sol Outdated Show resolved Hide resolved
src/WorldIDIdentityManagerImplV3.sol Show resolved Hide resolved
src/WorldIDIdentityManagerImplV3.sol Outdated Show resolved Hide resolved
src/WorldIDIdentityManagerImplV3.sol Outdated Show resolved Hide resolved
src/WorldIDIdentityManagerImplV3.sol Outdated Show resolved Hide resolved
@wzmuda wzmuda force-pushed the wz/4844 branch 3 times, most recently from 440e522 to c309622 Compare September 6, 2024 20:13
@wzmuda wzmuda force-pushed the wz/4844 branch 2 times, most recently from 6ca49e5 to a5ab7aa Compare September 10, 2024 21:23
Copy link
Contributor

@kustosz kustosz left a comment

Choose a reason for hiding this comment

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

One minor wording issue

/// @notice The root obtained after inserting all of `identityCommitments` into the tree described
/// by `preRoot`. Must be an element of the field `Kr`. (already in reduced form)
uint256 postRoot;
/// @notice Root of the merkle tree after adding identities to it.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is misleading. Better wording: Hash of all inserted identities, constructed by taking a root of the Merkle Tree of minimal depth containing all identities.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

Every contract seems to use 4 anyway.

Signed-off-by: Wojciech Zmuda <[email protected]>
This is groundwork for EIP-4844 support.
Cancun introduces the blobhash opcode and SOLC 0.8.24 is the first
revision to support this opcode in Solidity.
Signed-off-by: Wojciech Zmuda <[email protected]>
It does not seem to be used anywhere.

Signed-off-by: Wojciech Zmuda <[email protected]>
This is ground work for EIP-4844.

Vm.blobhashes allow for mocking value of the next call to the blobhash
opcode. This will be useful in EIP-4844 tests to mock this opcode.

Update the usage of the one symbol that changed its name.

Signed-off-by: Wojciech Zmuda <[email protected]>
Implement WorldID Identity Manager in its 3rd implementation.
The new implementation contains one main method - registerIdentities.
It's the EIP-4844 equivalent of the existing registerIdentities
from the V1 implementation.

The new registerIdentities calls verifyProof that is also different from
its classic counterpart. The new verifyProof comes from the
EIP-4844-ready InsertionTreeVerifier164844 contract, that was generated
from the semaphore-mtb's InsertionCircuit modified to accommodae
EIP-4844 features.

Since the new verifyProof function has a different prototype than the
previous one, the ITreeVerifier interface has been updated with the new
prototype. This change results in lots of boilerplate changes as all
contracts implementing this interface must contain a dummy implementation
of the new function, even if it does not make practical sense.

Add tests for the new feature. Tests are modelled by the existing
registration tests. The new contract is basically a copy of the old one,
with the main difference of calling the new registerIdentities
implementation.

Signed-off-by: Wojciech Zmuda <[email protected]>
Making these variables global shrunks stack significantly, allowing to
build contracts without the --via-ir flag.

Signed-off-by: Wojciech Zmuda <[email protected]>
The --gas-report flag of the forge test command seems to conflict with
blobhash mocks in tests, making the new EIP-4844 tests fail, despite
they pass without this flag.

Turn off benchmarks temporarily, to unblock EIP-4844 development.
This should be debugged and fixed later.

Signed-off-by: Wojciech Zmuda <[email protected]>
@wzmuda
Copy link
Author

wzmuda commented Sep 11, 2024

@Dzejkop @0xKitsune, after consulting @kustosz we decided to temporarily block the benchmark step of the CI. Forge tests go bonkers if --gas-report flag is present. This has something to do with the foundry cheat code I use in tests to mock blob hash value, but I failed to determine the exact cause.

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.

4 participants