feat(katana-messaging): remove sending messages to settlement layer - #3035
Conversation
|
Ohayo, sensei! Here are the updated details for this pull request: WalkthroughThis pull request refactors the messaging subsystem. The changes remove the Changes
Sequence Diagram(s)sequenceDiagram
participant MT as Messaging Task
participant MS as Messaging Service
participant SO as StarknetOS
participant SC as Settlement Chain
MT->>MS: Gather messages from settlement chain
MS->>SO: Execute proof generation (STARK proof)
SO-->>MS: Return generated proof
MS->>SC: Send proof for verification
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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: 0
🧹 Nitpick comments (3)
crates/katana/messaging/src/service.rs (1)
105-109: Ohayo sensei! There's a small nitpick on naming.
The field is spelledlastest_block, but you might wantlatest_blockto avoid confusion.- pub lastest_block: u64, + pub latest_block: u64,crates/katana/messaging/src/starknet.rs (2)
98-98: Consider improving error handling.Ohayo sensei! While the error handling change from
SendErrortoGatherErroraligns with the removal of sending functionality, consider adding more context to help with debugging:- return Err(Error::GatherError); + return Err(Error::GatherError.with_context("Failed to fetch settlement chain block number"));
118-118: Consider adding error context.Similar to above, consider adding more context to the error:
- .map_err(|_| Error::GatherError) + .map_err(|e| Error::GatherError.with_context(format!("Failed to fetch events: {}", e)))
📜 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 (7)
crates/katana/messaging/Cargo.toml(0 hunks)crates/katana/messaging/src/ethereum.rs(1 hunks)crates/katana/messaging/src/lib.rs(4 hunks)crates/katana/messaging/src/service.rs(4 hunks)crates/katana/messaging/src/starknet.rs(4 hunks)crates/katana/rpc/rpc/tests/messaging.rs(1 hunks)crates/katana/sync/stage/src/sequencing.rs(1 hunks)
💤 Files with no reviewable changes (1)
- crates/katana/messaging/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ensure-windows
- GitHub Check: test
🔇 Additional comments (19)
crates/katana/messaging/src/service.rs (6)
20-20: Ohayo sensei! Nice simplification by dropping the generic parameter.
This makes theMessagingServicemore straightforward and aligns well with the removal of direct message sending.
33-33: Ohayo sensei!
No concerns here—removing the generic bounds fromimpl MessagingServicekeeps the code maintainable.
53-53: Ohayo sensei!
Initializingmsg_gather_futasNoneis an elegant way to defer gathering until needed. Good job.
112-112: Ohayo sensei!
Good usage ofStreamhere. This improves clarity by consistently polling for new messaging outcomes.
118-124: Ohayo sensei! Consider verifying the interval scheduling.
If gathering takes longer than the interval, ensure we don’t queue extra futures. This code is likely fine, but keep an eye on possible repeated scheduling.
133-133: Ohayo sensei!
ReturningPoll::Ready(Some(MessagingOutcome { ... }))is a neat approach to publish the new block’s outcome. Nicely handled.crates/katana/messaging/src/lib.rs (10)
5-8: Ohayo sensei!
These doc lines clearly reflect the new focus on message gathering and proof-based sending. Good clarity.
11-11: Ohayo sensei!
Mentioning Anvil usage is helpful for local dev context. Thumbs up!
18-21: Ohayo sensei!
Great job highlighting the collecting of messages and proof generation steps. This clarifies how messages reach the settlement chain.
25-27: Ohayo sensei!
Thanks for documenting theto_addresslimitation. This warns devs to use an applicative workaround.
29-31: Ohayo sensei!
Again, good doc about carrying the real address inpayload. This offers a straightforward approach to circumventBlockifierconstraints.
100-101: Ohayo sensei!
Documenting the interval with thorough comments is beneficial. No issues here.
122-122: Ohayo sensei!
Specifically pullingrpc_urlandcore_contractfrom the chain spec ensures consistency with settlement-layer config. Nice synergy.Also applies to: 131-131
211-212: Ohayo sensei!
MakingMessagingTaskmore concise without generics is consistent with the refactoring. Looks good.
215-216: Ohayo sensei!
A simple constructor with only the needed parameter is a good step towards clarity.
228-235: Ohayo sensei!
Nice logging approach. Conditionally logging message collection is helpful for debugging and verifying flows.crates/katana/sync/stage/src/sequencing.rs (1)
44-44: Ohayo sensei!
Using the updatedMessagingService::newsignature with fewer parameters is neat and aligns with the new design.crates/katana/messaging/src/ethereum.rs (1)
21-21: LGTM! Simplified imports.Ohayo! The import cleanup looks good, removing unused
warnwhile keeping necessary logging levels.crates/katana/rpc/rpc/tests/messaging.rs (1)
67-73: LGTM! Configuration simplified.Ohayo sensei! The
MessagingConfighas been correctly simplified by removing the unusedsender_addressandprivate_keyfields, aligning with the removal of settlement layer messaging functionality.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3035 +/- ##
==========================================
- Coverage 56.31% 56.21% -0.10%
==========================================
Files 437 437
Lines 59013 58757 -256
==========================================
- Hits 33232 33031 -201
+ Misses 25781 25726 -55 ☔ View full report in Codecov by Sentry. |
| //! execute them, and send messages to a settlement chain via a provable mechanism (STARK proof in | ||
| //! the case of Starknet). |
There was a problem hiding this comment.
STARK proof in the case of Starknet
I'd imagine it's same case as well when settling to Ethereum - that the messages would be settled by the proof (or to be exact, on the call to updateState function on the settlement contract, no?
There was a problem hiding this comment.
Yes exactly, the proof is verified with the SNOS output. And the SNOS output is the one submitted to the L1 core contract, where messages are processed (but only once the proof is verified).
| // Send message from L2 to L1 testing must be done using Saya or part of | ||
| // it to ensure the settlement contract is test on piltover and its `update_state` method. |
There was a problem hiding this comment.
We definitely need katana <> saya e2e tests.
I know, to some extent, parts of the settlement process can be already be mocked. But we should think how we can make the whole katana to saya flow in a way where each part can be easily mocked for testing purposes.
There was a problem hiding this comment.
Currently Saya has a mode where:
- The PIE is generated from SNOS execution. This output inside the PIE is the exact same one that will be in the proof.
- Then the bridge layout is also mocked since with this setup, it's a mocked facts registry.
This allows to conduct e2e test, where the only mocked part is the proof verification and bootloader output wrapping.
We can setup those tests in two ways:
- Having 1 katana acting as starknet and 1 katana acting as a rollup, and then Saya will be started to settle rollup blocks on the top of katana (then we can verify that the core contract last verified block has changed.
- Having 1 katana acting as a rollup, and the core contract on Sepolia (small cost every time to be deployed, but will be a test on a public network).
There was a problem hiding this comment.
For this we may want a Saya runner (as we have for Katana), which ease the testing phase.
There was a problem hiding this comment.
We can setup those tests in two ways:
We could try doing both ways, the (1) will be tested every time, and (2) will only be run when it gets merged to main. As there might be some mismatch between having katana and Starknet as the settlement layer (eg confirmation delay etc), so being able to test both gives higher confidence.
2 participants
The feature of sending message to settlement layer was done mainly to test messaging in the early days of Starknet.
Now that app chains are here, there is no need of such feature.
This PR aims at removing all the code related to send a message to the settlement layout from Katana operated as a Rollup sequencer. Module documentation has been updated and types simplified when possible.
We also had some errors of type
Sendwhere it should have beenGather, confusing me during some tests on Katana. Those errors have been fixed toGatherinstead.In the current setup, we still have the blockifier limitation for the
to_addressfield as mentioned in comment into the messaging crate doc. But this can be resolved by a simple application logic with theMSG_MAGICas it was implemented in Katana before trimming this code.Ideally @kariy as we have discussed, if the blockifier changes to remove this constraint aren't big, it will be beneficial to have
to_addressof typefelt.Summary by CodeRabbit
MessagingServiceandStarknetMessagingstructures by removing unnecessary fields and methods.