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(katana): classes trie hash #2876

Merged
merged 3 commits into from
Jan 8, 2025
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/katana/rpc/rpc/tests/proofs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ async fn genesis_states() {

let classes_proof = MultiProof::from(proofs.classes_proof.nodes);
let classes_tree_root = proofs.global_roots.classes_tree_root;
let classes_verification_result = katana_trie::verify_proof::<hash::Pedersen>(
let classes_verification_result = katana_trie::verify_proof::<hash::Poseidon>(
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Critical: Inconsistent hash function usage detected, sensei!

While classes verification has been updated to use Poseidon, contract and storage proofs are still using Pedersen. This inconsistency could lead to:

  1. Confusion in the codebase
  2. Potential verification issues
  3. Maintenance challenges

Consider:

  1. Updating all proof verifications to use Poseidon consistently
  2. Or documenting why different hash functions are used for different proofs

&classes_proof,
classes_tree_root,
genesis_classes,
Expand Down
8 changes: 4 additions & 4 deletions crates/katana/storage/db/src/trie/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,7 @@ fn to_db_key(key: &DatabaseKey<'_>) -> models::trie::TrieDatabaseKey {

#[cfg(test)]
mod tests {
use katana_primitives::hash::{Pedersen, StarkHash};
use katana_primitives::hash::{Pedersen, Poseidon, StarkHash};
use katana_primitives::{felt, hash};
use katana_trie::{verify_proof, ClassesTrie, CommitId};
use starknet::macros::short_string;
Expand Down Expand Up @@ -447,7 +447,7 @@ mod tests {

let proofs0 = snapshot0.multiproof(vec![felt!("0x9999")]);
let verify_result0 =
verify_proof::<Pedersen>(&proofs0, snapshot_root0, vec![felt!("0x9999")]);
verify_proof::<Poseidon>(&proofs0, snapshot_root0, vec![felt!("0x9999")]);

let value =
hash::Poseidon::hash(&short_string!("CONTRACT_CLASS_LEAF_V0"), &felt!("0xdead"));
Expand All @@ -464,7 +464,7 @@ mod tests {

let proofs1 = snapshot1.multiproof(vec![felt!("0x6969")]);
let verify_result1 =
verify_proof::<Pedersen>(&proofs1, snapshot_root1, vec![felt!("0x6969")]);
verify_proof::<Poseidon>(&proofs1, snapshot_root1, vec![felt!("0x6969")]);

let value =
hash::Poseidon::hash(&short_string!("CONTRACT_CLASS_LEAF_V0"), &felt!("0x80085"));
Expand All @@ -475,7 +475,7 @@ mod tests {
let root = trie.root();
let proofs = trie.multiproof(vec![felt!("0x6969"), felt!("0x9999")]);
let result =
verify_proof::<Pedersen>(&proofs, root, vec![felt!("0x6969"), felt!("0x9999")]);
verify_proof::<Poseidon>(&proofs, root, vec![felt!("0x6969"), felt!("0x9999")]);

let value0 =
hash::Poseidon::hash(&short_string!("CONTRACT_CLASS_LEAF_V0"), &felt!("0x80085"));
Expand Down
7 changes: 3 additions & 4 deletions crates/katana/trie/src/classes.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
use bonsai_trie::{BonsaiDatabase, BonsaiPersistentDatabase, MultiProof};
use katana_primitives::block::BlockNumber;
use katana_primitives::class::{ClassHash, CompiledClassHash};
use katana_primitives::hash::Pedersen;
use katana_primitives::hash::{Poseidon, StarkHash};
use katana_primitives::Felt;
use starknet::macros::short_string;
use starknet_types_core::hash::{Poseidon, StarkHash};

use crate::id::CommitId;

Expand All @@ -15,7 +14,7 @@ impl ClassesMultiProof {
// TODO: maybe perform results check in this method as well. make it accept the compiled class
// hashes
pub fn verify(&self, root: Felt, class_hashes: Vec<ClassHash>) -> Vec<Felt> {
crate::verify_proof::<Pedersen>(&self.0, root, class_hashes)
crate::verify_proof::<Poseidon>(&self.0, root, class_hashes)
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Ohayo! Test files need updating, sensei!

The switch to Poseidon affects these test files:

  • crates/katana/storage/db/src/trie/mod.rs (test cases)
  • crates/katana/rpc/rpc/tests/proofs.rs

Please update the test cases to use Poseidon instead of Pedersen to maintain consistency with the production code.

🔗 Analysis chain

Verify compatibility with existing callers, sensei!

The switch from Pedersen to Poseidon in verify_proof is a breaking change. Let's verify all callers are updated.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for all calls to verify_proof to ensure they're updated
rg -l "verify_proof::<.*Pedersen>" 

Length of output: 115


Script:

#!/bin/bash
# Check the actual usage in both files
echo "=== storage/db/src/trie/mod.rs ==="
rg "verify_proof::<.*Pedersen>" crates/katana/storage/db/src/trie/mod.rs -B 2 -A 2

echo -e "\n=== rpc/rpc/tests/proofs.rs ==="
rg "verify_proof::<.*Pedersen>" crates/katana/rpc/rpc/tests/proofs.rs -B 2 -A 2

Length of output: 1531

}
}

Expand All @@ -27,7 +26,7 @@ impl From<MultiProof> for ClassesMultiProof {

#[derive(Debug)]
pub struct ClassesTrie<DB: BonsaiDatabase> {
trie: crate::BonsaiTrie<DB, Pedersen>,
trie: crate::BonsaiTrie<DB, Poseidon>,
}

//////////////////////////////////////////////////////////////
Expand Down
Loading