feat: `Tree` trait #179

enricobottazzi · 2023-11-06T10:09:27Z

Replacement of #177

Added Tree trait
MerkleSumTree now implements Tree trait
Modified build_merkle_tree_from_entries utils function to build_merkle_tree_from_leaves to make it more generic.
Modified MerkleProof struct to take leaf instead of entry. This makes it more generic.
Modified MstInclusion Circuit constructor function to take MerkleProof rather than whole MerkleSumTree

Round should no longer take a csv_path as input. Instead, take an object that implements the Tree trait.
generate_proof_of_inclusion API on round should be modified to support the new
Update test and example accordingly
Import the summa_solvency::MerkleSumTree module inside backend module (First, you include the external library in your Cargo.toml file and then use use statement to bring the module into scope in your library code). Then, expose the imported module to other users of summa_backend library by using pub use statement. In this way, the aggregation-tree repo would only need to use summa_backend as dependency while still being able to access the API needed to build the MerkleSumTree, which is needed for the workers.

* feat: create bash script for updating verifier interface files in backend * fix: error propagation with try operator; remove unnecessaries * refactor: changed data type in 'MstInclusionProof' * fix: generate solvency verifier contract * chore: remove left over * chore: update README * fix: remove left over; assert term * fix: update README; small fixes * feat: Signer accepts address or file path for init

alxkzmn · 2023-11-06T12:28:18Z

zk_prover/src/merkle_sum_tree/mst.rs

+        [usize; N_ASSETS + 1]: Sized,
+        [usize; 2 * (1 + N_ASSETS)]: Sized,
+    {
+        verify_proof(proof)


Sorry for miscommunication, but I meant that the method implementation should live in the trait directly. There's no benefit of putting it into a separate file. It would be convenient for any developer to click on a method name and see the implementation right away, no need to click through one more level. I suggested to move all the "util" methods to the tree trait as they are common for both trees and also represent the fire functionality of the Merkle tree (not auxiliary utility code).

I applied the changes in b3990eb. I believe that this can only apply to verify_proof while I cannot define the logic of generate_proof within the Tree trait as this will be different between MerkleSumTree and AggregationMerkleSumTree

…f utils

alxkzmn

LGTM!

sifnoc · 2023-11-07T17:11:59Z

generate_proof_of_inclusion API on round should be modified to support the new

Would you give me some hint to understand about this?

enricobottazzi · 2023-11-07T17:21:38Z

@sifnoc sorry about that. I think that my comment was cut off. What I meant was to update generate_proof_of_inclusion to reflect the new API of the MstInclusionCircuit => https://github.com/summa-dev/summa-solvency/blob/2f14b3633f4fd0dd6ad9a8f75f82807109e9e57d/zk_prover/src/circuits/merkle_sum_tree.rs#L72

…Snapshot

enricobottazzi · 2023-11-09T07:23:44Z

contracts/src/Summa.sol

@@ -85,6 +85,15 @@ contract Summa is Ownable {
        inclusionVerifier = _inclusionVerifier;
    }

+    /*


What is this @alxkzmn @sifnoc?

It's leftover from my audit comment, I will remove it.

Removed c575a29

enricobottazzi · 2023-11-09T07:29:28Z

backend/examples/summa_solvency_flow.rs

-    let signature_csv_path = "src/apis/csv/signatures.csv";
-
+    // The signer instance would be shared with `address_ownership` and `round` instances
+    //
    // Using AddressInput::Address to directly provide the summa_contract's address.
    // For deployed contracts, if the address is stored in a config file,
    // you can alternatively use AddressInput::Path to specify the file's path.


Do we have an example of such config file? Can you add it to the comments if possible?

Yes, there is "backend/src/contracts/deployments.json".
I will add comment

Updated c575a29

enricobottazzi · 2023-11-09T07:36:09Z

backend/src/tests.rs

-                amount: U256::from(556863),
-            },
-        ];
+        assert_eq!(liability_commitment_logs.len(), 0);

        // Send sovlecy proof to contract


fix comment

Fixed at 5a9120f

enricobottazzi · 2023-11-09T07:38:59Z

backend/examples/summa_solvency_flow.rs

-        1,
-    )
-    .unwrap();
+    let entries = get_sample_entries();


Can we add entry.csv file back here? I think there was a misunderstanding in one of my previous comments. I didn't mean to remove the usage of entry.csv. I just meants that this should no longer be an input for the Round builder. But it's still ok to me to build the mst using let merkle_tree =MerkleSumTree::new("src/merkle_sum_tree/csv/entry_16.csv") and then pass it to Round

Updated at 5a9120f

enricobottazzi · 2023-11-09T07:39:20Z

backend/src/sample_entries.rs

+use num_bigint::ToBigUint;
+use summa_solvency::merkle_sum_tree::Entry;
+
+pub fn get_sample_entries() -> Vec<Entry<2>> {


remove this file

Removed at 5a9120f

enricobottazzi · 2023-11-09T07:42:01Z

backend/src/apis/round.rs

-        rpc_url: &str,
-        summa_address_input: AddressInput,
-        entry_csv_path: &str,
+    pub fn new<'a>(


The main point of the PR for me was that Round should accept as input an object binded to the Tree trait. Similarly to what you applied to Solvency::init. But I don't know why you applied it to Solvency which is now dismissed

Ah, that's what you were looking for. Honestly, I was trying to figure out how to define the Tree trait as a concrete type in the SolvencyCircuit struct, but I realized it's impossible without using Box, which is kind of smart pointer.

Why is that?

Can't you define it like this? https://github.com/summa-dev/summa-solvency/blob/enrico-tree-trait/zk_prover/src/circuits/solvency.rs#L76

It because the merkle_sum_tree in the solvency::init evaluates some values, like root_hash, not assign into SolvencyCircuit. Anyway I fixed like this:

pub struct Snapshot<const LEVELS: usize, const N_ASSETS: usize, const N_BYTES: usize> { mst: Box<dyn Tree<N_ASSETS, N_BYTES>>, // Instead of MerkleSumTree<N_ASSETS, N_BYTES>, assets_state: [Asset; N_ASSETS], trusted_setup: SetupArtifacts, }

Updated at 5a9120f

enricobottazzi · 2023-11-09T07:45:48Z

backend/src/apis/round.rs

        })
    }

    pub fn generate_proof_of_inclusion(
        &self,
        user_index: usize,
    ) -> Result<MstInclusionProof, &'static str> {
+        let merkle_proof = self.mst.generate_proof(user_index).unwrap();


As soon as you modify Snapshot to take an object with a Binded Trait Tree this object will no longer have the get_entry method as this method doesn't live on the Tree trait. I don't know what's the best way to handle this

Well, as you already know I added get_entry method in Tree trait. and it works well.
but for removing get_entry from Tree trait, we have to add username and balances fields on the MerkleProof.
the entry is used to get username and balances In the MstInclusionCircuit.

summa-solvency/zk_prover/src/circuits/merkle_sum_tree.rs

Lines 228 to 247 in 4e180b2

// Assign the entry username

let username = self.assign_value_to_witness(

layouter.namespace(|| "assign entry username"),

big_uint_to_fp(self.entry.username_as_big_uint()),

"entry username",

config.advices[0],

)?;

// Assign the entry balances

let mut current_balances = vec![];

for i in 0..N_ASSETS {

let balance = self.assign_value_to_witness(

layouter.namespace(|| format!("assign entry balance {}", i)),

big_uint_to_fp(&self.entry.balances()[i]),

"entry balance",

config.advices[1],

)?;

current_balances.push(balance);

}

What do you think?

Ok let's keep it as it is for now!

If something comes up during the AggregationMerkleSumTree implementation, we can just come back to summa-solvency and fix that

alxkzmn · 2023-11-10T07:53:44Z

backend/src/apis/round.rs

-                proof.public_inputs[0],
+            .submit_commitment(
+                mst_root,
+                root_sums,
                self.snapshot.assets_state.to_vec(),


Something to think about for the future: assets come from the same source as the user balances. E.g., when the CEX is building the tree, it knows that it's now querying the ETH balances or BTC balances. That's why I don't think that the assets would live in a separate CSV file. Imagine that when the CEX is querying the balances, it specifies the assets in the query. If we're sticking to CSV, I would imagine a CSV that looks like userId,balance_ETH_ETH,balance_USDT_ETH,.... Won't request changes now but Interesting to know what you think @enricobottazzi @sifnoc

It's not really an asset "state" per se, it's kind of a mapping of what assets are submitted in the MST to leave the record inside the transaction.

I agree. It used to be a state when it also contained the asset_balances. Now it doesn't make sense for asset.csv to live in a separate file. Opening an issue.

sifnoc and others added 6 commits November 3, 2023 13:04

chore: add audit comments (#168)

15a6d0a

feat: added mutex lock to signer; used it as ref

d538b28

fix: mutex deadlock in signer

c750083

chore: minor updates

be0f52d

feat: Tree trait

022dde6

alxkzmn requested changes Nov 6, 2023

View reviewed changes

fix: move verify_proof logic to Tree trait

b3990eb

enricobottazzi requested a review from alxkzmn November 6, 2023 13:10

feat: added method implementation to Tree trait and moved outside o…

35478ec

…f utils

alxkzmn approved these changes Nov 6, 2023

View reviewed changes

enricobottazzi mentioned this pull request Nov 6, 2023

No need to perform Cloning when initiating MstInclusionCircuit #180

Closed

enricobottazzi changed the base branch from master to v1-improvements-and-consolidation November 7, 2023 07:51

sifnoc added 4 commits November 7, 2023 09:41

refactor: signer spawn provider internally; updated comments

e17e493

Merge branch 'shared-signer' into enrico-tree-trait

d3c5562

fix: round now use Tree trait

391f37c

refactor: Round needs MST and Assets instead of csv files path

2f14b36

fix: rollback applying csv_parser for AddressOwnership and Assets in …

3e8c855

…Snapshot

sifnoc force-pushed the enrico-tree-trait branch from 8b44b2f to 3e8c855 Compare November 8, 2023 09:02

sifnoc added 2 commits November 8, 2023 14:45

fix: Solvency::init fn accept type that has Tree trait

c80e964

Merge branch 'v1-improvements-and-consolidation' into enrico-tree-trait

d42af95

sifnoc force-pushed the enrico-tree-trait branch from 23c8d8e to d42af95 Compare November 8, 2023 15:43

enricobottazzi mentioned this pull request Nov 8, 2023

Update interactions with contracts in backend #182

Closed

fix: updates for summa solvency contract v1.1

0c5153b

enricobottazzi requested a review from alxkzmn November 9, 2023 07:11

chore: rename compute_leaves api

4e180b2

enricobottazzi commented Nov 9, 2023

View reviewed changes

enricobottazzi linked an issue Nov 9, 2023 that may be closed by this pull request

Add AggregationMerkleSumTree to summa-aggregation #172

Closed

sifnoc added 3 commits November 9, 2023 15:37

fix: Rounds accept 'Tree' trait object

5a9120f

chore: removed and updated comments

c575a29

chore: removed env variables 'SIGNATURE_VERIFICATION_MESSAGE'

3afa5d7

alxkzmn reviewed Nov 10, 2023

View reviewed changes

alxkzmn approved these changes Nov 10, 2023

View reviewed changes

enricobottazzi marked this pull request as ready for review November 10, 2023 08:27

enricobottazzi mentioned this pull request Nov 10, 2023

Merge asset.csv with entry.csv #183

Closed

enricobottazzi merged commit 978a645 into v1-improvements-and-consolidation Nov 10, 2023
4 checks passed

enricobottazzi deleted the enrico-tree-trait branch November 21, 2023 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: `Tree` trait #179

feat: `Tree` trait #179

enricobottazzi commented Nov 6, 2023 •

edited

Loading

alxkzmn Nov 6, 2023

enricobottazzi Nov 6, 2023

alxkzmn left a comment

sifnoc commented Nov 7, 2023

enricobottazzi commented Nov 7, 2023

enricobottazzi Nov 9, 2023

sifnoc Nov 9, 2023

sifnoc Nov 9, 2023

enricobottazzi Nov 9, 2023

sifnoc Nov 9, 2023

sifnoc Nov 9, 2023

enricobottazzi Nov 9, 2023

sifnoc Nov 9, 2023

enricobottazzi Nov 9, 2023

sifnoc Nov 9, 2023

enricobottazzi Nov 9, 2023

sifnoc Nov 9, 2023

enricobottazzi Nov 9, 2023

sifnoc Nov 9, 2023

enricobottazzi Nov 9, 2023

sifnoc Nov 9, 2023

sifnoc Nov 9, 2023

enricobottazzi Nov 9, 2023

sifnoc Nov 9, 2023 •

edited

Loading

enricobottazzi Nov 9, 2023

alxkzmn Nov 10, 2023

alxkzmn Nov 10, 2023

enricobottazzi Nov 10, 2023

	// Assign the entry username
	let username = self.assign_value_to_witness(
	layouter.namespace(\|\| "assign entry username"),
	big_uint_to_fp(self.entry.username_as_big_uint()),
	"entry username",
	config.advices[0],
	)?;

	// Assign the entry balances
	let mut current_balances = vec![];

	for i in 0..N_ASSETS {
	let balance = self.assign_value_to_witness(
	layouter.namespace(\|\| format!("assign entry balance {}", i)),
	big_uint_to_fp(&self.entry.balances()[i]),
	"entry balance",
	config.advices[1],
	)?;
	current_balances.push(balance);
	}

feat: Tree trait #179

feat: Tree trait #179

Conversation

enricobottazzi commented Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alxkzmn left a comment

Choose a reason for hiding this comment

sifnoc commented Nov 7, 2023

enricobottazzi commented Nov 7, 2023

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

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

sifnoc Nov 9, 2023 • 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

feat: `Tree` trait #179

feat: `Tree` trait #179

enricobottazzi commented Nov 6, 2023 •

edited

Loading

sifnoc Nov 9, 2023 •

edited

Loading