-
Notifications
You must be signed in to change notification settings - Fork 70
Sync Prover mainnet fix #821
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
Changes from all commits
b6c25d3
59d5d2b
7512ac5
06de3bc
a961225
24ad89d
258b594
c068559
d5c62ec
99635dc
1b6b854
1f141a3
0a0d64c
a7a0528
82ddc8e
971dec8
99425da
6236577
aa9c876
9303110
44f37d1
95aa8e9
437e350
580485c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,4 +24,6 @@ shadow-proving | |
| contracts/cache | ||
| contracts | ||
| out | ||
| lib | ||
| lib | ||
|
|
||
| proof | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,6 @@ use eyre::anyhow; | |
| use serde::{Deserialize, Serialize}; | ||
| use std::env::var; | ||
| use std::error::Error; | ||
| use std::ops::Mul; | ||
| use std::str::FromStr; | ||
| use std::sync::Arc; | ||
| use std::time::Duration; | ||
|
|
@@ -175,6 +174,7 @@ impl ChallengeHandler { | |
| if !batch_proof.proof_data.is_empty() { | ||
| log::info!("query proof and prove state: {:#?}", batch_index); | ||
| let batch_header = batch_info.fill_ext(batch_proof.batch_header.clone()).encode(); | ||
| sleep(Duration::from_secs(600)).await; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# First, let's examine the code around line 177 in handler.rs with context
echo "=== Code around line 177 ==="
sed -n '160,195p' prover/bin/challenge/src/handler.rsRepository: morph-l2/morph Length of output: 1721 🏁 Script executed: #!/bin/bash
# Check recent commits affecting this file
echo "=== Recent commits for handler.rs ==="
git log --oneline -20 -- prover/bin/challenge/src/handler.rs
echo -e "\n=== Detailed commit info for the sleep addition ==="
# Try to find the commit that added this sleep
git log -p --all -S 'Duration::from_secs(600)' -- prover/bin/challenge/src/handler.rs | head -100Repository: morph-l2/morph Length of output: 177 🏁 Script executed: #!/bin/bash
# Get full commit details
echo "=== Full commit details ==="
git show 6236577
echo -e "\n=== Check for PR related to this commit ==="
# Try to find associated PR
git log --all --grep="Execute before prove" --oneline
git log --all --grep="sleep" --oneline | head -10Repository: morph-l2/morph Length of output: 50443 🏁 Script executed: #!/bin/bash
# Search for comments about timing, proof, or delays near the sleep
echo "=== Searching for related comments or timing logic ==="
rg -n -B5 -A5 "query_proof|prove_state|proof_data" prover/bin/challenge/src/handler.rs | head -80
echo -e "\n=== Searching for any TODO or FIXME comments in handler.rs ==="
rg -n "TODO|FIXME|HACK|XXX" prover/bin/challenge/src/handler.rs
echo -e "\n=== Looking for other timing/delay related code ==="
rg -n "Duration|sleep|delay" prover/bin/challenge/src/handler.rsRepository: morph-l2/morph Length of output: 4746 🏁 Script executed: #!/bin/bash
# Check git history to see if this sleep was added in this commit
echo "=== Check git log with -p limited output for the sleep ==="
git log --all -p -- prover/bin/challenge/src/handler.rs | grep -A10 -B10 "Duration::from_secs(600)" | head -50
echo -e "\n=== Check line 110 context ==="
sed -n '105,115p' prover/bin/challenge/src/handler.rs
echo -e "\n=== Check the full context around both prove_state calls ==="
sed -n '195,210p' prover/bin/challenge/src/handler.rsRepository: morph-l2/morph Length of output: 2785 🏁 Script executed: #!/bin/bash
# Let me verify the full extent of the inconsistency
echo "=== Full comparison of both prove_state call paths ==="
echo "PATH 1 (line 173-179): Early query finds proof"
sed -n '173,179p' prover/bin/challenge/src/handler.rs
echo -e "\nPATH 2 (line 200-205): Status PROVED finds proof"
sed -n '200,205p' prover/bin/challenge/src/handler.rs
echo -e "\n=== Checking if there's any documentation about L1 finality timing ==="
rg -n "finality|L1|confirmation" prover/bin/challenge/src/handler.rs | head -20
echo -e "\n=== Check challenge deadline or timing constants ==="
rg -n "CHALLENGE|challenge" prover/bin/challenge/src/handler.rs | head -20Repository: morph-l2/morph Length of output: 3523 Based on my thorough verification, I have found critical evidence that confirms and extends the original review concern: Key findings:
Rewritten Review Comment: Resolve the inconsistent and unexplained 600-second delay before proving state. The 600-second (10-minute) blocking sleep at line 177 is newly added but appears only in one of two identical code paths that prove state with proof data:
Both paths perform identical operations on proof data, yet behave differently. This inconsistency suggests either:
The hardcoded 10-minute delay blocks the entire event loop and could impact challenge responsiveness, especially given the code references a challenge period window. Without explanation or configurability, this is difficult to maintain and verify. Required actions:
🤖 Prompt for AI Agents |
||
| self.prove_state(batch_index, batch_header, batch_proof, l1_rollup).await; | ||
| continue; | ||
| } | ||
|
|
@@ -462,6 +462,8 @@ async fn batch_inspect(l1_rollup: &RollupType, l1_provider: &Provider<Http>, bat | |
| let withdrawal_root: [u8; 32] = param.batch_data_input.withdrawal_root; | ||
| let last_block_number: u64 = param.batch_data_input.last_block_number; | ||
| let num_l1_messages = param.batch_data_input.num_l1_messages; | ||
| log::info!("======> batch inspect: decode tx.input, version = {:#?}", version); | ||
| log::info!("======> batch inspect: decode tx.input, param = {:#?}", param); | ||
|
|
||
| let mut batch_info = BatchInfo { | ||
| version, | ||
|
|
@@ -574,4 +576,4 @@ pub fn contract_error<M: Middleware>(e: ContractError<M>) -> String { | |
| format!("error: {:?}", e) | ||
| }; | ||
| error_msg | ||
| } | ||
| } | ||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -86,7 +86,10 @@ impl Prover { | |
| if read_env_var("SAVE_TRACE", false) { | ||
| save_trace(batch_index, block_traces); | ||
| } | ||
| save_batch_header(block_traces, batch_index); | ||
| if !save_batch_header(block_traces, batch_index) { | ||
| save_trace(batch_index, block_traces); | ||
| continue; | ||
| } | ||
|
|
||
| // Step3. Generate evm proof | ||
| log::info!("Generate evm proof"); | ||
|
|
@@ -113,24 +116,40 @@ impl Prover { | |
| } | ||
| } | ||
|
|
||
| fn save_batch_header(blocks: &mut Vec<BlockTrace>, batch_index: u64) { | ||
| blocks.iter_mut().for_each(|blobk| blobk.flatten()); | ||
| let batch_info = EVMVerifier::verify(blocks).unwrap(); | ||
| let blob_info = morph_executor_host::get_blob_info(blocks).unwrap(); | ||
| let (versioned_hash, _) = BlobVerifier::verify(&blob_info, blocks.len()).unwrap(); | ||
|
|
||
| // Save batch_header | ||
| // | batch_data_hash | versioned_hash | sequencer_root | | ||
| // |-----------------|----------------|----------------| | ||
| // | bytes32 | bytes32 | bytes32 | | ||
| let mut batch_header: Vec<u8> = Vec::with_capacity(96); | ||
| batch_header.extend_from_slice(&batch_info.data_hash().0); | ||
| batch_header.extend_from_slice(&versioned_hash.0); | ||
| batch_header.extend_from_slice(&batch_info.sequencer_root().0); | ||
| fn save_batch_header(blocks: &mut Vec<BlockTrace>, batch_index: u64) -> bool { | ||
| let proof_dir = PROVER_PROOF_DIR.to_string() + format!("/batch_{}", batch_index).as_str(); | ||
| std::fs::create_dir_all(&proof_dir).expect("failed to create proof path"); | ||
| let mut batch_file = File::create(format!("{}/batch_header.data", proof_dir)).unwrap(); | ||
| batch_file.write_all(&batch_header[..]).expect("failed to batch_header"); | ||
| blocks.iter_mut().for_each(|block| block.flatten()); | ||
| let verify_result = EVMVerifier::verify(blocks); | ||
|
|
||
| if let Ok(batch_info) = verify_result { | ||
| let blob_info = morph_executor_host::get_blob_info(blocks).unwrap(); | ||
| let (versioned_hash, _) = BlobVerifier::verify(&blob_info, blocks.len()).unwrap(); | ||
|
|
||
| // Save batch_header | ||
| // | batch_data_hash | versioned_hash | sequencer_root | | ||
| // |-----------------|----------------|----------------| | ||
| // | bytes32 | bytes32 | bytes32 | | ||
| let mut batch_header: Vec<u8> = Vec::with_capacity(96); | ||
| batch_header.extend_from_slice(&batch_info.data_hash().0); | ||
| batch_header.extend_from_slice(&versioned_hash.0); | ||
| batch_header.extend_from_slice(&batch_info.sequencer_root().0); | ||
| let mut batch_file = File::create(format!("{}/batch_header.data", proof_dir)).unwrap(); | ||
| batch_file.write_all(&batch_header[..]).expect("failed to batch_header"); | ||
| true | ||
|
Comment on lines
+125
to
+139
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replace panicking Lines 126, 127, and 137 use Consider returning -fn save_batch_header(blocks: &mut Vec<BlockTrace>, batch_index: u64) -> bool {
+fn save_batch_header(blocks: &mut Vec<BlockTrace>, batch_index: u64) -> Result<bool, anyhow::Error> {
let proof_dir = PROVER_PROOF_DIR.to_string() + format!("/batch_{}", batch_index).as_str();
std::fs::create_dir_all(&proof_dir).expect("failed to create proof path");
blocks.iter_mut().for_each(|block| block.flatten());
let verify_result = EVMVerifier::verify(blocks);
if let Ok(batch_info) = verify_result {
- let blob_info = morph_executor_host::get_blob_info(blocks).unwrap();
- let (versioned_hash, _) = BlobVerifier::verify(&blob_info, blocks.len()).unwrap();
+ let blob_info = morph_executor_host::get_blob_info(blocks)?;
+ let (versioned_hash, _) = BlobVerifier::verify(&blob_info, blocks.len())?;
let mut batch_header: Vec<u8> = Vec::with_capacity(96);
batch_header.extend_from_slice(&batch_info.data_hash().0);
batch_header.extend_from_slice(&versioned_hash.0);
batch_header.extend_from_slice(&batch_info.sequencer_root().0);
- let mut batch_file = File::create(format!("{}/batch_header.data", proof_dir)).unwrap();
- batch_file.write_all(&batch_header[..]).expect("failed to batch_header");
- true
+ let mut batch_file = File::create(format!("{}/batch_header.data", proof_dir))?;
+ batch_file.write_all(&batch_header[..])?;
+ Ok(true)
} else {
// ... error handling ...
- false
+ Ok(false)
}
}Then update the caller at line 89 to handle the
🤖 Prompt for AI Agents |
||
| } else { | ||
| let e = verify_result.unwrap_err(); | ||
| let error_data = serde_json::json!({ | ||
| "error_code": "EVM_EXECUTE_NOT_EXPECTED", | ||
| "error_msg": e.to_string() | ||
| }); | ||
| let mut batch_file = File::create(format!("{}/execute_result.json", proof_dir)).unwrap(); | ||
| batch_file | ||
| .write_all(serde_json::to_string_pretty(&error_data).unwrap().as_bytes()) | ||
| .expect("failed to write error"); | ||
| log::error!("EVM verification failed for batch {}: {}", batch_index, e); | ||
| false | ||
| } | ||
|
Comment on lines
+140
to
+152
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Error path can still panic—defeats graceful error handling. The error path writes Convert these to return } else {
let e = verify_result.unwrap_err();
let error_data = serde_json::json!({
"error_code": "EVM_EXECUTE_NOT_EXPECTED",
"error_msg": e.to_string()
});
- let mut batch_file = File::create(format!("{}/execute_result.json", proof_dir)).unwrap();
- batch_file
- .write_all(serde_json::to_string_pretty(&error_data).unwrap().as_bytes())
- .expect("failed to write error");
- log::error!("EVM verification failed for batch {}: {}", batch_index, e);
+ match File::create(format!("{}/execute_result.json", proof_dir))
+ .and_then(|mut f| f.write_all(serde_json::to_string_pretty(&error_data)?.as_bytes()))
+ {
+ Ok(_) => log::error!("EVM verification failed for batch {}: {}", batch_index, e),
+ Err(write_err) => log::error!("EVM verification failed for batch {} ({}), and failed to write execute_result.json: {}", batch_index, e, write_err),
+ }
false
}
|
||
| } | ||
|
|
||
| fn save_proof(batch_index: u64, proof: EvmProofFixture) { | ||
|
|
@@ -191,3 +210,19 @@ fn save_trace(batch_index: u64, chunk_traces: &Vec<BlockTrace>) { | |
| serde_json::to_writer_pretty(writer, &chunk_traces).unwrap(); | ||
| log::info!("chunk_traces of batch_index = {:#?} saved", batch_index); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_save_execute() { | ||
| let batch_index = 102u64; | ||
|
|
||
| let mut blocks = load_trace("../../testdata/viridian/eip7702_traces.json"); | ||
| println!("blocks.len(): {:?}", blocks.len()); | ||
| let traces = blocks.first_mut().unwrap(); | ||
|
|
||
| if !save_batch_header(traces, batch_index) { | ||
| save_trace(batch_index, traces); | ||
| println!("save_batch_header error"); | ||
| } else { | ||
| println!("save_batch_header success"); | ||
| } | ||
| } | ||
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reconsider logging full external sign response at
infolevelrtholds the full JSON response from the external signing service, including signatures and transaction-related data. Logging this atinfoon every successful call can:Unless you explicitly want this in normal runtime logs, consider either:
or gating it behind a feature flag / config so it can be enabled only during targeted debugging.
📝 Committable suggestion