draft: measure SP1 checkpoint capacity for 2048 manifests#1930
draft: measure SP1 checkpoint capacity for 2048 manifests#1930krsnapaudel wants to merge 1 commit into
Conversation
krsnapaudel
requested review from
alexhui01,
bewakes,
delbonis,
evgenyzdanovich,
irnb,
prajwolrg,
purusang,
storopoli and
voidash
as code owners
krsnapaudel
added
the
DO NOT MERGE
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78d324e4af
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Commit: de83875 SP1 Execution Results
|
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #1930 +/- ##
==========================================
- Coverage 84.47% 84.12% -0.35%
==========================================
Files 645 645
Lines 77591 78051 +460
==========================================
+ Hits 65543 65659 +116
- Misses 12048 12392 +344
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 9 files with indirect coverage changes 🚀 New features to boost your workflow:
|
delbonis
left a comment
delbonis
left a comment
There was a problem hiding this comment.
Parts of how this works / is configured are not very self-explanatory and should have things renamed.
| class NoServicesEnv(flexitest.EnvConfig): | ||
| """Functional-test env with no long-running services.""" | ||
|
|
||
| def init(self, ectx: flexitest.EnvContext) -> flexitest.LiveEnv: | ||
| return flexitest.LiveEnv({}) | ||
|
|
There was a problem hiding this comment.
Technically this type should be called something like NoServicesEnvConfig, or really EmptyEnvConfig is more idiomatic. It's not an env itself it's the env conifg.
| const CAPACITY_MANIFEST_COUNTS_ENV: &str = "CHECKPOINT_CAPACITY_MANIFEST_COUNTS"; | ||
| const CAPACITY_OL_LOG_TARGET_ENV: &str = "CHECKPOINT_CAPACITY_OL_LOG_TARGET"; |
There was a problem hiding this comment.
This envvar naming is very confusing, I have no idea what they are supposed to represent. Should these maybe be
CHECKPOINT_MANIFEST_CAPACITY_somethingCHECKPOINT_TARGET_OL_LOG_CAPACITY
?
| fn candidate_manifest_counts() -> Vec<usize> { | ||
| if let Ok(raw_counts) = env::var(CAPACITY_MANIFEST_COUNTS_ENV) { | ||
| let mut counts: Vec<usize> = raw_counts | ||
| .split(',') | ||
| .map(str::trim) | ||
| .filter(|count| !count.is_empty()) | ||
| .map(|count| { | ||
| count.parse::<usize>().unwrap_or_else(|e| { | ||
| panic!("{CAPACITY_MANIFEST_COUNTS_ENV} contains invalid count {count:?}: {e}") | ||
| }) | ||
| }) | ||
| .collect(); | ||
| assert!( | ||
| !counts.is_empty(), | ||
| "{CAPACITY_MANIFEST_COUNTS_ENV} must include at least one count" | ||
| ); | ||
| counts.retain(|count| *count <= MAX_SEALING_MANIFEST_COUNT as usize); | ||
| assert!( | ||
| !counts.is_empty(), | ||
| "{CAPACITY_MANIFEST_COUNTS_ENV} contains no count <= MAX_SEALING_MANIFEST_COUNT" | ||
| ); | ||
| counts.sort_unstable(); | ||
| counts.dedup(); | ||
| return counts; | ||
| } | ||
|
|
||
| // First block is the parent (genesis); remaining blocks are the proving batch. | ||
| let parent = blocks.remove(0).into_header(); | ||
| let mut candidates = vec![256, 512, 1024, MAX_SEALING_MANIFEST_COUNT as usize]; | ||
| candidates.retain(|count| *count <= MAX_SEALING_MANIFEST_COUNT as usize); | ||
| candidates.sort_unstable(); | ||
| candidates.dedup(); | ||
| candidates | ||
| } | ||
|
|
||
| // Rebuild start_state: execute just the genesis block to get state after genesis. | ||
| let mut start_state = make_genesis_state(); | ||
| let _ = build_empty_chain(&mut start_state, 1, SLOTS_PER_EPOCH) | ||
| .expect("build_empty_chain should succeed"); | ||
| fn capacity_ol_log_target() -> Option<usize> { |
There was a problem hiding this comment.
Similarly, these fn names are really hard to understand.
| .collect() | ||
| } | ||
|
|
||
| fn checkpoint_report_name(manifest_count: usize, ol_log_target: Option<usize>) -> String { |
| fn buf32_from_l1_height(height: L1Height, domain: u8) -> Buf32 { | ||
| let mut bytes = [domain; 32]; | ||
| let height_bytes = height.to_le_bytes(); | ||
| bytes[..height_bytes.len()].copy_from_slice(&height_bytes); | ||
| Buf32::from(bytes) | ||
| } | ||
|
|
There was a problem hiding this comment.
I see a fn like this in checkpoint.rs. Does it make sense to consolidate these so there's a single consistent impl?
| fn prepare_input_with_terminal_manifests_and_ol_log_target( | ||
| manifest_count: usize, | ||
| ol_log_target: Option<usize>, | ||
| ) -> CheckpointProverInput { |
There was a problem hiding this comment.
Surely parts of this fn could be broken apart into more reusable components.
@delbonis This is not meant to be merged. Just experimenting to see manifest count of 2048 runs into any issues. |
2 participants
Description
Measurement-only PR. Do not merge.
This PR triggers a branch-gated SP1 network proof job for
CHECKPOINT_CAPACITY_MANIFEST_COUNTS=2048, repeated 3 times, to evaluate whetherMAX_SEALING_MANIFEST_COUNT=2048has acceptable proof stability/runtime.Also exercises max checkpoint log limit. For deposit log limit, it seems pending ASM logs limit kicks in limiting deposit logs per ASM manifest to 32.
Type of Change
Notes to Reviewers
Is this PR addressing any specification, design doc or external reference document?
If yes, please add relevant links:
Checklist
Related Issues
STR-2750