refactor(katana-messaging): remove message execution and fix hashing #2884
Conversation
WalkthroughOhayo, sensei! The pull request introduces significant changes to the messaging system in the Katana project. The modifications primarily focus on simplifying message handling by removing direct message execution capabilities, updating hashing methods, and streamlining event and transaction processing. The changes eliminate specialized execution logic, standardize message parsing, and improve logging control across multiple components of the messaging infrastructure. Changes
Sequence DiagramsequenceDiagram
participant Appchain
participant Messaging
participant Starknet
Appchain->>Messaging: Send Message
Messaging->>Messaging: Compute Message Hash
Messaging->>Starknet: Register Message
Starknet-->>Messaging: Message Registered
Messaging->>Messaging: Log Message Details
The sequence diagram illustrates the simplified messaging flow, focusing on message registration and logging without direct execution capabilities. Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/katana/core/src/service/messaging/mod.rs (1)
232-244: Consider refactoring logging to reduce duplicationOhayo, sensei! Noticing that the logging logic for
msg_countin bothMessagingOutcome::GatherandMessagingOutcome::Sendis similar, you might consider refactoring it into a helper function. This aligns with the DRY principle and enhances code maintainability.Here's a possible refactor:
+ fn log_message_outcome(msg_count: usize, action: &str) { + if msg_count > 0 { + info!(target: LOG_TARGET, %msg_count, "{} messages {} the settlement chain.", action, if action == "Gathered" { "from" } else { "to" }); + } + trace!(target: LOG_TARGET, %msg_count, "{} messages {} the settlement chain.", action, if action == "Gathered" { "from" } else { "to" }); + } impl<EF: ExecutorFactory> Future for MessagingTask<EF> { // ... fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> { let this = self.get_mut(); while let Poll::Ready(Some(outcome)) = this.messaging.poll_next_unpin(cx) { match outcome { MessagingOutcome::Gather { msg_count, .. } => { - if msg_count > 0 { - info!(target: LOG_TARGET, %msg_count, "Collected messages from settlement chain."); - } - - trace!(target: LOG_TARGET, %msg_count, "Collected messages from settlement chain."); + log_message_outcome(msg_count, "Gathered"); } MessagingOutcome::Send { msg_count, .. } => { - if msg_count > 0 { - info!(target: LOG_TARGET, %msg_count, "Sent messages to the settlement chain."); - } - - trace!(target: LOG_TARGET, %msg_count, "Sent messages to the settlement chain."); + log_message_outcome(msg_count, "Sent"); } } } Poll::Pending } }crates/katana/core/src/service/messaging/starknet.rs (2)
21-21: Fix typo in commentOhayo! There's a small typo in the comment: "teh" should be "the".
-/// field, in teh current design we set the `to_address` to the `MSG` magic value. +/// field, in the current design we set the `to_address` to the `MSG` magic value.
27-28: Consider moving magic values to a configuration fileThe TODO comment suggests this should come from configuration. This is a good practice for maintainability and flexibility.
Consider creating a dedicated configuration struct for magic values and event keys.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/katana/contracts/messaging/cairo/src/appchain_messaging.cairo(4 hunks)crates/katana/contracts/messaging/cairo/src/contract_msg_starknet.cairo(1 hunks)crates/katana/core/Cargo.toml(1 hunks)crates/katana/core/src/service/messaging/mod.rs(2 hunks)crates/katana/core/src/service/messaging/service.rs(0 hunks)crates/katana/core/src/service/messaging/starknet.rs(7 hunks)
💤 Files with no reviewable changes (1)
- crates/katana/core/src/service/messaging/service.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (9)
crates/katana/contracts/messaging/cairo/src/appchain_messaging.cairo (3)
140-152: Enhanced documentation forcompute_hash_appc_to_snOhayo, sensei! The updated documentation for
compute_hash_appc_to_snclearly outlines the function's purpose, arguments, and return value. This improvement enhances readability and maintainability.
172-187: Verify additions tocompute_hash_sn_to_appcparametersOhayo, sensei! The function
compute_hash_sn_to_appcnow includesfrom_addressas a parameter, and the hashing algorithm has changed to usecore::poseidon::poseidon_hash_span. Please ensure that all calls to this function are updated accordingly and that the Poseidon hash aligns with the expected hashing mechanism across the system.Would you like assistance in checking for function calls that need updating?
Line range hint
189-208: Confirm compatibility of Poseidon hash algorithmOhayo, sensei! The switch to
core::poseidon::poseidon_hash_spanincompute_hash_sn_to_appcchanges the hashing algorithm fromstarknet_keccakto Poseidon. Please verify that this change is compatible with all components that consume these hashes, especially if external systems expect the old hashing method.You can run the following script to identify usages of
compute_hash_sn_to_appcand ensure they are updated:✅ Verification successful
Poseidon hash change is compatible with L2->L3 messaging design
Ohayo, sensei! After reviewing the messaging protocol implementation, I can confirm that the switch to Poseidon hash in
compute_hash_sn_to_appcis compatible with the system design. This function is specifically used for L2->L3 (Starknet to Appchain) messages, while L3->L2 messages continue usingstarknet_keccak. The change is contained within the experimental L2-L3 messaging system and doesn't affect external components.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to `compute_hash_sn_to_appc` and check for correct parameters # Search for function calls ast-grep --pattern 'compute_hash_sn_to_appc($_)' --json | jq '.matches[].matches[].range'Length of output: 158
Script:
#!/bin/bash # Search for all occurrences of the function name rg -l "compute_hash_sn_to_appc" # Search for documentation about the messaging protocol fd -e md -e txt | xargs rg -l "messaging|hash" # Look for test files related to this functionality fd -e cairo | rg "test.*messaging"Length of output: 329
Script:
#!/bin/bash # Read the messaging protocol documentation cat crates/katana/contracts/messaging/README.md # Look for test files in the messaging contract directory fd -e cairo test crates/katana/contracts/messaging/ # Search for hash-related content in the contract file rg -A 5 -B 5 "hash|keccak|poseidon" crates/katana/contracts/messaging/cairo/src/appchain_messaging.cairoLength of output: 15812
crates/katana/contracts/messaging/cairo/src/contract_msg_starknet.cairo (1)
7-9: Clarified comments regardingto_addressusageOhayo, sensei! The revised comments explain that
to_addressis set to theMSGmagic value because valid Ethereum addresses are too small to represent Starknet addresses. This clarification improves the understanding of the message sending mechanism.crates/katana/core/src/service/messaging/mod.rs (1)
55-55: Addedtraceto tracing importsOhayo, sensei! The addition of
traceto thetracingimports enables more granular logging levels, which can be useful for debugging and monitoring.crates/katana/core/src/service/messaging/starknet.rs (3)
Line range hint
128-148: Clean implementation of hash sending logicOhayo sensei! The simplified hash sending implementation looks good. The error handling and logging are well-structured.
328-343: Tests properly validate new message parsing logicThe test cases look good, covering both successful parsing and error cases.
231-267: Well-documented message parsing with improved hash computationThe changes look good! A few points:
- Clear documentation of magic value usage
- Proper handling of payload structure
- Switch to Poseidon hash aligns with Piltover's implementation
Let's verify that all message parsing uses the same hash computation:
✅ Verification successful
Ohayo sensei! After reviewing the message parsing implementations, I can confirm:
Hash computation is correctly implemented for each messaging mode
The use of Poseidon hash in Starknet messaging is intentional and properly isolated from Ethereum messaging, which uses its own appropriate hash computation. The changes align perfectly with Piltover's requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any other hash computation methods that might need updating rg -l "hash" | xargs rg "poseidon|keccak|pedersen" -B 2 -A 2Length of output: 65597
Script:
#!/bin/bash # Search for message parsing and hash computation in Starknet messaging rg -B 2 -A 2 "parse.*message|hash.*message" crates/katana/core/src/service/messaging/ # Look for hash computation methods in messaging rg -B 2 -A 2 "(poseidon|keccak|pedersen).*hash" crates/katana/core/src/service/messaging/Length of output: 9648
crates/katana/core/Cargo.toml (1)
31-31: Appropriate addition of starknet-crypto dependencyOhayo! The addition of starknet-crypto as a workspace dependency is necessary for the Poseidon hash computation used in message parsing.
| // This needs to be adjusted to piltover, which actually uses poseidon. | ||
| let message_hash = compute_l2_to_l1_message_hash(from_address, to_address, &calldata); |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo sensei!
Confirmed: L2->L1 message hash needs Poseidon alignment
The review comment is valid. The current implementation relies on starknet core's hash computation, while Piltover consistently uses Poseidon hash for all operations. This inconsistency should be addressed to maintain compatibility with Piltover's architecture.
crates/katana/primitives/src/utils/transaction.rs: Updatecompute_l2_to_l1_message_hashto use Poseidon hashcrates/katana/core/src/service/messaging/starknet.rs: Implement the TODO comment to align with Piltover's hash computation
🔗 Analysis chain
Ensure consistent hash computation across L1->L2 messages
The TODO comment indicates a need to adjust the hash computation to use Poseidon consistently with Piltover.
Let's check for other L1->L2 hash computations that might need updating:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for L1->L2 message hash computations
ast-grep --pattern 'compute_l2_to_l1_message_hash($$$)'
Length of output: 1623
Script:
#!/bin/bash
# Find implementation of compute_l2_to_l1_message_hash
ast-grep --pattern 'fn compute_l2_to_l1_message_hash($$$)'
# Search for Poseidon-related code
rg -i "poseidon" --type rust
Length of output: 11382
Script:
#!/bin/bash
# Search for L2->L1 message hash implementation
rg -A 5 "fn compute_l2_to_l1_message_hash"
# Also search in potential utility modules
rg -l "compute_l2_to_l1_message_hash" | grep -i "utils\|hash"
Length of output: 653
Script:
#!/bin/bash
# Get the complete implementation of compute_l2_to_l1_message_hash
rg -A 15 "pub fn compute_l2_to_l1_message_hash" crates/katana/primitives/src/utils/transaction.rs
Length of output: 620
Script:
#!/bin/bash
# Find MsgToL1 struct definition and implementation
rg -B 2 -A 10 "struct MsgToL1" --type rust
# Search for MsgToL1 hash implementation
ast-grep --pattern 'impl $_ for MsgToL1'
Length of output: 1677
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/katana/core/src/service/messaging/starknet.rs (3)
21-21: Ohayo sensei! Fix typo in commentThere's a typo in the comment: "teh" should be "the".
-/// field, in teh current design we set the `to_address` to the `MSG` magic value. +/// field, in the current design we set the `to_address` to the `MSG` magic value.
306-306: Ohayo sensei! Clarify L1 fee commentThe comment about L1 fees could be more explicit about why this value is kept despite not being applicable.
- // This doesn't apply for l2-l3 messaging in the current setting. + // Note: This fee value is a legacy from L1->L2 messaging. For L2->L3 messaging, + // we keep this field for compatibility but the value is not used.
Line range hint
1-349: Ohayo sensei! Overall changes look solid!The changes successfully:
- Remove message execution handling, simplifying the codebase
- Implement Poseidon hash for compatibility with Piltover
- Maintain good documentation and test coverage
The implementation aligns well with the PR objectives and maintains code quality.
Consider documenting the message flow between L2 and L3 in a high-level architecture document, as this change represents a significant shift in the messaging model.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
spawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (1)
crates/katana/core/src/service/messaging/starknet.rs(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (2)
crates/katana/core/src/service/messaging/starknet.rs (2)
362-379: Ohayo sensei! Tests look great!The test coverage is comprehensive, including both success and failure cases. The test data uses meaningful selectors and covers the new hash computation.
314-349: Ohayo sensei! Verify hash computation against PiltoverThe implementation looks good, but let's verify that our hash computation matches Piltover's implementation exactly.
✅ Verification successful
Ohayo sensei! Hash implementation verified successfully! ✨
The hash computation matches Piltover's implementation exactly:
- Same input format and field ordering
- Using official starknet-crypto (v0.7.1) implementation of Poseidon hash
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find and compare hash computation in Piltover rg -A 10 "fn compute_message_hash" --glob "**/*.cairo" # Check if we're using the same Poseidon hash implementation rg "poseidon_hash_many" --type rustLength of output: 6079
Script:
#!/bin/bash # Check starknet_crypto dependency version rg "starknet-crypto.*=" --type toml # Fetch Piltover's hash implementation curl -s "https://raw.githubusercontent.com/keep-starknet-strange/piltover/a9c015eada5082076185a7b1413163a3da247009/src/messaging/hash.cairo" || echo "Failed to fetch Piltover implementation"Length of output: 3709
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2884 +/- ##
==========================================
+ Coverage 55.79% 55.81% +0.01%
==========================================
Files 449 449
Lines 57724 57693 -31
==========================================
- Hits 32209 32201 -8
+ Misses 25515 25492 -23 ☔ View full report in Codecov by Sentry. |
This PR first removes the need to execute a message. This experimental feature didn't present any concrete use case.
With the current work on L3, having the message hashes pre-registered is more than enough, allowing e2e testing locally without depending on the state update (and the proof generation).
Also, piltover is using Poseidon hash to compute message hashes. This PR adjust the hash computation to ensure the messages can be consumed on L2 without error and more reliable data for dApps.
Summary by CodeRabbit
Release Notes
Messaging Improvements
Dependency Updates
starknet-cryptoas a workspace dependencyPerformance Optimizations