Persistent mode for saya using mock of SNOS - #2253
Conversation
| pub struct SayaModeArg(pub SayaMode); | ||
|
|
||
| #[derive(Debug, Args, Clone)] | ||
| pub struct ShardOptions { |
There was a problem hiding this comment.
We should think how the terminology will be separated, as shard is mostly for ephemeral execution and persistent execution is done in a different manner at some point.
Or the name shouldn't be piltover but settlement_contract which can be the world for shards and piltover for persistency.
There was a problem hiding this comment.
Sure, that makes sense.
| &[ | ||
| SayaModeArg(SayaMode::Ephemeral), | ||
| SayaModeArg(SayaMode::Persistent), | ||
| SayaModeArg(SayaMode::PersistentMerging), |
There was a problem hiding this comment.
So the one using the merger program recursively should be soverein?
There was a problem hiding this comment.
Hum, then no. It's something unrelated. I wasn't sure why we had a persistent and persistent merging. Seems to be only at provable program layer, right?
There was a problem hiding this comment.
It changes the program flow in both merging and updating state. It is a different program, so it produces a different output. I think at this point PersistentMerging should be treated as a sort of legacy program, as it is still useful, but Persistent is a newer, "sounder" way to prove state.
There was a problem hiding this comment.
Let's remove PersistentMerging if it's not used anymore?
|
|
||
| // Wait until Ctrl + C is pressed, then shutdown | ||
| ctrl_c().await?; | ||
| // ctrl_c().await?; |
There was a problem hiding this comment.
not required anymore to be able to kill the program? Or it's handled by the .start() function?
There was a problem hiding this comment.
Not required, as the ephemeral shard will shut itself down after reaching the shutdown event.
There was a problem hiding this comment.
Yeah but we may want to kill saya. :) In this case it would be better keeping it to ensure graceful shutdown in case of manual interruption.
There was a problem hiding this comment.
It is the opposite, this line simply prevents saya from closing until ctrl+c is pressed. Now we do the same thing by simply not returning from the start function until all the blocks are prover (for ephemeral shard).
A graceful shutdown would have to be implemented there.
| use crate::{SayaStarknetAccount, LOG_TARGET}; | ||
|
|
||
| #[derive(Debug, Serialize)] | ||
| pub struct PiltoverCalldata { |
There was a problem hiding this comment.
I'm not sure if the cainome bindgen is already integrated to piltover. But if yes, we should use it to already have concrete types in rust that are automatically serializable in cairo.
There was a problem hiding this comment.
Looks to me like it is integrated, will use it as a dependcy.
By the way those are automatically serializable, as they use serde_felt from this PR: cartridge-gg/cairo-proof-parser#10.
There was a problem hiding this comment.
It doesn't seem to be working at the moment. Create a bug report here: keep-starknet-strange/piltover#39
There was a problem hiding this comment.
Thanks for that, I'll have a look at it to ensure it works as it will be better to rely on generated code.
Are we using a fork of piltover or the original contract?
There was a problem hiding this comment.
I'm using this one, https://github.com/matzayonc/piltover/tree/feat/fact-registry.
| to: c.to, | ||
| selector: c.selector, | ||
| calldata: c.calldata, | ||
| starknet_messages: Vec::new(), // TODO: Fill messages. |
There was a problem hiding this comment.
This should be done in a subsequent PR, or could be done in this one? The messages are or L1HandlerTransaction or available into the receipts.
There was a problem hiding this comment.
Should be trivial to implement, but might be less so to test it, that why I left it for now to avoid a false positive.
This PR is quite big already so I would leave it for the following out, unless you think otherwise.
There was a problem hiding this comment.
Sounds great in a follow up PR. 👍
| cairo_version: Felt, | ||
| account: &SayaStarknetAccount, | ||
| ) -> anyhow::Result<(String, Felt)> { | ||
| if serialized_proof.len() > 2000 { |
There was a problem hiding this comment.
I exprienced that some transactions failed when greater than 3000, but can test further now that it's in a more mature state.
| let mut nonce = account.get_nonce().await?; | ||
| let mut hashes = Vec::new(); | ||
|
|
||
| for fragment in serialized_proof.into_iter().chunks(2000).into_iter() { |
There was a problem hiding this comment.
If a fragment upload fail, do we have a way that saya can make a call on the chain to resume from where it was?
There was a problem hiding this comment.
Not at the moment, but should be pretty straight forward to implement.
There was a problem hiding this comment.
Same then let's keep that in the track for a follow up PR to be implemented to ensure more reliable settlement.
3580386 to
5841d9b
Compare
9cf9984 to
d861f1f
Compare
f405916 to
3513cfe
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2253 +/- ##
==========================================
+ Coverage 67.73% 68.54% +0.80%
==========================================
Files 388 387 -1
Lines 50421 49902 -519
==========================================
+ Hits 34153 34203 +50
+ Misses 16268 15699 -569 ☔ View full report in Codecov by Sentry. |
Co-authored-by: glihm <dev@glihm.net>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
crates/saya/core/src/lib.rs (4)
14-14: Ohayo, sensei! The new imports look great!The additions from
itertools,prover, andstarknetcrates align well with the new functionality. The public re-export ofProverAccessKeyis a nice touch for API usability.Consider grouping related imports together for better readability. For example, you could group all
starknet-related imports.Also applies to: 19-22, 26-32
60-63: Ohayo, sensei! TheSayaConfigstruct has evolved nicely!The new
modeandsettlement_contractfields, along with the more flexibleblock_range, align well with the PR objectives. These changes enhance the configurability of the Saya system.Consider adding documentation comments for the new fields to explain their purpose and expected values. This will help users understand how to configure Saya correctly.
93-96: Ohayo, sensei! TheSayastruct has gained some new superpowers!The addition of
prover_identifier,prev_commitment, andprev_heightfields enhances Saya's capabilities for proving and state tracking. The initialization in thenewmethod looks good.Consider adding comments to explain the purpose of
prev_commitmentandprev_height, as their roles might not be immediately clear to newcomers.Also applies to: 126-139
Line range hint
151-279: Ohayo, sensei! Thestartmethod has become a true master of ceremonies!The refactoring to support different modes of operation and batch processing is impressive. The error handling and logging improvements are also great additions.
However, the method has grown quite complex. Consider breaking it down into smaller, more focused methods to improve readability and maintainability. For example:
- Extract the mode-specific logic into separate methods.
- Create a helper method for block fetching and processing.
- Move the proof generation and processing logic into a separate method.
This refactoring would make the
startmethod easier to understand and maintain.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- crates/saya/core/src/lib.rs (13 hunks)
🧰 Additional context used
🔇 Additional comments (1)
crates/saya/core/src/lib.rs (1)
Line range hint
1-573: Ohayo, sensei! Overall, the changes to the Saya core library are impressive!The introduction of different operational modes, enhanced StarkNet integration, and improved block processing align well with the PR objectives. The code structure and new functionality demonstrate a significant step forward for the Saya system.
However, there are a few areas that could benefit from further attention:
- The
startmethod has become quite complex and could be refactored for better maintainability.- There are some TODOs and commented-out code that need to be addressed.
- The
StarknetAccountDatastruct could be improved for better security.- Some methods could benefit from additional error handling and documentation.
Addressing these points will further enhance the quality and robustness of the codebase. Great work on this substantial update, sensei!
| async fn process_proven( | ||
| &mut self, | ||
| proof: ProverResult, | ||
| state_diff: Vec<Felt>, | ||
| last_block: u64, | ||
| state_roots: (Felt, Felt), | ||
| ) -> SayaResult<()> { | ||
| trace!(target: LOG_TARGET, last_block, "Processing proven blocks."); | ||
|
|
||
| if self.config.store_proofs { | ||
| let filename = format!("proof_{}.json", last_block); | ||
| let mut file = File::create(filename).await.context("Failed to create proof file.")?; | ||
| file.write_all(proof.as_bytes()).await.context("Failed to write proof.")?; | ||
| } | ||
|
|
||
| let serialized_proof: Vec<Felt> = parse(&proof)?.into(); | ||
| let world_da = state_diff.world_da.unwrap(); | ||
|
|
||
| // Publish state difference if DA client is available | ||
| let serialized_proof = proof.serialized_proof; | ||
| // Publish state difference if DA client is available. | ||
| if let Some(da) = &self.da_client { | ||
| trace!(target: LOG_TARGET, last_block, "Publishing DA."); | ||
|
|
||
| if self.config.skip_publishing_proof { | ||
| da.publish_state_diff_felts(&world_da).await?; | ||
| let checkpoint = PublishedStateDiff { | ||
| prev_state_root: state_roots.0, | ||
| state_root: state_roots.1, | ||
| prev_height: self.prev_height, | ||
| prev_commitment: self.prev_commitment, | ||
| proof: serde_json::to_value(&proof.proof).unwrap(), | ||
| }; | ||
| // let ns = Namespace::new_v0(b"saya-dev").unwrap(); | ||
| // let commitment = Commitment::from_blob(ns, 0, &serialized_proof.iter().map(|felt| | ||
| // felt.to_bytes()).collect::<Vec<_>>()); | ||
| let (commitment, height) = if self.config.mode != SayaMode::Ephemeral { | ||
| da.publish_checkpoint(checkpoint).await? | ||
| } else if self.config.skip_publishing_proof { | ||
| da.publish_state_diff_felts(&state_diff).await? | ||
| } else { | ||
| da.publish_state_diff_and_proof_felts(&world_da, &serialized_proof).await?; | ||
| } | ||
| da.publish_state_diff_and_proof_felts(&state_diff, &serialized_proof).await? | ||
| }; | ||
| self.prev_commitment = Some(commitment); | ||
| self.prev_height = Some(height); | ||
|
|
||
| info!(target: LOG_TARGET,"commitment: {:?}, height: {:?}", commitment.0, height); | ||
| } | ||
|
|
||
| let program_hash = proof.program_hash; | ||
| let program_output_hash = proof.program_output_hash; | ||
| let program_output = proof.program_output; | ||
|
|
||
| let program_hash_string = program_hash; | ||
| let program_output_hash_string = program_output_hash; | ||
|
|
||
| info!(target: LOG_TARGET,"Extracted program hash and output hash. {:?} {:?}", program_hash_string, program_output_hash_string); | ||
| let expected_fact = poseidon_hash_many(&[program_hash, program_output_hash]).to_string(); | ||
| let program = program_hash.to_string(); | ||
| info!(target: LOG_TARGET, expected_fact, program, "Expected fact."); | ||
|
|
||
| let starknet_account = self.config.starknet_account.get_starknet_account()?; | ||
|
|
||
| // Verify the proof and register fact. | ||
| trace!(target: LOG_TARGET, last_block, "Verifying block."); | ||
| let (transaction_hash, nonce_after) = verifier::verify( | ||
| let (transaction_hash, _nonce) = verifier::verify( | ||
| VerifierIdentifier::HerodotusStarknetSepolia(self.config.fact_registry_address), | ||
| serialized_proof, | ||
| self.config.starknet_account.clone(), | ||
| &starknet_account, | ||
| self.config.mode.to_program().cairo_version(), | ||
| ) | ||
| .await?; | ||
| info!(target: LOG_TARGET, last_block, transaction_hash, "Block verified."); | ||
|
|
||
| let ExtractProgramResult { program: _, program_hash } = extract_program(&proof)?; | ||
| let ExtractOutputResult { program_output, program_output_hash } = extract_output(&proof)?; | ||
| let expected_fact = poseidon_hash_many(&[program_hash, program_output_hash]).to_string(); | ||
| info!(target: LOG_TARGET, expected_fact, "Expected fact."); | ||
|
|
||
| // When not waiting for couple of second `apply_diffs` will sometimes fail due to reliance | ||
| // on registered fact | ||
| tokio::time::sleep(std::time::Duration::from_secs(2)).await; | ||
|
|
||
| trace!(target: LOG_TARGET, last_block, "Applying diffs."); | ||
| let transaction_hash = dojo_os::starknet_apply_diffs( | ||
| self.config.world_address, | ||
| world_da, | ||
| program_output, | ||
| program_hash, | ||
| nonce_after + Felt::ONE, | ||
| self.config.starknet_account.clone(), | ||
| ) | ||
| .await?; | ||
| info!(target: LOG_TARGET, last_block, transaction_hash, "Diffs applied."); | ||
|
|
||
| // Apply the diffs to the world state. | ||
| match self.config.mode { | ||
| SayaMode::Ephemeral => { | ||
| // Needs checker program to be verified, and set as the upgrade_state authority | ||
| todo!("Ephemeral mode does not support publishing updated state yet."); | ||
| } | ||
| SayaMode::Persistent => { | ||
| let serialized_output = program_output.iter().copied().collect_vec(); | ||
| println!("serialized_output: {:?}", serialized_output); | ||
|
|
||
| // todo!("Persistent mode does not support publishing updated state with SNOS | ||
| // yet."); | ||
|
|
||
| let deduplicated_output = | ||
| serialized_output[1..serialized_output.len() / 2].to_vec(); | ||
| let batcher_output = from_felts::<StarknetOsOutput>(&deduplicated_output).unwrap(); | ||
| let piltover_calldata = PiltoverCalldata { | ||
| program_output: serialized_output, | ||
| // onchain_data_hash: batcher_output.new_state_root, | ||
| onchain_data_hash: batcher_output.new_block_hash, | ||
| onchain_data_size: (Felt::ZERO, Felt::ZERO), | ||
| }; | ||
|
|
||
| let expected_state_root = batcher_output.new_block_hash.to_string(); | ||
| let expected_block_number = | ||
| (batcher_output.new_block_number - Felt::ONE).to_string(); | ||
| info!(target: LOG_TARGET, last_block, expected_state_root, expected_block_number, "Applying snos to piltover."); | ||
|
|
||
| starknet_apply_piltover( | ||
| piltover_calldata, | ||
| self.config.settlement_contract, | ||
| &starknet_account, | ||
| ) | ||
| .await?; | ||
| } | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei! The process_proven method is a true proof master!
The implementation for publishing state differences and verifying proofs looks solid. Great job on supporting different modes of operation!
However, there are a few areas that need attention:
- There's a TODO comment for the Ephemeral mode (line 491). This needs to be implemented.
- There's a commented-out line for
onchain_data_hash(line 505). Decide whether to use this or remove it. - The error handling for
from_felts(line 502) usesunwrap(), which could lead to panics. Consider using?or proper error handling.
Please address these issues to ensure the robustness of the process_proven method.
| #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] | ||
| pub struct StarknetAccountData { | ||
| pub starknet_url: Url, | ||
| #[serde(deserialize_with = "felt_string_deserializer")] | ||
| pub chain_id: Felt, | ||
| pub signer_address: Felt, | ||
| pub signer_key: Felt, | ||
| } | ||
|
|
||
| impl StarknetAccountData { | ||
| pub fn get_starknet_account(&self) -> anyhow::Result<SayaStarknetAccount> { | ||
| let provider = JsonRpcClient::new(HttpTransport::new(self.starknet_url.clone())); | ||
| let signer = LocalWallet::from(SigningKey::from_secret_scalar(self.signer_key)); | ||
|
|
||
| let mut account = SingleOwnerAccount::new( | ||
| provider, | ||
| signer, | ||
| self.signer_address, | ||
| self.chain_id, | ||
| ExecutionEncoding::New, | ||
| ); | ||
|
|
||
| account.set_block_id(BlockId::Tag(BlockTag::Pending)); | ||
| Ok(account) | ||
| } | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei! The StarknetAccountData struct is a nice addition, but let's talk security!
The implementation of StarknetAccountData and its get_starknet_account method improves the handling of StarkNet account information. However, there's a potential security concern:
Storing the signer_key directly in the struct might expose sensitive information. Consider using a more secure method for handling the signer key, such as:
- Using a separate, encrypted storage for the key.
- Implementing a method to securely retrieve the key when needed, possibly requiring user input for decryption.
Here's a conceptual example:
pub struct StarknetAccountData {
pub starknet_url: Url,
#[serde(deserialize_with = "felt_string_deserializer")]
pub chain_id: Felt,
pub signer_address: Felt,
// Remove the signer_key field
}
impl StarknetAccountData {
pub fn get_starknet_account(&self, key_provider: &dyn SecureKeyProvider) -> anyhow::Result<SayaStarknetAccount> {
let provider = JsonRpcClient::new(HttpTransport::new(self.starknet_url.clone()));
let signer_key = key_provider.get_signer_key()?; // Securely retrieve the key
let signer = LocalWallet::from(SigningKey::from_secret_scalar(signer_key));
// ... rest of the method ...
}
}This approach would enhance the security of the sensitive signer key.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
bin/saya/src/args/mod.rs (4)
49-50: Ohayo! TheSayaArgsstruct is leveling up, sensei!The new
end_blockfield and thesettlementoptions are great additions that provide more flexibility in controlling block processing and settlement. The conflict betweenbatch_sizeandend_blockis a smart move to prevent inconsistencies.One small suggestion:
Consider adding a comment explaining why
batch_sizeandend_blockare mutually exclusive. This will help future developers understand the reasoning behind this constraint./// The number of blocks to be merged into a single proof. /// This option conflicts with `end_block` to ensure consistent batch processing. #[arg(conflicts_with = "end_block")] pub batch_size: usize,Also applies to: 54-60
100-193: Ohayo, sensei! Thetry_fromimplementation is looking sharp!The new logic for handling
DataAvailabilityConfig, private keys, and settlement options is well-implemented. The validation forbatch_sizein persistent mode is a crucial addition to ensure correct behavior.However, I have a small suggestion to improve error handling consistency:
Consider using a custom error type instead of
Box<dyn std::error::Error>for more specific error handling. This would make it easier to handle different error cases and provide more informative error messages to users.For example:
#[derive(thiserror::Error, Debug)] enum SayaConfigError { #[error("Celestia config: {0}")] CelestiaConfig(String), #[error("Invalid input: {0}")] InvalidInput(String), // Add more error variants as needed } impl TryFrom<SayaArgs> for SayaConfig { type Error = SayaConfigError; fn try_from(args: SayaArgs) -> Result<Self, Self::Error> { // Use the custom error type in your implementation // For example: // Err(SayaConfigError::CelestiaConfig("Node url is required".to_string())) } }This approach would make error handling more robust and easier to maintain as the codebase grows.
Line range hint
199-297: Ohayo! The test case is looking good, sensei!The updates to the test case accurately reflect the changes in the structs and implementation. Good job on maintaining test coverage!
To make your tests even more robust, consider adding these additional test cases:
- Test the validation of
batch_sizein persistent mode:#[test] fn test_batch_size_validation_in_persistent_mode() { let args = SayaArgs { // ... other fields ... batch_size: 2, settlement: SettlementOptions { saya_mode: settlement::SayaModeArg(SayaMode::Persistent), // ... other fields ... }, // ... other fields ... }; let result: Result<SayaConfig, _> = args.try_into(); assert!(result.is_err()); assert!(result.unwrap_err().to_string().contains("Batch size must be 1 for persistent mode")); }
- Test the mutual exclusivity of
batch_sizeandend_block:#[test] fn test_batch_size_and_end_block_conflict() { let args = SayaArgs { // ... other fields ... batch_size: 10, end_block: Some(100), // ... other fields ... }; let result: Result<SayaConfig, _> = args.try_into(); assert!(result.is_err()); assert!(result.unwrap_err().to_string().contains("The argument '--batch-size <BATCH_SIZE>' cannot be used with '--end-block <END_BLOCK>'")); }These additional tests will help ensure that the new constraints and validations are working as expected.
169-177: Ohayo, sensei! Thesettlement_contractvalidation is on point!The addition of the
settlement_contractfield and its validation for persistent mode is crucial. Great job on ensuring this important requirement is met!To make the error message even more helpful:
Consider updating the error message to be more specific about the requirement:
return Err(SayaConfigError::InvalidInput( "A `settlement_contract` must be specified when using persistent mode.".to_string() ));This more detailed message will help users understand exactly what they need to provide when using persistent mode.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- bin/saya/src/args/mod.rs (6 hunks)
🧰 Additional context used
🔇 Additional comments (2)
bin/saya/src/args/mod.rs (2)
6-7: Ohayo, sensei! The new imports and module look good!The addition of
SayaModeandSettlementOptionsimports, along with the newsettlementmodule, aligns well with the introduction of settlement-related functionality. Nice work on keeping the code structure organized!Also applies to: 21-21
Line range hint
1-297: Ohayo, sensei! Your code is evolving beautifully!Overall, the changes to this file significantly enhance the Saya configuration process. The new settlement options, block range controls, and data availability configurations provide great flexibility for users. The added validations, especially for the persistent mode and
settlement_contract, help prevent misconfigurations.Here's a summary of the improvements:
- Enhanced
SayaArgsstruct with new options- Improved
try_fromimplementation with robust error checking- Updated test cases to cover new functionality
To take your code to the next level, consider:
- Implementing a custom error type for more specific error handling
- Adding a few more test cases to cover edge cases
- Improving some error messages for clarity
Great work on evolving the Saya tool, sensei! These changes will definitely improve the user experience and make the tool more powerful.
| #[arg(short, long)] | ||
| pub end_block: Option<u64>, |
There was a problem hiding this comment.
Ohayo, sensei! Let's add a quick check for end_block!
The addition of the end_block field is a great improvement for flexibility. However, to prevent potential issues, we should ensure that end_block is greater than or equal to start_block when it's provided.
Consider adding this validation in the try_from implementation:
if let Some(end_block) = args.end_block {
if end_block < args.start_block {
return Err(SayaConfigError::InvalidInput(
"`end_block` must be greater than or equal to `start_block`".to_string()
));
}
}This check will help prevent invalid block ranges and potential runtime errors.
Also applies to: 184-184
5 participants
Description
I added a Cairo 1 program simulating SNOS, committing to piltover.
Related issue
Tests
The full development flow is in this script:
bin/saya/saya.sh.Added to documentation?
Checklist
scripts/prettier.sh,scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
New Features
Bug Fixes
world_address, enhancing deployment instructions.Documentation
Chores