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 Tree storage implementation #7

Merged
merged 1 commit into from
Jun 1, 2023
Merged

Conversation

magestrio
Copy link
Contributor

Feature type

  • UI
  • Implementation
  • Specification
  • CI/CD
  • Other

Feature description

Implemented a Poseidon Sparse Merkle Tree that supports storing it on IPFS in both JSON and IPDL formats. This approach is fully aligned with the Merkle Tree from the Snarkyjs lib.

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Your code builds clean without any errors or warnings.
  • Tests for changes have been added (for bug fixes / features). If no, why?

Copy link
Contributor

Choose a reason for hiding this comment

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

if this is a test file , I think it's good to have a description of each test, and also more comments on each step. it would be extremely helpful to understand more quickly what you are trying to achieve

Copy link
Contributor

Choose a reason for hiding this comment

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

ideally also, we would be able to run tests with jest, and/or from a script in package.json this will be very helpful to run locally , understand the purpose of each component. some tests like : "checking structure of merkle tree" "storing parse merkle tree" , "storing basic merkle"

Copy link
Contributor

@JoE11-y JoE11-y left a comment

Choose a reason for hiding this comment

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

All tests work locally provided a new Merkle tree instance is passed as an argument.

Copy link
Contributor

@JoE11-y JoE11-y left a comment

Choose a reason for hiding this comment

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

Did some more tests and realized that when trying to use a witness to calculate the root of the Merkle tree, it returns the wrong root.
Is there any way to get the root using a witness?

Code Snippet

class MyMerkleWitness extends createExtendedMerkleWitness(8) { }

const Tree = new MerkleTreeJSON(8, storage);
//const Tree = new MerkleTree(8);

Tree.setLeaf(0n, Field(56));

// check
let w = await Tree.getWitness(0n);

let witness = new MyMerkleWitness(w);

let currentRoot = await Tree.getRoot();
console.log(currentRoot.toString())
console.log(witness.calculateRoot(Field(56)).toString())

issue resolved by adding the await to the setLeaf method
await Tree.setLeaf(0n, Field(56))

JoE11-y
JoE11-y previously approved these changes May 25, 2023
@JoE11-y JoE11-y dismissed their stale review May 25, 2023 09:42

rookie mistake

(async function run() {
await isReady;

const storage = await IPFS.create();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think, this implementation is a bit out-date. Please consider to rebase and use the new implementation of StorageEngineIPFS instead of old js-ipfs.

}

const jsonString = Buffer.concat(chunks).toString();
const jsonObject: NodesMap = JSON.parse(jsonString);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer, to use BSON instead of JSON since it's more friendly with binary and also the serialization/deserialization much more better than JSON.

@chiro-hiro
Copy link
Contributor

Overall everything looks good to me, there is something we need to change due to the recent change to storage engine.

  1. Im prefer to use StorageEngineIPFS in Feature/ipfs storage engine #9 instead of js-ipfs since it's deprecated soon.
  2. Use BSON instead of JSON
  3. We should use a pure poseidon Merkle tree instead of put CID in the Merkle tree. Since the data identify is its has not CID, handling CID and Merkle tree link is something that we do in the background.

@chiro-hiro
Copy link
Contributor

Did some more tests and realized that when trying to use a witness to calculate the root of the Merkle tree, it returns the wrong root. Is there any way to get the root using a witness?

Code Snippet

class MyMerkleWitness extends createExtendedMerkleWitness(8) { }

const Tree = new MerkleTreeJSON(8, storage);
//const Tree = new MerkleTree(8);

Tree.setLeaf(0n, Field(56));

// check
let w = await Tree.getWitness(0n);

let witness = new MyMerkleWitness(w);

let currentRoot = await Tree.getRoot();
console.log(currentRoot.toString())
console.log(witness.calculateRoot(Field(56)).toString())

issue resolved by adding the await to the setLeaf method await Tree.setLeaf(0n, Field(56))

Did you mean, we're unable to create a valid witness for a given root digest?.

@JoE11-y
Copy link
Contributor

JoE11-y commented May 27, 2023

Did some more tests and realized that when trying to use a witness to calculate the root of the Merkle tree, it returns the wrong root. Is there any way to get the root using a witness?

Code Snippet

class MyMerkleWitness extends createExtendedMerkleWitness(8) { }

const Tree = new MerkleTreeJSON(8, storage);
//const Tree = new MerkleTree(8);

Tree.setLeaf(0n, Field(56));

// check
let w = await Tree.getWitness(0n);

let witness = new MyMerkleWitness(w);

let currentRoot = await Tree.getRoot();
console.log(currentRoot.toString())
console.log(witness.calculateRoot(Field(56)).toString())

issue resolved by adding the await to the setLeaf method await Tree.setLeaf(0n, Field(56))

Did you mean, we're unable to create a valid witness for a given root digest?

At first, I did, but turns out I didn't wait for the promise so it returned the wrong root.
Everything works fine.

@chiro-hiro
Copy link
Contributor

Btw we need to change the title of this PR as well, this PR is about Mekle tree construction/reconstruction and providing Merkle tree witness.

@chiro-hiro
Copy link
Contributor

@magestrio Please update the code and resolve the conflict then ping me.

@magestrio magestrio force-pushed the feature/merkle-tree-impl branch from b4572df to 25a82a6 Compare June 1, 2023 06:38
@JoE11-y JoE11-y merged commit 8dff31b into main Jun 1, 2023
@chiro-hiro chiro-hiro deleted the feature/merkle-tree-impl branch June 1, 2023 07:52
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