STR-799: Upgrade CL guest code to prove range of N blocks #563
+480
−265
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
13 tasks
MdTeach
force-pushed
the
STR-702-ol-prove-range-2
branch
from
befae62 to
5509e46
Compare
MdTeach
force-pushed
the
STR-702-ol-prove-range-2
branch
from
5509e46 to
871160f
Compare
delbonis
requested changes
Dec 26, 2024
There was a problem hiding this comment.
Looks good, main issue is that error handling should be a little more deliberate instead of panicking if things seem wrong. I know a lot of this was written with .expect in places but since we're in here making changes to related code we have good reason to clean it up a bit more.
MdTeach
force-pushed
the
STR-702-ol-prove-range-2
branch
from
871160f to
f75932e
Compare
delbonis
requested changes
Dec 30, 2024
MdTeach
force-pushed
the
STR-702-ol-prove-range-2
branch
from
f842bf3 to
391f71c
Compare
MdTeach
force-pushed
the
STR-702-ol-prove-range-2
branch
from
391f71c to
1a74eaa
Compare
MdTeach
force-pushed
the
STR-702-ol-prove-range-2
branch
from
1a74eaa to
ad3a19b
Compare
prajwolrg
requested changes
Dec 31, 2024
Comment on lines
+64
to
+88
| /// Retrieves the [`L2BlockId`] for the given `block_num` | ||
| pub async fn get_l2id(&self, block_num: u64) -> Result<L2BlockId, ProvingTaskError> { | ||
| let l2_headers = self | ||
| .cl_client | ||
| .get_headers_at_idx(block_num) | ||
| .await | ||
| .inspect_err(|_| error!(%block_num, "Failed to fetch l2_headers")) | ||
| .map_err(|e| ProvingTaskError::RpcError(e.to_string()))?; | ||
|
|
||
| let headers = l2_headers.ok_or_else(|| { | ||
| error!(%block_num, "Failed to fetch L2 block"); | ||
| ProvingTaskError::InvalidWitness(format!("Invalid L2 block height {}", block_num)) | ||
| })?; | ||
|
|
||
| let first_header: Buf32 = headers | ||
| .first() | ||
| .ok_or_else(|| { | ||
| ProvingTaskError::InvalidWitness(format!("Invalid L2 block height {}", block_num)) | ||
| })? | ||
| .block_id | ||
| .into(); | ||
|
|
||
| Ok(first_header.into()) | ||
| } | ||
|
|
There was a problem hiding this comment.
This should be part of CL Operator.
| .context(); | ||
|
|
||
| // Doing the manual block idx to id transformation. Will be removed once checkpoint_info | ||
| // include the range interms of block_id. |
Comment on lines
+1
to
+6
| /// Maximum number of blocks allowed in a proving range. | ||
| /// | ||
| /// This constant serves as a safety limit to prevent proving tasks from processing | ||
| /// an excessively large number of blocks. If the number of blocks to prove exceeds | ||
| /// this limit, the task will be aborted early. | ||
| pub const MAX_PROVING_BLOCK_RANGE: u64 = 1024; |
There was a problem hiding this comment.
Does it make sense it to be passed as an argument? I don't have a strong opinion on this. Also let's add a rationale for using 1024 instead of other number.
| None => return Ok(None), | ||
| }; | ||
|
|
||
| async fn get_cl_block_witness_raw(&self, blkid: L2BlockId) -> RpcResult<Option<Vec<u8>>> { |
There was a problem hiding this comment.
I think this should not return an option. It might make sense to return Error when the information is not found.
Comment on lines
44
to
+47
| async fn get_exec_update_by_id(&self, block_id: L2BlockId) -> RpcResult<Option<RpcExecUpdate>>; | ||
|
|
||
| #[method(name = "getCLBlockWitness")] | ||
| async fn get_cl_block_witness_raw(&self, index: u64) -> RpcResult<Option<Vec<u8>>>; | ||
| async fn get_cl_block_witness_raw(&self, block_id: L2BlockId) -> RpcResult<Option<Vec<u8>>>; |
There was a problem hiding this comment.
Same as above. I think this should return error if not found instead of None.
Comment on lines
+16
to
17
| strata-primitives = { workspace = true } | ||
| strata-rpc-types = { workspace = true } |
| @@ -52,7 +52,7 @@ impl<H: ZkVmHost> ProofGenerator for CheckpointProofGenerator<H> { | |||
|
|
|||
| let l2_batch_proof = self | |||
| .l2_batch_prover | |||
| .get_proof(&(l2_start_height, l2_end_height)) | |||
| .get_proof(&vec![(l2_start_height, l2_end_height)]) | |||
There was a problem hiding this comment.
If we are passing reference, we might not need vec.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Type of Change
Notes to Reviewers
Checklist
Related Issues