Anonymity mining claims pallet update #303

semaraugusto · 2023-03-10T19:08:59Z

Summary of changes
Changes introduced in this pull request:
Updates anonimity-mining-claims pallet.

Can now initialize/update Unspent and Spent tree roots and verify against the proof roots during claiming.
Adding claims-verifier pallet for the zero-knowledge verification using our fixtures.
Adding handle_proof_verification implementation

Reference issue to close (if applicable)

Closes #299

…s_circom.rs` accordingly. Standalone node broke for some reason

…for it

…msVerifier::force_set_parameters`

drewstone

One big thing is to convert this pallet back to without an instancing. Let me know your thoughts, if you think we should have multiple versions of these pallets then that's certainly a worthwhile discussion.

drewstone · 2023-03-11T03:19:21Z

circom-proving/src/lib.rs

+	circom_from_raw(wasm_buffer)
+}
+
+pub fn generate_proof<const N: usize>(


Can you break proving out into a separate file and put a feature flag on the mod? Best to keep code that can't go in the runtime separate.

This entire folder circom-proving is already meant to not be used in the runtime. It is only meant for testing. Technically speaking, we can get rid of the verify_proof function in this file, since it is identical to that in primitives/verifying/circom.rs.

drewstone · 2023-03-11T03:20:04Z

circom-proving/src/lib.rs

+	Ok((proof, full_assignment))
+}
+
+/// Verifies a given RLN proof


Docs / comments need more closer review please

Also worth attributing where we got the code from in a citation on the top (add a license header and cite relevant entities w/ links)

drewstone · 2023-03-11T03:22:38Z

pallets/anonymity-mining-claims/src/lib.rs

+	/// The map of trees to the maximum number of anchor edges they can have
+	#[pallet::storage]
+	#[pallet::getter(fn max_edges)]
+	pub type MaxEdges<T: Config<I>, I: 'static = ()> = StorageValue<_, u32, ValueQuery>;


This currently doesn't match the comment. Is this meant to be a map? Also since it's rewards pallet we should probably map ResourceID -> u32 to indicate the size of the bridge that a resource ID in question has.

drewstone · 2023-03-11T03:24:28Z

pallets/anonymity-mining-claims/src/lib.rs

-	pub type LinkableTreeHasEdge<T: Config<I>, I: 'static = ()> =
-		StorageMap<_, Blake2_128Concat, (T::TreeId, T::ChainId), bool, ValueQuery>;
+	#[pallet::getter(fn registered_resource_ids)]
+	pub type RegisteredResourceIds<T: Config<I>, I: 'static = ()> =


In theory the anonymity claims could be built on a chain with a SignatureBridge which does register resource IDs. We could and maybe should support a post-registration hook on that pallet which we can use to hook in here so we don't have to double register things all the time.

drewstone · 2023-03-11T03:25:03Z

pallets/anonymity-mining-claims/src/lib.rs


 	/// The next spent tree root index
 	#[pallet::storage]
 	#[pallet::getter(fn next_spent_root_index)]
 	pub(super) type NextSpentRootIndex<T: Config<I>, I: 'static = ()> =
-		StorageValue<_, T::RootIndex, ValueQuery>;
+		StorageMap<_, Blake2_128Concat, ResourceId, T::RootIndex, ValueQuery>;

 	// Map of treeId -> root index -> root value for unspent roots


Need to update comments to account for ResourceId update.

drewstone · 2023-03-11T03:26:54Z

pallets/anonymity-mining-claims/src/lib.rs

-		// Add nullifier on VAnchor
-		for nullifier in &proof_data.input_nullifiers {
-			T::VAnchor::add_nullifier_hash(Self::ap_vanchor_tree_id(), *nullifier)?;
+	/// Ensure valid unspent roots


Need better function docs, will help speed up review process to understand the expectation of each function.

drewstone · 2023-03-11T03:31:08Z

pallets/anonymity-mining-claims/src/mock.rs

 		TokenWrapper: pallet_token_wrapper::{Pallet, Call, Storage, Event<T>},
 		KeyStorage: pallet_key_storage::{Pallet, Call, Storage, Event<T>, Config<T>},
-		AnonymityMiningClaims: pallet_anonymity_mining_claims::{Pallet, Call, Storage, Event<T>, Config<T>}
+		AnonymityMiningClaims: pallet_anonymity_mining_claims::<Instance1>::{Pallet, Call, Storage, Event<T>, Config<T>}


I think we shouldn't make this pallet instance-able. That allows us to have multiple versions of the same pallet in the runtime, which does seem a bit overkill. The way to do this is to remove all <I> and instance trait extensions. Things like:

<T, I> -> <T>

<T, I = ()> -> <T>

<T: Config<I>, I: 'static = ()> -> <T: Config>

Do book time with me if you want to pair and I can help you learn to do this quickly.

Can a non-instanceable pallet take in an instanceable pallet in its Config trait? Because we take in a VAnchor type that represents the APVAnchor and pallet_vanchor is instanceable?

I presume so, maybe @1xstj can chime in too.

drewstone · 2023-03-11T03:31:48Z

pallets/claims-verifier/src/lib.rs

+
+	#[pallet::storage]
+	#[pallet::getter(fn parameters)]
+	/// Details of the module's parameters for different vanchor configurations


Comments need updating

drewstone · 2023-03-11T18:24:39Z

Worth trying but should be possible. Just have to pass in a single instance to the non instance pallet.

…

On Sat, Mar 11, 2023 at 8:36 AM Akilesh Tangella ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pallets/anonymity-mining-claims/src/mock.rs <#303 (comment)> : > TokenWrapper: pallet_token_wrapper::{Pallet, Call, Storage, Event<T>}, KeyStorage: pallet_key_storage::{Pallet, Call, Storage, Event<T>, Config<T>}, - AnonymityMiningClaims: pallet_anonymity_mining_claims::{Pallet, Call, Storage, Event<T>, Config<T>} + AnonymityMiningClaims: pallet_anonymity_mining_claims::<Instance1>::{Pallet, Call, Storage, Event<T>, Config<T>} Can a non-instanceable pallet take in an instanceable pallet in its Config trait. Because we take in a VAnchor type that represents the APVAnchor and pallet_vanchor is instanceable? — Reply to this email directly, view it on GitHub <#303 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADELLF3ZG46NGUDJS3J5TGTW3SLXFANCNFSM6AAAAAAVW3E6TM> . You are receiving this because you commented.Message ID: ***@***.***>

…o fmt.

… the benchmarks

This reverts commit 37cb28c.

This reverts commit 061813b.

…e-generated" This reverts commit 8fae9f2.

…_zkey in the benchmarks" This reverts commit 471017d.

This reverts commit 30c6e34.

This reverts commit 4b44895.

akileshtangella · 2023-03-20T22:50:37Z

@drewstone I'm in favor of merging this. There was a bit trouble getting the benchmarks done, but at least the claims pallet verifier is working. I want @semaraugusto to move onto some other tasks. WDYT?

drewstone · 2023-03-21T20:34:04Z

Sounds fine to me. Please make tasks to track benchmarking separately so we can add to future sprints. @saiakilesh.

akileshtangella · 2023-03-21T20:34:49Z

Sounds fine to me. Please make tasks to track benchmarking separately so we can add to future sprints. @saiakilesh.

Will do

semaraugusto and others added 13 commits March 7, 2023 16:56

update claims pallet logic

7a9bfd7

add claims-verifier pallet

9d24896

move circom proof code into primitives and update `vanchor/src/test…

0198927

…s_circom.rs` accordingly. Standalone node broke for some reason

commiting

1fb747c

push

492a448

moved circom code to new directory and removed it from primitives

e84290a

failed to find errrs on pallet

1647f4e

add ark-circom to claims pallet

2f01fb7

clear errors

1bac819

add claims-verifier to mock and implementation

2a8804d

fix bug with init/update functions for unspent/spent roots. Add test …

036149c

…for it

getting VerifyingParametersNotInitialized even though calling `Clai…

872b858

…msVerifier::force_set_parameters`

adding circuitInput.json

2fceeb8

drewstone requested a review from 1xstj March 11, 2023 03:17

Merge branch 'main' into semar/claims-pallet

38e531f

drewstone requested changes Mar 11, 2023

View reviewed changes

akileshtangella changed the title ~~Anonimity mining claims pallet update~~ Anonymity mining claims pallet update Mar 11, 2023

semaraugusto added 5 commits March 13, 2023 18:34

claim_ap works for one transaction

0a801d6

test verifies both claims

1013d1b

update comments on anonimity-mining-claims/src/lib.rs. running carg…

a489df7

…o fmt.

fmt

d64453d

update errors in anonimity-mining-claims/src/tests.rs

23cae3c

semaraugusto force-pushed the semar/claims-pallet branch from 6372e4c to 23cae3c Compare March 14, 2023 21:40

drewstone and others added 6 commits March 14, 2023 17:01

Adds the callable extrinsic

3e8710a

Fmt

dd6a37a

update fixtures, fix pathing error

fad35e2

reupdating fixtures

6232537

Merge branch 'main' into semar/claims-pallet

a1d4de7

adding benchmarking logic

4b44895

semaraugusto and others added 16 commits March 17, 2023 15:08

Merge branch 'main' into semar/claims-pallet

13e84e3

add anonymity-mining-claims to runtime

30c6e34

fix signature bridge compilation error while on runtime-benchmarks

4943157

trying to add ark-circom to dependencies in order to use read_zkey in…

471017d

… the benchmarks

removing circom-proving from dependencies as proof will be pre-generated

8fae9f2

anon mining benchmarks compiling

061813b

progress

37cb28c

Revert "progress"

faa0e6e

This reverts commit 37cb28c.

Revert "anon mining benchmarks compiling"

b38a5b3

This reverts commit 061813b.

Revert "removing circom-proving from dependencies as proof will be pr…

8e46a50

…e-generated" This reverts commit 8fae9f2.

Revert "trying to add ark-circom to dependencies in order to use read…

5d161d3

…_zkey in the benchmarks" This reverts commit 471017d.

Revert "add anonymity-mining-claims to runtime"

fb0dcba

This reverts commit 30c6e34.

Revert "adding benchmarking logic"

3e22191

This reverts commit 4b44895.

Merge branch 'main' into semar/claims-pallet

d4cc9f4

refactoring

2048d0f

fmt

0998f1f

Merge branch 'main' into semar/claims-pallet

5ffa4fe

akileshtangella marked this pull request as ready for review March 21, 2023 14:59

Merge branch 'main' into semar/claims-pallet

775faca

drewstone merged commit f06bc17 into main Mar 21, 2023

drewstone deleted the semar/claims-pallet branch March 21, 2023 20:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Anonymity mining claims pallet update #303

Anonymity mining claims pallet update #303

semaraugusto commented Mar 10, 2023

drewstone left a comment

drewstone Mar 11, 2023

akileshtangella Mar 11, 2023

drewstone Mar 11, 2023

drewstone Mar 11, 2023

drewstone Mar 11, 2023

drewstone Mar 11, 2023

drewstone Mar 11, 2023

drewstone Mar 11, 2023

drewstone Mar 11, 2023

akileshtangella Mar 11, 2023 •

edited

Loading

drewstone Mar 13, 2023

drewstone Mar 11, 2023

drewstone commented Mar 11, 2023 via email

akileshtangella commented Mar 20, 2023

drewstone commented Mar 21, 2023 •

edited

Loading

akileshtangella commented Mar 21, 2023

Anonymity mining claims pallet update #303

Anonymity mining claims pallet update #303

Conversation

semaraugusto commented Mar 10, 2023

drewstone 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

akileshtangella Mar 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewstone commented Mar 11, 2023 via email

akileshtangella commented Mar 20, 2023

drewstone commented Mar 21, 2023 • edited Loading

akileshtangella commented Mar 21, 2023

akileshtangella Mar 11, 2023 •

edited

Loading

drewstone commented Mar 21, 2023 •

edited

Loading