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

feat: Add compiled merkle proof with C verifier #27

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

xxuejie
Copy link

@xxuejie xxuejie commented Feb 24, 2023

Similar to sparse-merkle-tree, this commit adds a new stack based compiled merkle proof format. This new format can be used to build highly performant C verifier on CKB-VM. An optimized C implementation of the verifier is also included in this commit, including tests.

Based on a random generated MMR of 1116 leafs, a proof containing 71 leafs is generated, the following results are then obtained:

  • Existing Rust based verifier using MerkleProof takes 3055952 cycles to run
  • Rust based verifier using CompiledMerkleProof takes 2958014 cycles to run
  • C based verifier takes 1302941 cycles to run

Similar to sparse-merkle-tree, this commit adds a new stack based
compiled merkle proof format. This new format can be used to build
highly performant C verifier on CKB-VM. An optimized C implementation
of the verifier is also included in this commit, including tests.
c/ckb_mmr.h Outdated Show resolved Hide resolved
c/ckb_mmr.h Show resolved Hide resolved
Copy link
Collaborator

@jjyr jjyr left a comment

Choose a reason for hiding this comment

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

Found potential overflow attack

c/ckb_mmr.h Show resolved Hide resolved
} else {
(lhs_pos, lhs_height, rhs_pos, rhs_t, lhs, rhs)
};
// calculate sibling
Copy link
Member

@quake quake Mar 13, 2023

Choose a reason for hiding this comment

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

I think the height var in stack is only used in Command::Hash, can we remove it from the stack and calculate it via pos here?

Suggested change
// calculate sibling
// calculate sibling
let height = pos_height_in_tree(pos);

and a step further, the pos is only used with StackItemType::Node, we may change the enum variant to StackItemType::Node(u64) and stack to <(T, StackItemType)

Copy link
Author

Choose a reason for hiding this comment

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

I tried this, but noticing that recalculating pos_height_in_tree would have a noticeable impact on verifier performance (~2% in cycle consumptions of Rust code), so I would rather suggest keeping the code as it is. Counting heights only by incrementing as in current code, can be faster.

Copy link
Member

@quake quake Mar 13, 2023

Choose a reason for hiding this comment

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

I tried this, but noticing that recalculating pos_height_in_tree would have a noticeable impact on verifier performance (~2% in cycle consumptions of Rust code), so I would rather suggest keeping the code as it is. Counting heights only by incrementing as in current code, can be faster.

I see, can you help to try the cycle difference between StackItemType::Node(u64, u8) // pos, height | Peak(u8) and original solution?

Copy link
Author

Choose a reason for hiding this comment

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

This additional commit b5b4602 which refactors as you mentioned, takes an additional 2000 cycles out of a total of 1583330 cycles, which is quite neglegible, hence I pull the trigger and include the refactored code.

src/compiled_proof.rs Outdated Show resolved Hide resolved
src/compiled_proof.rs Outdated Show resolved Hide resolved
}
let (rhs, rhs_t) = stack.pop().unwrap();
let (lhs, lhs_t) = stack.pop().unwrap();
let (pos, height, next_pos, next_t, item, sibling_item) = match (lhs_t, &rhs_t) {
Copy link
Author

Choose a reason for hiding this comment

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

Just an FYI: I have tried the path of introducing a pos function to StackItemType enum, so we don't need next_pos variable here. But surprisingly, a common pos function here actually will make performance worse(roughly 30000 more cycles on the overall 1583330 cycles), hence I'm keeping this next_pos variable.

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.

3 participants