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

Merkle Memberships Program; Clean up #2

Merged
merged 21 commits into from
Oct 13, 2023

Conversation

chfanghr
Copy link
Contributor

No description provided.

@chfanghr chfanghr changed the base branch from master to develop September 20, 2023 11:30
@chfanghr chfanghr changed the title Store password tree root unchain Store password tree root onchain Sep 20, 2023
@chfanghr chfanghr force-pushed the connor/simple-password-tree-root-update branch from ddb8b08 to f34c12f Compare September 28, 2023 14:14
@chfanghr chfanghr marked this pull request as ready for review September 28, 2023 14:15
@chfanghr chfanghr force-pushed the connor/simple-password-tree-root-update branch from f34c12f to 2afdd64 Compare September 28, 2023 14:18
@adamczykm adamczykm force-pushed the connor/simple-password-tree-root-update branch 4 times, most recently from b2319f6 to accb962 Compare October 2, 2023 21:39
@adamczykm adamczykm force-pushed the connor/simple-password-tree-root-update branch from accb962 to 230514e Compare October 2, 2023 21:42
src/library/plugin/pluginType.ts Outdated Show resolved Hide resolved
src/library/tools/pluginServer/config.ts Outdated Show resolved Hide resolved
src/library/tools/pluginServer/index.ts Outdated Show resolved Hide resolved
src/plugins/passwordTree/common/passwordTreeProgram.ts Outdated Show resolved Hide resolved
@adamczykm
Copy link
Contributor

Additionally, I think that we will need to rethink the IProver interface. Good that we don't use it anywhere yet.

@chfanghr chfanghr force-pushed the connor/simple-password-tree-root-update branch from 0c810c5 to 4fdf63b Compare October 4, 2023 17:39
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a nice functional implementation but got noisy. Typescript syntax is not very beautiful it appears. I'll propose some changes.

Copy link
Contributor

@adamczykm adamczykm left a comment

Choose a reason for hiding this comment

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

There are some changes required.
Please try to add some docstrings already. They don't have to be beautiful yet, but should explain the high-level operations of the code. To make it easier to understand how the proof is achieved etc.
Additionaly, we need the headless client to be able to test the flow of the updated plugin.
It can be done with another PR though.

src/plugins/merkleMemberships/server/index.ts Outdated Show resolved Hide resolved
src/plugins/merkleMemberships/server/index.ts Show resolved Hide resolved
tsconfig.json Show resolved Hide resolved
@chfanghr chfanghr changed the title Store password tree root onchain Merkle Memberships Program; Clean up Oct 9, 2023
Copy link
Contributor

@adamczykm adamczykm left a comment

Choose a reason for hiding this comment

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

The leaf index problem is still unresolved. The server should be sending the entire tree or all the leaves to ensure anonimity.

src/library/tools/pluginServer/index.ts Outdated Show resolved Hide resolved
src/plugins/merkleMemberships/client/index.ts Show resolved Hide resolved
)(proof);
}

async fetchPublicInputs(
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem here is that asking for a specific index of the tree reveals the identity.
The only solution is to transfer the entire tree. Then it should be searched for the index and this way the witness could be generated.
If there's no easy way to serialize the tree then the list of all leaves should be send and the tree should be rebuilt.

Copy link
Contributor Author

@chfanghr chfanghr Oct 10, 2023

Choose a reason for hiding this comment

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

This is apparently not very efficient. I don't see why this hurts anonymity - a client can always make a bunch of junk requests to confuse the server if the server decides to log the traffic.

Aside from that, I have some concerns regarding the security of this implementation - if someone knows the value x where Poseidon.hash([x]) == Field(0) and if any of the trees have one or more holes, we are screwed. Is this possible? Not so familiar with the poseidon hash algorithm.

src/plugins/merkleMemberships/server/index.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@adamczykm adamczykm left a comment

Choose a reason for hiding this comment

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

Approving before the deeper review.

@adamczykm adamczykm merged commit cff9069 into develop Oct 13, 2023
2 checks passed
adamczykm pushed a commit that referenced this pull request Mar 30, 2024
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